[webkit-reviews] review granted: [Bug 124071] Web Inspector: [CSS Shapes] Highlight shape-outside when its element is selected in the Web Inspector : [Attachment 216449] Increasing compile guard scope

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Bear Travis
<betravis at adobe.com>'s request for review:
Bug 124071: Web Inspector: [CSS Shapes] Highlight shape-outside when its
element is selected in the Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=124071

Attachment 216449: Increasing compile guard scope
https://bugs.webkit.org/attachment.cgi?id=216449&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.