Bug 221979 – Web Inspector: Implement Grid Overlay "Show area names" drawing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

Implement the drawing of area boxes and names in the grid overlay

[Patrick Angle]

Created attachment 420526 [details]
Patch v1.0

[Patrick Angle]

Created attachment 420527 [details]
Screenshot of Patch v1.0

[Devin Rousso]

Comment on attachment 420526 [details]
Patch v1.0

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

r=me

> Source/WebCore/ChangeLog:14
> +        - Added support for stroking grid areas and showing their names in a label.

dunno why this makes me imagine you having a cat named "grid areas" :P

> Source/WebCore/inspector/InspectorOverlay.cpp:1177
> +void InspectorOverlay::drawLayoutLabel(GraphicsContext& context, String label, FloatPoint point, InspectorOverlay::LabelArrowDirection direction = InspectorOverlay::LabelArrowDirection::None)

Style: we usually put default parameter values in the `.h`

> Source/WebCore/inspector/InspectorOverlay.cpp:1386
> +            columnPositions[area.columns.startLine()] + gridBoundingBox.x(),

NIT: you might consider pulling `area.foo.startLine()` and `area.foo.endLine()` into local variables since they're each repeated (but only if the resulting code looks cleaner)

> Source/WebCore/inspector/InspectorOverlay.h:175
> +        None,

NIT: i'd put this first in the list as it's kinda a base-case of sorts

[Devin Rousso]

Comment on attachment 420527 [details]
Screenshot of Patch v1.0

Should we maybe add some transparency to the label background?  I don't think it's as important for the row/column numbers since they're (usually) drawn outside of the grid, but for stuff inside the grid (especially if they're larger, like an identifier string) we should try to avoid occluding page content.

[Patrick Angle]

Created attachment 420536 [details]
Patch v1.1 - NIT and Style Notes

[Patrick Angle]

Created attachment 420538 [details]
Patch v1.1 - NIT and Style Notes

[Radar WebKit Bug Importer]

<rdar://problem/74405136>

[Patrick Angle]

Created attachment 420551 [details]
Patch v1.2 - Label Translucency and Clipping

[Patrick Angle]

Created attachment 420552 [details]
Screenshot of Patch v1.2

[Devin Rousso]

Comment on attachment 420551 [details]
Patch v1.2 - Label Translucency and Clipping

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

r=me (again 😛)

> Source/WebCore/inspector/InspectorOverlay.cpp:1197
> +        // Add an ellipsis to the label.

redundant IMO

> Source/WebCore/inspector/InspectorOverlay.cpp:1198
> +        label.append("...");

add `_s` after the closing `"` (this converts the cstring to `ASCIILiteral`)

> Source/WebCore/inspector/InspectorOverlay.cpp:1199
> +        while (textWidth + (padding * 2) > maximumWidth || label.length() <= 1) {

Shouldn't this be `&& label.length() >= 4`?

> Source/WebCore/inspector/InspectorOverlay.h:187
> +    void drawLayoutLabel(GraphicsContext&, String, FloatPoint, LabelArrowDirection, Color = Color::white, float = 0);

Style: please give a name to generic arguments like `Color backgroundColor = Color::white` and `float maximumWidth = 0` so that someone just looking at the header can figure out what the arguments are for without having to go spelunking into the implementation :)

[Patrick Angle]

Created attachment 420558 [details]
Patch v1.3 - Review notes

[EWS]

Committed r272948: <https://commits.webkit.org/r272948>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420558 [details].