Pipe App Highlight scrolling through UI Process in preparation for Note Pip avoidance.
Created attachment 433450 [details] Patch
Comment on attachment 433450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433450&action=review > Source/WebCore/ChangeLog:30 > + Pipe App Highlight scrolling through UI Process in preparation for Note Pip avoidance. double changelog > Source/WebKit/ChangeLog:45 > + we need to pipe the scrolling through the UI process. This patch does that work and again > Source/WebCore/editing/Editor.h:124 > OverrideSmoothScrollFeatureEnablment = 1 << 5, Maybe fix the spelling of "enablment"? while you're here > Source/WebCore/editing/FrameSelection.h:127 > + OverrideSmoothScrollFeatureEnablement = 1 << 10, Oh dear, it's right in this one > Source/WebCore/rendering/RenderLayer.cpp:2481 > + page().chrome().scrollRectIntoView(snappedIntRect(absoluteRect)); Wonder if we want to change the name of the chrome/client method to be more clear that it's only about the main frame
Created attachment 433471 [details] Patch
Comment on attachment 433471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433471&action=review > Source/WebCore/ChangeLog:8 > + In order to correctly avoid the note pip that can potentially obscure the web view, I feel like pip should be PIP or have another name because it's not Picture in Picture. Overlay? Popover? > Source/WebCore/page/ChromeClient.h:231 > + virtual void scrollRectInMainFrameIntoView(const IntRect&) const { }; // Currently only Mac has a non empty implementation. scrollMainFrameToRevealRect ? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1420 > +void WebProcess::scrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin) Might warn about origin being unused.
Comment on attachment 433471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433471&action=review >> Source/WebCore/page/ChromeClient.h:231 >> + virtual void scrollRectInMainFrameIntoView(const IntRect&) const { }; // Currently only Mac has a non empty implementation. > > scrollMainFrameToRevealRect ? See Tim's comment about trying to make the name more clear. your '?' makes me think that hasn't happened.
Comment on attachment 433471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433471&action=review > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:717 > + process().send(Messages::WebProcess::ScrollToRect(targetRect, origin), 0); Is there a reason we're sending this to the WebProcess (and targeting the focused page) instead of the WebPage corresponding to the WebPageProxy?
Created attachment 433473 [details] Patch
(In reply to Megan Gardner from comment #5) > Comment on attachment 433471 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433471&action=review > > >> Source/WebCore/page/ChromeClient.h:231 > >> + virtual void scrollRectInMainFrameIntoView(const IntRect&) const { }; // Currently only Mac has a non empty implementation. > > > > scrollMainFrameToRevealRect ? > > See Tim's comment about trying to make the name more clear. your '?' makes > me think that hasn't happened. Pretty sure Simon was suggesting an alternative even better name :)
Created attachment 433478 [details] Patch
Created attachment 433479 [details] Patch
Created attachment 433487 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 433487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433487&action=review > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:100 > +() Extra "()" here. > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:101 > +void PageClientImpl::requestScrollToRect(WebCore::FloatPoint&, WebCore::FloatPoint&) The first argument should be `WebCore::FloatRect`.
Created attachment 433488 [details] Patch
Created attachment 433490 [details] Patch
Created attachment 433491 [details] Patch
Created attachment 433512 [details] Patch
Comment on attachment 433512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433512&action=review > Source/WebCore/ChangeLog:39 > +2021-07-13 Megan Gardner <megan_gardner@apple.com> Still duplicate changelogs > Source/WebKit/UIProcess/WebPageProxy.h:912 > + void requestScrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin); > + void scrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin); Very unclear why this is PLATFORM(COCOA) > Source/WebKit/UIProcess/WebPageProxy.messages.in:515 > + RequestScrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin) Very unclear why this is PLATFORM(COCOA) > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:136 > - void scrollRectIntoView(const WebCore::IntRect&) const final; // Currently only Mac has a non empty implementation. > + void scrollMainFrameToRevealRect(const WebCore::IntRect&) const final; // Currently only Mac has a non empty implementation. I assume the comment is a lie now?
Comment on attachment 433512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433512&action=review > Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:100 > +void PageClientImpl::requestScrollToRect(WebCore::FloatRect&, WebCore::FloatPoint&) It looks like the arguments are (FloatRect, FloatPoint) in the declaration, but (FloatRect&, FloatPoint&) here in the implementation.
Created attachment 433518 [details] Patch
Created attachment 433525 [details] Patch
Comment on attachment 433525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433525&action=review > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:710 > +void WebPageProxy::requestScrollToRect(FloatRect targetRect, FloatPoint origin) How come we're not passing these as `const FloatRect&`? > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:715 > +void WebPageProxy::scrollToRect(FloatRect targetRect, FloatPoint origin) ditto. > Source/WebKit/UIProcess/ios/PageClientImplIOS.h:218 > + void requestScrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin) override; ditto. > Source/WebKit/WebProcess/WebPage/WebPage.h:1137 > + void scrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin); ditto. > Source/WebKit/WebProcess/WebProcess.h:548 > + void scrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin); ditto.
Created attachment 433529 [details] Patch
Created attachment 433534 [details] Patch
Created attachment 433536 [details] Patch
Created attachment 433540 [details] Patch
Comment on attachment 433540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433540&action=review > Source/WebCore/page/ChromeClient.h:231 > - virtual void scrollRectIntoView(const IntRect&) const { }; // Currently only Mac has a non empty implementation. > + virtual void scrollMainFrameToRevealRect(const IntRect&) const { }; // Currently only Mac has a non empty implementation. See earlier comment about this comment maybe being a lie now (unless I am wrong?) Honestly though I would just remove it. > Source/WebKit/UIProcess/WebPageProxy.cpp:10738 > +void WebPageProxy::requestScrollToRect(FloatRect targetRect, FloatPoint origin) I think you missed a review comment from Chris here. > Source/WebKit/UIProcess/WebPageProxy.cpp:10745 > + process().send(Messages::WebPage::ScrollToRect(targetRect, origin), 0); You can say this just like this: send(Messages::WebPage::ScrollToRect(targetRect, origin)); > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:136 > - void scrollRectIntoView(const WebCore::IntRect&) const final; // Currently only Mac has a non empty implementation. > + void scrollMainFrameToRevealRect(const WebCore::IntRect&) const final; // Currently only Mac has a non empty implementation. See earlier comment about this comment maybe being a lie now (unless I am wrong?) Honestly though I would just remove it. > Source/WebKit/WebProcess/WebProcess.h:548 > + void scrollToRect(WebCore::FloatRect targetRect, WebCore::FloatPoint origin); I think this is stale and needs deleting
Created attachment 433553 [details] Patch
Comment on attachment 433553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433553&action=review > Source/WebKit/UIProcess/ios/PageClientImplIOS.h:218 > + void requestScrollToRect(const WebCore::FloatRect& targetRect, const WebCore::FloatPoint& origin) override; Not sure why this is clinging to the comment below
Comment on attachment 433553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433553&action=review > Source/WebCore/rendering/RenderLayer.cpp:2524 > - page().chrome().scrollRectIntoView(snappedIntRect(absoluteRect)); > + page().chrome().scrollMainFrameToRevealRect(snappedIntRect(absoluteRect)); I think this Is the problem; this existing code called `scrollRectIntoView`, which actually only scrolled scrollers ABOVE the main frame, right? Maybe this one needs to be a different method that still has no implementation for WebKit2 (this is def. a behavior change). This is why good names are important, and the old name wasn't great? Maybe split into `scrollMainFrameToRevealRect` like you did, and `scrollContainingScrollViewsToRevealRect` for this one? Best check with smfr though, and do some debugging.
Created attachment 433635 [details] Patch
Created attachment 433636 [details] Patch
Created attachment 433679 [details] Patch for landing
Committed r279988 (239731@main): <https://commits.webkit.org/239731@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433679 [details].
<rdar://problem/80691049>