Issue 249563003: REGRESSION(r171966): Custom elements in async imports don't get upgrade.

Issue 249563003: REGRESSION(r171966): Custom elements in async imports don't get upgrade. (Closed)

Created:
6 years, 8 months ago by Hajime Morrita

Modified:
6 years, 7 months ago

Reviewers:
dglazkov, dominicc (has gone to gerrit)

CC:
blink-reviews, webcomponents-bugzilla_chromium.org, sof, eae+blinkwatch, ojan, dglazkov+blink, adamk+blink_chromium.org, rwlbuis, Inactive

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

REGRESSION(r171966): Custom elements in async imports don't get upgrade. For these custom elements, microstask steps are queued but never get dispatched as the import isn't part of microtask dispatch tree. This change supresses microtask queue creation for async imports so that the microtask steps from these imports go to the top-level queue. TEST=import-custom-element-async-resolve.html R=dominicc@chromium.org, dglazkov@chromium.org BUG=365956 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173402

Patch Set 1 #

Total comments: 7

Patch Set 2 : Added and revised tests #

Total comments: 8

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 6

Patch Set 6 : #

Total comments: 6

Patch Set 7 : #

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+386 lines, -49 lines)			Patch
A	LayoutTests/fast/html/imports/import-custom-element-async-resolve.html	View	1 2 3 4 5 6	1 chunk	+65 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-async-resolve-expected.txt	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-cycle.html	View	1 2 3 4 5	1 chunk	+30 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-dup-resolve.html	View	1 2 3 4 5	1 chunk	+43 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-dup-resolve-expected.txt	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-in-grandchild.html	View	1 2 3 4 5	1 chunk	+31 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-in-grandchild-expected.txt	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-in-master-and-grandchild.html	View	1 2 3 4 5	1 chunk	+37 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/import-custom-element-in-master-and-grandchild-expected.txt	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-1.html	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/html/imports/resources/custom-element-hello-2.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-3.html	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-4.html	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A +	LayoutTests/fast/html/imports/resources/custom-element-hello-5.html	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-6.html	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-7.html	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-8.html	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-cycle-child.html	View	1 2 3 4 5	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-cycle-parent.html	View	1 2 3 4 5	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-parent-12.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-parent-34.html	View	1 2 3 4 5 6	1 chunk	+6 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/html/imports/resources/custom-element-hello-parent-56.html	View	1 2 3 4 5 6	1 chunk	+6 lines, -0 lines	0 comments	Download
A	LayoutTests/http/tests/htmlimports/import-custom-element-order.html	View	1 2 3 4 5	1 chunk	+37 lines, -0 lines	0 comments	Download
A	LayoutTests/http/tests/htmlimports/import-custom-element-order-expected.txt	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
A	LayoutTests/http/tests/htmlimports/resources/import-custom-element-hello-1.html	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/http/tests/htmlimports/resources/import-custom-element-helper.js	View	1 2	1 chunk	+21 lines, -0 lines	0 comments	Download
M	LayoutTests/http/tests/htmlimports/resources/import-slow-child.cgi	View	1	1 chunk	+2 lines, -3 lines	0 comments	Download
A	LayoutTests/http/tests/htmlimports/resources/import-slow-custom-element-hello.cgi	View	1 2 3 4 5	1 chunk	+10 lines, -0 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	Source/core/dom/custom/CustomElementMicrotaskImportStep.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementMicrotaskImportStep.cpp	View	1 2 3 4 5 6	1 chunk	+13 lines, -7 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementMicrotaskQueue.cpp	View	1 2 3 4 5 6	2 chunks	+11 lines, -10 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementMicrotaskResolutionStep.cpp	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementMicrotaskStep.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -2 lines	0 comments	Download
M	Source/core/html/imports/HTMLImport.h	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/html/imports/HTMLImport.cpp	View	1 2 3 4 5	1 chunk	+11 lines, -0 lines	0 comments	Download
M	Source/core/html/imports/HTMLImportChild.cpp	View	1 2 3 4 5 6	2 chunks	+1 line, -19 lines	0 comments	Download

Messages

Total messages: 21 (0 generated)

Expand Messages | Collapse Messages

Hajime Morrita

Dominic: PTAL? That simplicity I hoped didn't work after all :-/

6 years, 8 months ago (2014-04-23 19:48:06 UTC) #1

dominicc (has gone to gerrit)

On 2014/04/23 19:48:06, morrita1 wrote: > Dominic: PTAL? > That simplicity I hoped didn't work ...

6 years, 8 months ago (2014-04-24 07:28:11 UTC) #2

dominicc (has gone to gerrit)

Oops, and here are some comments. https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/imports/import-custom-element-async-resolve.html File LayoutTests/fast/html/imports/import-custom-element-async-resolve.html (right): https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/imports/import-custom-element-async-resolve.html#newcode1 LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:1: <!DOCTYPE html> I ...

6 years, 8 months ago (2014-04-24 07:28:32 UTC) #3

Hajime Morrita

Thanks for the review, Dominic! I added bunch of tests to cover more corners. And ...

6 years, 8 months ago (2014-04-24 23:29:32 UTC) #4

Thanks for the review, Dominic!
I added bunch of tests to cover more corners.

And then, |HTMLImportLoader::m_microtaskQueue| guarantees that the custom
elements inside the import 
being upgraded before elements that follow the <link> in the parent (the first
parent precisely.)

The queue is needed for sync import. Async import doesn't need it as it doesn't
block followers.
But before this change the queue was created regardless of sync/async.
That was wrong because the queue isn't reached from anywhere if there is no sync
<link>.
That's because CustomElementMicrotaskImportStep is created only for sync
imports. (See HTMLImportChild.cpp) Thus the steps in the queue are never
dispatched.

This change lets the queue created only if there is at least one blocking <link
rel=import> for that imported document (or the loader).
If there is only <link rel=import async> for that import, the queue isn't
created.

Where do upgrade callbacks go then? They go to the global queue, which will
dispatch the task eventually - in async way.

A possible alternative fix might be to create the step regardless of the
sync/async. 
I'm however not sure how make it work. The step is a "sentinel" of the queue
consumption and having it for async import doesn't seem making sense for me.




On 2014/04/24 07:28:32, dominicc wrote:
> Oops, and here are some comments.
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> File LayoutTests/fast/html/imports/import-custom-element-async-resolve.html
> (right):
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:1:
> <!DOCTYPE html>
> I think you want way more tests at this point. For example:
> 
> What if a sync import has an async import child?
> What if an async import has a sync import child?
> What if a sync import is duped as an async import, but blocked?
> What if an async import is duped as a sync import, but blocked?
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:14:
> document.registerElement('x-hello', Hello);
> Why is Hello in PascalCase? It's not the real constructor.
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:25: var
> t1 = async_test('Instantiate custom elements in Async imports');
> Async -> async
> next line too
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:29: {
> Open brace goes on the previous line. Below too.
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:32:
> assert_array_equals(['hello-1', 'hello-2', 'hello-3'], Hello.ids.sort());
> You must be able to write a tighter assertion than this. For example, can
> c-e-hello-2 ever load before c-e-hello-1?
> 
>
https://codereview.chromium.org/249563003/diff/1/LayoutTests/fast/html/import...
> LayoutTests/fast/html/imports/import-custom-element-async-resolve.html:38: Bye
=
> function() {};
> Since this is essentially the same as Hello, you can extract a helper
function.
> 
>
https://codereview.chromium.org/249563003/diff/1/Source/core/dom/custom/Custo...
> File Source/core/dom/custom/CustomElementScheduler.cpp (right):
> 
>
https://codereview.chromium.org/249563003/diff/1/Source/core/dom/custom/Custo...
> Source/core/dom/custom/CustomElementScheduler.cpp:88:
> OwnPtr<CustomElementMicrotaskImportStep> step =
>
CustomElementMicrotaskImportStep::create(import->loader()->ensureMicrotaskQueue());
> I thought we were always creating microtask queues for imports now. Don't the
> above assertions mean this queue is always in existence at this point?

dominicc (has gone to gerrit)

Thanks for the description, that was helpful, but there's still one part I don't understand. ...

6 years, 8 months ago (2014-04-25 02:39:45 UTC) #5

Hajime Morrita

> Don't all imports go through CustomElementScheduler::scheduleImport? No, only sync imports go there. > > ...

6 years, 8 months ago (2014-04-25 20:53:16 UTC) #6

dominicc (has gone to gerrit)

Some test comments. What did Dimitri and Polymer say about the idea of interleaving the ...

6 years, 7 months ago (2014-04-28 01:32:31 UTC) #7

Hajime Morrita

On 2014/04/28 01:32:31, dominicc wrote: > Some test comments. > > What did Dimitri and ...

6 years, 7 months ago (2014-04-28 18:05:50 UTC) #8

On 2014/04/28 01:32:31, dominicc wrote:
> Some test comments.
> 
> What did Dimitri and Polymer say about the idea of interleaving the
> createdCallback, upgrade of individual custom elements from different async
> imports?
> 
> The Custom Elements spec doesn't talk specifically about async imports, but it
> has that highest stable order stuff. Since the async imports can still be
> arranged in a tree, I'm not sure interleaving individual elements from
different
> async imports is right?

Hm, this is good point. I did prefer eliminating the queue because of
implementation simiplicity.
But probably keeping tree provides better, more reasonable model overall. Le met
try that way.

> 
>
https://codereview.chromium.org/249563003/diff/80001/LayoutTests/fast/html/im...
> File LayoutTests/fast/html/imports/import-custom-element-dup-resolve.html
> (right):
> 
>
https://codereview.chromium.org/249563003/diff/80001/LayoutTests/fast/html/im...
> LayoutTests/fast/html/imports/import-custom-element-dup-resolve.html:20: //
> Remove async links whose loading orders are't deterministic.
> aren't
> 
>
https://codereview.chromium.org/249563003/diff/80001/LayoutTests/fast/html/im...
> File
>
LayoutTests/fast/html/imports/import-custom-element-in-master-and-grandchild.html
> (right):
> 
>
https://codereview.chromium.org/249563003/diff/80001/LayoutTests/fast/html/im...
>
LayoutTests/fast/html/imports/import-custom-element-in-master-and-grandchild.html:16:
> setTimeout(function() {
> Do you do this everywhere? Maybe it makes sense to just make the FIXME part of
> the latch?
> 
>
https://codereview.chromium.org/249563003/diff/80001/LayoutTests/http/tests/h...
> File LayoutTests/http/tests/htmlimports/import-custom-element-order.html
> (right):
> 
>
https://codereview.chromium.org/249563003/diff/80001/LayoutTests/http/tests/h...
> LayoutTests/http/tests/htmlimports/import-custom-element-order.html:32: <link
> rel=import async href="resources/import-custom-element-hello-1.html"
> onload="ready()">
> There's another test where you use the latch with number of imports = 1; why
is
> this different? Maybe this way is simpler?

Hajime Morrita

Hi Dominic, thanks for another review! So PTAL another time? I changed the way so ...

6 years, 7 months ago (2014-04-29 23:11:37 UTC) #9

dominicc (has gone to gerrit)

Getting there. The one remaining issue is I think this "remains" thing isn't the right ...

6 years, 7 months ago (2014-04-30 23:57:20 UTC) #10

dominicc (has gone to gerrit)

One more thought: Why not break this patch up at this point? The debugging stuff ...

6 years, 7 months ago (2014-05-01 00:01:11 UTC) #11

Hajime Morrita

Dominic, thanks for taking time! I'll split patches out two three. On 2014/04/30 23:57:20, dominicc ...

6 years, 7 months ago (2014-05-01 01:23:56 UTC) #12

Hajime Morrita

On 2014/05/01 01:23:56, morrita1 wrote: > Dominic, thanks for taking time! I'll split patches out ...

6 years, 7 months ago (2014-05-01 02:20:56 UTC) #13

Hajime Morrita

Dominic: Ping, just in case. I appreciated your help.

6 years, 7 months ago (2014-05-06 00:19:47 UTC) #15

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/249563003/110001

6 years, 7 months ago (2014-05-06 16:41:43 UTC) #18

Hajime Morrita

6 years, 7 months ago (2014-05-07 00:05:39 UTC) #21

Message was sent while issue was closed.

Uh, sorry for interrupting your holiday/vacation if you are still in. And thanks
for the reply.

Expand Messages | Collapse Messages