Bug 236013 – Web Inspector: [Flexbox] Add support for showing/hiding flex container overlays and basic overlay drawing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

Implement support for the most basic of unstyled overlay for flex containers, and turning on/off said overlay via the inspector protocol. For grid this was done in https://bugs.webkit.org/show_bug.cgi?id=221062.

We should also make sure that the drawing path for flex overlays works the same way as it does for other grid overlays, with support for making the trip to the UIProcess on iOS.

[Razvan Caliman]

<rdar://87893201>

[Razvan Caliman]

Created attachment 450649 [details]
WIP 1.0

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Nikita Vasilyev]

Once this builds successfully, I can add all necessary methods to OverlayManager.js to handle Flexbox (as a part of bug 235924, or separately, I don't really care). Just letting you know so you don't work on the same thing.

[Razvan Caliman]

(In reply to Nikita Vasilyev from comment #4)
> Once this builds successfully, I can add all necessary methods to
> OverlayManager.js to handle Flexbox (as a part of bug 235924, or separately,
> I don't really care). Just letting you know so you don't work on the same
> thing.

I already started adding the necessary methods to `OverlayManager.js` in the patch at Bug 235647 (patch not yet uploaded). That bug is blocked on this one.

[Razvan Caliman]

Created attachment 450798 [details]
WIP 1.1

[Razvan Caliman]

Created attachment 451095 [details]
WIP 1.2

[Razvan Caliman]

Created attachment 451110 [details]
Patch 1.0

Add new commands to show/hide flex overlays;
Add new test to exercise the new commands: inspector/dom/showFlexOverlay.html
Add basic drawing code to show an outline around the bounding box of the flex container;

[Patrick Angle]

Comment on attachment 451110 [details]
Patch 1.0

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

Nice!

> Source/WebCore/inspector/InspectorOverlay.cpp:655
> +    RenderObject* renderer = node.renderer();
> +    if (!is<RenderFlexibleBox>(renderer))

Nit: We can inline `renderer` here. e.g. `if (!is<RenderFlexibleBox>(node.renderer()))`

> Source/WebCore/inspector/InspectorOverlay.cpp:1551
> +void InspectorOverlay::drawFlexOverlay(GraphicsContext& context, const InspectorOverlay::Highlight::FlexHighlightOverlay& flexHighlightOverlay)

Nit: Can we put this just above `InspectorOverlay::buildFlexOverlay` to make the relationship between the two easier to spot. The only reason the `::drawGridOverlay` isn't immediately before `::buildGridOverlay` is that static methods below that are used to help build the grid overlay.

> Source/WebCore/inspector/InspectorOverlay.cpp:2036
> +    flexHighlightOverlay.flexContainerQuad = localQuadToRootQuad({ renderFlex.absoluteBoundingBoxRectIgnoringTransforms() });

In my work on top of this patch I've had to change this from using `renderFlex.absoluteBoundingBoxRectIgnoringTransforms()` to using `renderFlex.absoluteContentQuad()` which I suggest you go ahead and do here to reduce code churn between our patches.

> Source/WebCore/inspector/InspectorOverlay.h:148
> +            FloatQuad flexContainerQuad;

Nit: I'd just call this `containerBounds`. The only field in the `GridHighlightOverlay` that contains the word grid is `gridLines` because `lines` is not descriptive enough to tell what those lines represent. And there is no need for `Quad` here, since the type already tells us that.

> LayoutTests/inspector/dom/showFlexOverlay.html:183
> +        <div class="item">A</div>
> +        <div class="item">B</div>
> +        <div class="item">C</div>

Is it necessary for there to be text in these divs, or does the test work fine without it? If it works fine without it, I'd suggest removing the text so that the expectation file is cleaner.

> LayoutTests/inspector/dom/showFlexOverlay.html:189
> +        <div class="item">A</div>
> +        <div class="item">B</div>
> +        <div class="item">C</div>

ditto :181-183

> LayoutTests/inspector/dom/showGridOverlay.html:-75
> -            // No error should occur if showing grid overlay for a node that already has one.

This should still be true? Re-showing a visible overlay is how colors are changed, for example.

> LayoutTests/inspector/dom/showGridOverlay.html:-103
> -            // No error should occur if showing grid overlay for a node that already has one.

ditto :75

[Razvan Caliman]

Created attachment 451132 [details]
Patch 1.1

Address code review:
- simplified test
- renamed flexContainerQuad to containerBounds
- replaced renderFlex.absoluteBoundingBoxRectIgnoringTransforms() with renderFlex.absoluteBoundingBoxRectIgnoringTransforms()
- addressed nits

[Razvan Caliman]

Comment on attachment 451110 [details]
Patch 1.0

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:655
>> +    if (!is<RenderFlexibleBox>(renderer))
> 
> Nit: We can inline `renderer` here. e.g. `if (!is<RenderFlexibleBox>(node.renderer()))`

Done

>> Source/WebCore/inspector/InspectorOverlay.cpp:1551
>> +void InspectorOverlay::drawFlexOverlay(GraphicsContext& context, const InspectorOverlay::Highlight::FlexHighlightOverlay& flexHighlightOverlay)
> 
> Nit: Can we put this just above `InspectorOverlay::buildFlexOverlay` to make the relationship between the two easier to spot. The only reason the `::drawGridOverlay` isn't immediately before `::buildGridOverlay` is that static methods below that are used to help build the grid overlay.

Make sense. Done

>> Source/WebCore/inspector/InspectorOverlay.cpp:2036
>> +    flexHighlightOverlay.flexContainerQuad = localQuadToRootQuad({ renderFlex.absoluteBoundingBoxRectIgnoringTransforms() });
> 
> In my work on top of this patch I've had to change this from using `renderFlex.absoluteBoundingBoxRectIgnoringTransforms()` to using `renderFlex.absoluteContentQuad()` which I suggest you go ahead and do here to reduce code churn between our patches.

Brilliant! Done.

>> Source/WebCore/inspector/InspectorOverlay.h:148
>> +            FloatQuad flexContainerQuad;
> 
> Nit: I'd just call this `containerBounds`. The only field in the `GridHighlightOverlay` that contains the word grid is `gridLines` because `lines` is not descriptive enough to tell what those lines represent. And there is no need for `Quad` here, since the type already tells us that.

Done

>> LayoutTests/inspector/dom/showFlexOverlay.html:183
>> +        <div class="item">C</div>
> 
> Is it necessary for there to be text in these divs, or does the test work fine without it? If it works fine without it, I'd suggest removing the text so that the expectation file is cleaner.

The test just checks the number of show overlays for flex containers. The flex items are not necessary. Removed.

>> LayoutTests/inspector/dom/showFlexOverlay.html:189
>> +        <div class="item">C</div>
> 
> ditto :181-183

Done.

>> LayoutTests/inspector/dom/showGridOverlay.html:-75
>> -            // No error should occur if showing grid overlay for a node that already has one.
> 
> This should still be true? Re-showing a visible overlay is how colors are changed, for example.

This was likely a copy-paste leftover in the original patch for CSS grid.

The comment does not reflect the context. The code below toggles the overlay for the _second_ grid container which doesn't have one already. It has nothing to do with repeat calls for the same node.

>> LayoutTests/inspector/dom/showGridOverlay.html:-103
>> -            // No error should occur if showing grid overlay for a node that already has one.
> 
> ditto :75

ditto explanation :75

[Patrick Angle]

Comment on attachment 451132 [details]
Patch 1.1

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

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

Nit: We can actually do `auto localQuadToRootQuad = [&](const FloatQuad& quad) -> FloatQuad {` to avoid an unnecessary copy since we don't modify the passed quad.

> LayoutTests/inspector/dom/showGridOverlay.html:-75
> -            // No error should occur if showing grid overlay for a node that already has one.

Can you add a note to the changelog that removing these two comments are drive-by fixes?

[Razvan Caliman]

Created attachment 451249 [details]
Patch 1.2

Address code review nits

[Razvan Caliman]

Comment on attachment 451132 [details]
Patch 1.1

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:2024
>> +    auto localQuadToRootQuad = [&](FloatQuad quad) -> FloatQuad {
> 
> Nit: We can actually do `auto localQuadToRootQuad = [&](const FloatQuad& quad) -> FloatQuad {` to avoid an unnecessary copy since we don't modify the passed quad.

Done

>> LayoutTests/inspector/dom/showGridOverlay.html:-75
>> -            // No error should occur if showing grid overlay for a node that already has one.
> 
> Can you add a note to the changelog that removing these two comments are drive-by fixes?

Done

[Patrick Angle]

Comment on attachment 451249 [details]
Patch 1.2

rs=me Nice work!

[EWS]

Committed r289416 (246979@main): <https://commits.webkit.org/246979@main>

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