Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 87503003: Moving fontSelector from StyleResolver to StyleEngine. (Closed)

Created:
5 years, 2 months ago by tasak
Modified:
5 years, 2 months ago
Reviewers:
dglazkov, ojan
CC:
blink-reviews, krit, zoltan1, bemjb+rendering_chromium.org, eae+blinkwatch, leviw+renderwatch, dglazkov+blink, Rik, apavlov+blink_chromium.org, adamk+blink_chromium.org, jchaffraix+rendering, darktears, pdr, f(malita), Stephen Chennney, aandrey+blink_chromium.org, rwlbuis, Kunihiko Sakamoto
Visibility:
Public.

Description

Moving fontSelector from StyleResolver to StyleEngine. To avoid dispatching fontLoaded event multiple times for the same font (caused by clearResolver), we should change fontSelector's owner. BUG=236298, 305885 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163369

Patch Set 1 : #

Patch Set 2 : Fixed fast/css/remove-style-after-remove-font-face.html #

Patch Set 3 : Rebased #

Patch Set 4 : Reverted hasAnyFontFaceRule #

Total comments: 4

Patch Set 5 : Should not create FontSelector when document.frame() == 0 #

Patch Set 6 : Removed document.fontSelector() #

Total comments: 10

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -50 lines) Patch
M Source/core/css/FontFaceSet.cpp View 1 2 3 4 5 4 chunks +4 lines, -4 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.h View 1 2 3 3 chunks +0 lines, -4 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 2 3 4 5 6 12 chunks +9 lines, -25 lines 0 comments Download
M Source/core/dom/Document.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 3 chunks +6 lines, -1 line 0 comments Download
M Source/core/dom/DocumentStyleSheetCollection.cpp View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M Source/core/dom/StyleEngine.h View 1 2 3 4 2 chunks +5 lines, -1 line 0 comments Download
M Source/core/dom/StyleEngine.cpp View 1 2 3 4 5 6 6 chunks +31 lines, -5 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.cpp View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/rendering/RenderListBox.cpp View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/rendering/RenderMenuList.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M Source/core/rendering/svg/RenderSVGInlineText.cpp View 1 2 3 4 5 2 chunks +1 line, -2 lines 0 comments Download
M Source/web/AutofillPopupMenuClient.cpp View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
tasak
5 years, 2 months ago (2013-11-26 07:46:31 UTC) #1
tasak
I temporary added StyleEngine::resetFontSelector() and CSSFontSelector::hasAnyFontFaceRule() to move fontSelector. I will remove the methods in ...
5 years, 2 months ago (2013-11-26 09:56:22 UTC) #2
ojan
lgtm
5 years, 2 months ago (2013-11-26 17:22:23 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/87503003/40001
5 years, 2 months ago (2013-11-26 23:10:37 UTC) #4
commit-bot: I haz the power
Failed to apply patch for Source/core/dom/StyleEngine.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
5 years, 2 months ago (2013-11-26 23:10:43 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/87503003/60001
5 years, 2 months ago (2013-11-27 05:26:42 UTC) #6
commit-bot: I haz the power
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_rel&number=12235
5 years, 2 months ago (2013-11-27 09:13:54 UTC) #7
tasak
On 2013/11/27 09:13:54, I haz the power (commit-bot) wrote: > Retried try job too often ...
5 years, 2 months ago (2013-11-27 10:17:14 UTC) #8
tasak
Would you review this CL again? On 2013/11/27 10:17:14, tasak wrote: > On 2013/11/27 09:13:54, ...
5 years, 2 months ago (2013-11-29 09:03:20 UTC) #9
tasak
On 2013/11/29 09:03:20, tasak wrote: > > This patch affects mac-blink-rel (and mac-blink)'s: > > ...
5 years, 2 months ago (2013-12-02 05:30:50 UTC) #10
dglazkov
lgtm++ with nits https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/Document.cpp#newcode1959 Source/core/dom/Document.cpp:1959: CSSFontSelector* Document::fontSelector() const This function doesn't ...
5 years, 2 months ago (2013-12-02 17:47:41 UTC) #11
dglazkov
On 2013/12/02 05:30:50, tasak wrote: > I think, we should do: > > - #if ...
5 years, 2 months ago (2013-12-02 17:48:19 UTC) #12
Nico
(nit: according to comment 76 on crbug.com/236298 this patch changes behavior, hence the cl description ...
5 years, 2 months ago (2013-12-02 18:36:24 UTC) #13
tasak
Thank you for reviewing. I modified StyleEngine not to create CSSFontSelector when document.frame() is null. ...
5 years, 2 months ago (2013-12-05 05:09:43 UTC) #14
dglazkov
godspeed.
5 years, 2 months ago (2013-12-05 05:41:16 UTC) #15
ojan
A couple more questions. Sorry to slow this down. I'm really eager for this patch ...
5 years, 2 months ago (2013-12-05 18:57:14 UTC) #16
tasak
https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEngine.cpp File Source/core/dom/StyleEngine.cpp (right): https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEngine.cpp#newcode74 Source/core/dom/StyleEngine.cpp:74: , m_fontSelector(document.frame() ? CSSFontSelector::create(&document) : 0) On 2013/12/05 18:57:15, ...
5 years, 2 months ago (2013-12-06 07:13:46 UTC) #17
ojan
lgtm
5 years, 2 months ago (2013-12-07 01:04:32 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tasak@google.com/87503003/200001
5 years, 2 months ago (2013-12-07 01:06:02 UTC) #19
commit-bot: I haz the power
5 years, 2 months ago (2013-12-07 03:05:31 UTC) #20
Message was sent while issue was closed.
Change committed as 163369

Powered by Google App Engine
This is Rietveld 408576698