New issue
Advanced search Search tips

Issue 685945 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Address FIXMEs in font code

Project Member Reported by meade@chromium.org, Jan 27 2017

Issue description

Specifically I'm looking at these two:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSFontSelector.h?l=92
Move m_fontFaceCache to Document or StyleEngine


https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/FontFaceCache.h?l=50
Remove CSSFontSelector as argument [to FontFaceCache::add]. Passing CSSFontSelector here is a result of egregious spaghettification in FontFace/FontFaceSet.

Both of these FIXMEs were added by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f


 

Comment 1 by meade@chromium.org, Jan 27 2017

Components: Blink>Fonts

Comment 2 by drott@chromium.org, Jan 27 2017

👍
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e07e958d62db5bb606c0dbbb446d2a25e33c3db1

commit e07e958d62db5bb606c0dbbb446d2a25e33c3db1
Author: meade <meade@chromium.org>
Date: Fri Feb 24 03:15:53 2017

Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code. This FIXME was added in
2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.

BUG=685945

Review-Url: https://codereview.chromium.org/2624893003
Cr-Commit-Position: refs/heads/master@{#452734}

[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/css/CSSFontSelector.cpp
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/css/CSSFontSelector.h
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/css/FontFaceSet.cpp
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/css/FontFaceSet.h
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/e07e958d62db5bb606c0dbbb446d2a25e33c3db1/third_party/WebKit/Source/core/dom/StyleEngine.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36809b6d2ff54018e85e75197b52015173aab25d

commit 36809b6d2ff54018e85e75197b52015173aab25d
Author: erikchen <erikchen@chromium.org>
Date: Wed Mar 01 04:47:04 2017

Revert of Move the FontFaceCache stored in CSSFontSelector to be stored in Document. (patchset #5 id:80001 of https://codereview.chromium.org/2624893003/ )

Reason for revert:
Reverted on suspicion of causing the top renderer crash on Mac.

https://bugs.chromium.org/p/chromium/issues/detail?id=696231

Original issue's description:
> Move the FontFaceCache stored in CSSFontSelector to be stored in Document.
>
> This addresses a FIXME found in the code. This FIXME was added in
> 2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.
>
> BUG=685945
>
> Review-Url: https://codereview.chromium.org/2624893003
> Cr-Commit-Position: refs/heads/master@{#452734}
> Committed: https://chromium.googlesource.com/chromium/src/+/e07e958d62db5bb606c0dbbb446d2a25e33c3db1

TBR=drott@chromium.org,eae@chromium.org,sashab@chromium.org,meade@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=685945

Review-Url: https://codereview.chromium.org/2728513002
Cr-Commit-Position: refs/heads/master@{#453842}

[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/css/CSSFontSelector.cpp
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/css/CSSFontSelector.h
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/css/FontFaceSet.cpp
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/css/FontFaceSet.h
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/36809b6d2ff54018e85e75197b52015173aab25d/third_party/WebKit/Source/core/dom/StyleEngine.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 1 2017

Labels: merge-merged-3026
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4f5718bb7569a5a265f173183329cd6ebf55e5e

commit b4f5718bb7569a5a265f173183329cd6ebf55e5e
Author: Koji Ishii <kojii@chromium.org>
Date: Wed Mar 01 05:19:06 2017

Merge 3026: Revert of Move the FontFaceCache stored in CSSFontSelector to be stored in Document. (patchset #5 id:80001 of https://codereview.chromium.org/2624893003/ )

Merge requested by manoranjanr.

Reason for revert:
Reverted on suspicion of causing the top renderer crash on Mac.

https://bugs.chromium.org/p/chromium/issues/detail?id=696231

Original issue's description:
> Move the FontFaceCache stored in CSSFontSelector to be stored in Document.
>
> This addresses a FIXME found in the code. This FIXME was added in
> 2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.
>
> BUG=685945
>
> Review-Url: https://codereview.chromium.org/2624893003
> Cr-Commit-Position: refs/heads/master@{#452734}
> Committed: https://chromium.googlesource.com/chromium/src/+/e07e958d62db5bb606c0dbbb446d2a25e33c3db1

TBR=drott@chromium.org,eae@chromium.org,sashab@chromium.org,meade@chromium.org
BUG=685945

Review-Url: https://codereview.chromium.org/2728513002
Cr-Commit-Position: refs/heads/master@{#453842}
(cherry picked from commit 36809b6d2ff54018e85e75197b52015173aab25d)

Review-Url: https://codereview.chromium.org/2721273002 .
Cr-Commit-Position: refs/branch-heads/3026@{#3}
Cr-Branched-From: fe586ab75aca1b8ab839db23bceac5f621389fed-refs/heads/master@{#453454}

[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/css/CSSFontSelector.cpp
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/css/CSSFontSelector.h
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/css/FontFaceSet.cpp
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/css/FontFaceSet.h
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/b4f5718bb7569a5a265f173183329cd6ebf55e5e/third_party/WebKit/Source/core/dom/StyleEngine.cpp

Comment 6 by meade@chromium.org, May 5 2017

Cc: -e...@chromium.org meade@chromium.org
Owner: e...@chromium.org
Assigning to eae since I'm not working on this any more. Sorry I didn't end up actually fixing anything :/
Cc: -meade@chromium.org e...@chromium.org
Labels: -Type-Bug Type-Task
Owner: ----
Status: Available (was: Assigned)

Sign in to add a comment