Bug 89913 – [chromium] Add WebLayer API for scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Add WebLayer API for scrolling

[James Robinson]

Created attachment 149362 [details]
Patch

[James Robinson]

Created attachment 149572 [details]
Patch

[James Robinson]

How do you feel about this?  I'm anticipating needing somewhere to register a scrolling delegate (or client) to receive scrolling updates from the compositor thread in the near future.

[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.

[Adrienne Walker]

Comment on attachment 149572 [details]
Patch

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

> Source/Platform/chromium/public/WebLayer.h:141
> +    WEBKIT_EXPORT void setAlwaysReserveTextures(bool);
> +

I'm hoping this can go away with prioritized textures, but I guess we need it for now.

> Source/Platform/chromium/public/WebScrollableLayer.h:47
> +    WEBKIT_EXPORT void setScrollable(bool);

Does this need to be exposed? If we have a separate scrollable layer type, can we just assume that it's scrollable and mark it as such internally?

[James Robinson]

(In reply to comment #5)
> > Source/Platform/chromium/public/WebScrollableLayer.h:47
> > +    WEBKIT_EXPORT void setScrollable(bool);
> 
> Does this need to be exposed? If we have a separate scrollable layer type, can we just assume that it's scrollable and mark it as such internally?

In this patch WebContentLayer derives from WebScrollableLayer, so without a way to mark a layer as scrollable/not scrollable all content layers are assumed scrollable.

[Adrienne Walker]

(In reply to comment #5)
> (From update of attachment 149572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149572&action=review
> 
> > Source/Platform/chromium/public/WebScrollableLayer.h:47
> > +    WEBKIT_EXPORT void setScrollable(bool);
> 
> Does this need to be exposed? If we have a separate scrollable layer type, can we just assume that it's scrollable and mark it as such internally?

Oh, right.

[Antoine Labour]

Comment on attachment 149572 [details]
Patch

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

> Source/Platform/chromium/public/WebLayer.h:140
> +    WEBKIT_EXPORT void setAlwaysReserveTextures(bool);

Does this need to be on WebLayer? It wouldn't mean anything for WebExternalTextureLayer for example.

[James Robinson]

It doesn't make much sense in general - it something that we want to remove soon.  It's used to do a crazy thing where the code walks through part of the layer tree and calls this function on every drawable layer to try to make sure root layer scrollbars don't get evicted under memory pressure.  While the call would make more sense on WebContentsLayer (or perhaps WebScrollbarLayer if/when that exists) since it's doing this wacky tree traversal in terms of WebLayer I'd have to do some sort of cast to get to this function.

[James Robinson]

Or to put it another way, the API is somewhat poo but I would prefer to preserve the existing behavior and delete the call when we fix up texture reservation in the compositor to not need this.  I don't want to block wrapping this bit in API on fixing up the compositor's texture reservation stuff.

[Antoine Labour]

Ok, maybe worth a comment noting that it is deprecated then.

[James Robinson]

Marked deprecated in the commit that adds it FTW.

[James Robinson]

Committed r121284: <http://trac.webkit.org/changeset/121284>

[Antoine Labour]

(In reply to comment #12)
> Marked deprecated in the commit that adds it FTW.

If that isn't the Google way, I don't know what is.