Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18191020:
UI Resource Manager (Closed)
|
Created:
7 years, 5 months ago by powei Modified:
7 years, 4 months ago CC:
chromium-reviews, cc-bugs_chromium.org, David Trainor- moved to gerrit, clholgat, jdduke (slow) Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionProposed UI Resource Manager. See internal doc:
https://docs.google.com/a/google.com/document/d/1Im8fR0bVsBHC2I1f2MuzW-e-h1H1F5GYH4fbdBTsbQ8/edit
BUG=259095, 173947
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214975
Patch Set 1 : #
Total comments: 16
Patch Set 2 : #
Total comments: 63
Patch Set 3 : #
Total comments: 99
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 19
Patch Set 6 : Switched to resource client instead of callback #
Total comments: 21
Patch Set 7 : #
Total comments: 38
Patch Set 8 : Fixed nits and bugs, removed cruft, added test for Create/DeleteUIResource in LTHI #
Total comments: 2
Patch Set 9 : Added DCHECK of resource queue size to PushPropertiesTo #
Messages
Total messages: 46 (0 generated)
Sorry for the delay. Please take a look. Thanks!
On 2013/06/29 07:09:47, powei wrote: > Sorry for the delay. Please take a look. Thanks! Added a scrollbarlayer class (PictureScrollbarLayer). Can be switched off using #define flag USE_REGULAR_SCROLLBAR in cc_export.h. Thanks!
Could you modify the existing ScrollbarLayer in place instead? With this plan, there's no need to introduce another layer type, and that will also make it easier to see what you changed during codereview. https://codereview.chromium.org/18191020/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1448: void ThreadProxy::CreateUIResource(UIResourceId uid, Let's avoid adding these functions by buffering until commit. You can imitate how PageScaleAnimations are handled.
No need for a #define either. Let's just make UIResources the default way of using scrollbars as soon as you land.
https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... File cc/layers/picture_scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... cc/layers/picture_scrollbar_layer.cc:281: // Is this the only way? I think you're looking for SetNeedsCommit. https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... cc/layers/picture_scrollbar_layer.cc:295: void ScrollbarLayer::UIResourceLost(UIResourceId id) { As I mentioned in the doc about this, I would really prefer that the UI resource manager handle this and not individual layer types. That way individual layers could just persist ui resource ids that wouldn't ever need to change unless the resource changed. Create resource could pass a callback to recreate these resources initially or in the future when needed. https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.cc:24: delete [] reinterpret_cast<uint8_t*>(pixels_); Just make this a scoped_array<uint8_t>. https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.cc:25: // handle other formats? You're probably going to need to handle BGRA or swizzle this elsewhere. https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:268: void UIResourceReady(UIResourceId id); What does UIResourceReady mean? Does that mean that the texture is uploaded and ready to draw? If so, what would you do in response to that notification on the main thread? https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:269: void UIResourceLost(UIResourceId id); Do you think we'll ever want to lose just one resource vs. losing them all?
https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... File cc/layers/picture_scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... cc/layers/picture_scrollbar_layer.cc:295: void ScrollbarLayer::UIResourceLost(UIResourceId id) { On 2013/07/09 17:57:48, enne wrote: > As I mentioned in the doc about this, I would really prefer that the UI resource > manager handle this and not individual layer types. > > That way individual layers could just persist ui resource ids that wouldn't ever > need to change unless the resource changed. Create resource could pass a > callback to recreate these resources initially or in the future when needed. OK, after discussing it with dtrainor@, we agree. Let's change to that approach. The only condition is that it should be allowed to reupload a resource of a different size from the original, so that a low-res version or 1x1 placeholder can be uploaded. https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:268: void UIResourceReady(UIResourceId id); On 2013/07/09 17:57:48, enne wrote: > What does UIResourceReady mean? Does that mean that the texture is uploaded and > ready to draw? If so, what would you do in response to that notification on the > main thread? Yes, it means it's uploaded and ready to draw. The main thread can respond by attaching the UIResourceID to a layer. Note that attaching to a layer before this UIResourceReady call is allowed, however, it may result in a framerate jank. dtrainor@'s Android thumbnail usecase creates large resources while animations are in progress, so this hint is useful there. https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:269: void UIResourceLost(UIResourceId id); On 2013/07/09 17:57:48, enne wrote: > Do you think we'll ever want to lose just one resource vs. losing them all? Since the only cause is lost context in the current design, we can only lose them all. (I don't see much value in CC evicting UIResources due to memory limits.) It just seemed natural API-wise to have one call per UIResourceID -- one reasonable alternative would be to provide one call per client with a vector of UIResourceIDs.
https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:268: void UIResourceReady(UIResourceId id); On 2013/07/09 20:22:19, aelias wrote: > On 2013/07/09 17:57:48, enne wrote: > > What does UIResourceReady mean? Does that mean that the texture is uploaded > and > > ready to draw? If so, what would you do in response to that notification on > the > > main thread? > > Yes, it means it's uploaded and ready to draw. > > The main thread can respond by attaching the UIResourceID to a layer. Note that > attaching to a layer before this UIResourceReady call is allowed, however, it > may result in a framerate jank. dtrainor@'s Android thumbnail usecase creates > large resources while animations are in progress, so this hint is useful there. Huh. I guess I'm just curious about what you would do differently on the main thread vs. the compositor thread. When creating quads, the compositor-side layer would still have to ask "can I get a resource for this id" and could behave differently, while the main thread side doesn't have to care about the state of the resource.
https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/20001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:268: void UIResourceReady(UIResourceId id); On 2013/07/09 20:32:52, enne wrote: > On 2013/07/09 20:22:19, aelias wrote: > > On 2013/07/09 17:57:48, enne wrote: > > > What does UIResourceReady mean? Does that mean that the texture is uploaded > > and > > > ready to draw? If so, what would you do in response to that notification on > > the > > > main thread? > > > > Yes, it means it's uploaded and ready to draw. > > > > The main thread can respond by attaching the UIResourceID to a layer. Note > that > > attaching to a layer before this UIResourceReady call is allowed, however, it > > may result in a framerate jank. dtrainor@'s Android thumbnail usecase creates > > large resources while animations are in progress, so this hint is useful > there. > > Huh. I guess I'm just curious about what you would do differently on the main > thread vs. the compositor thread. When creating quads, the compositor-side > layer would still have to ask "can I get a resource for this id" and could > behave differently, while the main thread side doesn't have to care about the > state of the resource. My thinking is that the impl-side will always block in some way until the resource is available -- because most users of UIResources never want to see a "white flash" during texture upload. Those few users -- i.e. the thumbnail system -- who do consider "white flash" acceptable can manage that themselves on the main thread using UIResourceReady.
https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... File cc/layers/picture_scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/layers/picture_scrollb... cc/layers/picture_scrollbar_layer.cc:281: // Is this the only way? On 2013/07/09 17:57:48, enne wrote: > I think you're looking for SetNeedsCommit. Done. https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.cc:24: delete [] reinterpret_cast<uint8_t*>(pixels_); On 2013/07/09 17:57:48, enne wrote: > Just make this a scoped_array<uint8_t>. Can you elaborate on this? Are we assume that pixels is always going to be uint8_t? Here I only assume that RGBA8 bitmaps are stored as uint8_t. Also, scoped_array is not found in base/memory/scoped_ptr.h, but it is found in googleurl/base/scoped_ptr.h, and that header contains an identical definition for scoped_ptr, which conflicts with base/memory/ref_counted.h. Would you point me to how this is usually resolved? https://codereview.chromium.org/18191020/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1448: void ThreadProxy::CreateUIResource(UIResourceId uid, On 2013/07/08 23:40:49, aelias wrote: > Let's avoid adding these functions by buffering until commit. You can imitate > how PageScaleAnimations are handled. Requests on the main thread can be buffered until commit, but what about notifications from the impl-side to the main thread. Can they be buffered? Or do they need to go through thread proxy?
On 2013/07/10 17:47:18, powei wrote: > https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... > cc/resources/ui_resource_manager.cc:24: delete [] > reinterpret_cast<uint8_t*>(pixels_); > On 2013/07/09 17:57:48, enne wrote: > > Just make this a scoped_array<uint8_t>. > > Also, scoped_array is not found in base/memory/scoped_ptr.h, but it is found in > googleurl/base/scoped_ptr.h, and that header contains an identical definition > for scoped_ptr, which conflicts with base/memory/ref_counted.h. Would you point > me to how this is usually resolved? > scoped_ptr<uint8[]>
Another round of review. Minor update wrt the comments. Still unclear about internal formats of the UIResourceBitmap (always uint8?). No management for ResourceLost yet.
Can there be a bug attached to this CL?
On 2013/07/10 21:57:54, danakj wrote: > Can there be a bug attached to this CL? Attached
Thanks! On Wed, Jul 10, 2013 at 3:19 PM, <powei@chromium.org> wrote: > On 2013/07/10 21:57:54, danakj wrote: > >> Can there be a bug attached to this CL? >> > > Attached > > https://codereview.chromium.**org/18191020/<https://codereview.chromium.org/1... >
Looks good structurally, here's a lot of comments about details. Two high-level comments: 1) let's get rid of the UIResourceReady() callback entirely for this patch, and punt it for later. It's meaningless until we start using the async upload system and the initial use cases don't need it. That will simplify things and you can focus more on context loss. 2) You'll need to write tests for this. Start looking at the test directories in CC to figure out how. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.cc File cc/layers/scrollbar_layer.cc (left): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:12: #include "cc/resources/caching_bitmap_content_layer_updater.h" Three more includes to delete here. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.cc File cc/layers/scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:26: nit: accidental spacing change https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:116: layer_tree_host()->settings().solid_color_scrollbar_thickness_dip; nit: accidental spacing change https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:129: nit: accidental spacing change https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:149: if (host != layer_tree_host() && host == NULL) { You changed the behavior here by changing this line, bring it back to if (!host || host != layer_tree_host()) (which also deals with LTH changes) https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:151: layer_tree_host()->DeleteUIResource(track_ui_resource_id_); Also throw out the UIResourceBitmaps here. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:205: new uint8_t[track_rect.width()*track_rect.height()*4], nit: spaces around * https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:242: new uint8_t[thumb_rect.width()*thumb_rect.height()*4], nit: spaces around * https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:278: void ScrollbarLayer::UIResourceReady(UIResourceId id) { This code is unnecessary, please delete it. It's not even possible for UIResourceReady to get called unless the UI resource was already pushed to the impl thread earlier. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (left): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:16: class ResourceUpdateQueue; Also delete this. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:20: public UIResourceManagerClient { nit: wrong spacing https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:37: virtual void Update(ResourceUpdateQueue* queue, This should be deleted. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:51: virtual void UIResourceReady(UIResourceId id) OVERRIDE; No need for this here, it should have an empty implementation in the base class. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:80: bool skip_update_; It doesn't look like this variable does anything useful, please delete it. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... File cc/layers/scrollbar_layer_impl.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:80: NoteLayerPropertyChanged(); This is never called in PushPropertiesTo anywhere else so I don't think these 2 lines should be here. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:85: bool ScrollbarLayerImpl::WillDraw(DrawMode draw_mode, Please revert the unnecessary changes in this method, you're ignoring the return value of LayerImpl::WillDraw. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:121: scoped_ptr<TileDrawQuad> thumb_quad = TileDrawQuad::Create(); TileDrawQuad is not the right choice here, it's only for tiled web content. Please switch back to TextureDrawQuad. https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.cc:24: delete [] reinterpret_cast<uint8_t*>(pixels_); There should be a way to do this with a scoped_ptr of some kind instead https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.h:15: class UIResourceManagerClient { Each class should be defined in a file with the same name. https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.h:23: virtual void UIResourceReady(UIResourceId id) = 0; Most users won't need to take any action for this so please switch it to an empty implementation {} https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.h:27: class UIResourceBitmap : public base::RefCountedThreadSafe<UIResourceBitmap> { Each class should be defined in a file with the same name. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:100: nit: accidental whitespace https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:257: nit: accidental whitespace https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:355: // ui resource processing Delete this comment https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:67: nit: unnecessary spacing change https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:264: void UpdateUIResource(UIResourceId uid, I think we can delete this for simplicity, there aren't any use cases that need this functionality. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:269: void UIResourceReady(UIResourceId id); Put these two into the "// LayerTreeHost interface to Proxy." as they are not client-facing. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:568: nit: extra whitespace https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2380: gfx::Rect(1, 1, bitmap_size.width(), "1, 1" looks wrong here. It should be 0, 0. In fact, you can write gfx::Rect(bitmap_size) instead. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:82: virtual void UIResourceCreatedOnImplThread(UIResourceId uid) = 0; "UIResourceReadyOnImplThread" would be a better name as "creation" is a nebulous concept here. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:384: bool async); Sorry, I meant to delete this "async" boolean from my design doc, you can get rid of it.
https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.cc (right): https://codereview.chromium.org/18191020/diff/20001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.cc:24: delete [] reinterpret_cast<uint8_t*>(pixels_); On 2013/07/09 17:57:48, enne wrote: > Just make this a scoped_array<uint8_t>. this is spelled today as scoped_ptr<uint8_t[]>, see dcheng's patches.
Most comments have been addressed. My only question is, in scrollbar_layer.h, whether "Update" can really be removed. If so, where should the rasterization code go? New patch will be uploaded soon. Tests will follow after. Thanks. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.cc File cc/layers/scrollbar_layer.cc (left): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:12: #include "cc/resources/caching_bitmap_content_layer_updater.h" On 2013/07/10 23:07:22, aelias wrote: > Three more includes to delete here. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.cc File cc/layers/scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:26: On 2013/07/10 23:07:22, aelias wrote: > nit: accidental spacing change Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:116: layer_tree_host()->settings().solid_color_scrollbar_thickness_dip; On 2013/07/10 23:07:22, aelias wrote: > nit: accidental spacing change Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:129: On 2013/07/10 23:07:22, aelias wrote: > nit: accidental spacing change Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:149: if (host != layer_tree_host() && host == NULL) { On 2013/07/10 23:07:22, aelias wrote: > You changed the behavior here by changing this line, bring it back to if (!host > || host != layer_tree_host()) (which also deals with LTH changes) Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:151: layer_tree_host()->DeleteUIResource(track_ui_resource_id_); On 2013/07/10 23:07:22, aelias wrote: > Also throw out the UIResourceBitmaps here. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:205: new uint8_t[track_rect.width()*track_rect.height()*4], On 2013/07/10 23:07:22, aelias wrote: > nit: spaces around * Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:242: new uint8_t[thumb_rect.width()*thumb_rect.height()*4], On 2013/07/10 23:07:22, aelias wrote: > nit: spaces around * Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:278: void ScrollbarLayer::UIResourceReady(UIResourceId id) { On 2013/07/10 23:07:22, aelias wrote: > This code is unnecessary, please delete it. It's not even possible for > UIResourceReady to get called unless the UI resource was already pushed to the > impl thread earlier. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (left): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:16: class ResourceUpdateQueue; On 2013/07/10 23:07:22, aelias wrote: > Also delete this. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:20: public UIResourceManagerClient { On 2013/07/10 23:07:22, aelias wrote: > nit: wrong spacing Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:37: virtual void Update(ResourceUpdateQueue* queue, On 2013/07/10 23:07:22, aelias wrote: > This should be deleted. Isn't this function where the bitmaps are generated? If not here, then where should the texture updates go? https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:51: virtual void UIResourceReady(UIResourceId id) OVERRIDE; On 2013/07/10 23:07:22, aelias wrote: > No need for this here, it should have an empty implementation in the base class. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:80: bool skip_update_; On 2013/07/10 23:07:22, aelias wrote: > It doesn't look like this variable does anything useful, please delete it. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... File cc/layers/scrollbar_layer_impl.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:80: NoteLayerPropertyChanged(); On 2013/07/10 23:07:22, aelias wrote: > This is never called in PushPropertiesTo anywhere else so I don't think these 2 > lines should be here. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:85: bool ScrollbarLayerImpl::WillDraw(DrawMode draw_mode, On 2013/07/10 23:07:22, aelias wrote: > Please revert the unnecessary changes in this method, you're ignoring the return > value of LayerImpl::WillDraw. Done. https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:121: scoped_ptr<TileDrawQuad> thumb_quad = TileDrawQuad::Create(); On 2013/07/10 23:07:22, aelias wrote: > TileDrawQuad is not the right choice here, it's only for tiled web content. > Please switch back to TextureDrawQuad. Done. https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.cc:24: delete [] reinterpret_cast<uint8_t*>(pixels_); On 2013/07/10 23:07:22, aelias wrote: > There should be a way to do this with a scoped_ptr of some kind instead Done. https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... File cc/resources/ui_resource_manager.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.h:15: class UIResourceManagerClient { On 2013/07/10 23:07:22, aelias wrote: > Each class should be defined in a file with the same name. Done. https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.h:23: virtual void UIResourceReady(UIResourceId id) = 0; On 2013/07/10 23:07:22, aelias wrote: > Most users won't need to take any action for this so please switch it to an > empty implementation {} Done. https://codereview.chromium.org/18191020/diff/50001/cc/resources/ui_resource_... cc/resources/ui_resource_manager.h:27: class UIResourceBitmap : public base::RefCountedThreadSafe<UIResourceBitmap> { On 2013/07/10 23:07:22, aelias wrote: > Each class should be defined in a file with the same name. Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:100: On 2013/07/10 23:07:22, aelias wrote: > nit: accidental whitespace Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:257: On 2013/07/10 23:07:22, aelias wrote: > nit: accidental whitespace Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:355: // ui resource processing On 2013/07/10 23:07:22, aelias wrote: > Delete this comment Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:67: On 2013/07/10 23:07:22, aelias wrote: > nit: unnecessary spacing change Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:264: void UpdateUIResource(UIResourceId uid, On 2013/07/10 23:07:22, aelias wrote: > I think we can delete this for simplicity, there aren't any use cases that need > this functionality. Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:269: void UIResourceReady(UIResourceId id); On 2013/07/10 23:07:22, aelias wrote: > Put these two into the "// LayerTreeHost interface to Proxy." as they are not > client-facing. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:568: On 2013/07/10 23:07:22, aelias wrote: > nit: extra whitespace Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2380: gfx::Rect(1, 1, bitmap_size.width(), On 2013/07/10 23:07:22, aelias wrote: > "1, 1" looks wrong here. It should be 0, 0. In fact, you can write > gfx::Rect(bitmap_size) instead. Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:82: virtual void UIResourceCreatedOnImplThread(UIResourceId uid) = 0; On 2013/07/10 23:07:22, aelias wrote: > "UIResourceReadyOnImplThread" would be a better name as "creation" is a nebulous > concept here. Done. https://codereview.chromium.org/18191020/diff/50001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:384: bool async); On 2013/07/10 23:07:22, aelias wrote: > Sorry, I meant to delete this "async" boolean from my design doc, you can get > rid of it. Done.
https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (right): https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:37: virtual void Update(ResourceUpdateQueue* queue, On 2013/07/11 23:54:44, powei wrote: > On 2013/07/10 23:07:22, aelias wrote: > > This should be deleted. > > Isn't this function where the bitmaps are generated? If not here, then where > should the texture updates go? This function is tied to the legacy PrioritizedResourceManager system, that's why we should delete it. I believe the only reason to reraster scrollbars is when contents scale changes. So you could trigger raster at the end of CalculateContentsScale (after checking that resulted in a change from the previous value).
On 2013/07/12 00:52:27, aelias wrote: > https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer.h > File cc/layers/scrollbar_layer.h (right): > > https://codereview.chromium.org/18191020/diff/50001/cc/layers/scrollbar_layer... > cc/layers/scrollbar_layer.h:37: virtual void Update(ResourceUpdateQueue* queue, > On 2013/07/11 23:54:44, powei wrote: > > On 2013/07/10 23:07:22, aelias wrote: > > > This should be deleted. > > > > Isn't this function where the bitmaps are generated? If not here, then where > > should the texture updates go? > > This function is tied to the legacy PrioritizedResourceManager system, that's > why we should delete it. > > I believe the only reason to reraster scrollbars is when contents scale changes. > So you could trigger raster at the end of CalculateContentsScale (after > checking that resulted in a change from the previous value). Hmm? We have to reraster scrollbar components whenever they are invalidated (i.e. changes to tickmarks, changes to hover state, etc etc).
On 2013/07/13 01:42:48, jamesr wrote: > Hmm? We have to reraster scrollbar components whenever they are invalidated > (i.e. changes to tickmarks, changes to hover state, etc etc). OK. I clearly don't know much about desktop scrollbar requirements. In that case, let's leave the Update() method after all. It looks like PictureLayer similarly uses it but doesn't make use of the arguments, so it's not as illegitimate as I thought.
On 2013/07/13 01:59:59, aelias wrote: > On 2013/07/13 01:42:48, jamesr wrote: > > Hmm? We have to reraster scrollbar components whenever they are invalidated > > (i.e. changes to tickmarks, changes to hover state, etc etc). > > OK. I clearly don't know much about desktop scrollbar requirements. > > In that case, let's leave the Update() method after all. It looks like > PictureLayer similarly uses it but doesn't make use of the arguments, so it's > not as illegitimate as I thought. A new patch is up for review. Added tests and handling of lost context. Please take a look. Thank you.
A bunch of nits, with some a few more real questions somewhere in the middle. :) https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer.cc File cc/layers/scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:103: track_rect_ = scrollbar_->TrackRect(); Why does this come before the solid color early-out? Also, why do you need to stash these values during CalculateContentsScale? Can you just do this work in Update as before? https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:20: : public ContentsScalingLayer { style nit: the previous code was also correct style, and is what clang-format would have given you https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:49: virtual void Update(ResourceUpdateQueue* queue, style nit: please keep this where it was with the layer interface block https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... File cc/layers/scrollbar_layer_impl.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:74: if (scrollbar_layer->track_ui_resource_id_ != track_ui_resource_id_ || No need for this to be conditional. https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.cc:265: void ScrollbarLayerImpl::DidLoseOutputSurface() {} This can be removed entirely. https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... File cc/resources/ui_resource_bitmap.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_bitmap.h:13: // Ref-counted bitmap class (can’t use SkBitmap because of ETC1) Could you also mention why this needs to be thread-safe resource counted? https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_bitmap.h:18: ETC1 If ETC1 isn't handled yet, please don't include it in the UIResourceFormat enum. https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... File cc/resources/ui_resource_manager_client.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_manager_client.h:17: UIResourceCallback; style nit: weird indentation. Consider running 'git cl format' on your patch. https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_manager_client.h:19: // class UIResourceManagerClient { Commented code that should be removed? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:357: while (ui_resource_request_queue_.size() > 0) { Can you share this commit-time queue-processing functionality with the one during activation? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1084: // The boolean parameter indicates this retrieval is not due to a style nit: Rather than a comment, just say "bool resource_lost = false" and pass that to bitmap-cb.Run(). https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1088: ui_resource_client_map_[request.id] = bitmap_cb; Can you DCHECK that this doesn't exist in the map yet? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:233: void UIResourceReady(UIResourceId id); Can you remove this if it isn't called anywhere? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:275: // UI Resource management This comment doesn't really add much. Could you instead mention that ui resource ids are always > 0 and what the callback is for? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1304: // exisiting entries. typo: exisiting => existing https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1308: client_->UIResourceLostOnImplThread(iter->first); I am still a little confused about this path. It still seems to me that losing one implies losing all. Can LayerTreeHost::DidLoseOutputSurface drop all these resources without needing all of the UIResourceLost message bouncing? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1454: CreateUIResource(req.id, req.bitmap); In general activation is gated on having all of your resources uploaded and ready, so it's a little odd to create ui resources as a part of activation. Is the assumption that these resources are not going to be so costly that this will cause jank? Are these resources always going to be synchronously uploaded? Is there any way to (at least for impl-side painting) reuse the tile manager's asynchronous upload and also not make this memory usage invisible to it? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2398: resource_provider_->CreateGLTexture(bitmap->GetSize(), CreateGLTexture in LayerTreeHostImpl? What if there's a software renderer? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2399: GL_RGBA, Should you DCHECK that bitmap's format is RGBA8 and not ETC1? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2403: ui_resource_map_[uid] = id; Can you DCHECK that it doesn't previously exist? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2422: if (ui_resource_map_.find(uid) != ui_resource_map_.end()) { If you're going to find and then do something with the results, please keep the resulting iterator around rather than doing a second find. https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:465: UIResourceMap; style nit: weird indentation. https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:1473: // SINGLE_AND_MULTI_THREAD_DIRECT_RENDERER_TEST_F( Please don't comment out this test. There's still testing to be had for partial updates with content layers. Maybe this test should be updated to only deal with content layers. https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest_context.cc:1623: // A valid Resource Id on the impl-side. I'm not sure what this comment is trying to say. Maybe non-impl-side painting loses resources during commit but impl-side painting loses them during activation? https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest_context.cc:1659: if (layer_tree_host()->settings().impl_side_painting) { Does this even get called for non-impl-side painting?
https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer.cc File cc/layers/scrollbar_layer.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:213: if (track_ui_resource_id_) { nit: no {} https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:220: if (thumb_ui_resource_id_) { nit: no {} https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:225: base::Bind(&ScrollbarLayer::GetThumbBitmap, this)); This will crash if the ScrollbarLayer no longer exists when the context is lost. I suggest using base::WeakPtrFactory<ScrollbarLayer> here instead of directly "this". (I don't think it's a good idea to refcount the ScrollbarLayer.) https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:229: void ScrollbarLayer::RasterizeTrackAndThumb() { Add a DCHECK(!layer_tree_host()->settings().solid_color_scrollbars); https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:230: { nit: this block looks weird, I suggest removing it. https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:242: reinterpret_cast<uint8_t*>( Should be static_cast<> https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:257: track_canvas->drawRect(layer_sk_rect, paint); You are clearing the track canvas but not the thumb canvas, why? Please get rid of this clear if it's unnecessary, or add it to the thumb paint if it's needed. https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.cc:274: reinterpret_cast<uint8_t*>( Should be static_cast<> https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer.h File cc/layers/scrollbar_layer.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer.h:53: scoped_refptr<UIResourceBitmap> GetTrackBitmap(bool resource_lost); Make these private (that should compile fine). https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... File cc/layers/scrollbar_layer_impl.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_impl.h:71: void set_track_ui_resource_id(UIResourceId id); Please move these up near the other set_...() calls. https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_unittest.cc:432: CreateUIResource(const UIResourceCallback& content) OVERRIDE { Nit: 4-space indent. The usual way to style this is: virtual UIResourceId CreateUIResource( const UIResourceCallback& content) OVERRIDE { https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_unittest.cc:434: UIResourceId nid = ui_resource_next_id_++; Nit: rename to next_id https://codereview.chromium.org/18191020/diff/96046/cc/layers/scrollbar_layer... cc/layers/scrollbar_layer_unittest.cc:442: ui_resource_id_set_.erase(id); nit: 2-space indent. Note that there is a tool that can fix the spacing for you, see: https://groups.google.com/a/google.com/forum/#!searchin/chrome-team/Automatic... https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... File cc/resources/ui_resource_bitmap.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_bitmap.cc:15: ret->pixels_ = scoped_ptr<uint8_t[]>(reinterpret_cast<uint8_t*>(pixels)); static_cast<> https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... File cc/resources/ui_resource_bitmap.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_bitmap.h:22: static scoped_refptr<UIResourceBitmap> Create(void* pixels, Could these void* just be uint8_t* instead? Then we could avoid all the casts. https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... File cc/resources/ui_resource_manager_client.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/resources/ui_resource_... cc/resources/ui_resource_manager_client.h:5: #ifndef CC_RESOURCES_UI_RESOURCE_MANAGER_CLIENT_H_ May as well delete this file and move the typedefs to layer_tree_host.h for now, I think. https://codereview.chromium.org/18191020/diff/96046/cc/test/fake_layer_tree_h... File cc/test/fake_layer_tree_host_impl_client.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/test/fake_layer_tree_h... cc/test/fake_layer_tree_host_impl_client.h:43: virtual void UIResourceReadyOnImplThread(UIResourceId uid) OVERRIDE {} Should be removed https://codereview.chromium.org/18191020/diff/96046/cc/test/test_ui_resource_... File cc/test/test_ui_resource_client.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/test/test_ui_resource_... cc/test/test_ui_resource_client.h:25: unsigned lost_resource_count; int. "unsigned" is not used in Chromium (sometimes "size_t" is). https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1109: request.bitmap = iter->second.Run(true); I think that if this callback is a destroyed weak_ptr, you will probably get a return value of NULL. You should do something appropriate in that circumstance (I guess create a 1x1 white resource). Please also add a unit test for this case. https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1308: client_->UIResourceLostOnImplThread(iter->first); On 2013/07/22 23:09:15, enne wrote: > I am still a little confused about this path. It still seems to me that losing > one implies losing all. Can LayerTreeHost::DidLoseOutputSurface drop all these > resources without needing all of the UIResourceLost message bouncing? I agree with Enne, the loop should be in LayerTreeHost. https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1454: CreateUIResource(req.id, req.bitmap); On 2013/07/22 23:09:15, enne wrote: > In general activation is gated on having all of your resources uploaded and > ready, so it's a little odd to create ui resources as a part of activation. > > Is the assumption that these resources are not going to be so costly that this > will cause jank? Are these resources always going to be synchronously uploaded? > > Is there any way to (at least for impl-side painting) reuse the tile manager's > asynchronous upload and also not make this memory usage invisible to it? Yes, the assumption is that for v1 of this patch, it will only be used for small resources that are unlikely to cause jank or cause memory problems. In the future, we'll improve this to use the async upload system (this is one of the requirements for the Android thumbnail use case). https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:462: UIResourceRequestQueue ui_resource_request_queue_; This should live on the layer_tree_impl, since it's tree-associated state, not global. (The active tree will always have zero requests, but that's fine.) https://codereview.chromium.org/18191020/diff/96046/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (left): https://codereview.chromium.org/18191020/diff/96046/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:787: TRACE_EVENT0("cc", "ThreadProxy::StartCommitOnImplThread"); Looks like you deleted this line by accident. https://codereview.chromium.org/18191020/diff/96046/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/18191020/diff/96046/cc/trees/thread_proxy.h#n... cc/trees/thread_proxy.h:97: virtual void PostUIResourceLostToMainThread(UIResourceId uid); Change this to: private: ... void UIResourceLostOnMainThread(UIResourceId uid);
Most comments addressed. One question about the weak_ptr though, see scrollbar_layer.cc in Patch 5. New patch up and ran a few trybots. Thanks! https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... File cc/layers/scrollbar_layer.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:103: track_rect_ = scrollbar_->TrackRect(); On 2013/07/22 23:09:15, enne wrote: > Why does this come before the solid color early-out? > > Also, why do you need to stash these values during CalculateContentsScale? Can > you just do this work in Update as before? Done. I moved this from Update originally thinking that all work should be done in CalculateContentsScale. Forgot to move it back. trace_rect_ needs to come before the solid color check because these values are also used in solid mode. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:213: if (track_ui_resource_id_) { On 2013/07/23 00:06:48, aelias wrote: > nit: no {} Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:220: if (thumb_ui_resource_id_) { On 2013/07/23 00:06:48, aelias wrote: > nit: no {} Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:225: base::Bind(&ScrollbarLayer::GetThumbBitmap, this)); On 2013/07/23 00:06:48, aelias wrote: > This will crash if the ScrollbarLayer no longer exists when the context is lost. > > I suggest using base::WeakPtrFactory<ScrollbarLayer> here instead of directly > "this". (I don't think it's a good idea to refcount the ScrollbarLayer.) Apparently, "WeakCalls" are only supported for null return-type callbacks (see bind_internal.h:1093). This callback would crash if the ScrollbarLayer is not detached from LTH before it is deleted (with SetLayerTreeHost(NULL)). Can we assume that this situation will never happen? https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:229: void ScrollbarLayer::RasterizeTrackAndThumb() { On 2013/07/23 00:06:48, aelias wrote: > Add a DCHECK(!layer_tree_host()->settings().solid_color_scrollbars); Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:230: { On 2013/07/23 00:06:48, aelias wrote: > nit: this block looks weird, I suggest removing it. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:242: reinterpret_cast<uint8_t*>( On 2013/07/23 00:06:48, aelias wrote: > Should be static_cast<> Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:257: track_canvas->drawRect(layer_sk_rect, paint); On 2013/07/23 00:06:48, aelias wrote: > You are clearing the track canvas but not the thumb canvas, why? Please get rid > of this clear if it's unnecessary, or add it to the thumb paint if it's needed. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:274: reinterpret_cast<uint8_t*>( On 2013/07/23 00:06:48, aelias wrote: > Should be static_cast<> Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... File cc/layers/scrollbar_layer.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.h:20: : public ContentsScalingLayer { On 2013/07/22 23:09:15, enne wrote: > style nit: the previous code was also correct style, and is what clang-format > would have given you Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.h:49: virtual void Update(ResourceUpdateQueue* queue, On 2013/07/22 23:09:15, enne wrote: > style nit: please keep this where it was with the layer interface block Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.h:53: scoped_refptr<UIResourceBitmap> GetTrackBitmap(bool resource_lost); On 2013/07/23 00:06:48, aelias wrote: > Make these private (that should compile fine). Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... File cc/layers/scrollbar_layer_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer_impl.cc:74: if (scrollbar_layer->track_ui_resource_id_ != track_ui_resource_id_ || On 2013/07/22 23:09:15, enne wrote: > No need for this to be conditional. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer_impl.cc:265: void ScrollbarLayerImpl::DidLoseOutputSurface() {} On 2013/07/22 23:09:15, enne wrote: > This can be removed entirely. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... File cc/layers/scrollbar_layer_impl.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer_impl.h:71: void set_track_ui_resource_id(UIResourceId id); On 2013/07/23 00:06:48, aelias wrote: > Please move these up near the other set_...() calls. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... File cc/layers/scrollbar_layer_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer_unittest.cc:432: CreateUIResource(const UIResourceCallback& content) OVERRIDE { On 2013/07/23 00:06:48, aelias wrote: > Nit: 4-space indent. The usual way to style this is: > > virtual UIResourceId CreateUIResource( > const UIResourceCallback& content) OVERRIDE { Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer_unittest.cc:434: UIResourceId nid = ui_resource_next_id_++; On 2013/07/23 00:06:48, aelias wrote: > Nit: rename to next_id Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer_unittest.cc:442: ui_resource_id_set_.erase(id); On 2013/07/23 00:06:48, aelias wrote: > nit: 2-space indent. > > Note that there is a tool that can fix the spacing for you, see: > https://groups.google.com/a/google.com/forum/#%21searchin/chrome-team/Automat... Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... File cc/resources/ui_resource_bitmap.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_bitmap.cc:15: ret->pixels_ = scoped_ptr<uint8_t[]>(reinterpret_cast<uint8_t*>(pixels)); On 2013/07/23 00:06:48, aelias wrote: > static_cast<> Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... File cc/resources/ui_resource_bitmap.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_bitmap.h:13: // Ref-counted bitmap class (can’t use SkBitmap because of ETC1) On 2013/07/22 23:09:15, enne wrote: > Could you also mention why this needs to be thread-safe resource counted? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_bitmap.h:18: ETC1 On 2013/07/22 23:09:15, enne wrote: > If ETC1 isn't handled yet, please don't include it in the UIResourceFormat enum. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_bitmap.h:22: static scoped_refptr<UIResourceBitmap> Create(void* pixels, On 2013/07/23 00:06:48, aelias wrote: > Could these void* just be uint8_t* instead? Then we could avoid all the casts. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... File cc/resources/ui_resource_manager_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_manager_client.h:5: #ifndef CC_RESOURCES_UI_RESOURCE_MANAGER_CLIENT_H_ On 2013/07/23 00:06:48, aelias wrote: > May as well delete this file and move the typedefs to layer_tree_host.h for now, > I think. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_manager_client.h:17: UIResourceCallback; On 2013/07/22 23:09:15, enne wrote: > style nit: weird indentation. Consider running 'git cl format' on your patch. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/resources/ui_re... cc/resources/ui_resource_manager_client.h:19: // class UIResourceManagerClient { On 2013/07/22 23:09:15, enne wrote: > Commented code that should be removed? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/test/fake_layer... File cc/test/fake_layer_tree_host_impl_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/test/fake_layer... cc/test/fake_layer_tree_host_impl_client.h:43: virtual void UIResourceReadyOnImplThread(UIResourceId uid) OVERRIDE {} On 2013/07/23 00:06:48, aelias wrote: > Should be removed Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/test/test_ui_re... File cc/test/test_ui_resource_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/test/test_ui_re... cc/test/test_ui_resource_client.h:25: unsigned lost_resource_count; On 2013/07/23 00:06:48, aelias wrote: > int. "unsigned" is not used in Chromium (sometimes "size_t" is). Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host.cc:357: while (ui_resource_request_queue_.size() > 0) { On 2013/07/22 23:09:15, enne wrote: > Can you share this commit-time queue-processing functionality with the one > during activation? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host.cc:1084: // The boolean parameter indicates this retrieval is not due to a On 2013/07/22 23:09:15, enne wrote: > style nit: Rather than a comment, just say "bool resource_lost = false" and pass > that to bitmap-cb.Run(). Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host.cc:1088: ui_resource_client_map_[request.id] = bitmap_cb; On 2013/07/22 23:09:15, enne wrote: > Can you DCHECK that this doesn't exist in the map yet? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host.cc:1109: request.bitmap = iter->second.Run(true); On 2013/07/23 00:06:48, aelias wrote: > I think that if this callback is a destroyed weak_ptr, you will probably get a > return value of NULL. You should do something appropriate in that circumstance > (I guess create a 1x1 white resource). Please also add a unit test for this > case. I don't know if a weak_ptr can be passed with the callback. See comment in scrollbar_layer.cc. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... File cc/trees/layer_tree_host.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host.h:233: void UIResourceReady(UIResourceId id); On 2013/07/22 23:09:15, enne wrote: > Can you remove this if it isn't called anywhere? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host.h:275: // UI Resource management On 2013/07/22 23:09:15, enne wrote: > This comment doesn't really add much. Could you instead mention that ui > resource ids are always > 0 and what the callback is for? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:1304: // exisiting entries. On 2013/07/22 23:09:15, enne wrote: > typo: exisiting => existing Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:1308: client_->UIResourceLostOnImplThread(iter->first); On 2013/07/23 00:06:48, aelias wrote: > On 2013/07/22 23:09:15, enne wrote: > > I am still a little confused about this path. It still seems to me that > losing > > one implies losing all. Can LayerTreeHost::DidLoseOutputSurface drop all > these > > resources without needing all of the UIResourceLost message bouncing? > > I agree with Enne, the loop should be in LayerTreeHost. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:1308: client_->UIResourceLostOnImplThread(iter->first); On 2013/07/22 23:09:15, enne wrote: > I am still a little confused about this path. It still seems to me that losing > one implies losing all. Can LayerTreeHost::DidLoseOutputSurface drop all these > resources without needing all of the UIResourceLost message bouncing? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:1454: CreateUIResource(req.id, req.bitmap); On 2013/07/23 00:06:48, aelias wrote: > On 2013/07/22 23:09:15, enne wrote: > > In general activation is gated on having all of your resources uploaded and > > ready, so it's a little odd to create ui resources as a part of activation. > > > > Is the assumption that these resources are not going to be so costly that this > > will cause jank? Are these resources always going to be synchronously > uploaded? > > > > Is there any way to (at least for impl-side painting) reuse the tile manager's > > asynchronous upload and also not make this memory usage invisible to it? > > Yes, the assumption is that for v1 of this patch, it will only be used for small > resources that are unlikely to cause jank or cause memory problems. In the > future, we'll improve this to use the async upload system (this is one of the > requirements for the Android thumbnail use case). Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:1454: CreateUIResource(req.id, req.bitmap); On 2013/07/22 23:09:15, enne wrote: > In general activation is gated on having all of your resources uploaded and > ready, so it's a little odd to create ui resources as a part of activation. > > Is the assumption that these resources are not going to be so costly that this > will cause jank? Are these resources always going to be synchronously uploaded? > > Is there any way to (at least for impl-side painting) reuse the tile manager's > asynchronous upload and also not make this memory usage invisible to it? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:2398: resource_provider_->CreateGLTexture(bitmap->GetSize(), On 2013/07/22 23:09:15, enne wrote: > CreateGLTexture in LayerTreeHostImpl? What if there's a software renderer? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:2399: GL_RGBA, On 2013/07/22 23:09:15, enne wrote: > Should you DCHECK that bitmap's format is RGBA8 and not ETC1? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:2403: ui_resource_map_[uid] = id; On 2013/07/22 23:09:15, enne wrote: > Can you DCHECK that it doesn't previously exist? Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.cc:2422: if (ui_resource_map_.find(uid) != ui_resource_map_.end()) { On 2013/07/22 23:09:15, enne wrote: > If you're going to find and then do something with the results, please keep the > resulting iterator around rather than doing a second find. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... File cc/trees/layer_tree_host_impl.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.h:462: UIResourceRequestQueue ui_resource_request_queue_; On 2013/07/23 00:06:48, aelias wrote: > This should live on the layer_tree_impl, since it's tree-associated state, not > global. (The active tree will always have zero requests, but that's fine.) Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_impl.h:465: UIResourceMap; On 2013/07/22 23:09:15, enne wrote: > style nit: weird indentation. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... File cc/trees/layer_tree_host_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_unittest.cc:1473: // SINGLE_AND_MULTI_THREAD_DIRECT_RENDERER_TEST_F( On 2013/07/22 23:09:15, enne wrote: > Please don't comment out this test. There's still testing to be had for partial > updates with content layers. Maybe this test should be updated to only deal > with content layers. Done. Uncommented the test and kept the content layer parts. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... File cc/trees/layer_tree_host_unittest_context.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_unittest_context.cc:1623: // A valid Resource Id on the impl-side. On 2013/07/22 23:09:15, enne wrote: > I'm not sure what this comment is trying to say. Maybe non-impl-side painting > loses resources during commit but impl-side painting loses them during > activation? Done. Changed wording. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/layer_tre... cc/trees/layer_tree_host_unittest_context.cc:1659: if (layer_tree_host()->settings().impl_side_painting) { On 2013/07/22 23:09:15, enne wrote: > Does this even get called for non-impl-side painting? Done. I wasn't sure, but apparently it does get called (see layer_tree_test.cc:89). https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/thread_pr... File cc/trees/thread_proxy.cc (left): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/thread_pr... cc/trees/thread_proxy.cc:787: TRACE_EVENT0("cc", "ThreadProxy::StartCommitOnImplThread"); On 2013/07/23 00:06:48, aelias wrote: > Looks like you deleted this line by accident. Done. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/thread_pr... File cc/trees/thread_proxy.h (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/trees/thread_pr... cc/trees/thread_proxy.h:97: virtual void PostUIResourceLostToMainThread(UIResourceId uid); On 2013/07/23 00:06:48, aelias wrote: > Change this to: > > private: > ... > void UIResourceLostOnMainThread(UIResourceId uid); Done.
Getting there, thanks for plugging away at this. https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... File cc/layers/scrollbar_layer.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/96046/cc/layers/scrollba... cc/layers/scrollbar_layer.cc:225: base::Bind(&ScrollbarLayer::GetThumbBitmap, this)); On 2013/07/24 02:28:29, powei wrote: > On 2013/07/23 00:06:48, aelias wrote: > > This will crash if the ScrollbarLayer no longer exists when the context is > lost. > > > > I suggest using base::WeakPtrFactory<ScrollbarLayer> here instead of directly > > "this". (I don't think it's a good idea to refcount the ScrollbarLayer.) > > Apparently, "WeakCalls" are only supported for null return-type callbacks (see > bind_internal.h:1093). This callback would crash if the ScrollbarLayer is not > detached from LTH before it is deleted (with SetLayerTreeHost(NULL)). Can we > assume that this situation will never happen? OK, that makes sense. We should address the issue by being more careful about ScrollbarLayer destruction. https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/layers/scrollb... File cc/layers/scrollbar_layer.h (right): https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/layers/scrollb... cc/layers/scrollbar_layer.h:77: UIResourceId track_ui_resource_id_; I think we should contain these in a separate helper class that looks like: class ScopedUIResource { public: ScopedUIResource() {} ScopedUIResource(LayerTreeHost* host, scoped_refptr<UIResourceBitmap> bitmap) { reset(host, bitmap); } ~ScopedUIResource() { clear(); } void clear(); UIResourceID reset(LayerTreeHost* host, scoped_refptr<UIResourceBitmap> bitmap); UIResourceID id() const { return id_; } private: UIResourceId id_; scoped_refptr<UIResourceBitmap> bitmap_; LayerTreeHost* host_; }; This will be a common pattern for UIResource users and avoid leaks. https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:432: // Remove all pending UI resource requests because there might be deletion We need to honor both creation and deletion requests. Creation requests represent new resources that are probably going to be used soon. Deletion requests mean it's no longer safe to call the creation callback. Please fix this and add some unit tests making sure LayerTreeHost does the right thing when there are already pending requests during context loss.
Made changes wrt to aelias' suggestions. New patch set up for review. Thanks. https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/layers/scrollb... File cc/layers/scrollbar_layer.h (right): https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/layers/scrollb... cc/layers/scrollbar_layer.h:77: UIResourceId track_ui_resource_id_; On 2013/07/24 02:57:46, aelias wrote: > I think we should contain these in a separate helper class that looks like: > > class ScopedUIResource { > public: > ScopedUIResource() {} > ScopedUIResource(LayerTreeHost* host, scoped_refptr<UIResourceBitmap> bitmap) > { reset(host, bitmap); } > ~ScopedUIResource() { clear(); } > > void clear(); > UIResourceID reset(LayerTreeHost* host, scoped_refptr<UIResourceBitmap> > bitmap); > > UIResourceID id() const { return id_; } > private: > UIResourceId id_; > scoped_refptr<UIResourceBitmap> bitmap_; > LayerTreeHost* host_; > }; > > This will be a common pattern for UIResource users and avoid leaks. Done. https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/175003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:432: // Remove all pending UI resource requests because there might be deletion On 2013/07/24 02:57:46, aelias wrote: > We need to honor both creation and deletion requests. Creation requests > represent new resources that are probably going to be used soon. Deletion > requests mean it's no longer safe to call the creation callback. > > Please fix this and add some unit tests making sure LayerTreeHost does the right > thing when there are already pending requests during context loss. Done. And added tests in layer_tree_host_unittest_context.cc
https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... File cc/resources/scoped_ui_resource.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.cc:27: void ScopedUIResource::ClearResource() { This method is not ever safe to call on its own, you can no longer respond to a lost context callback. Please delete it. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.cc:35: ReleaseResource(host); No guarantee it's the same host. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.cc:38: if (host) Replace this if with DCHECK(host); https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... File cc/resources/scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:15: typedef int UIResourceId; I'm not sure every compiler out there is happy with redefined typedefs and this can be considered duplicated code anyway, I suggest just including the header. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:17: class ScopedUIResource : public base::RefCounted<ScopedUIResource> { I don't see a reason to refcount this class, please remove this and hold it in a scoped_ptr<> in ScrollbarLayer instead. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:23: ~ScopedUIResource() { ClearResource(); } This needs to call ReleaseResource, not ClearResource. This code is leaking the UIResourceID as it stands. So you'll need to hold the LayerTreeHost as a member. Also, make this non-inline (I think clang will complain). https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:26: void ResetResource(LayerTreeHost* host, On second thought, this method isn't needed after all, sorry for suggesting it. The user can just create a new ScopedUIResource instead. It doesn't look like ClearResource and ReleaseResource are useful either. Please delete all the public methods except for id().
https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1147: if (iter->type == UIResourceCreate && I suggest removing this if() and just creating a "request_set" from all UIResourceRequests instead (to avoid n^2 finds and for no other reasons). Ultimately you just want to avoid calling UIResourceLost for anything with an existing request, whether create or delete. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1155: ui_resource_request_queue_ = request_recovery_queue; I don't see the need for this assignment. It seems to be for the purpose of removing the deletes, but I think the impl-side code should be robust to spurious deletes instead.
Comments addressed. New patch. Thanks. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... File cc/resources/scoped_ui_resource.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.cc:27: void ScopedUIResource::ClearResource() { On 2013/07/25 23:45:11, aelias wrote: > This method is not ever safe to call on its own, you can no longer respond to a > lost context callback. Please delete it. Done. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.cc:35: ReleaseResource(host); On 2013/07/25 23:45:11, aelias wrote: > No guarantee it's the same host. Done. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.cc:38: if (host) On 2013/07/25 23:45:11, aelias wrote: > Replace this if with DCHECK(host); Done. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... File cc/resources/scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:15: typedef int UIResourceId; On 2013/07/25 23:45:11, aelias wrote: > I'm not sure every compiler out there is happy with redefined typedefs and this > can be considered duplicated code anyway, I suggest just including the header. Where should this definition live? It could be in layer_tree_host.h, but then including layer_tree_host everywhere we want to use UIResourceId does not seem appropriate. Should I just make a stub class (ui_resource_manager or ui_resource_id) that has this definition? https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:17: class ScopedUIResource : public base::RefCounted<ScopedUIResource> { On 2013/07/25 23:45:11, aelias wrote: > I don't see a reason to refcount this class, please remove this and hold it in a > scoped_ptr<> in ScrollbarLayer instead. Done. Switched back to client-based. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:23: ~ScopedUIResource() { ClearResource(); } On 2013/07/25 23:45:11, aelias wrote: > This needs to call ReleaseResource, not ClearResource. This code is leaking the > UIResourceID as it stands. So you'll need to hold the LayerTreeHost as a > member. > > Also, make this non-inline (I think clang will complain). Done. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:26: void ResetResource(LayerTreeHost* host, On 2013/07/25 23:45:11, aelias wrote: > On second thought, this method isn't needed after all, sorry for suggesting it. > The user can just create a new ScopedUIResource instead. It doesn't look like > ClearResource and ReleaseResource are useful either. Please delete all the > public methods except for id(). Done. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1147: if (iter->type == UIResourceCreate && On 2013/07/25 23:59:45, aelias wrote: > I suggest removing this if() and just creating a "request_set" from all > UIResourceRequests instead (to avoid n^2 finds and for no other reasons). > Ultimately you just want to avoid calling UIResourceLost for anything with an > existing request, whether create or delete. Done. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1155: ui_resource_request_queue_ = request_recovery_queue; On 2013/07/25 23:59:45, aelias wrote: > I don't see the need for this assignment. It seems to be for the purpose of > removing the deletes, but I think the impl-side code should be robust to > spurious deletes instead. Done.
Getting there, starting to look pretty clean overall. https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... File cc/resources/scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/242001/cc/resources/scop... cc/resources/scoped_ui_resource.h:15: typedef int UIResourceId; On 2013/07/26 23:07:27, powei wrote: > On 2013/07/25 23:45:11, aelias wrote: > > I'm not sure every compiler out there is happy with redefined typedefs and > this > > can be considered duplicated code anyway, I suggest just including the header. > > Where should this definition live? It could be in layer_tree_host.h, but then > including layer_tree_host everywhere we want to use UIResourceId does not seem > appropriate. Should I just make a stub class (ui_resource_manager or > ui_resource_id) that has this definition? ui_resource_client.h seems OK. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/cc.gyp File cc/cc.gyp (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/cc.gyp#newcode331 cc/cc.gyp:331: 'resources/ui_resource_bitmap.cc', Also add ui_resource_client.h https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/scop... File cc/resources/scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/scop... cc/resources/scoped_ui_resource.h:19: ScopedUIResource(LayerTreeHost* host, scoped_refptr<UIResourceBitmap> bitmap); Actually, the static Create thing you did earlier was great, you can go back to that. Just rename the other Create to something else. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/ui_r... File cc/resources/ui_resource_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/ui_r... cc/resources/ui_resource_client.h:19: virtual scoped_refptr<UIResourceBitmap> Create(bool resource_lost) = 0; Could you pass in the UIResourceID as argument here? It could be useful to some clients. I suggest renaming this to "GetBitmap" and add a comment: "GetBitmap() will be called once soon after resource creation and then will be called afterwards whenever the GL context is lost, on the same thread that LayerTreeHost::CreateUIResource was called on. It is only safe to delete a UIResourceClient object after DeleteUIResource has been called for all IDs associated with it. A valid bitmap always must be returned but it doesn't need to be the same size or format as the original." https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1132: request.bitmap = iter->second->Create(resource_lost); Please add DCHECK(request.bitmap.get()) as we don't support NULL return values. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1144: iter++) { nit: ++iter (as least in the past this compiled to more efficient code with STL containers) https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1151: iter++) { nit: ++iter
https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host.h (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.h:231: void UIResourceLost(UIResourceId id); Make this private. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.h:323: int ui_resource_id_; Rename to next_ui_resource_id_ https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1318: // 1. If the request was a creation, then the main thread will issue a special It won't reissue creations so this is incorrect. I think you should never clear the request queue. It's enough to have wiped the LayerTreeHostImpl::ui_resource_map_. Since at the moment of processing each delete request, nothing will happen if it was already unmapped to due lost context.
addressed comments. Some clarification on resource lost behavior might be needed (see layer_tree_host_impl.cc). Patch up with changes wrt comments. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/cc.gyp File cc/cc.gyp (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/cc.gyp#newcode331 cc/cc.gyp:331: 'resources/ui_resource_bitmap.cc', On 2013/07/26 23:35:32, aelias wrote: > Also add ui_resource_client.h Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/scop... File cc/resources/scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/scop... cc/resources/scoped_ui_resource.h:19: ScopedUIResource(LayerTreeHost* host, scoped_refptr<UIResourceBitmap> bitmap); On 2013/07/26 23:35:32, aelias wrote: > Actually, the static Create thing you did earlier was great, you can go back to > that. Just rename the other Create to something else. Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/ui_r... File cc/resources/ui_resource_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/resources/ui_r... cc/resources/ui_resource_client.h:19: virtual scoped_refptr<UIResourceBitmap> Create(bool resource_lost) = 0; On 2013/07/26 23:35:32, aelias wrote: > Could you pass in the UIResourceID as argument here? It could be useful to some > clients. > > I suggest renaming this to "GetBitmap" and add a comment: "GetBitmap() will be > called once soon after resource creation and then will be called afterwards > whenever the GL context is lost, on the same thread that > LayerTreeHost::CreateUIResource was called on. It is only safe to delete a > UIResourceClient object after DeleteUIResource has been called for all IDs > associated with it. A valid bitmap always must be returned but it doesn't need > to be the same size or format as the original." Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1132: request.bitmap = iter->second->Create(resource_lost); On 2013/07/26 23:35:32, aelias wrote: > Please add DCHECK(request.bitmap.get()) as we don't support NULL return values. Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1144: iter++) { On 2013/07/26 23:35:32, aelias wrote: > nit: ++iter (as least in the past this compiled to more efficient code with STL > containers) Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1151: iter++) { On 2013/07/26 23:35:32, aelias wrote: > nit: ++iter Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host.h (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.h:231: void UIResourceLost(UIResourceId id); On 2013/07/26 23:57:31, aelias wrote: > Make this private. Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host.h:323: int ui_resource_id_; On 2013/07/26 23:57:31, aelias wrote: > Rename to next_ui_resource_id_ Done. https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1318: // 1. If the request was a creation, then the main thread will issue a special On 2013/07/26 23:57:31, aelias wrote: > It won't reissue creations so this is incorrect. > > I think you should never clear the request queue. It's enough to have wiped the > LayerTreeHostImpl::ui_resource_map_. Since at the moment of processing each > delete request, nothing will happen if it was already unmapped to due lost > context. The main thread issues a "lost resource" creation for all entries in the ui_resource_client_map_. If a request is in the impl-side request queue, then it must have an entry in ui_resource_client_map_. That's what I mean by the comment. This impl-side request queue probably needs to be cleared because any creation requests left in this queue will be duplicated by the main-thread "re-creation requests", and there would be two creation requests for the same UIResourceId, and would not pass the DCHECK on line 2439.
https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1318: // 1. If the request was a creation, then the main thread will issue a special On 2013/07/30 21:47:00, powei wrote: > This impl-side request queue probably needs to be cleared because any creation > requests left in this queue will be duplicated by the main-thread "re-creation > requests", and there would be two creation requests for the same UIResourceId, > and would not pass the DCHECK on line 2439. But the main thread is avoiding re-creation requests for anything that has a pending create or delete, so it seems like that shouldn't be a problem?
https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1318: // 1. If the request was a creation, then the main thread will issue a special On 2013/07/30 22:28:44, aelias wrote: > On 2013/07/30 21:47:00, powei wrote: > > This impl-side request queue probably needs to be cleared because any creation > > requests left in this queue will be duplicated by the main-thread "re-creation > > requests", and there would be two creation requests for the same UIResourceId, > > and would not pass the DCHECK on line 2439. > > But the main thread is avoiding re-creation requests for anything that has a > pending create or delete, so it seems like that shouldn't be a problem? For impl-side painting, the main thread actually copies the request queue to the pending tree and clears its queue (layer_tree_host.cc:366). I guess, alternatively, we can think about keeping the main-thread queue around until impl-side has finished the requests.
https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/293001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1318: // 1. If the request was a creation, then the main thread will issue a special On 2013/07/30 22:37:50, powei wrote: > On 2013/07/30 22:28:44, aelias wrote: > > On 2013/07/30 21:47:00, powei wrote: > > > This impl-side request queue probably needs to be cleared because any > creation > > > requests left in this queue will be duplicated by the main-thread > "re-creation > > > requests", and there would be two creation requests for the same > UIResourceId, > > > and would not pass the DCHECK on line 2439. > > > > But the main thread is avoiding re-creation requests for anything that has a > > pending create or delete, so it seems like that shouldn't be a problem? > > For impl-side painting, the main thread actually copies the request queue to the > pending tree and clears its queue (layer_tree_host.cc:366). I guess, > alternatively, we can think about keeping the main-thread queue around until > impl-side has finished the requests. > Good point. The main thread has a pointer to the pending tree so we don't need to do "keep around" anything extra. We could just exclude pending-tree create/deletes as well when deciding what to send recreates requests for.
Ready for another look. A few notable changes since the last update: - for multiple resource creations with the same id in LTHI, old resource gets deleted and a new resource is created. - the ui resource request queue is never emptied in LTHI even after context lost. - the UIResourceId/ResourceId map is now cleared in ReleaseTreeResources as oppose to DidLoseOutputSurface to align with typical post-context-lost behavior. Thanks!
LGTM overall. Please let enne@ take another look before landing though. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/cc_tests.gyp#n... cc/cc_tests.gyp:9: # 'animation/animation_unittest.cc', Please remove these commented-out tests before submit. By the way, you can do --gtest_filter=LayerTreeHostImplTest.* instead of doing this. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... File cc/layers/scrollbar_layer_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... cc/layers/scrollbar_layer_impl.cc:14: #include "cc/quads/tile_draw_quad.h" remove this include https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1167: UIResourceIdSet request_set; Is this needed anymore? For simplicity, can we just call UIResourceLost on all resources in the map? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.h (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.h:87: enum UIResourceRequestType { nit: put this inside UIResourceRequest. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2456: DCHECK(bitmap->GetFormat() == UIResourceBitmap::RGBA8); DCHECK_EQ(UIResourceBitmap::RGBA8, bitmap->GetFormat()); (this gives more informative output) https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2468: gfx::Size bitmap_size = bitmap->GetSize(); nit: no need for this temp variable https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl_unittest.cc:6244: TEST_F(LayerTreeHostImplTest, UIResourceManagement) { Add a test case for two creations with the same UIResourceId.
Please also edit your patch description to give more detail.
https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/cc_tests.gyp#n... cc/cc_tests.gyp:9: # 'animation/animation_unittest.cc', On 2013/07/31 21:00:30, aelias wrote: > Please remove these commented-out tests before submit. > > By the way, you can do --gtest_filter=LayerTreeHostImplTest.* instead of doing > this. Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... File cc/layers/scrollbar_layer_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... cc/layers/scrollbar_layer_impl.cc:14: #include "cc/quads/tile_draw_quad.h" On 2013/07/31 21:00:30, aelias wrote: > remove this include Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1167: UIResourceIdSet request_set; On 2013/07/31 21:00:30, aelias wrote: > Is this needed anymore? For simplicity, can we just call UIResourceLost on all > resources in the map? If a user has created a resource, but the context was lost before the creation can be carried out on the impl-side, should we honor the original request or make a new resource-lost request? I don't know the use-cases well-enough to say one way or another. What are your thoughts? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.h (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.h:87: enum UIResourceRequestType { On 2013/07/31 21:00:30, aelias wrote: > nit: put this inside UIResourceRequest. Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2456: DCHECK(bitmap->GetFormat() == UIResourceBitmap::RGBA8); On 2013/07/31 21:00:30, aelias wrote: > DCHECK_EQ(UIResourceBitmap::RGBA8, bitmap->GetFormat()); (this gives more > informative output) Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2468: gfx::Size bitmap_size = bitmap->GetSize(); On 2013/07/31 21:00:30, aelias wrote: > nit: no need for this temp variable Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl_unittest.cc:6244: TEST_F(LayerTreeHostImplTest, UIResourceManagement) { On 2013/07/31 21:00:30, aelias wrote: > Add a test case for two creations with the same UIResourceId. Done.
https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1167: UIResourceIdSet request_set; On 2013/07/31 21:29:39, powei wrote: > On 2013/07/31 21:00:30, aelias wrote: > > Is this needed anymore? For simplicity, can we just call UIResourceLost on > all > > resources in the map? > > If a user has created a resource, but the context was lost before the creation > can be carried out on the impl-side, should we honor the original request or > make a new resource-lost request? I don't know the use-cases well-enough to say > one way or another. What are your thoughts? It doesn't matter much as long as we don't crash, leak, or incorrectly delete a resource. I think we should optimize to make the code simpler.
https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... File cc/layers/scrollbar_layer_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... cc/layers/scrollbar_layer_impl.cc:110: gfx::Rect track_quad_rect = content_bounds_rect; This temporary variable isn't used for anything. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/resources/ui_r... File cc/resources/ui_resource_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/resources/ui_r... cc/resources/ui_resource_client.h:29: virtual ~UIResourceClient() {} Do you need to add a destructor here for what should be a pure virtual interface? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/test/fake_scop... File cc/test/fake_scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/test/fake_scop... cc/test/fake_scoped_ui_resource.h:29: FakeScopedUIResource(); Unused? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1143: if (iter != ui_resource_client_map_.end()) { style nit: make this an early return rather than a large conditional block? (and same below?) https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:53: #include "gpu/GLES2/gl2extchromium.h" What is this include for? I'm a little skeptical of gpu or gl concerns leaking into LTHI. If you need this for GL_RGBA, maybe ResourceProvider should have a CreateResource(UIResourceBitmap) function? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1458: pending_tree_->ProcessUIResourceRequestQueue(); For sanity's sake, what about doing this before pending_tree_->PushPropertiesTo and DCHECKing that this list is empty and doesn't need to be pushed like all of the other tree properties? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2454: UIResourceId uid, DCHECK_GT(uid, 0)? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2461: if (id) style nit: {} for both the if and the else https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl_unittest.cc:6272: host_impl_->DeleteUIResource(ui_resource_id); Could you delete an invalid resource id here too? https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_unittest.cc:411: // Before the end of the test, we need to release the UI resources. Why is that? This seems like something that should happen automatically. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_unittest.cc:1165: // Number of textures should be doubled as the first textures This comment is no longer true. Could you update it to explain what the test is now doing?
https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... File cc/layers/scrollbar_layer_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/layers/scrollb... cc/layers/scrollbar_layer_impl.cc:110: gfx::Rect track_quad_rect = content_bounds_rect; On 2013/07/31 22:02:40, enne wrote: > This temporary variable isn't used for anything. Done. It is, but placed too far away from its uses. Moved for clarification. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/resources/ui_r... File cc/resources/ui_resource_client.h (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/resources/ui_r... cc/resources/ui_resource_client.h:29: virtual ~UIResourceClient() {} On 2013/07/31 22:02:40, enne wrote: > Do you need to add a destructor here for what should be a pure virtual > interface? The lint-er complained that a virtual destructor is needed because this class has a virtual function. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/test/fake_scop... File cc/test/fake_scoped_ui_resource.h (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/test/fake_scop... cc/test/fake_scoped_ui_resource.h:29: FakeScopedUIResource(); On 2013/07/31 22:02:40, enne wrote: > Unused? Done. Removed. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1143: if (iter != ui_resource_client_map_.end()) { On 2013/07/31 22:02:40, enne wrote: > style nit: make this an early return rather than a large conditional block? (and > same below?) Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host.cc:1167: UIResourceIdSet request_set; On 2013/07/31 21:56:51, aelias wrote: > On 2013/07/31 21:29:39, powei wrote: > > On 2013/07/31 21:00:30, aelias wrote: > > > Is this needed anymore? For simplicity, can we just call UIResourceLost on > > all > > > resources in the map? > > > > If a user has created a resource, but the context was lost before the creation > > can be carried out on the impl-side, should we honor the original request or > > make a new resource-lost request? I don't know the use-cases well-enough to > say > > one way or another. What are your thoughts? > > It doesn't matter much as long as we don't crash, leak, or incorrectly delete a > resource. I think we should optimize to make the code simpler. Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:53: #include "gpu/GLES2/gl2extchromium.h" On 2013/07/31 22:02:40, enne wrote: > What is this include for? I'm a little skeptical of gpu or gl concerns leaking > into LTHI. If you need this for GL_RGBA, maybe ResourceProvider should have a > CreateResource(UIResourceBitmap) function? Done. Removed. My old way of creating resource required this. No longer necessary. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1458: pending_tree_->ProcessUIResourceRequestQueue(); On 2013/07/31 22:02:40, enne wrote: > For sanity's sake, what about doing this before pending_tree_->PushPropertiesTo > and DCHECKing that this list is empty and doesn't need to be pushed like all of > the other tree properties? Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2454: UIResourceId uid, On 2013/07/31 22:02:40, enne wrote: > DCHECK_GT(uid, 0)? Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:2461: if (id) On 2013/07/31 22:02:40, enne wrote: > style nit: {} for both the if and the else Done. Only the if remains. The else conditional is actually incorrect. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_impl_unittest.cc:6272: host_impl_->DeleteUIResource(ui_resource_id); On 2013/07/31 22:02:40, enne wrote: > Could you delete an invalid resource id here too? Done. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... File cc/trees/layer_tree_host_unittest.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_unittest.cc:411: // Before the end of the test, we need to release the UI resources. On 2013/07/31 22:02:40, enne wrote: > Why is that? This seems like something that should happen automatically. Done. Revmoved. This was from when I still used the chromium callbacks, which ref-counted the owner. No longer necessary now that we switched back to the client model. https://chromiumcodereview.appspot.com/18191020/diff/363003/cc/trees/layer_tr... cc/trees/layer_tree_host_unittest.cc:1165: // Number of textures should be doubled as the first textures On 2013/07/31 22:02:40, enne wrote: > This comment is no longer true. Could you update it to explain what the test is > now doing? Done.
lgtm https://chromiumcodereview.appspot.com/18191020/diff/522001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/522001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1462: DCHECK_EQ(active_tree_->ui_resource_request_queue_size(), 0u); Ah, I meant to check this in LayerTreeImpl::PushPropertiesTo, as documentation that this property doesn't need to be pushed, so that somebody else looking at this later will know that it's expected.
Thanks everyone! I'm going to try and land this. https://chromiumcodereview.appspot.com/18191020/diff/522001/cc/trees/layer_tr... File cc/trees/layer_tree_host_impl.cc (right): https://chromiumcodereview.appspot.com/18191020/diff/522001/cc/trees/layer_tr... cc/trees/layer_tree_host_impl.cc:1462: DCHECK_EQ(active_tree_->ui_resource_request_queue_size(), 0u); On 2013/08/01 00:26:56, enne wrote: > Ah, I meant to check this in LayerTreeImpl::PushPropertiesTo, as documentation > that this property doesn't need to be pushed, so that somebody else looking at > this later will know that it's expected. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/18191020/558001
Message was sent while issue was closed.
Change committed as 214975 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
