[webkit-reviews] review granted: [Bug 32263] Web Inspector: Add rulers and guides : [Attachment 340395] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 32263: Web Inspector: Add rulers and guides
https://bugs.webkit.org/show_bug.cgi?id=32263

Attachment 340395: Patch

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




--- Comment #15 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 340395
  --> https://bugs.webkit.org/attachment.cgi?id=340395
Patch

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

r=me, with some cleanup and behavior suggestions. Other than that, it looks
great. Nice work.

> Source/WebCore/inspector/InspectorOverlayPage.html:41
> +    <div id="log"></div>

Nice cleanup.

> Source/WebCore/inspector/InspectorOverlayPage.js:69
> +	   mirrorY: bounds.minX < DATA.contentInset.width + gridSize &&
bounds.maxX < DATA.viewportSize.width - gridSize,

I don't think we should ever mirror the ruler axis. I understand this is to
prevent overlapping, but I think overlapping is acceptable. It was surprising
to see the ruler move during element selection. At first I thought it had
disappeared.

> Source/WebCore/inspector/InspectorOverlayPage.js:222
> +	   context.lineWidth = 2;

Make this magic number a named const with the others (gridSize, etc).

> Source/WebCore/inspector/InspectorOverlayPage.js:387
> +	   context.strokeStyle = "rgb(128, 128, 128)";

We have some other color consts at the top of the file. Let's move these up
there too.

> Source/WebCore/inspector/InspectorOverlayPage.js:501
> +		       context.fillText(x, 2, options.mirrorX ? zoom(height) -
DATA.contentInset.height - 7 : 13);

Please name these constants.

> Source/WebCore/inspector/InspectorOverlayPage.js:539
> +		       context.lineTo(zoom(x), scrollY + ((x % (gridStep * 2))
? 5 : 8));

Please make these numbers named constants. They get used for both axes, so they
should probably go above the "// Draw horizontal grid" comment.