Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084533004:
SkTextBlob: Begin implementing Extended TextBlob API (Closed)
|
Created:
4 years, 6 months ago by hal.canary Modified:
4 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkTextBlob: Begin implementing Extended TextBlob API
BUG=skia:5434
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084533004
Committed: https://skia.googlesource.com/skia/+/4f0a23a8d54f5eb0fdacfff7c109b9045b548978
Patch Set 1 : 2016-06-20 (Monday) 15:33:30 EDT #
Total comments: 19
Patch Set 2 : 2016-06-22 (Wednesday) 13:59:00 EDT #Patch Set 3 : 2016-06-24 (Friday) 17:22:15 EDT #
Total comments: 38
Patch Set 4 : comments from fmalita #
Total comments: 1
Patch Set 5 : 2016-06-28 (Tuesday) 12:06:30 EDT #Patch Set 6 : rebase: 2016-08-11 (Thursday) 09:24:38 EDT #Patch Set 7 : 2016-08-15 (Monday) 12:23:59 EDT #Patch Set 8 : rebase #
Messages
Total messages: 57 (28 generated)
Description was changed from ========== SkTextBlob: Begin implementing Extended TextBlob API ========== to ========== SkTextBlob: Begin implementing Extended TextBlob API GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084533004 ==========
The CQ bit was checked by halcanary@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/2084533004/1
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...)
Description was changed from ========== SkTextBlob: Begin implementing Extended TextBlob API GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084533004 ========== to ========== SkTextBlob: Begin implementing Extended TextBlob API BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084533004 ==========
halcanary@google.com changed reviewers: + cabanier@adobe.com, fmalita@chromium.org, reed@google.com, tomhudson@google.com
PTAL.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2084533004/40001
Does this change the RAM size of a textblob that doesn't contain any unichars? https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob.h File include/core/SkTextBlob.h (right): https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:203: int count, int testSize, SkPoint offset, const SkRect* bounds); size_t? testSize -> textSize? is this a byte count?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/20 19:56:25, reed1 wrote: > Does this change the RAM size of a textblob that doesn't contain any unichars? No, it does not. Florin asked that I ensure that. Since the RunRecord is padded out to pointer alignment, I had room for a bool.
https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob.h File include/core/SkTextBlob.h (right): https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:109: * If textByteCount is 0, utf8text and clusters will be NUL. It's unclear where textByteCount comes from. Maybe better to say "if there is no original unicode mapping" https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:113: * clusters (if not NULL) will point to an array of size count. Could be more descriptive. https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:144: int textByteCount, Why not pass the UTF-8 buffer here?
unless we're in a rush, I'd prefer to not finalize this until next week when Florin is back working.
On 2016/06/21 01:41:36, reed1 wrote: > unless we're in a rush, I'd prefer to not finalize this until next week when > Florin is back working. Right. I'm working on the second part of this project: doing something with this new information in SkPDF. That will take another week anyway.
https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob.h File include/core/SkTextBlob.h (right): https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:92: class SK_API SkTextBlobBuilder { Tangent not for this CL: do we really have to have a helper class that does direct allocation via public methods in the public include/ directories? https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:109: * If textByteCount is 0, utf8text and clusters will be NUL. nit: NULL? https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:132: * sequence of glyphs. If 0, text will be unkown. nit: unknown? https://codereview.chromium.org/2084533004/diff/40001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/2084533004/diff/40001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:103: // | SkTextBlob | RunRecord | Glyphs[] | Pos[] | RunRecord | Glyphs[] | Pos[] | ... This needs to be updated? Or have an alternate line added? https://codereview.chromium.org/2084533004/diff/40001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:153: return *(uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]); Is it worth having an inlined private function for &this->posBuffer(([ScalarsPerGlyph(fPositioning) * fCount]? https://codereview.chromium.org/2084533004/diff/40001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:202: nit: extra blank line? https://codereview.chromium.org/2084533004/diff/40001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:363: if (!reader.readByteArray(buf->clusters, glyphCount * sizeof(uint32_t)) || If textSize is 0, shouldn't this also be a 0-length read? https://codereview.chromium.org/2084533004/diff/40001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:612: return false; Tangent not for this CL: Because it's not worth the code complexity?
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob.h File include/core/SkTextBlob.h (right): https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:92: class SK_API SkTextBlobBuilder { On 2016/06/21 14:51:28, tomhudson wrote: > Tangent not for this CL: do we really have to have a helper class that does > direct allocation via public methods in the public include/ directories? How else will a client construct aSkTextBlob? https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:109: * If textByteCount is 0, utf8text and clusters will be NUL. On 2016/06/21 14:51:28, tomhudson wrote: > nit: NULL? Done. https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:109: * If textByteCount is 0, utf8text and clusters will be NUL. On 2016/06/20 23:05:16, Rik wrote: > It's unclear where textByteCount comes from. > Maybe better to say "if there is no original unicode mapping" Acknowledged. https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:113: * clusters (if not NULL) will point to an array of size count. On 2016/06/20 23:05:16, Rik wrote: > Could be more descriptive. Done. https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:132: * sequence of glyphs. If 0, text will be unkown. On 2016/06/21 14:51:28, tomhudson wrote: > nit: unknown? Done. https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:144: int textByteCount, On 2016/06/20 23:05:16, Rik wrote: > Why not pass the UTF-8 buffer here? Let's pass the text and glyphs the same way. https://codereview.chromium.org/2084533004/diff/40001/include/core/SkTextBlob... include/core/SkTextBlob.h:203: int count, int testSize, SkPoint offset, const SkRect* bounds); On 2016/06/20 19:56:24, reed1 wrote: > size_t? I don't know. int, unsigned, or size_t would all be fine to me. count is an int, so I kept it uniform. > testSize -> textSize? > is this a byte count? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2084533004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) 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-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@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/2084533004/60001
The CQ bit was unchecked by halcanary@google.com
lgtm
Looks good, just some nits/suggestions. https://codereview.chromium.org/2084533004/diff/80001/include/core/SkTextBlob.h File include/core/SkTextBlob.h (right): https://codereview.chromium.org/2084533004/diff/80001/include/core/SkTextBlob... include/core/SkTextBlob.h:142: * @param lang Language code, currently unimplemented. If not implemented, is it worth adding the param at this point? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:103: // | SkTextBlob | RunRecord | Glyphs[] | Pos[] | RunRecord | Glyphs[] | Pos[] | ... We should also update this layout diagram w/ text info. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:140: // Glyph are stored immediately following the record. nit: "Glyphs", while we're here https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:150: uint32_t textSize() const { maybe uint32_t* textSizePtr() for reuse? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:153: return *(uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]); nit: reinterpret_cast instead of old-style cast? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:156: void setTextSize(uint32_t size) { I think we can get rid of the setter, see suggestion below. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:158: *(uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]) = size; nit: reinterpret_cast https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:164: return (uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]) + 1; return textSizePtr() + 1 ? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:169: return (char*)((uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]) + 1 + fCount); return reinterpret_cast<char*>(clusterBuffer() + fCount) ? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:185: } nit: maybe somewhat DRY-er size_t size = ...; if (textSize) { size += ...; } return SkAlignPtr(size); https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:204: SkASSERT(posBuffer() + fCount * ScalarsPerGlyph(fPositioning) <= (SkScalar*)Next(this)); We should also add asserts for the new buffers. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:230: }; Similar to RunFont above: namespace { struct RunRecordStorageEquivalent { RunFont fFont; SkPoint fOffset; uint32_t fCount, fFlags; }; static_assert(sizeof(RunRecord) == sizeof(RunRecordStorageEquivalent), "runrecord_should_stay_packed"); } ? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:249: #endif Not needed with RunRecordStorageEquivalent above. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:672: run->setTextSize(textSize); Why not pass textSize to the RunRecord ctor? Then I think we don't need the setter anymore. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:687: fCurrentRunBuffer.clusters = nullptr; I don't think we need to clear these: textSize == 0 AND mergeRun() succeeded (which implies textSize == 0 for prev run also). Maybe SkASSERTs instead? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... File src/core/SkTextBlobRunIterator.h (right): https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... src/core/SkTextBlobRunIterator.h:31: bool isExtended() const; This is just an alias for textSize() > 0, right? Clients can always check themselves, I don't think we need this helper. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... src/core/SkTextBlobRunIterator.h:32: uint32_t* clusterBuffer() const; nit: clusters() for consistency? https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... src/core/SkTextBlobRunIterator.h:34: char* textBuffer() const; nit: text() for consistency? https://codereview.chromium.org/2084533004/diff/80001/tests/ExtendedTextBlob.cpp File tests/ExtendedTextBlob.cpp (right): https://codereview.chromium.org/2084533004/diff/80001/tests/ExtendedTextBlob.... tests/ExtendedTextBlob.cpp:13: DEF_TEST(ExtendedTextBlob, reporter) { This could be added to TextBlobTest.cpp instead of new file.
https://codereview.chromium.org/2084533004/diff/80001/include/core/SkTextBlob.h File include/core/SkTextBlob.h (right): https://codereview.chromium.org/2084533004/diff/80001/include/core/SkTextBlob... include/core/SkTextBlob.h:142: * @param lang Language code, currently unimplemented. On 2016/06/27 20:21:55, f(malita) wrote: > If not implemented, is it worth adding the param at this point? Good question. It's a pain to change the API, and this information is usually considered an important part of the source text (necessary to determine which glyphs to use). The PDF standard allows the specification of natural language, and encourages that as part of accessibility. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:103: // | SkTextBlob | RunRecord | Glyphs[] | Pos[] | RunRecord | Glyphs[] | Pos[] | ... On 2016/06/27 20:21:55, f(malita) wrote: > We should also update this layout diagram w/ text info. Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:140: // Glyph are stored immediately following the record. On 2016/06/27 20:21:55, f(malita) wrote: > nit: "Glyphs", while we're here Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:150: uint32_t textSize() const { On 2016/06/27 20:21:55, f(malita) wrote: > maybe uint32_t* textSizePtr() for reuse? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:153: return *(uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]); On 2016/06/27 20:21:55, f(malita) wrote: > nit: reinterpret_cast instead of old-style cast? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:156: void setTextSize(uint32_t size) { On 2016/06/27 20:21:55, f(malita) wrote: > I think we can get rid of the setter, see suggestion below. Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:158: *(uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]) = size; On 2016/06/27 20:21:55, f(malita) wrote: > nit: reinterpret_cast Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:164: return (uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]) + 1; On 2016/06/27 20:21:55, f(malita) wrote: > return textSizePtr() + 1 ? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:169: return (char*)((uint32_t*)(&this->posBuffer()[ScalarsPerGlyph(fPositioning) * fCount]) + 1 + fCount); On 2016/06/27 20:21:55, f(malita) wrote: > return reinterpret_cast<char*>(clusterBuffer() + fCount) ? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:185: } On 2016/06/27 20:21:55, f(malita) wrote: > nit: maybe somewhat DRY-er > > size_t size = ...; > if (textSize) { > size += ...; > } > return SkAlignPtr(size); Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:204: SkASSERT(posBuffer() + fCount * ScalarsPerGlyph(fPositioning) <= (SkScalar*)Next(this)); On 2016/06/27 20:21:55, f(malita) wrote: > We should also add asserts for the new buffers. Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:230: }; On 2016/06/27 20:21:55, f(malita) wrote: > Similar to RunFont above: > > namespace { > > struct RunRecordStorageEquivalent { > RunFont fFont; > SkPoint fOffset; > uint32_t fCount, fFlags; > }; > > static_assert(sizeof(RunRecord) == sizeof(RunRecordStorageEquivalent), > "runrecord_should_stay_packed"); > } > > ? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:249: #endif On 2016/06/27 20:21:55, f(malita) wrote: > Not needed with RunRecordStorageEquivalent above. Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:672: run->setTextSize(textSize); On 2016/06/27 20:21:55, f(malita) wrote: > Why not pass textSize to the RunRecord ctor? Then I think we don't need the > setter anymore. Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:687: fCurrentRunBuffer.clusters = nullptr; On 2016/06/27 20:21:55, f(malita) wrote: > I don't think we need to clear these: textSize == 0 AND mergeRun() succeeded > (which implies textSize == 0 for prev run also). > > Maybe SkASSERTs instead? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... File src/core/SkTextBlobRunIterator.h (right): https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... src/core/SkTextBlobRunIterator.h:31: bool isExtended() const; On 2016/06/27 20:21:56, f(malita) wrote: > This is just an alias for textSize() > 0, right? > > Clients can always check themselves, I don't think we need this helper. Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... src/core/SkTextBlobRunIterator.h:32: uint32_t* clusterBuffer() const; On 2016/06/27 20:21:56, f(malita) wrote: > nit: clusters() for consistency? Done. https://codereview.chromium.org/2084533004/diff/80001/src/core/SkTextBlobRunI... src/core/SkTextBlobRunIterator.h:34: char* textBuffer() const; On 2016/06/27 20:21:56, f(malita) wrote: > nit: text() for consistency? Done. https://codereview.chromium.org/2084533004/diff/80001/tests/ExtendedTextBlob.cpp File tests/ExtendedTextBlob.cpp (right): https://codereview.chromium.org/2084533004/diff/80001/tests/ExtendedTextBlob.... tests/ExtendedTextBlob.cpp:13: DEF_TEST(ExtendedTextBlob, reporter) { On 2016/06/27 20:21:56, f(malita) wrote: > This could be added to TextBlobTest.cpp instead of new file. Done.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
Thanks, LGTM. Do we have test coverage for the new serialization code? If not, maybe you can tweak one of the textblob GMs to use the extended format (follow-up is fine). https://codereview.chromium.org/2084533004/diff/100001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/2084533004/diff/100001/src/core/SkTextBlob.cp... src/core/SkTextBlob.cpp:175: void setTextSize(uint32_t size) { We can delete the setter now.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/28 15:57:44, f(malita) wrote: > Thanks, LGTM. > > Do we have test coverage for the new serialization code? If not, maybe you can > tweak one of the textblob GMs to use the extended format (follow-up is fine). I'm working on it. I'd like to generate a .skp from my Harfbuzz test harness. I'll have it in place when I land the SkPDF CL that uses this new data. > We can delete the setter now. done
The CQ bit was unchecked by halcanary@google.com
ping reed@ for API
On 2016/06/28 20:23:07, Hal Canary wrote: > ping reed@ for API
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/26 19:01:11, Hal Canary wrote: > On 2016/06/28 20:23:07, Hal Canary wrote: > > ping reed@ for API
lgtm
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cabanier@adobe.com, fmalita@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/2084533004/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkTextBlob: Begin implementing Extended TextBlob API BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084533004 ========== to ========== SkTextBlob: Begin implementing Extended TextBlob API BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084533004 Committed: https://skia.googlesource.com/skia/+/4f0a23a8d54f5eb0fdacfff7c109b9045b548978 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://skia.googlesource.com/skia/+/4f0a23a8d54f5eb0fdacfff7c109b9045b548978 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
