Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
287593002:
android: content::UIResourceProvider and content::UIResourceClientAndroid (Closed)
|
Created:
6 years, 7 months ago by powei Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, clholgat, jdduke (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionandroid: content::UIResourceProvider and content::UIResourceClientAndroid
We abstract out the allocation of UI resources into the UIResourceProvider.
We add a client class that allows for callback from the provider when
resources are invalidated (due to change in LayerTreeHost).
android= https://chrome-internal-review.googlesource.com/#/c/164986
BUG=
TBR=jam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276955
Patch Set 1 #Patch Set 2 : user->listener #
Total comments: 16
Patch Set 3 : addressed dtrainor comments #
Total comments: 15
Patch Set 4 : addressed sievers comments #Patch Set 5 : update wrt to offline discussion #
Total comments: 4
Patch Set 6 : addressed dtrainor comments #
Total comments: 6
Patch Set 7 : addressed sievers comments #Messages
Total messages: 23 (0 generated)
ptal, thanks!
looking good! some comments. https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... File content/browser/android/ui_resource_provider_impl.cc (right): https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.cc:28: if (!retrieved_) { This feels a bit fragile. So we're returning the right bitmap for the first call of GetBitmap, then all subsequent calls will return an empty bitmap? What if the caller that creates resources needs to call getBitmap more than once? https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.cc:46: return bitmap_; return ScopedUIResource::GetBitmap(...)? https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.cc:55: bool retrieved_; Do we need DISALLOW_COPY_AND_ASSIGN? https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... File content/browser/android/ui_resource_provider_impl.h (right): https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014 here and all files https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.h:39: // indicates whether or not to release the resource once the bitmap We release the resource right after building it? I'm a bit confused. When do we use transient uploads? https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.h:50: // Same as above method but makes a copy of |pixels|. :( copies so sad for large thumbnails... https://chromiumcodereview.appspot.com/287593002/diff/20001/content/public/br... File content/public/browser/android/ui_resource_listener.h (right): https://chromiumcodereview.appspot.com/287593002/diff/20001/content/public/br... content/public/browser/android/ui_resource_listener.h:15: class CONTENT_EXPORT UIResourceListener { Some comments on this class about when all methods are called, etc. https://chromiumcodereview.appspot.com/287593002/diff/20001/content/public/br... content/public/browser/android/ui_resource_listener.h:17: virtual void SetUIResourceProvider( Do we need this?
https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... File content/browser/android/ui_resource_provider_impl.cc (right): https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... content/browser/android/ui_resource_provider_impl.cc:28: if (!retrieved_) { On 2014/05/13 20:38:00, David Trainor wrote: > This feels a bit fragile. So we're returning the right bitmap for the first > call of GetBitmap, then all subsequent calls will return an empty bitmap? What > if the caller that creates resources needs to call getBitmap more than once? GetBitmap is called (again) only when the context is lost and the compositor needs to recreate the resource. In that case, maybe we need to add ui_resource_provider_->LostUIResources(); ui_resource_provider_->RecreateUIResources(); to CompositorImpl::OnLostResources(); The idea of a transient Ui resource is to not hold onto the raw bitmap once it has been uploaded as a UI resource. This is necessary for large bitmaps like the thumbnail. https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... content/browser/android/ui_resource_provider_impl.cc:46: return bitmap_; On 2014/05/13 20:38:00, David Trainor wrote: > return ScopedUIResource::GetBitmap(...)? Done. https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... content/browser/android/ui_resource_provider_impl.cc:55: bool retrieved_; On 2014/05/13 20:38:00, David Trainor wrote: > Do we need DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... File content/browser/android/ui_resource_provider_impl.h (right): https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... content/browser/android/ui_resource_provider_impl.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/05/13 20:38:00, David Trainor wrote: > 2014 here and all files Done. https://codereview.chromium.org/287593002/diff/20001/content/browser/android/... content/browser/android/ui_resource_provider_impl.h:39: // indicates whether or not to release the resource once the bitmap On 2014/05/13 20:38:00, David Trainor wrote: > We release the resource right after building it? I'm a bit confused. When do > we use transient uploads? See comment in cpp. https://codereview.chromium.org/287593002/diff/20001/content/public/browser/a... File content/public/browser/android/ui_resource_listener.h (right): https://codereview.chromium.org/287593002/diff/20001/content/public/browser/a... content/public/browser/android/ui_resource_listener.h:15: class CONTENT_EXPORT UIResourceListener { On 2014/05/13 20:38:00, David Trainor wrote: > Some comments on this class about when all methods are called, etc. Done. https://codereview.chromium.org/287593002/diff/20001/content/public/browser/a... content/public/browser/android/ui_resource_listener.h:17: virtual void SetUIResourceProvider( On 2014/05/13 20:38:00, David Trainor wrote: > Do we need this? No. Removed.
lgtm https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... File content/browser/android/ui_resource_provider_impl.cc (right): https://chromiumcodereview.appspot.com/287593002/diff/20001/content/browser/a... content/browser/android/ui_resource_provider_impl.cc:28: if (!retrieved_) { On 2014/05/13 23:38:03, powei wrote: > On 2014/05/13 20:38:00, David Trainor wrote: > > This feels a bit fragile. So we're returning the right bitmap for the first > > call of GetBitmap, then all subsequent calls will return an empty bitmap? > What > > if the caller that creates resources needs to call getBitmap more than once? > > GetBitmap is called (again) only when the context is lost and the compositor > needs to recreate the resource. In that case, maybe we need to add > > ui_resource_provider_->LostUIResources(); > ui_resource_provider_->RecreateUIResources(); > > to CompositorImpl::OnLostResources(); > > The idea of a transient Ui resource is to not hold onto the raw bitmap once it > has been uploaded as a UI resource. This is necessary for large bitmaps like > the thumbnail. I guess I just think that's a scary assumption unless it's explicitly stated somewhere. But I don't know this particular bit of code well enough to say whether or not it's okay. https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... File content/public/browser/android/ui_resource_listener.h (right): https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... content/public/browser/android/ui_resource_listener.h:24: virtual void OnRecreateUIResources() = 0; Ah the comment makes these names a little more clear. Maybe ignore my comment downstream.
https://chromiumcodereview.appspot.com/287593002/diff/40001/content/browser/a... File content/browser/android/ui_resource_provider_impl.h (right): https://chromiumcodereview.appspot.com/287593002/diff/40001/content/browser/a... content/browser/android/ui_resource_provider_impl.h:30: ~UIResourceProviderImpl(); nit: virtual https://chromiumcodereview.appspot.com/287593002/diff/40001/content/browser/a... content/browser/android/ui_resource_provider_impl.h:70: typedef std::list<UIResourceListener*> UIResourceListenerList; Maybe use ObserverList for this? https://chromiumcodereview.appspot.com/287593002/diff/40001/content/browser/r... File content/browser/renderer_host/compositor_impl_android.cc (right): https://chromiumcodereview.appspot.com/287593002/diff/40001/content/browser/r... content/browser/renderer_host/compositor_impl_android.cc:388: client_->DidLoseResources(); I think we need to signal UIResourcesAreInvalid() here. But it can immediately recreate them. https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... File content/public/browser/android/compositor.h (right): https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... content/public/browser/android/compositor.h:77: // caller of this method cannot out-live the compositor. nit: You mean the UIResourceProvider does not outlive the compositor? You could return a reference instead of a pointer to make it obvious. https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... File content/public/browser/android/ui_resource_listener.h (right): https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... content/public/browser/android/ui_resource_listener.h:24: virtual void OnRecreateUIResources() = 0; On 2014/05/16 18:18:35, David Trainor wrote: > Ah the comment makes these names a little more clear. Maybe ignore my comment > downstream. Do we even need this? You cannot create UI resources while the Compositor is hidden (or has no surface) which is more or less under the app's control anyway. Or the UIResourceProvider could just hold on to the resources until the host is ready. https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... File content/public/browser/android/ui_resource_provider.h (right): https://chromiumcodereview.appspot.com/287593002/diff/40001/content/public/br... content/public/browser/android/ui_resource_provider.h:31: virtual void SetLayerTreeHost(cc::LayerTreeHost* host) = 0; I think we don't need to expost SetLayerTreeHost() here (only in the impl).
https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... File content/public/browser/android/ui_resource_listener.h (right): https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... content/public/browser/android/ui_resource_listener.h:24: virtual void OnRecreateUIResources() = 0; On 2014/05/16 20:12:19, sievers wrote: > On 2014/05/16 18:18:35, David Trainor wrote: > > Ah the comment makes these names a little more clear. Maybe ignore my comment > > downstream. > > Do we even need this? > You cannot create UI resources while the Compositor is hidden (or has no > surface) which is more or less under the app's control anyway. > > Or the UIResourceProvider could just hold on to the resources until the host is > ready. Also, how do we currently handle it to make sure that UIResources were passed before we start rendering, or rather the compositor is set to visible?
On 2014/05/16 20:22:35, sievers wrote: > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > File content/public/browser/android/ui_resource_listener.h (right): > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > content/public/browser/android/ui_resource_listener.h:24: virtual void > OnRecreateUIResources() = 0; > On 2014/05/16 20:12:19, sievers wrote: > > On 2014/05/16 18:18:35, David Trainor wrote: > > > Ah the comment makes these names a little more clear. Maybe ignore my > comment > > > downstream. > > > > Do we even need this? > > You cannot create UI resources while the Compositor is hidden (or has no > > surface) which is more or less under the app's control anyway. > > > > Or the UIResourceProvider could just hold on to the resources until the host > is > > ready. > > Also, how do we currently handle it to make sure that UIResources were passed > before we start rendering, or rather the compositor is set to visible? I feel like the interface is the opposite way it should be. Shouldn't the application be the UI resource provider and the compositor ask for the resources when it needs them? That's also how it seems to be in the underlying mechanism in cc.
On 2014/05/16 20:26:16, sievers wrote: > On 2014/05/16 20:22:35, sievers wrote: > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > File content/public/browser/android/ui_resource_listener.h (right): > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > content/public/browser/android/ui_resource_listener.h:24: virtual void > > OnRecreateUIResources() = 0; > > On 2014/05/16 20:12:19, sievers wrote: > > > On 2014/05/16 18:18:35, David Trainor wrote: > > > > Ah the comment makes these names a little more clear. Maybe ignore my > > comment > > > > downstream. > > > > > > Do we even need this? > > > You cannot create UI resources while the Compositor is hidden (or has no > > > surface) which is more or less under the app's control anyway. > > > > > > Or the UIResourceProvider could just hold on to the resources until the host > > is > > > ready. > > > > Also, how do we currently handle it to make sure that UIResources were passed > > before we start rendering, or rather the compositor is set to visible? > > I feel like the interface is the opposite way it should be. Shouldn't the > application be the UI resource provider and the compositor ask for the resources > when it needs them? That's also how it seems to be in the underlying mechanism > in cc. The compositor only asks for resources on creation, lost context or visibility change. UIResources are still primarily a push model. I don't think we can switch to 100% pull without losing our ability to manage memory usage.
On 2014/05/16 21:11:44, aelias wrote: > On 2014/05/16 20:26:16, sievers wrote: > > On 2014/05/16 20:22:35, sievers wrote: > > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > > File content/public/browser/android/ui_resource_listener.h (right): > > > > > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > > content/public/browser/android/ui_resource_listener.h:24: virtual void > > > OnRecreateUIResources() = 0; > > > On 2014/05/16 20:12:19, sievers wrote: > > > > On 2014/05/16 18:18:35, David Trainor wrote: > > > > > Ah the comment makes these names a little more clear. Maybe ignore my > > > comment > > > > > downstream. > > > > > > > > Do we even need this? > > > > You cannot create UI resources while the Compositor is hidden (or has no > > > > surface) which is more or less under the app's control anyway. > > > > > > > > Or the UIResourceProvider could just hold on to the resources until the > host > > > is > > > > ready. > > > > > > Also, how do we currently handle it to make sure that UIResources were > passed > > > before we start rendering, or rather the compositor is set to visible? > > > > I feel like the interface is the opposite way it should be. Shouldn't the > > application be the UI resource provider and the compositor ask for the > resources > > when it needs them? That's also how it seems to be in the underlying mechanism > > in cc. > > The compositor only asks for resources on creation, lost context or visibility > change. UIResources are still primarily a push model. I don't think we can > switch to 100% pull without losing our ability to manage memory usage. I'm just wondering whether this interface could be much simpler. Do we need anything else than a method that asks the app for a given resource bitmap? If the app provides a dummy because it didn't have a real bitmap, then it can just create a new resource later when it has a good bitmap. And we can just not allow calls to generate UI resources when the compositor is hidden.
On 2014/05/16 22:33:25, sievers wrote: > On 2014/05/16 21:11:44, aelias wrote: > > On 2014/05/16 20:26:16, sievers wrote: > > > On 2014/05/16 20:22:35, sievers wrote: > > > > > > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > > > File content/public/browser/android/ui_resource_listener.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > > > content/public/browser/android/ui_resource_listener.h:24: virtual void > > > > OnRecreateUIResources() = 0; > > > > On 2014/05/16 20:12:19, sievers wrote: > > > > > On 2014/05/16 18:18:35, David Trainor wrote: > > > > > > Ah the comment makes these names a little more clear. Maybe ignore my > > > > comment > > > > > > downstream. > > > > > > > > > > Do we even need this? > > > > > You cannot create UI resources while the Compositor is hidden (or has no > > > > > surface) which is more or less under the app's control anyway. > > > > > > > > > > Or the UIResourceProvider could just hold on to the resources until the > > host > > > > is > > > > > ready. > > > > > > > > Also, how do we currently handle it to make sure that UIResources were > > passed > > > > before we start rendering, or rather the compositor is set to visible? > > > > > > I feel like the interface is the opposite way it should be. Shouldn't the > > > application be the UI resource provider and the compositor ask for the > > resources > > > when it needs them? That's also how it seems to be in the underlying > mechanism > > > in cc. > > > > The compositor only asks for resources on creation, lost context or visibility > > change. UIResources are still primarily a push model. I don't think we can > > switch to 100% pull without losing our ability to manage memory usage. > > I'm just wondering whether this interface could be much simpler. Do we need > anything else than a method that asks the app for a given resource bitmap? > If the app provides a dummy because it didn't have a real bitmap, then it can > just create a new resource later when it has a good bitmap. > > And we can just not allow calls to generate UI resources when the compositor is > hidden. @sievers. Maybe you're thinking that the app-side users would be more like UIResourceClients? I guess I see two things: - The current app-side code is particular about when to load up the bitmaps and when to discard them. For what you're describing, there's little guarantee about when the app needs to have the bitmaps on hand. - We're still faced the problem that the LTH is cleared out every time the compositor is hidden. I can't think of an easy way to maintain the validity of the app's old UIResourceId through that. So we'd still need a way to signal the users that they need to create new resources.
https://codereview.chromium.org/287593002/diff/40001/content/browser/android/... File content/browser/android/ui_resource_provider_impl.h (right): https://codereview.chromium.org/287593002/diff/40001/content/browser/android/... content/browser/android/ui_resource_provider_impl.h:30: ~UIResourceProviderImpl(); On 2014/05/16 20:12:19, sievers wrote: > nit: virtual Done. https://codereview.chromium.org/287593002/diff/40001/content/browser/android/... content/browser/android/ui_resource_provider_impl.h:70: typedef std::list<UIResourceListener*> UIResourceListenerList; On 2014/05/16 20:12:19, sievers wrote: > Maybe use ObserverList for this? Done. https://codereview.chromium.org/287593002/diff/40001/content/browser/renderer... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/287593002/diff/40001/content/browser/renderer... content/browser/renderer_host/compositor_impl_android.cc:388: client_->DidLoseResources(); On 2014/05/16 20:12:19, sievers wrote: > I think we need to signal UIResourcesAreInvalid() here. But it can immediately > recreate them. Done. https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... content/public/browser/android/compositor.h:77: // caller of this method cannot out-live the compositor. On 2014/05/16 20:12:19, sievers wrote: > nit: You mean the UIResourceProvider does not outlive the compositor? > You could return a reference instead of a pointer to make it obvious. Done. https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... File content/public/browser/android/ui_resource_listener.h (right): https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... content/public/browser/android/ui_resource_listener.h:24: virtual void OnRecreateUIResources() = 0; On 2014/05/16 20:12:19, sievers wrote: > On 2014/05/16 18:18:35, David Trainor wrote: > > Ah the comment makes these names a little more clear. Maybe ignore my comment > > downstream. > > Do we even need this? > You cannot create UI resources while the Compositor is hidden (or has no > surface) which is more or less under the app's control anyway. > > Or the UIResourceProvider could just hold on to the resources until the host is > ready. Some users prefer to throw the bitmap away as soon as it has been uploaded as resource (large bitmaps like thumbnails). The provider holding on to the bitmap seems counter to that. https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... content/public/browser/android/ui_resource_listener.h:24: virtual void OnRecreateUIResources() = 0; On 2014/05/16 20:22:36, sievers wrote: > On 2014/05/16 20:12:19, sievers wrote: > > On 2014/05/16 18:18:35, David Trainor wrote: > > > Ah the comment makes these names a little more clear. Maybe ignore my > comment > > > downstream. > > > > Do we even need this? > > You cannot create UI resources while the Compositor is hidden (or has no > > surface) which is more or less under the app's control anyway. > > > > Or the UIResourceProvider could just hold on to the resources until the host > is > > ready. > > Also, how do we currently handle it to make sure that UIResources were passed > before we start rendering, or rather the compositor is set to visible? Updates to the various resource managers are called before every frame to make sure that the resources are up to date. https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... File content/public/browser/android/ui_resource_provider.h (right): https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... content/public/browser/android/ui_resource_provider.h:31: virtual void SetLayerTreeHost(cc::LayerTreeHost* host) = 0; On 2014/05/16 20:12:19, sievers wrote: > I think we don't need to expost SetLayerTreeHost() here (only in the impl). Done. good catch.
On 2014/05/16 23:39:37, powei wrote: > On 2014/05/16 22:33:25, sievers wrote: > > On 2014/05/16 21:11:44, aelias wrote: > > > On 2014/05/16 20:26:16, sievers wrote: > > > > On 2014/05/16 20:22:35, sievers wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > > > > File content/public/browser/android/ui_resource_listener.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/287593002/diff/40001/content/public/browser/a... > > > > > content/public/browser/android/ui_resource_listener.h:24: virtual void > > > > > OnRecreateUIResources() = 0; > > > > > On 2014/05/16 20:12:19, sievers wrote: > > > > > > On 2014/05/16 18:18:35, David Trainor wrote: > > > > > > > Ah the comment makes these names a little more clear. Maybe ignore > my > > > > > comment > > > > > > > downstream. > > > > > > > > > > > > Do we even need this? > > > > > > You cannot create UI resources while the Compositor is hidden (or has > no > > > > > > surface) which is more or less under the app's control anyway. > > > > > > > > > > > > Or the UIResourceProvider could just hold on to the resources until > the > > > host > > > > > is > > > > > > ready. > > > > > > > > > > Also, how do we currently handle it to make sure that UIResources were > > > passed > > > > > before we start rendering, or rather the compositor is set to visible? > > > > > > > > I feel like the interface is the opposite way it should be. Shouldn't the > > > > application be the UI resource provider and the compositor ask for the > > > resources > > > > when it needs them? That's also how it seems to be in the underlying > > mechanism > > > > in cc. > > > > > > The compositor only asks for resources on creation, lost context or > visibility > > > change. UIResources are still primarily a push model. I don't think we can > > > switch to 100% pull without losing our ability to manage memory usage. > > > > I'm just wondering whether this interface could be much simpler. Do we need > > anything else than a method that asks the app for a given resource bitmap? > > If the app provides a dummy because it didn't have a real bitmap, then it can > > just create a new resource later when it has a good bitmap. > > > > And we can just not allow calls to generate UI resources when the compositor > is > > hidden. > > @sievers. Maybe you're thinking that the app-side users would be more like > UIResourceClients? > > I guess I see two things: > - The current app-side code is particular about when to load up the bitmaps and > when to discard them. For what you're describing, there's little guarantee > about when the app needs to have the bitmaps on hand. > > - We're still faced the problem that the LTH is cleared out every time the > compositor is hidden. I can't think of an easy way to maintain the validity of > the app's old UIResourceId through that. So we'd still need a way to signal the > users that they need to create new resources. Yea that's a bit unfortunate. Maybe eventually we can not tear down the LTH but just the OutputSurface. For now, can we still hide that from the app by having the UIResourceProvider hold on to the resource until it can pass it to the LTH? In fact it already owns the resources. I think you just need to change it to an IDMap or something so that you can give the app an id independent from LTH. Then you also don't need the 'now you can create/pass your UI resources callback', which is a bit flaky anyways since the app really would have to pass over resources from *within* that callback (otherwise the LTH might be gone again). Also, how about just making the API take a ScopedUIResource and returning the id? The transient hack/optimization could be hidden in the caller. The code to create an SkBitmap from an ETC1 pixel buffer also seems a bit more like helper functionality.
PTAL. Thanks! Simplified interface after offline discussion with Daniel and David.
lgtm https://chromiumcodereview.appspot.com/287593002/diff/130001/content/browser/... File content/browser/android/ui_resource_provider_impl.cc (right): https://chromiumcodereview.appspot.com/287593002/diff/130001/content/browser/... content/browser/android/ui_resource_provider_impl.cc:23: UIResourcesAreInvalid(); Do we have to worry about this calling back into this class to try to rebuild resources with the incorrect host? What happens if we set host_ first? https://chromiumcodereview.appspot.com/287593002/diff/130001/content/browser/... content/browser/android/ui_resource_provider_impl.cc:33: ui_resource_client_map_.clear(); Same thing, what if the client tries to recreate a resource id here in UIResourceIsInvalid? I feel like we should support that case. I can see that being a clean way to implement a client actually...
On 2014/06/04 07:22:42, David Trainor wrote: > lgtm > > https://chromiumcodereview.appspot.com/287593002/diff/130001/content/browser/... > File content/browser/android/ui_resource_provider_impl.cc (right): > > https://chromiumcodereview.appspot.com/287593002/diff/130001/content/browser/... > content/browser/android/ui_resource_provider_impl.cc:23: > UIResourcesAreInvalid(); > Do we have to worry about this calling back into this class to try to rebuild > resources with the incorrect host? What happens if we set host_ first? > > https://chromiumcodereview.appspot.com/287593002/diff/130001/content/browser/... > content/browser/android/ui_resource_provider_impl.cc:33: > ui_resource_client_map_.clear(); > Same thing, what if the client tries to recreate a resource id here in > UIResourceIsInvalid? I feel like we should support that case. I can see that > being a clean way to implement a client actually... Ah sorry didn't mean to press the quick lgtm just yet. Had those questions about calling back into the provider on an invalidate. Once those are resolved I'm good with it.
https://codereview.chromium.org/287593002/diff/130001/content/browser/android... File content/browser/android/ui_resource_provider_impl.cc (right): https://codereview.chromium.org/287593002/diff/130001/content/browser/android... content/browser/android/ui_resource_provider_impl.cc:23: UIResourcesAreInvalid(); On 2014/06/04 07:22:41, David Trainor wrote: > Do we have to worry about this calling back into this class to try to rebuild > resources with the incorrect host? What happens if we set host_ first? Maybe this line should come after next line. That way if the user does try to re-create any ui resources, they'll just get 0 if the host is null. https://codereview.chromium.org/287593002/diff/130001/content/browser/android... content/browser/android/ui_resource_provider_impl.cc:33: ui_resource_client_map_.clear(); On 2014/06/04 07:22:41, David Trainor wrote: > Same thing, what if the client tries to recreate a resource id here in > UIResourceIsInvalid? I feel like we should support that case. I can see that > being a clean way to implement a client actually... Essentially I was thinking that we should not recreate when this signal is received. My thought is that we generate UI resource only when an UI resource Id is requested right before attaching that Id to a layer. At that point, the user should be fairly safe in that the compositor state is ok, and we're ok to generate an UI resource. On the other hand, I think you're suggesting that an UI resource should be created whenever the client gets the UIResourcesAreInvalid signal. At that point, either the LTH is there, and resources are created successfully or LTH is gone, and we get 0s. I guess we also need to try to create at client's initialization. The trouble here is that if we keep around the UI resource clients even when the Ids are invalid, then we'd have to add another method to allow for the client to detach itself from the provider. Delete and detach would become separate concepts. Would you prefer this?
offline ok from dtrainor. ping sievers@. Would you please take another look? Thanks!
Sorry missed this! LGTM with mainly one comment. https://codereview.chromium.org/287593002/diff/150001/content/browser/android... File content/browser/android/ui_resource_provider_impl.cc (right): https://codereview.chromium.org/287593002/diff/150001/content/browser/android... content/browser/android/ui_resource_provider_impl.cc:51: if (iter == ui_resource_client_map_.end()) This is a bit risky: If resources become invalid but the client still tries to delete an old resource (I assume that's why you allow returning here), this resource id might as well be a valid id for a new resource that was created in the meantime. Then we would not return here, but delete it below. Should we DCHECK() here and say that UIResourcesAreInvalid() means the user should not call DeleteUIResource() for any ids returned from Create() calls before that? (since we clear the map anyways) https://codereview.chromium.org/287593002/diff/150001/content/browser/android... File content/browser/android/ui_resource_provider_impl.h (right): https://codereview.chromium.org/287593002/diff/150001/content/browser/android... content/browser/android/ui_resource_provider_impl.h:8: #include <list> not used https://codereview.chromium.org/287593002/diff/150001/content/browser/android... content/browser/android/ui_resource_provider_impl.h:12: #include "content/public/browser/android/ui_resource_client_android.h" don't need to include line 11 and 12. UIResourceClientAndroid is already forward declared below
+tedchoc for content/public/browser/android OWNER https://codereview.chromium.org/287593002/diff/150001/content/browser/android... File content/browser/android/ui_resource_provider_impl.cc (right): https://codereview.chromium.org/287593002/diff/150001/content/browser/android... content/browser/android/ui_resource_provider_impl.cc:51: if (iter == ui_resource_client_map_.end()) On 2014/06/11 23:00:09, sievers wrote: > This is a bit risky: If resources become invalid but the client still tries to > delete an old resource (I assume that's why you allow returning here), this > resource id might as well be a valid id for a new resource that was created in > the meantime. Then we would not return here, but delete it below. > > Should we DCHECK() here and say that UIResourcesAreInvalid() means the user > should not call DeleteUIResource() for any ids returned from Create() calls > before that? (since we clear the map anyways) Done. Put this in a dcheck and added a comment in the client interface. https://codereview.chromium.org/287593002/diff/150001/content/browser/android... File content/browser/android/ui_resource_provider_impl.h (right): https://codereview.chromium.org/287593002/diff/150001/content/browser/android... content/browser/android/ui_resource_provider_impl.h:8: #include <list> On 2014/06/11 23:00:09, sievers wrote: > not used Done. https://codereview.chromium.org/287593002/diff/150001/content/browser/android... content/browser/android/ui_resource_provider_impl.h:12: #include "content/public/browser/android/ui_resource_client_android.h" On 2014/06/11 23:00:09, sievers wrote: > don't need to include line 11 and 12. UIResourceClientAndroid is already forward > declared below Done.
On 2014/06/13 00:02:11, powei wrote: > +tedchoc for content/public/browser/android OWNER > > https://codereview.chromium.org/287593002/diff/150001/content/browser/android... > File content/browser/android/ui_resource_provider_impl.cc (right): > > https://codereview.chromium.org/287593002/diff/150001/content/browser/android... > content/browser/android/ui_resource_provider_impl.cc:51: if (iter == > ui_resource_client_map_.end()) > On 2014/06/11 23:00:09, sievers wrote: > > This is a bit risky: If resources become invalid but the client still tries to > > delete an old resource (I assume that's why you allow returning here), this > > resource id might as well be a valid id for a new resource that was created in > > the meantime. Then we would not return here, but delete it below. > > > > Should we DCHECK() here and say that UIResourcesAreInvalid() means the user > > should not call DeleteUIResource() for any ids returned from Create() calls > > before that? (since we clear the map anyways) > > Done. Put this in a dcheck and added a comment in the client interface. > > https://codereview.chromium.org/287593002/diff/150001/content/browser/android... > File content/browser/android/ui_resource_provider_impl.h (right): > > https://codereview.chromium.org/287593002/diff/150001/content/browser/android... > content/browser/android/ui_resource_provider_impl.h:8: #include <list> > On 2014/06/11 23:00:09, sievers wrote: > > not used > > Done. > > https://codereview.chromium.org/287593002/diff/150001/content/browser/android... > content/browser/android/ui_resource_provider_impl.h:12: #include > "content/public/browser/android/ui_resource_client_android.h" > On 2014/06/11 23:00:09, sievers wrote: > > don't need to include line 11 and 12. UIResourceClientAndroid is already > forward > > declared below > > Done. OWNERS - lgtm
The CQ bit was checked by powei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/powei@chromium.org/287593002/170001
Message was sent while issue was closed.
Change committed as 276955 |
