Implement the drawing of area boxes and names in the grid overlay
Created attachment 420526 [details] Patch v1.0
Created attachment 420527 [details] Screenshot of Patch v1.0
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 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.
Created attachment 420536 [details] Patch v1.1 - NIT and Style Notes
Created attachment 420538 [details] Patch v1.1 - NIT and Style Notes
<rdar://problem/74405136>
Created attachment 420551 [details] Patch v1.2 - Label Translucency and Clipping
Created attachment 420552 [details] Screenshot of Patch v1.2
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 :)
Created attachment 420558 [details] Patch v1.3 - Review notes
Committed r272948: <https://commits.webkit.org/r272948> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420558 [details].