Bug 227914 – Pipe App Highlight scrolling through UI Process in preparation for Note Overlay avoidance.

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Pipe App Highlight scrolling through UI Process in preparation for Note Pip avoidance.

[Megan Gardner]

Created attachment 433450 [details]
Patch

[Tim Horton]

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

[Megan Gardner]

Created attachment 433471 [details]
Patch

[Simon Fraser (smfr)]

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.

[Megan Gardner]

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.

[Wenson Hsieh]

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?

[Megan Gardner]

Created attachment 433473 [details]
Patch

[Tim Horton]

(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 :)

[Megan Gardner]

Created attachment 433478 [details]
Patch

[Megan Gardner]

Created attachment 433479 [details]
Patch

[Megan Gardner]

Created attachment 433487 [details]
Patch

[EWS Watchlist]

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

[Wenson Hsieh]

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`.

[Megan Gardner]

Created attachment 433488 [details]
Patch

[Megan Gardner]

Created attachment 433490 [details]
Patch

[Megan Gardner]

Created attachment 433491 [details]
Patch

[Megan Gardner]

Created attachment 433512 [details]
Patch

[Tim Horton]

Comment on attachment 433512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433512&action=review

> Source/WebCore/ChangeLog:39
> [phone]  Megan Gardner  <[e-mail]>

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?

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 433518 [details]
Patch

[Megan Gardner]

Created attachment 433525 [details]
Patch

[Chris Dumez]

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.

[Megan Gardner]

Created attachment 433529 [details]
Patch

[Megan Gardner]

Created attachment 433534 [details]
Patch

[Megan Gardner]

Created attachment 433536 [details]
Patch

[Megan Gardner]

Created attachment 433540 [details]
Patch

[Tim Horton]

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

[Megan Gardner]

Created attachment 433553 [details]
Patch

[Tim Horton]

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

[Tim Horton]

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.

[Megan Gardner]

Created attachment 433635 [details]
Patch

[Megan Gardner]

Created attachment 433636 [details]
Patch

[Megan Gardner]

Created attachment 433679 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/80691049>