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

Issue 82583005: Use removeFontFace to avoid resetting fontSelector. (Closed)

Created:
7 years, 1 month ago by tasak
Modified:
6 years, 10 months ago
CC:
blink-reviews, eae+blinkwatch, dglazkov+blink, apavlov+blink_chromium.org, adamk+blink_chromium.org, darktears
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : #

Total comments: 10

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : Reset when Settings is changed #

Total comments: 2

Patch Set 6 : Added 2 layout tests #

Total comments: 6

Patch Set 7 : Patch for landing #

Patch Set 8 : Use CSSSegmentedFontFaceCache::remove #

Patch Set 9 : Modified resetFontSelector #

Total comments: 6

Patch Set 10 : Fix fast/text/international/ regression #

Total comments: 9

Patch Set 11 : Rebased #

Patch Set 12 : Rebased and added setNeedsRecalcStyleInAllFrames #

Patch Set 13 : Patch for landing #

Patch Set 14 : Fixed patch conflict #

Patch Set 15 : Fixed TextAutosizing #

Total comments: 10

Patch Set 16 : Patch for landing #

Total comments: 2

Patch Set 17 : Patch for landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+214 lines, -75 lines) Patch
A + LayoutTests/fast/css/cssom-modify-font-face-rule.html View 1 2 3 4 5 6 1 chunk +2 lines, -3 lines 0 comments Download
A LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html View 1 2 3 4 5 1 chunk +13 lines, -0 lines 0 comments Download
A + LayoutTests/fast/css/font-face-insertBefore.html View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
A LayoutTests/fast/css/font-face-insertBefore-expected.html View 1 2 3 4 5 1 chunk +20 lines, -0 lines 0 comments Download
M Source/core/css/CSSFontSelector.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSFontSelector.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/css/FontFaceCache.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/FontFaceCache.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/css/StyleSheetContents.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +3 lines, -0 lines 0 comments Download
M Source/core/css/StyleSheetContents.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +43 lines, -1 line 0 comments Download
M Source/core/dom/DocumentStyleSheetCollection.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -6 lines 0 comments Download
M Source/core/dom/StyleEngine.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -1 line 0 comments Download
M Source/core/dom/StyleEngine.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +24 lines, -14 lines 0 comments Download
M Source/core/dom/TreeScopeStyleSheetCollection.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/dom/TreeScopeStyleSheetCollection.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +11 lines, -24 lines 0 comments Download
M Source/core/frame/Settings.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/frame/Settings.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/frame/SettingsDelegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/page/Page.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/page/Page.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +28 lines, -6 lines 0 comments Download
M Source/core/testing/InternalSettings.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 7 chunks +8 lines, -7 lines 0 comments Download
M Source/platform/fonts/FontDescription.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -4 lines 0 comments Download
M Source/platform/fonts/GenericFontFamilySettings.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M Source/platform/fonts/GenericFontFamilySettings.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -0 lines 0 comments Download
M Source/web/WebSettingsImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 76 (0 generated)
tasak
WIP. Need to fix LayoutTests/fast/css/font-face-remove.html's regression.
7 years, 1 month ago (2013-11-22 10:49:19 UTC) #1
tasak
+ksakamoto
7 years ago (2013-11-25 02:16:21 UTC) #2
tasak
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.
7 years ago (2013-11-25 07:22:31 UTC) #3
dglazkov
https://codereview.chromium.org/82583005/diff/50001/Source/core/css/StyleSheetContents.cpp File Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/82583005/diff/50001/Source/core/css/StyleSheetContents.cpp#newcode567 Source/core/css/StyleSheetContents.cpp:567: void StyleSheetContents::extractFontFaceRule(Vector<const StyleRuleFontFace*>& fontFaceRules) extractFontFaceRules? Actually, you're not removing ...
7 years ago (2013-11-25 16:27:29 UTC) #4
tasak
Thank you for reviewing. I found that firstly I need to move fontSelector from StyleResolver ...
7 years ago (2013-11-26 07:49:06 UTC) #5
ojan
I'm confused. Why are there no changed tests or expectations with this patch? Or is ...
7 years ago (2013-11-26 17:25:15 UTC) #6
dglazkov
On 2013/11/26 17:25:15, ojan wrote: > I'm confused. Why are there no changed tests or ...
7 years ago (2013-11-26 17:58:51 UTC) #7
ojan
On 2013/11/26 17:58:51, Dimitri Glazkov wrote: > On 2013/11/26 17:25:15, ojan wrote: > > I'm ...
7 years ago (2013-11-26 18:30:46 UTC) #8
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
7 years ago (2013-11-26 23:09:24 UTC) #9
ojan
On 2013/11/26 18:30:46, ojan wrote: > On 2013/11/26 17:58:51, Dimitri Glazkov wrote: > > On ...
7 years ago (2013-11-26 23:24:53 UTC) #10
dglazkov
On 2013/11/26 23:24:53, ojan wrote: > Actually, I think this will show up on our ...
7 years ago (2013-12-02 17:59:03 UTC) #11
ojan
On 2013/12/02 17:59:03, Dimitri Glazkov wrote: > On 2013/11/26 23:24:53, ojan wrote: > > > ...
7 years ago (2013-12-02 18:22:31 UTC) #12
ojan
On 2013/12/02 18:22:31, ojan wrote: > On 2013/12/02 17:59:03, Dimitri Glazkov wrote: > > On ...
7 years ago (2013-12-03 19:44:08 UTC) #13
tasak
Since moving CSSFontSelector to StyleEngine, I have just updated this patch. Since I updated CSSFontSelector's ...
7 years ago (2013-12-10 07:40:48 UTC) #14
tasak
On 2013/12/10 07:40:48, tasak wrote: > Since moving CSSFontSelector to StyleEngine, I have just updated ...
7 years ago (2013-12-13 04:42:27 UTC) #15
tasak
On 2013/12/13 04:42:27, tasak wrote: > I think, we still need resetFontSelector(=recreateFontSelector) when generic > ...
7 years ago (2013-12-13 05:36:46 UTC) #16
ojan
This looks good except the one question... https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/DocumentStyleSheetCollection.cpp File Source/core/dom/DocumentStyleSheetCollection.cpp (left): https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/DocumentStyleSheetCollection.cpp#oldcode167 Source/core/dom/DocumentStyleSheetCollection.cpp:167: engine->resetFontSelector(); Is ...
7 years ago (2013-12-18 00:20:01 UTC) #17
tasak
Thank you for reviewing. https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/DocumentStyleSheetCollection.cpp File Source/core/dom/DocumentStyleSheetCollection.cpp (left): https://codereview.chromium.org/82583005/diff/150001/Source/core/dom/DocumentStyleSheetCollection.cpp#oldcode167 Source/core/dom/DocumentStyleSheetCollection.cpp:167: engine->resetFontSelector(); On 2013/12/18 00:20:02, ojan ...
7 years ago (2013-12-18 05:12:23 UTC) #18
ojan
lgtm https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html File LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html (right): https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html#newcode1 LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html:1: <!doctype html> Nit: here and elsewhere, we usually ...
6 years, 11 months ago (2014-01-03 01:20:43 UTC) #19
tasak
Thank you for reviewing. https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html File LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html (right): https://codereview.chromium.org/82583005/diff/170001/LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html#newcode1 LayoutTests/fast/css/cssom-modify-font-face-rule-expected.html:1: <!doctype html> On 2014/01/03 01:20:44, ...
6 years, 11 months ago (2014-01-07 08:42:11 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/350001
6 years, 11 months ago (2014-01-07 09:51:10 UTC) #21
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=13522
6 years, 11 months ago (2014-01-07 10:12:26 UTC) #22
tasak
dglazkov, would you review this CL? I need lgtm from Source/platform OWNER.
6 years, 11 months ago (2014-01-08 04:43:32 UTC) #23
esprehn
On 2014/01/08 04:43:32, tasak wrote: > dglazkov, would you review this CL? > > I ...
6 years, 11 months ago (2014-01-08 04:45:50 UTC) #24
dglazkov
lgtm
6 years, 11 months ago (2014-01-08 17:34:07 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/350001
6 years, 11 months ago (2014-01-08 17:34:17 UTC) #26
commit-bot: I haz the power
Change committed as 164691
6 years, 11 months ago (2014-01-08 17:43:03 UTC) #27
jochen (gone - plz use gerrit)
A revert of this CL has been created in https://codereview.chromium.org/131403008/ by jochen@chromium.org. The reason for ...
6 years, 11 months ago (2014-01-09 15:41:39 UTC) #28
tasak
On 2014/01/09 15:41:39, jochen wrote: > A revert of this CL has been created in ...
6 years, 11 months ago (2014-01-15 06:18:26 UTC) #29
tasak
I modified StyleEngine::resetFontSelector to use CSSSegmentedFontFaceCache::clear. (Thanks, ksakamoto-san) So we can keep the same FontSelector ...
6 years, 11 months ago (2014-01-23 10:48:10 UTC) #30
dglazkov
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.h#newcode398 Source/core/dom/Document.h:398: CSSFontSelector* fontSelector() const; Please remove this ...
6 years, 11 months ago (2014-01-23 19:15:05 UTC) #31
tasak
Thank you for reviewing. I needed to update m_genericFontFamilySettings owned by CSSFontSelector. This mistake caused ...
6 years, 10 months ago (2014-01-28 07:16:56 UTC) #32
dglazkov
https://codereview.chromium.org/82583005/diff/720001/Source/core/css/CSSFontSelector.h File Source/core/css/CSSFontSelector.h (right): https://codereview.chromium.org/82583005/diff/720001/Source/core/css/CSSFontSelector.h#newcode93 Source/core/css/CSSFontSelector.h:93: void updateGenericFontFamilySettings(Document&); This sounds terrifying. Why do we ever ...
6 years, 10 months ago (2014-01-29 05:11:49 UTC) #33
tasak
Thank you for reviewing. Before updating the patch, I think, I need to understand GenericFontFamilySettings-related ...
6 years, 10 months ago (2014-01-29 10:13:43 UTC) #34
tasak
On 2014/01/29 10:13:43, tasak wrote: > I'm not sure. At least, some layout test depends ...
6 years, 10 months ago (2014-01-29 10:15:44 UTC) #35
dglazkov
On 2014/01/29 10:13:43, tasak wrote: > I think, you are very familiar with GenericFontFamilySettings things. ...
6 years, 10 months ago (2014-01-29 17:59:52 UTC) #36
tasak
On 2014/01/29 17:59:52, Dimitri Glazkov wrote: > On 2014/01/29 10:13:43, tasak wrote: > > I ...
6 years, 10 months ago (2014-02-05 06:09:51 UTC) #37
tasak
I finished fixing browser_tests (r249043). So I will land this patch.
6 years, 10 months ago (2014-02-06 00:54:04 UTC) #38
tasak
The CQ bit was checked by tasak@google.com
6 years, 10 months ago (2014-02-06 00:54:11 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/1000001
6 years, 10 months ago (2014-02-06 00:54:20 UTC) #40
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-06 07:18:24 UTC) #41
commit-bot: I haz the power
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_rel&number=25989
6 years, 10 months ago (2014-02-06 07:18:24 UTC) #42
tasak
The CQ bit was checked by tasak@google.com
6 years, 10 months ago (2014-02-07 02:50:30 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/1270001
6 years, 10 months ago (2014-02-07 02:50:36 UTC) #44
tasak
The CQ bit was unchecked by tasak@google.com
6 years, 10 months ago (2014-02-07 02:54:43 UTC) #45
tasak
The CQ bit was checked by tasak@google.com
6 years, 10 months ago (2014-02-07 03:07:52 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/1270001
6 years, 10 months ago (2014-02-07 03:08:04 UTC) #47
commit-bot: I haz the power
Change committed as 166681
6 years, 10 months ago (2014-02-07 05:43:40 UTC) #48
abarth-chromium
A revert of this CL has been created in https://codereview.chromium.org/157853002/ by abarth@chromium.org. The reason for ...
6 years, 10 months ago (2014-02-07 19:38:43 UTC) #49
tasak
On 2014/02/07 19:38:43, abarth wrote: > A revert of this CL has been created in ...
6 years, 10 months ago (2014-02-12 07:12:08 UTC) #50
tasak
I modified Page::settingsChanged, Settings::setTextAutosizingEnabled and Settings::setTextAutosizingWindowSizeOverride for fixing AwSettingsTest.testLayoutAlgorithmWithTwoViews test regression. Looking at TextAutosizing, it ...
6 years, 10 months ago (2014-02-12 07:15:41 UTC) #51
ojan
+a couple others to look at the TextAutosizing changes.
6 years, 10 months ago (2014-02-12 18:46:38 UTC) #52
skobes
On 2014/02/12 07:15:41, tasak wrote: > I modified Page::settingsChanged, Settings::setTextAutosizingEnabled and > Settings::setTextAutosizingWindowSizeOverride for fixing ...
6 years, 10 months ago (2014-02-12 19:38:41 UTC) #53
tasak
Thank you for comments. On 2014/02/12 19:38:41, skobes wrote: > This sounds reasonable to me ...
6 years, 10 months ago (2014-02-13 04:54:09 UTC) #54
tasak
johnme, would you review this change of TextAutosizing setting? I think, we should use setNeedsLayout ...
6 years, 10 months ago (2014-02-13 05:04:24 UTC) #55
tasak
esprehn, would you review Settings.cpp and Page.cpp's change? The change is for fixing TextAutosizing problem.
6 years, 10 months ago (2014-02-14 05:35:40 UTC) #56
eseidel
lgtm This is really difficult for me to verify correctness of. I just have to ...
6 years, 10 months ago (2014-02-14 05:39:15 UTC) #57
esprehn
On 2014/02/14 05:35:40, tasak wrote: > esprehn, would you review Settings.cpp and Page.cpp's change? > ...
6 years, 10 months ago (2014-02-14 05:42:30 UTC) #58
tasak
On 2014/02/14 05:42:30, esprehn wrote: > On 2014/02/14 05:35:40, tasak wrote: > > esprehn, would ...
6 years, 10 months ago (2014-02-14 05:49:10 UTC) #59
tasak
Thank you for reviewing. I found that FastTextAutosizer needs styleRecalc. So if FastTextAutosizer is enabled, ...
6 years, 10 months ago (2014-02-14 08:27:45 UTC) #60
tasak
The CQ bit was checked by tasak@google.com
6 years, 10 months ago (2014-02-14 08:37:02 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/1820001
6 years, 10 months ago (2014-02-14 08:43:11 UTC) #62
johnme
On 2014/02/13 05:04:24, tasak wrote: > johnme, would you review this change of TextAutosizing setting? ...
6 years, 10 months ago (2014-02-14 12:08:28 UTC) #63
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-14 13:26:12 UTC) #64
commit-bot: I haz the power
Retried try job too often on linux_blink for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink&number=12426
6 years, 10 months ago (2014-02-14 13:26:13 UTC) #65
skobes
lgtm On 2014/02/14 08:27:45, tasak wrote: > So if FastTextAutosizer is enabled, invoke setNeedsStyleRecalc. Otherwise, ...
6 years, 10 months ago (2014-02-14 18:06:53 UTC) #66
skobes
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.cpp#newcode504 Source/core/page/Page.cpp:504: // FastTextAutosizer works with RenderBlock::styleWillChange. This comment is misleading. ...
6 years, 10 months ago (2014-02-14 18:11:02 UTC) #67
skobes
On 2014/02/14 18:06:53, skobes wrote: > I'm not sure why FTA would need style recalc ...
6 years, 10 months ago (2014-02-14 18:14:35 UTC) #68
ojan
On 2014/02/14 18:14:35, skobes wrote: > On 2014/02/14 18:06:53, skobes wrote: > > I'm not ...
6 years, 10 months ago (2014-02-14 18:28:57 UTC) #69
skobes
On 2014/02/14 18:28:57, ojan wrote: > It should only need recalc. If the recalc changes ...
6 years, 10 months ago (2014-02-14 18:31:02 UTC) #70
ojan
On 2014/02/14 18:31:02, skobes wrote: > On 2014/02/14 18:28:57, ojan wrote: > > It should ...
6 years, 10 months ago (2014-02-14 18:42:42 UTC) #71
skobes
On 2014/02/14 18:42:42, ojan wrote: > If the internal state in FTA changes such that ...
6 years, 10 months ago (2014-02-14 18:53:42 UTC) #72
tasak
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 ...
6 years, 10 months ago (2014-02-17 02:39:23 UTC) #73
tasak
The CQ bit was checked by tasak@google.com
6 years, 10 months ago (2014-02-17 02:39:38 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/82583005/2120001
6 years, 10 months ago (2014-02-17 02:39:56 UTC) #75
commit-bot: I haz the power
6 years, 10 months ago (2014-02-17 03:50:30 UTC) #76
Message was sent while issue was closed.
Change committed as 167257

Powered by Google App Engine
This is Rietveld 408576698