First step of HTMLImports

Implement bare-bones of HTMLImports

This CL includes following parts of HTMLImports

- HTMLLinkElement.import property
- Single-level imports loading
- Duplication management - Imports of same URL point same DocumentFragment
- Proper CORS control

But it doesn't have:

- Sub-import loading,
- Anything more than DOM tree construction (like running script, etc.),
- Reasonable performance: No progressive HTML parsing, no prefetching.

This change introduces two major classes:
HTMLImportsController and LinkImport:

- The controller is a per-master-document registry of imports. It tracks
  all imports in the master document.
- LinkImport is a dual of LinkStyle. It manages per-element states
  like imported document fragment. LinkImport is a client of
  CachedResource.

Several parts of the core module are extended to adopt HTMLImports:

- HTMLScriptRunner and StyleElement becomes aware of blocking imports.
  They pauses until all imports, not only stylesheets, are loaded before execution.
- HTMLLinkElement adopts rel=import attribute and create
  a LinkImport instance instead of one of LinkStyle for the attribute.
- LinkResource superclass is pulled out from LinkStyle
  and new LinkImport derives from it.

BUG=240592
TEST=fast/html/imports/, http/tests/htmlimports/
R=dglazkov@chromium.org

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

[Hajime Morrita, 2013-05-23 10:42:45]

This is not ready yet. I need to investigate how CORS things work and 
write some tsets for that. But I believe this is almost shaped.

[dglazkov, 2013-05-23 18:31:59]

Thanks for working on this. Some comments:

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.cpp
File Source/core/html/HTMLImports.cpp (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.cpp:110: m_cachedDocument =
m_owner->document()->cachedResourceLoader()->requestHTMLDocument(request);
I don't know if we need to be creating a new document for each load. Do we have
to?

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.h
File Source/core/html/HTMLImports.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.h:89: class HTMLImports {
I was sort of expecting to find the imported fragment owner here, but I don't
see it. What's the ownerDocument of the elements, loaded by import?

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.h:109: LinkList m_links;
Is this the loaded import list? I am not sure...

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImportsH...
File Source/core/html/HTMLImportsHost.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImportsH...
Source/core/html/HTMLImportsHost.h:39: class HTMLImportsHost {
ImportsMasterDocument maybe? :)

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkRelAttri...
File Source/core/html/LinkRelAttribute.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkRelAttri...
Source/core/html/LinkRelAttribute.h:51: bool isImport() const { return
m_isImport; }
Don't forget to add a test to core/tests/LinkRelAttribute.cpp :)

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkResource.h
File Source/core/html/LinkResource.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkResource...
Source/core/html/LinkResource.h:63: class LinkRequestBuilder {
I wonder if this could be just folded into LinkResource?

[Hajime Morrita, 2013-05-24 02:28:27]

Thanks for the review! I'll come back soon.

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.cpp
File Source/core/html/HTMLImports.cpp (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.cpp:110: m_cachedDocument =
m_owner->document()->cachedResourceLoader()->requestHTMLDocument(request);
That's what I've been wondering, too. There are some tradeoff.

- Using CachedDocument means the Document is cached. we don't need to re-parse
it when the cache is hit.
   We cannot do it with DocumetFragment because we cannot define the
ownerDocument of cached DocumentFragment.
- If we're going to do progressive parsing later, we can do it using
DocumetWriter. DocumentWriter produces Document.
- Caching Document will be memory-heavy. So it might better off not do that.

My feeling is that we can use CachedDocument to keep CL small, then come back
later to explore better option.

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.h
File Source/core/html/HTMLImports.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.h:89: class HTMLImports {
That is HTMLImport::m_owner Document, which is created per HTMLImports without
browsing context.
We do adoptNode() created document fragment in createFragmentFrom().

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.h:109: LinkList m_links;
On 2013/05/23 18:32:00, Dimitri Glazkov wrote:
> Is this the loaded import list? I am not sure...
This is a list of LinkImports which are loaded or being loaded. Should have
better name. Let me think.

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImportsH...
File Source/core/html/HTMLImportsHost.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImportsH...
Source/core/html/HTMLImportsHost.h:39: class HTMLImportsHost {
I'd keep Document concept away.
But you're right. It'd better to pick terms from the spec.
I'll rename this to HTMLImportsMaster.

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkRelAttri...
File Source/core/html/LinkRelAttribute.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkRelAttri...
Source/core/html/LinkRelAttribute.h:51: bool isImport() const { return
m_isImport; }
Oh right! Will do.

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkResource.h
File Source/core/html/LinkResource.h (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/LinkResource...
Source/core/html/LinkResource.h:63: class LinkRequestBuilder {
That makes LinkResource bigger. I'd make this a short-living object.

[dglazkov, 2013-05-24 03:15:16]

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.cpp
File Source/core/html/HTMLImports.cpp (right):

https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
Source/core/html/HTMLImports.cpp:110: m_cachedDocument =
m_owner->document()->cachedResourceLoader()->requestHTMLDocument(request);
On 2013/05/24 02:28:27, morrita1 wrote:
>    We cannot do it with DocumetFragment because we cannot define the
> ownerDocument of cached DocumentFragment.

Can you explain why?

> - If we're going to do progressive parsing later, we can do it using
> DocumetWriter. DocumentWriter produces Document.

Maybe we should invent DocumentFragmentWriter? :)

> - Caching Document will be memory-heavy. So it might better off not do that.
> 
> My feeling is that we can use CachedDocument to keep CL small, then come back
> later to explore better option.

My only worry here is that if we do this later, we won't be able to explore
feasibility of the better option, and then we'll be stuck with a terrible spec
:)

[Hajime Morrita, 2013-05-24 03:31:15]

On 2013/05/24 03:15:16, Dimitri Glazkov wrote:
>
https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.cpp
> File Source/core/html/HTMLImports.cpp (right):
> 
>
https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
> Source/core/html/HTMLImports.cpp:110: m_cachedDocument =
> m_owner->document()->cachedResourceLoader()->requestHTMLDocument(request);
> On 2013/05/24 02:28:27, morrita1 wrote:
> >    We cannot do it with DocumetFragment because we cannot define the
> > ownerDocument of cached DocumentFragment.
> 
> Can you explain why?

The cached DocumentFragment lives even after the document which requests
the import is gone. We need alternative owner document which lives
across Frame lifetime, which seems complicated to me.

> 
> > - If we're going to do progressive parsing later, we can do it using
> > DocumetWriter. DocumentWriter produces Document.
> 
> Maybe we should invent DocumentFragmentWriter? :)

And DocumentFragmentParser? It will be a big deal :-/


> 
> > - Caching Document will be memory-heavy. So it might better off not do that.
> > 
> > My feeling is that we can use CachedDocument to keep CL small, then come
back
> > later to explore better option.
> 
> My only worry here is that if we do this later, we won't be able to explore
> feasibility of the better option, 

This sounds like a valid concern.

Well, I don't think I understand whole your point here. Do you think it is
better to create DocumentFragment directly, even though we need to invent more
plumbing?

[dglazkov, 2013-05-25 20:15:55]

On 2013/05/24 03:31:15, morrita1 wrote:
> On 2013/05/24 03:15:16, Dimitri Glazkov wrote:
> >
>
https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports.cpp
> > File Source/core/html/HTMLImports.cpp (right):
> > 
> >
>
https://codereview.chromium.org/15856002/diff/1/Source/core/html/HTMLImports....
> > Source/core/html/HTMLImports.cpp:110: m_cachedDocument =
> > m_owner->document()->cachedResourceLoader()->requestHTMLDocument(request);
> > On 2013/05/24 02:28:27, morrita1 wrote:
> > >    We cannot do it with DocumetFragment because we cannot define the
> > > ownerDocument of cached DocumentFragment.
> > 
> > Can you explain why?
> 
> The cached DocumentFragment lives even after the document which requests
> the import is gone. We need alternative owner document which lives
> across Frame lifetime, which seems complicated to me.
> 
> > 
> > > - If we're going to do progressive parsing later, we can do it using
> > > DocumetWriter. DocumentWriter produces Document.
> > 
> > Maybe we should invent DocumentFragmentWriter? :)
> 
> And DocumentFragmentParser? It will be a big deal :-/
> 
> 
> > 
> > > - Caching Document will be memory-heavy. So it might better off not do
that.
> > > 
> > > My feeling is that we can use CachedDocument to keep CL small, then come
> back
> > > later to explore better option.
> > 
> > My only worry here is that if we do this later, we won't be able to explore
> > feasibility of the better option, 
> 
> This sounds like a valid concern.
> 
> Well, I don't think I understand whole your point here. Do you think it is
> better to create DocumentFragment directly, even though we need to invent more
> plumbing?

I think we should try that, even if not in this patch. I looked over the loader
code, and I can see definite issues with trying to invent
CachedDocumentFragment. We should use these challenge to get a better sense of
direction we should take with the spec. Let's continue with this patch, but keep
in mind that:

1) we should aim to avoid keeping an extra CachedDocument instance for each
import. The whole point of turning them into DocumentFragments in the spec was
to try and optimize memory use.

2) we should aim to avoid cloning the DocumentFragment, since that will be
performance-sensitive for large set of dependencies.

[Hajime Morrita, 2013-05-27 08:15:10]

Thanks for sharing your thought, Dimitri! I think I'm getting your point.

Considering that imported documents won't be cached in DOM state, 
I feel that we could treat them mostly like scripts - cached as text, parsed for
each page load.
I simplified the patch in that perspective.

In new patch, a temporal Document is created for each import.  It takes care of
parsing.
If we support incremental parsing, the lifetime of the document will become a
bit longer, 
but I think the approach will be basically same as this CL.

[dglazkov, 2013-05-28 17:12:47]

This is really taking a good shape.

Also, this is helping me tremendously in shaping the spec. Thank you for doing
this!

Here are my nits/comments:

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/Document....
Source/core/dom/Document.cpp:868: HTMLImports* Document::imports()
Don't we usually use ensure* pattern for RIIA accessors?

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/Document....
Source/core/dom/Document.cpp:2665: if (!haveStylesheetsAndImportsLoaded())
Why is this check here now, but not in the old code?

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/DocumentT...
File Source/core/dom/DocumentType.h (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/DocumentT...
Source/core/dom/DocumentType.h:1: 
Oops :)

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImports.cpp (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.cpp:179: , m_owner(HTMLDocument::create(0, KURL()))
THIS is where things will get super-crazy. Because we only have one imported
fragment owner, we can't adjust document location for multiple imports, which
means that we can't resolve URLs properly. See
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19870 for more info.

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImports.h (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.h:100: void showSecurityErrorMessage(const
String&);
I am somewhat queasy about the plumbing of errors through multiple functions
like that. Perhaps HTMLImportsMaster doesn't need to be a separate interface and
could be just Document*?

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.h:110: RefPtr<Document> m_owner;
m_owner collides with the typical naming pattern where "owner" is usually named
so because it's the owner (as in, held by OwnPtr) of the class. Let's
unabbreviate this to m_importedFragmentOwner to match spec terminology.

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.h:114: ImportList m_imports;
HTMLImports contains m_imports... Seems a bit redundant/confusing/non-DRY. Maybe
we should name HTMLImports->HTMLImportsController to be more descriptive?

[Hajime Morrita, 2013-05-29 01:36:00]

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/Document....
Source/core/dom/Document.cpp:868: HTMLImports* Document::imports()
On 2013/05/28 17:12:47, Dimitri Glazkov wrote:
> Don't we usually use ensure* pattern for RIIA accessors?

Done.

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/DocumentT...
File Source/core/dom/DocumentType.h (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/dom/DocumentT...
Source/core/dom/DocumentType.h:1: 
:-o

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImports.cpp (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.cpp:179: , m_owner(HTMLDocument::create(0, KURL()))
Probably we should have multiple master documents each of which represents a
"base" url,
and let each component being owned by corresponding master document. I'm not
sure if this works though.

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImports.h (right):

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.h:100: void showSecurityErrorMessage(const
String&);
My hope is to make HTMLImports unit-testable eventually.
But yes, this doesn't seem a good abstraction. Wll get rid of HTMLImportsMaster.

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.h:110: RefPtr<Document> m_owner;
On 2013/05/28 17:12:47, Dimitri Glazkov wrote:
> m_owner collides with the typical naming pattern where "owner" is usually
named
> so because it's the owner (as in, held by OwnPtr) of the class. Let's
> unabbreviate this to m_importedFragmentOwner to match spec terminology.

Done.

https://codereview.chromium.org/15856002/diff/17001/Source/core/html/HTMLImpo...
Source/core/html/HTMLImports.h:114: ImportList m_imports;
On 2013/05/28 17:12:47, Dimitri Glazkov wrote:
> HTMLImports contains m_imports... Seems a bit redundant/confusing/non-DRY.
Maybe
> we should name HTMLImports->HTMLImportsController to be more descriptive?

Done.

[dglazkov, 2013-05-29 02:15:37]

LGTM, but this commit probably needs a better title before landing, like
"Implement HTML Imports" or "Hooray, the future comes to the Web platform!
Except it's still very unshaped and raw, so please be gentle" :)


Also:

https://codereview.chromium.org/15856002/diff/26001/Source/core/html/HTMLLink...
File Source/core/html/HTMLLinkElement.idl (right):

https://codereview.chromium.org/15856002/diff/26001/Source/core/html/HTMLLink...
Source/core/html/HTMLLinkElement.idl:37: readonly attribute DocumentFragment
import;
Whoops! Need the flag, right?

[commit-bot: I haz the power, 2013-05-29 07:38:29]

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

[commit-bot: I haz the power, 2013-05-29 07:55:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-29 08:42:30]

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

[commit-bot: I haz the power, 2013-05-29 09:00:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Hajime Morrita, 2013-05-29 23:25:18]

what's this :-(
I need to try it locally.

[commit-bot: I haz the power, 2013-05-30 08:42:01]

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

[commit-bot: I haz the power, 2013-05-30 09:36:03]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-30 23:58:03]

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

[commit-bot: I haz the power, 2013-05-31 04:41:40]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[Hajime Morrita, 2013-05-31 05:09:56]

Committed patchset #7 manually as r151523 (presubmit successful).