Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1607083003:
Allow custom font collection for DirectWrite font manager. (Closed)
|
Created:
4 years, 11 months ago by Ilya Kulshin Modified:
4 years, 10 months ago CC:
reviews_skia.org Base URL:
https://chromium.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd option to specify a font collection when creating a
DirectWrite font manager.
The corresponding Chromium change can be found at
https://codereview.chromium.org/1591883002/ .
TBR=reed
This is a trivial and long planned addition to the API.
Committed: https://skia.googlesource.com/skia/+/69d160326659dcf64e1584018b713631ddbc061c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix refcount bug #
Total comments: 6
Patch Set 3 : More refactoring to remove an AddRef call #
Messages
Total messages: 35 (17 generated)
Description was changed from ========== Add option to specify a font collection when creating the skia font manager. BUG=skia: ========== to ========== Add option to specify a font collection when creating the skia font manager. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add option to specify a font collection when creating the skia font manager. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify a font collection when creating the skia font manager. This will be used by Chromium, so that we don't have to patch the DirectWrite factory vtable. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/. ptal. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
kulshin@chromium.org changed reviewers: + bsalomon@google.com, reed@google.com
Description was changed from ========== Add option to specify a font collection when creating the skia font manager. This will be used by Chromium, so that we don't have to patch the DirectWrite factory vtable. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/. ptal. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify a font collection when creating the skia font manager. This will be used by Chromium, so that we don't have to patch the DirectWrite factory vtable. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
ptal.
reed@google.com changed reviewers: + bungeman@google.com - bsalomon@google.com
A great deal of DirectWrite, including parts of the factory, font collections, and font fallback do not work properly inside the renderer sandbox. And they are not designed to. They are designed to run under Windows "Low Integrity Level", which is much more permissive. Attempting to use DiectWrite inside the renderer sandbox is the current source of many bugs, issues, and missing features. Far from pushing the font collection into blink, it needs to be pushed out of the renderer process altogether. If you're just looking to simplify the interface between Chromium and Blink, it would be better to just be passing the whole SkFontMgr across, as opposed to passing across little pieces. As the CLs stand, it's difficult for me to determine what the motivation for this is.
Short of abandoning DirectWrite altogether (possible, but that is a large project with its own risks), it's not really possible to abandon the use of the font collection. At least by explicitly telling Skia which collection to use we can eliminate the code that mucks with the factory. Point taken about the Blink<=>Chromium interface. I'll see what I can do about that, but those changes would not remove the need for being able to specify the collection override for Skia. On 2016/01/19 21:43:42, bungeman1 wrote: > A great deal of DirectWrite, including parts of the factory, font collections, > and font fallback do not work properly inside the renderer sandbox. And they are > not designed to. They are designed to run under Windows "Low Integrity Level", > which is much more permissive. Attempting to use DiectWrite inside the renderer > sandbox is the current source of many bugs, issues, and missing features. Far > from pushing the font collection into blink, it needs to be pushed out of the > renderer process altogether. > > If you're just looking to simplify the interface between Chromium and Blink, it > would be better to just be passing the whole SkFontMgr across, as opposed to > passing across little pieces. As the CLs stand, it's difficult for me to > determine what the motivation for this is.
Ping...
Ping...
reed@google.com changed reviewers: + djsollen@google.com
https://codereview.chromium.org/1607083003/diff/1/src/ports/SkFontMgr_win_dw.cpp File src/ports/SkFontMgr_win_dw.cpp (right): https://codereview.chromium.org/1607083003/diff/1/src/ports/SkFontMgr_win_dw.... src/ports/SkFontMgr_win_dw.cpp:1086: } I would write this whole block as SkTScopedComPtr<IDWriteFontCollection> fontCollection(collection); if (!fontCollection.get()) { HRNM(factory->GetSystemFontCollection(&fontCollection, FALSE), "Could not get system font collection."); } which avoids a branch and a long line with an unclear swap.
https://codereview.chromium.org/1607083003/diff/1/src/ports/SkFontMgr_win_dw.cpp File src/ports/SkFontMgr_win_dw.cpp (right): https://codereview.chromium.org/1607083003/diff/1/src/ports/SkFontMgr_win_dw.... src/ports/SkFontMgr_win_dw.cpp:1086: } On 2016/01/29 20:12:29, bungeman1 wrote: > I would write this whole block as > > SkTScopedComPtr<IDWriteFontCollection> fontCollection(collection); > if (!fontCollection.get()) { > HRNM(factory->GetSystemFontCollection(&fontCollection, FALSE), > "Could not get system font collection."); > } > > which avoids a branch and a long line with an unclear swap. Done. While looking at it I also noticed that SkTScopedComPtr unexpectedly does not AddRef, so I added an AddRef too. Aside: does anyone know why SkTScopedComPtr does not AddRef when constructed from a raw pointer? That is rather counterintuitive behavior.
https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... File src/ports/SkFontMgr_win_dw.cpp (right): https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... src/ports/SkFontMgr_win_dw.cpp:1081: // non-null, so it is safe to use even if |collection| might be null. I don't think any of this commentary is really required here. The 'Safe' in SkSafeRefComPtr already indicates that it checks for nullptr. SkTScopedComPtr doesn't AddRef because the intent is to bind a reference to the stack frame. Also, calling AddRef in the constructor would be strange for a COM smart pointer, as they are normally initialized this way be a function which returns a COM pointer which must already be ref'ed. Note that this is COM where reference counts logically start at 1 (because the stack owns that reference) and not 0 (as Chrome likes to do). As a more practical example, if SkTScopedComPtr did AddRef in the constructor, then everywhere it is used like 'SkTScopedComPtr<X> x(new X)' would need to immediately call Unref on the raw object, which is just ugly. The real confusion here is that we do not logically need to add a reference to 'collection', we could just pass it through to the constructor below (which will retain a reference for itself it it needs to). It's just the case where we create our own IDWriteFontCollection where it becomes necessary for a reference to be automatically disposed of. In other words, perhaps it is clearer to write this as: IDWriteFontCollection* fontCollection = collection; SkTScopedComPtr<IDWriteFontCollection> systemFontCollection; if (nullptr == fontCollection.get()) { HRNM(factory->GetSystemFontCollection(&systemFontCollection, FALSE), "Could not get system font collection."); fontCollection = systemFontCollection.get(); } and then drop the '.get()' from 'fontCollection.get()' when used below. This avoids the issue with needing to AddRef anything, as well as avoids unwanted reference count churn. https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... src/ports/SkFontMgr_win_dw.cpp:1084: HRNM(factory->GetSystemFontCollection(&fontCollection, FALSE), Skia uses four space indents. https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... src/ports/SkFontMgr_win_dw.cpp:1085: "Could not get system font collection."); The start of this line should be indented to go right under 'factory' in the line above.
https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... File src/ports/SkFontMgr_win_dw.cpp (right): https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... src/ports/SkFontMgr_win_dw.cpp:1081: // non-null, so it is safe to use even if |collection| might be null. On 2016/01/29 22:57:24, bungeman1 wrote: > I don't think any of this commentary is really required here. The 'Safe' in > SkSafeRefComPtr already indicates that it checks for nullptr. SkTScopedComPtr > doesn't AddRef because the intent is to bind a reference to the stack frame. > Also, calling AddRef in the constructor would be strange for a COM smart > pointer, as they are normally initialized this way be a function which returns a > COM pointer which must already be ref'ed. Note that this is COM where reference > counts logically start at 1 (because the stack owns that reference) and not 0 > (as Chrome likes to do). > > As a more practical example, if SkTScopedComPtr did AddRef in the constructor, > then everywhere it is used like 'SkTScopedComPtr<X> x(new X)' would need to > immediately call Unref on the raw object, which is just ugly. > > The real confusion here is that we do not logically need to add a reference to > 'collection', we could just pass it through to the constructor below (which will > retain a reference for itself it it needs to). It's just the case where we > create our own IDWriteFontCollection where it becomes necessary for a reference > to be automatically disposed of. In other words, perhaps it is clearer to write > this as: > > > IDWriteFontCollection* fontCollection = collection; > SkTScopedComPtr<IDWriteFontCollection> systemFontCollection; > if (nullptr == fontCollection.get()) { > HRNM(factory->GetSystemFontCollection(&systemFontCollection, FALSE), > "Could not get system font collection."); > fontCollection = systemFontCollection.get(); > } > > and then drop the '.get()' from 'fontCollection.get()' when used below. This > avoids the issue with needing to AddRef anything, as well as avoids unwanted > reference count churn. Switched code to avoid the extra addref. https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... src/ports/SkFontMgr_win_dw.cpp:1084: HRNM(factory->GetSystemFontCollection(&fontCollection, FALSE), On 2016/01/29 22:57:24, bungeman1 wrote: > Skia uses four space indents. Done. https://codereview.chromium.org/1607083003/diff/20001/src/ports/SkFontMgr_win... src/ports/SkFontMgr_win_dw.cpp:1085: "Could not get system font collection."); On 2016/01/29 22:57:24, bungeman1 wrote: > The start of this line should be indented to go right under 'factory' in the > line above. Done.
Description was changed from ========== Add option to specify a font collection when creating the skia font manager. This will be used by Chromium, so that we don't have to patch the DirectWrite factory vtable. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify a font collection when creating the skia font manager. This will be used by Chromium, so that we don't have to patch the DirectWrite factory vtable. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/. ==========
lgtm
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607083003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607083003/40001
Description was changed from ========== Add option to specify a font collection when creating the skia font manager. This will be used by Chromium, so that we don't have to patch the DirectWrite factory vtable. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/. ========== to ========== Add option to specify a font collection when creating a DirectWrite font manager. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/ . ==========
The CQ bit was unchecked by bungeman@google.com
The CQ bit was checked by kulshin@chromium.org
The CQ bit was unchecked by kulshin@chromium.org
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607083003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607083003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Description was changed from ========== Add option to specify a font collection when creating a DirectWrite font manager. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/ . ========== to ========== Add option to specify a font collection when creating a DirectWrite font manager. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/ . TBR=reed This is a trivial and long planned addition to the API. ==========
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607083003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607083003/40001
Message was sent while issue was closed.
Description was changed from ========== Add option to specify a font collection when creating a DirectWrite font manager. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/ . TBR=reed This is a trivial and long planned addition to the API. ========== to ========== Add option to specify a font collection when creating a DirectWrite font manager. The corresponding Chromium change can be found at https://codereview.chromium.org/1591883002/ . TBR=reed This is a trivial and long planned addition to the API. Committed: https://skia.googlesource.com/skia/+/69d160326659dcf64e1584018b713631ddbc061c ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/69d160326659dcf64e1584018b713631ddbc061c |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
