Use removeFontFace to avoid resetting fontSelector.

Use removeFontFace to avoid resetting fontSelector.

BUG=305885, 336756
TEST=fast/css/font-face-remove.html, fast/css/font-face-html-as-svg.html

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

[tasak, 2013-11-22 10:49:19]

WIP.
Need to fix LayoutTests/fast/css/font-face-remove.html's regression.

[tasak, 2013-11-25 02:16:21]

+ksakamoto

[tasak, 2013-11-25 07:22:31]

Would you review this CL?

fast/css/font-face-remove.html's regression was fixed by
http://src.chromium.org/viewvc/blink?view=rev&rev=162598.

[dglazkov, 2013-11-25 16:27:29]

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/StyleShee...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/StyleShee...
Source/core/css/StyleSheetContents.cpp:567: void
StyleSheetContents::extractFontFaceRule(Vector<const StyleRuleFontFace*>&
fontFaceRules)
extractFontFaceRules? Actually, you're not removing these rules here, so
"extract" might be a bit too drastic-sounding. findFontFaceRules?

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:294: void
StyleResolver::removeFontFaceRule(Vector<const StyleRuleFontFace*>&
removedFontFace)
removeFontFaceRules? Could the argument be const?

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/DocumentS...
File Source/core/dom/DocumentStyleSheetCollection.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/DocumentS...
Source/core/dom/DocumentStyleSheetCollection.cpp:170:
styleResolver->removeFontFaceRule(change.removedFontFace);
I am curious why we couldn't just do extract+remove here?

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
Source/core/dom/StyleSheetCollection.cpp:156: static bool
styleSheetContentsHasFontFaceRule(Vector<StyleSheetContents*> sheets,
Vector<const StyleRuleFontFace*>& removedRules)
This function now removes things, and the name still sounds like it just just
checks the state.

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
File Source/core/dom/StyleSheetCollection.h (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
Source/core/dom/StyleSheetCollection.h:108: Vector<const StyleRuleFontFace*>
removedFontFace;
fontFaceRulesToRemove?

[tasak, 2013-11-26 07:49:06]

Thank you for reviewing.

I found that firstly I need to move fontSelector from StyleResolver to
StyleEngine, because https://codereview.chromium.org/85693009/ will be landed.

So I created a patch for moving fontSelector, i.e.
https://codereview.chromium.org/87503003/

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/StyleShee...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/StyleShee...
Source/core/css/StyleSheetContents.cpp:567: void
StyleSheetContents::extractFontFaceRule(Vector<const StyleRuleFontFace*>&
fontFaceRules)
On 2013/11/25 16:27:29, Dimitri Glazkov wrote:
> extractFontFaceRules? Actually, you're not removing these rules here, so
> "extract" might be a bit too drastic-sounding. findFontFaceRules?

I see. Done.

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:294: void
StyleResolver::removeFontFaceRule(Vector<const StyleRuleFontFace*>&
removedFontFace)
On 2013/11/25 16:27:29, Dimitri Glazkov wrote:
> removeFontFaceRules? Could the argument be const?

Done.

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/DocumentS...
File Source/core/dom/DocumentStyleSheetCollection.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/DocumentS...
Source/core/dom/DocumentStyleSheetCollection.cpp:170:
styleResolver->removeFontFaceRule(change.removedFontFace);
On 2013/11/25 16:27:29, Dimitri Glazkov wrote:
> I am curious why we couldn't just do extract+remove here? 

So is it ok to move removeFontFaceRule to analyzeStyleSheetChange?

In this case, is it better to change the method name?

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
File Source/core/dom/StyleSheetCollection.cpp (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
Source/core/dom/StyleSheetCollection.cpp:156: static bool
styleSheetContentsHasFontFaceRule(Vector<StyleSheetContents*> sheets,
Vector<const StyleRuleFontFace*>& removedRules)
On 2013/11/25 16:27:29, Dimitri Glazkov wrote:
> This function now removes things, and the name still sounds like it just just
> checks the state.

I renamed the function to findFontFaceRulesFromStyleSheetContents.

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
File Source/core/dom/StyleSheetCollection.h (right):

https://codereview.chromium.org/82583005/diff/50001/Source/core/dom/StyleShee...
Source/core/dom/StyleSheetCollection.h:108: Vector<const StyleRuleFontFace*>
removedFontFace;
On 2013/11/25 16:27:29, Dimitri Glazkov wrote:
> fontFaceRulesToRemove?

Done.

[ojan, 2013-11-26 17:25:15]

I'm confused. Why are there no changed tests or expectations with this patch? Or
is this not the patch that fixes firing multiple load events for the same font?

[dglazkov, 2013-11-26 17:58:51]

On 2013/11/26 17:25:15, ojan wrote:
> I'm confused. Why are there no changed tests or expectations with this patch?
Or
> is this not the patch that fixes firing multiple load events for the same
font?

This patch is a performance/clarity optimization: it avoids dropping the entire
fontSelector on the floor whenever we have @font-face rule changes. It
definitely should not have changed test expectations.

[ojan, 2013-11-26 18:30:46]

On 2013/11/26 17:58:51, Dimitri Glazkov wrote:
> On 2013/11/26 17:25:15, ojan wrote:
> > I'm confused. Why are there no changed tests or expectations with this
patch?
> Or
> > is this not the patch that fixes firing multiple load events for the same
> font?
> 
> This patch is a performance/clarity optimization: it avoids dropping the
entire
> fontSelector on the floor whenever we have @font-face rule changes. It
> definitely should not have changed test expectations.

I see. Have we seen fontSelector recreation showing up on profiles? Can the
change description point to the test cases that get faster? Without more
information, someone looking at this 3 years from now will be unable to tell if
their changes to the fontSelector code cause a regression or not.

[commit-bot: I haz the power, 2013-11-26 23:09:24]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[ojan, 2013-11-26 23:24:53]

On 2013/11/26 18:30:46, ojan wrote:
> On 2013/11/26 17:58:51, Dimitri Glazkov wrote:
> > On 2013/11/26 17:25:15, ojan wrote:
> > > I'm confused. Why are there no changed tests or expectations with this
> patch?
> > Or
> > > is this not the patch that fixes firing multiple load events for the same
> > font?
> > 
> > This patch is a performance/clarity optimization: it avoids dropping the
> entire
> > fontSelector on the floor whenever we have @font-face rule changes. It
> > definitely should not have changed test expectations.
> 
> I see. Have we seen fontSelector recreation showing up on profiles? Can the
> change description point to the test cases that get faster? Without more
> information, someone looking at this 3 years from now will be unable to tell
if
> their changes to the fontSelector code cause a regression or not.

Actually, I think this will show up on our existing perf tests. At the very
least, https://codereview.chromium.org/87503003/ resolves most of the perf
regression in https://code.google.com/p/chromium/issues/detail?id=323370.

[dglazkov, 2013-12-02 17:59:03]

On 2013/11/26 23:24:53, ojan wrote:

> Actually, I think this will show up on our existing perf tests. At the very
> least, https://codereview.chromium.org/87503003/ resolves most of the perf
> regression in https://code.google.com/p/chromium/issues/detail?id=323370.

does it look good to you? :)

[ojan, 2013-12-02 18:22:31]

On 2013/12/02 17:59:03, Dimitri Glazkov wrote:
> On 2013/11/26 23:24:53, ojan wrote:
> 
> > Actually, I think this will show up on our existing perf tests. At the very
> > least, https://codereview.chromium.org/87503003/ resolves most of the perf
> > regression in https://code.google.com/p/chromium/issues/detail?id=323370.
> 
> does it look good to you? :)

I'm waiting for the updated version after
https://codereview.chromium.org/85693009/ lands to do this review.

[ojan, 2013-12-03 19:44:08]

On 2013/12/02 18:22:31, ojan wrote:
> On 2013/12/02 17:59:03, Dimitri Glazkov wrote:
> > On 2013/11/26 23:24:53, ojan wrote:
> > 
> > > Actually, I think this will show up on our existing perf tests. At the
very
> > > least, https://codereview.chromium.org/87503003/ resolves most of the perf
> > > regression in https://code.google.com/p/chromium/issues/detail?id=323370.
> > 
> > does it look good to you? :)
> 
> I'm waiting for the updated version after
> https://codereview.chromium.org/85693009/ lands to do this review.

Whoops. Wrong link. I'm waiting for https://codereview.chromium.org/87503003/ to
land.

[tasak, 2013-12-10 07:40:48]

Since moving CSSFontSelector to StyleEngine, I have just updated this patch.

Since I updated CSSFontSelector's GenericFontSettings from WebCore::Settings, it
would be better to wait for dglazkov's review.

[tasak, 2013-12-13 04:42:27]

On 2013/12/10 07:40:48, tasak wrote:
> Since moving CSSFontSelector to StyleEngine, I have just updated this patch.
> 
> Since I updated CSSFontSelector's GenericFontSettings from WebCore::Settings,
it
> would be better to wait for dglazkov's review.

The new patch is wrong, because the patch breaks the following:

A CSSFontSelector instance does not expect for generic font settings to
ever change within its lifetime. So, it makes sense to copy and stash
them locally and avoid holding/using a Document pointer for this
purpose.

I think, we still need resetFontSelector(=recreateFontSelector) when generic
font settings is changed.

[tasak, 2013-12-13 05:36:46]

On 2013/12/13 04:42:27, tasak wrote:

> I think, we still need resetFontSelector(=recreateFontSelector) when generic
> font settings is changed.

The newest patch works in the following:

(1) when GenericFontFamilySettings is changed, use resetFontSelector.

(2) when @font-face rule is removed, use removeFontFaceRules instead of
resetFontSelector.

[ojan, 2013-12-18 00:20:01]

This looks good except the one question...

https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/Document...
File Source/core/dom/DocumentStyleSheetCollection.cpp (left):

https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/Document...
Source/core/dom/DocumentStyleSheetCollection.cpp:167:
engine->resetFontSelector();
Is removing this line correct? In the reconstruct case, how do we know if font
face rules changed?

[tasak, 2013-12-18 05:12:23]

Thank you for reviewing.

https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/Document...
File Source/core/dom/DocumentStyleSheetCollection.cpp (left):

https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/Document...
Source/core/dom/DocumentStyleSheetCollection.cpp:167:
engine->resetFontSelector();
On 2013/12/18 00:20:02, ojan wrote:
> Is removing this line correct? In the reconstruct case, how do we know if font
> face rules changed?

You are right.
If StyleFontFaceRule is updated by using CSSOMWrapper, we should reset
FontSelector.
And if a new style which has @font-face rule is inserted before another style
which also has @font-face rule, we should also reset FontSelector (because of
changing order of @font-face).

I added two layout tests to cover these cases.

[ojan, 2014-01-03 01:20:43]

lgtm

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
File LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html (right):

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html:1: <!doctype
html>
Nit: here and elsewhere, we usually spell DOCTYPE in all caps.

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
File LayoutTests/fast/css/cssom-modify-font-face-rule.html (right):

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
LayoutTests/fast/css/cssom-modify-font-face-rule.html:7:
console.log(document.styleSheets[0].cssRules[0].style.fontFamily);
Are these console.logs accidentally included in this patch?

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/fon...
File LayoutTests/fast/css/font-face-insertBefore.html (right):

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/fon...
LayoutTests/fast/css/font-face-insertBefore.html:11:
console.log(document.styleSheets[1].cssRules[0].style.src);
Are these console.logs accidentally included in this patch?

[tasak, 2014-01-07 08:42:11]

Thank you for reviewing.

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
File LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html (right):

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html:1: <!doctype
html>
On 2014/01/03 01:20:44, ojan wrote:
> Nit: here and elsewhere, we usually spell DOCTYPE in all caps.

Done.

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
File LayoutTests/fast/css/cssom-modify-font-face-rule.html (right):

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/css...
LayoutTests/fast/css/cssom-modify-font-face-rule.html:7:
console.log(document.styleSheets[0].cssRules[0].style.fontFamily);
On 2014/01/03 01:20:44, ojan wrote:
> Are these console.logs accidentally included in this patch?

Sorry. I used the console.logs for debugging. Removed.

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/fon...
File LayoutTests/fast/css/font-face-insertBefore.html (right):

https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/fon...
LayoutTests/fast/css/font-face-insertBefore.html:11:
console.log(document.styleSheets[1].cssRules[0].style.src);
On 2014/01/03 01:20:44, ojan wrote:
> Are these console.logs accidentally included in this patch?

Done.

[commit-bot: I haz the power, 2014-01-07 09:51:10]

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

[commit-bot: I haz the power, 2014-01-07 10:12:26]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[tasak, 2014-01-08 04:43:32]

dglazkov, would you review this CL?

I need lgtm from Source/platform OWNER.

[esprehn, 2014-01-08 04:45:50]

On 2014/01/08 04:43:32, tasak wrote:
> dglazkov, would you review this CL?
> 
> I need lgtm from Source/platform OWNER.

rslgtm per ojan

[dglazkov, 2014-01-08 17:34:07]

lgtm

[commit-bot: I haz the power, 2014-01-08 17:34:17]

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

[commit-bot: I haz the power, 2014-01-08 17:43:03]

Change committed as 164691

[jochen (gone - plz use gerrit), 2014-01-09 15:41:39]

A revert of this CL has been created in
https://codereview.chromium.org/131403008/ by jochen@chromium.org.

The reason for reverting is: This broke PrintWebViewHelperTest.PrintLayoutTest
on Mac.

[tasak, 2014-01-15 06:18:26]

On 2014/01/09 15:41:39, jochen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/131403008/ by mailto:jochen@chromium.org.
> 
> The reason for reverting is: This broke PrintWebViewHelperTest.PrintLayoutTest
> on Mac.

I found the reason why the crash occurs.

After PrintWebViewHelperTest.PrintLayoutTest, Blink instance will be destroyed
and next Mac objects (NSFont and so on) will be destroyed.

However, some Mac objects (i.e. StringImplCF) have references to Blink instance
(i.e. underlyingString).

The objects have different lifetime. So because of bad memory access, browser
test crashes.

I'm now trying to remove bad conversion from AtomicString to NSString. We should
just copy characters and not share StringImpl.

[tasak, 2014-01-23 10:48:10]

I modified StyleEngine::resetFontSelector to use
CSSSegmentedFontFaceCache::clear.
(Thanks, ksakamoto-san)

So we can keep the same FontSelector instance. This fixes crbug.com/336756 and
crbug.com/335701.

[dglazkov, 2014-01-23 19:15:05]

lgtm with comments.

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/Document...
Source/core/dom/Document.h:398: CSSFontSelector* fontSelector() const;
Please remove this accessor. It's a simple plumb-through, and all it does is
obscure relationship between Document and CSSFontSelector.

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:530: void StyleEngine::resetFontSelector()
Add a fixme to rename the method?

https://codereview.chromium.org/82583005/diff/640001/Source/core/frame/Settin...
File Source/core/frame/Settings.cpp (right):

https://codereview.chromium.org/82583005/diff/640001/Source/core/frame/Settin...
Source/core/frame/Settings.cpp:170:
invalidate(SettingsDelegate::FontFamilyChange);
Maybe it's just me, but I think it's silly to wrap all these methods. There are
only two places where the invalidate needs to be called: once when the settings
are initialized and once for each time the InternalSettings function is called.
We should just do it then, instead of repeatedly invoking resetFontSelector at
startup.

I am not a fan of extra plumbing functions that hide what's happening :)

[tasak, 2014-01-28 07:16:56]

Thank you for reviewing.

I needed to update m_genericFontFamilySettings owned by CSSFontSelector.
This mistake caused fast/text/international/'s regression.

The newest patch fixed this issue.

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/Document...
Source/core/dom/Document.h:398: CSSFontSelector* fontSelector() const;
On 2014/01/23 19:15:06, Dimitri Glazkov wrote:
> Please remove this accessor. It's a simple plumb-through, and all it does is
> obscure relationship between Document and CSSFontSelector.

Done.

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/82583005/diff/640001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:530: void StyleEngine::resetFontSelector()
On 2014/01/23 19:15:06, Dimitri Glazkov wrote:
> Add a fixme to rename the method?

I renamed the method. Now it is clearFontCache().

https://codereview.chromium.org/82583005/diff/640001/Source/core/frame/Settin...
File Source/core/frame/Settings.cpp (right):

https://codereview.chromium.org/82583005/diff/640001/Source/core/frame/Settin...
Source/core/frame/Settings.cpp:170:
invalidate(SettingsDelegate::FontFamilyChange);
On 2014/01/23 19:15:06, Dimitri Glazkov wrote:
> Maybe it's just me, but I think it's silly to wrap all these methods. There
are
> only two places where the invalidate needs to be called: once when the
settings
> are initialized and once for each time the InternalSettings function is
called.
> We should just do it then, instead of repeatedly invoking resetFontSelector at
> startup.
> 
> I am not a fan of extra plumbing functions that hide what's happening :)

I see.
I reverted these changes and added "notifyGenericFontFamilyChange()" method to
update CSSFontSelector's m_genericFontFamilySettings.

[dglazkov, 2014-01-29 05:11:49]

https://codereview.chromium.org/82583005/diff/720001/Source/core/css/CSSFontS...
File Source/core/css/CSSFontSelector.h (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/css/CSSFontS...
Source/core/css/CSSFontSelector.h:93: void
updateGenericFontFamilySettings(Document&);
This sounds terrifying. Why do we ever need to do that? Can you please explain?

https://codereview.chromium.org/82583005/diff/720001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:547: void
StyleEngine::updateGenericFontFamilySettings()
Why do we need a separate function for this? When is this called?

https://codereview.chromium.org/82583005/diff/720001/Source/core/frame/Settin...
File Source/core/frame/Settings.h (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/frame/Settin...
Source/core/frame/Settings.h:58: void notifyGenericFontFamilyChange() {
invalidate(SettingsDelegate::FontFamilyChange); }
I was hoping to avoid this function altogether. Do we need a VC to talk this
through?

The key thing here is that in production, the genericFontFamilySettings is only
set once, when creating the renderer. There are no sub-systems that enable
changing them, right? Why do we need this complex notification system that
attempts to pretend that this change could happen anytime?

https://codereview.chromium.org/82583005/diff/720001/Source/core/testing/Inte...
File Source/core/testing/InternalSettings.cpp (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/testing/Inte...
Source/core/testing/InternalSettings.cpp:233:
settings()->notifyGenericFontFamilyChange();
Here, we just need a helper that recursively visits the frames and does the
work. This helper does not need to live anywhere outside of this special-case
testing code.

https://codereview.chromium.org/82583005/diff/720001/Source/web/WebSettingsIm...
File Source/web/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/82583005/diff/720001/Source/web/WebSettingsIm...
Source/web/WebSettingsImpl.cpp:70: m_settings->notifyGenericFontFamilyChange();
These WebSettingImpl functions are never called separately. Look at the
callsites. They are only called when updating web_preferences wholesale. Why do
we need to traverse the frames tree after each call?

[tasak, 2014-01-29 10:13:43]

Thank you for reviewing.

Before updating the patch, I think, I need to understand
GenericFontFamilySettings-related things.

So would you help me to understand it?

https://codereview.chromium.org/82583005/diff/720001/Source/core/css/CSSFontS...
File Source/core/css/CSSFontSelector.h (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/css/CSSFontS...
Source/core/css/CSSFontSelector.h:93: void
updateGenericFontFamilySettings(Document&);
On 2014/01/29 05:11:50, Dimitri Glazkov wrote:
> This sounds terrifying. Why do we ever need to do that? Can you please
explain?

So would you help me to fix
fast/text/international/content-language-font-selection.html's regression?

The test uses "window.internals.settings.setStandardFontFamily("Ahem",
"Hant");".

So if we don't update GenericFontFamilySettings, the test will fail.

I'm not sure whether I can modify the layout test or not.

https://codereview.chromium.org/82583005/diff/720001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:547: void
StyleEngine::updateGenericFontFamilySettings()
On 2014/01/29 05:11:50, Dimitri Glazkov wrote:
> Why do we need a separate function for this? When is this called?

The updateGenericFontFamilySettings will be invoked from WebSettings or
InternalSettings.

clearFontCache will be invoked from updateActiveStyleSheets when we update some
stylesheets which contains @font-face rule.

https://codereview.chromium.org/82583005/diff/720001/Source/core/frame/Settin...
File Source/core/frame/Settings.h (right):

https://codereview.chromium.org/82583005/diff/720001/Source/core/frame/Settin...
Source/core/frame/Settings.h:58: void notifyGenericFontFamilyChange() {
invalidate(SettingsDelegate::FontFamilyChange); }
On 2014/01/29 05:11:50, Dimitri Glazkov wrote:
> I was hoping to avoid this function altogether. Do we need a VC to talk this
> through?

Looking at some layout test or WebSettings, GenericFontFamilySettings might be
updated from chrome side via some interface?

I'm not sure. At least, some layout test depends on that
window.internals.settings.setXXXFontFamily can update all existing
GenericFontFamilySettings.

> The key thing here is that in production, the genericFontFamilySettings is
only
> set once, when creating the renderer. There are no sub-systems that enable
> changing them, right? Why do we need this complex notification system that
> attempts to pretend that this change could happen anytime?

So we can fix (or remove) the layout tests? (e.g.
fast/text/international/content-language-font-selection.html)

https://codereview.chromium.org/82583005/diff/720001/Source/web/WebSettingsIm...
File Source/web/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/82583005/diff/720001/Source/web/WebSettingsIm...
Source/web/WebSettingsImpl.cpp:70: m_settings->notifyGenericFontFamilyChange();
On 2014/01/29 05:11:50, Dimitri Glazkov wrote:
> These WebSettingImpl functions are never called separately. Look at the
> callsites. They are only called when updating web_preferences wholesale. Why
do
> we need to traverse the frames tree after each call?

I would like to confirm what you mean. As far as I investigated,

(1) Each frame has its own document.
(2) Each document has its own StyleEngine and its own CSSFontSelector.
(3) CSSFontSelector has its own GenericFontFamilySettings (different instance
from WebSettingsImpl).

Each page can have multiple frame -- i.e. iframes.

Looking at caller side, chrome just invokes setSansSerifFontFamilyWrapper and
seems not to recreate renderer or reload page?

So ... how can we update all GenericFontFamilySettings without traversing
document?

I think, you are very familiar with GenericFontFamilySettings things. I need
your help.

[tasak, 2014-01-29 10:15:44]

On 2014/01/29 10:13:43, tasak wrote:
> I'm not sure. At least, some layout test depends on that
> window.internals.settings.setXXXFontFamily can update all existing
> GenericFontFamilySettings.

I mean, the following layout tests:
fast/text/international/content-language-font-selection.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/international/locale-sensitive-fonts.html
fast/text/international/lang-sensitive-fonts.html
fast/text/international/content-language-with-subtags.html
fast/text/international/font-fallback-to-common-script.html
fast/text/international/jpan-script-equivalence.html

[dglazkov, 2014-01-29 17:59:52]

On 2014/01/29 10:13:43, tasak wrote:
> I think, you are very familiar with GenericFontFamilySettings things. I need
> your help.

No, I was wrong. I was under assumption that we never update font family
settings from the browser, but turns out we do.

LGTM for the current approach, I'll have to think about how to handle this more
elegantly. For example, we should probably just reset the entire style engine
when prefs are updated -- all bets are off at that point.

[tasak, 2014-02-05 06:09:51]

On 2014/01/29 17:59:52, Dimitri Glazkov wrote:
> On 2014/01/29 10:13:43, tasak wrote:
> > I think, you are very familiar with GenericFontFamilySettings things. I need
> > your help.
> 
> No, I was wrong. I was under assumption that we never update font family
> settings from the browser, but turns out we do.

ksakamoto-san told me how to change font settings.

0. Type "data:text/html,Hello%2CWorld!" in an address bar.
1. Open chrome://settings/ in another tab.
2. Click "show advanced settings".
3. Push "Cutomize fonts" button in Web content.

So "Fonts and encoding" dialog is shown.

4. Change standard font and return to the first tab (showing "Hello, World!")

And I found 1 mistake. I need "setNeedsRecalcStyleInAllFrames()" after
"updateGenericFontFamilySettings()".

If we don't invoke the setNeedsRecalcStyleInAllFrames, we need to reload the
first tab...

[tasak, 2014-02-06 00:54:04]

I finished fixing browser_tests (r249043).

So I will land this patch.

[tasak, 2014-02-06 00:54:11]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-02-06 00:54:20]

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

[commit-bot: I haz the power, 2014-02-06 07:18:24]

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

[commit-bot: I haz the power, 2014-02-06 07:18:24]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[tasak, 2014-02-07 02:50:30]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-02-07 02:50:36]

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

[tasak, 2014-02-07 02:54:43]

The CQ bit was unchecked by tasak@google.com

[tasak, 2014-02-07 03:07:52]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-02-07 03:08:04]

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

[commit-bot: I haz the power, 2014-02-07 05:43:40]

Change committed as 166681

[abarth-chromium, 2014-02-07 19:38:43]

A revert of this CL has been created in
https://codereview.chromium.org/157853002/ by abarth@chromium.org.

The reason for reverting is: This CL appears to have broken

org.chromium.android_webview.test.AwSettingsTest#testLayoutAlgorithmWithTwoViews

in androidwebview_instrumentation_tests.

http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg....

[tasak, 2014-02-12 07:12:08]

On 2014/02/07 19:38:43, abarth wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/157853002/ by mailto:abarth@chromium.org.
> 
> The reason for reverting is: This CL appears to have broken
> 
>
org.chromium.android_webview.test.AwSettingsTest#testLayoutAlgorithmWithTwoViews
> 
> in androidwebview_instrumentation_tests.
> 
>
http://build.chromium.org/p/chromium.webkit/builders/Android%2520Tests%2520%2....

androidwebview_instrumentation failed because of TextAutosizing bug.

TextAutosizing depends on layout (because TextAutosizer::processSubtree is
invoked from layout code), but it uses setNeedsStyleRecalc when
TextAutosizingEnabled setting is changed.

I merged its fixing patch into this patch, because the failure is revealed by
this keepFontSelector patch.

[tasak, 2014-02-12 07:15:41]

I modified Page::settingsChanged, Settings::setTextAutosizingEnabled and
Settings::setTextAutosizingWindowSizeOverride for fixing
AwSettingsTest.testLayoutAlgorithmWithTwoViews test regression.

Looking at TextAutosizing, it updates RenderStyle during layout time.
So when TextAutosizing-related settings are changed, we should use
setNeedsLayout, not setNeedsStyleRecalc.

[ojan, 2014-02-12 18:46:38]

+a couple others to look at the TextAutosizing changes.

[skobes, 2014-02-12 19:38:41]

On 2014/02/12 07:15:41, tasak wrote:
> I modified Page::settingsChanged, Settings::setTextAutosizingEnabled and
> Settings::setTextAutosizingWindowSizeOverride for fixing
> AwSettingsTest.testLayoutAlgorithmWithTwoViews test regression.
> 
> Looking at TextAutosizing, it updates RenderStyle during layout time.
> So when TextAutosizing-related settings are changed, we should use
> setNeedsLayout, not setNeedsStyleRecalc.

This sounds reasonable to me but I'm not sure exactly what the motivation was
for doing setNeedsRecalcStyleInAllFrames on a text autosizing setting change. 
It seems to have originated in
http://src.chromium.org/viewvc/blink?view=revision&revision=118407 and
http://src.chromium.org/viewvc/blink?view=revision&revision=124372.  +johnme to
comment on whether it makes sense to use setNeedsLayout instead.

(In any case I think the switch to FastTextAutosizer does not affect the answer
since both autosizers update RenderStyle "during layout" as far as settings is
concerned.)

[tasak, 2014-02-13 04:54:09]

Thank you for comments.

On 2014/02/12 19:38:41, skobes wrote:

> This sounds reasonable to me but I'm not sure exactly what the motivation was
> for doing setNeedsRecalcStyleInAllFrames on a text autosizing setting change. 
> It seems to have originated in
> http://src.chromium.org/viewvc/blink?view=revision&revision=118407 and
> http://src.chromium.org/viewvc/blink?view=revision&revision=124372.  +johnme
to
> comment on whether it makes sense to use setNeedsLayout instead.
> 
> (In any case I think the switch to FastTextAutosizer does not affect the
answer
> since both autosizers update RenderStyle "during layout" as far as settings is
> concerned.)

I will add johnme to my reviwer list.

[tasak, 2014-02-13 05:04:24]

johnme, would you review this change of TextAutosizing setting?

I think, we should use setNeedsLayout when TextAutosizing is enabled or
disabled.

I would like to explain what happens:

- This patch enables us to avoid resetting FontSelector.

- Before applying this patch, FontSelector is also recreated when
FullStyleUpdate is requested. Recreating FontSelector means "needsLayout". So
always FrameView::layout is invoked.
c.f. RenderStyle::diff and Font::operator==

- After applying this patch, FontSelector is not destroyed. So FullStyleUpdate
checks whether layout-related style is really changed or not. 

  Just changing TextAutosizing doesn't change layout-related style, because
StyleResolver doesn't know TextAutosizing.

  setNeedsStyleRecalcInAllFrames doesn't always causes FrameView::layout (or
FrameView::performLayout).

  Since FrameView::performLayout updates RenderStyles considering
TextAutosizing,  AwSettingsTest.testLayoutAlgorithmWithTwoViews test fails.

[tasak, 2014-02-14 05:35:40]

esprehn, would you review Settings.cpp and Page.cpp's change?

The change is for fixing TextAutosizing problem.

[eseidel, 2014-02-14 05:39:15]

lgtm

This is really difficult for me to verify correctness of.  I just have to trust
you.  I *really* hope we're not trying to pull a change this large into the
stable branch.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/CSSFont...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/CSSFont...
Source/core/css/CSSFontSelector.cpp:222: m_genericFontFamilySettings =
document.frame()->settings()->genericFontFamilySettings();
I think there is just a document.settings() as well.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/FontFac...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/FontFac...
Source/core/css/FontFaceCache.cpp:113: removeFontFace(it->value.get(), true);
bools as arguments are horrible.  Obviously not your fault here, but something
we should eventually clean up

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:586: if (root->m_clients.isEmpty())
This is redundant with the for loop.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:603: StyleRuleMedia* mediaRule =
static_cast<StyleRuleMedia*>(rule);
I'm surprised we don't have a toStyleRuleMedia() helper which ASSERTs.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/testing/Int...
File Source/core/testing/InternalSettings.cpp (left):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/testing/Int...
Source/core/testing/InternalSettings.cpp:235:
m_page->setNeedsRecalcStyleInAllFrames();
It seems this can just fold into the notify now?

[esprehn, 2014-02-14 05:42:30]

On 2014/02/14 05:35:40, tasak wrote:
> esprehn, would you review Settings.cpp and Page.cpp's change?
> 
> The change is for fixing TextAutosizing problem.

Calling setNeedayout isn't the same thing as calling styleResolverChanged like
the code did before. Is what you have really correct?

Lgtm to me though.

[tasak, 2014-02-14 05:49:10]

On 2014/02/14 05:42:30, esprehn wrote:
> On 2014/02/14 05:35:40, tasak wrote:
> > esprehn, would you review Settings.cpp and Page.cpp's change?
> > 
> > The change is for fixing TextAutosizing problem.
> 
> Calling setNeedayout isn't the same thing as calling styleResolverChanged like
> the code did before. Is what you have really correct?

Yeah.
Those are not the same.

I think, the old code is wrong.

> Lgtm to me though.

Thank you.

[tasak, 2014-02-14 08:27:45]

Thank you for reviewing.

I found that FastTextAutosizer needs styleRecalc.

So if FastTextAutosizer is enabled, invoke setNeedsStyleRecalc. Otherwise,
invoke setNeedsLayout.
(c.f.  virtual/fasttextautosizing/fast/text-autosizing/resize-window.html)

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/CSSFont...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/CSSFont...
Source/core/css/CSSFontSelector.cpp:222: m_genericFontFamilySettings =
document.frame()->settings()->genericFontFamilySettings();
On 2014/02/14 05:39:15, eseidel wrote:
> I think there is just a document.settings() as well.

Done.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/FontFac...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/FontFac...
Source/core/css/FontFaceCache.cpp:113: removeFontFace(it->value.get(), true);
On 2014/02/14 05:39:15, eseidel wrote:
> bools as arguments are horrible.  Obviously not your fault here, but something
> we should eventually clean up

I see. I will discuss with ksakamoto@. I think, he is an expert in WebFont /
FontSelector.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:586: if (root->m_clients.isEmpty())
On 2014/02/14 05:39:15, eseidel wrote:
> This is redundant with the for loop.

Done.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:603: StyleRuleMedia* mediaRule =
static_cast<StyleRuleMedia*>(rule);
On 2014/02/14 05:39:15, eseidel wrote:
> I'm surprised we don't have a toStyleRuleMedia() helper which ASSERTs.

Sorry. I missed the helper. Done.

https://codereview.chromium.org/82583005/diff/1560001/Source/core/testing/Int...
File Source/core/testing/InternalSettings.cpp (left):

https://codereview.chromium.org/82583005/diff/1560001/Source/core/testing/Int...
Source/core/testing/InternalSettings.cpp:235:
m_page->setNeedsRecalcStyleInAllFrames();
On 2014/02/14 05:39:15, eseidel wrote:
> It seems this can just fold into the notify now?

You are right. Removed.

[tasak, 2014-02-14 08:37:02]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-02-14 08:43:11]

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

[johnme, 2014-02-14 12:08:28]

On 2014/02/13 05:04:24, tasak wrote:
> johnme, would you review this change of TextAutosizing setting?

lgtm, setNeedsLayoutInAllFrames is indeed what is needed for TextAutosizer.
Thanks for explaining why setNeedsStyleRecalcInAllFrames will no longer trigger
layouts.

> So if FastTextAutosizer is enabled, invoke setNeedsStyleRecalc. Otherwise,
invoke setNeedsLayout.

My only concern is whether FastTextAutosizer might need both
setNeedsRecalcStyleInAllFrames and setNeedsLayoutInAllFrames, instead of just
setNeedsRecalcStyleInAllFrames. pdr or skobes can confirm whether the
FastTextAutosizer recalcStyles will always dirty layout when font sizes change.

[commit-bot: I haz the power, 2014-02-14 13:26:12]

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

[commit-bot: I haz the power, 2014-02-14 13:26:13]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[skobes, 2014-02-14 18:06:53]

lgtm

On 2014/02/14 08:27:45, tasak wrote:
> So if FastTextAutosizer is enabled, invoke setNeedsStyleRecalc. Otherwise,
> invoke setNeedsLayout.

I'm not sure why FTA would need style recalc, but I'll look into that
separately.

> (c.f.  virtual/fasttextautosizing/fast/text-autosizing/resize-window.html)

Incidentally I just fixed this test in http://crrev.com/145123010.

[skobes, 2014-02-14 18:11:02]

https://codereview.chromium.org/82583005/diff/1820001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/82583005/diff/1820001/Source/core/page/Page.c...
Source/core/page/Page.cpp:504: // FastTextAutosizer works with
RenderBlock::styleWillChange.
This comment is misleading.  FTA gathers some information during style recalc,
but the actual boosting occurs during layout (RenderBlockFlow::layoutBlockFlow).

[skobes, 2014-02-14 18:14:35]

On 2014/02/14 18:06:53, skobes wrote:
> I'm not sure why FTA would need style recalc

Ignore this, I think John is right - FTA needs both setNeedsRecalcStyle and
setNeedsLayout after a setting change.  Mainly because FTA::record is a nop if
autosizing is not enabled, so if it is toggled on and we only do layout, there
is no fingerprinting info.

[ojan, 2014-02-14 18:28:57]

On 2014/02/14 18:14:35, skobes wrote:
> On 2014/02/14 18:06:53, skobes wrote:
> > I'm not sure why FTA would need style recalc
> 
> Ignore this, I think John is right - FTA needs both setNeedsRecalcStyle and
> setNeedsLayout after a setting change.  Mainly because FTA::record is a nop if
> autosizing is not enabled, so if it is toggled on and we only do layout, there
> is no fingerprinting info.

It should only need recalc. If the recalc changes something such that layout is
needed, then recalcStyle should setNeedsLayout, no?

[skobes, 2014-02-14 18:31:02]

On 2014/02/14 18:28:57, ojan wrote:
> It should only need recalc. If the recalc changes something such that layout
is
> needed, then recalcStyle should setNeedsLayout, no?

Recalc won't actually change anything, but it may update some internal state in
FTA.

[ojan, 2014-02-14 18:42:42]

On 2014/02/14 18:31:02, skobes wrote:
> On 2014/02/14 18:28:57, ojan wrote:
> > It should only need recalc. If the recalc changes something such that layout
> is
> > needed, then recalcStyle should setNeedsLayout, no?
> 
> Recalc won't actually change anything, but it may update some internal state
in
> FTA.

If the internal state in FTA changes such that the subtree now needs a layout,
what causes that layout to happen?

[skobes, 2014-02-14 18:53:42]

On 2014/02/14 18:42:42, ojan wrote:
> If the internal state in FTA changes such that the subtree now needs a layout,
> what causes that layout to happen?

FTA is called *by* the layout code, it never "causes a layout".  It keeps some
internal state based on the styles in the tree, and uses that state during
layout.

This thread is discussing how to handle a setting change, such as turning FTA on
or off, or changing the font scale factor using the slider in Accessibility
settings.  We basically want to blow everything away and start over in that
case.  It sounds like setNeedsRecalcStyle used to have that effect, but now it
does not necessarily trigger layout.  So we also need setNeedsLayout.

We might have *only* needed setNeedsLayout, except that FTA doesn't maintain the
appropriate internal state when it is disabled.  So in the case of changing
disabled -> enabled, we need the recalc style to update that state, in addition
to the relayout.

[tasak, 2014-02-17 02:39:23]

Thank you for reviewing.

I have just fixed Settings code for FastTextAutosizer.

https://codereview.chromium.org/82583005/diff/1820001/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/82583005/diff/1820001/Source/core/page/Page.c...
Source/core/page/Page.cpp:504: // FastTextAutosizer works with
RenderBlock::styleWillChange.
On 2014/02/14 18:11:04, skobes wrote:
> This comment is misleading.  FTA gathers some information during style recalc,
> but the actual boosting occurs during layout
(RenderBlockFlow::layoutBlockFlow).

I modified to use setNeedsRecalc and setNeedsLayout here.

[tasak, 2014-02-17 02:39:38]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-02-17 02:39:56]

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

[commit-bot: I haz the power, 2014-02-17 03:50:30]

Change committed as 167257