fontSelector should be reset when removing stylesheets which has @font-face rule.

fontSelector should be reset when removing stylesheets which has @font-face rule.

r159957 changed to avoid destroying StyleResolver when removing stylesheets.
However if the removed stylesheets has any @font-face rule, we need to reset not only rulesets,
but fontselector.

BUG=305885
TEST=fast/css/font-face-remove.html,fast/css/remove-style-after-insert-font-face.html,fast/css/remove-style-after-remove-font-face.html

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

[tasak, 2013-11-08 10:15:37]

@viewport has almost the same issue, but the issue will be fixed by:
https://codereview.chromium.org/42543007

[dglazkov, 2013-11-08 18:05:33]

lgtm with comments.

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
Source/core/dom/StyleSheetCollection.cpp:182: requiresResetFontSelector =
updateType != Additive ? true : styleSheetContentsHasFontFaceRule(addedSheets);
I am not super-thrilled about adding this recursive check. Is there any way
stylesheetcontents could carry a flag that is updated when a fontfacerule is
added?

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
File Source/core/dom/StyleSheetCollection.h (right):

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
Source/core/dom/StyleSheetCollection.h:80: void
analyzeStyleSheetChange(StyleResolverUpdateMode, const
Vector<RefPtr<CSSStyleSheet> >& oldStyleSheets, const
Vector<RefPtr<CSSStyleSheet> >& newStylesheets, StyleResolverUpdateType&, bool&
requiresFullStyleRecalc, bool& requiresResetFontSelector);
Any change we could turn these bools into enums? Bools suck as params.

Also, it's starting to look like this function is has so many out params, that
it might be easier to group them into their own class?

[tasak, 2013-11-12 04:13:10]

Thank you for reviewing.

I added two more layout tests covering 2 cases: inserting @font-face by using
insertRule, and removing @font-face by deleteRule.

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
Source/core/dom/StyleSheetCollection.cpp:182: requiresResetFontSelector =
updateType != Additive ? true : styleSheetContentsHasFontFaceRule(addedSheets);
On 2013/11/08 18:05:34, Dimitri Glazkov wrote:
> I am not super-thrilled about adding this recursive check. Is there any way
> stylesheetcontents could carry a flag that is updated when a fontfacerule is
> added?

Done.

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
File Source/core/dom/StyleSheetCollection.h (right):

https://codereview.chromium.org/66483002/diff/1/Source/core/dom/StyleSheetCol...
Source/core/dom/StyleSheetCollection.h:80: void
analyzeStyleSheetChange(StyleResolverUpdateMode, const
Vector<RefPtr<CSSStyleSheet> >& oldStyleSheets, const
Vector<RefPtr<CSSStyleSheet> >& newStylesheets, StyleResolverUpdateType&, bool&
requiresFullStyleRecalc, bool& requiresResetFontSelector);
On 2013/11/08 18:05:34, Dimitri Glazkov wrote:
> Any change we could turn these bools into enums? Bools suck as params.
> 
> Also, it's starting to look like this function is has so many out params, that
> it might be easier to group them into their own class?

Done.

[dglazkov, 2013-11-12 04:43:00]

lgtm++

[commit-bot: I haz the power, 2013-11-15 08:12:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/66483002/260001

[commit-bot: I haz the power, 2013-11-15 09:42:53]

Change committed as 162094

[ojan, 2013-11-15 21:13:50]

I think we have the same problem if you remove a stylesheet that has a
viewport-dependent media queries or treeBoundaryCrossingRules, no?

[tasak, 2013-11-18 11:17:58]

On 2013/11/15 21:13:50, ojan wrote:
> I think we have the same problem if you remove a stylesheet that has a
> viewport-dependent media queries or treeBoundaryCrossingRules, no?

StyleSheetCollection::resetAllRuleSetsInTreeScope should cover. (If not, it's a
bug. Need to fix)

When removing a stylesheet, resetAllRuleSetsInTreeScope will be invoked.

treeBoundaryCrossingRules will be reset in StyleResolver::resetAuthorStyle
invoked from StyleSheetCollection::resetAllRuleSetsInTreeScope.

Since @viewport rules are stored in normal RuleSet,
StyleResolver::resetAuthorStyle also reset viewports.

And after appending all authorstyles,
i.e.StyleResolver::finishAppendAuthorStyleSheets, viewport-dependent description
will be updated.

Talking about @font-face rules, there still exists a bug. I created a patch for
fixing the bug, i.e. https://codereview.chromium.org/73993002
in_reply_to:
ahdzfmNocm9taXVtY29kZXJldmlldy1ocnIdCxIFSXNzdWUYuubZHwwLEgdNZXNzYWdlGKHRGQw
send_mail: 1
subject: fontSelector should be reset when removing stylesheets which has
@font-face rule.