[webkit-reviews] review granted: [Bug 105023] Web Inspector: Change the InspectorOverlay to use native rather than canvas : [Attachment 360812] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 105023: Web Inspector: Change the InspectorOverlay to use native rather
than canvas
https://bugs.webkit.org/show_bug.cgi?id=105023

Attachment 360812: Patch

https://bugs.webkit.org/attachment.cgi?id=360812&action=review




--- Comment #47 from Brian Burg <bburg at apple.com> ---
Comment on attachment 360812
  --> https://bugs.webkit.org/attachment.cgi?id=360812
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360812&action=review

r=me Looks good overall. I am not sure it's worth the effort to make a
regression test using the draw commands since they are not as easy to hook now.
Maybe you can take screenshots of the various highlight modes and save them in
a wiki so that myself / others can easily see if there is a visual regression.

> Source/WebCore/inspector/InspectorOverlay.cpp:266
> +		       contentQuad = highlight.quads[3];

This looks weird to me. Make it all aligned and not nested?

> Source/WebCore/inspector/InspectorOverlay.cpp:307
> +    if (paths.shape.length()) {

Please invert this so (!path.shape.length()) is handled first as an early
return. It's a much shorter case.

> Source/WebCore/inspector/InspectorOverlay.cpp:308
> +	   const auto mapPoints = [&] (const Path& path) {

This lambda would be better named 'convertPathLocalToRoot' or something. (I
wasn't familiar with WebCore graphics structs, but this does look pretty neat.)

> Source/WebCore/inspector/InspectorOverlay.cpp:347
> +	       context.setFillColor(Color(96, 82, 127, 153));

Please extract this to a named variable so I know what the color is for.

> Source/WebCore/inspector/InspectorOverlay.cpp:358
> +    } else {

(Right, that was so much drawing I forgot what the else is paired up with. ;-))

> Source/WebCore/inspector/InspectorOverlay.cpp:-192
> -#endif

Where did this code go? What does it do..

> Source/WebCore/inspector/InspectorOverlay.cpp:569
> +

Nit: extra newline.

> Source/WebCore/inspector/InspectorOverlay.cpp:574
> +

Nit: extra newline

> Source/WebCore/inspector/InspectorOverlay.cpp:694
> +    // Draw backgrounds

Nit (throughout): WebKit style requires trailing period in general.