Bug 45750 – Move backing-store contents on slow scrolls

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[João Paulo Rechi Vita]

[EFL] Move backing-store contents on slow scrolls

[João Paulo Rechi Vita]

Created attachment 67547 [details]
Patch

[Early Warning System Bot]

Attachment 67547 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4001005

[João Paulo Rechi Vita]

Created attachment 67551 [details]
Patch

[João Paulo Rechi Vita]

In order to make slow inner-frame scrolls faster, we want to use ewk_view_scroll() [0] to handle slow scrolls. It basically adds the scroll request to a scroll-requests array that is latter processed by _ewk_view_single_scroll_process_single() [1], which analyzes the scroll direction and offset (using the information provided through scrollDelta) and move the backing-store contents that are still going to be visible in the scrolled direction, so we're left with a smaller area to repaint.

[0] WebKit/efl/ewk/ewk_view.cpp
[1] WebKit/efl/ewk/ewk_view_single.c

[Kenneth Rohde Christiansen]

Comment on attachment 67551 [details]
Patch

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

> WebCore/ChangeLog:9
> +        With this information is possible to scroll the backing-store contents
> +        without redrawing the whole scrolled area.

Without this change we can acomplish this for Qt. You are also introducing behaviour changes elsewhere and there are no tests, thus r-.

> WebCore/page/ChromeClient.h:138
> +        virtual void invalidateContentsForSlowScroll(const IntSize&, const IntRect&, bool) = 0;

it is not obvious what the size here is for so I would write out the variable name... is it for a delta? The original method is not much better though, I wonder what the bool is for!

> WebCore/platform/ScrollView.cpp:544
> -       hostWindow()->invalidateContentsForSlowScroll(updateRect, false);
> +       hostWindow()->invalidateContentsForSlowScroll(-scrollDelta, updateRect, false);

There seems to be behaviour changes here, where are the layout tests?

[Kenneth Rohde Christiansen]

Ah, maybe it is not a behaviour change and it is an intrusive change to the API so we need to find out if this is really needed or not.

You should explain your case a bit better and I would suggest discussing it with dhyatt on irc.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> In order to make slow inner-frame scrolls faster, we want to use ewk_view_scroll() [0] to handle slow scrolls. It basically adds the scroll request to a scroll-requests array that is latter processed by _ewk_view_single_scroll_process_single() [1], which analyzes the scroll direction and offset (using the information provided through scrollDelta) and move the backing-store contents that are still going to be visible in the scrolled direction, so we're left with a smaller area to repaint.
> 
> [0] WebKit/efl/ewk/ewk_view.cpp
> [1] WebKit/efl/ewk/ewk_view_single.c

Nice comment, why is this not in the ChangeLog???

[João Paulo Rechi Vita]

(In reply to comment #5)
> (From update of attachment 67551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67551&action=review
> 
> > WebCore/ChangeLog:9
> > +        With this information is possible to scroll the backing-store contents
> > +        without redrawing the whole scrolled area.
> 
> Without this change we can acomplish this for Qt. You are also introducing behaviour changes elsewhere and there are no tests, thus r-.
> 

I can't imagine how you can move the pixels without having the direction and delta information, but from your latter comment I think you have misunderstood what I'm using the information for due to my bad explanation on the ChangeLog. But if you can really do that, please point me to where you do so on the Qt port.

> > WebCore/page/ChromeClient.h:138
> > +        virtual void invalidateContentsForSlowScroll(const IntSize&, const IntRect&, bool) = 0;
> 
> it is not obvious what the size here is for so I would write out the variable name... is it for a delta? The original method is not much better though, I wonder what the bool is for!
> 

Yes, it's for a delta. I've just followed the WebCore policy of putting only the types on the header files.

> > WebCore/platform/ScrollView.cpp:544
> > -       hostWindow()->invalidateContentsForSlowScroll(updateRect, false);
> > +       hostWindow()->invalidateContentsForSlowScroll(-scrollDelta, updateRect, false);
> 
> There seems to be behaviour changes here, where are the layout tests?

There are no behavior changes, just the additional parameter is passed on, as you also stated on comment #6. I've updated this function declaration/implementation on the other ports but the parameter is ignored. Only the EFL port uses this information so far.

I'll send a updated version with a better description on the ChangeLog (sorry, wasn't sure about how much info I should put there) and also CC dhyatt on this bug so he can let us know his opinion.

[Kenneth Rohde Christiansen]

Yes, I understand now that this is for the iframes. I still wonder why you are not following every other webkit mobile browser and using frame flattening instead... If this is for mobile, it is good that we all follow suit.

[João Paulo Rechi Vita]

Created attachment 69382 [details]
Patch

[João Paulo Rechi Vita]

(In reply to comment #9)
> Yes, I understand now that this is for the iframes. I still wonder why you are not following every other webkit mobile browser and using frame flattening instead... If this is for mobile, it is good that we all follow suit.

We already support frame-flattening, but prefer we leave to the browser the decision to flatten the frames or not.

[Sam Weinig]

Does EFL have a separate backing store for each subframe?

[Antonio Gomes]

> 
> > > WebCore/page/ChromeClient.h:138
> > > +        virtual void invalidateContentsForSlowScroll(const IntSize&, const IntRect&, bool) = 0;
> > 
> > it is not obvious what the size here is for so I would write out the variable name... is it for a delta? The original method is not much better though, I wonder what the bool is for!
> >
> Yes, it's for a delta. I've just followed the WebCore policy of putting only the types on the header files.

In WebCore, we do not name variable in the header when they do not add info.

For example, in "IntSize size" , the "size" is useless, and we omit it.

In your bool case, no one who reads the header will have a clue of what the bool is about, so we name it.

[João Paulo Rechi Vita]

(In reply to comment #12)
> Does EFL have a separate backing store for each subframe?

No, we treat sub-frames as normal smaller regions of the backing-store.

[João Paulo Rechi Vita]

(In reply to comment #13)
> > 
> > > > WebCore/page/ChromeClient.h:138
> > > > +        virtual void invalidateContentsForSlowScroll(const IntSize&, const IntRect&, bool) = 0;
> > > 
> > > it is not obvious what the size here is for so I would write out the variable name... is it for a delta? The original method is not much better though, I wonder what the bool is for!
> > >
> > Yes, it's for a delta. I've just followed the WebCore policy of putting only the types on the header files.
> 
> In WebCore, we do not name variable in the header when they do not add info.
> 
> For example, in "IntSize size" , the "size" is useless, and we omit it.
> 
> In your bool case, no one who reads the header will have a clue of what the bool is about, so we name it.

The bool is called 'immediate', was already there and it also present on the other invalidate* functions. I guess it's there to tell if the invalidate should be processed immediately, but I'm not 100% sure. On EFL port we ignore its value (with this patch).

The patch introduces the IntSize parameter, so according to Antonio's comments it is in conformance with WebCore's coding style.

[Dave Hyatt]

Created attachment 70551 [details]
Test case demonstrating why this patch is wrong.

[Dave Hyatt]

Comment on attachment 69382 [details]
Patch

We discussed this some on IRC, but basically if the slow scroll method is called it specifically means that WebCore has detected that it is not safe to just move the pixels.  The test case did actually work still with this patch, and that baffles me (it makes me think there may be some HostWindow mis-implementation leading to a lucky extra repaint).  The fact that you need this change makes me think maybe your canBlitOnScroll state is corrupted or not set up properly.

I'd love to see an actual test case that gets faster with this patch.  That would be really helpful for understanding what problem this patch is trying to solve.

[João Paulo Rechi Vita]

(In reply to comment #17)
> (From update of attachment 69382 [details])
> We discussed this some on IRC, but basically if the slow scroll method is called it specifically means that WebCore has detected that it is not safe to just move the pixels.  The test case did actually work still with this patch, and that baffles me (it makes me think there may be some HostWindow mis-implementation leading to a lucky extra repaint).  The fact that you need this change makes me think maybe your canBlitOnScroll state is corrupted or not set up properly.
> 
> I'd love to see an actual test case that gets faster with this patch.  That would be really helpful for understanding what problem this patch is trying to solve.

The test case actually fails, I've tested with a version without this patch I had compiled in my machine, sorry for the confusion. We're gonna investigate a bit more with the help of Dave's explanations on IRC whether we really need this or not.

[Rafael Antognolli]

The proposed solution doesn't work, so it's better to just close the bug.