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.
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.
(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?
Hyatt, are you going to review this?
Attachment 44169 [details] did not pass chromium-ews: Full output: http://webkit-commit-queue.appspot.com/results/127496
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?
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.
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.
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.
(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?
I think this bug fixes this issue: https://bugs.webkit.org/show_bug.cgi?id=34214 Just need time to update it...
(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?
(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.