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

Issue 93783005: Store a copy of generic font settings at CSSFontSelector. (Closed)

Created:
5 years, 2 months ago by dglazkov
Modified:
5 years, 2 months ago
CC:
blink-reviews, dglazkov+blink, apavlov+blink_chromium.org, darktears
Visibility:
Public.

Description

Store a copy of generic font settings at CSSFontSelector. 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. Eventually, CSSFontSelector hopes to become a per-Document font context, which knows nothing about the Document itself, but provides an API that a Document could use. This also fixes bug 236298, since determining generic font no longer relies on CSSFontSelector::m_document. BUG=324991, 236298 R=eae,eseidel,ksakamoto@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163130

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -17 lines) Patch
M Source/core/css/CSSFontSelector.h View 3 chunks +4 lines, -0 lines 1 comment Download
M Source/core/css/CSSFontSelector.cpp View 2 chunks +12 lines, -16 lines 0 comments Download
M Source/platform/fonts/GenericFontFamilySettings.h View 2 chunks +5 lines, -1 line 0 comments Download
M Source/platform/fonts/GenericFontFamilySettings.cpp View 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
dglazkov
This also fixes bug 236298, since determining generic font no longer relies on CSSFontSelector::m_document.
5 years, 2 months ago (2013-12-03 22:03:44 UTC) #1
eseidel
lgtm Kerpow!
5 years, 2 months ago (2013-12-03 22:05:36 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dglazkov@chromium.org/93783005/1
5 years, 2 months ago (2013-12-03 22:05:46 UTC) #3
commit-bot: I haz the power
Failed to apply patch for Source/core/css/CSSFontSelector.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
5 years, 2 months ago (2013-12-03 22:05:49 UTC) #4
dglazkov
On 2013/12/03 22:05:49, I haz the power (commit-bot) wrote: > Failed to apply patch for ...
5 years, 2 months ago (2013-12-03 22:07:13 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dglazkov@chromium.org/93783005/1
5 years, 2 months ago (2013-12-03 22:55:02 UTC) #6
commit-bot: I haz the power
Change committed as 163130
5 years, 2 months ago (2013-12-04 00:34:55 UTC) #7
Nico
No test!!
5 years, 2 months ago (2013-12-04 00:46:12 UTC) #8
esprehn
This object is pretty big, but I guess we only have one per page/tree scope? ...
5 years, 2 months ago (2013-12-04 00:57:22 UTC) #9
dglazkov
On 2013/12/04 00:46:12, Nico wrote: > No test!! Sorry :-\ At this point, it's not ...
5 years, 2 months ago (2013-12-04 04:48:42 UTC) #10
dglazkov
5 years, 2 months ago (2013-12-04 04:51:34 UTC) #11
Message was sent while issue was closed.
On 2013/12/04 00:57:22, esprehn wrote:
> This object is pretty big, but I guess we only have one per page/tree scope?
Is
> there a reason not to store a reference or a pointer to object?
> 
>
https://codereview.chromium.org/93783005/diff/1/Source/core/css/CSSFontSelect...
> File Source/core/css/CSSFontSelector.h (right):
> 
>
https://codereview.chromium.org/93783005/diff/1/Source/core/css/CSSFontSelect...
> Source/core/css/CSSFontSelector.h:108: GenericFontFamilySettings
> m_genericFontFamilySettings;
> Why not store a reference to it?

I actually started writing it as a RefCounted class, but then the aesthetics got
the best of me: when sharing an object, there's a feature/bug where you can
tweak the object in one place and see changes in another place. That would be a
loaded foot gun with this particular class.

Powered by Google App Engine
This is Rietveld 408576698