Implement proposed shadow tree cascade order.

Implement proposed shadow tree cascade order.

This CL implements the shadow tree cascade order proposed in [1].

Previously, in Blink, specificity would win over scope origin, even if the
scopes had an inner/outer scope relationship, and the order-of-appearance
was governed by the CascadeOrder type. Also, !important rules did not
apply in the reverse scope order, as the current spec says for inner/outer
scopes, and the proposal in [1] says apply between all shadow scopes.

What has been done is:

1. CascadeOrder is not used, as it represents order-of-appearance
   (Removal of CascadeOrder is not done here to make the CL smaller. Will
   be removed in a follow-up CL).

2. When collecting rules, sort after each scope instead of just after UA
   and Author since we had:

   UA important
   Author important
   Author
   UA

   and now we have (with A(n) appearing before A(n+1) in the tree-of-trees
   order):

   UA important
   Author scope A(n) important
   ...
   Author scope A(1) important
   Author scope A(1)
   ...
   Author scope A(n)
   UA

   The applyProperties code is hot, and I have made performance runs for
   the micro-benchmarks in Layout and CSS without consistent regressions.

3. Since the cascading order between scopes are just the inner/outer
   relationship in the composed tree (direction decided by !important),
   which is the same as the tree-of-trees order of the shadow trees,
   we can just traverse the DocumentOrderedList of scopes in the reverse
   order instead of doing calculation tricks for CascadeOrder values.

   Because of this, TreeBoundaryCrossingRules is now reduced to a
   DocumentOrderedList of scoping nodes, so the TreeBoundaryCrossingRules
   class is removed.

[1] https://lists.w3.org/Archives/Public/www-style/2015Jun/0303.html

BUG=452542, 455148, 487125
NOTRY=true

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

[rune, 2015-08-18 13:17:21]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 13:17:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/1

[rune, 2015-08-18 13:31:26]

rune@opera.com changed reviewers:
+ kochi@chromium.org

[rune, 2015-08-18 13:31:27]

Split out from https://codereview.chromium.org/1224673002/

[commit-bot: I haz the power, 2015-08-18 14:23:28]

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

[commit-bot: I haz the power, 2015-08-18 14:23:28]

Dry run: This issue passed the CQ dry run.

[kochi, 2015-08-19 10:29:25]

Hi Rune,

I'm still understanding the change here.
Could you answer my queries?

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:370: // Adding scoped resolvers for
active shadow roots for shadow host styling.
Nit: This comment seems copied from
collectScopedResolversForHostedShadowTrees(),
but the following lines do more than that?

Could you fix the comment to match what this code does?

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:392:
collector.finishAddingAuthorRulesForTreeScope();
Why this line can't be in the if() {} above?

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:398: bool matchElementScope = true;
nit: how about bool matchElementScopeDone = false; ?

I read this variable as "flag that indicates rule matching for element is still
necessary".

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:407: if
(matchElementScopeRules(element, collector, includeEmptyRules) == resolver)
This looks tricky. Could you add comment why the scoped style resolver used in
the matchElementScopeRules() is same as current loop, skip the rest of the loop?

Or

ScopedStyleResolver* someExplanatoryVariableName = matchElementScopeRules(...);
if (someExlanatoryVariableName == resolver)
    continue;

?

[rune, 2015-08-19 16:37:22]

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:370: // Adding scoped resolvers for
active shadow roots for shadow host styling.
On 2015/08/19 10:29:24, Takayoshi Kochi wrote:
> Nit: This comment seems copied from
> collectScopedResolversForHostedShadowTrees(),
> but the following lines do more than that?
> 
> Could you fix the comment to match what this code does?

I've just dropped the comment.

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:392:
collector.finishAddingAuthorRulesForTreeScope();
On 2015/08/19 10:29:24, Takayoshi Kochi wrote:
> Why this line can't be in the if() {} above?

Yes it could. I've restructured the code in patch set 2.

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:398: bool matchElementScope = true;
On 2015/08/19 10:29:24, Takayoshi Kochi wrote:
> nit: how about bool matchElementScopeDone = false; ?
> 
> I read this variable as "flag that indicates rule matching for element is
still
> necessary".

Done.

https://codereview.chromium.org/1298173004/diff/1/Source/core/css/resolver/St...
Source/core/css/resolver/StyleResolver.cpp:407: if
(matchElementScopeRules(element, collector, includeEmptyRules) == resolver)
On 2015/08/19 10:29:24, Takayoshi Kochi wrote:
> This looks tricky. Could you add comment why the scoped style resolver used in
> the matchElementScopeRules() is same as current loop, skip the rest of the
loop?
> 
> Or
> 
> ScopedStyleResolver* someExplanatoryVariableName =
matchElementScopeRules(...);
> if (someExlanatoryVariableName == resolver)
>     continue;
> 
> ?

I've restructured the code a bit, and added documentation. Take a look at patch
set 2 and see what you think.

[kochi, 2015-08-20 13:10:02]

.cpp / .h code mostly look good.
I'm still reviewing the layout tests.
Sorry for the slow turnaround ...

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/de...
File LayoutTests/fast/css/deep-cascade-order.html (right):

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/de...
LayoutTests/fast/css/deep-cascade-order.html:22: debug("Currently fails because
style attributes are added to the end, not in its element's scope's cascade
order");
Could you add TODO(rune) here?

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/ge...
File LayoutTests/fast/css/getComputedStyle/computed-style-redistribution.html
(right):

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/ge...
LayoutTests/fast/css/getComputedStyle/computed-style-redistribution.html:4:
custom-element { color: red }
My understanding is that the previous selector (.d1, .d2) was weaker
than one in the shadow (::content div), but with this change outer one wins,
but if you replace ".d1, .d2" with "custom-element", the <div>s are
styled via inheritance from custom-element, so always the style from
shadow (::content div) wins, right?

I think this change doesn't change what this test was meant to be, so IIUC
this change is okay.

https://codereview.chromium.org/1298173004/diff/20001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1298173004/diff/20001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:382: void
StyleResolver::matchElementScopeRules(ScopedStyleResolver* elementScopeResolver,
ElementRuleCollector& collector, bool includeEmptyRules)
You should pass reference instead of pointer to
omit assertion on line 384.

[kochi, 2015-08-21 02:07:27]

lgtm if the previous comments are addressed.

Reviewed all the layout tests here.

[rune, 2015-08-21 07:37:46]

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/de...
File LayoutTests/fast/css/deep-cascade-order.html (right):

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/de...
LayoutTests/fast/css/deep-cascade-order.html:22: debug("Currently fails because
style attributes are added to the end, not in its element's scope's cascade
order");
On 2015/08/20 13:10:02, Takayoshi Kochi wrote:
> Could you add TODO(rune) here?

Done.

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/ge...
File LayoutTests/fast/css/getComputedStyle/computed-style-redistribution.html
(right):

https://codereview.chromium.org/1298173004/diff/20001/LayoutTests/fast/css/ge...
LayoutTests/fast/css/getComputedStyle/computed-style-redistribution.html:4:
custom-element { color: red }
On 2015/08/20 13:10:02, Takayoshi Kochi wrote:
> My understanding is that the previous selector (.d1, .d2) was weaker
> than one in the shadow (::content div), but with this change outer one wins,
> but if you replace ".d1, .d2" with "custom-element", the <div>s are
> styled via inheritance from custom-element, so always the style from
> shadow (::content div) wins, right?
> 
> I think this change doesn't change what this test was meant to be, so IIUC
> this change is okay.

You're right. I had to change the control style for red because the outer-wins
bugfix would otherwise always make this red.

https://codereview.chromium.org/1298173004/diff/20001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1298173004/diff/20001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:382: void
StyleResolver::matchElementScopeRules(ScopedStyleResolver* elementScopeResolver,
ElementRuleCollector& collector, bool includeEmptyRules)
On 2015/08/20 13:10:02, Takayoshi Kochi wrote:
> You should pass reference instead of pointer to
> omit assertion on line 384.

Done.

[rune, 2015-08-21 07:38:36]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-21 07:38:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/1298173004/#ps40001
(title: "Fixed review issues")

[commit-bot: I haz the power, 2015-08-21 07:38:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/40001

[commit-bot: I haz the power, 2015-08-21 08:54:57]

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

[commit-bot: I haz the power, 2015-08-21 08:54:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kochi, 2015-08-21 08:55:21]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-08-21 08:55:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/40001

[rune, 2015-08-21 09:00:26]

On 2015/08/21 08:54:57, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Needs a new expected result for the changed TODO (rune... message.

[rune, 2015-08-21 09:00:41]

The CQ bit was unchecked by rune@opera.com

[rune, 2015-08-21 09:08:31]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-21 09:08:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/1298173004/#ps60001
(title: "Fixed expected results and added another TODO.")

[commit-bot: I haz the power, 2015-08-21 09:08:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/60001

[commit-bot: I haz the power, 2015-08-21 11:08:04]

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

[commit-bot: I haz the power, 2015-08-21 11:08:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rune, 2015-08-21 11:14:59]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-08-21 11:15:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/60001

[commit-bot: I haz the power, 2015-08-21 13:18:11]

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

[commit-bot: I haz the power, 2015-08-21 13:18:13]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rune, 2015-08-21 14:02:55]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-08-21 14:03:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/60001

[commit-bot: I haz the power, 2015-08-21 16:14:13]

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

[commit-bot: I haz the power, 2015-08-21 16:14:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rune, 2015-08-21 17:19:57]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-08-21 17:20:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1298173004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1298173004/60001

[commit-bot: I haz the power, 2015-08-21 17:25:20]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200994

[rune, 2015-08-21 17:26:32]

On 2015/08/21 17:25:20, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=200994

Landed with notry=true due to crbug.com/523229