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.
<rdar://87893201>
Created attachment 450649 [details] WIP 1.0
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
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.
(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.
Created attachment 450798 [details] WIP 1.1
Created attachment 451095 [details] WIP 1.2
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;
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
Created attachment 451132 [details] Patch 1.1 Address code review: - simplified test - renamed flexContainerQuad to containerBounds - replaced renderFlex.absoluteBoundingBoxRectIgnoringTransforms() with renderFlex.absoluteBoundingBoxRectIgnoringTransforms() - addressed nits
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
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?
Created attachment 451249 [details] Patch 1.2 Address code review nits
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
Comment on attachment 451249 [details] Patch 1.2 rs=me Nice work!
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].