Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[masonry] Expose grid items positions and sizes to Web Inspector #22941

Merged
merged 1 commit into from
Jan 19, 2024
Merged

[masonry] Expose grid items positions and sizes to Web Inspector #22941

merged 1 commit into from
Jan 19, 2024

Conversation

stwrt
Copy link
Member

@stwrt stwrt commented Jan 18, 2024 •
edited by webkit-early-warning-system
Loading

3816458

[masonry] Expose grid items positions and sizes to Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=267736
rdar://problem/121219933

Reviewed by Sammy Gill.

Masonry differs from Grid in only having one axis. This does not allow simple drawing
straight lines. The Web Inspector can instead draw boxes around each item, which this patch
is exposing in a simpler format.

This patch is not exclusive to masonry and can be used for all returning all grid items
layout rects.

* Source/WebCore/rendering/RenderGrid.cpp:
(WebCore::RenderGrid::gridItemsLayoutRects):
* Source/WebCore/rendering/RenderGrid.h:

Canonical link: https://commits.webkit.org/273232@main

f7419da

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@stwrt stwrt self-assigned this Jan 18, 2024
@stwrt stwrt added the Web Inspector Bugs related to the WebKit Web Inspector. label Jan 18, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 18, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 3e8749c)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ⏳ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ⏳ 🧪 mac-wk1 ⏳ 🛠 gtk
⏳ 🛠 🧪 jsc ⏳ 🧪 api-ios ⏳ 🧪 mac-wk2 ⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 api-gtk
⏳ 🛠 tv-sim
⏳ 🛠 watch
⏳ 🛠 watch-sim

@stwrt
Copy link
Member Author

stwrt commented Jan 18, 2024 •
edited
Loading

Simple way to grab data on the Inspector Overlay side:

auto items = renderGrid.gridItemsLayoutRects();
for (auto &item : items) {
    std::cout << "x " << item.x().rawValue() << std::endl;
    std::cout << "y " << item.y().rawValue() << std::endl;
    std::cout << "width " << item.width().rawValue() << std::endl;
    std::cout << "height " << item.height().rawValue() << std::endl;
    std::cout << std::endl;
}

@stwrt stwrt requested a review from sammygill January 18, 2024 23:33
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for f7419da. Live statuses available at the PR page, #22941

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jan 18, 2024 •
edited
Loading

EWS run on current version of this PR (hash f7419da)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

rcaliman-apple
rcaliman-apple approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@rcaliman-apple rcaliman-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very neat!

These metrics will be helpful in

WebKit/Source/WebCore/inspector/InspectorOverlay.cpp

Line 1494 in 8c2f912

std::optional<InspectorOverlay::Highlight::GridHighlightOverlay> InspectorOverlay::buildGridOverlay(const InspectorOverlay::Grid& gridOverlay, bool offsetBoundsByScroll)

to follow-up on Bug 265381 by improving the drawing for items in a masonry layout.

Thank you!

@stwrt stwrt added merge-queue Applied to send a pull request to merge-queue and removed merge-queue Applied to send a pull request to merge-queue labels Jan 19, 2024
sammygill
sammygill approved these changes Jan 19, 2024
View reviewed changes
Source/WebCore/rendering/RenderGrid.h
@@ -128,6 +128,7 @@ class RenderGrid final : public RenderBlock {
LayoutUnit gridGap(GridTrackSizingDirection, std::optional<LayoutUnit> availableSize) const;

LayoutUnit masonryContentSize() const;
Vector<LayoutRect> gridItemsLayoutRects();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a const function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discuss with Sammy.

This will require changing the orderiterator to support const better.

Source/WebCore/rendering/RenderGrid.cpp
@@ -1022,6 +1022,16 @@ LayoutUnit RenderGrid::masonryContentSize() const
return m_masonryLayout.gridContentSize();
}

Vector<LayoutRect> RenderGrid::gridItemsLayoutRects()
{
Vector<LayoutRect> items;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert here to make sure the caller does not call this function on a grid that needs to go through layout? Otherwise they would be getting stale geometry for the renderers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with Sammy about the assert.

This is somewhat more of a discussion who owns the responsibility of ensuring that the data is not stale.

Will handle this in a future patch.

@stwrt stwrt added the merge-queue Applied to send a pull request to merge-queue label Jan 19, 2024
@stwrt
[masonry] Expose grid items positions and sizes to Web Inspector
3816458 
https://bugs.webkit.org/show_bug.cgi?id=267736
rdar://problem/121219933

Reviewed by Sammy Gill.

Masonry differs from Grid in only having one axis. This does not allow simple drawing
straight lines. The Web Inspector can instead draw boxes around each item, which this patch
is exposing in a simpler format.

This patch is not exclusive to masonry and can be used for all returning all grid items
layout rects.

* Source/WebCore/rendering/RenderGrid.cpp:
(WebCore::RenderGrid::gridItemsLayoutRects):
* Source/WebCore/rendering/RenderGrid.h:

Canonical link: https://commits.webkit.org/273232@main
@webkit-commit-queue webkit-commit-queue merged commit 3816458 into WebKit:main Jan 19, 2024
@webkit-commit-queue
Copy link
Collaborator

Committed 273232@main (3816458): https://commits.webkit.org/273232@main

Reviewed commits have been landed. Closing PR #22941 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 19, 2024
@stwrt stwrt deleted the masonry_web_inspector branch February 28, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcaliman-apple rcaliman-apple rcaliman-apple approved these changes

@sammygill sammygill sammygill approved these changes

Assignees

@stwrt stwrt

Labels
Web Inspector Bugs related to the WebKit Web Inspector.
Projects
None yet
Milestone
No milestone
5 participants
@stwrt @webkit-early-warning-system @webkit-commit-queue @rcaliman-apple @sammygill