Make ScopedStyleResolver use apply-author-styles of a given element's treescope.

Make ScopedStyleResolver use apply-author-styles of a given element's treescope.

Suppose that a given shadow tree's apply-author-styles is true. Currently rules only in the nesting tree are applicable to elements in the shadow tree. However, rules in all enclosing trees should be applicable.

TEST=No new tests. Rebased the existing one.
BUG=244779
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151709

[hayato, 2013-05-29 10:21:54]

On 2013/05/29 10:18:49, tasak wrote:

I know the background of this change, but could you explain the motivation of
this change clearly in the description so that other reviewers know that?

[tasak, 2013-05-29 10:26:22]

On 2013/05/29 10:21:54, hayato wrote:
> On 2013/05/29 10:18:49, tasak wrote:
> 
> I know the background of this change, but could you explain the motivation of
> this change clearly in the description so that other reviewers know that?

Sure. Done.

[hayato, 2013-05-29 11:16:26]

Just a quick review. I'll take another look tomorrow.

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
File LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html
(right):

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:93:
debug('test a style in a shadow subtree is applied to a node in its descendant
shadow subtree when apply-author-styles in the shadow subtree is true.');
I feel 'the shadow tree' is ambiguous in this sentence.
It's not clear that which tree 'the shadow subtree'  in the clause of 'when
apply-author-styles in the shadow subtree...' points to.

It might be better to clarify that.

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
Source/core/css/resolver/ScopedStyleResolver.cpp:119: for ( ; scopeResolver;
scopeResolver = scopeResolver->parent())
Now we can remove L115-L117, that can be merged to the 'for' loop.

for (ScopedStyleResolver* scopeResolver = scopedResolverFor(element);
scopeResolver; scopeResolver = scopeResolver->parent())

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
File Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
Source/core/css/resolver/ScopedStyleResolver.h:122: void
resolveScopeStyles(const Element*, Vector<ScopedStyleResolver*, 8>& resolvers);
Looks like we can omit the parameter name of 'resolvers'.

[hayato, 2013-05-30 08:16:30]

It's optional and can be done as another patch, but I am feeling that we can use
reftest rather than dumping getComputedSytle() here. You might want to divide
each test into its own file.

WDYT?

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
File LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html
(right):

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:5:
<script src="../../dom/shadow/resources/polyfill.js"></script>
You can remove this line since polyfill.js is no longer available. :)

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:10:
shouldBeDefined("window.internals");
Looks like window.internals is not used.

[dglazkov, 2013-05-30 16:51:59]

I defer review to hayato.

[tasak, 2013-05-31 10:06:58]

Thank you for reviewing.

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
File LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html
(right):

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:5:
<script src="../../dom/shadow/resources/polyfill.js"></script>
On 2013/05/30 08:16:30, hayato wrote:
> You can remove this line since polyfill.js is no longer available. :)

Done.

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:10:
shouldBeDefined("window.internals");
On 2013/05/30 08:16:30, hayato wrote:
> Looks like window.internals is not used.

I rewrote this test by using shadow-dom.js, because it looks very dirty.

Since shadow-dom.js needs window.internals, I would like to keep this line.

https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:93:
debug('test a style in a shadow subtree is applied to a node in its descendant
shadow subtree when apply-author-styles in the shadow subtree is true.');
On 2013/05/29 11:16:27, hayato wrote:
> I feel 'the shadow tree' is ambiguous in this sentence.
> It's not clear that which tree 'the shadow subtree'  in the clause of 'when
> apply-author-styles in the shadow subtree...' points to.
> 
> It might be better to clarify that.

Sure. Probably done.

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
Source/core/css/resolver/ScopedStyleResolver.cpp:119: for ( ; scopeResolver;
scopeResolver = scopeResolver->parent())
On 2013/05/29 11:16:27, hayato wrote:
> Now we can remove L115-L117, that can be merged to the 'for' loop.
> 
> for (ScopedStyleResolver* scopeResolver = scopedResolverFor(element);
> scopeResolver; scopeResolver = scopeResolver->parent())

Done.

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
File Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
Source/core/css/resolver/ScopedStyleResolver.h:122: void
resolveScopeStyles(const Element*, Vector<ScopedStyleResolver*, 8>& resolvers);
On 2013/05/29 11:16:27, hayato wrote:
> Looks like we can omit the parameter name of 'resolvers'.

Done.

[hayato, 2013-06-03 01:02:01]

lgtm.

On 2013/05/31 10:06:58, tasak wrote:
> Thank you for reviewing.
> 
>
https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
> File LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html
> (right):
> 
>
https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
> LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:5:
> <script src="../../dom/shadow/resources/polyfill.js"></script>
> On 2013/05/30 08:16:30, hayato wrote:
> > You can remove this line since polyfill.js is no longer available. :)
> 
> Done.
> 
>
https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
> LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:10:
> shouldBeDefined("window.internals");
> On 2013/05/30 08:16:30, hayato wrote:
> > Looks like window.internals is not used.
> 
> I rewrote this test by using shadow-dom.js, because it looks very dirty.

Yeah, the tests became much readable than before.

> 
> Since shadow-dom.js needs window.internals, I would like to keep this line.
> 
>
https://codereview.chromium.org/16194002/diff/1/LayoutTests/fast/css/style-sc...
> LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:93:
> debug('test a style in a shadow subtree is applied to a node in its descendant
> shadow subtree when apply-author-styles in the shadow subtree is true.');
> On 2013/05/29 11:16:27, hayato wrote:
> > I feel 'the shadow tree' is ambiguous in this sentence.
> > It's not clear that which tree 'the shadow subtree'  in the clause of 'when
> > apply-author-styles in the shadow subtree...' points to.
> > 
> > It might be better to clarify that.
> 
> Sure. Probably done.
> 
>
https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
> File Source/core/css/resolver/ScopedStyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
> Source/core/css/resolver/ScopedStyleResolver.cpp:119: for ( ; scopeResolver;
> scopeResolver = scopeResolver->parent())
> On 2013/05/29 11:16:27, hayato wrote:
> > Now we can remove L115-L117, that can be merged to the 'for' loop.
> > 
> > for (ScopedStyleResolver* scopeResolver = scopedResolverFor(element);
> > scopeResolver; scopeResolver = scopeResolver->parent())
> 
> Done.
> 
>
https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
> File Source/core/css/resolver/ScopedStyleResolver.h (right):
> 
>
https://codereview.chromium.org/16194002/diff/1/Source/core/css/resolver/Scop...
> Source/core/css/resolver/ScopedStyleResolver.h:122: void
> resolveScopeStyles(const Element*, Vector<ScopedStyleResolver*, 8>&
resolvers);
> On 2013/05/29 11:16:27, hayato wrote:
> > Looks like we can omit the parameter name of 'resolvers'.
> 
> Done.

[commit-bot: I haz the power, 2013-06-03 04:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/16194002/10001

[commit-bot: I haz the power, 2013-06-03 05:47:56]

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...

[commit-bot: I haz the power, 2013-06-04 00:29:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/16194002/10001

[commit-bot: I haz the power, 2013-06-04 00:29:51]

Change committed as 151709