A first step to visualizing shapes would be to highlight them at the same time as drawing the box highlights.
<rdar://problem/15427868>
Created attachment 216442 [details] Initial patch
Created attachment 216444 [details] Screenshot of highlight behavior What the highlighting behavior looks like.
Comment on attachment 216442 [details] Initial patch View in context: https://bugs.webkit.org/attachment.cgi?id=216442&action=review > Source/WebCore/inspector/InspectorOverlay.cpp:594 > +#if ENABLE(CSS_SHAPES) Shouldn't all of these new functions be enclosed in the #if ENABLE?
Created attachment 216449 [details] Increasing compile guard scope Done and done.
Comment on attachment 216449 [details] Increasing compile guard scope View in context: https://bugs.webkit.org/attachment.cgi?id=216449&action=review > Source/WebCore/inspector/InspectorOverlay.cpp:616 > + } > + case Shape::PolygonType: { Newline would be nice here. > Source/WebCore/inspector/InspectorOverlay.cpp:625 > + } > + for (size_t i = 1; i < polygon.numberOfVertices(); i++) { Ditto. > Source/WebCore/inspector/InspectorOverlay.cpp:629 > + } > + if (polygon.numberOfVertices()) Ditto. > Source/WebCore/inspector/InspectorOverlay.cpp:633 > + default: break; Instead of default, use a case for RasterType. That way any new shape type added won't me ignored and cause a compile warning. And add a FIXME comment about why raster isn't supported yet. > Source/WebCore/inspector/InspectorOverlay.cpp:635 > + } > + if (path.length()) { Newline would be nice here. > Source/WebCore/inspector/InspectorOverlay.cpp:638 > + PathApplyInfo info; > + RefPtr<InspectorArray> shapePath = InspectorArray::create(); > + info.rootView = containingFrame->page()->mainFrame().view(); Putting the info variable closer to where it is used would be nice. > Source/WebCore/inspector/InspectorOverlay.cpp:643 > + info.renderer = renderer; > + path.apply(&info, &appendPathSegment); > + shapeObject->setArray("path", shapePath.release()); A newline between these lines would help separate the setup from the work. > Source/WebCore/inspector/InspectorOverlay.cpp:707 > + RefPtr<InspectorObject> shapeObject = buildObjectForShapeOutside(containingFrame, renderBox); > + if (shapeObject) Can be combined into one line with shapeObject being defined in the if (). > Source/WebCore/inspector/InspectorOverlayPage.js:216 > + switch(commands[0]) { > + // 1 point > + case 'M': > + commands = pathCommand(context, commands, "moveTo", 2); > + break; > + // 1 point > + case 'L': > + commands = pathCommand(context, commands, "lineTo", 2); > + break; > + // 3 points > + case 'C': > + commands = pathCommand(context, commands, "bezierCurveTo", 6); > + break; > + default: > + commands = commands.slice(1); > + } WebKit style does not indent the cases. They align with the switch.
Created attachment 216457 [details] Incorporating feedback Making the suggested changes.
Created attachment 216470 [details] Updating patch
Comment on attachment 216457 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=216457&action=review Some late mostly drive-by comments, but one bit of concern. > Source/WebCore/inspector/InspectorOverlay.cpp:553 > +static void appendPathCommandAndPoints(PathApplyInfo* info, const String& command, const FloatPoint points[], unsigned length) Nit: Instead of passing length, this could be determined dynamically: unsigned length = points ? WTF_ARRAY_LENGTH(points) : 0; Might be safer if things ever changed. But I think you have the right idea, since the overlay has hardcoded to match numbers. > Source/WebCore/inspector/InspectorOverlay.cpp:571 > + appendPathCommandAndPoints(pathApplyInfo, "M", pathElement->points, 1); The constant strings in each of these "M", "L", "C", etc could be made ASCIILiterals for a faster conversation to WTFString: <http://trac.webkit.org/wiki/EfficientStrings> ASCIILiteral("M") > Source/WebCore/inspector/InspectorOverlay.cpp:638 > + case Shape::RasterType: break; It would be clearer to put the FIXME & break inside the case. But that is not a big deal. > Source/WebCore/inspector/InspectorOverlayPage.js:195 > +function pathCommand(context, commands, name, length) { > + context[name].apply(context, commands.slice(1, length + 1)); > + return commands.slice(length + 1); > +} > + > +function drawPath(context, commands, fillColor, outlineColor) I have some concerns about this code. While it is clever and elegant, it does a lot of unnecessary array manipulation. I wondered if this could cause unnecessary allocations / copies. Especially since array.slice(…) with primitives returns a copy. For a very long list of values that would be bad. I wrote up a test comparing arrayManipulation (via slice) to an implementation that just walks the single array (via index) and the results were dramatically different: <http://jsfiddle.net/4r4v3/> Of course, this all depends on how long the list of commands are… But still, I'd suggest revisiting this and just walking over the array. > Source/WebCore/inspector/InspectorOverlayPage.js:217 > + while(commands.length) { > + switch(commands[0]) { > + // 1 point > + case 'M': > + commands = pathCommand(context, commands, "moveTo", 2); > + break; > + // 1 point > + case 'L': > + commands = pathCommand(context, commands, "lineTo", 2); > + break; > + // 3 points > + case 'C': > + commands = pathCommand(context, commands, "bezierCurveTo", 6); > + break; > + default: > + commands = commands.slice(1); > + } > + } Does this need to handle "Q", output above via PathElementAddQuadCurveToPoint, outputting 2 points? > Source/WebCore/rendering/shapes/ShapeInfo.h:140 > + ShapeInfo(const RenderType* renderer): m_renderer(renderer) { } Nit: I know you are not changing this, but normally for constructors with a single argument we add the "explicit" keyword to prevent implicit casts. Feel free to add that here.
(In reply to comment #9) > The constant strings in each of these "M", "L", "C", etc could be made ASCIILiterals for a faster conversation to WTFString: Lol. "conversation" => "conversion".
Created attachment 216486 [details] Incorporating Feedback Got in all of the above feedback except for using WTF_ARRAY_LENGTH, which unfortunately isn't viable as the array is passed in as a pointer. The path commands for a shape should tend to be short, but it shouldn't be a problem to iterate by index rather than continuously splicing. The shapes don't ever generate quad curves, but I went ahead and added them for completeness. Only move, line, and bezier curves are used. I may eventually clean this up via SVGPathStringBuilder, but I would have to reimplement the rounded rectangle generation code.
Do you need a cq+? (You could put cq? to signal this).
Comment on attachment 216486 [details] Incorporating Feedback I just wanted to check that the changes met your concerns. I'll take your comment as a yes ;)
Comment on attachment 216486 [details] Incorporating Feedback Rejecting attachment 216486 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 216486, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: t.org/repository/webkit/trunk ... Authentication realm: <http://svn.webkit.org:80> Mac OS Forge Password for 'commit-queue@webkit.org': Authentication realm: <http://svn.webkit.org:80> Mac OS Forge Username: Use of uninitialized value $username in chomp at /usr/share/git-core/perl/Git/SVN/Prompt.pm line 114. error: git-svn died of signal 11 Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.appspot.com/results/22868489
Created attachment 216607 [details] Updating patch
Comment on attachment 216607 [details] Updating patch Clearing flags on attachment: 216607 Committed r159070: <http://trac.webkit.org/changeset/159070>
All reviewed patches have been landed. Closing bug.