Issue 2242743002: Make custom elements work in HTML imports

Issue 2242743002: Make custom elements work in HTML imports (Closed)

Created:
4 years, 4 months ago by kochi

Modified:
4 years, 3 months ago

Reviewers:
tkent, kojii, dominicc (has gone to gerrit)

CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, dominicc+watchlist_chromium.org, eae+blinkwatch, kojii, rwlbuis, sof, tfarina, webcomponents-bugzilla_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Make custom elements work in HTML imports Continuation from https://codereview.chromium.org/2132343002/ In HTML imports, custom elements should be created or upgraded. BUG=594918 Committed: https://crrev.com/2443446ab5e812272779bb80a82d680fb4d92126 Cr-Commit-Position: refs/heads/master@{#414263}

Patch Set 1 #

Patch Set 2 : fix #

Patch Set 3 : +1 nesting #

Patch Set 4 : add more tests #

Patch Set 5 : rebase wip #

Total comments: 42

Patch Set 6 : reparent #

Patch Set 7 : wip fixed detached document handling #

Patch Set 8 : tests should pass #

Patch Set 9 : update test #

Total comments: 15

Patch Set 10 : reparent #

Patch Set 11 : WIP cpp update for UpgradeSorter #

Patch Set 12 : resolve most comments #

Total comments: 15

Patch Set 13 : wip #

Patch Set 14 : wip (CustomElementsRegistry.cpp change is gone) #

Total comments: 7

Patch Set 15 : Add TODO and stylistic change for create-element.html #

Patch Set 16 : Update layout tests for more consistency. #

Patch Set 17 : Tentative: back out CEDefitions.cpp change and add failure TestExpectations #

Patch Set 18 : Add detached import test case. #

Patch Set 19 : Clean up CustomElementUpgradeSorter code. #

Patch Set 20 : clean up tests, remove redundant cases #

Total comments: 8

Patch Set 21 : Addresses tkent's comments. #

Created: 4 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+314 lines, -16 lines)			Patch
M	third_party/WebKit/LayoutTests/TestExpectations	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+2 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/async-imports.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+21 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/async-nested-imports.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+30 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/circular-imports.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+3 lines, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/create-element-in-import.html	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/detached-import.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+21 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/inner-html-in-import.html	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/nested-imports.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+2 lines, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/resources/async-component.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+8 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/resources/async-nested-component.html	View	1 2 3 4 5 6 7	1 chunk	+5 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/resources/circular-level1.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/resources/circular-level2.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/resources/create-element.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+38 lines, -0 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/custom-elements/imports/resources/import-custom.html	View		1 chunk	+1 line, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/resources/inner-html.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+28 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/resources/nested-level1.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/resources/nested-level2.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/imports/resources/nested-level3.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/resources/upgrade.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+13 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/sync-create-element-order.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+45 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+30 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+5 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/Document.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/dom/Document.cpp	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/dom/ExecutionContext.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/dom/custom/CustomElement.cpp	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+6 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	3 chunks	+36 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorterTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 55 (35 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dominicc (has gone to gerrit)

Some comments inline. I did not get through the whole thing but I started seeing ...

4 years, 4 months ago (2016-08-19 04:54:11 UTC) #8

kochi

I think tests are ready for next round of reviews. I'm working on C++ side ...

4 years, 4 months ago (2016-08-19 10:51:48 UTC) #9

dominicc (has gone to gerrit)

Some comments inline. Some of these comments are repetitive from last time so I have ...

4 years, 4 months ago (2016-08-22 01:46:36 UTC) #10

kochi

Resolved most of comments, though there are still some remaining. Could you take a look? ...

4 years, 4 months ago (2016-08-22 07:37:35 UTC) #11

Resolved most of comments, though there are still some
remaining.

Could you take a look?

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html
(right):

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-create.html:1:
<!DOCTYPE html>
On 2016/08/19 10:51:47, kochi wrote:
> On 2016/08/19 04:54:11, dominicc wrote:
> > I'm not sure from the name what this test is about. Dynamically created
> imports?
> 
> The counterpart of this test is imports-upgrade.html.
> This tests when custom elements constructor called when "created" (by parser),
> while the other tests when custom elements constructor called when "upgraded"
> (via queued callback).
> 
> Will rename.

Renamed to imports-and-synchronous-crate.html.

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:59: &&
element->document().contextDocument() != document.contextDocument()) {
On 2016/08/19 04:54:11, dominicc wrote:
> I'm not sure this is right. Why would we disallow moving from the main
document,
> to an unrelated document, but allow moving from one import to another import?

I'm not sure I understand "unrelated document" here.

My understanding is that, custom element definition is
always on window.customElements (registry), and both document.createElement()
and importDocument.createElement() should work.

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:82: //
!contextDocument() just for unit testing.
On 2016/08/19 10:51:48, kochi wrote:
> On 2016/08/19 04:54:11, dominicc wrote:
> > Can you explain what you're thinking here? Which unit tests don't have
> document
> > === context document? I think this changed with the switch from
> > CustomElementsRegistry taking the document to taking the window.
> 
> Will update the comment.

Looks the comment is obsolete, and the latter condition is not necessary.

https://codereview.chromium.org/2242743002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:325:
addToImports(imports, element);
On 2016/08/19 04:54:11, dominicc wrote:
> Why not just put them all in the sorter? Sorter can handle having elements in
> random documents added to it. Then you could simplify this code a lot. You
just
> need to have the sorter for imported documents treat them like children of the
> main document. Maybe you could put them in import tree order.

Done.

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/custom-elements/imports/async-imports.html
(right):

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/async-imports.html:10:
let el = document.querySelector('async-component');
On 2016/08/22 01:46:36, dominicc wrote:
> 'el' is kind of neither here nor there; the name could be shorter (like just
'a'
> or 'e') or it could be a word like 'element'. 'elInImport' is weird because it
> mixes abbreviated and unabbreviated things. Maybe go for
> 
> el => a
> elInImport => b
> 
> and have the assertion messages explain the "in import" part.

Done.

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/async-imports.html:11:
assert_true(el instanceof AsyncComponent,
On 2016/08/22 01:46:36, dominicc wrote:
> In a lot of tests we assert something has been upgraded, but we do it
> differently everywhere.
> 
> Maybe add a helper assert_is_upgraded and use that everywhere. Maybe put a
TODO
> on it to make everywhere do it.
> 
> That could assert:
> - not just instanceof, but exact prototype match.
> - matches :defined

Done.

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/custom-elements/imports/async-nested-imports.html
(right):

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/async-nested-imports.html:11:
setup({ explicit_done: true });
On 2016/08/22 01:46:36, dominicc wrote:
> I think async_test is better than explicit done. async_test is more
flexible--it
> can coordinating multiple tests in the one file; you can use the event
watcher;
> etc.

Done.

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/custom-elements/imports/circular-imports.html
(right):

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/circular-imports.html:15:
// upgraded upon definition is ready.
On 2016/08/22 01:46:36, dominicc wrote:
> grammar: upon ... is ready
> 
> maybe "when the" instead of "upon" is easy to read and makes sense
> 
> grammar: definition is -> definitions are
> 
> there are two different kinds of elements, so there must be two definitions?
> Singular "is" is confusing here.

Done.

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/custom-elements/imports/imports-define-in-import.html
(right):

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-define-in-import.html:17:
assert_equals(reactions[0].element, elementsInImport[0]);
On 2016/08/22 01:46:36, dominicc wrote:
> I think I have mentioned this a few times, but simpler array contents and
> assert_array_equals would make these tests easier to understand.

Done.

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right):

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:30: if
(LocalDOMWindow* window = const_cast<Document&>(document).executingWindow())
On 2016/08/22 01:46:36, dominicc wrote:
> const cast :/

Done :-)

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2242743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:58: if
(&element->document() != &document
On 2016/08/22 01:46:36, dominicc wrote:
> Just noticed that you didn't respond to the C++ comments? Don't forget them!

Yeah, this one was only layout tests.
In the next round I will - though I'm not sure specifically what should be done
to this one.

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/custom-elements/imports/imports-and-synchronous-create.html
(right):

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/imports-and-synchronous-create.html:5:
'use strict';
Conversion to async-style is not yet done for this file.

dominicc (has gone to gerrit)

Great! This is starting to come together. https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js File third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js (right): https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js#newcode47 third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js:47: function assert_is_upgraded(element, ...

4 years, 4 months ago (2016-08-22 08:07:36 UTC) #13

dominicc (has gone to gerrit)

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp (right): https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp#newcode62 third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:62: for (Node* n = element, *parent = n->parentOrShadowHostNode(); parent;) ...

4 years, 4 months ago (2016-08-22 08:10:55 UTC) #14

kochi

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp (right): https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp#newcode58 third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:58: if (&element->document() != &document On 2016/08/22 08:07:36, dominicc wrote: ...

4 years, 4 months ago (2016-08-22 12:02:00 UTC) #17

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:58: if
(&element->document() != &document
On 2016/08/22 08:07:36, dominicc wrote:
> I have two problems with this.
> 
> First, you write a DCHECK in the CustomElementsRegistry constructor that
implies
> the main document's context document is the main document. If that's true,
then
> line 58 is redundant.

CustomElementsRegistry constructor and constructor here (a custom element's
constructor) run in different context, right?  CustomElementRegistry is a
property of window, while custom element's constructor can run in the context
of main document, or imported document.

The original DCHECK in CustomElementsRegistry's constructor was to make sure
the registry is created for the main document, not for any imported documents.
(and I think that is no longer necessary.)

> Second, this still seems wrong. Why can we not require that d.createElement
> returns elements in d, whether d is an import document or not?

Looks like I misunderstood, that 
document.createElement('a-a') -> el.ownerDocument = document
importDoc.createElement('a-a') -> el.ownerDocument = importDoc

but now the latter is el.ownerDocument = document.
I'm looking into this.  (maybe a bug?  importDoc.createElement('div') will
make div with ownerDocument = importDoc.)

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp
(right):

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:24:
static HTMLLinkElement* GetParentLinkElement(Document* d)
On 2016/08/22 08:07:36, dominicc wrote:
> Get -> get
> 
> Does HTML imports use this nomenclature of "parent link element"?

Renamed the function to getLinkElement().

> Can't you do this with document->importLoader()->firstImport()->link() and
avoid
> adding stuff to HTMLImportTreeRoot an O(n) search?

Thanks!  I did not notice that path.  It worked.

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:62: for
(Node* n = element, *parent = n->parentOrShadowHostNode(); parent;) {
On 2016/08/22 08:10:55, dominicc wrote:
> On 2016/08/22 at 08:07:36, dominicc wrote:
> > Think about what you've done here when the document element of an import is
a
> custom element. It won't work because the whole body of the for loop is
skipped;
> you've broadened the concept of 'parent' for imports but not applied it
> consistently.
> > 
> > You need to pull 'parent' retrieval out into a function and call it
> consistently for this, and below for line 67+.
> 
> Hmm, actually, maybe this is fine, because this will step into the document.
> Maybe just add a comment about why the initializer of the for loop is special.

Added some comments.

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:82:
DCHECK(m_owner->document() == m_owner->document()->contextDocument());
Hmm, this is a leftover from Koji's original patch, but as this makes little
sense now (it used to get document as a parameter, but now window).
Removing.

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:238:
DCHECK(candidate->document().contextDocument() == m_owner->document());
On 2016/08/22 08:07:36, dominicc wrote:
> This should actually be harmless, but I guess I don't mind either way. Maybe
add
> a comment.

After https://codereview.chromium.org/2252003002/ (Change custom element state
in documents without browsing context) I think this assertion no longer holds.
Removing.

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/imports/HTMLImportTreeRoot.cpp (right):

https://codereview.chromium.org/2242743002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/imports/HTMLImportTreeRoot.cpp:90:
HTMLImportChild* HTMLImportTreeRoot::find(const Document& document) const
On 2016/08/22 08:07:36, dominicc wrote:
> Per above, don't think you need this.

Removed!

dominicc (has gone to gerrit)

A couple more comments inline. https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp (right): https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp#newcode58 third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:58: if (&element->document() != &document ...

4 years, 4 months ago (2016-08-22 12:05:58 UTC) #18

kochi

https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp (right): https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp#newcode58 third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:58: if (&element->document() != &document On 2016/08/22 12:05:58, dominicc (OOF ...

4 years, 4 months ago (2016-08-22 15:56:30 UTC) #21

kochi

https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElement.cpp#newcode30 third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:30: if (LocalDOMWindow* window = document.executingWindow()) This was necessary to ...

4 years, 4 months ago (2016-08-22 16:10:21 UTC) #22

kochi

tkent-san, could you take a look? https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp (right): https://codereview.chromium.org/2242743002/diff/290023/third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp#newcode54 third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:54: // |parent| can ...

4 years, 4 months ago (2016-08-24 11:26:03 UTC) #35

tkent

lgtm https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html File third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html (right): https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html#newcode21 third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html:21: super(); wrong indentation. https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp (right): https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp#newcode23 ...

4 years, 4 months ago (2016-08-24 12:07:20 UTC) #38

kochi

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html File third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html (right): https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html#newcode21 third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html:21: super(); On 2016/08/24 12:07:20, tkent wrote: > wrong indentation. ...

4 years, 4 months ago (2016-08-24 15:36:52 UTC) #43

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html
(right):

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/custom-elements/imports/upgrade-order.html:21:
super();
On 2016/08/24 12:07:20, tkent wrote:
> wrong indentation.

Done.

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp
(right):

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:23:
static HTMLLinkElement* getLinkElement(const Document& document)
On 2016/08/24 12:07:20, tkent wrote:
> * Chromium coding style prefers anonymous namespace to static function.
> * The name isn't descriptive enough.  getLinkElementImporting(const Document&)
?

The static function follows the convention used in this directory.

For the function name, renamed getLinkElementForImport(), and renamed its
argument
from document to import.

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:25:
HTMLImportLoader* loader = document.importLoader();
On 2016/08/24 12:07:20, tkent wrote:
> I prefer shorter code like:
>     if (HTMLImportLoader* loader = document.importLoader())
>         return loader->firstImport()->link();
>     return nullptr;

Neat.  Done.

https://codereview.chromium.org/2242743002/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeSorter.cpp:42:
ParentChildMap::AddResult result =
On 2016/08/24 12:07:20, tkent wrote:
> This function contains two hash lookups and avoidable copy of
> HeapHashSet<Member<Node>>.  It's an issue of the original code, and you don't
> need to fix it in this CL.

Will do in a separate CL.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2242743002/510001

4 years, 4 months ago (2016-08-24 15:37:57 UTC) #47

commit-bot: I haz the power

Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/128912)

4 years, 4 months ago (2016-08-24 17:16:45 UTC) #49

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2242743002/510001

4 years, 3 months ago (2016-08-24 23:14:41 UTC) #51

commit-bot: I haz the power

4 years, 3 months ago (2016-08-25 02:25:43 UTC) #55

Message was sent while issue was closed.

Patchset 21 (id:??) landed as
https://crrev.com/2443446ab5e812272779bb80a82d680fb4d92126
Cr-Commit-Position: refs/heads/master@{#414263}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages