Issue 106903007: Let unresolved custom element go through CustomElementCallbackQueue.

Issue 106903007: Let unresolved custom element go through CustomElementCallbackQueue. (Closed)

Created:
7 years ago by Hajime Morrita

Modified:
6 years, 12 months ago

Reviewers:
falken, dominicc (has gone to gerrit), dglazkov1

CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org

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

Visibility:
Public.

More Reviews

Description

Let unresolved custom element go through CustomElementCallbackQueue. As a preparation for crbug.com/313384, this change introduces CustomElementResolutionInvocation that is created per unresolved element and goes through CECallbackQueue, then it does retry the resolution attempt at its own invocation. By going through the queue, we can guarantee the tree order of unresolved element in CEUpdagradeCandidateMap even with HTMLImports, which currently throw unresolved elements into the candidate map in unpredictable way. In coming changes, we'll fix the invocation order of import-provided elements. This change will work with those changes and will provide well defined order of element upgrade. BUG=313384 TEST=document-register-on-create-callback.html R=dominicc,dglazkov,falken Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164309

Patch Set 1 #

Total comments: 17

Patch Set 2 : Updated #

Total comments: 12

Patch Set 3 : Updated. #

Total comments: 24

Patch Set 4 : Updated #

Total comments: 1

Patch Set 5 : For landing #

Patch Set 6 : Diff was broken. Landing again #

Patch Set 7 : Updated test. Landing again. #

Created: 6 years, 12 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+164 lines, -111 lines)			Patch
M	LayoutTests/fast/dom/custom/created-callback.html	View	1 2 3 4 5 6	1 chunk	+29 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/custom/created-callback-expected.txt	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/custom/document-register-on-create-callback.html	View	1 2 3 4 5 6	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt	View	1 2 3 4 5 6	1 chunk	+11 lines, -0 lines	0 comments	Download
M	Source/core/core.gypi	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/dom/custom/CustomElement.cpp	View	1 2 3 4	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/dom/custom/CustomElementCallbackInvocation.h	View	1 2 3	2 chunks	+3 lines, -7 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementCallbackInvocation.cpp	View	1 2 3 4	2 chunks	+0 lines, -23 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementCallbackQueue.h	View	1	4 chunks	+4 lines, -3 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementCallbackScheduler.h	View	1 2 3 4	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/dom/custom/CustomElementCallbackScheduler.cpp	View	1 2 3 4	3 chunks	+10 lines, -9 lines	0 comments	Download
A +	Source/core/dom/custom/CustomElementProcessingStep.h	View	1 2 3	1 chunk	+14 lines, -7 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementRegistrationContext.h	View	1 2	2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/core/dom/custom/CustomElementRegistrationContext.cpp	View	1	3 chunks	+14 lines, -17 lines	0 comments	Download
A +	Source/core/dom/custom/CustomElementResolutionStep.h	View	1 2 3	1 chunk	+19 lines, -23 lines	0 comments	Download
A +	Source/core/dom/custom/CustomElementResolutionStep.cpp	View	1 2	1 chunk	+20 lines, -17 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

falken

I'm not familiar enough to comment on the design but the code change lgtm

7 years ago (2013-12-12 10:55:31 UTC) #2

dominicc (has gone to gerrit)

Comments inline. I like the direction this is going, I think the naming and factoring ...

7 years ago (2013-12-13 05:11:15 UTC) #3

Comments inline.

I like the direction this is going, I think the naming and factoring will end up
much better when we push this to conclusion.

I don't have a 100% clear idea of what the end product should be but I have some
thoughts inline.

I think we should do the renaming in a separate changelist. But I think we can
get the factoring closer to what we want in this changelist.

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
File
LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt
(right):

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt:1:
document.register() should affect even for queued elements.
Can you be more specific? I'm not sure what to expect this test to cover.

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
File LayoutTests/fast/dom/custom/document-register-on-create-callback.html
(right):

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
LayoutTests/fast/dom/custom/document-register-on-create-callback.html:4: <script
src="../../../resources/js-test.js"></script>
We could use the W3C test harness for this test and consider upstreaming it to
W3C.

Maybe copy fast/dom/custom/attribute-changed-callback.html for the style... I
have been cutting down on elements the HTML parser will insert like <html>,
<head>.

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
LayoutTests/fast/dom/custom/document-register-on-create-callback.html:27: <x-c
id=elemC></x-c>
I think this element is a distraction; if it isn't needed let's remove it.

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
LayoutTests/fast/dom/custom/document-register-on-create-callback.html:29:
shouldBe("createdElements.map(function(e) { return e.id; })", "['elemA',
'elemB']");
It might be simpler to have the createdCallback simply push the IDs into the
array and just compare the value of createdElements to the expected value.

https://codereview.chromium.org/106903007/diff/1/LayoutTests/fast/dom/custom/...
LayoutTests/fast/dom/custom/document-register-on-create-callback.html:32:
</html>
I think I understand what this is testing... it's testing that if A and B are
parsed as a group, but A's created callback registers B, that upgrade happens.
Can the test make that clearer?

Would it be different/more interesting/also interesting if B is parsed before A?

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementCallbackQueue.h (right):

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementCallbackQueue.h:43: // FIXME: Probably this
should be renamed to CustomElementInvocationQueue
Let's do the renaming in a follow-up changelist.

I think CustomElementInvocation also needs to be renamed. When processing an
unresolved element, nothing of the author's is getting invoked; "invocation" is
too generic.

Here's a proposal:

CustomElementQueue ("Element Queue" is a spec term)
CustomElementQueueEntry
CustomElementResolveStep : CustomElementQueueEntry
CustomElementUpgradeStep : CustomElementQueueEntry
CustomElementCallbackStep : CustomElementQueueEntry

For now this can be:

CustomElementCallbackQueue
CustomElementQueueEntry
CustomElementResolveStep : CustomElementQueueEntry
CustomElementCallbackInvocation : CustomElementQueueEntry

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementCallbackScheduler.h (right):

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementCallbackScheduler.h:52: static void
scheduleResolutionCallback(const CustomElementDescriptor&, PassRefPtr<Element>);
Likewise we can call this scheduleResolveStep, and in a follow up changelist
scheduleCreatedCallback -> scheduleUpgradeStep.

I think schedule{EnteredView,LeftView,AttributeChanged}Callback names are OK.

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementRegistrationContext.cpp (right):

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementRegistrationContext.cpp:1: /*
I think this isn't quite right. resolvePoppedElement/define will push creation
callbacks. But they might be in the wrong order now.

The naming could be better too... popped isn't quite right; it's a queue.

And didFailResolution happens a LOT later then being marked as "unresolved." But
it would be an easy mistake to think the unresolved state is only downstream of
didFailResolution.

Why do we even need two attempts at resolution? We could just always enqueue a
resolve step I think. That might be simpler.

so createCustomTagElement, didGiveTypeExtension just tell the scheduler to
enqueue a resolve step. Their job is to build the CustomElementDescriptor and
pass it with the element to the resolve step.

The resolve step looks for the definition. If one is available, it does upgrade
immediately. If not, it enters the element in the upgrade candidates map.

We can probably shift resolve, didResolveElement, didCreateUnresolvedElement
into the resolution step class. And rename didCreateUnresolvedElement to
didSomethingUpgradeCandidate.

The code for actual upgrade steps needs to be shared between the resolve step,
and the post-hoc upgrade candidate upgrade step.

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementResolutionInvocation.h (right):

https://codereview.chromium.org/106903007/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementResolutionInvocation.h:51: virtual void
dispatch(Element*);
This needs OVERRIDE.

Hajime Morrita

Thanks for the thorough review Dominic! I updated the patch to address some of your ...

7 years ago (2013-12-13 07:02:29 UTC) #4

dominicc (has gone to gerrit)

Great work! I think how you have it down is easier to understand. I've made ...

7 years ago (2013-12-14 02:42:00 UTC) #5

dominicc (has gone to gerrit)

Oops, ...and here are the comments. https://codereview.chromium.org/106903007/diff/20001/LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt File LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt (right): https://codereview.chromium.org/106903007/diff/20001/LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt#newcode1 LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt:1: document.register() should affect ...

7 years ago (2013-12-14 02:42:38 UTC) #6

Hajime Morrita

Thanks for the review! PTAL at the updated one? https://codereview.chromium.org/106903007/diff/20001/LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt File LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt (right): https://codereview.chromium.org/106903007/diff/20001/LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt#newcode1 LayoutTests/fast/dom/custom/document-register-on-create-callback-expected.txt:1: ...

7 years ago (2013-12-16 09:00:33 UTC) #7

dominicc (has gone to gerrit)

Comments inline. Re: renaming isCreated, feel free to add a FIXME and I can do ...

7 years ago (2013-12-18 08:46:41 UTC) #8

Hajime Morrita

PTAL? https://codereview.chromium.org/106903007/diff/40001/LayoutTests/fast/dom/custom/created-callback.html File LayoutTests/fast/dom/custom/created-callback.html (right): https://codereview.chromium.org/106903007/diff/40001/LayoutTests/fast/dom/custom/created-callback.html#newcode125 LayoutTests/fast/dom/custom/created-callback.html:125: var toBeMoved = originalFrame.contentDocument.getElementById("toBeMoved"); On 2013/12/18 08:46:41, dominicc ...

7 years ago (2013-12-19 05:06:58 UTC) #9

dominicc (has gone to gerrit)

This LGTM, thanks for working on this, it looks neat. In the follow-up changelists we ...

7 years ago (2013-12-20 05:36:34 UTC) #10

commit-bot: I haz the power

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

6 years, 12 months ago (2013-12-24 05:39:58 UTC) #11

commit-bot: I haz the power

Retried try job too often on linux_blink for step(s) blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests ...

6 years, 12 months ago (2013-12-24 06:19:44 UTC) #12

commit-bot: I haz the power

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

6 years, 12 months ago (2013-12-24 07:08:37 UTC) #13

commit-bot: I haz the power

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

6 years, 12 months ago (2013-12-24 07:54:32 UTC) #14

Message was sent while issue was closed.

Change committed as 164309

Expand Messages | Collapse Messages