Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10661039:
added preliminary ShadowDOM tests (Closed)
|
Created:
8 years, 6 months ago by samhop Modified:
8 years, 5 months ago CC:
reviews_dartlang.org, Emily Fortuna Visibility:
Public. |
Descriptionadded preliminary ShadowDOM tests
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=9141
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 2
Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:14: expect(sr, isNotNull); why would this be null? https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:26: sr.nodes.add(new Element.html('<p class="foo"></p>')); be consistent about creating elements using Element.html or new SomeElement() https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:34: nodes in a shadow DOM subtree are not accessible using selectors from use a shorter test case name. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:44: expect(sr, isNotNull); how is this testing that the parent node of a shadow root must be null?
https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:1: #library('ShadowDOMTest'); + copyright notice at the top https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:10: () { style nit: move the '() {' to the previous line. Then only indent the body below +2 https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:38: expect(queryAll('.foo').length, 2); it might be good to validate which elements are there. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:42: () { nit: join with prev line https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:57: within the shadowed fragment. same comment from jacob here. Ideally a 1-line. If it needs more explanation, it's ok to put it in a comment below. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:60: expect(sr.queryAll('.foo').length, 1); similarly to the other test: it would be good to check that this is the actual element you expect (holding a ref to the <p> in div1 or something similar)
Sorry for the messy first round :). Second coming soon... https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:1: #library('ShadowDOMTest'); On 2012/06/25 22:16:00, sigmund wrote: > + copyright notice at the top Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:10: () { On 2012/06/25 22:16:00, sigmund wrote: > style nit: move the '() {' to the previous line. Then only indent the body below > +2 Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:14: expect(sr, isNotNull); On 2012/06/25 21:53:11, Jacob wrote: > why would this be null? At one point I thought that was the failure behavior of the ShadowRoot constructor if --enable-shadow-dom is not set, but I was wrong. Fixed. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:26: sr.nodes.add(new Element.html('<p class="foo"></p>')); On 2012/06/25 21:53:11, Jacob wrote: > be consistent about creating elements using > Element.html > or new SomeElement() Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:34: nodes in a shadow DOM subtree are not accessible using selectors from On 2012/06/25 21:53:11, Jacob wrote: > use a shorter test case name. Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:38: expect(queryAll('.foo').length, 2); On 2012/06/25 22:16:00, sigmund wrote: > it might be good to validate which elements are there. Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:42: () { On 2012/06/25 22:16:00, sigmund wrote: > nit: join with prev line Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:44: expect(sr, isNotNull); How embarrassing :p. Fixed. On 2012/06/25 21:53:11, Jacob wrote: > how is this testing that the parent node of a shadow root must be null? https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:57: within the shadowed fragment. On 2012/06/25 22:16:00, sigmund wrote: > same comment from jacob here. Ideally a 1-line. If it needs more explanation, > it's ok to put it in a comment below. Done. https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... tests/html/shadow_dom_test.dart:60: expect(sr.queryAll('.foo').length, 1); On 2012/06/25 22:16:00, sigmund wrote: > similarly to the other test: it would be good to check that this is the actual > element you expect (holding a ref to the <p> in div1 or something similar) Done.
Sam: can you do another 'git cl upload', so we can see the latest with your changes? On 2012/06/26 00:19:15, samhop wrote: > Sorry for the messy first round :). Second coming soon... > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > File tests/html/shadow_dom_test.dart (right): > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:1: #library('ShadowDOMTest'); > On 2012/06/25 22:16:00, sigmund wrote: > > + copyright notice at the top > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:10: () { > On 2012/06/25 22:16:00, sigmund wrote: > > style nit: move the '() {' to the previous line. Then only indent the body > below > > +2 > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:14: expect(sr, isNotNull); > On 2012/06/25 21:53:11, Jacob wrote: > > why would this be null? > At one point I thought that was the failure behavior of the ShadowRoot > constructor if --enable-shadow-dom is not set, but I was wrong. Fixed. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:26: sr.nodes.add(new Element.html('<p > class="foo"></p>')); > On 2012/06/25 21:53:11, Jacob wrote: > > be consistent about creating elements using > > Element.html > > or new SomeElement() > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:34: nodes in a shadow DOM subtree are not > accessible using selectors from > On 2012/06/25 21:53:11, Jacob wrote: > > use a shorter test case name. > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:38: expect(queryAll('.foo').length, 2); > On 2012/06/25 22:16:00, sigmund wrote: > > it might be good to validate which elements are there. > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:42: () { > On 2012/06/25 22:16:00, sigmund wrote: > > nit: join with prev line > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:44: expect(sr, isNotNull); > How embarrassing :p. Fixed. > > On 2012/06/25 21:53:11, Jacob wrote: > > how is this testing that the parent node of a shadow root must be null? > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:57: within the shadowed fragment. > On 2012/06/25 22:16:00, sigmund wrote: > > same comment from jacob here. Ideally a 1-line. If it needs more explanation, > > it's ok to put it in a comment below. > > Done. > > https://chromiumcodereview.appspot.com/10661039/diff/1/tests/html/shadow_dom_... > tests/html/shadow_dom_test.dart:60: expect(sr.queryAll('.foo').length, 1); > On 2012/06/25 22:16:00, sigmund wrote: > > similarly to the other test: it would be good to check that this is the actual > > element you expect (holding a ref to the <p> in div1 or something similar) > > Done.
+efortuna as CC
https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:18: para1 = new ParagraphElement(); avoid variable names with abbreviations such as para and sr div is sufficiently common that it is fine but sr should be shadowRoot or shadow. https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:21: p.classes.add("foo"); odd spacing. Also, might as well just use for (var p in [para1, para2]) { p.classes.add("foo"); }
https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/html.status File tests/html/html.status (right): https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/html.sta... tests/html/html.status:28: shadow_dom_test: Skip # No ShadowDOM support except on tip chrome/dartium. since this Skip includes 'chrome' (which is dev channel & not tip build), maybe say only 'tip dartium'? https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:22: }); nit: I prefer here just the 2 lines: para1.classes.add('foo'); para2.classes.add('foo'); (the current way ends up having more code for the same purpose) https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:37: expect(queryAll('.foo'), equals([div1, para2])); << (only +2 indent) https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:40: test("Parent node of a shadow root must be null.", () { nit: we try to consistently prefer ' over " unless your string contains ' in them. (replace here, below, and in some places where you have "foo" or ".foo")
thumbs up on the change for run_selenium.py https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:21: p.classes.add("foo"); On 2012/06/26 18:22:57, Jacob wrote: > odd spacing. > Also, > might as well just use > for (var p in [para1, para2]) { > p.classes.add("foo"); > } Or just [para1, para2].forEach( (p) { p.classes.add("foo");}); if you feel like you need to do it functionally. :-)
PTAL https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/html.status File tests/html/html.status (right): https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/html.sta... tests/html/html.status:28: shadow_dom_test: Skip # No ShadowDOM support except on tip chrome/dartium. On 2012/06/26 18:28:30, sigmund wrote: > since this Skip includes 'chrome' (which is dev channel & not tip build), maybe > say only 'tip dartium'? Done. https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:18: para1 = new ParagraphElement(); fixed by switching to shadowRoot and paragraph1, paragraph2 On 2012/06/26 18:22:57, Jacob wrote: > avoid variable names with abbreviations such as > para > and > sr > > div is sufficiently common that it is fine but > sr should be shadowRoot or shadow. https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:21: p.classes.add("foo"); On 2012/06/26 18:22:57, Jacob wrote: > odd spacing. > Also, > might as well just use > for (var p in [para1, para2]) { > p.classes.add("foo"); > } Thank you for your respect for *proper* programming, Emily :). Fixed. https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:22: }); On 2012/06/26 18:28:30, sigmund wrote: > nit: I prefer here just the 2 lines: > para1.classes.add('foo'); > para2.classes.add('foo'); > > (the current way ends up having more code for the same purpose) Done. https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:37: expect(queryAll('.foo'), equals([div1, para2])); On 2012/06/26 18:28:30, sigmund wrote: > << (only +2 indent) Done. https://chromiumcodereview.appspot.com/10661039/diff/6001/tests/html/shadow_d... tests/html/shadow_dom_test.dart:40: test("Parent node of a shadow root must be null.", () { On 2012/06/26 18:28:30, sigmund wrote: > nit: we try to consistently prefer ' over " unless your string contains ' in > them. (replace here, below, and in some places where you have "foo" or ".foo") Done.
LGTM! (with little comment below :)) https://chromiumcodereview.appspot.com/10661039/diff/12001/tests/html/shadow_... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/12001/tests/html/shadow_... tests/html/shadow_dom_test.dart:20: [paragraph1, paragraph2].forEach( (p) { p.classes.add('foo');}); remove space between ( (
committed https://chromiumcodereview.appspot.com/10661039/diff/12001/tests/html/shadow_... File tests/html/shadow_dom_test.dart (right): https://chromiumcodereview.appspot.com/10661039/diff/12001/tests/html/shadow_... tests/html/shadow_dom_test.dart:20: [paragraph1, paragraph2].forEach( (p) { p.classes.add('foo');}); On 2012/06/26 23:10:49, sigmund wrote: > remove space between ( ( Done.
committed |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
