Bug 45773 – Add isMainFrame flag to scroll functions

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

[João Paulo Rechi Vita]

[EFL] Add isMainFrame flag to scroll functions

[João Paulo Rechi Vita]

Created attachment 67592 [details]
Patch

[João Paulo Rechi Vita]

This patch adds a parameter to scroll functions to tell if we're scrolling the main frame or an inner-frame. This information is needed by the new EFL tiled backing-store, submitted upstream on #45397.

[Kenneth Rohde Christiansen]

Comment on attachment 67592 [details]
Patch

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

I don't really understand why this is needed, and as such I cannot say if this is the best way of accomplishing what you are trying to do.

r- for this reason.

> WebCore/ChangeLog:8
> +        This information is needed by the new EFL tiled backing store.

This would probably need more explanation. Why is that needed for the EFL tiled backing store. - you should explain that instead

> WebCore/loader/EmptyClients.h:151
> +    virtual void invalidateContentsForSlowScroll(const IntSize&, const IntRect&, bool, bool) {};
> +    virtual void scroll(const IntSize&, const IntRect&, const IntRect&, bool) { }

We are trying to more away from bool arguments. Consider using enums instead.

[João Paulo Rechi Vita]

(In reply to comment #3)
> (From update of attachment 67592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67592&action=review
> 
> I don't really understand why this is needed, and as such I cannot say if this is the best way of accomplishing what you are trying to do.
> 
> r- for this reason.
> 
> > WebCore/ChangeLog:8
> > +        This information is needed by the new EFL tiled backing store.
> 
> This would probably need more explanation. Why is that needed for the EFL tiled backing store. - you should explain that instead
> 

When when the scroll is on the main frame we want to simply scroll the backing store instead of doing repaints. To accomplish that we need to know whether a scroll is on the main frame or on a inner frame when processing the scroll. Although this is not necessary with frame flattening, we want to support frames as well.

If you have any suggestion on other ways to do this I'll be happy to hear.

> > WebCore/loader/EmptyClients.h:151
> > +    virtual void invalidateContentsForSlowScroll(const IntSize&, const IntRect&, bool, bool) {};
> > +    virtual void scroll(const IntSize&, const IntRect&, const IntRect&, bool) { }
> 
> We are trying to more away from bool arguments. Consider using enums instead.

I'll have to send an updated version of the patch anyway, because we found a small bug in this modifications (that's why this bug has been left for so long). I'll take this into consideration when sending the new version. But could please share your thoughts on the overall approach before I do so?

Thanks.

[Antonio Gomes]

Is it still needed?

[Antonio Gomes]

(In reply to comment #5)
> Is it still needed?

Seems not.