[webkit-reviews] review granted: [Bug 124080] Web Inspector: [CSS Shapes] Support raster shape visualizations : [Attachment 217905] Updated patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Bear Travis
<betravis at adobe.com>'s request for review:
Bug 124080: Web Inspector: [CSS Shapes] Support raster shape visualizations
https://bugs.webkit.org/show_bug.cgi?id=124080

Attachment 217905: Updated patch
https://bugs.webkit.org/attachment.cgi?id=217905&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217905&action=review


r=me

> Source/WebCore/rendering/shapes/RasterShape.cpp:266
> +	   for (; endY < bounds().maxY(); endY++) {
> +	       if (intervalsAt(endY).isEmpty() || limitIntervalAt(endY) !=
extent)
> +		   break;
> +	   }

It looks to me like limitIntervalAt will give you one big rect at a y, even if
there are holes between shapes on that y. So essentially you'll get an outline
of all the shapes, but you won't get perfectly accurate information within the
outlines.

In the future it might be nice to show exactly where the shape boundaries are
in an overlay. But that was not the intent of this patch.

> Source/WebCore/rendering/shapes/RasterShape.h:58
> +    void buildPath(Path&) const;

Maybe this could be made something like "buildBoundsPath" /
"buildOutlineLimitPath" to be more specific? I could imagine a
"buildShapesPath" which build path as specific as possible.

> LayoutTests/inspector-protocol/model/highlight-shape-outside.html:38
> +.raster-simple {
> +    -webkit-shape-outside: url("data:image/svg+xml;utf8,<svg
xmlns='http://www.w3.org/2000/svg' viewBox='0 0 100 100'
preserveAspectRatio='none' width='100px' height='100px'><rect x='25' y='25'
width='50' height='50' fill='blue' /></svg>");
> +}

I'd like to see a test with a data:image/png like some of the other
-webkit-shape-outside tests. Maybe just a simple 25x25 png.