Return ActiveSheetsChanged when rulesets change in common prefix.

Return ActiveSheetsChanged when rulesets change in common prefix.

When comparing old and new active sheets, we only append the added
sheets to the ScopedStyleResolver if the old sheet vector is a prefix
of the new sheets. However, that's not correct if any of the RuleSets
in the common prefix changed due to media query changes or cssom
modifications of a stylesheet.

I can confirm that this fixes 681472. The other two issues in the BUG
field look like duplicates, but I've not been able to reproduce them.

R=meade@chromium.org,sashab@chromium.org
BUG=681472, 677371, 681882
Review-Url: https://codereview.chromium.org/2650743002
Cr-Commit-Position: refs/heads/master@{#446008}
Committed: https://chromium.googlesource.com/chromium/src/+/67cfc67e3b62993bb0df9395559e1d1a76d26213

[rune, 2017-01-23 15:05:19]

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

[commit-bot: I haz the power, 2017-01-23 15:05:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 15:06:16]

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

[commit-bot: I haz the power, 2017-01-23 15:06:18]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rune, 2017-01-23 15:37:23]

ptal

[sashab, 2017-01-23 22:06:33]

https://codereview.chromium.org/2650743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp (right):

https://codereview.chromium.org/2650743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp:49: return change;
I'm having trouble comparing these two logic. So the old logic was:

if (index == newStyleSheetCount && changedRuleSets.isEmpty()) ->
NoActiveSheetsChanged
if (index == newStyleSheetCount && !changedRuleSets.isEmpty()) ->
ActiveSheetsChanged
...
if (changedRuleSets.isEmpty()) -> NoActiveSheetsChanged
if (changedRuleSets.isEmpty()) -> ActiveSheetsAppended

New logic:

if (oldchangedRuleSets.isEmpty() && !changedRuleSets.isEmpty()) ->
ActiveSheetsAppended
if (!oldchangedRuleSets.isEmpty() && !changedRuleSets.isEmpty()) ->
ActiveSheetsChanged

This can probably be written in a simpler way, without the ActiveSheetsChange
enum ternary but just bool noChangedRuleSets = !changedRuleSets.isEmpty(). And
then just a ternary operator/if-return at the end.

[rune, 2017-01-24 01:08:10]

https://codereview.chromium.org/2650743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp (right):

https://codereview.chromium.org/2650743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp:49: return change;
On 2017/01/23 22:06:33, sashab wrote:
> I'm having trouble comparing these two logic. So the old logic was:
> 
> if (index == newStyleSheetCount && changedRuleSets.isEmpty()) ->
> NoActiveSheetsChanged
> if (index == newStyleSheetCount && !changedRuleSets.isEmpty()) ->
> ActiveSheetsChanged
> ...
> if (changedRuleSets.isEmpty()) -> NoActiveSheetsChanged
> if (changedRuleSets.isEmpty()) -> ActiveSheetsAppended
> 
> New logic:
> 
> if (oldchangedRuleSets.isEmpty() && !changedRuleSets.isEmpty()) ->
> ActiveSheetsAppended
> if (!oldchangedRuleSets.isEmpty() && !changedRuleSets.isEmpty()) ->
> ActiveSheetsChanged
> 
> This can probably be written in a simpler way, without the ActiveSheetsChange
> enum ternary but just bool noChangedRuleSets = !changedRuleSets.isEmpty(). And
> then just a ternary operator/if-return at the end.

Tried with a bool approach now.

[rune, 2017-01-24 01:17:42]

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

[commit-bot: I haz the power, 2017-01-24 01:18:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 03:15:26]

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

[commit-bot: I haz the power, 2017-01-24 03:15:27]

Dry run: This issue passed the CQ dry run.

[sashab, 2017-01-25 02:45:00]

Looking much better! :) LGTM

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp (right):

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp:41: bool
ruleSetsChanged = !changedRuleSets.isEmpty();
Hmm, is this the same logic as before? This is way better. :) Just make this:

if (!changedRuleSets.isEmpty())
    return ActiveSheetsChanged;

and remove it from below.

[meade_UTC10, 2017-01-25 05:14:40]

lgtm

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp (right):

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp:41: bool
ruleSetsChanged = !changedRuleSets.isEmpty();
On 2017/01/25 02:45:00, sashab wrote:
> Hmm, is this the same logic as before? This is way better. :) Just make this:
> 
> if (!changedRuleSets.isEmpty())
>     return ActiveSheetsChanged;
> 
> and remove it from below.

I don't think that'd have the same behaviour, Sasha :)

But I do think it'd be clearer to just use this condition directly in the if
statement, since you only use the variable ruleSetsChanged once.

[rune, 2017-01-25 08:57:34]

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp (right):

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp:41: bool
ruleSetsChanged = !changedRuleSets.isEmpty();
On 2017/01/25 02:45:00, sashab wrote:
> Hmm, is this the same logic as before? This is way better. :) Just make this:
> 
> if (!changedRuleSets.isEmpty())
>     return ActiveSheetsChanged;
> 
> and remove it from below.

Those are two different tests. The test here is the key of this fix.

Given that upper case letters are style sheets and lower case are their rulesets
below.

If we have stylesheets appended to the end, we can return ActiveSheetsAppended
and just add C and D to the ScopedStyleResolver like in this case:

old:

AB
ab

new

ABCD
abcd

But, if you have:

old:

AB
ab

new:

ABCD
aecd

You cannot simply add C and D because we would not have the ScopedStyleResolver
pick up the b -> e change. So instead we need to re-add all sheets to
ScopedStyleResolver, which is what ActiveSheetsChanged does.

We cannot do an early return here since we need the trailing sheets' rulesets
added to the changedRuleSets.

The changedRuleSets.isEmpty() below is to check if the trailing sheets added any
rulesets.

https://codereview.chromium.org/2650743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp:41: bool
ruleSetsChanged = !changedRuleSets.isEmpty();
On 2017/01/25 05:14:40, Eddy wrote:
> On 2017/01/25 02:45:00, sashab wrote:
> > Hmm, is this the same logic as before? This is way better. :) Just make
this:
> > 
> > if (!changedRuleSets.isEmpty())
> >     return ActiveSheetsChanged;
> > 
> > and remove it from below.
> 
> I don't think that'd have the same behaviour, Sasha :)
> 
> But I do think it'd be clearer to just use this condition directly in the if
> statement, since you only use the variable ruleSetsChanged once.

The isEmpty check must be done before the for-loop. See the other reply.

[rune, 2017-01-25 09:26:27]

I've added documentation and renamed the boolean to make this clearer.

[rune, 2017-01-25 09:30:55]

The CQ bit was checked by rune@opera.com

[rune, 2017-01-25 09:30:55]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2650743002/#ps40001
(title: "Documentation and more descriptive variable name.")

[commit-bot: I haz the power, 2017-01-25 09:31:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 13:09:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485336655143830,
"parent_rev": "6e4374172f45f3ad816070233709b31530dcdebc", "commit_rev":
"67cfc67e3b62993bb0df9395559e1d1a76d26213"}

[commit-bot: I haz the power, 2017-01-25 13:10:30]

Description was changed from

==========
Return ActiveSheetsChanged when rulesets change in common prefix.

When comparing old and new active sheets, we only append the added
sheets to the ScopedStyleResolver if the old sheet vector is a prefix
of the new sheets. However, that's not correct if any of the RuleSets
in the common prefix changed due to media query changes or cssom
modifications of a stylesheet.

I can confirm that this fixes 681472. The other two issues in the BUG
field look like duplicates, but I've not been able to reproduce them.

R=meade@chromium.org,sashab@chromium.org
BUG=681472,677371,681882
==========

to

==========
Return ActiveSheetsChanged when rulesets change in common prefix.

When comparing old and new active sheets, we only append the added
sheets to the ScopedStyleResolver if the old sheet vector is a prefix
of the new sheets. However, that's not correct if any of the RuleSets
in the common prefix changed due to media query changes or cssom
modifications of a stylesheet.

I can confirm that this fixes 681472. The other two issues in the BUG
field look like duplicates, but I've not been able to reproduce them.

R=meade@chromium.org,sashab@chromium.org
BUG=681472,677371,681882

Review-Url: https://codereview.chromium.org/2650743002
Cr-Commit-Position: refs/heads/master@{#446008}
Committed:
https://chromium.googlesource.com/chromium/src/+/67cfc67e3b62993bb0df9395559e...
==========

[commit-bot: I haz the power, 2017-01-25 13:10:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/67cfc67e3b62993bb0df9395559e...