Bug 32080 – Make it possible to keep track of scrolls

• Status: NEW
• Component: Layout and Rendering

[Kenneth Rohde Christiansen]

Created attachment 44169 [details]
Make it possible to keep track of scrolls

Some platforms may need to keep track of scrolls even if it requires full repaint of the viewport. For instance, if we have a tiled backing store, we may repaint the whole viewport, but we still need to move the tiles by the given offset.

The implemented approach was discussed with David Hyatt on IRC.

[Simon Fraser (smfr)]

Comment on attachment 44169 [details]
Make it possible to keep track of scrolls

When is canBlitOnScroll() going to return true, but scroll() return false?

I don't really like how this patch adds a second way that 'can blit' is affected. I also don't like seeing a boolean parameter that is hard to read at the call site.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> (From update of attachment 44169 [details])
> When is canBlitOnScroll() going to return true, but scroll() return false?

the return bool says whether you handled it or not. Some platforms might want to always repaint, and thus ignore the scroll.

But for me it is ok to remove this part and not let it return anything.
 
> I don't really like how this patch adds a second way that 'can blit' is
> affected.

Sorry I didn't get you here

>I also don't like seeing a boolean parameter that is hard to read at
> the call site.

referring to the return value again?

[Darin Adler]

Hyatt, are you going to review this?

[WebKit Review Bot]

Attachment 44169 [details] did not pass chromium-ews:
Full output: http://webkit-commit-queue.appspot.com/results/127496

[Adam Barth]

Comment on attachment 44169 [details]
Make it possible to keep track of scrolls

Looks like you missed the Chromium one.  Would you be willing to post an updated version of the patch that builds on that port?

[Adam Treat]

The stated reason for the patch is so that a client can be notified of a scroll even if the scroll caused the repaint of the entire window.  If that is the case, then I don't see why you are adding a return value here.  The bool return value would indicate that some clients wish to override the repaint of the entire window when canBlitOnScroll == false.  However, the patch does not implement this.

[Adam Treat]

Comment on attachment 44169 [details]
Make it possible to keep track of scrolls

> This patch makes sure that there is no behaviour change for Haiku.
...
> +bool ChromeClientHaiku::scroll(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect, bool canBlit)
>  {
>      notImplemented();
> +    return true;

There is a behavior change here.  Haiku previously would have repainted the entire viewport for a slow scroll.  Now it won't since it returns true indicating it 'canBlit'.

> This patch makes sure that there is no behaviour change for Mac.
...
> -void WebChromeClient::scroll(const IntSize&, const IntRect&, const IntRect&)
> +bool WebChromeClient::scroll(const IntSize&, const IntRect&, const IntRect&, bool)
>  {
> +    return true;
>  }

There is a behavior change here.  Mac previously would have repainted the entire viewport for a slow scroll.  Now it won't since it returns true indicating it 'canBlit'.

You don't need to add a bool return value to scroll IMHO.  I'd prefer you add a new 'slowScroll' notification to the various ChromeClient's instead of doing it this way.

r- for the behavior changes.

[Gustavo Sverzut Barbieri]

The bool return was something David Hyatt insisted to have minimum changes with maximum flexibility.

As for those "return true;", I guess Kenneth did a mistake, they all should return the last received boolean parameter, that is "return canBlit;" to get the old behavior, I did this in my first patch, but I did just the essential to my platform and Kenneth helped by doing all the others.

[Rafael Antognolli]

(In reply to comment #7)
> (From update of attachment 44169 [details])
> > This patch makes sure that there is no behaviour change for Haiku.
> ...
> > +bool ChromeClientHaiku::scroll(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect, bool canBlit)
> >  {
> >      notImplemented();
> > +    return true;
> 
> There is a behavior change here.  Haiku previously would have repainted the
> entire viewport for a slow scroll.  Now it won't since it returns true
> indicating it 'canBlit'.
> 
> > This patch makes sure that there is no behaviour change for Mac.
> ...
> > -void WebChromeClient::scroll(const IntSize&, const IntRect&, const IntRect&)
> > +bool WebChromeClient::scroll(const IntSize&, const IntRect&, const IntRect&, bool)
> >  {
> > +    return true;
> >  }
> 
> There is a behavior change here.  Mac previously would have repainted the
> entire viewport for a slow scroll.  Now it won't since it returns true
> indicating it 'canBlit'.
> 
> You don't need to add a bool return value to scroll IMHO.  I'd prefer you add a
> new 'slowScroll' notification to the various ChromeClient's instead of doing it
> this way.

So would this new 'slowScroll' substitute the call to hostWindow()->repaint() when canBlitOnScroll() is false?

[Adam Treat]

I think this bug fixes this issue: https://bugs.webkit.org/show_bug.cgi?id=34214

Just need time to update it...

[Antonio Gomes]

(In reply to comment #10)
> I think this bug fixes this issue: https://bugs.webkit.org/show_bug.cgi?id=34214
> 
> Just need time to update it...

Could you please confirm that? What do still miss in this bug?

[Rafael Antognolli]

(In reply to comment #11)
> (In reply to comment #10)
> > I think this bug fixes this issue: https://bugs.webkit.org/show_bug.cgi?id=34214
> > 
> > Just need time to update it...
> 
> Could you please confirm that? What do still miss in this bug?

I don't know if the Qt port still needs this, but at least for us (EFL port) this is not completely solved by these changes. But we can just reopen this bug (or create a new one if necessary) later, when the code that actually uses this is sent upstream.