Bug 124071 – Web Inspector: [CSS Shapes] Highlight shape-outside when its element is selected in the Web Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Bear Travis]

A first step to visualizing shapes would be to highlight them at the same time as drawing the box highlights.

[Radar WebKit Bug Importer]

<rdar://problem/15427868>

[Bear Travis]

Created attachment 216442 [details]
Initial patch

[Bear Travis]

Created attachment 216444 [details]
Screenshot of highlight behavior

What the highlighting behavior looks like.

[Bem Jones-Bey]

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?

[Bear Travis]

Created attachment 216449 [details]
Increasing compile guard scope

Done and done.

[Timothy Hatcher]

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.

[Bear Travis]

Created attachment 216457 [details]
Incorporating feedback

Making the suggested changes.

[Bear Travis]

Created attachment 216470 [details]
Updating patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

(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".

[Bear Travis]

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.

[Joseph Pecoraro]

Do you need a cq+? (You could put cq? to signal this).

[Bear Travis]

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 ;)

[WebKit Commit Bot]

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 '[e-mail]': 
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

[Bear Travis]

Created attachment 216607 [details]
Updating patch

[WebKit Commit Bot]

Comment on attachment 216607 [details]
Updating patch

Clearing flags on attachment: 216607

Committed r159070: <http://trac.webkit.org/changeset/159070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.