New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Archived
Owner:
OOO until NaN
Closed: Jan 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 315783



Sign in to add a comment
link

Issue 324991: CSSFontSelector should not need to hold on a Document pointer

Reported by dglazkov@chromium.org, Dec 3 2013 Project Member

Issue description

There are several reasons it does that now:

1) To grab at generic font family settings in CSSFontSelector:getFontData

2) To provide document value for LoadFontPromiseResolver

3) To grab at settings in FontFace::createCSSFontFace

4) Some UseCounter and other miscellany in CSSFontFace.

We should teach all these places to either forget about the document or learn to obtain the document by other means.
 

Comment 1 by dglazkov@chromium.org, Dec 3 2013

Blocking: chromium:315783

Comment 2 by bugdroid1@chromium.org, Dec 3 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=163117

------------------------------------------------------------------------
r163117 | dglazkov@chromium.org | 2013-12-03T22:51:06.338347Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/blink_platform.gypi?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebSettingsImpl.cpp?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FontBuilder.cpp?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontSelector.cpp?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/InternalSettings.cpp?r1=163117&r2=163116&pathrev=163117
   A http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/GenericFontFamilySettings.cpp?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/page/Settings.cpp?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/InspectorOverlay.cpp?r1=163117&r2=163116&pathrev=163117
   A http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/GenericFontFamilySettings.h?r1=163117&r2=163116&pathrev=163117
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/page/Settings.h?r1=163117&r2=163116&pathrev=163117

Introduce GenericFontFamilySettings.

The newborn will hold all settings, related to generic font
families, and will be used by Settings.

Aside from mechanical extraction, there is one functional change:
setting font families will require explicit call to recalc style
on all frames. Thankfully, this is already done in most places,
aside from InternalSettings, where I added it.

R=eae,ksakamoto@chromium.org
BUG= 324991 

Review URL: https://codereview.chromium.org/100653002
------------------------------------------------------------------------

Comment 3 by bugdroid1@chromium.org, Dec 4 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=163130

------------------------------------------------------------------------
r163130 | dglazkov@chromium.org | 2013-12-04T00:34:52.089950Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontSelector.h?r1=163130&r2=163129&pathrev=163130
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/GenericFontFamilySettings.h?r1=163130&r2=163129&pathrev=163130
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontSelector.cpp?r1=163130&r2=163129&pathrev=163130
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/fonts/GenericFontFamilySettings.cpp?r1=163130&r2=163129&pathrev=163130

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

Review URL: https://codereview.chromium.org/93783005
------------------------------------------------------------------------

Comment 4 by bugdroid1@chromium.org, Dec 4 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=163152

------------------------------------------------------------------------
r163152 | dglazkov@chromium.org | 2013-12-04T09:54:22.964303Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontFace.h?r1=163152&r2=163151&pathrev=163152
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSSegmentedFontFaceCache.h?r1=163152&r2=163151&pathrev=163152
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleResolver.cpp?r1=163152&r2=163151&pathrev=163152
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontSelector.h?r1=163152&r2=163151&pathrev=163152
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontFace.cpp?r1=163152&r2=163151&pathrev=163152
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSSegmentedFontFaceCache.cpp?r1=163152&r2=163151&pathrev=163152
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSFontSelector.cpp?r1=163152&r2=163151&pathrev=163152

CSSSegmentedFontFaceCache should not know about Document.

1) Remove dependency on Document (and core/dom, for that matter)
in CSSSegmentedFontFaceCache
2) Shorten function names to make the sounds sweet.

This removes another place where CSSFontSelector is expected to know
about Document.

R=eae,eseidel,ksakamoto@chromium.org
BUG= 324991 

Review URL: https://codereview.chromium.org/103273002
------------------------------------------------------------------------

Comment 5 by lafo...@chromium.org, Sep 23 2015

Labels: Hotlist-Recharge Hotlist-Recharge-Stale
This issue likely requires triage.  The current issue owner maybe inactive (i.e. hasn't fixed an issue in the last 30 days).  It has also not been modified in a year (prior to this update).  Thanks for helping out!

-Anthony

Comment 6 by dglazkov@chromium.org, Jan 4 2016

Status: Archived
The world has moved on.

Sign in to add a comment