Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionAdd 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
Messages
Total messages: 58 (18 generated)
Description was changed from ========== Add plumbing in webkit and blink to allow overriding the default font collection. BUG= ========== to ========== Add plumbing in webkit and blink to allow overriding the skia font manager. Note that this change depends on skia change https://codereview.chromium.org/1607083003/ which must land first. BUG= ==========
kulshin@chromium.org changed reviewers: + dglazkov+blink@chromium.org
ptal, or feel free to refer to other reviewers if someone else would be more appropriate.
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
Could you update your subject to remove "webkit"? third_party/WebKit is Blink. So just say: "Add plumbing in blink...", otherwise it is very confusing. Unless you are refering to something else with "webkit"? Thanks,
Description was changed from ========== Add plumbing in webkit and blink to allow overriding the skia font manager. Note that this change depends on skia change https://codereview.chromium.org/1607083003/ which must land first. BUG= ========== to ========== Add plumbing in blink to allow overriding the skia font manager. Note that this change depends on skia change https://codereview.chromium.org/1607083003/ which must land first. BUG= ==========
bungeman@chromium.org changed reviewers: + bungeman@chromium.org
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... 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 place s_directWriteFactory is used. It appears that with this code change this branch will never be taken, so s_directWriteFactory and related should be removed.
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.h:181: skia::RefPtr<SkFontMgr> m_fontManager; drive-by: Blink currently uses WTF::RefPtr for Skia objects, rather than skia::RefPtr.
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... 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: Blink currently uses WTF::RefPtr for Skia objects, rather than > skia::RefPtr. Done. I switched back to raw pointers for the public API, since I didn't want to force yet another smart pointer class on callers. Switched to WTF::RefPtr elsewhere. https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:103: m_fontManager = skia::AdoptRef(SkFontMgr_New_DirectWrite(s_directWriteFactory)); On 2016/01/21 15:48:13, bungeman2 wrote: > This appears to be the only place s_directWriteFactory is used. It appears that > with this code change this branch will never be taken, so s_directWriteFactory > and related should be removed. s_useDirectWrite is set via setUseDirectWrite, which is still called from https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... and https://code.google.com/p/chromium/codesearch#chromium/src/content/ppapi_plug.... setDirectWriteFactory can still be called from https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fon..., which is called if ShouldUseDirectWriteFontProxyFieldTrial returns false. For now, it is still needed, but I will make a note to remove it when deleting the old chrome font cache codepath.
bungeman@google.com changed reviewers: + bungeman@google.com
https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... 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: > On 2016/01/21 15:48:13, bungeman2 wrote: > > This appears to be the only place s_directWriteFactory is used. It appears > that > > with this code change this branch will never be taken, so s_directWriteFactory > > and related should be removed. > > s_useDirectWrite is set via setUseDirectWrite, which is still called from > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > and > https://code.google.com/p/chromium/codesearch#chromium/src/content/ppapi_plug.... > setDirectWriteFactory can still be called from > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fon..., > which is called if ShouldUseDirectWriteFontProxyFieldTrial returns false. For > now, it is still needed, but I will make a note to remove it when deleting the > old chrome font cache codepath. Sure, s_useDirectWrite is still called (and should also be removed) but you'll still never get here, because every path which will set it to true will also already have set the s_fontManager (and if it doesn't, it should be). In other words, this CL is adding code to support s_directWriteFactory and s_useDirectWrite, but it doesn't seem that this is necessary (or wanted).
On 2016/01/21 21:11:41, bungeman1 wrote: > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp (right): > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > 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: > > On 2016/01/21 15:48:13, bungeman2 wrote: > > > This appears to be the only place s_directWriteFactory is used. It appears > > that > > > with this code change this branch will never be taken, so > s_directWriteFactory > > > and related should be removed. > > > > s_useDirectWrite is set via setUseDirectWrite, which is still called from > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > and > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/ppapi_plug.... > > setDirectWriteFactory can still be called from > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fon..., > > which is called if ShouldUseDirectWriteFontProxyFieldTrial returns false. For > > now, it is still needed, but I will make a note to remove it when deleting the > > old chrome font cache codepath. > > Sure, s_useDirectWrite is still called (and should also be removed) but you'll > still never get here, because every path which will set it to true will also > already have set the s_fontManager (and if it doesn't, it should be). > > In other words, this CL is adding code to support s_directWriteFactory and > s_useDirectWrite, but it doesn't seem that this is necessary (or wanted). Ah, I see what you mean now. Done.
https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:56: #if OS(WIN) This doesn't seem to be needed anymore. https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:10: #include <SkFontMgr.h> Not sure what the current policy is on forward declarations, but I think this can just be class SkFontMgr;
On Thu, Jan 21, 2016 at 12:59 PM, <kulshin@chromium.org> wrote: > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/FontCache.h (right): > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > 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: Blink currently uses WTF::RefPtr for Skia objects, rather >> > than > >> skia::RefPtr. >> > > Done. I switched back to raw pointers for the public API, since I didn't > want to force yet another smart pointer class on callers. Switched to > WTF::RefPtr elsewhere. Erm, please don't pass ownership with a raw pointer. If you're getting a raw ref-counted pointer from skia please put it into a smart pointer before you do anything else with it. > > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp > (right): > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:103: > m_fontManager = > skia::AdoptRef(SkFontMgr_New_DirectWrite(s_directWriteFactory)); > On 2016/01/21 15:48:13, bungeman2 wrote: > >> This appears to be the only place s_directWriteFactory is used. It >> > appears that > >> with this code change this branch will never be taken, so >> > s_directWriteFactory > >> and related should be removed. >> > > s_useDirectWrite is set via setUseDirectWrite, which is still called > from > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > and > > https://code.google.com/p/chromium/codesearch#chromium/src/content/ppapi_plug... > . > setDirectWriteFactory can still be called from > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fon... > , > which is called if ShouldUseDirectWriteFontProxyFieldTrial returns > false. For now, it is still needed, but I will make a note to remove it > when deleting the old chrome font cache codepath. > > https://codereview.chromium.org/1591883002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Jan 21, 2016 at 12:59 PM, <kulshin@chromium.org> wrote: > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/FontCache.h (right): > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > 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: Blink currently uses WTF::RefPtr for Skia objects, rather >> > than > >> skia::RefPtr. >> > > Done. I switched back to raw pointers for the public API, since I didn't > want to force yet another smart pointer class on callers. Switched to > WTF::RefPtr elsewhere. Erm, please don't pass ownership with a raw pointer. If you're getting a raw ref-counted pointer from skia please put it into a smart pointer before you do anything else with it. > > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp > (right): > > > https://codereview.chromium.org/1591883002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:103: > m_fontManager = > skia::AdoptRef(SkFontMgr_New_DirectWrite(s_directWriteFactory)); > On 2016/01/21 15:48:13, bungeman2 wrote: > >> This appears to be the only place s_directWriteFactory is used. It >> > appears that > >> with this code change this branch will never be taken, so >> > s_directWriteFactory > >> and related should be removed. >> > > s_useDirectWrite is set via setUseDirectWrite, which is still called > from > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > and > > https://code.google.com/p/chromium/codesearch#chromium/src/content/ppapi_plug... > . > setDirectWriteFactory can still be called from > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fon... > , > which is called if ShouldUseDirectWriteFontProxyFieldTrial returns > false. For now, it is still needed, but I will make a note to remove it > when deleting the old chrome font cache codepath. > > https://codereview.chromium.org/1591883002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <SkFontHost.h> shouldn't we include this by its full path now? third_party/skia/include/... https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:10: #include <SkFontMgr.h> On 2016/01/21 22:25:09, bungeman2 wrote: > Not sure what the current policy is on forward declarations, but I think this > can just be > > class SkFontMgr; Yes, please. Blink is already too slow to compile!
Patchset #9 (id:160001) has been deleted
https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:56: #if OS(WIN) On 2016/01/21 22:25:09, bungeman2 wrote: > This doesn't seem to be needed anymore. Done. https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <SkFontHost.h> On 2016/01/21 22:45:27, tfarina wrote: > shouldn't we include this by its full path now? third_party/skia/include/... Done. https://codereview.chromium.org/1591883002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:10: #include <SkFontMgr.h> On 2016/01/21 22:25:09, bungeman2 wrote: > Not sure what the current policy is on forward declarations, but I think this > can just be > > class SkFontMgr; Done.
One nit, will look more later. https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <third_party/skia/include/core/SkFontHost.h> Any particular reason for this to be in pointy brackets instead of in double quotes? It just looks a little strange here, and is different from the other file from which it is included. I suppose it was that way before, but I'd change them.
kulshin@chromium.org changed reviewers: + scottmg@chromium.org
+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/pub... File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <third_party/skia/include/core/SkFontHost.h> On 2016/01/27 22:18:29, bungeman1 wrote: > Any particular reason for this to be in pointy brackets instead of in double > quotes? It just looks a little strange here, and is different from the other > file from which it is included. I suppose it was that way before, but I'd change > them. It was like that when I found it? I'll change it.
Those two lgtm. fyi, an involved files just moved because browser shouldn't be using blink https://chromium.googlesource.com/chromium/src/+/f94a6cd48ef99bb585a872480104....
What's the reason for this change? What bug does it fix? It would be nice to have better descriptions of why you're adding public APIs.
Description was changed from ========== Add plumbing in blink to allow overriding the skia font manager. Note that this change depends on skia change https://codereview.chromium.org/1607083003/ which must land first. BUG= ========== to ========== 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= ==========
On 2016/01/30 10:52:46, esprehn wrote: > What's the reason for this change? What bug does it fix? It would be nice to > have better descriptions of why you're adding public APIs. The main goal is to remove the Chromium code that patches the vtable of the DirectWrite factory. I updated the review description accordingly.
https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_f... 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 third_party/skia .
https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/1591883002/diff/260001/content/child/dwrite_f... 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 two (SkFontMgr.h and SkTypeface_win.h) are in third_party/skia . So they are. I was wondering why there are two skia include paths... Oddly, it builds locally for me, trying to figure out what's going on...
ptal. The skia portion of this change has landed, so if there's no more comments this change can land as well.
lgtm 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:104: m_fontManager = adoptRef(SkFontMgr_New_DirectWrite()); I'm assuming this is only true then not running in the sandbox.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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:104: m_fontManager = adoptRef(SkFontMgr_New_DirectWrite()); On 2016/02/04 21:40:32, bungeman2 wrote: > I'm assuming this is only true then not running in the sandbox. Yes, I believe this case only happens during tests when not using the sandbox. This codepath would not work if run during the sandbox (we would be unable to get any fonts).
The CQ bit was checked by kulshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1591883002/#ps300001 (title: "Add back the option to use blink with DirectWrite without specifying a font manager, which was appa…")
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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kulshin@chromium.org changed reviewers: + pfeldman@chromium.org
+pfeldman, could I get an owners signoff for this change?
platform lgtm
The CQ bit was checked by kulshin@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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= ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/0e752e79260c775500c78cc351887fc19410bd8b Cr-Commit-Position: refs/heads/master@{#374482}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:300001) has been created in https://codereview.chromium.org/1682813004/ by iclelland@chromium.org. The reason for reverting is: This patch is breaking GPU builds -- See https://build.chromium.org/p/chromium.gpu/builders/GPU%20Win%20Builder/builds... specifically. We're seeing the error: e:\b\build\slave\gpu_win_builder\build\src\third_party\webkit\source\platform\fonts\win\fontcacheskiawin.cpp(101) : error C3861: 'adopted': identifier not found .
Message was sent while issue was closed.
On 2016/02/09 22:10:37, iclelland wrote: > A revert of this CL (patchset #15 id:300001) has been created in > https://codereview.chromium.org/1682813004/ by mailto:iclelland@chromium.org. > > The reason for reverting is: This patch is breaking GPU builds -- > > See > https://build.chromium.org/p/chromium.gpu/builders/GPU%20Win%20Builder/builds... > specifically. > > We're seeing the error: > e:\b\build\slave\gpu_win_builder\build\src\third_party\webkit\source\platform\fonts\win\fontcacheskiawin.cpp(101) > : error C3861: 'adopted': identifier not found > . It looks like it's failing in release builds (Win x64 - https://build.chromium.org/p/chromium.win/waterfall?builder=Win%20x64%20Builder fails as well)
Message was sent while issue was closed.
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); The adoption checks are only done in debug, so 'adopted' only exists in debug.
Message was sent while issue was closed.
https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... 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/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:155: s_fontManager->ref(); shouldn't be necessary, as the incoming PassRefPtr already owns a ref https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.h (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.h:109: static void setFontManager(const RefPtr<SkFontMgr>&); should take PassRefPtr<SkFontMgr> https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/win/WebFontRendering.cpp (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/win/WebFontRendering.cpp:20: adopted(fontMgr); Suggest a comment indicating why this is called (since it's unusual). (e.g. "original ref was already adopted by Chromium") https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/win/WebFontRendering.cpp:21: FontCache::setFontManager(RefPtr<SkFontMgr>(fontMgr)); should simply be "FontCache::setFontManager(fontMgr);": PassRefPtr(T*) takes a reference and can be called implicitly https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/win/WebFontRendering.h (right): https://codereview.chromium.org/1591883002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/win/WebFontRendering.h:9: #include <third_party/skia/include/core/SkFontHost.h> this include shouldn't be needed, given the fwd decl 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 at 22:25:17, bungeman2 wrote: > The adoption checks are only done in debug, so 'adopted' only exists in debug. Well, it does exist here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... as long as PassRefPtr.h is included, and this is called as WTF::adopted (I don't think ADL will work here?), this should work?
Message was sent while issue was closed.
> https://codeeview.chromium.org/1591883002/diff/300001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp:101: adopted(s_fontManager); > On 2016/02/09 at 22:25:17, bungeman2 wrote: > > The adoption checks are only done in debug, so 'adopted' only exists in debug. > > Well, it does exist here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > as long as PassRefPtr.h is included, and this is called as WTF::adopted (I don't think ADL will work here?), this should work? Urgh, disregard every comment I just made except this one: Rietveld decided to publish a bunch of old drafts.
Message was sent while issue was closed.
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'.
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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?
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. |
