Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1471033002:
Fix Google3 fonts. (Closed)
|
Created:
5 years, 1 month ago by dogben Modified:
5 years ago Reviewers:
bungeman-skia CC:
jcgregorio, mtklein, reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching.
Some future dependents require these changes.
Depends on internal cl/108287941.
Committed: https://skia.googlesource.com/skia/+/2211a7bdd1680003a4decbebef76f153cd0a28fa
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Refactor based on code review comments. #Patch Set 3 : Fix gyp. #Patch Set 4 : Fix gyp; rename files; follow Skia style guide. #Patch Set 5 : Move files back to ports. #
Total comments: 6
Patch Set 6 : Respond to code review comments. #Patch Set 7 : Fix include. #
Total comments: 2
Patch Set 8 : Fix BUILD.public, remove virtual from private method. #
Total comments: 8
Patch Set 9 : Reorder includes. #Patch Set 10 : Rename back to SkFontConfigInterface_direct.h #
Messages
Total messages: 63 (29 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add Google3 font caching. Some future dependents require this functionality. BUG=skia: ========== to ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. BUG=skia: ==========
Description was changed from ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. BUG=skia: ========== to ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on cl/108287941. BUG=skia: ==========
Description was changed from ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on cl/108287941. BUG=skia: ========== to ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on internal cl/108287941. BUG=skia: ==========
benjaminwagner@google.com changed reviewers: + bungeman@google.com
https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... src/ports/SkFontConfigInterface_direct.cpp:26: #if defined(GOOGLE3) I would very much like to minimize this craziness. The issue being that if there's precedent here, it's much more likely that we'll end up with requests for more of it. All of this seems a bit hacky, but isn't GOOGLE_BUFFERED_FONTS what this really cares about?
https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... src/ports/SkFontConfigInterface_direct.cpp:114: class SkFontConfigInterfaceDirect : public SkFontConfigInterface { So here's a plan. Take this definition and put it in a header somewhere. Create a class SkFontConfigInterfaceDirectForGoogle : public SkFontConfigInterfaceDirect { SkStreamAsset* openStream(const FontIdentity&) override { const char* c_filename = identity.fString.c_str(); const char* buffer; int length = GoogleFreeType::GoogleFt2ReadFontFromMemory(c_filename, &buffer); if (length >= 0) return new SkMemoryStream(buffer, length); return SkFontConfigInterfaceDirect::openStream(buffer, length); } } https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... src/ports/SkFontConfigInterface_direct.cpp:136: SkFontConfigInterface* SkFontConfigInterface::GetSingletonDirectInterface(SkBaseMutex* mutex) { Then, since this is a factory, put this in a separate compilation unit in Skia. Skia would normally compile it, but internally there could be another one which creates a SkFontConfigInterfaceDirectForGoogle instead of SkFontConfigInterfaceDirect. https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... src/ports/SkFontConfigInterface_direct.cpp:356: #if defined(GOOGLE3) This block is harder to remove, but since it wasn't here before I'm not sure it's really needed.
The CQ bit was checked by benjaminwagner@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/1471033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by benjaminwagner@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/1471033002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/60001
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 benjaminwagner@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/1471033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by benjaminwagner@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/1471033002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by benjaminwagner@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/1471033002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, I think I've done what you suggested, and hopefully I updated the correct gyp correctly.
Description was changed from ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on internal cl/108287941. BUG=skia: ========== to ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on internal cl/108287941. ==========
I generally like this. https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/1471033002/diff/20001/src/ports/SkFontConfigI... src/ports/SkFontConfigInterface_direct.cpp:356: #if defined(GOOGLE3) On 2015/11/23 21:39:59, bungeman1 wrote: > This block is harder to remove, but since it wasn't here before I'm not sure > it's really needed. This block is needed, or at least the 'access' code below needs to go. Otherwise this method will return false for all fonts. https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:36: virtual bool isValidPattern(FcPattern* pattern); can this just be virtual bool isAccessible(const char* filename); then there's less copy-pasted code and removes the need for GetName to be mentioned here. https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... src/ports/SkFontConfigInterface_direct.cpp:1: /* It would be nice to name this file SkFontConfigInterface_direct_factory.cpp Note that in order to land this splitting out, we're going to need a custom DEPS roll which adds this new file to Chromium's build. https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterface_direct_google3.cpp (right): https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... src/ports/SkFontConfigInterface_direct_google3.cpp:34: SkFontConfigInterface* SkFontConfigInterface::GetSingletonDirectInterface(SkBaseMutex* mutex) { It would be nice if this were in SkFontConfigInterface_direct_google3_factory.cpp for consistency, but I'm not going to require that.
The CQ bit was checked by benjaminwagner@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/1471033002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/160001
The CQ bit was checked by benjaminwagner@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/1471033002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/180001
The CQ bit was unchecked by benjaminwagner@google.com
https://codereview.chromium.org/1471033002/diff/180001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/180001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:39: virtual bool isValidPattern(FcPattern* pattern); can we make isValidPattern non-virtual?
The CQ bit was checked by benjaminwagner@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/1471033002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Sorry for the delay, had other things needing fixing. https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:36: virtual bool isValidPattern(FcPattern* pattern); On 2015/11/24 at 21:56:36, bungeman1 wrote: > can this just be > > virtual bool isAccessible(const char* filename); > > then there's less copy-pasted code and removes the need for GetName to be mentioned here. Done. https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterface_direct.cpp (right): https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... src/ports/SkFontConfigInterface_direct.cpp:1: /* On 2015/11/24 at 21:56:36, bungeman1 wrote: > It would be nice to name this file > > SkFontConfigInterface_direct_factory.cpp > > Note that in order to land this splitting out, we're going to need a custom DEPS roll which adds this new file to Chromium's build. Done. https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterface_direct_google3.cpp (right): https://codereview.chromium.org/1471033002/diff/140001/src/ports/SkFontConfig... src/ports/SkFontConfigInterface_direct_google3.cpp:34: SkFontConfigInterface* SkFontConfigInterface::GetSingletonDirectInterface(SkBaseMutex* mutex) { On 2015/11/24 at 21:56:36, bungeman1 wrote: > It would be nice if this were in > > SkFontConfigInterface_direct_google3_factory.cpp > > for consistency, but I'm not going to require that. Done. Required adding a .h. https://codereview.chromium.org/1471033002/diff/180001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/180001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:39: virtual bool isValidPattern(FcPattern* pattern); On 2015/11/30 at 16:15:20, bungeman1 wrote: > can we make isValidPattern non-virtual? Done.
https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.cpp (right): https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.cpp:10: #include "SkFontConfigInterfaceDirect.h" I would just leave this where it was. The general argument is to put associated headers first so that each header is included first at least once (to ensure it isn't dependent on other headers being included first). However, we use tools/generate_includes_cpp.py for that. https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:13: #include <fontconfig/fontconfig.h> Can this just be forward declarations, like struct FcFontSet; struct FcPattern; since these types are only used privately. https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:34: virtual bool isAccessible(const char* filename); As documentation, isAccessible should be private. We don't ever expect a derived class to need to call it, just define it.
https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:13: #include <fontconfig/fontconfig.h> On 2015/11/30 at 17:54:40, bungeman1 wrote: > Can this just be forward declarations, like > > struct FcFontSet; > struct FcPattern; > > since these types are only used privately. FcPattern is a typedef. Is there a way to forward-declare a typedef in C++? https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:34: virtual bool isAccessible(const char* filename); On 2015/11/30 at 17:54:40, bungeman1 wrote: > As documentation, isAccessible should be private. We don't ever expect a derived class to need to call it, just define it. I called it in src/ports/SkFontConfigInterface_direct_google3.cpp. I think I need to support loading fonts both from the cache and from disk. I can investigate further if that seems like a bad idea.
lgtm https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.h (right): https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:13: #include <fontconfig/fontconfig.h> On 2015/11/30 18:20:21, Ben Wagner wrote: > On 2015/11/30 at 17:54:40, bungeman1 wrote: > > Can this just be forward declarations, like > > > > struct FcFontSet; > > struct FcPattern; > > > > since these types are only used privately. > > FcPattern is a typedef. Is there a way to forward-declare a typedef in C++? Ah, yes, ick. Ok, we can leave this alone then. https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.h:34: virtual bool isAccessible(const char* filename); On 2015/11/30 18:20:21, Ben Wagner wrote: > On 2015/11/30 at 17:54:40, bungeman1 wrote: > > As documentation, isAccessible should be private. We don't ever expect a > derived class to need to call it, just define it. > > I called it in src/ports/SkFontConfigInterface_direct_google3.cpp. > > I think I need to support loading fonts both from the cache and from disk. I can > investigate further if that seems like a bad idea. Ah, I see. The code before just defined out the 'access' call I thought. Though if we want to do both, sgtm.
The CQ bit was checked by benjaminwagner@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/1471033002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... File src/ports/SkFontConfigInterfaceDirect.cpp (right): https://codereview.chromium.org/1471033002/diff/200001/src/ports/SkFontConfig... src/ports/SkFontConfigInterfaceDirect.cpp:10: #include "SkFontConfigInterfaceDirect.h" On 2015/11/30 17:54:40, bungeman1 wrote: > I would just leave this where it was. The general argument is to put associated > headers first so that each header is included first at least once (to ensure it > isn't dependent on other headers being included first). However, we use > tools/generate_includes_cpp.py for that. Done.
Hmmm... in Patch Set 4 SkFontConfigInterface_direct.cpp became SkFontConfigInterfaceDirect.cpp . Is there a specific reason for this? It gets a little out of sync with the rest of the names here, and makes the roll in to Chromium a bit more noisy. I think I would be in favor of SkFontConfigInterface_direct.cpp and SkFontConfigInterface_direct.h so that this mirrors the naming convention used by the other files in ports.
The CQ bit was checked by benjaminwagner@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/1471033002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/11/30 at 20:16:08, bungeman wrote: > Hmmm... in Patch Set 4 SkFontConfigInterface_direct.cpp became SkFontConfigInterfaceDirect.cpp . Is there a specific reason for this? It gets a little out of sync with the rest of the names here, and makes the roll in to Chromium a bit more noisy. I think I would be in favor of SkFontConfigInterface_direct.cpp and SkFontConfigInterface_direct.h so that this mirrors the naming convention used by the other files in ports. Sorry, I didn't really understand the naming convention.
On 2015/11/30 20:53:53, Ben Wagner wrote: > On 2015/11/30 at 20:16:08, bungeman wrote: > > Hmmm... in Patch Set 4 SkFontConfigInterface_direct.cpp became > SkFontConfigInterfaceDirect.cpp . Is there a specific reason for this? It gets a > little out of sync with the rest of the names here, and makes the roll in to > Chromium a bit more noisy. I think I would be in favor of > SkFontConfigInterface_direct.cpp and SkFontConfigInterface_direct.h so that this > mirrors the naming convention used by the other files in ports. > > Sorry, I didn't really understand the naming convention. Yeah, ports kinda does its own thing here, for various reasons. lgtm even more.
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/1471033002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1471033002/240001
Message was sent while issue was closed.
Description was changed from ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on internal cl/108287941. ========== to ========== Fix Google3 fonts. Use fontconfig rather than custom_directory_factory. Add Google3 font caching. Some future dependents require these changes. Depends on internal cl/108287941. Committed: https://skia.googlesource.com/skia/+/2211a7bdd1680003a4decbebef76f153cd0a28fa ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://skia.googlesource.com/skia/+/2211a7bdd1680003a4decbebef76f153cd0a28fa |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
