[import] support document.stylesheets in imported documents.

[import] support document.stylesheets in imported documents.

This change adds document.stylesheets support. Before this change,
these these sheets were just skipped during stylesheet update
traversal. This change lets the traversal recursively update StyleEngine
of imported document. In the update, the stylesheet value is computed.

Note that as activeStyleSheet of the master document have to contains
sheets in the import tree, this change turns DocumentStyleSheetCollector
to a carrier of stylsheet list to be updated: The collector doesn't own
its own list and have a reference to lists, that are provided by StyleEngine
or DocumentStyleSheetCollection.

BUG=334819
TEST=import-style-basic.html
R=esprehn

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

[Hajime Morrita, 2014-01-23 07:15:45]

PTAL?

[esprehn, 2014-01-23 07:51:02]

It seems like there's some abstraction issue here, each StyleEngine should
manage the sheets for it's own document (and it's own styleSheets list). The top
level one shouldn't need to do the traversing or the collecting for sub
documents.

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
File Source/core/dom/DocumentStyleSheetCollection.cpp (right):

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
Source/core/dom/DocumentStyleSheetCollection.cpp:158: for
(DocumentStyleSheetCollector::SheetListMapIteraor i =
collector.sheetListMap().begin(); i != collector.sheetListMap().end(); ++i)
typo, you want SheetListMapIterator (missing t)

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
File Source/core/dom/DocumentStyleSheetCollector.cpp (right):

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
Source/core/dom/DocumentStyleSheetCollector.cpp:49: void
DocumentStyleSheetCollector::appendSheetForList(StyleSheet* sheet)
reference?

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
Source/core/dom/DocumentStyleSheetCollector.cpp:67: return
m_sheetListMap.contains(document);
This seems like the wrong abstraction, the StyleEngine at the top should just
ask each style engine down the import tree to collect the sheets and keep their
own candidate nodes. No StyleEngine should be looking at other documents.

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/StyleSheetCo...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/StyleSheetCo...
Source/core/dom/StyleSheetCollection.cpp:60: m_styleSheetsForStyleSheetList =
sheets;
swap() I think is what you want, this currently does a copy.

[Hajime Morrita, 2014-01-24 01:25:50]

PTAL?

Updated to get saner abstraction (I hope).
See the issue description for overview.

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
File Source/core/dom/DocumentStyleSheetCollection.cpp (right):

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
Source/core/dom/DocumentStyleSheetCollection.cpp:158: for
(DocumentStyleSheetCollector::SheetListMapIteraor i =
collector.sheetListMap().begin(); i != collector.sheetListMap().end(); ++i)
On 2014/01/23 07:51:03, esprehn wrote:
> typo, you want SheetListMapIterator (missing t)

Done.

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
File Source/core/dom/DocumentStyleSheetCollector.cpp (right):

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
Source/core/dom/DocumentStyleSheetCollector.cpp:49: void
DocumentStyleSheetCollector::appendSheetForList(StyleSheet* sheet)
On 2014/01/23 07:51:03, esprehn wrote:
> reference?
This is aligned to DocumentStyleSheetCollection API

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/DocumentStyl...
Source/core/dom/DocumentStyleSheetCollector.cpp:67: return
m_sheetListMap.contains(document);
On 2014/01/23 07:51:03, esprehn wrote:
> This seems like the wrong abstraction, the StyleEngine at the top should just
> ask each style engine down the import tree to collect the sheets and keep
their
> own candidate nodes. No StyleEngine should be looking at other documents.
Makes sense. Changed the approach.

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/StyleSheetCo...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/143463011/diff/1/Source/core/dom/StyleSheetCo...
Source/core/dom/StyleSheetCollection.cpp:60: m_styleSheetsForStyleSheetList =
sheets;
On 2014/01/23 07:51:03, esprehn wrote:
> swap() I think is what you want, this currently does a copy.

Done.

[esprehn, 2014-01-24 09:30:10]

lgtm, returning that object by value is super weird, it seems like you want to
not create the "Base" thing at all and just create the right object type?

https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/Documen...
File Source/core/dom/DocumentStyleSheetCollection.cpp (right):

https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/Documen...
Source/core/dom/DocumentStyleSheetCollection.cpp:127:
DocumentStyleSheetCollector collector = collection.collector();
This is a lot of copying, why not just create the Collector directly or use a
reference here?

It's also strange to create an instance of something that ends in "Base".

https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/StyleSh...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/StyleSh...
Source/core/dom/StyleSheetCollection.cpp:56: void
StyleSheetCollectionBase::swapSheetsForSheetList(Vector<RefPtr<StyleSheet> >&
sheets)
We should rename this something like StyleSheetCollection if you plan to create
instances of it. Usually "Base" things have protected constructors and you can't
create them like you did.

https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/StyleSh...
Source/core/dom/StyleSheetCollection.cpp:78: DocumentStyleSheetCollector
StyleSheetCollectionBase::collector()
More copying, why not just create the DocumentStyleSheetCollector on the stack?
It's weird to return an object like this by value.

[commit-bot: I haz the power, 2014-01-24 10:48:03]

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

[Hajime Morrita, 2014-01-24 10:50:12]

On 2014/01/24 09:30:10, esprehn wrote:
> lgtm, returning that object by value is super weird, it seems like you want to
> not create the "Base" thing at all and just create the right object type?
> 
>
https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/Documen...
> File Source/core/dom/DocumentStyleSheetCollection.cpp (right):
> 
>
https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/Documen...
> Source/core/dom/DocumentStyleSheetCollection.cpp:127:
> DocumentStyleSheetCollector collector = collection.collector();
> This is a lot of copying, why not just create the Collector directly or use a
> reference here?
> 
> It's also strange to create an instance of something that ends in "Base".
> 
>
https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/StyleSh...
> File Source/core/dom/StyleSheetCollection.cpp (right):
> 
>
https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/StyleSh...
> Source/core/dom/StyleSheetCollection.cpp:56: void
> StyleSheetCollectionBase::swapSheetsForSheetList(Vector<RefPtr<StyleSheet> >&
> sheets)
> We should rename this something like StyleSheetCollection if you plan to
create
> instances of it. Usually "Base" things have protected constructors and you
can't
> create them like you did.
Yeah... and I found that there is a couple of FIXME to do that :-/
The rename CL will follow.

> 
>
https://codereview.chromium.org/143463011/diff/140001/Source/core/dom/StyleSh...
> Source/core/dom/StyleSheetCollection.cpp:78: DocumentStyleSheetCollector
> StyleSheetCollectionBase::collector()
> More copying, why not just create the DocumentStyleSheetCollector on the
stack?
> It's weird to return an object like this by value.
It need to do that because the collector class have to access private fields of
the collection class.
Updated CL uses friend class instead to avoid copying.

[commit-bot: I haz the power, 2014-01-24 13:39:11]

Change committed as 165741