HTML Imports: No more BlockingDocumentCreation. (issue 238923009)

[mor...@chromium.org]

Reviewers: dominicc,

Message:
PTAL? This is what we discussed last week.

Description:
HTML Imports: No more BlockingDocumentCreation.

This change gets rid of HTMLImportState::BlockingDocumentCreation.
This is a perf improvement as Blink no longer delay parsing of imports
which leads earlier fetch requests.

To make it work, the change:

* Gets rid of the notion of "owning loader".
Now fewer places depends on the import tree ordering.

* HTMLImportLoader::addImport() is one of such place.
It does ensure that:

* The firstImport() is the first import of the same URL in
tree order of the import tree.
* The firstImport() has all children that is loaded by the document.

HTMLImport::recalcTreeState() and
HTMLImportLoader::shouldBlockScriptExecution()
relies on these invariants.

* The microtask queue of CustomElementMicrotaskImportSteps is got owned by
HTMLImportLoader. Also, now each HTMLImportChild can have the step
regardless of its tree order. This ensures that each imported document
blocks
microtask queue consumption of all referring documents.

* HTMLImportResolver::shouldBlockDocumentCreation() is merged into
shouldBlockScriptExecution()

The change on import-nested-dup.html is to cover the observable invariant of
script execution order more precisely.

TEST=import-dup-custom-element.html,import-nested-dup.html,import-nested-dup-2.html
R=domi...@chromium.org
BUG=none

Please review this at https://codereview.chromium.org/238923009/

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

Affected files (+259, -136 lines):
M LayoutTests/fast/html/imports/import-nested-dup.html
M LayoutTests/fast/html/imports/import-nested-dup-2.html
M LayoutTests/fast/html/imports/import-nested-dup-2-expected.txt
M LayoutTests/fast/html/imports/import-nested-dup-expected.txt
M LayoutTests/fast/html/imports/resources/nest-dup.html
A + LayoutTests/fast/html/imports/resources/nest-dup-child.html
A LayoutTests/http/tests/htmlimports/import-dup-custom-element.html
A
LayoutTests/http/tests/htmlimports/import-dup-custom-element-expected.txt
A LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
A
LayoutTests/http/tests/htmlimports/resources/pointing-custom-element-def.html
A LayoutTests/http/tests/htmlimports/resources/using-custom-element-1.html
A LayoutTests/http/tests/htmlimports/resources/using-custom-element-2.html
M Source/core/core.gypi
M Source/core/dom/custom/CustomElementMicrotaskDispatcher.h
M Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp
M Source/core/dom/custom/CustomElementMicrotaskImportStep.h
M Source/core/dom/custom/CustomElementMicrotaskImportStep.cpp
M Source/core/dom/custom/CustomElementScheduler.cpp
A + Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h
M Source/core/html/HTMLLinkElement.h
M Source/core/html/HTMLLinkElement.cpp
M Source/core/html/imports/HTMLImport.h
M Source/core/html/imports/HTMLImport.cpp
M Source/core/html/imports/HTMLImportChild.h
M Source/core/html/imports/HTMLImportChild.cpp
M Source/core/html/imports/HTMLImportLoader.h
M Source/core/html/imports/HTMLImportLoader.cpp
M Source/core/html/imports/HTMLImportState.h
M Source/core/html/imports/HTMLImportStateResolver.h
M Source/core/html/imports/HTMLImportStateResolver.cpp
M Source/core/html/imports/HTMLImportsController.h
M Source/core/html/imports/HTMLImportsController.cpp
M Source/core/html/imports/LinkImport.h
M Source/core/html/imports/LinkImport.cpp
M Source/wtf/TreeNode.h
M Source/wtf/TreeNodeTest.cpp

[domi...@chromium.org]

LGTM

I think this achieves your objective, is possible to understand, nothing
fishy
(ownership of the CEMicrotaskQueue is now shared, but dispatch will still
clear
out backreferences as before), leaves room for optimizations later if
necessary.

I have a bunch of comments inline. I'm happy to look at this again if you
need
me to but the comments are trivial.

We talked on IM about making CustomElementMicrotaskQueue refcounted. I
think the
design you have in this patch is fine, making CEMQ refcounted and removing
RefCountedCEMQ could be explored later.

It should be possible to assert that one CEMQ doesn't reenter dispatch and
doesn't dispatch work with two different CEMQs as parents (ie you might call
dispatch again, but the queue should be empty by that point.) Could you
investigate adding assertions like that in a follow-up changelist?


https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/import-dup-custom-element.html
File LayoutTests/http/tests/htmlimports/import-dup-custom-element.html
(right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/import-dup-custom-element.html#newcode22
LayoutTests/http/tests/htmlimports/import-dup-custom-element.html:22: --
|using-custom-element-1.html| reaches |def-custom-element.html| with one
indirection
in direction -> indirection?

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/import-dup-custom-element.html#newcode22
LayoutTests/http/tests/htmlimports/import-dup-custom-element.html:22: --
|using-custom-element-1.html| reaches |def-custom-element.html| with one
indirection
I'm not crazy about the -- prefixes. Just left align and enjoy the extra
four character line length.

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
File
LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
(right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/resources/custom-element-def.html#newcode6
LayoutTests/http/tests/htmlimports/resources/custom-element-def.html:6:
proto.createdCallback = function () {
It's my bad habit to write function () { but I *think* the preferred
style is function() {

https://codereview.chromium.org/238923009/diff/20001/Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h
File Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h
(right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h#newcode41
Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h:41: class
RefCountedCustomElementMicrotaskQueue : public
RefCounted<RefCountedCustomElementMicrotaskQueue>, public
CustomElementMicrotaskQueue {
What would it look like if we just made the CustomElementMicrotaskQueue
refcounted? There's only two places this is used, right? One is in the
JavaScript context for the "root" queue. And another in an import. Or am
I missing something?

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImport.cpp
File Source/core/html/imports/HTMLImport.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImport.cpp#newcode47
Source/core/html/imports/HTMLImport.cpp:47: bool
HTMLImport::isPreceding(HTMLImport* import)
Maybe "precedes"?

this->precedes(that)

seems unambiguous, but

this->isPreceding(that)

is awkward...

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportChild.cpp
File Source/core/html/imports/HTMLImportChild.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportChild.cpp#newcode154
Source/core/html/imports/HTMLImportChild.cpp:154:
m_customElementMicrotaskStep = CustomElement::didCreateImport(this);
I like the simplicity of this. Its a bit wasteful to create the
microtask queue for an import that doesn't have Custom Elements, but I
suppose many imports will have Custom Elements in practice?

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportsController.cpp
File Source/core/html/imports/HTMLImportsController.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportsController.cpp#newcode82
Source/core/html/imports/HTMLImportsController.cpp:82: static bool
makesCyclce(HTMLImport* parent, const KURL& url)
Spelling: cycle

https://codereview.chromium.org/238923009/

[mor...@chromium.org]

Thanks or the thorough review! Landing with fix.
I added scope re-enter detector for the assertion.

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/import-dup-custom-element.html
File LayoutTests/http/tests/htmlimports/import-dup-custom-element.html
(right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/import-dup-custom-element.html#newcode22
LayoutTests/http/tests/htmlimports/import-dup-custom-element.html:22: --
|using-custom-element-1.html| reaches |def-custom-element.html| with one
indirection

On 2014/04/18 00:56:25, dominicc wrote:
> in direction -> indirection?
Rietveld wraps it badly. The real code doesn't have no space here.

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/import-dup-custom-element.html#newcode22
LayoutTests/http/tests/htmlimports/import-dup-custom-element.html:22: --
|using-custom-element-1.html| reaches |def-custom-element.html| with one
indirection

On 2014/04/18 00:56:25, dominicc wrote:
> I'm not crazy about the -- prefixes. Just left align and enjoy the
extra four
> character line length.

Done.

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
File
LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
(right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/htmlimports/resources/custom-element-def.html#newcode6
LayoutTests/http/tests/htmlimports/resources/custom-element-def.html:6:
proto.createdCallback = function () {

On 2014/04/18 00:56:25, dominicc wrote:
> It's my bad habit to write function () { but I *think* the preferred
style is
> function() {

Done.

https://codereview.chromium.org/238923009/diff/20001/Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h
File Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h
(right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h#newcode41
Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h:41: class
RefCountedCustomElementMicrotaskQueue : public
RefCounted<RefCountedCustomElementMicrotaskQueue>, public
CustomElementMicrotaskQueue {

On 2014/04/18 00:56:26, dominicc wrote:
> What would it look like if we just made the
CustomElementMicrotaskQueue
> refcounted? There's only two places this is used, right? One is in the
> JavaScript context for the "root" queue. And another in an import. Or
am I
> missing something?

Right. Done.

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImport.cpp
File Source/core/html/imports/HTMLImport.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImport.cpp#newcode47
Source/core/html/imports/HTMLImport.cpp:47: bool
HTMLImport::isPreceding(HTMLImport* import)

On 2014/04/18 00:56:26, dominicc wrote:
> Maybe "precedes"?

> this->precedes(that)

> seems unambiguous, but

> this->isPreceding(that)

> is awkward...

Let me take precedes() here.

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportChild.cpp
File Source/core/html/imports/HTMLImportChild.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportChild.cpp#newcode154
Source/core/html/imports/HTMLImportChild.cpp:154:
m_customElementMicrotaskStep = CustomElement::didCreateImport(this);

On 2014/04/18 00:56:26, dominicc wrote:
> I like the simplicity of this. Its a bit wasteful to create the
microtask queue
> for an import that doesn't have Custom Elements, but I suppose many
imports will
> have Custom Elements in practice?

That's my thinking, and making it lazy complicates the lifecycle
management and dependency.

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportsController.cpp
File Source/core/html/imports/HTMLImportsController.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports/HTMLImportsController.cpp#newcode82
Source/core/html/imports/HTMLImportsController.cpp:82: static bool
makesCyclce(HTMLImport* parent, const KURL& url)

On 2014/04/18 00:56:26, dominicc wrote:
> Spelling: cycle

Done.

https://codereview.chromium.org/238923009/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mor...@chromium.org/238923009/60001


https://codereview.chromium.org/238923009/

[commi...@chromium.org]

Try jobs failed on following builders:
tryserver.blink on mac_blink_rel

https://codereview.chromium.org/238923009/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mor...@chromium.org/238923009/80001


https://codereview.chromium.org/238923009/

[commi...@chromium.org]

Failed to apply patch for Source/core/html/HTMLLinkElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/html/HTMLLinkElement.cpp
Hunk #1 FAILED at 231.
1 out of 1 hunk FAILED -- saving rejects to file
Source/core/html/HTMLLinkElement.cpp.rej

Patch: Source/core/html/HTMLLinkElement.cpp
Index: Source/core/html/HTMLLinkElement.cpp
diff --git a/Source/core/html/HTMLLinkElement.cpp
b/Source/core/html/HTMLLinkElement.cpp
index
11257d8cbd118da2b48794182e2fbeb9f6214b83..361d841e69368bb26c98d5a8e2829aea134c4ee1
100644
--- a/Source/core/html/HTMLLinkElement.cpp
+++ b/Source/core/html/HTMLLinkElement.cpp
@@ -231,14 +231,6 @@ LinkImport* HTMLLinkElement::linkImport() const
return static_cast<LinkImport*>(m_link.get());
}

-bool HTMLLinkElement::importOwnsLoader() const
-{
- LinkImport* import = linkImport();
- if (!import)
- return false;
- return import->ownsLoader();
-}
-
Document* HTMLLinkElement::import() const
{
if (LinkImport* link = linkImport())


https://codereview.chromium.org/238923009/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mor...@chromium.org/238923009/100001


https://codereview.chromium.org/238923009/

[commi...@chromium.org]

Change committed as 171966

https://codereview.chromium.org/238923009/