Make Custom Elements V1 work in HTML imports documents

Make Custom Elements V1 work in HTML imports documents

This patch extends Custom Elements V1 to support documents loaded by
HTML Imports.

BUG=594918

[kojii, 2016-07-12 06:59:48]

Description was changed from

==========
WIP: imports

BUG=
==========

to

==========
Make Custom Elements V1 work in HTML imports documents

This patch extends Custom Elements V1 to support documents loaded by
HTML Imports.

BUG=594918
==========

[kojii, 2016-07-12 07:22:40]

kojii@chromium.org changed reviewers:
+ dominicc@chromium.org

[kojii, 2016-07-12 07:22:41]

PTAL.

[dominicc (has gone to gerrit), 2016-07-13 06:12:30]

Some comments inline.

This doesn't really explain the semantics you're going for here. I think that
needs to be documented somewhere, maybe add a .md file explaining the challenges
and how this works.

I think we need a lot more tests; imports are very dynamic and you can add and
remove imports to block and unblock the tree. See the imports tests in
fast/dom/custom for some ideas. You may want to port those to V1 as a starting
point.

C++ unit tests would be good too.

https://codereview.chromium.org/2132343002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html
(right):

https://codereview.chromium.org/2132343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:12:
console.log("constructor", this);
Maybe remove logs from tests.

https://codereview.chromium.org/2132343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:15:
test(() => {
I feel like you use tests where I use assertions :)

TBH for a test like this, imports integration which won't upstream, I don't
really mind. In general I'd like to have a structure with:

directory -- spec/big area (here, custom elements + imports, which is perfect
granularity.)
file -- set of related tests
test -- one specific aspect under test
assert -- one specific condition that relates to the test

https://codereview.chromium.org/2132343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right):

https://codereview.chromium.org/2132343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:30: if (Document*
contextDocument = const_cast<Document&>(document).contextDocument()) {
FYI haraken says in Q4 that Blink will be able to run JavaScript in detached
contexts. What we have today means elements will not run callbacks if their
document is detached. We may need to have elements point to their definitions or
something.

Just a "heads up" that something like this is coming--nothing to do in this
patch.

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:34:
assert_equals(reactions.length, 2);
These presumably have to be in order. Can you push the actual objects and make
sure they're the right ones?

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:35:
}, 'import should create custom if after define');
These descriptions are cryptic; what is create custom? How is this different to
imports-upgrade? (I know they're different, but you could make it easier to
understand with longer, more specific file names and more specific assertions.)

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:56: &&
element->document().contextDocument() != document.contextDocument()) {
I don't understand why we need this here. If element->document is document, then
element->document->foo is document->foo unless there's some subtle side effects
happening right at this point.

It would be good to write some C++ unit tests for this that demonstrates bailing
out at each point due to each case.

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:304: }
else-if might be clearer than continues.

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:316: if
(!imports.isEmpty())
I'm not really grokking this change at this point. For each element, we grabble
through imports? Why? What if there are tons of imports?

Why can't we just define a sort order over nodes in imported documents too and
keep everything more uniform here?

[kojii, 2016-07-13 06:47:17]

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:34:
assert_equals(reactions.length, 2);
On 2016/07/13 at 06:12:30, dominicc wrote:
> These presumably have to be in order. Can you push the actual objects and make
sure they're the right ones?

The test does push the actual object. Also the other test checks the order, but
no in this test because this tests "length == 1" before. I can add that to this
test too.

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:56: &&
element->document().contextDocument() != document.contextDocument()) {
On 2016/07/13 at 06:12:30, dominicc wrote:
> I don't understand why we need this here. If element->document is document,
then element->document->foo is document->foo unless there's some subtle side
effects happening right at this point.

We'd like to check if contextDocument() match, so this could be just:
  if (element->document().contextDocument() != document.contextDocument())
but I left the original "&element->document() != &document" because it's the
common, fast path.

Should I just remove the fast path? It might be easier to read then.

> It would be good to write some C++ unit tests for this that demonstrates
bailing out at each point due to each case.

One of JS test hit this. Can't that do? This is how I found the following spec
issue:
https://github.com/w3c/webcomponents/issues/533

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:304: }
On 2016/07/13 at 06:12:30, dominicc wrote:
> else-if might be clearer than continues.

Done.

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:304: }
On 2016/07/13 at 06:12:30, dominicc wrote:
> else-if might be clearer than continues.

Done.

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:316: if
(!imports.isEmpty())
On 2016/07/13 at 06:12:30, dominicc wrote:
> I'm not really grokking this change at this point. For each element, we
grabble through imports? Why? What if there are tons of imports?
> 
> Why can't we just define a sort order over nodes in imported documents too and
keep everything more uniform here?

I'm sorry, I can't understand what "grabble" means.

I think this code defines the order, by grouping by documents and process import
documents first.

If you're saying all these should be done hidden in CustomElementUpgradeSorter,
that I can understand. Is this what you're saying here?

[kojii, 2016-07-13 07:00:14]

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html
(right):

https://codereview.chromium.org/2132343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:34:
assert_equals(reactions.length, 2);
On 2016/07/13 at 06:47:16, kojii wrote:
> On 2016/07/13 at 06:12:30, dominicc wrote:
> > These presumably have to be in order. Can you push the actual objects and
make sure they're the right ones?
> 
> The test does push the actual object. Also the other test checks the order,
but no in this test because this tests "length == 1" before. I can add that to
this test too.

Done in PS5.

[dominicc (has gone to gerrit), 2016-07-19 01:57:40]

More comments inline.

I think these semantics are pretty different to v1. It would be interesting to
change v0 to have semantics this loose and see whether Polymer apps break. I
suspect they will be surprisingly resilient. Another thing that could be
interesting is to port the v0 tests and change them to pass; the diff will tell
us more what the different semantic means in practice, gut check the same issues
morrita-san wrote tests for v0 for, etc.

By "grabble" I mean basically "access"/"walk"

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html
(right):

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:1:
<!DOCTYPE html>
I don't see any tests for:

- deep import trees
- trees with duplicate imports
- asynchronous imports
- dynamic import trees

Could you add some?

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-upgrade.html
(right):

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-upgrade.html:25:
assert_equals(reactions[0].element, elementsInImport[0]);
This test would be more descriptive if it had

<x-x id="a">
<link rel="import" href="..."> <-- contains ID c
<x-x id="b">
<script>... define
assert_array_equals(reactions, ...

Note how I've put elements before and after the import. Then someone reading the
test cant tell if upgrades happen import-first (which I think is what you have
implemented here) or "flatten imports into tree order" order.

The test as written isn't descriptive because both of those orders produce the
same result.

The same thing goes for the test above, where the call to 'define' is in the
imported document.

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:260: using
ElementsDocumentMap = HeapHashMap<Document*, ElementsInDocument>;
Maybe DocumentElementsMap, since it's a map *from* document *to* element?

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:278: for
(const auto& it : imports) {
I don't think hashtable iteration order is a good idea; generally web platform
APIs do things in some stable order.

https://codereview.chromium.org/2132343002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:311:
collectFromImports(imports, elements);
TL;DR I think you should handle sorting in the custom elements upgrade sorter
and not split that logic into two places unless there's a good reason to do so.