Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2037133002:
SkFontConfigInterface createTypeface to makeTypeface. (Closed)
|
Created:
4 years, 6 months ago by bungeman-chromium Modified:
4 years, 6 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jam, jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkFontConfigInterface createTypeface to makeTypeface.
The createTypeface method (which returned a bare pointer to a reference
counted object) is being replaced with makeTypeface (which returns a
smart pointer to a reference counted object). This removes some manual
reference counting in Chromium and allows for removing the deprecated
createTypeface method.
Committed: https://crrev.com/9d8ada9acb3bb8287191e193a2c1c9f85b2d124a
Cr-Commit-Position: refs/heads/master@{#397794}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address comments. #Patch Set 3 : move comes from utility. #
Total comments: 1
Messages
Total messages: 22 (9 generated)
The CQ bit was checked by bungeman@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/2037133002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2037133002/1
bungeman@chromium.org changed reviewers: + dcheng@chromium.org, fmalita@chromium.org
There is a general effort in Skia for factory methods to return smart pointers to reference counted objects instead of bare pointers. This CL is part of the effort to update our callers so the old methods which returned bare pointers can be removed. On the Chromium side this slightly simplifies the calling code as the reference count is now automatically incremented when a copy of the smart pointer needs to be made. On the blink side the change is minimal as the reference is transferred to WTF::RefPtr. fmalita, let me know if it makes sense to do this differently. I'd rather not change the entire Blink side from WTF::RefPtr<SkTypeface> to sk_sp<SkTypeface> in this CL, though perhaps that something which should be considered in the future.
LGTM w/ some nits. https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... content/common/font_config_ipc_linux.cc:172: SkTypeface::CreateFromStream(typeface_stream, identity.fTTCIndex)); SkTypeface::MakeFromStream while here? https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... content/common/font_config_ipc_linux.cc:175: return mapped_typefaces_insert_it->second; Not new to this CL, but I wonder if it'd help to return typeface_from_stream instead - would RVO avoid a ref churn cycle? https://codereview.chromium.org/2037133002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2037133002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:57: return adoptRef(fci->makeTypeface(fontIdentity).release()); Nit: return fromSkSp(fci->makeTypeface(fontIdentity)); (SkiaUtils.h)
Smart pointers, yay. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@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/2037133002/40001
https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... content/common/font_config_ipc_linux.cc:172: SkTypeface::CreateFromStream(typeface_stream, identity.fTTCIndex)); On 2016/06/03 17:11:59, f(malita) wrote: > SkTypeface::MakeFromStream while here? Done. https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... content/common/font_config_ipc_linux.cc:175: return mapped_typefaces_insert_it->second; On 2016/06/03 17:11:59, f(malita) wrote: > Not new to this CL, but I wonder if it'd help to return typeface_from_stream > instead - would RVO avoid a ref churn cycle? Ah, I see, the stack has a ref and then the map gets a ref; so you're suggesting this should return the ref from the stack instead of copying the ref from the map. Good idea. However, I think we want it to be very clear that we're returning whatever is in the map. Fortunately, 'Put' takes the payload by universal reference so we can just construct the sk_sp and move directly into the map, then return a copy of what ended up in the map. Done. https://codereview.chromium.org/2037133002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2037133002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:57: return adoptRef(fci->makeTypeface(fontIdentity).release()); On 2016/06/03 17:11:59, f(malita) wrote: > Nit: return fromSkSp(fci->makeTypeface(fontIdentity)); > > (SkiaUtils.h) Thanks, I thought something like this existed, but I didn't know what it was called. Done.
https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... File content/common/font_config_ipc_linux.cc (right): https://codereview.chromium.org/2037133002/diff/1/content/common/font_config_... content/common/font_config_ipc_linux.cc:175: return mapped_typefaces_insert_it->second; On 2016/06/03 18:28:18, bungeman-chromium wrote: > On 2016/06/03 17:11:59, f(malita) wrote: > > Not new to this CL, but I wonder if it'd help to return typeface_from_stream > > instead - would RVO avoid a ref churn cycle? > > Ah, I see, the stack has a ref and then the map gets a ref; so you're suggesting > this should return the ref from the stack instead of copying the ref from the > map. Good idea. > > However, I think we want it to be very clear that we're returning whatever is in > the map. Fortunately, 'Put' takes the payload by universal reference so we can > just construct the sk_sp and move directly into the map, then return a copy of > what ended up in the map. > > Done. Awesome, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2037133002/#ps40001 (title: "move comes from utility.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037133002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== SkFontConfigInterface createTypeface to makeTypeface. The createTypeface method (which returned a bare pointer to a reference counted object) is being replaced with makeTypeface (which returns a smart pointer to a reference counted object). This removes some manual reference counting in Chromium and allows for removing the deprecated createTypeface method. ========== to ========== SkFontConfigInterface createTypeface to makeTypeface. The createTypeface method (which returned a bare pointer to a reference counted object) is being replaced with makeTypeface (which returns a smart pointer to a reference counted object). This removes some manual reference counting in Chromium and allows for removing the deprecated createTypeface method. Committed: https://crrev.com/9d8ada9acb3bb8287191e193a2c1c9f85b2d124a Cr-Commit-Position: refs/heads/master@{#397794} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9d8ada9acb3bb8287191e193a2c1c9f85b2d124a Cr-Commit-Position: refs/heads/master@{#397794}
Message was sent while issue was closed.
drott@chromium.org changed reviewers: + drott@chromium.org
Message was sent while issue was closed.
Nice change, thanks! https://codereview.chromium.org/2037133002/diff/40001/content/common/font_con... File content/common/font_config_ipc_linux.h (right): https://codereview.chromium.org/2037133002/diff/40001/content/common/font_con... content/common/font_config_ipc_linux.h:42: // Returns a new SkTypeface instance or a ref'ed one from the cache. The I guess this comment can be removed, or at least updated for the sk_sp return type? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
