Prepare for multiple !important author ranges.

Prepare for multiple !important author ranges.

This CL is split out from [1] where we support multiple author origins in
terms of shadow dom scopes. The vector of MatchedProperties contains
matched declarations from all origins in the order in which they should
be applied. For !important declarations, they are applied in the same
order within the same origin, but the order of the origins are different
as the cascading order is:

  - UA important
  - Author important
  - Author
  - UA

We used to have two ranges, one for UA and one for author to accomplish
this. However, with the normal/!important cascading order for
declarations from different shadow doms, we need multiple author ranges:

  - UA important
  - Author important scope n
  ...
  - Author important scope 1
  - Author scope 1
  ...
  - Author scope n
  - UA

We introduce an ImportantAuthorRangeIterator to iterate through the
author ranges and add iterator-style iteration for each range through the
MatchedPropertiesRange class.

As the applyMatchedProperties code is hot, I've run the blink_perf.css
tests locally. Some improved by 2-4%, some became worse by similar
percentages.

[1] https://codereview.chromium.org/1224673002/

BUG=487125
TEST=fast/css

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

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

[rune, 2015-08-10 22:59:11]

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

[kochi, 2015-08-11 06:49:37]

On 2015/08/10 22:59:12, rune wrote:

Is this okay to review?
I applied patch set 3, webkit_unit_tests and content_shell dies immediately
after assertion.

Anyway I continue to look at this.

[rune, 2015-08-11 09:28:46]

On 2015/08/11 06:49:37, Takayoshi Kochi wrote:
> On 2015/08/10 22:59:12, rune wrote:
> 
> Is this okay to review?
> I applied patch set 3, webkit_unit_tests and content_shell dies immediately
> after assertion.

Sorry, an off-by-one problem in the ASSERT. Fixed now.

> Anyway I continue to look at this.

Thanks.

[kochi, 2015-08-11 11:07:35]

Thanks for splitting the CL!
It made my life a lot easier.

This still doesn't have tests, and if I run layout tests
against this, lots of failures (esp. fast/forms).

This is not intended to submit as is, but for review, right?

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:91: void uaRulesFinished();
It sounds a bit strange to have "Finished" suffix as it sounds
returning a boolean value.  Actually this method changes the
internal state of MatchedResult object, so verb+noun sounds
better (note: I'm not a native English speaker) to me.

What about finishAddingUARules()?

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:92: void authorScopeFinished();
Ditto, and why "Scope", not "Rules"?

What about finishAddingAuthorRules()?

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:108: Vector<unsigned, 16>
m_authorRangeEnd;
At first I was confused why m_uaRangeEnd is a scaler while m_authorRangeEnd
is a vector.  Maybe m_authorRangeEnds?

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

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1233: if (range.begin() ==
range.end())
Maybe we can have isEmpty() for MatchedPropertiesRange.

[rune, 2015-08-11 11:44:56]

On 2015/08/11 11:07:35, Takayoshi Kochi wrote:
> Thanks for splitting the CL!
> It made my life a lot easier.
> 
> This still doesn't have tests, and if I run layout tests

This CL should not make any observable changes, so I thought the existing
LayoutTests would cover it. I can add some unit tests to check that I get the
ranges I expect for given style input.

> against this, lots of failures (esp. fast/forms).

I don't see any failures in fast/forms apart from an image failure for
fast/forms/select/popup-menu-appearance-tall.html which failed before I applied
my changes.

> This is not intended to submit as is, but for review, right?

It was intended for submission, but I should make tests. I haven't seen the test
failures you've seen (I've run the fast/ directory).

>
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
> File Source/core/css/resolver/MatchResult.h (right):
> 
>
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
> Source/core/css/resolver/MatchResult.h:91: void uaRulesFinished();
> It sounds a bit strange to have "Finished" suffix as it sounds
> returning a boolean value.  Actually this method changes the
> internal state of MatchedResult object, so verb+noun sounds
> better (note: I'm not a native English speaker) to me.
> 
> What about finishAddingUARules()?

Yes. Will fix.

>
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
> Source/core/css/resolver/MatchResult.h:92: void authorScopeFinished();
> Ditto, and why "Scope", not "Rules"?
> 
> What about finishAddingAuthorRules()?

Yes, or finishAddingAuthorRulesForTreeScope() (which will be a more correct name
when we add the rest of the original patch.

>
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
> Source/core/css/resolver/MatchResult.h:108: Vector<unsigned, 16>
> m_authorRangeEnd;
> At first I was confused why m_uaRangeEnd is a scaler while m_authorRangeEnd
> is a vector.  Maybe m_authorRangeEnds?

Will fix.

>
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
> Source/core/css/resolver/StyleResolver.cpp:1233: if (range.begin() ==
> range.end())
> Maybe we can have isEmpty() for MatchedPropertiesRange.

Will fix.

[rune, 2015-08-12 01:13:26]

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
File Source/core/css/resolver/MatchResult.h (right):

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:91: void uaRulesFinished();
On 2015/08/11 11:07:35, Takayoshi Kochi wrote:
> It sounds a bit strange to have "Finished" suffix as it sounds
> returning a boolean value.  Actually this method changes the
> internal state of MatchedResult object, so verb+noun sounds
> better (note: I'm not a native English speaker) to me.
> 
> What about finishAddingUARules()?

Done.

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:92: void authorScopeFinished();
On 2015/08/11 11:07:35, Takayoshi Kochi wrote:
> Ditto, and why "Scope", not "Rules"?
> 
> What about finishAddingAuthorRules()?

Done.

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/MatchResult.h:108: Vector<unsigned, 16>
m_authorRangeEnd;
On 2015/08/11 11:07:35, Takayoshi Kochi wrote:
> At first I was confused why m_uaRangeEnd is a scaler while m_authorRangeEnd
> is a vector.  Maybe m_authorRangeEnds?

Done.

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

https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1233: if (range.begin() ==
range.end())
On 2015/08/11 11:07:35, Takayoshi Kochi wrote:
> Maybe we can have isEmpty() for MatchedPropertiesRange.

Done.

[kochi, 2015-08-12 08:53:00]

On 2015/08/11 11:44:56, rune wrote:
> On 2015/08/11 11:07:35, Takayoshi Kochi wrote:
> > Thanks for splitting the CL!
> > It made my life a lot easier.
> > 
> > This still doesn't have tests, and if I run layout tests
> 
> This CL should not make any observable changes, so I thought the existing
> LayoutTests would cover it. I can add some unit tests to check that I get the
> ranges I expect for given style input.
> 
> > against this, lots of failures (esp. fast/forms).
> 
> I don't see any failures in fast/forms apart from an image failure for
> fast/forms/select/popup-menu-appearance-tall.html which failed before I
applied
> my changes.
> 
> > This is not intended to submit as is, but for review, right?
> 
> It was intended for submission, but I should make tests. I haven't seen the
test
> failures you've seen (I've run the fast/ directory).

Sorry, my bad.
I used ASAN builds, and on some environments (like mine, on Linux) many
tests in forms are known to fail (i.e. a known issue).

FYI, popup-menu-appearance-tall.html is also failing for me as well.

[kochi, 2015-08-12 08:59:03]

lgtm

Thanks for adding the unittest.

[rune, 2015-08-13 11:04:06]

On 2015/08/12 08:59:03, Takayoshi Kochi wrote:
> lgtm
> 
> Thanks for adding the unittest.

The unittest has caused a crash in a different unittest. Struggling to figure
out why.

[rune, 2015-08-16 21:55:05]

On 2015/08/13 11:04:06, rune wrote:
> On 2015/08/12 08:59:03, Takayoshi Kochi wrote:
> > lgtm
> > 
> > Thanks for adding the unittest.
> 
> The unittest has caused a crash in a different unittest. Struggling to figure
> out why.

Hopefully issue 521230, which has now been fixed.

[rune, 2015-08-17 07:43:19]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-17 07:43:19]

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/1282243002/#ps140001
(title: "Oilpan fix in unit test")

[commit-bot: I haz the power, 2015-08-17 07:43:28]

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

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

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

[commit-bot: I haz the power, 2015-08-17 07:53:22]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[rune, 2015-08-17 08:54:29]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-17 08:54:29]

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/1282243002/#ps160001
(title: "Signed/unsigned issue")

[commit-bot: I haz the power, 2015-08-17 08:54:34]

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

[commit-bot: I haz the power, 2015-08-17 09:11:36]

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

[commit-bot: I haz the power, 2015-08-17 09:11:36]

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

[rune, 2015-08-17 09:56:22]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-17 09:56:22]

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/1282243002/#ps180001
(title: "CORE_EXPORT needed for unit tests")

[commit-bot: I haz the power, 2015-08-17 09:56:28]

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

[commit-bot: I haz the power, 2015-08-17 11:33:42]

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

[commit-bot: I haz the power, 2015-08-17 11:33:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rune, 2015-08-17 12:28:39]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-08-17 12:28:51]

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

[commit-bot: I haz the power, 2015-08-17 13:37:49]

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

[commit-bot: I haz the power, 2015-08-17 13:37:50]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rune, 2015-08-17 19:11:33]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-08-17 19:12:01]

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

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

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

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

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-17 22:03:05]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-17 22:03:05]

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/1282243002/#ps200001
(title: "Missing call to finishAddingAuthorRulesForTreeScope.")

[commit-bot: I haz the power, 2015-08-17 22:03:22]

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

[commit-bot: I haz the power, 2015-08-18 00:51:54]

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

[commit-bot: I haz the power, 2015-08-18 00:51:55]

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-18 02:43:09]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-08-18 02:43:24]

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

[commit-bot: I haz the power, 2015-08-18 04:41:20]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200697

[hans, 2015-08-18 14:58:30]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1303513002/ by hans@chromium.org.

The reason for reverting is: This caused oilpan plugin errors on Windows.
From
http://build.chromium.org/p/chromium.fyi/builders/ClangToTWin/builds/2535/ste...:

..\..\third_party\WebKit\Source\core/css/resolver/MatchResult.h(115,1) :  error:
[blink-gc] Class 'ImportantAuthorRangeIterator' contains invalid fields.
class ImportantAuthorRangeIterator {
^
..\..\third_party\WebKit\Source\core/css/resolver/MatchResult.h(140,5) :  note:
[blink-gc] Stack-allocated field 'm_result' declared here:
    const MatchResult& m_result;
    ^
..\..\third_party\WebKit\Source\core/css/resolver/MatchResult.h(144,1) :  error:
[blink-gc] Class 'ImportantAuthorRanges' contains invalid fields.
class ImportantAuthorRanges {
^
..\..\third_party\WebKit\Source\core/css/resolver/MatchResult.h(152,5) :  note:
[blink-gc] Stack-allocated field 'm_result' declared here:
    const MatchResult& m_result;
    ^.

[rune, 2015-08-19 09:09:16]

The CQ bit was checked by rune@opera.com

[rune, 2015-08-19 09:09:16]

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/1282243002/#ps220001
(title: "Missing STACK_ALLOCATED()")

[commit-bot: I haz the power, 2015-08-19 09:09:28]

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

[commit-bot: I haz the power, 2015-08-19 11:52:26]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200818