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

Issue 1591883002: Add plumbing in blink to allow overriding the default font collection. (Closed)

Created:
4 years, 11 months ago by Ilya Kulshin
Modified:
4 years, 10 months ago
CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-platform-graphics_chromium.org, bungeman-skia, Rik, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jam, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add plumbing in blink to allow overriding the skia font manager. This allows Chromium to use a custom font collection with skia, without having to patch the vtable of the DirectWrite factory. Note that this change depends on skia change https://codereview.chromium.org/1607083003/ which must land first. BUG= Committed: https://crrev.com/0e752e79260c775500c78cc351887fc19410bd8b Cr-Commit-Position: refs/heads/master@{#374482}

Patch Set 1 #

Patch Set 2 : Delete dead code #

Patch Set 3 : Delete some more dead code. #

Patch Set 4 : Add an API to allow overriding WebKit's font manager creation. #

Patch Set 5 : Merge to head #

Patch Set 6 : Clean up extra headers and declarations #

Total comments: 5

Patch Set 7 : Smartpointer refactoring #

Patch Set 8 : Remove setDirectWriteFactory #

Total comments: 7

Patch Set 9 : More smartpointer refactoring #

Total comments: 8

Patch Set 10 : Change angle brackets to quotes #

Patch Set 11 : Merge to head #

Patch Set 12 : Remove another instance of PatchDWriteFactory #

Patch Set 13 : Merge to skia changes #

Total comments: 2

Patch Set 14 : Fix includes #

Patch Set 15 : Add back the option to use blink with DirectWrite without specifying a font manager, which was appa… #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -67 lines) Patch
M content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +9 lines, -22 lines 0 comments Download
M content/child/font_warmup_win.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -25 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/FontCache.h View 1 2 3 4 5 6 7 8 3 chunks +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/FontCache.cpp View 1 2 3 4 5 6 7 8 2 chunks +11 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +10 lines, -10 lines 5 comments Download
M third_party/WebKit/Source/web/win/WebFontRendering.cpp View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -2 lines 0 comments Download
M third_party/WebKit/public/web/win/WebFontRendering.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 58 (18 generated)
Ilya Kulshin
ptal, or feel free to refer to other reviewers if someone else would be more ...
4 years, 11 months ago (2016-01-20 22:58:44 UTC) #3
tfarina
Could you update your subject to remove "webkit"? third_party/WebKit is Blink. So just say: "Add ...
4 years, 11 months ago (2016-01-21 02:14:36 UTC) #5
bungeman-chromium
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode103 third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:103: m_fontManager = skia::AdoptRef(SkFontMgr_New_DirectWrite(s_directWriteFactory)); This appears to be the only ...
4 years, 11 months ago (2016-01-21 15:48:13 UTC) #8
jbroman
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/FontCache.h File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/FontCache.h#newcode181 third_party/WebKit/Source/platform/fonts/FontCache.h:181: skia::RefPtr<SkFontMgr> m_fontManager; drive-by: Blink currently uses WTF::RefPtr for Skia ...
4 years, 11 months ago (2016-01-21 15:51:31 UTC) #9
Ilya Kulshin
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/FontCache.h File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/FontCache.h#newcode181 third_party/WebKit/Source/platform/fonts/FontCache.h:181: skia::RefPtr<SkFontMgr> m_fontManager; On 2016/01/21 15:51:31, jbroman wrote: > drive-by: ...
4 years, 11 months ago (2016-01-21 20:59:22 UTC) #10
bungeman-skia
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode103 third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:103: m_fontManager = skia::AdoptRef(SkFontMgr_New_DirectWrite(s_directWriteFactory)); On 2016/01/21 20:59:22, Ilya Kulshin wrote: ...
4 years, 11 months ago (2016-01-21 21:11:41 UTC) #12
Ilya Kulshin
On 2016/01/21 21:11:41, bungeman1 wrote: > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp > File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode103 > ...
4 years, 11 months ago (2016-01-21 22:15:55 UTC) #13
bungeman-chromium
https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Source/platform/fonts/FontCache.cpp File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Source/platform/fonts/FontCache.cpp#newcode56 third_party/WebKit/Source/platform/fonts/FontCache.cpp:56: #if OS(WIN) This doesn't seem to be needed anymore. ...
4 years, 11 months ago (2016-01-21 22:25:09 UTC) #14
blink-reviews
On Thu, Jan 21, 2016 at 12:59 PM, <kulshin@chromium.org> wrote: > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/FontCache.h > ...
4 years, 11 months ago (2016-01-21 22:30:01 UTC) #15
chromium-reviews
On Thu, Jan 21, 2016 at 12:59 PM, <kulshin@chromium.org> wrote: > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Source/platform/fonts/FontCache.h > ...
4 years, 11 months ago (2016-01-21 22:30:02 UTC) #16
tfarina
https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/public/web/win/WebFontRendering.h File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/public/web/win/WebFontRendering.h#newcode9 third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <SkFontHost.h> shouldn't we include this by its full ...
4 years, 11 months ago (2016-01-21 22:45:28 UTC) #17
Ilya Kulshin
https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Source/platform/fonts/FontCache.cpp File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Source/platform/fonts/FontCache.cpp#newcode56 third_party/WebKit/Source/platform/fonts/FontCache.cpp:56: #if OS(WIN) On 2016/01/21 22:25:09, bungeman2 wrote: > This ...
4 years, 10 months ago (2016-01-27 22:07:25 UTC) #19
bungeman-skia
One nit, will look more later. https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/public/web/win/WebFontRendering.h File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/public/web/win/WebFontRendering.h#newcode9 third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <third_party/skia/include/core/SkFontHost.h> Any ...
4 years, 10 months ago (2016-01-27 22:18:30 UTC) #20
Ilya Kulshin
+scottmg. Scott, can you review /content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc and /content/common/font_warmup_win.cc https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/public/web/win/WebFontRendering.h File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/public/web/win/WebFontRendering.h#newcode9 third_party/WebKit/public/web/win/WebFontRendering.h:9: #include ...
4 years, 10 months ago (2016-01-29 19:49:41 UTC) #22
scottmg
Those two lgtm. fyi, an involved files just moved because browser shouldn't be using blink ...
4 years, 10 months ago (2016-01-29 20:04:54 UTC) #23
esprehn
What's the reason for this change? What bug does it fix? It would be nice ...
4 years, 10 months ago (2016-01-30 10:52:46 UTC) #24
Ilya Kulshin
On 2016/01/30 10:52:46, esprehn wrote: > What's the reason for this change? What bug does ...
4 years, 10 months ago (2016-02-02 22:54:43 UTC) #26
bungeman-skia
https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc#newcode19 content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:19: #include "skia/include/ports/SkTypeface_win.h" These two (SkFontMgr.h and SkTypeface_win.h) are in ...
4 years, 10 months ago (2016-02-03 21:55:53 UTC) #27
Ilya Kulshin
https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc#newcode19 content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:19: #include "skia/include/ports/SkTypeface_win.h" On 2016/02/03 21:55:53, bungeman1 wrote: > These ...
4 years, 10 months ago (2016-02-04 00:38:33 UTC) #28
Ilya Kulshin
ptal. The skia portion of this change has landed, so if there's no more comments ...
4 years, 10 months ago (2016-02-04 21:30:55 UTC) #29
bungeman-chromium
lgtm https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode104 third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:104: m_fontManager = adoptRef(SkFontMgr_New_DirectWrite()); I'm assuming this is only ...
4 years, 10 months ago (2016-02-04 21:40:32 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591883002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591883002/300001
4 years, 10 months ago (2016-02-04 21:56:44 UTC) #32
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-04 22:06:12 UTC) #34
Ilya Kulshin
https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode104 third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:104: m_fontManager = adoptRef(SkFontMgr_New_DirectWrite()); On 2016/02/04 21:40:32, bungeman2 wrote: > ...
4 years, 10 months ago (2016-02-05 02:02:47 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591883002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591883002/300001
4 years, 10 months ago (2016-02-05 02:05:33 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/142934)
4 years, 10 months ago (2016-02-05 02:15:26 UTC) #40
Ilya Kulshin
+pfeldman, could I get an owners signoff for this change?
4 years, 10 months ago (2016-02-08 20:15:23 UTC) #42
pfeldman
platform lgtm
4 years, 10 months ago (2016-02-09 19:16:08 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591883002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591883002/300001
4 years, 10 months ago (2016-02-09 20:04:30 UTC) #45
commit-bot: I haz the power
Committed patchset #15 (id:300001)
4 years, 10 months ago (2016-02-09 21:52:21 UTC) #47
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/0e752e79260c775500c78cc351887fc19410bd8b Cr-Commit-Position: refs/heads/master@{#374482}
4 years, 10 months ago (2016-02-09 21:54:24 UTC) #49
iclelland
A revert of this CL (patchset #15 id:300001) has been created in https://codereview.chromium.org/1682813004/ by iclelland@chromium.org. ...
4 years, 10 months ago (2016-02-09 22:10:37 UTC) #50
iclelland
On 2016/02/09 22:10:37, iclelland wrote: > A revert of this CL (patchset #15 id:300001) has ...
4 years, 10 months ago (2016-02-09 22:15:40 UTC) #51
bungeman-chromium
https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode101 third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:101: adopted(s_fontManager); The adoption checks are only done in debug, ...
4 years, 10 months ago (2016-02-09 22:25:18 UTC) #52
jbroman
https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Source/platform/fonts/FontCache.cpp File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Source/platform/fonts/FontCache.cpp#newcode153 third_party/WebKit/Source/platform/fonts/FontCache.cpp:153: s_fontManager = fontManager.get(); should be "s_fontManager = fontManager.leakRef();" https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Source/platform/fonts/FontCache.cpp#newcode155 ...
4 years, 10 months ago (2016-02-09 22:31:31 UTC) #53
jbroman
> https://codeeview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode101 > third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:101: adopted(s_fontManager); > On 2016/02/09 at 22:25:17, bungeman2 wrote: > > The ...
4 years, 10 months ago (2016-02-09 22:32:03 UTC) #54
bungeman-chromium
https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode101 third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:101: adopted(s_fontManager); On 2016/02/09 22:25:17, bungeman2 wrote: > The adoption ...
4 years, 10 months ago (2016-02-09 22:36:06 UTC) #55
Ilya Kulshin
On 2016/02/09 22:36:06, bungeman2 wrote: > https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp > File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): > > https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp#newcode101 > ...
4 years, 10 months ago (2016-02-11 01:30:31 UTC) #56
Ilya Kulshin
On 2016/02/11 01:30:31, Ilya Kulshin wrote: > On 2016/02/09 22:36:06, bungeman2 wrote: > > > ...
4 years, 10 months ago (2016-02-11 01:38:59 UTC) #57
bungeman-chromium
4 years, 10 months ago (2016-02-11 16:10:29 UTC) #58
Message was sent while issue was closed.
On 2016/02/11 01:38:59, Ilya Kulshin wrote:
> On 2016/02/11 01:30:31, Ilya Kulshin wrote:
> > On 2016/02/09 22:36:06, bungeman2 wrote:
> > >
> >
>
https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1591883002/diff/300001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:101:
> > > adopted(s_fontManager);
> > > On 2016/02/09 22:25:17, bungeman2 wrote:
> > > > The adoption checks are only done in debug, so 'adopted' only exists in
> > debug.
> > > 
> > > Note that adding 'using WTF::adopted' should work to fix release. I
haven't
> > > verified that though (or if that is a good idea). This is being provided
in
> > > debug from sk_ref_cnt_ext_debug.h (and isn't from
sk_ref_cnt_ext_release.h).
> I
> > > would rely on someone more familiar with Blink's reference counting for
real
> > > advice though.
> > > 
> > > This seems to be complicated by the 'rel' trybots actually being 'debug
with
> > > optimizations' as opposed to the 'rel' bots on the waterfall which are
> > actually
> > > 'release'.
> > 
> > I checked, and it looks like changing the call to "WTF::adopted(...)" does
in
> > fact fix it, but have no idea why - if it's a namespace issue it should've
> been
> > broken in debug as well, and if it's not a namespace issue adding a
namespace
> > shouldn't matter? Can anyone shed some light on this?
> 
> Never mind, I finally noticed the "using WTF::adopted" declaration in the
> debug-only skia header. Now it makes sense. Any objections to adding analogous
> declarations to the release version of the header?

This is a bit weird. I'm not sure why the "using WTF::adopted" and "using
WTF::requireAdoption" are in sk_ref_cnt_ext_debug.h . I just removed them
locally, and everything seems to work ok. Apparently, I'm credited for these
lines, though I got them from https://codereview.chromium.org/29763002 (so you
could ask junov if he remembers why they're there). I think the best thing might
be to remove those lines from sk_ref_cnt_ext_debug.h (at least eventually), and
just call WTF::adopted here explicitly. It looks like maybe an attempt at using
ADL for adopted, but this isn't quite the right and doesn't seem needed.

Powered by Google App Engine
This is Rietveld 408576698