Bug 221979 - Web Inspector: Implement Grid Overlay "Show area names" drawing
Summary: Web Inspector: Implement Grid Overlay "Show area names" drawing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-16 11:25 PST by Patrick Angle
Modified: 2021-02-16 17:16 PST (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (4.43 KB, patch)
2021-02-16 12:56 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.0 (467.74 KB, image/png)
2021-02-16 12:57 PST, Patrick Angle
no flags Details
Patch v1.1 - NIT and Style Notes (4.62 KB, patch)
2021-02-16 13:45 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - NIT and Style Notes (4.62 KB, patch)
2021-02-16 13:46 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.2 - Label Translucency and Clipping (6.64 KB, patch)
2021-02-16 15:47 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.2 (974.53 KB, image/png)
2021-02-16 15:48 PST, Patrick Angle
no flags Details
Patch v1.3 - Review notes (6.63 KB, patch)
2021-02-16 16:18 PST, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-02-16 11:25:40 PST
Implement the drawing of area boxes and names in the grid overlay
Comment 1 Patrick Angle 2021-02-16 12:56:41 PST
Created attachment 420526 [details]
Patch v1.0
Comment 2 Patrick Angle 2021-02-16 12:57:05 PST
Created attachment 420527 [details]
Screenshot of Patch v1.0
Comment 3 Devin Rousso 2021-02-16 13:28:16 PST
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
Comment 4 Devin Rousso 2021-02-16 13:30:02 PST
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.
Comment 5 Patrick Angle 2021-02-16 13:45:52 PST
Created attachment 420536 [details]
Patch v1.1 - NIT and Style Notes
Comment 6 Patrick Angle 2021-02-16 13:46:58 PST
Created attachment 420538 [details]
Patch v1.1 - NIT and Style Notes
Comment 7 Radar WebKit Bug Importer 2021-02-16 13:47:53 PST
<rdar://problem/74405136>
Comment 8 Patrick Angle 2021-02-16 15:47:36 PST
Created attachment 420551 [details]
Patch v1.2 - Label Translucency and Clipping
Comment 9 Patrick Angle 2021-02-16 15:48:17 PST
Created attachment 420552 [details]
Screenshot of Patch v1.2
Comment 10 Devin Rousso 2021-02-16 16:08:04 PST
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 :)
Comment 11 Patrick Angle 2021-02-16 16:18:22 PST
Created attachment 420558 [details]
Patch v1.3 - Review notes
Comment 12 EWS 2021-02-16 17:16:22 PST
Committed r272948: <https://commits.webkit.org/r272948>

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