Bug 107027 – [Chromium] Notify embedder of programmatic scrolls.

• Component: New Bugs
• Status: RESOLVED WONTFIX

[John Knottenbelt]

Chromium] Route the Javascript scroll event to the layer tree.

[John Knottenbelt]

Created attachment 182999 [details]
Patch

[John Knottenbelt]

This change depends on https://bugs.webkit.org/show_bug.cgi?id=107026
and is needed by https://codereview.chromium.org/1196701

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 182999 [details]
Patch

Attachment 182999 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15913370

[Peter Beverloo (cr-android ews)]

Comment on attachment 182999 [details]
Patch

Attachment 182999 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15913378

[James Robinson]

Comment on attachment 182999 [details]
Patch

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

> Source/Platform/chromium/public/WebLayerTreeView.h:132
> +    // JavaScript has requested a scroll. Used to implement the window.scrollTo(0,1) API to
> +    // hide the location bar.
> +    virtual void scrollFromJavaScript(const WebPoint& scrollPoint) = 0;

This doesn't really have anything to do with WebLayerTreeView or compositing.  If you want a notification out to the embedder that a certain type of scroll has happened, route it through the WebView or WebWidget family.  You can then hook it up to whatever you want on the embedder side (aka chromium code)

[John Knottenbelt]

(In reply to comment #6)
> (From update of attachment 182999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182999&action=review
> 
> > Source/Platform/chromium/public/WebLayerTreeView.h:132
> > +    // JavaScript has requested a scroll. Used to implement the window.scrollTo(0,1) API to
> > +    // hide the location bar.
> > +    virtual void scrollFromJavaScript(const WebPoint& scrollPoint) = 0;
> 
> This doesn't really have anything to do with WebLayerTreeView or compositing.  If you want a notification out to the embedder that a certain type of scroll has happened, route it through the WebView or WebWidget family.  You can then hook it up to whatever you want on the embedder side (aka chromium code)

Thanks for reviewing. I've been looking into doing this by adding the method to the WebWidgetClient interface, which then gets implemented by RenderViewImpl on the chromium side. What I need to do is to plumb through to the TopControlsManager, which is owned by the cc::LayerTreeHostImpl. 

The difficulty I am having is that I can't see how to get at the cc::LayerTreeHostImpl, as WebWidgetClient::layerTreeHost() is not implemented by Chromium's RenderView, and simply returns NULL. Indeed, it is the WebViewImpl itself that creates and owns the WebLayerTreeView object.

Does this make sense? Any suggestions on how to proceed would be much appreciated.

[James Robinson]

From the point of view of the WebKit API, how the top bar controls positioning maps to the compositor or even if it maps to the compositor at all are considerations of the embedder (the system on the WebWidgetClient/WebViewClient side of the interface boundary, typically chromium), not of WebKit.  Any code that's aware of the internals of top bar positioning should be on the chromium side.  In particular, I don't expect this code to live in the compositor embedding in the render process in chromium for very long.

On the chromium side, http://src.chromium.org/viewvc/chrome?view=rev&revision=178256 should give you access to everything you need.

[John Knottenbelt]

Created attachment 184515 [details]
Patch

[John Knottenbelt]

Created attachment 191249 [details]
Patch

[James Robinson]

Comment on attachment 191249 [details]
Patch

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

Where are your tests?

> Source/WebKit/chromium/src/WebViewImpl.cpp:4046
> +    // We want to hide the URL bar on ordinary load types (not back-forward
> +    // navigation / page reload) that are not originating from the compositor.
> +    if (loader->loadType() == FrameLoadTypeStandard && !m_scrollFromCompositor) {

You seem to be conflating scrolls from the compositor with user-initiated scrolls, but that's simply not the case.  The compositor will only handle user-initiated scrolls but in several cases we fall back to the main thread to handle the scroll itself.  How do those behave with this code?

[Alexandre Elias]

I think this kind of boolean suppressing URL-bar hiding is probably the way to go given the desired behavior mentioned on the bug, but there are several other cases to cover:

1. Navigation
2. Restoring scroll offset from HistoryItem
3. Slow-path scrolling

For (1), checking whether a load is in progress may work.  For (2), the load-in-progress might cover it, and if that's not enough can put an explicit guard around HistoryController restore.  For (3), you should be able to guard it in WebViewImpl::handleInputEvent.

[John Knottenbelt]

Comment on attachment 191249 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:4046
>> +    if (loader->loadType() == FrameLoadTypeStandard && !m_scrollFromCompositor) {
> 
> You seem to be conflating scrolls from the compositor with user-initiated scrolls, but that's simply not the case.  The compositor will only handle user-initiated scrolls but in several cases we fall back to the main thread to handle the scroll itself.  How do those behave with this code?

Very good point. In that case this code doesn't work correctly.

I'm thinking that FrameView::inProgrammaticScroll() may help here. 

If https://bugs.webkit.org/show_bug.cgi?id=109712 can be solved, then this code would read instead:

if (loader->loadType() == FrameLoadTypeStandard && frameView->inProgrammaticScroll()) {
   ...
}

otherwise, we need to check for compositor scrolls as well:

if (loader->loadType() == FrameLoadTypeStandard && frameView->inProgrammaticScroll() && !m_scrollFromCompositor) {
   ...
}

Do you think that this points to https://bugs.webkit.org/show_bug.cgi?id=109712 should be addressed first?

[James Robinson]

(In reply to comment #13)
> Do you think that this points to https://bugs.webkit.org/show_bug.cgi?id=109712 should be addressed first?

I think so

[John Knottenbelt]

Created attachment 192948 [details]
Patch

[John Knottenbelt]

This patch depends on https://bugs.webkit.org/show_bug.cgi?id=109712

In this patch I have hooked in at the ChromeClient level so that I can capture both fast and slow scrolls. 

Do you have any suggestions on where I should try to add test code? Would WebViewTest.cpp or WebFrameTest.cpp be suitable?

(In reply to comment #15)
> Created an attachment (id=192948) [details]
> Patch

[James Robinson]

(In reply to comment #12)
> I think this kind of boolean suppressing URL-bar hiding is probably the way to go given the desired behavior mentioned on the bug, but there are several other cases to cover:
> 
> 1. Navigation
> 2. Restoring scroll offset from HistoryItem
> 3. Slow-path scrolling
> 
> For (1), checking whether a load is in progress may work.  For (2), the load-in-progress might cover it, and if that's not enough can put an explicit guard around HistoryController restore.  For (3), you should be able to guard it in WebViewImpl::handleInputEvent.

Has this been addressed?  I don't have a great knowledge of how navigations work.

[John Knottenbelt]

(In reply to comment #17)
> (In reply to comment #12)
> > I think this kind of boolean suppressing URL-bar hiding is probably the way to go given the desired behavior mentioned on the bug, but there are several other cases to cover:
> > 
> > 1. Navigation
> > 2. Restoring scroll offset from HistoryItem
> > 3. Slow-path scrolling
> > 
> > For (1), checking whether a load is in progress may work.  For (2), the load-in-progress might cover it, and if that's not enough can put an explicit guard around HistoryController restore.  For (3), you should be able to guard it in WebViewImpl::handleInputEvent.
> 
> Has this been addressed?  I don't have a great knowledge of how navigations work.

I'm working on this at the moment, hopefully have something for you today.

[John Knottenbelt]

Created attachment 193353 [details]
Patch

[John Knottenbelt]

(In reply to comment #19)
> Created an attachment (id=193353) [details]
> Patch

This adds tests for the above. Comments appreciated.

[James Robinson]

Comment on attachment 193353 [details]
Patch

I don't get the changes to WebViewTest.cpp.  Are there supposed to be test changes there?

[John Knottenbelt]

Created attachment 193615 [details]
Patch

[John Knottenbelt]

(In reply to comment #21)
> (From update of attachment 193353 [details])
> I don't get the changes to WebViewTest.cpp.  Are there supposed to be test changes there?

No, they were accidental. I've cleaned it up now.

[Alexandre Elias]

Comment on attachment 193615 [details]
Patch

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:514
> +    m_webView->hideTopControlsOnScroll();

I don't think this does what you intend, as slow scrolls are normally for sublayers.  Have you seen a case where this is needed?  If not I suggest deleting it.

[John Knottenbelt]

Comment on attachment 193615 [details]
Patch

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

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:514
>> +    m_webView->hideTopControlsOnScroll();
> 
> I don't think this does what you intend, as slow scrolls are normally for sublayers.  Have you seen a case where this is needed?  If not I suggest deleting it.

I do think we need to hook in here as http://bing.com and http://jquerymobile.com/test both use this API to scroll. bing.com also triggers the fast path scroll later on.

I've attached a stack trace is below. ScrollView::scrollContents is calling FrameView::scrollContentsSlowPath because ScrollView::canBlitOnScroll() is returning false.

#0  WebKit::ChromeClientImpl::invalidateContentsForSlowScroll (this=0x71c7e9d0, updateRect=..., 
    immediate=false) at ../../third_party/WebKit/Source/WebKit/chromium/src/ChromeClientImpl.cpp:512
#1  0x752e3ee8 in WebCore::Chrome::invalidateContentsForSlowScroll (this=<optimized out>, 
    updateRect=<optimized out>, immediate=<optimized out>)
    at ../../third_party/WebKit/Source/WebCore/page/Chrome.cpp:94
#2  0x74949afe in WebCore::ScrollView::scrollContentsSlowPath (this=<optimized out>, updateRect=...)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:704
#3  0x753042d0 in WebCore::FrameView::scrollContentsSlowPath (this=0x727e5758, updateRect=...)
    at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1687
#4  0x7494a8ec in WebCore::ScrollView::scrollContents (this=0x727e5758, scrollDelta=...)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:683
#5  0x7494a94e in WebCore::ScrollView::scrollTo (this=0x727e5758, newOffset=<optimized out>)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:388
#6  0x75303cb8 in WebCore::FrameView::scrollTo (this=0x727e5758, newOffset=...)
    at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:2905
#7  0x7494a0c0 in WebCore::ScrollView::setScrollOffset (this=0x727e5758, offset=<optimized out>)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:368
#8  0x7494bf34 in WebCore::ScrollableArea::scrollPositionChanged (this=0x727e5790, position=...)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollableArea.cpp:154
#9  0x74948922 in WebCore::ScrollAnimator::notifyPositionChanged (this=<optimized out>, 
    delta=<optimized out>) at ../../third_party/WebKit/Source/WebCore/platform/ScrollAnimator.cpp:147
#10 0x749488fa in WebCore::ScrollAnimator::scrollToOffsetWithoutAnimation (this=<optimized out>, 
    offset=<optimized out>) at ../../third_party/WebKit/Source/WebCore/platform/ScrollAnimator.cpp:81
#11 0x7494c098 in WebCore::ScrollableArea::scrollToOffsetWithoutAnimation (this=<optimized out>, offset=...)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollableArea.cpp:129
#12 0x75c73d22 in updateScrollbars (desiredOffset=..., this=0x727e5758)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:623
#13 WebCore::ScrollView::updateScrollbars (this=0x727e5758, desiredOffset=...)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:468
#14 0x7494af6e in setScrollPosition (scrollPoint=<optimized out>, this=0x727e5758)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:423
#15 WebCore::ScrollView::setScrollPosition (this=0x727e5758, scrollPoint=<optimized out>)
    at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:401
#16 0x7530495c in WebCore::FrameView::setScrollPosition (this=0x727e5758, scrollPoint=...)
    at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1847
#17 0x752f0ac8 in WebCore::DOMWindow::scrollTo (this=<optimized out>, x=<optimized out>, y=1)
    at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:1438
#18 0x754a8ce8 in scrollToMethod (args=<optimized out>) at gen/webcore/bindings/V8DOMWindow.cpp:4126
#19 WebCore::DOMWindowV8Internal::scrollToMethodCallback (args=<optimized out>)

[John Knottenbelt]

Created attachment 193875 [details]
Patch

[John Knottenbelt]

Created attachment 193884 [details]
Patch

[John Knottenbelt]

Comment on attachment 193884 [details]
Patch

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

> Source/WebKit/chromium/tests/HideTopControlsTest.cpp:145
> +    gesture.data.scrollUpdate.deltaY = 40;

I've added this test to check the handleInputEvent for user scrolls that bounced from the compositor. Does this look good to you?

[Alexandre Elias]

Comment on attachment 193875 [details]
Patch

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

I feel this is taking an overly indirect approach because we're afraid of making changes to WebCore.  How about adding a method hideTopControls to ChromeClient and call it from WebCore?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:514
> +    m_webView->hideTopControlsOnScroll();

I'm pretty sure this will get called for subframes as well if they slow scroll.  To be more robust, we can either call ChromeClient::hideTopControls() from FrameView (checking first if it's the mainFrame), or we could ensure setCanBlitScroll is always false if compositing mode is active.

> Source/WebKit/chromium/src/WebViewImpl.cpp:4297
> +    if (loader->loadType() != FrameLoadTypeStandard && frameView->scrollPosition() != zeroOne)

Could we just call it from HistoryController instead of using this trick?  I'm not sure why this works and I don't have much trust this would be robust.

[John Knottenbelt]

Comment on attachment 193875 [details]
Patch

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

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:514
>> +    m_webView->hideTopControlsOnScroll();
> 
> I'm pretty sure this will get called for subframes as well if they slow scroll.  To be more robust, we can either call ChromeClient::hideTopControls() from FrameView (checking first if it's the mainFrame), or we could ensure setCanBlitScroll is always false if compositing mode is active.

Yes, I think that hooking in at WebCore::FrameView::setScrollPosition would be a good place, and would avoid any details of fast and slow scrolls. In https://bugs.webkit.org/show_bug.cgi?id=107026 it was suggested that a 'hideTopControls' method in Chrome should be avoided because such naming makes assumptions about app behaviour (i.e. not applicable to all ports). Instead a more general name such as didScrollTo could work.

This sounds like a reasonable way forward to me. Do you agree?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:4297
>> +    if (loader->loadType() != FrameLoadTypeStandard && frameView->scrollPosition() != zeroOne)
> 
> Could we just call it from HistoryController instead of using this trick?  I'm not sure why this works and I don't have much trust this would be robust.

HistoryController will call this method anyway because it calls Page::setPageScaleFactor which will adjust the scroll offset. My intention was to try and guess whether the URL bar should be showing or hidden based on the scroll position, but I agree with James that we should really be storing this in the HistoryItem to do that properly. Also, I agree with you that the condition above looks rather magic, so:

What I think we should do in the medium/short term is remove this suppression from here and allow logic further down (in the browser) determine whether the URL bar should be hid. The logic in the browser should ensure that the URL bar will be showing unless the page programmatically scrolls after it has finished loading. I believe that this is consistent with our intended user experience.

In the longer term, I think that we should implement saving the state of the URLBar on the HistoryItem and restore it along with the rest of the page state. I think that this is more work than can fit into our current milestone target.

[John Knottenbelt]

Created attachment 194605 [details]
Patch

[John Knottenbelt]

(In reply to comment #31)
> Created an attachment (id=194605) [details]
> Patch

This shows what I am thinking. Instead of directly hiding the top controls, the WebViewImpl now requests that the top controls be hidden. The actual decision as to whether the controls can be hidden takes place in the browser. This avoids any race conditions with the browser enabling or disabling hiding the top controls. It is also the browser's responsibility to ignore hiding the top controls during a page load, or navigation (reload / backwards / forwards).

Also hook in at FrameView::setScrollPosition as suggested by Alexandre. Should I move the WebCore changes to a separate patch, maybe https://bugs.webkit.org/show_bug.cgi?id=107026?

[Alexandre Elias]

Comment on attachment 194605 [details]
Patch

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

This looks better to me.

> Source/WebCore/page/ChromeClient.h:181
> +    virtual void didScrollBy(Frame*, const IntSize&) const { };

Do we want to call this didSetScrollPosition instead?  It would avoid the duplicative zero-delta call.  And it should have a comment saying that it should be called upon any attempt to set the scroll position, even to the same offset.

[James Robinson]

Comment on attachment 194605 [details]
Patch

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

>> Source/WebCore/page/ChromeClient.h:181
>> +    virtual void didScrollBy(Frame*, const IntSize&) const { };
> 
> Do we want to call this didSetScrollPosition instead?  It would avoid the duplicative zero-delta call.  And it should have a comment saying that it should be called upon any attempt to set the scroll position, even to the same offset.

No trailing ;

> Source/WebCore/page/FrameView.cpp:1855
> +    if (page)
> +        page->chrome()->client()->didScrollBy(m_frame.get(), scrollPosition() - oldScrollPosition);

Why two calls?

> Source/WebKit/chromium/tests/HideTopControlsTest.cpp:121
> +    frameImpl->executeScript(WebScriptSource("window.scrollTo(0, 21);"));

You should also have explicit tests somewhere for scrollBy(0, 0)

[John Knottenbelt]

Created attachment 194847 [details]
Patch

[John Knottenbelt]

Comment on attachment 194605 [details]
Patch

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

>>> Source/WebCore/page/ChromeClient.h:181
>>> +    virtual void didScrollBy(Frame*, const IntSize&) const { };
>> 
>> Do we want to call this didSetScrollPosition instead?  It would avoid the duplicative zero-delta call.  And it should have a comment saying that it should be called upon any attempt to set the scroll position, even to the same offset.
> 
> No trailing ;

Done.

>> Source/WebCore/page/FrameView.cpp:1855
>> +        page->chrome()->client()->didScrollBy(m_frame.get(), scrollPosition() - oldScrollPosition);
> 
> Why two calls?

I was thinking that it might be useful to have the actual scrolled delta in case we decide later that this is useful input to the top controls hiding logic. However, I think that we can get pretty much the same effect by putting a single callback at the beginning of this function, before any early return statements.

>> Source/WebKit/chromium/tests/HideTopControlsTest.cpp:121
>> +    frameImpl->executeScript(WebScriptSource("window.scrollTo(0, 21);"));
> 
> You should also have explicit tests somewhere for scrollBy(0, 0)

Done.

[Alexandre Elias]

LGTM.

[James Robinson]

Comment on attachment 194847 [details]
Patch

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

> Source/WebKit/chromium/tests/HideTopControlsTest.cpp:128
> +    frameImpl->executeScript(WebScriptSource("window.scrollBy(0, 0);"));
> +    EXPECT_TRUE(client.didHideTopControls());

This seems like the opposite of the behavior we're trying to implement.  No-op scrolls should be no-ops.

[Alexandre Elias]

Comment on attachment 194847 [details]
Patch

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

Sorry I LGTMed too early.  I got sidetracked by the complicated argument on the email thread and forgot the underlying issue was scrollTo(0, 0).  I agree there hasn't been a compelling reason to support that.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:578
> +    ASSERT(frame->view()->inProgrammaticScroll());

OK, since you're asserting this, the function should be named ChromeClientImpl::didProgrammaticallyScroll.

> Source/WebKit/chromium/src/WebViewImpl.cpp:4286
> +    m_client->requestShowTopControls(show);

Please rename the Chromium-side interface to didProgrammaticallyScroll as well.

> Source/WebKit/chromium/src/WebViewImpl.h:543
> +    void showTopControls(bool show);

Call this WebViewImpl::didProgrammaticallyScroll.

[John Knottenbelt]

Comment on attachment 194847 [details]
Patch

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

>> Source/WebKit/chromium/tests/HideTopControlsTest.cpp:128
>> +    EXPECT_TRUE(client.didHideTopControls());
> 
> This seems like the opposite of the behavior we're trying to implement.  No-op scrolls should be no-ops.

Requiring that the scroll offset change in order for hiding to be triggered, is problematic. Take, for example, a site that scrolls programmatically to (0,1). The first time it is loaded, it will hide the URL bar. If we then reload, WebKit will first restore the scroll position to (0,1) and the subsequent Javascript scroll to (0,1) will not cause the URL bar to be hidden because there is no change in scroll offset. There are two solutions to this:
  1) Try to guess whether the URL bar should be hidden during the restore scroll offset step. 
  2) Store the state of the URL bar in the HistoryItem, and restore the state of the URL bar at the same time that WebKit restores the scroll position and other history item properties.

Solution 1, we already discussed is not a very good. We shouldn't be trying to guess the state of the URL bar based on specific scroll offsets.

Solution 2 requires making WebCore even more aware of the concept of URL bar hiding: it requires plumbing the current state of the URL bar up into WebKit, making changes to HistoryItem in WebCore, and changes to the way the scroll offset is restored so that it is not viewed as a programmatic scroll. Given the amount of work and complexity this will introduce, I think it's worth avoiding if we can achieve a similar aim with a simpler solution.

By allowing scrolls to the same position to trigger hiding of the URL bar, we can have the URL bar showing until the URL bar programmatically scrolls. When WebKit restores the scroll position on reload/forward/backward, WebKit will still send the hide request, but the browser will be discard it because this occurs during page load. If the page then programmatically scrolls after the load event, the request to hide will be sent again and this time the browser will hide it. 

I think that making it work in this way is a reasonable first step to implementing this feature. Android Browser and Mobile Safari also hide the URL bar on scrollBy(0,0), so the behaviour, as such, is not unexpected. The scroll position (as observed by Javascript) doesn't change, so it still appears as a no-op to Javascript.

[John Knottenbelt]

Created attachment 195085 [details]
Patch

[James Robinson]

Comment on attachment 195085 [details]
Patch

We've had several extensive threads about this subject and reached an across-the-board consensus on scrollBy(0, 0).  I don't think trying to revisit it in a patch review is a very productive way to proceed, respond to those threads if you think you have new data to present that would change the conclusion.

It sounds like the problem you are trying to address has to do with maintaining proper history state, which is something that needs to be addressed anyway.

[John Knottenbelt]

Having thought about it more, I'm not convinced that maintaining the state of the URL bar in history is exactly what we should be doing, as the user will have to bring up the URL bar to do a reload, so sites that hide it on initial page load would then have their URL bar state restored to showing.

Let us proceed, however, without scrollBy(0, 0), as previously discussed. If it turns out to be a problem in practice, we can revisit this decision.

I've added the scroll offset as a parameter to didProgrammaticallyScroll. This is so that the browser can decide exactly which kinds of scroll deltas should hide the URL bar, something which is has yet to be decided.

(In reply to comment #42)
> (From update of attachment 195085 [details])
> We've had several extensive threads about this subject and reached an across-the-board consensus on scrollBy(0, 0).  I don't think trying to revisit it in a patch review is a very productive way to proceed, respond to those threads if you think you have new data to present that would change the conclusion.
> 
> It sounds like the problem you are trying to address has to do with maintaining proper history state, which is something that needs to be addressed anyway.

[John Knottenbelt]

Created attachment 195274 [details]
Patch

[James Robinson]

Comment on attachment 195274 [details]
Patch

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

R=me but comment and signature need some tweaking.

> Source/WebCore/page/ChromeClient.h:182
> +    // didProgrammaticallyScroll should be called on any attempt to set the scroll position programmatically,
> +    // even to the same offset.

This comment is wrong - we don't call (and shouldn't call) this function if the the scroll offset is the same.

> Source/WebCore/page/ChromeClient.h:183
> +    virtual void didProgrammaticallyScroll(Frame*, const IntPoint&) const { }

The second parameter's meaning is not clear from the type, so please add a name.  This is the new scroll position, right?