Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039103004:
Add tests of state changes for :defined() selector (Closed)
DescriptionAdd tests of state changes for :defined() selector
BUG=594918
Committed: https://crrev.com/60be1ee7d7a12f8fc88cdb2da9d12dc677f9e489
Cr-Commit-Position: refs/heads/master@{#398444}
Patch Set 1 #
Total comments: 7
Patch Set 2 : dominicc review #Messages
Total messages: 17 (7 generated)
Description was changed from ========== defined-tests BUG= ========== to ========== Add tests of state changes for :defined() selector BUG=594918 ==========
kojii@chromium.org changed reviewers: + dominicc@chromium.org, rune@opera.com
PTAL. The state change landed, and it just worked with existing code.
dstreetmc@gmail.com changed reviewers: + Dstreetmc@gmail.com
rune@opera.com changed reviewers: + dstreetmc@gmail.com - Dstreetmc@gmail.com
lgtm https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html (right): https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:61: }); Would it make sense to test the two other cases in example 2 in https://dom.spec.whatwg.org/#concept-element-defined as well? The throwing constructor, and the <p is=""> cases?
Thank you for the prompt review. https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html (right): https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:61: }); On 2016/06/07 at 09:39:08, rune wrote: > Would it make sense to test the two other cases in example 2 in https://dom.spec.whatwg.org/#concept-element-defined as well? > > The throwing constructor, and the <p is=""> cases? is="" is not supported yet, it requires quite amount of work, we're planning to work on it a little later. Throwing constructor is interesting, I wasn't aware it's in the example. IIUC in the old spec it should be "undefined" but with https://github.com/whatwg/html/issues/1297 resolved, I think it should be "defined" now. Filed https://github.com/whatwg/html/issues/1396 for clarification. We'll add that when the spec was clarified. We just got an intern to work on upstreaming tests to web-platform-tests, this should be a good one for her to work on.
https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html (right): https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:61: }); On 2016/06/07 10:48:54, kojii wrote: > On 2016/06/07 at 09:39:08, rune wrote: > > Would it make sense to test the two other cases in example 2 in > https://dom.spec.whatwg.org/#concept-element-defined as well? > > > > The throwing constructor, and the <p is=""> cases? > > is="" is not supported yet, it requires quite amount of work, we're planning to > work on it a little later. > > Throwing constructor is interesting, I wasn't aware it's in the example. IIUC in > the old spec it should be "undefined" but with > https://github.com/whatwg/html/issues/1297 resolved, I think it should be > "defined" now. > > Filed https://github.com/whatwg/html/issues/1396 for clarification. > > We'll add that when the spec was clarified. We just got an intern to work on > upstreaming tests to web-platform-tests, this should be a good one for her to > work on. Acknowledged.
https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html (right): https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:40: function test_defined_for_createElement(defined, doc, tagName, description = '') { Nit: Consider tagName -> tag_name if you're going to write svg_element? https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:42: const element = doc.createElement(tagName); Why const and not let?
https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html (right): https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:40: function test_defined_for_createElement(defined, doc, tagName, description = '') { On 2016/06/07 at 12:01:06, dominicc wrote: > Nit: Consider tagName -> tag_name if you're going to write svg_element? Done. testharness.js forcing us to mix styles always confuses me, I wish it was consistent with DOM... https://codereview.chromium.org/2039103004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/selectors/pseudo-class-defined.html:42: const element = doc.createElement(tagName); On 2016/06/07 at 12:01:05, dominicc wrote: > Why const and not let? Changed back to let. Read somewhere that I should use const if I were not re-assigning to the variable, and the change makes it so, but I don't have opinions.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com Link to the patchset: https://codereview.chromium.org/2039103004/#ps20001 (title: "dominicc review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039103004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add tests of state changes for :defined() selector BUG=594918 ========== to ========== Add tests of state changes for :defined() selector BUG=594918 Committed: https://crrev.com/60be1ee7d7a12f8fc88cdb2da9d12dc677f9e489 Cr-Commit-Position: refs/heads/master@{#398444} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/60be1ee7d7a12f8fc88cdb2da9d12dc677f9e489 Cr-Commit-Position: refs/heads/master@{#398444} |
