Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2321363002:
Custom Elements: layout tests for custom built-in elements definition (Closed)
DescriptionCustom Elements: layout tests for custom built-in elements definition
For custom built-in elements:
https://html.spec.whatwg.org/multipage/scripting.html#element-definition
7. If an element interface is provided for the extends option, it is assumed to be a customized built-in element.
7.1. If element interface is valid custom element name, throws NotSupportedError DOMException
https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name
7.2. If element interface is HTMLUnknownElement, throws NotSupportedError DOMException
https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-interface
7.3. localName set to extends value and not the define-name
A -expected.txt file was included since this isn't implemented yet.
BUG=
Committed: https://crrev.com/cf5d797834090f88d671a9cc97659a2011f094c6
Cr-Commit-Position: refs/heads/master@{#418762}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Changes made according to comments #Patch Set 3 : Removed comment #Patch Set 4 : New file for custom built-in element definition #Patch Set 5 : New file for custom built-in element definition #
Total comments: 3
Patch Set 6 : New file for custom built-in element definition #
Messages
Total messages: 20 (6 generated)
Description was changed from ========== Custom Elements: layout tests for custom built-in elements definition For custom built-in elements: https://html.spec.whatwg.org/multipage/scripting.html#element-definition 7. The extends value must be specified and cannot be null, but the spec didn't say what kind of error should be thrown in this case. 7.1. Extends value cannot be valid custom element name, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name 7.2. Extends value must cannot be HTMLUnknownElement, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-i... 7.3. localName set to extends value and not the define-name A -expected.txt file was included since this isn't implemented yet. BUG= ========== to ========== Custom Elements: layout tests for custom built-in elements definition For custom built-in elements: https://html.spec.whatwg.org/multipage/scripting.html#element-definition 7. The extends value must be specified and cannot be null, but the spec didn't say what kind of error should be thrown in this case. 7.1. Extends value cannot be valid custom element name, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name 7.2. Extends value must cannot be HTMLUnknownElement, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-i... 7.3. localName set to extends value and not the define-name A -expected.txt file was included since this isn't implemented yet. BUG= ==========
yurak@google.com changed reviewers: + dominicc@chromium.org, kojii@chromium.org
I think it's better to have separate test files for extends, and it'd be helpful if file name can indicate it's a test for extends. Can you do it? https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:109: let invalid_values = [ Let's make the variable name more explicit; in this case this is a list of "valid custom element name", correct? https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:125: let invalid_values = [ ditto; such as "element_names_for_HTMLUnknownElement" or such. https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:144: assert_equals(new A().localName, 'button', 'extends value should == local ' + Let's make human text; such as "localName should be extends". https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:146: assert_not_equals(new A().localName, 'defined-name', 'extends value ' + ditto.
Great start, comments inline. Check grammar in the description: must cannot https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:105: // TODO: What error should be thrown if extends option not specified or null? Part of it is element definition [1] step 6. If the option is not specified then _extends_ is null. Step 7 skips stuff if that's the case. Then there's the question of what to do if extends is specified, but null or undefined. This is a bit more subtle. Because ElementRegistrationOptions is a Dictionary a lot of its handling is specified in Web IDL, especially the EcmaScript binding [2]. You see the values are copied out of the argument into the dictionary in step 5.1.3.2, but undefined and null are filtered in step 5.1.2-3. So null and undefined also end up missing from the dictionary and step 7 is skipped for them, too. [1] https://html.spec.whatwg.org/#element-definition [2] https://heycam.github.io/webidl/#es-dictionary https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:119: }, 'extends value must not be valid customized element name') It might be good to include the actual value in the failure message to make it easy to understand which case is failing. https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:121: }, 'Invalid extends value'); This and the next test may show up in output like PASS Invalid extends value FAIL Invalid extends value which isn't very illuminating. They're different tests, so briefly describe the difference: mention potential custom element names and names of elements whose interface is HTMLUnknown element so people looking at the test output have an high-level idea what each test is doing. https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:137: }, 'extends value cannot be HTMLUnknownElement'); Make this more precise; the value isn't HTMLUnknownElement but a string; I think the spec text calls the thing you're interested in the "element interface for extends"? Briefly paraphrase it for the test. BTW it's a common challenge in these tests to be testing some concept in the spec that has a different name or is only available implicitly in the API. Try to balance brevity, precision, readability. When all else fails cutting and pasting lines from the spec to cite it directly, and make variable names match spec terms, is a safe way to go. https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:143: w.customElements.define('defined-name', A, { extends: 'button'}); Be consistent with spacing; you put a space before } above but not here. It may be more succinct without spaces after {/before } for object literals. https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:144: assert_equals(new A().localName, 'button', 'extends value should == local ' + Use English, so not == but just 'be' or something like that. When wrapping, wrap at a higher-level expression if it results in the same number of line breaks. In this case, you can probably put the whole string on one line by wrapping after , and then you don't need the ' + ' either which is simpler anyway. https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:148: }, 'Localname should be extends value not defined-name'); Write 'Local name ...' with a space. localName is OK when you need to refer to that specific API, but then match the case.
On 2016/09/12 at 04:52:24, kojii wrote: > I think it's better to have separate test files for extends, and it'd be helpful if file name can indicate it's a test for extends. Can you do it? > > https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html (right): > > https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:109: let invalid_values = [ > Let's make the variable name more explicit; in this case this is a list of "valid custom element name", correct? > > https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:125: let invalid_values = [ > ditto; such as "element_names_for_HTMLUnknownElement" or such. > > https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:144: assert_equals(new A().localName, 'button', 'extends value should == local ' + > Let's make human text; such as "localName should be extends". > > https://codereview.chromium.org/2321363002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:146: assert_not_equals(new A().localName, 'defined-name', 'extends value ' + > ditto. Extends is part of the element definition algorithm so I think it's best if it all stays in the same file. I was considering creating a new test file for customized built-in elements definition (including extends), but I thought it would have too much overlap with this one. Do you think the difference between autonomous and built-in custom elements definition is big enough to create a separate test file?
On 2016/09/12 at 06:24:21, yurak wrote: > > Extends is part of the element definition algorithm so I think it's best if it all stays in the same file. I was considering creating a new test file for customized built-in elements definition (including extends), but I thought it would have too much overlap with this one. Do you think the difference between autonomous and built-in custom elements definition is big enough to create a separate test file? One big test is harder to debug when fails, so I prefer to split even more. From that perspective, extends is rather a big chunk of feature that can test separately, but ok if others feel fine.
Updated according to comments, PTL @dominicc So if there is no extends option, is it assumed to be autonomous?
Separated test cases for custom built-in elements into a new test file.
LGTM. A couple of comments inline, let me know when you've addressed those and we'll commit your first patch! https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html (right): https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html:2: <title>Custom Builtin Elements: defineElement</title> Let's use a more specific title. https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html:10: // These aren't implemented yet Scratch this comment; the -expected.txt file will make that evident. We may want to upstream this file regardless of where our implementation is up to. https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html:26: }, 'having valid custon element name element interface (' + val + ') ' + You can use `... ${val} ...` for more succinct string interpolation. Below too.
On 2016/09/12 at 07:45:53, yurak wrote: > Updated according to comments, PTL > > @dominicc > So if there is no extends option, is it assumed to be autonomous? Yes.
One more nit: There's still a "must cannot" in the description; use the edit link to fix that.
Description was changed from ========== Custom Elements: layout tests for custom built-in elements definition For custom built-in elements: https://html.spec.whatwg.org/multipage/scripting.html#element-definition 7. The extends value must be specified and cannot be null, but the spec didn't say what kind of error should be thrown in this case. 7.1. Extends value cannot be valid custom element name, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name 7.2. Extends value must cannot be HTMLUnknownElement, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-i... 7.3. localName set to extends value and not the define-name A -expected.txt file was included since this isn't implemented yet. BUG= ========== to ========== Custom Elements: layout tests for custom built-in elements definition For custom built-in elements: https://html.spec.whatwg.org/multipage/scripting.html#element-definition 7. If an element interface is provided for the extends option, it is assumed to be a customized built-in element. 7.1. If element interface is valid custom element name, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name 7.2. If element interface is HTMLUnknownElement, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-i... 7.3. localName set to extends value and not the define-name A -expected.txt file was included since this isn't implemented yet. BUG= ==========
On 2016/09/13 at 08:22:45, dominicc wrote: > LGTM. A couple of comments inline, let me know when you've addressed those and we'll commit your first patch! > > https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html (right): > > https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html:2: <title>Custom Builtin Elements: defineElement</title> > Let's use a more specific title. > > https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html:10: // These aren't implemented yet > Scratch this comment; the -expected.txt file will make that evident. We may want to upstream this file regardless of where our implementation is up to. > > https://codereview.chromium.org/2321363002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/custom-elements/spec/define-builtin-element.html:26: }, 'having valid custon element name element interface (' + val + ') ' + > You can use `... ${val} ...` for more succinct string interpolation. Below too. I've made these changes and updated the description.
The CQ bit was checked by dominicc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/2321363002/#ps100001 (title: "New file for custom built-in element definition")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Custom Elements: layout tests for custom built-in elements definition For custom built-in elements: https://html.spec.whatwg.org/multipage/scripting.html#element-definition 7. If an element interface is provided for the extends option, it is assumed to be a customized built-in element. 7.1. If element interface is valid custom element name, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name 7.2. If element interface is HTMLUnknownElement, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-i... 7.3. localName set to extends value and not the define-name A -expected.txt file was included since this isn't implemented yet. BUG= ========== to ========== Custom Elements: layout tests for custom built-in elements definition For custom built-in elements: https://html.spec.whatwg.org/multipage/scripting.html#element-definition 7. If an element interface is provided for the extends option, it is assumed to be a customized built-in element. 7.1. If element interface is valid custom element name, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name 7.2. If element interface is HTMLUnknownElement, throws NotSupportedError DOMException https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:element-i... 7.3. localName set to extends value and not the define-name A -expected.txt file was included since this isn't implemented yet. BUG= Committed: https://crrev.com/cf5d797834090f88d671a9cc97659a2011f094c6 Cr-Commit-Position: refs/heads/master@{#418762} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cf5d797834090f88d671a9cc97659a2011f094c6 Cr-Commit-Position: refs/heads/master@{#418762} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
