HTML Imports: No more BlockingDocumentCreation.

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=dominicc@chromium.org
BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171966

[Hajime Morrita, 2014-04-17 00:21:09]

PTAL? This is what we discussed last week.

[dominicc (has gone to gerrit), 2014-04-18 00:56:25]

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/h...
File LayoutTests/http/tests/htmlimports/import-dup-custom-element.html (right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/h...
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/h...
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/h...
File LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
(right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/h...
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/R...
File Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/dom/custom/R...
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...
File Source/core/html/imports/HTMLImport.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports...
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...
File Source/core/html/imports/HTMLImportChild.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports...
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...
File Source/core/html/imports/HTMLImportsController.cpp (right):

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

[Hajime Morrita, 2014-04-18 01:48:32]

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/h...
File LayoutTests/http/tests/htmlimports/import-dup-custom-element.html (right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/h...
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/h...
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/h...
File LayoutTests/http/tests/htmlimports/resources/custom-element-def.html
(right):

https://codereview.chromium.org/238923009/diff/20001/LayoutTests/http/tests/h...
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/R...
File Source/core/dom/custom/RefCountedCustomElementMicrotaskQueue.h (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/dom/custom/R...
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...
File Source/core/html/imports/HTMLImport.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports...
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...
File Source/core/html/imports/HTMLImportChild.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports...
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...
File Source/core/html/imports/HTMLImportsController.cpp (right):

https://codereview.chromium.org/238923009/diff/20001/Source/core/html/imports...
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.

[Hajime Morrita, 2014-04-18 01:49:33]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-04-18 01:49:45]

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

[commit-bot: I haz the power, 2014-04-18 02:00:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 02:00:13]

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

[Hajime Morrita, 2014-04-18 18:12:22]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-04-18 18:12:34]

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

[commit-bot: I haz the power, 2014-04-18 18:12:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 18:12:54]

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())

[Hajime Morrita, 2014-04-18 18:25:39]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-04-18 18:25:54]

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

[commit-bot: I haz the power, 2014-04-18 19:37:35]

Change committed as 171966