Don't clear FontFaceCache if no @font-face were removed.

Don't clear FontFaceCache if no @font-face were removed.

AnalyzedStyleUpdate will end up with a Reconstruct for the StyleResolver
and a if we remove stylesheets which do not contain @font-face rules and
don't contain any other rules that will cause dirtiesAllStyle() to be
true. Skip clearing the font cache in those cases.

As part of understanding how the AnalyzedStyleUpdate works, I wrote a few
unit tests for compareStyleSheets. This method looks fragile and doesn't
seem to work well when stylesheets are added and removed at the same time.
I haven't found a way to trigger a real bug yet, as we don't seem to be
using AnalyzedStyleUpdate for a case where the active stylesheet list
changes in a way that triggers this bug.

BUG=471079, 475858
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193625

[rune, 2015-04-10 15:52:59]

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

[rune, 2015-04-10 15:52:59]

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

[rune, 2015-04-13 06:44:31]

rune@opera.com changed reviewers:
+ tasak@google.com
- tasak@chromium.org

[rune, 2015-04-13 06:44:31]

PTAL

[tasak, 2015-04-13 07:11:19]

lgtm.

I agree that this method looks fragile and doesn't
seem to work well when stylesheets are added and removed at the same time.

It's nice to add unit tests.

So I'm afraid of the following case:
- the method says Reconstruct, but some StyleSheetContents which has @font-face
rules and the StyleSheetContents is removed.

The unit tests says the case doesn't occur?

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

On 2015/04/13 07:11:19, tasak wrote:
> lgtm.
> 
> I agree that this method looks fragile and doesn't
> seem to work well when stylesheets are added and removed at the same time.
> 
> It's nice to add unit tests.
> 
> So I'm afraid of the following case:
> - the method says Reconstruct, but some StyleSheetContents which has
@font-face
> rules and the StyleSheetContents is removed.
> 
> The unit tests says the case doesn't occur?

Removed @font-face is handled at a higher level than the tested method.

Reconstruct accompanied by a requiresFullStyleRecalc will still clear the
font-face-cache. requiresFullStyleRecalc will only be set to false when we reach
the StyleSheetInvalidationAnalysis in
TreeScopeStyleSheetCollection::analyzeStyleSheetChange. If we remove a
stylesheet, the addedSheets will actually be the removed sheets and
ruleAdditionMightRequireDocumentStyleRecalc will set dirtiesAllStyle during the
analysis if we see an @font-face in the removed sheet.

I think the reason why we don't have any issues with the fragility of
compareStyleSheets today is that we synchronously update the active stylesheets
for each stylesheet that is removed or added to the DOM, in which case we use
the AnalyzedStyleUpdate. Cases where this would break would be in cases where we
both inserted and removed stylesheets from the active list at the same time like
for changed media queries or changed alternate stylesheets, but in those cases
we currently don't use AnalyzedStyleUpdate.

[rune, 2015-04-13 08:14:08]

The CQ bit was checked by rune@opera.com

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076163002/1

[commit-bot: I haz the power, 2015-04-13 10:00:28]

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

[tasak, 2015-04-14 04:18:22]

On 2015/04/13 08:11:01, rune wrote:
> On 2015/04/13 07:11:19, tasak wrote:
> > lgtm.
> > 
> > I agree that this method looks fragile and doesn't
> > seem to work well when stylesheets are added and removed at the same time.
> > 
> > It's nice to add unit tests.
> > 
> > So I'm afraid of the following case:
> > - the method says Reconstruct, but some StyleSheetContents which has
> @font-face
> > rules and the StyleSheetContents is removed.
> > 
> > The unit tests says the case doesn't occur?
> 
> Removed @font-face is handled at a higher level than the tested method.
> 
> Reconstruct accompanied by a requiresFullStyleRecalc will still clear the
> font-face-cache. requiresFullStyleRecalc will only be set to false when we
reach
> the StyleSheetInvalidationAnalysis in
> TreeScopeStyleSheetCollection::analyzeStyleSheetChange. If we remove a
> stylesheet, the addedSheets will actually be the removed sheets and
> ruleAdditionMightRequireDocumentStyleRecalc will set dirtiesAllStyle during
the
> analysis if we see an @font-face in the removed sheet.

I see. Thank you for explanation.

> I think the reason why we don't have any issues with the fragility of
> compareStyleSheets today is that we synchronously update the active
stylesheets
> for each stylesheet that is removed or added to the DOM, in which case we use
> the AnalyzedStyleUpdate. 

Yeah. I agree.
When I tried to make analyzeStyleSheets to use "HashSet" (to obtain correctly
removed and added sheets),
blink-perf (StyleSheetInsert, I think) became very slow, because of synchronous
update.