Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
892973002:
[SkSVGDevice] Initial shader/gradient support (Closed)
Description[SkSVGDevice] Initial shader/gradient support
* linear gradient support (based on shawcroft@google.com's CL)
* generic paint resources reorg
* opacity support
R=reed@google.com,mtklein@google.com,halcanary@google.com
Committed: https://skia.googlesource.com/skia/+/532faa9021cde714bc1fcc5650257fd7310f9712
Patch Set 1 #
Total comments: 22
Patch Set 2 : release build fix #Patch Set 3 : review comments #Patch Set 4 : get rid of AutoResources #
Total comments: 10
Patch Set 5 : moar review #
Total comments: 2
Patch Set 6 : final tweak #Messages
Total messages: 11 (1 generated)
https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... File experimental/svg/SkSVGDevice.cpp (right): https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:18: namespace { You can probably drop this namespace for now, given that there's only static function inside it. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:35: class SkSVGDevice::AutoResources { : SkNoncopyable for both these? https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:38: Maybe dumb question... if this guy doesn't have a custom destructor, what is the Auto-nature of the class? Am I reading it right that it's really logically just a function to SVGize an SkPaint, but we're using a class to anticipate future internal methods and other structure? https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:98: void addTransform(const SkMatrix &t, const char* name = NULL) { SkMatrix& ? https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:115: default: case SkMatrix::kAffineMask: <this code> default: SkDebugf("Can't handle non-affine matrices."); ? https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:123: fWriter->addAttribute(SkToBool(name) ? name : "transform", tstr.c_str()); Can we not just default name to "transform"? https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:148: AutoElement defs("defs", fWriter); It might help these standalone AutoElements to jump out if we explicitly scope their lifetimes with braces? Seems easy to skim over and miss the nesting. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:212: AutoElement elem("svg", fWriter, false); Probably worth a note to point out how this tag gets closed. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:218: this->pushElement(elem); Seems like we really don't need an explicit stack yet? Can we just make this elem an fRootElement or similar member, and let its destructor close it naturally at the right time? https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:223: popElement(); this-> https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:230: void SkSVGDevice::pushElement(const AutoElement&) { Might want to assert the AutoElement isn't auto-closing here?
https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... File experimental/svg/SkSVGDevice.cpp (right): https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:18: namespace { On 2015/02/02 16:52:48, mtklein wrote: > You can probably drop this namespace for now, given that there's only static > function inside it. Done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:35: class SkSVGDevice::AutoResources { On 2015/02/02 16:52:48, mtklein wrote: > : SkNoncopyable for both these? Done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:38: On 2015/02/02 16:52:49, mtklein wrote: > Maybe dumb question... if this guy doesn't have a custom destructor, what is the > Auto-nature of the class? Am I reading it right that it's really logically just > a function to SVGize an SkPaint, but we're using a class to anticipate future > internal methods and other structure? Yeah there is no auto dtor logic for this class, but it has enough magic in its ctor (emits a <defs> block with all the associated resources) to warrant an "auto" name IMO. Unless you find it totally misleading, in which case we should rename it. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:98: void addTransform(const SkMatrix &t, const char* name = NULL) { On 2015/02/02 16:52:48, mtklein wrote: > SkMatrix& ? Done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:115: default: On 2015/02/02 16:52:48, mtklein wrote: > case SkMatrix::kAffineMask: > <this code> > default: > SkDebugf("Can't handle non-affine matrices."); > > ? kAffineMask is only set when rotating/skewing so it won't quite work in this form. But good catch for the non-affine case. Refactored. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:123: fWriter->addAttribute(SkToBool(name) ? name : "transform", tstr.c_str()); On 2015/02/02 16:52:48, mtklein wrote: > Can we not just default name to "transform"? Done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:148: AutoElement defs("defs", fWriter); On 2015/02/02 16:52:48, mtklein wrote: > It might help these standalone AutoElements to jump out if we explicitly scope > their lifetimes with braces? Seems easy to skim over and miss the nesting. Done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:212: AutoElement elem("svg", fWriter, false); On 2015/02/02 16:52:49, mtklein wrote: > Probably worth a note to point out how this tag gets closed. Done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:218: this->pushElement(elem); On 2015/02/02 16:52:48, mtklein wrote: > Seems like we really don't need an explicit stack yet? Can we just make this > elem an fRootElement or similar member, and let its destructor close it > naturally at the right time? That was my first thought also, but gave up initially because of its relative order vs. the ~SkSVGDevice flush() call. Taking a closer look, I don't think that's needed: the stream dtor takes care of it. So yes, this should work - done. https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:223: popElement(); On 2015/02/02 16:52:48, mtklein wrote: > this-> removed https://codereview.chromium.org/892973002/diff/1/experimental/svg/SkSVGDevice... experimental/svg/SkSVGDevice.cpp:230: void SkSVGDevice::pushElement(const AutoElement&) { On 2015/02/02 16:52:48, mtklein wrote: > Might want to assert the AutoElement isn't auto-closing here? removed
Refactored to get rid of AutoResources and hide all the magic behind the AutoElement ctor. I think this is cleaner and DRYer - PTAL.
Don't make me sic TSAN on you. https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... File experimental/svg/SkSVGDevice.cpp (right): https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:33: struct Resources { FWIW, I am a big fan of data passed by value until proven slow. +1 here. https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:37: SkString fPaint; fPaintColor? https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:93: fWriter->addAttribute("fill", resources.fPaint.c_str()); Might be more readable here if you dogfood your own methods, e.g. this->addAttribute("fill", resources.fPaint); ? https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:142: Resources resources(paint); Given the number of times you need to "return resources", you might consider factoring that out: Resources addResources(const SkPaint& paint) { Resources resources(paint); this->addResourceDefs(resources, paint); return resources; } void addResourceDefs(const Resources&, const SkPaint&) { const SkShader* shader = paint.getShader(); if (!shader) { return; } ...etc... } https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:179: static uint32_t linear_gradient_counter = 0; ಠ_ಠ Not incremented atomically. Not initialized to zero in a threadsafe way in Chrome. Yet another thread-hostile Skia backend... lame. (I'd go with static int32_t linear_gradient_counter = 0 outside the function and sk_atomic_inc() it for now, or even better scope the counter to your device.)
https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... File experimental/svg/SkSVGDevice.cpp (right): https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:33: struct Resources { On 2015/02/02 22:30:10, mtklein wrote: > FWIW, I am a big fan of data passed by value until proven slow. +1 here. Ack. https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:37: SkString fPaint; On 2015/02/02 22:30:10, mtklein wrote: > fPaintColor? The thing is it's not necessarily a color: it can be a gradient/pattern reference. In SVG land they're called paint servers, so maybe we should use the same. Updated. https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:93: fWriter->addAttribute("fill", resources.fPaint.c_str()); On 2015/02/02 22:30:10, mtklein wrote: > Might be more readable here if you dogfood your own methods, e.g. > this->addAttribute("fill", resources.fPaint); ? Done. https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:142: Resources resources(paint); On 2015/02/02 22:30:10, mtklein wrote: > Given the number of times you need to "return resources", you might consider > factoring that out: > > Resources addResources(const SkPaint& paint) { > Resources resources(paint); > this->addResourceDefs(resources, paint); > return resources; > } > > void addResourceDefs(const Resources&, const SkPaint&) { > const SkShader* shader = paint.getShader(); > if (!shader) { > return; > } > ...etc... > } Done. https://codereview.chromium.org/892973002/diff/60001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:179: static uint32_t linear_gradient_counter = 0; On 2015/02/02 22:30:10, mtklein wrote: > ಠ_ಠ > > Not incremented atomically. Not initialized to zero in a threadsafe way in > Chrome. Yet another thread-hostile Skia backend... lame. > > (I'd go with static int32_t linear_gradient_counter = 0 outside the function and > sk_atomic_inc() it for now, or even better scope the counter to your device.) What?! You're telling me this is not single-threaded Blink?! One of these days I'll turn full-schizo from all this context switching. In the meantime, you can find me in the shame cube.
lgtm https://codereview.chromium.org/892973002/diff/80001/experimental/svg/SkSVGDe... File experimental/svg/SkSVGDevice.cpp (right): https://codereview.chromium.org/892973002/diff/80001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:169: void SkSVGDevice::AutoElement::addResourceDefs(const SkPaint& paint, Resources *resources) { Resources* ?
https://codereview.chromium.org/892973002/diff/80001/experimental/svg/SkSVGDe... File experimental/svg/SkSVGDevice.cpp (right): https://codereview.chromium.org/892973002/diff/80001/experimental/svg/SkSVGDe... experimental/svg/SkSVGDevice.cpp:169: void SkSVGDevice::AutoElement::addResourceDefs(const SkPaint& paint, Resources *resources) { On 2015/02/03 03:55:04, mtklein wrote: > Resources* ? Done.
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892973002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/532faa9021cde714bc1fcc5650257fd7310f9712 |
