[webkit-reviews] review granted: [Bug 221979] Web Inspector: Implement Grid Overlay "Show area names" drawing : [Attachment 420526] Patch v1.0
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 16 13:28:16 PST 2021
Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 221979: Web Inspector: Implement Grid Overlay "Show area names" drawing
https://bugs.webkit.org/show_bug.cgi?id=221979
Attachment 420526: Patch v1.0
https://bugs.webkit.org/attachment.cgi?id=420526&action=review
--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 420526
--> https://bugs.webkit.org/attachment.cgi?id=420526
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
More information about the webkit-reviews
mailing list