<rdar://87893185>
Created attachment 451451 [details] Patch v1.0
Created attachment 451452 [details] Screenshot of Patch v1.0
Comment on attachment 451451 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=451451&action=review > Source/WebCore/inspector/InspectorOverlay.h:312 > + encoder << mainAxisGaps Oops
Comment on attachment 451451 [details] 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?
Created attachment 451593 [details] Patch v1.1 - Address review nits, only collect line starts when Inspector is attached
Comment on attachment 451451 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=451451&action=review >> 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`? I'll comment here about all of these. It isn't necessary for almost all of them currently. There were a few below that were using list initializers that need the type defined somewhere. For consistency I've updated those rarer cases to use explicit initializers and dropped the return types across the board. >> 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`)? IMO that would make this harder to follow. We really only care if blocks are flipped for the current writing mode for the stuff below, and `isFlippedBlocksWritingMode` is already a convenience method that ends up making a text flow and checking if that text flow is flipped. >> Source/WebCore/rendering/RenderFlexibleBox.h:228 >> + Vector<size_t> m_indexesOfItemsAtLineStart; > > Is there any way we can do this lazily? Yes.
Created attachment 451595 [details] Patch v1.2 - Fix double/float issue on watchOS
Comment on attachment 451595 [details] Patch v1.2 - Fix double/float issue on watchOS View in context: https://bugs.webkit.org/attachment.cgi?id=451595&action=review > Source/WebCore/inspector/InspectorOverlay.cpp:598 > + m_flexibleBoxRendererCachedItemsAtStartOfLine.set(renderer, Vector<size_t> { 0 }); NIT: Is the `Vector<size_t>` necessary? Or can it just be `{ 0 }`? > Source/WebCore/inspector/InspectorOverlay.cpp:607 > + auto cachedItemsAtStartOfLine = m_flexibleBoxRendererCachedItemsAtStartOfLine.take(renderer); > + cachedItemsAtStartOfLine.append(lineStartItemIndex); > + m_flexibleBoxRendererCachedItemsAtStartOfLine.set(renderer, WTFMove(cachedItemsAtStartOfLine)); Why do we remove from `m_flexibleBoxRendererCachedItemsAtStartOfLine` and then re-add it? Can we just `m_flexibleBoxRendererCachedItemsAtStartOfLine.get(renderer).append(lineStarItemIndex);` instead? > Source/WebCore/inspector/InspectorOverlay.cpp:1271 > - > + the curious case of the extra whitespace :P > Source/WebCore/inspector/InspectorOverlay.h:70 > + enum class Flip : bool { No, Yes }; This is only used in the `.cpp`, right? I'd move it to somewhere at the top of the file (or right above the first usage) instead of having it be part of `InspectorOverlay`. > Source/WebCore/inspector/InspectorOverlay.h:307 > + WeakHashMap<RenderObject, Vector<size_t>> m_flexibleBoxRendererCachedItemsAtStartOfLine; It's a bit odd that this is on `InspectorOverlay` instead of something like `InspectorCSSAgent`/`InspectorDOMAgent` 🤔 > Source/WebCore/rendering/RenderFlexibleBox.cpp:1156 > + InspectorInstrumentation::flexibleBoxRendererBeganLayout(*this); Do we force a layout when Web Inspector opens (or maybe just when the overlay is drawn) so that we ensure this happens before we attempt to draw the flex overlay?
Comment on attachment 451595 [details] Patch v1.2 - Fix double/float issue on watchOS View in context: https://bugs.webkit.org/attachment.cgi?id=451595&action=review >> Source/WebCore/inspector/InspectorOverlay.h:307 >> + WeakHashMap<RenderObject, Vector<size_t>> m_flexibleBoxRendererCachedItemsAtStartOfLine; > > It's a bit odd that this is on `InspectorOverlay` instead of something like `InspectorCSSAgent`/`InspectorDOMAgent` 🤔 Yeah. Combined with my comment below I think I'm going to move this to InspectorDOMAgent (since it controls the overlay). And when enabled make sure we are also doing an explicit re-layout. >> Source/WebCore/rendering/RenderFlexibleBox.cpp:1156 >> + InspectorInstrumentation::flexibleBoxRendererBeganLayout(*this); > > Do we force a layout when Web Inspector opens (or maybe just when the overlay is drawn) so that we ensure this happens before we attempt to draw the flex overlay? Heh. So I double checked. We do... kinda. Right now setEmulatedMedia triggers a re-layout when we attach an inspector. I'll make that a purposeful thing though so we aren't relying on that side effect.
Created attachment 451631 [details] Patch v1.3 - Further refinement to flex line collection strategy, resolve Windows build issue
Created attachment 451712 [details] Patch v1.3 (reupload for EWS)
Committed r289698 (247183@main): <https://commits.webkit.org/247183@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451712 [details].