Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1673373003:
Add support for caching font files in the Android SkFontMgr. (Closed)
DescriptionAdd support for caching font files in the Android SkFontMgr.
SkFontMgr_Android lazily access font files from disk, which is not
possible when it is used in the renderer sandbox on Linux. Add a flag to
SkFontMgr_Android_CustomFonts for caching readonly FILE streams when
creating the font mgr.
Since the font mgr is created before the sandbox is initialized, it can
access these files on initialization, and use these cached streams safely
for its lifetime.
Committed: https://skia.googlesource.com/skia/+/ebc465b8f271f362015fdf352b8355989e59a3f3
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed comments. #
Total comments: 15
Patch Set 3 : Addressed comments. #Patch Set 4 : Add enclosing brackets for if statement. #
Total comments: 2
Patch Set 5 : Remove unnecessary fIsolated var. #
Total comments: 3
Patch Set 6 : Addressed comments. #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== fontMgr: Add support for caching font files in the Android SkFontMgr. The android SkFontMgr lazily access font files from disk, which is not possible when it is used in the renderer sandbox on linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sanbox is initialized, it can access these files on initilization, and use these cached streams safely till its lifetime. BUG=skia: ========== to ========== fontMgr: Add support for caching font files in the Android SkFontMgr. The android SkFontMgr lazily access font files from disk, which is not possible when it is used in the renderer sandbox on linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sanbox is initialized, it can access these files on initilization, and use these cached streams safely till its lifetime. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== fontMgr: Add support for caching font files in the Android SkFontMgr. The android SkFontMgr lazily access font files from disk, which is not possible when it is used in the renderer sandbox on linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sanbox is initialized, it can access these files on initilization, and use these cached streams safely till its lifetime. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== fontMgr: Add support for caching font files in the Android SkFontMgr. The android SkFontMgr lazily access font files from disk, which is not possible when it is used in the renderer sandbox on linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sanbox is initialized, it can access these files on initilization, and use these cached streams safely till its lifetime. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
khushalsagar@chromium.org changed reviewers: + bungeman@chromium.org
khushalsagar@chromium.org changed reviewers: + bungeman@google.com - bungeman@chromium.org
https://codereview.chromium.org/1673373003/diff/1/include/ports/SkFontMgr_and... File include/ports/SkFontMgr_android.h (right): https://codereview.chromium.org/1673373003/diff/1/include/ports/SkFontMgr_and... include/ports/SkFontMgr_android.h:46: bool fCacheFontFiles; I'm not sure I like the naming of this. Perhaps something more like 'fHermetic' or something, with the comment stating that with this flag set to true, after the font manager is created it will not attempt to do IO on anything other than resources/fds it has already acquired. As things stand, the name seems to leak too much of the implementation as opposed to the intent. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:72: fFile = sk_fopen(fPathName.c_str(), kRead_SkFILE_Flag); This should just be handled in the init list with a fairly simple ternary. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:82: SkStreamAsset* onOpenStream() const { The 'on' part of the name indicates that this is an implementation for a superclass' wrapper to call, which this isn't this should be marked as private and named something like 'createStream'. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:106: const bool fCacheFontFiles; This seems redundant with fFile being nullptr or not-nullptr. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:111: FILE* fFile; This should be SkAutoCallVProc<FILE, sk_fclose> so it automatic and no need for explicit destructor. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:152: explicit SkFontStyleSet_Android(const FontFamily& family, const Scanner& scanner, const bool cacheFontFiles) { Skia has a 100 character limit (there are a few instances of long line below as well). Normally Skia doesn't normally declare a primitive parameter const. I don't mind, since it states that the value isn't intended to change, but it looks odd against the other Skia code. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:333: : fCacheFontFiles(custom ? custom->fCacheFontFiles : false) { The '{' should go on the next line, four spaces in. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:548: // We only return the default family when the font manager is caching Why this change? Does blink on Linux do bad things when getting a nullptr back? I'd rather not have a change in behavior here.
Done. Thanks Ben. Could you take another look? https://codereview.chromium.org/1673373003/diff/1/include/ports/SkFontMgr_and... File include/ports/SkFontMgr_android.h (right): https://codereview.chromium.org/1673373003/diff/1/include/ports/SkFontMgr_and... include/ports/SkFontMgr_android.h:46: bool fCacheFontFiles; On 2016/02/08 22:11:56, bungeman1 wrote: > I'm not sure I like the naming of this. Perhaps something more like 'fHermetic' > or something, with the comment stating that with this flag set to true, after > the font manager is created it will not attempt to do IO on anything other than > resources/fds it has already acquired. As things stand, the name seems to leak > too much of the implementation as opposed to the intent. Done. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:72: fFile = sk_fopen(fPathName.c_str(), kRead_SkFILE_Flag); On 2016/02/08 22:11:56, bungeman1 wrote: > This should just be handled in the init list with a fairly simple ternary. I wanted to add an assert here. It might as well fail on initialization if its not able to read the font file. Does this look cleaner? https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:82: SkStreamAsset* onOpenStream() const { On 2016/02/08 22:11:56, bungeman1 wrote: > The 'on' part of the name indicates that this is an implementation for a > superclass' wrapper to call, which this isn't this should be marked as private > and named something like 'createStream'. Done. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:106: const bool fCacheFontFiles; On 2016/02/08 22:11:56, bungeman1 wrote: > This seems redundant with fFile being nullptr or not-nullptr. Removed. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:111: FILE* fFile; On 2016/02/08 22:11:56, bungeman1 wrote: > This should be SkAutoCallVProc<FILE, sk_fclose> so it automatic and no need for > explicit destructor. Done. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:152: explicit SkFontStyleSet_Android(const FontFamily& family, const Scanner& scanner, const bool cacheFontFiles) { On 2016/02/08 22:11:56, bungeman1 wrote: > Skia has a 100 character limit (there are a few instances of long line below as > well). Done. > > Normally Skia doesn't normally declare a primitive parameter const. I don't > mind, since it states that the value isn't intended to change, but it looks odd > against the other Skia code. Yeah. Just wanted to make it explicit in the code that this shouldn't change since there is some plumbing till it gets where it needs to go. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:333: : fCacheFontFiles(custom ? custom->fCacheFontFiles : false) { On 2016/02/08 22:11:56, bungeman1 wrote: > The '{' should go on the next line, four spaces in. Done. https://codereview.chromium.org/1673373003/diff/1/src/ports/SkFontMgr_android... src/ports/SkFontMgr_android.cpp:548: // We only return the default family when the font manager is caching On 2016/02/08 22:11:56, bungeman1 wrote: > Why this change? Does blink on Linux do bad things when getting a nullptr back? > I'd rather not have a change in behavior here. It just crashes. Because like the comment above says, on other platforms there is no recovery mechanism. I tried looking at where this call is coming from, and it seems like it comes internally from SkTypeFace public API calls, and I wasn't sure if I should be changing the calls in blink to have an understanding that those calls on SkTypeface can now return a nullptr.
friendly ping. :)
khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org
https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:70: , fFile( Skia allows up to 100 character lines, can probably pull up the next line. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:74: if(cacheFontFiles) space between 'if' and '('. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:75: SkASSERT(fFile); four space indents, here and below https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:107: SkAutoTCallVProc<FILE, sk_fclose> closeOnDestroy; This extra field isn't needed, SkAUtoTCallVProc is actually just a wrapper for unique_ptr with a deleter with calls the given proc. In other words, these two lines can just be: SkAutoTCallVProc<FILE, sk_fclose> fFile; https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:250: pathName, cacheFontFiles, ttcIndex, axisValues.get(), axisDefinitions.count(), style, This line is a bit long (more than 100 characters) https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:549: if (typeface || !fIsolated) I'm going to have to take quite a look into this. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:583: SkFontStyleSet_Android* newSet = new SkFontStyleSet_Android(family, fScanner, fIsolated); this line ran a bit long too.
https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:70: , fFile( On 2016/02/11 20:06:09, bungeman1 wrote: > Skia allows up to 100 character lines, can probably pull up the next line. Done. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:74: if(cacheFontFiles) On 2016/02/11 20:06:09, bungeman1 wrote: > space between 'if' and '('. Done. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:75: SkASSERT(fFile); On 2016/02/11 20:06:09, bungeman1 wrote: > four space indents, here and below Done. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:107: SkAutoTCallVProc<FILE, sk_fclose> closeOnDestroy; On 2016/02/11 20:06:09, bungeman1 wrote: > This extra field isn't needed, SkAUtoTCallVProc is actually just a wrapper for > unique_ptr with a deleter with calls the given proc. In other words, these two > lines can just be: > > SkAutoTCallVProc<FILE, sk_fclose> fFile; Oh. Done. Thanks! https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:250: pathName, cacheFontFiles, ttcIndex, axisValues.get(), axisDefinitions.count(), style, On 2016/02/11 20:06:09, bungeman1 wrote: > This line is a bit long (more than 100 characters) Done. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:549: if (typeface || !fIsolated) On 2016/02/11 20:06:09, bungeman1 wrote: > I'm going to have to take quite a look into this. I tried reproducing this to trace back the calls. I think I might have made an error with how I was testing this change with the corresponding change in blink. I'm not seeing it with the final patch, https://codereview.chromium.org/1685053002/. Sorry about the confusion. Removed it. https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:583: SkFontStyleSet_Android* newSet = new SkFontStyleSet_Android(family, fScanner, fIsolated); On 2016/02/11 20:06:09, bungeman1 wrote: > this line ran a bit long too. Done.
The CQ bit was checked by khushalsagar@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/1673373003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673373003/40001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-02-12 10:55 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by khushalsagar@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/1673373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673373003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
One last thing... https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/20001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:549: if (typeface || !fIsolated) On 2016/02/12 04:54:07, Khushal wrote: > On 2016/02/11 20:06:09, bungeman1 wrote: > > I'm going to have to take quite a look into this. > > I tried reproducing this to trace back the calls. I think I might have made an > error with how I was testing this change with the corresponding change in blink. > I'm not seeing it with the final patch, > https://codereview.chromium.org/1685053002/. > Sorry about the confusion. Removed it. That is awesome, I was just building Chromium to take a look at this. Saves me a day of investigating. https://codereview.chromium.org/1673373003/diff/60001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/60001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:559: const bool fIsolated; Interestingly, we no longer need to store this value. buildNameToFamilyMap is only called from the constructor, so the signature can be changed to void buildNameToFamilyMap(SkTDArray<FontFamily*> families, bool isolated) and the one call to buildNameToFamilyMap (in the constructor) changed to just pass 'custom->fIsolated' for this new parameter. (I wish I could edit this CL directly here, this would be really easy to do inline.)
Thanks Ben. Done. https://codereview.chromium.org/1673373003/diff/60001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/60001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:559: const bool fIsolated; On 2016/02/12 15:13:47, bungeman1 wrote: > Interestingly, we no longer need to store this value. buildNameToFamilyMap is > only called from the constructor, so the signature can be changed to > > void buildNameToFamilyMap(SkTDArray<FontFamily*> families, bool isolated) > > and the one call to buildNameToFamilyMap (in the constructor) changed to just > pass 'custom->fIsolated' for this new parameter. (I wish I could edit this CL > directly here, this would be really easy to do inline.) Yup, I missed it in when I was removing the variable. Done.
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/1673373003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673373003/80001
bungeman@google.com changed reviewers: + reed@google.com
The implementation part of this lgtm with one nit. This does touch a public API, so want to have reed take a quick look. https://codereview.chromium.org/1673373003/diff/80001/include/ports/SkFontMgr... File include/ports/SkFontMgr_android.h (right): https://codereview.chromium.org/1673373003/diff/80001/include/ports/SkFontMgr... include/ports/SkFontMgr_android.h:46: bool fIsolated; Question for reed: should this be an enum? https://codereview.chromium.org/1673373003/diff/80001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/80001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:326: { nit: this '{' should go on the previous line (no need to have this line changing in this CL after all).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1673373003/diff/80001/src/ports/SkFontMgr_and... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/1673373003/diff/80001/src/ports/SkFontMgr_and... src/ports/SkFontMgr_android.cpp:326: { On 2016/02/12 18:59:35, bungeman1 wrote: > nit: this '{' should go on the previous line (no need to have this line changing > in this CL after all). Done.
lgtm
The CQ bit was checked by bungeman@google.com to run a CQ dry run
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/1673373003/#ps100001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673373003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673373003/100001
Description was changed from ========== fontMgr: Add support for caching font files in the Android SkFontMgr. The android SkFontMgr lazily access font files from disk, which is not possible when it is used in the renderer sandbox on linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sanbox is initialized, it can access these files on initilization, and use these cached streams safely till its lifetime. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add support for caching font files in the Android SkFontMgr. SkFontMgr_Android lazily access font files from disk, which is not possible when it is used in the renderer sandbox on Linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sandbox is initialized, it can access these files on initialization, and use these cached streams safely for its lifetime. ==========
Message was sent while issue was closed.
Description was changed from ========== Add support for caching font files in the Android SkFontMgr. SkFontMgr_Android lazily access font files from disk, which is not possible when it is used in the renderer sandbox on Linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sandbox is initialized, it can access these files on initialization, and use these cached streams safely for its lifetime. ========== to ========== Add support for caching font files in the Android SkFontMgr. SkFontMgr_Android lazily access font files from disk, which is not possible when it is used in the renderer sandbox on Linux. Add a flag to SkFontMgr_Android_CustomFonts for caching readonly FILE streams when creating the font mgr. Since the font mgr is created before the sandbox is initialized, it can access these files on initialization, and use these cached streams safely for its lifetime. Committed: https://skia.googlesource.com/skia/+/ebc465b8f271f362015fdf352b8355989e59a3f3 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/ebc465b8f271f362015fdf352b8355989e59a3f3 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
