[webkit-reviews] review granted: [Bug 236410] Web Inspector: [Flexbox] Show item bounds, gaps, and free space in flex overlays : [Attachment 451451] Patch v1.0

Wed Feb 9 17:45:18 PST 2022

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 236410: Web Inspector: [Flexbox] Show item bounds, gaps, and free space in
flex overlays
https://bugs.webkit.org/show_bug.cgi?id=236410

Attachment 451451: Patch v1.0

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

--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 451451
  --> https://bugs.webkit.org/attachment.cgi?id=451451
Patch v1.0

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

r=mews, awesome work!

It might be worth getting someone more familiar with the inner workings of flex
to at least take a look at the more complex bits of
`InspectorOverlay::buildFlexOverlay`.  It looked fine to me, but I'm not very
familiar with some of the more advanced uses of flex so there's likely stuff
I'm guessing about ��

> Source/WebCore/inspector/InspectorOverlay.cpp:1225
> +static void drawLayoutPattern(GraphicsContext& context, const FloatQuad&
quad, int hatchSpacing, bool flip)

NIT: Can we do `enum class Flip : bool { No, Yes }` instead?

> Source/WebCore/inspector/InspectorOverlay.cpp:1234
> +    auto correctedLineForPoints = [&](const FloatPoint& start, const
FloatPoint& end) -> FloatLine {

NIT: While I do appreciate the explicitness, is it actually necessary to have
`-> FloatLine` since the return value is always a `FloatLine`?

> Source/WebCore/inspector/InspectorOverlay.cpp:1266
> +    context.setLineDash({ 1, (double)density }, 1);

Why not make `density` a `double` to begin with?

> Source/WebCore/inspector/InspectorOverlay.cpp:1272
> +static void drawLayoutHatching(GraphicsContext& context, const FloatQuad&
quad, bool flip = false)

ditto (:1225)

> Source/WebCore/inspector/InspectorOverlay.cpp:1279
> +    constexpr auto hatchSpacing = 12;

NIT: I realize that this is the parameter name, but could we maybe be slightly
more descriptive?  Maybe `defaultLayoutHatchSpacing` or something?

> Source/WebCore/inspector/InspectorOverlay.cpp:2021
> +

oops?

> Source/WebCore/inspector/InspectorOverlay.cpp:2024
> +    for (auto bounds : flexHighlightOverlay.itemBounds)

`const auto& bounds`

> Source/WebCore/inspector/InspectorOverlay.cpp:2027
> +    for (auto mainAxisGap : flexHighlightOverlay.mainAxisGaps) {

ditto (:2024)

> Source/WebCore/inspector/InspectorOverlay.cpp:2037
> +	   for (auto mainAxisSpaceBetweenItemAndGap :
flexHighlightOverlay.mainAxisSpaceBetweenItemsAndGaps)

ditto (:2024)

> Source/WebCore/inspector/InspectorOverlay.cpp:2042
> +    for (auto crossAxisGap : flexHighlightOverlay.crossAxisGaps) {

ditto (:2024)

> Source/WebCore/inspector/InspectorOverlay.cpp:2048
> +    constexpr auto spaceBetweenItemsAndCrossAxisSpaceDensity = 6;

I see "space" twice :P

> Source/WebCore/inspector/InspectorOverlay.cpp:2049
> +    for (auto crossAxisSpaceBetweenItemAndGap :
flexHighlightOverlay.spaceBetweenItemsAndCrossAxisSpace)

ditto (:2024)

> Source/WebCore/inspector/InspectorOverlay.cpp:2084
> +    auto isFlippedBlocksWritingMode =
computedStyle->isFlippedBlocksWritingMode();

While I do think that what you've done is good and fine, I wonder if maybe we
could use some of the existing helper methods that do this (e.g. `makeTextFlow`
and `mapLogicalSideToPhysicalSide`)?

> Source/WebCore/inspector/InspectorOverlay.cpp:2085
> +    auto isRightToLeftDirection = (computedStyle->direction() ==
TextDirection::RTL);

Style: unnecessary `(` `)`

> Source/WebCore/inspector/InspectorOverlay.cpp:2087
> +    auto isRowDirection = wasRowDirection ^
!computedStyle->isHorizontalWritingMode();

zomg it's xor ��

> Source/WebCore/inspector/InspectorOverlay.cpp:2100
> +    auto childQuadToRootQuad = [&](const FloatQuad& quad) -> FloatQuad {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2105
> +	       localPointToRootPoint(containingView,
renderFlex.localToContainerPoint(quad.p4(), nullptr))

Style: missing `,`

> Source/WebCore/inspector/InspectorOverlay.cpp:2109
> +    auto correctedMainAxisLeadingEdge = [&](const LayoutRect& rect) -> float
{

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2115
> +    auto correctedMainAxisTrailingEdge = [&](const LayoutRect& rect) ->
float {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2121
> +    auto correctedCrossAxisLeadingEdge = [&](const LayoutRect& rect) ->
float {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2127
> +    auto correctedCrossAxisTrailingEdge = [&](const LayoutRect& rect) ->
float {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2133
> +    auto correctedCrossAxisMin = [&](float a, float b) -> float {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2137
> +    auto correctedCrossAxisMax = [&](float a, float b) -> float {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2141
> +    auto correctedPoint = [&](float mainAxisLocation, float
crossAxisLocation) -> FloatPoint {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2145
> +    auto populateHighlightForGapOrSpace = [&](float fromMainAxisEdge, float
toMainAxisEdge, float fromCrossAxisEdge, float toCrossAxisEdge,
Vector<FloatQuad>& gapsSet) -> void {

ditto (:1234)

> Source/WebCore/inspector/InspectorOverlay.cpp:2150
> +	       correctedPoint(fromMainAxisEdge, toCrossAxisEdge)

ditto (:2105)

> Source/WebCore/inspector/InspectorOverlay.cpp:2169
> +    Vector<LayoutRect> currentLineChildrenRects = { };

i don't think the `= { }` is needed

> Source/WebCore/inspector/InspectorOverlay.cpp:2199
> +	   for (auto& childRect : currentLineChildrenRects) {

`const`?

> Source/WebCore/rendering/RenderBox.h:264
> +    LayoutBoxExtent marginBox() const { return m_marginBox; }

Should this be `const LayoutBoxExtent&`?

> Source/WebCore/rendering/RenderFlexibleBox.h:43
> +    enum class GapType { BetweenLines, BetweenItems };

NIT: I'd move this down closer to where it's used

> Source/WebCore/rendering/RenderFlexibleBox.h:93
> +    Vector<size_t> indexesOfItemsAtLineStart() { return
m_indexesOfItemsAtLineStart; }

```
const Vector<size_t>& indexesOfItemsAtLineStart() const { return
m_indexesOfItemsAtLineStart; }
```

Also, instead of directly exposing the member's value, maybe we have a `bool
isItemAtLineStart(size_t itemIndex) const`?

> Source/WebCore/rendering/RenderFlexibleBox.h:228
> +    Vector<size_t> m_indexesOfItemsAtLineStart;

Is there any way we can do this lazily?