Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(32)

Issue 1076163002: Don't clear FontFaceCache if no @font-face were removed. (Closed)

Created:
5 years, 8 months ago by rune
Modified:
5 years, 8 months ago
Reviewers:
tasak
CC:
blink-reviews, blink-reviews-style_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+218 lines, -3 lines) Patch
A LayoutTests/fast/css/remove-sheet-no-layout.html View 1 chunk +43 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/remove-sheet-no-layout-expected.txt View 1 chunk +14 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/DocumentStyleSheetCollection.cpp View 1 chunk +6 lines, -3 lines 0 comments Download
M Source/core/dom/TreeScopeStyleSheetCollection.h View 1 chunk +2 lines, -0 lines 0 comments Download
A Source/core/dom/TreeScopeStyleSheetCollectionTest.cpp View 1 chunk +152 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
rune
5 years, 8 months ago (2015-04-10 15:52:59 UTC) #3
rune
5 years, 8 months ago (2015-04-10 15:53:00 UTC) #4
rune
PTAL
5 years, 8 months ago (2015-04-13 06:44:31 UTC) #6
tasak
lgtm. I agree that this method looks fragile and doesn't seem to work well when ...
5 years, 8 months ago (2015-04-13 07:11:19 UTC) #7
rune
On 2015/04/13 07:11:19, tasak wrote: > lgtm. > > I agree that this method looks ...
5 years, 8 months ago (2015-04-13 08:11:01 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076163002/1
5 years, 8 months ago (2015-04-13 08:14:21 UTC) #10
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=193625
5 years, 8 months ago (2015-04-13 10:00:28 UTC) #11
tasak
5 years, 8 months ago (2015-04-14 04:18:22 UTC) #12
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698