Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1420973005:
Refactor glyph selection and positioning (Closed)
DescriptionExtract the glyph picking and placing code.
There is a common piece of code which finds and positions glyphs and is used in four places. Some places copied the code, some places added callbacks. Here is a list of code:
SkDraw::drawPosText
GrAtlasTextContext::internalDrawBMPPosText
GrAtlasTextContext::internalDrawDFPosText
SkXPSDevice::drawPosText
This only extracts the code from SkDraw::drawPosText. I would like to use it in the other three places. I think this code is performance neutral.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/f553e4e09cd2baf15fc041daab8a08bd46e352f0
Patch Set 1 #Patch Set 2 : working passing DrawText gm #Patch Set 3 : All comparisons matching #Patch Set 4 : Better consts and start experiment #Patch Set 5 : working about at parity #Patch Set 6 : Firstpass done. #Patch Set 7 : fix pure virtual problem, and the union problem #Patch Set 8 : noncopyable #Patch Set 9 : try more modern noncopyable #Patch Set 10 : experiment with delete copy ctor #Patch Set 11 : use sk storage #Patch Set 12 : fix ifdef #Patch Set 13 : exclude alignof for windows #Patch Set 14 : make windows happy #Patch Set 15 : fix comment #
Total comments: 16
Patch Set 16 : remove delete experiment #Patch Set 17 : fix Ben's comment #
Total comments: 63
Patch Set 18 : address mtklein's comments #Patch Set 19 : cleanup unused structs and comments #
Total comments: 18
Patch Set 20 : more mtklein comments #
Total comments: 20
Patch Set 21 : clean up #Messages
Total messages: 38 (13 generated)
The CQ bit was checked by herb@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/1420973005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420973005/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-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by herb@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/1420973005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420973005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) 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 herb@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/1420973005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420973005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
Description was changed from ========== Part way need to make handle XY BUG=skia: ========== to ========== Extract the glyph picking and placing code. There is a common piece of code which finds and positions glyphs and is used in four places. Some places copied the code, some places added callbacks. Here is a list of code: SkDraw::drawPosText GrAtlasTextContext::internalDrawBMPPosText GrAtlasTextContext::internalDrawDFPosText SkXPSDevice::drawPosText This only extracts the code from SkDraw::drawPosText. I would like to use it in the other three places. I think this code is performance neutral. BUG=skia: ==========
herb@google.com changed reviewers: + bungeman@google.com, joshualitt@google.com, jvanverth@google.com
The CQ bit was checked by herb@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/1420973005/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420973005/280001
A couple of off-the-cuff thoughts: - The distance field text renderer uses a slightly modified layout method so I'm not sure you'll be able to drop this in as is (unless you've already taken that into account). The bitmap text renderer should be identical to raster. - You may want to use this in GrStencilAndCoverTextContext as well.
Some first pass things, I'll take a better look at it later. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1758: template <typename... Ts> struct SizeOf; Seems like this should be called something like MaxSizeOf? The name leads me to think that it's adding up the sizes of all of the types (discovering the size for a heterogeneous container as opposed to a union). https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1760: template <typename T> struct SizeOf<T> { static const size_t value = sizeof(T); }; It seems like this would be cleaner if the base case were 0. Then I don't think you need this case, the base can just have value = 0. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1775: // Basically UntaggedVariant is a pile of memory that can hold one of the Ts. It provides a way Remove 'Basically' https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1800: // PolymorphicVariant holds supclasses of Base without slicing. Ts must be supclasses of Base. Is this supposed to be super classes or subclasses? https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1819: class Noncopyable { Is there any reason why SkNoncopyable can't be used (or just changed to this)? Also, above it looks like you want a NoncopyableAndNonmovable? Do we want a name for that? https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1824: }; Everything down to here is rather general and doesn't seem like it would need to be in this file. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2106: break; case kNone_SkAxisAlignment: I think this case needs to go on the next line. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2192: init_subpixel<GlyphProcessor, SkPaint::kLeft_Align>(to_init, All the parameters should just go on the next line to avoid this jumpy indentation.
@Ben: thanks for the quick review. Do you think that ProcessPosText could replace your call to myDraw.drawPosText in SkXPSDevice::drawPosText? https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1758: template <typename... Ts> struct SizeOf; On 2015/11/02 21:09:35, bungeman1 wrote: > Seems like this should be called something like MaxSizeOf? The name leads me to > think that it's adding up the sizes of all of the types (discovering the size > for a heterogeneous container as opposed to a union). Done. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1760: template <typename T> struct SizeOf<T> { static const size_t value = sizeof(T); }; On 2015/11/02 21:09:35, bungeman1 wrote: > It seems like this would be cleaner if the base case were 0. Then I don't think > you need this case, the base can just have value = 0. Done. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1775: // Basically UntaggedVariant is a pile of memory that can hold one of the Ts. It provides a way On 2015/11/02 21:09:34, bungeman1 wrote: > Remove 'Basically' Done. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1800: // PolymorphicVariant holds supclasses of Base without slicing. Ts must be supclasses of Base. On 2015/11/02 21:09:34, bungeman1 wrote: > Is this supposed to be super classes or subclasses? Done. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1819: class Noncopyable { On 2015/11/02 21:09:34, bungeman1 wrote: > Is there any reason why SkNoncopyable can't be used (or just changed to this)? > Also, above it looks like you want a NoncopyableAndNonmovable? Do we want a name > for that? Changed to SkNoncopyable. I was struggling with the windows union implementation. It kept saying there was a move constructor. It was just confused. I ended implementing SameSizeAndAlignmentAs in a different way. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1824: }; On 2015/11/02 21:09:35, bungeman1 wrote: > Everything down to here is rather general and doesn't seem like it would need to > be in this file. I hope to extract the code through ProcessPosText into its own module, and then share it among the different uses. This is just the first step. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2106: break; case kNone_SkAxisAlignment: On 2015/11/02 21:09:34, bungeman1 wrote: > I think this case needs to go on the next line. Done. https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2192: init_subpixel<GlyphProcessor, SkPaint::kLeft_Align>(to_init, On 2015/11/02 21:09:35, bungeman1 wrote: > All the parameters should just go on the next line to avoid this jumpy > indentation. Done.
On 2015/11/02 21:26:50, herb_g wrote: > @Ben: thanks for the quick review. Do you think that ProcessPosText could > replace your call to myDraw.drawPosText in SkXPSDevice::drawPosText? > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp > File src/core/SkDraw.cpp (right): > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:1758: template <typename... Ts> struct SizeOf; > On 2015/11/02 21:09:35, bungeman1 wrote: > > Seems like this should be called something like MaxSizeOf? The name leads me > to > > think that it's adding up the sizes of all of the types (discovering the size > > for a heterogeneous container as opposed to a union). > > Done. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:1760: template <typename T> struct SizeOf<T> { static const > size_t value = sizeof(T); }; > On 2015/11/02 21:09:35, bungeman1 wrote: > > It seems like this would be cleaner if the base case were 0. Then I don't > think > > you need this case, the base can just have value = 0. > > Done. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:1775: // Basically UntaggedVariant is a pile of memory that > can hold one of the Ts. It provides a way > On 2015/11/02 21:09:34, bungeman1 wrote: > > Remove 'Basically' > > Done. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:1800: // PolymorphicVariant holds supclasses of Base without > slicing. Ts must be supclasses of Base. > On 2015/11/02 21:09:34, bungeman1 wrote: > > Is this supposed to be super classes or subclasses? > > Done. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:1819: class Noncopyable { > On 2015/11/02 21:09:34, bungeman1 wrote: > > Is there any reason why SkNoncopyable can't be used (or just changed to this)? > > Also, above it looks like you want a NoncopyableAndNonmovable? Do we want a > name > > for that? > > Changed to SkNoncopyable. I was struggling with the windows union > implementation. It kept saying there was a move constructor. It was just > confused. I ended implementing SameSizeAndAlignmentAs in a different way. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:1824: }; > On 2015/11/02 21:09:35, bungeman1 wrote: > > Everything down to here is rather general and doesn't seem like it would need > to > > be in this file. > > I hope to extract the code through ProcessPosText into its own module, and then > share it among the different uses. This is just the first step. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:2106: break; case kNone_SkAxisAlignment: > On 2015/11/02 21:09:34, bungeman1 wrote: > > I think this case needs to go on the next line. > > Done. > > https://codereview.chromium.org/1420973005/diff/280001/src/core/SkDraw.cpp#ne... > src/core/SkDraw.cpp:2192: init_subpixel<GlyphProcessor, > SkPaint::kLeft_Align>(to_init, > On 2015/11/02 21:09:35, bungeman1 wrote: > > All the parameters should just go on the next line to avoid this jumpy > > indentation. > > Done. looks fine to me, I think we'll have to see how it generalizes by implementing GrAtlasTextContext's use cases.
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1733: // Calculate a type with the same size and alignment as a list of types Ts. You might just drop the AndAlignment on these names. It's aligned enough, but doesn't have the same alignment as Ts. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1745: SkAlignedSStorage<MaxSizeOf<Ts...>::value> unused; Even template <typename... Ts> using SameSizeAndAlignmentAs = SkAlignedSStorage<MaxSizeOf<Ts...>::value>; https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1752: class UntaggedVariant { I'm not sure I see why SameSizeAndAlignmentAs is separate from UntaggedVariant, or UntaggedVariant is separate from PolymorphicVariant. Seems like things would be clearer if we just merged them all together into PolymorphicVariant. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1760: template <typename Variant, typename... Args> It'd be nice to add a newline before template. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1761: void Init(Args&&... args) { -> init https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1763: #ifndef SK_BUILD_FOR_WIN32 Alternatively, I think we could do #if defined(_MSC_VER) && _MSC_VER < 1900 #define alignof __alignof #endif ... use alignof() normally ... Might be nice to back off the #ifndef and #endif one tab stop left? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1774: // PolymorphicVariant holds supclasses of Base without slicing. Ts must be subclasses of Base. sup -> sub https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1781: skstd::forward<Initializer>(initializer)(&fVariants); What's the deal with forward here? We don't seem to be forwarding it anywhere. Can we not just PolymorphicVariant(Initializer&& initializer) { initializer(&fVariants); } ? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1784: Base* get() const { return (Base*) &fVariants; } This seems like a spot where reinterpret_cast<> would make things a bit clearer. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1793: // PositionReaderInterface read a point from the pos vector. reads https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1799: virtual const SkPoint NextPoint() = 0; nextPoint() https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1802: class LinearPositions final : public PositionReaderInterface { Even, HorizontalPositions. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1804: LinearPositions(const SkScalar* positions) It seems surprising we don't pass the shared y here and have NextPoint feed it back. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1819: : fPositions(positions) { } Typo? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1841: virtual SkPoint Map(SkPoint position) const = 0; map https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1843: Let's write down a formula for Map explicitly in terms of point, matrix, and origin? I'm finding myself confused about whether offsetting by origin happens first, or second, or if it matters. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1872: GeneralMapper(const SkMatrix& matrix, const SkPoint origin) // Caller must keep matrix alive. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1888: typedef PolymorphicVariant<MapperInterface, TranslationMapper, XScaleMapper, GeneralMapper> Mapper; So, under this scheme, we're always going to occupy enough space for a matrix, an origin point, and a cached MapXYProc. That's enough to implement all three of these strategies efficiently, simply by switching out different cached MapXYProcs. Seems like this doesn't need to have any inheritance at all. E.g. class Mapper { Mapper(const SkMatrix& m, SkPoint origin) : fMatrix(m), fOrigin(origin), fMapProc(m.getMapXYProc()) {} void map(SkPoint p) const { SkPoint result; fMapProc(fMatrix, p.fX + fOrigin.fX, p.fY + fOrigin.fY, &result); return result; } ... }; This would have a uniform single indirect function call, where the implementations we've got here require 1 or 2. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1892: template <SkPaint::Align textAlignment> Reading this file is a serious cognitive load. Given the complexity of all this code we're adding, let's see if there are places we can simplify. I think this is a good place to start: I don't really think there's any benefit to having this be a template method. It's static and called with constants, so I've got to figure that if we write this as a normal function of (glyph, Align) -> SkPoint, good code will be generated. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1907: // Even though the entire enum is covered above, MVSC doesn't think so. Make it happy. Might be nice to SkASSERT(false); too. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1918: static const SkScalar kSubpixelRounding = SkFixedToScalar(SkGlyph::kSubpixelRound); Doesn't this require a pre-main call? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1921: static SkPoint Rounding() { return {kSubpixelRounding, kSubpixelRounding}; } rounding, xAlignment, yAlignment https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1924: }; Again, simplifying, think there's any harm in expressing these all as a single function of (enum, x,y) -> (fx, fy, rounding)? The three methods all seem to be called right at the same time, and it'd be static so it could be optimized the crap out of. Given that these guys have no state, it'd be nice to just work with a plain old function instead of structs, etc. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1951: virtual ~GlyphFindAndPlaceInterface() { }; Mixing templates and virtuals is getting me kind of seasick. After reading this file for a while, I've realized I've got no idea what a GlyphProcessor is. I am worried that we're making up noun-entities and inheritance relationships so that we can use virtual dispatch to emulate what can be expressed as simple conditional branches. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1953: // compile error. Can you tack on Clang <= vX.Y so we know when to try removing this? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1954: virtual void FindAndPositionGlyph(const char **text, SkPoint position, findAndPositionGlyph https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2036: // and allows the baseline to look crisp. What's the code-size impact of all these new classes? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2091: static void SpecializedProcessPosText(const char *const text, size_t byteLength, specialized_process_pos_text https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2128: PositionReader positionReader { I think I'd still prefer these to be written with () on the outside. It just still reads to me like we're creating a struct with a single lambda field, not calling a constructor. I don't see anything wrong with using {} for struct-y things, like SkPoints. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2142: || scalarsPerPosition == 2) { What's scalarsPerPosition have to do with what mapper we use? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2144: } else if (mtype & SkMatrix::kScale_Mask) { Is y-scale impossible? https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2236: There's something to be said for this existing code. You can see the control flow explicitly.
https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1733: // Calculate a type with the same size and alignment as a list of types Ts. On 2015/11/04 17:54:11, mtklein wrote: > You might just drop the AndAlignment on these names. It's aligned enough, but > doesn't have the same alignment as Ts. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1745: SkAlignedSStorage<MaxSizeOf<Ts...>::value> unused; On 2015/11/04 17:54:11, mtklein wrote: > Even > > template <typename... Ts> > using SameSizeAndAlignmentAs = SkAlignedSStorage<MaxSizeOf<Ts...>::value>; I just embeded SkAlignedSStorage<MaxSizeOf<Ts...>::value> into UntaggedVariant. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1752: class UntaggedVariant { On 2015/11/04 17:54:12, mtklein wrote: > I'm not sure I see why SameSizeAndAlignmentAs is separate from UntaggedVariant, > or UntaggedVariant is separate from PolymorphicVariant. Seems like things would > be clearer if we just merged them all together into PolymorphicVariant. I agree with the SameSizeAndAlignmentAs. This is from code I previously wrote, and it was used in a couple of places. UntaggedVariant is passed to the PolymorphicVariant Initialize function to allow manual initialization of raw memory. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1760: template <typename Variant, typename... Args> On 2015/11/04 17:54:11, mtklein wrote: > It'd be nice to add a newline before template. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1763: #ifndef SK_BUILD_FOR_WIN32 On 2015/11/04 17:54:11, mtklein wrote: > Alternatively, I think we could do > > #if defined(_MSC_VER) && _MSC_VER < 1900 > #define alignof __alignof > #endif > > ... use alignof() normally ... > > Might be nice to back off the #ifndef and #endif one tab stop left? Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1774: // PolymorphicVariant holds supclasses of Base without slicing. Ts must be subclasses of Base. On 2015/11/04 17:54:12, mtklein wrote: > sup -> sub Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1781: skstd::forward<Initializer>(initializer)(&fVariants); On 2015/11/04 17:54:12, mtklein wrote: > What's the deal with forward here? We don't seem to be forwarding it anywhere. > Can we not just > > PolymorphicVariant(Initializer&& initializer) { > initializer(&fVariants); > } > > ? Forgotten experiment. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1784: Base* get() const { return (Base*) &fVariants; } On 2015/11/04 17:54:11, mtklein wrote: > This seems like a spot where reinterpret_cast<> would make things a bit clearer. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1793: // PositionReaderInterface read a point from the pos vector. On 2015/11/04 17:54:11, mtklein wrote: > reads Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1799: virtual const SkPoint NextPoint() = 0; On 2015/11/04 17:54:11, mtklein wrote: > nextPoint() Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1802: class LinearPositions final : public PositionReaderInterface { On 2015/11/04 17:54:12, mtklein wrote: > Even, HorizontalPositions. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1804: LinearPositions(const SkScalar* positions) On 2015/11/04 17:54:11, mtklein wrote: > It seems surprising we don't pass the shared y here and have NextPoint feed it > back. Right. This is a confusion with the existing code. In the existing code, the offset (origin) is backed into the transformation, the general case the offset is added to each point as it comes in (the more intuitive approach). I maintained this behavior. This code can be simplified if one strategy or the other is adopted. Next CL. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1819: : fPositions(positions) { } On 2015/11/04 17:54:12, mtklein wrote: > Typo? Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1841: virtual SkPoint Map(SkPoint position) const = 0; On 2015/11/04 17:54:11, mtklein wrote: > map Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1843: On 2015/11/04 17:54:11, mtklein wrote: > Let's write down a formula for Map explicitly in terms of point, matrix, and > origin? I'm finding myself confused about whether offsetting by origin happens > first, or second, or if it matters. This code faithfully reproduces the existing code. It handles the code in to ways: 1) For the general case M(p+o). 2) For Xscale and Translate it does q = Mo then it calculates Mp+q for all points. I think this can be simplified to (M * translate(o))p But, I want to keep the code as is until it is used in a few places. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1872: GeneralMapper(const SkMatrix& matrix, const SkPoint origin) On 2015/11/04 17:54:11, mtklein wrote: > // Caller must keep matrix alive. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1888: typedef PolymorphicVariant<MapperInterface, TranslationMapper, XScaleMapper, GeneralMapper> Mapper; On 2015/11/04 17:54:12, mtklein wrote: > So, under this scheme, we're always going to occupy enough space for a matrix, > an origin point, and a cached MapXYProc. That's enough to implement all three > of these strategies efficiently, simply by switching out different cached > MapXYProcs. Seems like this doesn't need to have any inheritance at all. E.g. > > class Mapper { > Mapper(const SkMatrix& m, SkPoint origin) : fMatrix(m), fOrigin(origin), > fMapProc(m.getMapXYProc()) {} > > void map(SkPoint p) const { > SkPoint result; > fMapProc(fMatrix, p.fX + fOrigin.fX, p.fY + fOrigin.fY, &result); > return result; > } > ... > }; > > This would have a uniform single indirect function call, where the > implementations we've got here require 1 or 2. The maximum this stores is an SkPoint and two pointers. So, 24 bytes. An SkMatrix is 40 bytes. As I said above, this is a faithful rendering of the existing code. The existing code does not call the matrix proc for the common cases. It is structured as a switch statement to the common case or a case statement to a proc for the general case. The existing code inlines the operation for the common cases, and only does a single call for the general case. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1892: template <SkPaint::Align textAlignment> On 2015/11/04 17:54:12, mtklein wrote: > Reading this file is a serious cognitive load. Given the complexity of all this > code we're adding, let's see if there are places we can simplify. > > I think this is a good place to start: I don't really think there's any benefit > to having this be a template method. It's static and called with constants, so > I've got to figure that if we write this as a normal function of (glyph, Align) > -> SkPoint, good code will be generated. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1907: // Even though the entire enum is covered above, MVSC doesn't think so. Make it happy. On 2015/11/04 17:54:12, mtklein wrote: > Might be nice to SkASSERT(false); too. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1918: static const SkScalar kSubpixelRounding = SkFixedToScalar(SkGlyph::kSubpixelRound); On 2015/11/04 17:54:12, mtklein wrote: > Doesn't this require a pre-main call? No. SkFixedToScalar is actually a macro. There are no function calls, it's implemented using more macros. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1921: static SkPoint Rounding() { return {kSubpixelRounding, kSubpixelRounding}; } On 2015/11/04 17:54:12, mtklein wrote: > rounding, xAlignment, yAlignment Re-did all of this. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1924: }; On 2015/11/04 17:54:11, mtklein wrote: > Again, simplifying, think there's any harm in expressing these all as a single > function of (enum, x,y) -> (fx, fy, rounding)? > The three methods all seem to be called right at the same time, and it'd be > static so it could be optimized the crap out of. > Given that these guys have no state, it'd be nice to just work with a plain old > function instead of structs, etc. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1951: virtual ~GlyphFindAndPlaceInterface() { }; On 2015/11/04 17:54:11, mtklein wrote: > Mixing templates and virtuals is getting me kind of seasick. > > After reading this file for a while, I've realized I've got no idea what a > GlyphProcessor is. > I am worried that we're making up noun-entities and inheritance relationships so > that we can > use virtual dispatch to emulate what can be expressed as simple conditional > branches. Acknowledged. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1953: // compile error. On 2015/11/04 17:54:11, mtklein wrote: > Can you tack on Clang <= vX.Y so we know when to try removing this? Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1954: virtual void FindAndPositionGlyph(const char **text, SkPoint position, On 2015/11/04 17:54:12, mtklein wrote: > findAndPositionGlyph Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2036: // and allows the baseline to look crisp. On 2015/11/04 17:54:12, mtklein wrote: > What's the code-size impact of all these new classes? According too objdump The sub-pixel variants take 1806 bytes. The full pixel variants take 555 bytes. Total = 2361 bytes https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2091: static void SpecializedProcessPosText(const char *const text, size_t byteLength, On 2015/11/04 17:54:11, mtklein wrote: > specialized_process_pos_text Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2128: PositionReader positionReader { On 2015/11/04 17:54:12, mtklein wrote: > I think I'd still prefer these to be written with () on the outside. > It just still reads to me like we're creating a struct with a single lambda > field, not calling a constructor. > > I don't see anything wrong with using {} for struct-y things, like SkPoints. Done. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2142: || scalarsPerPosition == 2) { On 2015/11/04 17:54:12, mtklein wrote: > What's scalarsPerPosition have to do with what mapper we use? Unfortunately, the original code uses that to determine special had written cases. I'm using the same cases for now. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2144: } else if (mtype & SkMatrix::kScale_Mask) { On 2015/11/04 17:54:12, mtklein wrote: > Is y-scale impossible? The Skia API does not have a drawVPosText, so it is impossible. I was surprised also. https://codereview.chromium.org/1420973005/diff/320001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2236: On 2015/11/04 17:54:12, mtklein wrote: > There's something to be said for this existing code. You can see the control > flow explicitly. Acknowledged.
https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1965: virtual void findAndPositionGlyph(const char** text, SkPoint position, How come this is const char**? https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1969: //////////////////////////////////////////////////////////////////////////////////////////////////// let's cut out the ////////////////// section markers until we're done with FindAndPlaces (e.g. 2067 looks good). https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1971: // requested. After it has found a placed the glyph it calls the templated function found and placed https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1973: template <typename GlyphProcessor, SkPaint::Align textAlignment, SkAxisAlignment axisAlignment> I think code like this reads a little more clearly if we follow the naming convention for constants for non-type-non-function template arguments like textAlignment and axisAlignment. kTextAlignment / kAxisAlignment? https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2098: // only one variant that is used: sub-pixel position, left-aligned, x-axis-aligned, translation, used -> specialized ? Might also add // This is by far the most common type of text Blink draws. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2101: static void specialized_process_pos_text(const char *const text, size_t byteLength, I'd like to find a way to get the can-we-use-this logic here, rather than down far away in drawPosText(). E.g., have this return a bool and take all the arguments, bailing out early if they don't meet the required specialization, and write drawPosText() like: if (specialized_process_pos_text(...) { return; } process_pos_text(...); https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2138: static void ProcessPosText(const char text[], size_t byteLength, process_pos_text https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2260: && (mtype I think SkMatrix::getType() does this mask for you. Should be fMatrix->getType() <= SkMatrix::kTranslate_Mask is fine. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2275: [&](const SkGlyph &glyph, SkPoint position, SkPoint rounding) { These are the same lambda, right? Why don't we give it a name and write it once?
https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1965: virtual void findAndPositionGlyph(const char** text, SkPoint position, On 2015/11/05 18:33:08, mtklein wrote: > How come this is const char**? Because the internal call to the GlyphCacheProc advances the text pointer. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1969: //////////////////////////////////////////////////////////////////////////////////////////////////// On 2015/11/05 18:33:08, mtklein wrote: > let's cut out the ////////////////// section markers until we're done with > FindAndPlaces (e.g. 2067 looks good). Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1971: // requested. After it has found a placed the glyph it calls the templated function On 2015/11/05 18:33:08, mtklein wrote: > found and placed Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1973: template <typename GlyphProcessor, SkPaint::Align textAlignment, SkAxisAlignment axisAlignment> On 2015/11/05 18:33:08, mtklein wrote: > I think code like this reads a little more clearly if we follow the naming > convention for constants for non-type-non-function template arguments like > textAlignment and axisAlignment. > > kTextAlignment / kAxisAlignment? Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2098: // only one variant that is used: sub-pixel position, left-aligned, x-axis-aligned, translation, On 2015/11/05 18:33:08, mtklein wrote: > used -> specialized ? > > Might also add // This is by far the most common type of text Blink draws. Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2101: static void specialized_process_pos_text(const char *const text, size_t byteLength, On 2015/11/05 18:33:08, mtklein wrote: > I'd like to find a way to get the can-we-use-this logic here, rather than down > far away in drawPosText(). > > E.g., have this return a bool and take all the arguments, bailing out early if > they don't meet the required specialization, and write drawPosText() like: > > if (specialized_process_pos_text(...) { > return; > } > process_pos_text(...); Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2138: static void ProcessPosText(const char text[], size_t byteLength, On 2015/11/05 18:33:08, mtklein wrote: > process_pos_text Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2260: && (mtype On 2015/11/05 18:33:08, mtklein wrote: > I think SkMatrix::getType() does this mask for you. Should be > fMatrix->getType() <= SkMatrix::kTranslate_Mask > is fine. Done. https://codereview.chromium.org/1420973005/diff/360001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2275: [&](const SkGlyph &glyph, SkPoint position, SkPoint rounding) { On 2015/11/05 18:33:08, mtklein wrote: > These are the same lambda, right? Why don't we give it a name and write it > once? Done.
The CQ bit was checked by herb@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/1420973005/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420973005/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
just nits https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1794: virtual const SkPoint nextPoint() = 0; The const here is sort of pointless eh? https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1855: return {SkScalarMul(fXScale, position.fX) + fTranslate.fX, fTranslate.fY}; SkScalarMul(x,y) can be safely replaced with (x*y) if you like. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1920: default: I sort of like your default-less style in text_alignment_adjustment better. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1938: default: Ditto. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1948: template <typename GlyphProcessor> As a name, I think I'd have followed this better if it were GlyphCallback GlyphFn ProcessOneGlyph ... something to indicate it's a callable type. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2097: SkDrawCacheProc &glyphCacheProc, Usually we put the * or & on the type. It's also fairly unusual for us to pass by non-const&. If this is a function pointer, pass by value? https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2117: &cursor, mappedPoint, skstd::forward<GlyphProcessor>(processOneGlyph)); Out of curiosity, what happens if we don't forward the GlyphProcessor here? https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2215: &text, mappedPoint, skstd::forward<GlyphProcessor>(processOneGlyph)); Same? https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2256: auto glyphProcessor = processOneGlyph, for consistency? https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2257: [&](const SkGlyph &glyph, SkPoint position, SkPoint rounding) { const SkGlyph&
https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1794: virtual const SkPoint nextPoint() = 0; On 2015/11/09 16:06:40, mtklein wrote: > The const here is sort of pointless eh? Done. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1855: return {SkScalarMul(fXScale, position.fX) + fTranslate.fX, fTranslate.fY}; On 2015/11/09 16:06:40, mtklein wrote: > SkScalarMul(x,y) can be safely replaced with (x*y) if you like. Done. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1920: default: On 2015/11/09 16:06:40, mtklein wrote: > I sort of like your default-less style in text_alignment_adjustment better. Done. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1938: default: On 2015/11/09 16:06:40, mtklein wrote: > Ditto. Done. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:1948: template <typename GlyphProcessor> On 2015/11/09 16:06:40, mtklein wrote: > As a name, I think I'd have followed this better if it were > GlyphCallback > GlyphFn > ProcessOneGlyph > ... something to indicate it's a callable type. Done. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2097: SkDrawCacheProc &glyphCacheProc, On 2015/11/09 16:06:40, mtklein wrote: > Usually we put the * or & on the type. > > It's also fairly unusual for us to pass by non-const&. If this is a function > pointer, pass by value? Yep. Sometimes when I upgrade my IDE, my prefs get changed, and the editor changes everything behind my back. Grrr. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2117: &cursor, mappedPoint, skstd::forward<GlyphProcessor>(processOneGlyph)); On 2015/11/09 16:06:40, mtklein wrote: > Out of curiosity, what happens if we don't forward the GlyphProcessor here? Things get slower if I use const T& style, but I have not investigated the code. According to the microsoft STL maintainer (stl@microsoft) there is a move to making universal refs be the normal way of passing callable things instead of by value. See http://en.cppreference.com/w/cpp/algorithm/random_shuffle. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2215: &text, mappedPoint, skstd::forward<GlyphProcessor>(processOneGlyph)); On 2015/11/09 16:06:40, mtklein wrote: > Same? See above https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2256: auto glyphProcessor = On 2015/11/09 16:06:40, mtklein wrote: > processOneGlyph, for consistency? Done. https://codereview.chromium.org/1420973005/diff/380001/src/core/SkDraw.cpp#ne... src/core/SkDraw.cpp:2257: [&](const SkGlyph &glyph, SkPoint position, SkPoint rounding) { On 2015/11/09 16:06:40, mtklein wrote: > const SkGlyph& Grrr.
PTAL
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1420973005/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420973005/400001
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 2015-11-09 22:40 UTC
lgtm
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://skia.googlesource.com/skia/+/f553e4e09cd2baf15fc041daab8a08bd46e352f0 |
