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

[scroll-anchoring] Add release logging for scroll anchoring adjustments #22629

Merged
merged 1 commit into from
Jan 12, 2024
Merged

[scroll-anchoring] Add release logging for scroll anchoring adjustments #22629

merged 1 commit into from
Jan 12, 2024

Conversation

nmoucht
Copy link
Contributor

@nmoucht nmoucht commented Jan 11, 2024 •
edited by webkit-early-warning-system
Loading

235f339

[scroll-anchoring] Add release logging for scroll anchoring adjustments
https://bugs.webkit.org/show_bug.cgi?id=267379
rdar://120812771

Reviewed by Wenson Hsieh.

Add release logs for scroll anchoring adjusmtents for easier diagnosing of scroll
anchoring bugs.

* Source/WebCore/page/scrolling/ScrollAnchoringController.cpp:
(WebCore::ScrollAnchoringController::adjustScrollPositionForAnchoring):

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

5e3a11d

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
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@nmoucht nmoucht requested a review from cdumez as a code owner January 11, 2024 00:11
@nmoucht nmoucht self-assigned this Jan 11, 2024
@nmoucht nmoucht added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Jan 11, 2024
@nmoucht nmoucht force-pushed the eng/scroll-anchoring-Add-release-logging-for-scroll-anchoring-adjustments branch 2 times, most recently from 18f1543 to e6676c9 Compare January 11, 2024 00:55
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash d7a119f)

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
⏳ 🛠 tv   🧪 mac-AS-debug-wk2   🧪 api-gtk
⏳ 🛠 tv-sim   🧪 mac-wk2-stress
⏳ 🛠 watch
  🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 18f1543)

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   🧪 mac-wk2-stress
⏳ 🛠 watch
  🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash e6676c9)

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
✅ 🛠 watch
✅ 🛠 watch-sim

whsieh
whsieh reviewed Jan 11, 2024
View reviewed changes
Source/WebCore/page/scrolling/ScrollAnchoringController.cpp Outdated
@@ -369,7 +369,8 @@ void ScrollAnchoringController::adjustScrollPositionForAnchoring()
}
#endif
auto newScrollPosition = m_owningScrollableArea.scrollPosition() + IntPoint(adjustment.width(), adjustment.height());
LOG_WITH_STREAM(ScrollAnchoring, stream << "ScrollAnchoringController::updateScrollPosition() for frame: " << frameView() << " for scroller: " << m_owningScrollableArea << " adjusting from: " << m_owningScrollableArea.scrollPosition() << " to: " << newScrollPosition);
ALWAYS_LOG_WITH_STREAM(stream << "ScrollAnchoringController::updateScrollPosition() is main frame: " << frameView().frame().isMainFrame() << " for scroller: " << m_owningScrollableArea << " adjusting from: " << m_owningScrollableArea.scrollPosition() << " to: " << newScrollPosition);
Copy link
Member

Choose a reason for hiding this comment

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

(It would also be good to double check that m_owningScrollableArea doesn't contain any sensitive information, as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering changing this to log the element of the scrollable area as well as the anchor element. Should we be concerned about logging the element (as it would show generally where the user was looking on the page) or are we just worried about logging the url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the elements could also have revealing class names as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, class and id names (while mostly innocuous) could potentially reveal what websites the user was visiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right I'll split those logs into debug logs and keep just the mainframe/main scroller and scroll position logging for release.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! rs=me

@nmoucht nmoucht force-pushed the eng/scroll-anchoring-Add-release-logging-for-scroll-anchoring-adjustments branch from e6676c9 to 5e3a11d Compare January 11, 2024 21:06
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on current version of this PR (hash 5e3a11d)

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

@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on current version of this PR (hash 5e3a11d)

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
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@nmoucht nmoucht added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 12, 2024
@nmoucht
[scroll-anchoring] Add release logging for scroll anchoring adjustments
235f339 
https://bugs.webkit.org/show_bug.cgi?id=267379
rdar://120812771

Reviewed by Wenson Hsieh.

Add release logs for scroll anchoring adjusmtents for easier diagnosing of scroll
anchoring bugs.

* Source/WebCore/page/scrolling/ScrollAnchoringController.cpp:
(WebCore::ScrollAnchoringController::adjustScrollPositionForAnchoring):

Canonical link: https://commits.webkit.org/272985@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/scroll-anchoring-Add-release-logging-for-scroll-anchoring-adjustments branch from 5e3a11d to 235f339 Compare January 12, 2024 19:22
@webkit-commit-queue
Copy link
Collaborator

Committed 272985@main (235f339): https://commits.webkit.org/272985@main

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

@webkit-commit-queue webkit-commit-queue merged commit 235f339 into WebKit:main Jan 12, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whsieh whsieh whsieh approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@nmoucht nmoucht

Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nmoucht @webkit-early-warning-system @webkit-commit-queue @whsieh