Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
396143004:
Add a working SkFontMgr_fontconfig. (Closed)
|
Created:
6 years, 5 months ago by bungeman-skia Modified:
6 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionAdd a working SkFontMgr_fontconfig.
Committed: https://skia.googlesource.com/skia/+/a6785ccb540b1b752ab536cdf579a698eadbf7d2
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fix FC_POSTSCRIPT_NAME and address comments. #Patch Set 3 : Fix double free. #Patch Set 4 : Fix ttc index and descriptor. #
Total comments: 60
Patch Set 5 : Address comments. #
Total comments: 6
Patch Set 6 : Address comments. #Patch Set 7 : Remove T& operator* from SkAutoTCallVProc. #
Total comments: 1
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/396143004/diff/1/tests/FontHostTest.cpp File tests/FontHostTest.cpp (left): https://codereview.chromium.org/396143004/diff/1/tests/FontHostTest.cpp#oldco... tests/FontHostTest.cpp:30: { kFontTableTag_maxp, 32 }, The 'maxp' table comes in two sizes, 32 (for TT) and 6 (for CFF).
api lgtm w/ nit https://codereview.chromium.org/396143004/diff/1/include/core/SkTemplates.h File include/core/SkTemplates.h (right): https://codereview.chromium.org/396143004/diff/1/include/core/SkTemplates.h#n... include/core/SkTemplates.h:90: fObj = obj; nitty nit: lets not keep the "if (fObj) { ... }" on one line. The getters above seem readable, but not the "if".
More later... https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:29: * FcConfig is a library state. There exists a default global state which is affected by Nit: Not sure what you mean by a "library state". Stateful library? https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:37: * However, there is actually no real enforcement. Nit: The previous sentence is just a bit awkward, but this one is unclear: does this mean the library doesn't check whether the list is mis-typed or inconsistently-typed, or the library doesn't error in those cases? https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:46: // See skia:1497 for background. Nit: please spell out the link to http://skbug.com/1497 here. https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:280: static SkFontStyle skfontstyle_from_fcpattern(FcPattern* pattern) { Nit: line lengths? https://codereview.chromium.org/396143004/diff/1/tests/FontHostTest.cpp File tests/FontHostTest.cpp (left): https://codereview.chromium.org/396143004/diff/1/tests/FontHostTest.cpp#oldco... tests/FontHostTest.cpp:30: { kFontTableTag_maxp, 32 }, On 2014/07/17 20:38:30, bungeman1 wrote: > The 'maxp' table comes in two sizes, 32 (for TT) and 6 (for CFF). So why are we removing it?
https://codereview.chromium.org/396143004/diff/1/include/core/SkTemplates.h File include/core/SkTemplates.h (right): https://codereview.chromium.org/396143004/diff/1/include/core/SkTemplates.h#n... include/core/SkTemplates.h:90: fObj = obj; On 2014/07/17 20:41:42, reed1 wrote: > nitty nit: lets not keep the "if (fObj) { ... }" on one line. The getters above > seem readable, but not the "if". Done. https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:29: * FcConfig is a library state. There exists a default global state which is affected by On 2014/07/17 21:14:46, tomhudson wrote: > Nit: Not sure what you mean by a "library state". Stateful library? Yeah, I wan't entirely sure what to call it. FreeType calls the same sort of thing a FT_Library, DirectWrite calls a similar thing an IDWriteFactory. It's basically the global state of the library packaged up into one nice package so that there can be multiple users of the library in one process and they don't step on each other. If you have a suggestion for wording, I'd consider it. I'll think about wording this differently. In fact, I'll try to use wording similar to how FreeType explains it. https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:37: * However, there is actually no real enforcement. On 2014/07/17 21:14:46, tomhudson wrote: > Nit: The previous sentence is just a bit awkward, but this one is unclear: does > this mean the library doesn't check whether the list is mis-typed or > inconsistently-typed, or the library doesn't error in those cases? Hmmm... yes. The list isn't typed at all, except by convention. I'll try to make that more clear. Any collection of FcValues must be assumed to be heterogeneous by the code, but the code need not do anything particularly interesting if the values go against convention. https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:46: // See skia:1497 for background. On 2014/07/17 21:14:46, tomhudson wrote: > Nit: please spell out the link to http://skbug.com/1497 here. Done. https://codereview.chromium.org/396143004/diff/1/src/ports/SkFontMgr_fontconf... src/ports/SkFontMgr_fontconfig.cpp:280: static SkFontStyle skfontstyle_from_fcpattern(FcPattern* pattern) { On 2014/07/17 21:14:46, tomhudson wrote: > Nit: line lengths? Done. https://codereview.chromium.org/396143004/diff/1/tests/FontHostTest.cpp File tests/FontHostTest.cpp (left): https://codereview.chromium.org/396143004/diff/1/tests/FontHostTest.cpp#oldco... tests/FontHostTest.cpp:30: { kFontTableTag_maxp, 32 }, On 2014/07/17 21:14:46, tomhudson wrote: > On 2014/07/17 20:38:30, bungeman1 wrote: > > The 'maxp' table comes in two sizes, 32 (for TT) and 6 (for CFF). > > So why are we removing it? This was more of a note to Mike, but the one place where this is used is just checking that the GetTableTags and GetTableSize work and return sane values for system font streams. This CL (at least on my local machine) allows some CFF style fonts show up, which invalidates the premise that the 'maxp' table is of a single given size. In other words, this was already an incorrect assertion, it just happens that we don't have many CFF fonts installed on our test machines, so we haven't seen the a failure of this assumption in the past. In theory we could add a version number to go with a size but that's just a complication which doesn't add much to this test.
What's our testing story here? https://codereview.chromium.org/396143004/diff/60001/include/core/SkTemplates.h File include/core/SkTemplates.h (right): https://codereview.chromium.org/396143004/diff/60001/include/core/SkTemplates... include/core/SkTemplates.h:81: operator T*() const { return fObj; } Is there any chance, given that we get to start from scratch with this, that we can get away with returning const pointers from const methods and non-const pointers from non-const methods? It can be very confusing (and thread-hostile) to get a non-const pointer out of a smart pointer member from within a const method. https://codereview.chromium.org/396143004/diff/60001/include/core/SkTemplates... include/core/SkTemplates.h:84: T** operator&() { return &fObj; } Can we avoid this one? Seems really easy to write code that'd swap around pointers without ever calling P. operator& is pretty bonkers. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:59: #ifdef SK_DEBUG What's the idea here? So that we can test the locking correctness even on platforms that are thread safe? Why not just use the mutex when SK_DEBUG or FcGetVersion() is old enough? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:60: void *CreateThreadFcLocked() { return SkNEW(bool); } * with void? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:61: void DeleteThreadFcLocked(void* v) { SkDELETE(reinterpret_cast<bool*>(v)); } I think you can get away with a simple static_cast for these guys? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:75: SkASSERT(0 == *threadLocked); 0 -> false? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:82: if (fUnlock) { Do FcGetVersion() < 21091 and fUnlock ever evaluate to different values? Can't we drop fUnlock, or only call FcGetVersion once? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:87: SkASSERT(1 == *threadLocked); 1 -> true? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:110: typedef SkAutoTCallVProc<FcConfig, FcTDestroy<FcConfig, FcConfigDestroy> > SkAutoFcConfig; OCD these guys into columns? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:118: if (FcPatternGetInteger(pattern, object, 0, &value) != FcResultMatch) { It makes me so sad these don't take a const FcPattern*... https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:132: enum is_weak_return { Funky type name and constant values for Skia? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:263: if (val < ranges[0].old_val) {} around all these ifs? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:267: for (int i = 0; i < rangesCount - 1; ++i) { Is it ever worth doing some o(N) search here? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:294: { SkTFixed<FC_WEIGHT_THIN>::value, SkTFixed<SkFS::kThin_Weight>::value }, Line all these guys up too? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:375: /** @param stream does not ownership of the reference, does take ownership of the stream. */ Awkward? Taking ownership of the stream sounds like "we will SkDELETE the stream". How about /** @params stream: will take a ref */ https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:405: mutable SkAutoFcPattern fPattern; public, mutable, and sometimes we need to hold a global lock to touch this. I don't know... can you make this a little more terrifying? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:442: mutable SkAutoFcConfig fFC; Ditto? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:509: static bool find_name(const SkTDArray<const char*>& list, const char* str) { FindName, etc? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:519: SkDataTable* get_family_names() { getFamilyNames? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:544: const char* familyName = reinterpret_cast<const char*>(fcFamilyName); also probably can static_cast? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:553: return SkDataTable::NewCopyArrays((void *const *const)names.begin(), woah. Is that, a const pointer to a const pointer to non-const thing? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:558: SkTypeface_fontconfig* cshFace = reinterpret_cast<SkTypeface_fontconfig*>(cached); Also should both safely static_cast, no? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:563: mutable SkMutex fTFCacheMutex; Can you break some of the bodies of these methods out? I got a little lost and forgot I was in a class until I got here. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:599: virtual int onCountFamilies() const SK_OVERRIDE { Does anything else derive from this? If not, might as well promote the onFoo() to private? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:612: * as a string object value in the pattern. <3 alignment <3 https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:618: // Set an arbitrary limit on the number of pattern object values to consider. Why? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:749: reinterpret_cast<const SkTypeface_fontconfig*>(typeface); static_cast? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:756: if (!(0 < length && length < (1u << 30))) { Might invert this? if (length <= 0 || length >= (1u << 30)) ? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:767: reinterpret_cast<SkStreamAsset*>(stream))); static_cast? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:799: return SkNEW_ARGS(SkFontMgr_fontconfig, ()); Also known as SkNEW(SkFontMgr_fontconfig) :P
The story on testing is that this becomes the new font handler for Skia and gets tested. I've also actually run Chromium with this (without the sandbox) and things work (and better). We wouldn't be testing the old path, but Chromium would be. Eventually I want to get Chromium onto a RemotableFontMgr that looks like this. I think I've addressed or ignored most of the comments so far. Show up locally and we can fight about what I didn't change today. https://codereview.chromium.org/396143004/diff/60001/include/core/SkTemplates.h File include/core/SkTemplates.h (right): https://codereview.chromium.org/396143004/diff/60001/include/core/SkTemplates... include/core/SkTemplates.h:81: operator T*() const { return fObj; } On 2014/07/22 15:48:42, mtklein wrote: > Is there any chance, given that we get to start from scratch with this, that we > can get away with returning const pointers from const methods and non-const > pointers from non-const methods? It can be very confusing (and thread-hostile) > to get a non-const pointer out of a smart pointer member from within a const > method. Not sure what you mean here. If you create an SkAutoTCallVProc<const SkType, ...> then the pointer you get back here will be const. This const here states that returning the pointer here does not mutate the SkAutoTCallProc. A const SkAutoTCallProc just means you can't detach or reset it. None of the std xxx_ptr types have transitive constness, and the idea seems confusing (c++ normally separates the constness of the pointer from the constness of the thing). https://codereview.chromium.org/396143004/diff/60001/include/core/SkTemplates... include/core/SkTemplates.h:84: T** operator&() { return &fObj; } On 2014/07/22 15:48:42, mtklein wrote: > Can we avoid this one? Seems really easy to write code that'd swap around > pointers without ever calling P. operator& is pretty bonkers. It is, but it makes certain things easier when dealing with the type signatures in font config. Will work around and remove, since it is a bit iffy. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:59: #ifdef SK_DEBUG On 2014/07/22 15:48:44, mtklein wrote: > What's the idea here? So that we can test the locking correctness even on > platforms that are thread safe? Yes, I can't test the old stuff on my new machine, and soon I don't think we'll have any bot coverage for old enough versions. > > Why not just use the mutex when SK_DEBUG or FcGetVersion() is old enough? Because I also want to test this code running the multi-threaded way. Let the threads run free!!! https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:60: void *CreateThreadFcLocked() { return SkNEW(bool); } On 2014/07/22 15:48:44, mtklein wrote: > * with void? I tried that, but you aren't allowed to muck with the returned pointer. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:61: void DeleteThreadFcLocked(void* v) { SkDELETE(reinterpret_cast<bool*>(v)); } On 2014/07/22 15:48:43, mtklein wrote: > I think you can get away with a simple static_cast for these guys? A prvalue of type pointer to void (possibly cv-qualified) can be converted to pointer to any type. So I guess so. This was all copy-paste from elsewhere, so there are other places which could use the same change. Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:75: SkASSERT(0 == *threadLocked); On 2014/07/22 15:48:43, mtklein wrote: > 0 -> false? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:82: if (fUnlock) { On 2014/07/22 15:48:44, mtklein wrote: > Do FcGetVersion() < 21091 and fUnlock ever evaluate to different values? Can't > we drop fUnlock, or only call FcGetVersion once? Hmmm... yes, that does fall out of changing everything. This was mostly here just because it used to be. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:87: SkASSERT(1 == *threadLocked); On 2014/07/22 15:48:44, mtklein wrote: > 1 -> true? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:110: typedef SkAutoTCallVProc<FcConfig, FcTDestroy<FcConfig, FcConfigDestroy> > SkAutoFcConfig; On 2014/07/22 15:48:44, mtklein wrote: > OCD these guys into columns? Your OCD is making fun of my OCD. When this sort of thing is 'columnized' it drives me crazy because I feel it emphasizes the wrong thing. I don't care that they're doing approximately the same thing, I just care that each line is correct individually. (Same thing for lining up names in declarations, I feel like its a holdover from poorly formatted assembly, and nobody wrote assembly that way after the 90's anyway.) On the other hand, these lines are really long and repeat bits. So I've fixed that. See if the new format reduces your twitch to acceptable levels. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:118: if (FcPatternGetInteger(pattern, object, 0, &value) != FcResultMatch) { On 2014/07/22 15:48:44, mtklein wrote: > It makes me so sad these don't take a const FcPattern*... Yeah, COM does the same thing, where 'const' is never used. The argument goes that it doesn't mean anything because when you call into an external library there is nothing enforcing that const on the other end. On the other hand, it would be nice for them to document that sort of thing anyway in their public headers. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:132: enum is_weak_return { On 2014/07/22 15:48:43, mtklein wrote: > Funky type name and constant values for Skia? This is a cpp file!!! Eh, why not. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:263: if (val < ranges[0].old_val) On 2014/07/22 15:48:43, mtklein wrote: > {} around all these ifs? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:267: for (int i = 0; i < rangesCount - 1; ++i) { On 2014/07/22 15:48:43, mtklein wrote: > Is it ever worth doing some o(N) search here? Probably not. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:294: { SkTFixed<FC_WEIGHT_THIN>::value, SkTFixed<SkFS::kThin_Weight>::value }, On 2014/07/22 15:48:43, mtklein wrote: > Line all these guys up too? Generally, I'm against monospace ascii art, but since you asked nicely... https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:375: /** @param stream does not ownership of the reference, does take ownership of the stream. */ On 2014/07/22 15:48:43, mtklein wrote: > Awkward? Taking ownership of the stream sounds like "we will SkDELETE the > stream". How about /** @params stream: will take a ref */ Haha, 'will take a ref' sounds like it takes ownership of the reference. And we will, in fact, in most cases be the one to call SkDELETE on the stream indirectly by releasing the reference we add. And we totally take ownership of the stream in general. The caller of this can really only validly call 'unref' on the stream pointer they have. In the future when we get rid of this silly ref counting on stream, this will just be 'takes ownership of the stream'. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:405: mutable SkAutoFcPattern fPattern; On 2014/07/22 15:48:44, mtklein wrote: > public, mutable, and sometimes we need to hold a global lock to touch this. I > don't know... can you make this a little more terrifying? Yes, I could put it in a global linked list. I'd make it 'const', but I'd have to cast away the constness every time it goes to FontConfig. Once it's created, it isn't modified. Actually, we probably only need the lock in the destructor and not in the other methods anyway (we only really need the lock anytime something is created or destroyed). https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:442: mutable SkAutoFcConfig fFC; On 2014/07/22 15:48:43, mtklein wrote: > Ditto? What? This one's already private, what do you have to fear? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:509: static bool find_name(const SkTDArray<const char*>& list, const char* str) { On 2014/07/22 15:48:43, mtklein wrote: > FindName, etc? Why do we name private static methods of classes (especially in cpp files) different from regular static functions? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:519: SkDataTable* get_family_names() { On 2014/07/22 15:48:43, mtklein wrote: > getFamilyNames? Acknowledged. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:544: const char* familyName = reinterpret_cast<const char*>(fcFamilyName); On 2014/07/22 15:48:44, mtklein wrote: > also probably can static_cast? Nope. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:553: return SkDataTable::NewCopyArrays((void *const *const)names.begin(), On 2014/07/22 15:48:43, mtklein wrote: > woah. Is that, a const pointer to a const pointer to non-const thing? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:558: SkTypeface_fontconfig* cshFace = reinterpret_cast<SkTypeface_fontconfig*>(cached); On 2014/07/22 15:48:44, mtklein wrote: > Also should both safely static_cast, no? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:563: mutable SkMutex fTFCacheMutex; On 2014/07/22 15:48:43, mtklein wrote: > Can you break some of the bodies of these methods out? I got a little lost and > forgot I was in a class until I got here. It did end up with ~350 lines in this block. Maybe I like it like this because I spent too much time having my brains melted by Java (and my IDE doesn't really let me get lost)? If you ask again, I'll see what it looks like broken up. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:599: virtual int onCountFamilies() const SK_OVERRIDE { On 2014/07/22 15:48:44, mtklein wrote: > Does anything else derive from this? If not, might as well promote the onFoo() > to private? We could, I suppose. Seems odd since others don't do it, and also oddly restrictive. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:612: * as a string object value in the pattern. On 2014/07/22 15:48:44, mtklein wrote: > <3 alignment <3 Heh, you're all over vertical alignment above, but not here? https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:618: // Set an arbitrary limit on the number of pattern object values to consider. On 2014/07/22 15:48:44, mtklein wrote: > Why? To prevent N*M from getting too big. Eventually this should be done with some faster hash/set mechanism, but for now there's no limit on the number of names in a list, so this is more of a time-out mechanism than anything else. This seems to be sufficient for all current distro setups. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:749: reinterpret_cast<const SkTypeface_fontconfig*>(typeface); On 2014/07/22 15:48:44, mtklein wrote: > static_cast? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:756: if (!(0 < length && length < (1u << 30))) { On 2014/07/22 15:48:45, mtklein wrote: > Might invert this? > > if (length <= 0 || length >= (1u << 30)) ? Changed from !(in_bounds) to out_of_bounds. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:767: reinterpret_cast<SkStreamAsset*>(stream))); On 2014/07/22 15:48:44, mtklein wrote: > static_cast? Done. https://codereview.chromium.org/396143004/diff/60001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:799: return SkNEW_ARGS(SkFontMgr_fontconfig, ()); On 2014/07/22 15:48:44, mtklein wrote: > Also known as SkNEW(SkFontMgr_fontconfig) :P Ah yes, another victim of copy pasta. Done.
lgtm https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:97: bool fUnlock; Delete? https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:255: old_max - old_min); some funky formatting here? https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:384: /** @param stream does not ownership of the reference, does take ownership of the stream. */ does not _take_ ownership?
https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:97: bool fUnlock; On 2014/07/30 19:43:37, mtklein wrote: > Delete? Done. https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:255: old_max - old_min); On 2014/07/30 19:43:37, mtklein wrote: > some funky formatting here? It's... division!!! Maybe I could put a comment here like /* ---------- */. https://codereview.chromium.org/396143004/diff/80001/src/ports/SkFontMgr_font... src/ports/SkFontMgr_fontconfig.cpp:384: /** @param stream does not ownership of the reference, does take ownership of the stream. */ On 2014/07/30 19:43:37, mtklein wrote: > does not _take_ ownership? Done.
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bungeman@google.com/396143004/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bungeman@google.com/396143004/120001
Message was sent while issue was closed.
Committed patchset #7 (120001) as a6785ccb540b1b752ab536cdf579a698eadbf7d2
Message was sent while issue was closed.
djsollen@google.com changed reviewers: + djsollen@google.com
Message was sent while issue was closed.
This is causing the valgrind bot to fail because we are leaking memory. http://108.170.220.120:10115/builders/Test-Ubuntu12-ShuttleA-GTX550Ti-x86_64-... https://codereview.chromium.org/396143004/diff/120001/src/ports/SkFontMgr_fon... File src/ports/SkFontMgr_fontconfig.cpp (right): https://codereview.chromium.org/396143004/diff/120001/src/ports/SkFontMgr_fon... src/ports/SkFontMgr_fontconfig.cpp:703: FcFontRenderPrepare(fFC, pattern, allFonts->fonts[fontIndex])); Valgrind is picking up a leak here! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
