Bug 89913

Summary: [chromium] Add WebLayer API for scrolling
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, dglazkov, enne, fishd, piman, tkent+wkapi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch enne: review+

James Robinson
Reported 2012-06-25 14:41:18 PDT
[chromium] Add WebLayer API for scrolling
Attachments
Patch (24.81 KB, patch)
2012-06-25 14:43 PDT, James Robinson
no flags
Patch (24.31 KB, patch)
2012-06-26 11:31 PDT, James Robinson
enne: review+
James Robinson
Comment 1 2012-06-25 14:43:54 PDT
James Robinson
Comment 2 2012-06-26 11:31:39 PDT
James Robinson
Comment 3 2012-06-26 11:33:11 PDT
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
Comment 4 2012-06-26 11:33:52 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Adrienne Walker
Comment 5 2012-06-26 12:04:38 PDT
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
Comment 6 2012-06-26 12:23:54 PDT
(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
Comment 7 2012-06-26 12:25:33 PDT
(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 8 2012-06-26 12:38:34 PDT
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
Comment 9 2012-06-26 12:48:45 PDT
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
Comment 10 2012-06-26 12:54:21 PDT
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
Comment 11 2012-06-26 12:55:30 PDT
Ok, maybe worth a comment noting that it is deprecated then.
James Robinson
Comment 12 2012-06-26 13:11:00 PDT
Marked deprecated in the commit that adds it FTW.
James Robinson
Comment 13 2012-06-26 13:17:25 PDT
Antoine Labour
Comment 14 2012-06-26 13:50:24 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.