android: Use UIResource for overscroll glow

android: Use UIResource for overscroll glow

Moved UIResourceProvider/UIResourceClientAndroid to ui from content to
allow loading the glow resources as UIResource.  The bitmaps are now
loaded from the SystemUIResourceManager class.

previous patch (abandoned): https://codereview.chromium.org/82553015/

android= https://chrome-internal-review.googlesource.com/#/c/168402/1
BUG=326326

[powei, 2014-07-08 19:12:43]

ptal.  Thanks!

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
File content/browser/android/edge_effect.cc (right):

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
content/browser/android/edge_effect.cc:163: pull_distance_(0) {
result of clang-format.  Let me know if the other way's preferrable.

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
File content/browser/android/overscroll_glow.cc (left):

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
content/browser/android/overscroll_glow.cc:38: return
skia::ImageOperations::Resize(
I didn't move this over.  I thought BitmapHelper.decodeDrawableResource already
took care of the size?

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:67: void
InitializeSystemUIResource(SystemUIResource system_resource,
having this feels a little awkward, but since ui does not depend on cc, we can't
actually upload the bitmaps as ui resource in WindowAndroid.

[jdduke (slow), 2014-07-08 20:10:12]

Thanks for taking this on!

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
File content/browser/android/edge_effect.cc (right):

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
content/browser/android/edge_effect.cc:163: pull_distance_(0) {
On 2014/07/08 19:12:43, powei wrote:
> result of clang-format.  Let me know if the other way's preferrable.

It's not, this is great.

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
File content/browser/android/overscroll_glow.cc (left):

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
content/browser/android/overscroll_glow.cc:38: return
skia::ImageOperations::Resize(
On 2014/07/08 19:12:43, powei wrote:
> I didn't move this over.  I thought BitmapHelper.decodeDrawableResource
already
> took care of the size?

Unfortunately no, if you look at the comments, decodeDrawableResource will
downsample to the smallest size still *larger* than the provide dimensions that
preserves the aspect ratio (a very cheap operation).  This last resize is to
reduce it further to the exact desired dimensions (slightly more expensive but
still desirable).  I originally had this resize code in the java_bitmap.cc
source as part of CreateJavaBitmapFromAndroidResource, we could move it back
there if this is the only consumer.

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.cc (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.cc:99: if (ui_resource_map_.find(system_resource)
== ui_resource_map_.end())
Premature optimization nit =/: Might as well use get once to save a query:

UIResourceClientAndroid* client = ui_resource_map_.get(system_resource);
return client ? client->id() : 0;

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:67: void
InitializeSystemUIResource(SystemUIResource system_resource,
On 2014/07/08 19:12:43, powei wrote:
> having this feels a little awkward, but since ui does not depend on cc, we
can't
> actually upload the bitmaps as ui resource in WindowAndroid.

Hmm, who is responsible for calling this now for the overscroll resources?

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:70: cc::UIResourceId
GetSystemUIResource(SystemUIResource system_resource);
Is it worth making this method const?

[jdduke (slow), 2014-07-08 20:42:08]

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:70: cc::UIResourceId
GetSystemUIResource(SystemUIResource system_resource);
This pull and push model makes synchronization somewhat hard to follow,
particularly if we scale this to more resources (as I'm planning with the new
composited selection handles).  

I'm wondering if we could have a subscriber model, where instead of polling
|GetSystemUIResource()| and then pushing it to the UIResourceLayer every frame,
or as often as we can to make sure the two are in sync, we subscribe a
UIResourceLayer to a particular SystemUIResource (perhaps storing a weak pointer
to the layer, and then pushing the resource id when it has changed?). 

void SubscribeToSystemUIResource(SystemUIResource system_resource,
UIResourceLayer* layer);
void UnsubscribeFromSystemUIResource(UIResourceLayer*);

I'm not sure if that makes sense in the current architecture, or if that would
be better accomplished at a later time?

[powei, 2014-07-09 20:41:04]

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
File content/browser/android/overscroll_glow.cc (left):

https://codereview.chromium.org/377013002/diff/20002/content/browser/android/...
content/browser/android/overscroll_glow.cc:38: return
skia::ImageOperations::Resize(
On 2014/07/08 20:10:11, jdduke wrote:
> On 2014/07/08 19:12:43, powei wrote:
> > I didn't move this over.  I thought BitmapHelper.decodeDrawableResource
> already
> > took care of the size?
> 
> Unfortunately no, if you look at the comments, decodeDrawableResource will
> downsample to the smallest size still *larger* than the provide dimensions
that
> preserves the aspect ratio (a very cheap operation).  This last resize is to
> reduce it further to the exact desired dimensions (slightly more expensive but
> still desirable).  I originally had this resize code in the java_bitmap.cc
> source as part of CreateJavaBitmapFromAndroidResource, we could move it back
> there if this is the only consumer.

This is the only consumer.  I can move it to java_bitmap.cc

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:70: cc::UIResourceId
GetSystemUIResource(SystemUIResource system_resource);
On 2014/07/08 20:42:08, jdduke wrote:
> This pull and push model makes synchronization somewhat hard to follow,
> particularly if we scale this to more resources (as I'm planning with the new
> composited selection handles).  
> 
> I'm wondering if we could have a subscriber model, where instead of polling
> |GetSystemUIResource()| and then pushing it to the UIResourceLayer every
frame,
> or as often as we can to make sure the two are in sync, we subscribe a
> UIResourceLayer to a particular SystemUIResource (perhaps storing a weak
pointer
> to the layer, and then pushing the resource id when it has changed?). 
> 
> void SubscribeToSystemUIResource(SystemUIResource system_resource,
> UIResourceLayer* layer);
> void UnsubscribeFromSystemUIResource(UIResourceLayer*);
> 
> I'm not sure if that makes sense in the current architecture, or if that would
> be better accomplished at a later time?

A subscriber model sounds good.  Although it would be different from the other
android ui elements.  Would we then keep a raw pointer to windowAndroid for when
we want to unsubscribe in EdgeEffect?

[jdduke (slow), 2014-07-09 21:12:13]

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:70: cc::UIResourceId
GetSystemUIResource(SystemUIResource system_resource);
On 2014/07/09 20:41:03, powei wrote:
> On 2014/07/08 20:42:08, jdduke wrote:
> > This pull and push model makes synchronization somewhat hard to follow,
> > particularly if we scale this to more resources (as I'm planning with the
new
> > composited selection handles).  
> > 
> > I'm wondering if we could have a subscriber model, where instead of polling
> > |GetSystemUIResource()| and then pushing it to the UIResourceLayer every
> frame,
> > or as often as we can to make sure the two are in sync, we subscribe a
> > UIResourceLayer to a particular SystemUIResource (perhaps storing a weak
> pointer
> > to the layer, and then pushing the resource id when it has changed?). 
> > 
> > void SubscribeToSystemUIResource(SystemUIResource system_resource,
> > UIResourceLayer* layer);
> > void UnsubscribeFromSystemUIResource(UIResourceLayer*);
> > 
> > I'm not sure if that makes sense in the current architecture, or if that
would
> > be better accomplished at a later time?
> 
> A subscriber model sounds good.  Although it would be different from the other
> android ui elements.  Would we then keep a raw pointer to windowAndroid for
when
> we want to unsubscribe in EdgeEffect?

Are there any downsides? I'm thinking perhaps we provide OverscrollGlow the raw
WindowAndroid pointer upon creation, and it would use that for subscription when
the layers are created, and when the effect is destroyed? Maybe down the road we
could have a helper type that manages the subscription automatically when the
layer is created/destroyed, but that's probably overkill now (and maybe
subscription is overkill now if it wouldn't be used by anything but the
overscroll effect, I could always add that in later?).

[powei, 2014-07-09 23:43:14]

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:70: cc::UIResourceId
GetSystemUIResource(SystemUIResource system_resource);
On 2014/07/09 21:12:13, jdduke wrote:
> On 2014/07/09 20:41:03, powei wrote:
> > On 2014/07/08 20:42:08, jdduke wrote:
> > > This pull and push model makes synchronization somewhat hard to follow,
> > > particularly if we scale this to more resources (as I'm planning with the
> new
> > > composited selection handles).  
> > > 
> > > I'm wondering if we could have a subscriber model, where instead of
polling
> > > |GetSystemUIResource()| and then pushing it to the UIResourceLayer every
> > frame,
> > > or as often as we can to make sure the two are in sync, we subscribe a
> > > UIResourceLayer to a particular SystemUIResource (perhaps storing a weak
> > pointer
> > > to the layer, and then pushing the resource id when it has changed?). 
> > > 
> > > void SubscribeToSystemUIResource(SystemUIResource system_resource,
> > > UIResourceLayer* layer);
> > > void UnsubscribeFromSystemUIResource(UIResourceLayer*);
> > > 
> > > I'm not sure if that makes sense in the current architecture, or if that
> would
> > > be better accomplished at a later time?
> > 
> > A subscriber model sounds good.  Although it would be different from the
other
> > android ui elements.  Would we then keep a raw pointer to windowAndroid for
> when
> > we want to unsubscribe in EdgeEffect?
> 
> Are there any downsides? I'm thinking perhaps we provide OverscrollGlow the
raw
> WindowAndroid pointer upon creation, and it would use that for subscription
when
> the layers are created, and when the effect is destroyed? Maybe down the road
we
> could have a helper type that manages the subscription automatically when the
> layer is created/destroyed, but that's probably overkill now (and maybe
> subscription is overkill now if it wouldn't be used by anything but the
> overscroll effect, I could always add that in later?).

So we'll need another interface like ui::SystemUIResourceLayer to avoid
depending on cc in ui.  Then its implementation in OverscrollGlow can serve as
the auto-subscription helper type you mentioned.

I'd like to give this a go.  I'll put this in OverscrollGlow's anon namespace
for now.  Maybe you can pull this implementation out if you'd like to do the
same for the selection handles.  WDYT?

[jdduke (slow), 2014-07-09 23:48:36]

On 2014/07/09 23:43:14, powei wrote:
> So we'll need another interface like ui::SystemUIResourceLayer to avoid
> depending on cc in ui.  Then its implementation in OverscrollGlow can serve as
> the auto-subscription helper type you mentioned.
> 
> I'd like to give this a go.  I'll put this in OverscrollGlow's anon namespace
> for now.  Maybe you can pull this implementation out if you'd like to do the
> same for the selection handles.  WDYT?

Perfect!

[powei, 2014-07-12 00:24:47]

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.cc (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.cc:99: if (ui_resource_map_.find(system_resource)
== ui_resource_map_.end())
On 2014/07/08 20:10:11, jdduke wrote:
> Premature optimization nit =/: Might as well use get once to save a query:
> 
> UIResourceClientAndroid* client = ui_resource_map_.get(system_resource);
> return client ? client->id() : 0;

Done.  No longer here.

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/377013002/diff/20002/ui/base/android/window_a...
ui/base/android/window_android.h:67: void
InitializeSystemUIResource(SystemUIResource system_resource,
On 2014/07/08 20:10:11, jdduke wrote:
> On 2014/07/08 19:12:43, powei wrote:
> > having this feels a little awkward, but since ui does not depend on cc, we
> can't
> > actually upload the bitmaps as ui resource in WindowAndroid.
> 
> Hmm, who is responsible for calling this now for the overscroll resources?

Done. Moved resource loading to system_ui_resource_manager_impl

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.cc:211:
resource_manager_->RefreshResourceOnSubscribers(
this the hacky part.  We need to call this to update the resource id on the
layer.  The previous id might have been invalidated due to app switch. 
Suggestions are welcomed here.

[jdduke (slow), 2014-07-12 01:01:09]

I'll take a closer look Monday, still wrapping my head around the
impl/manager/client/provider nuances :)

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.cc:211:
resource_manager_->RefreshResourceOnSubscribers(
On 2014/07/12 00:24:47, powei wrote:
> this the hacky part.  We need to call this to update the resource id on the
> layer.  The previous id might have been invalidated due to app switch. 
> Suggestions are welcomed here.

Hmm, yeah I'll think about this.

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
File content/browser/android/overscroll_glow.h (right):

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.h:28: class OverscrollGlowLayerWrapper {
Could we have EdgeEffect define this interface (in edge_effect.h)?
EdgeEffectLayerWrapper? I'd like to keep EdgeEffect from having an
OverscrollGlow dependency.

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.h:30: virtual scoped_refptr<cc::Layer>
layer() = 0;
What about just returning a naked pointer?

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.h:40: // Create a new effect.
WindowAndroid provides the resource for the effect.
Hmm, did you mean to reference WindowAndroid instead of SystemUIResourceManager?
Maybe put a brief note that |resource_manager| must outlive the effect?

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
File ui/base/android/system_ui_resource.h (right):

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource.h:23: virtual cc::UIResourceId id() = 0;
Do you anticipate having multiple implementations of this? Otherwise I wonder if
we should just implement it here?

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
File ui/base/android/system_ui_resource_manager.h (right):

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource_manager.h:27: void
SetUIResourceProvider(ui::UIResourceProvider* provider) {
Could we instead make this an argument to the constructor so there's no
confusion about lifetimes?

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource_manager.h:34: virtual
~SystemUIResourceManager() {}
Nit: Let's move the destructor up below the constructor.

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource_manager.h:37: virtual void
BuildResource(ui::SystemUIResource::Type type) = 0;
Can you explain why this class needs to be abstract?

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/window_a...
File ui/base/android/window_android.cc (right):

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/window_a...
ui/base/android/window_android.cc:13: #include
"ui/base/android/system_ui_resource_layer.h"
Do we still need these inclusions?

[jdduke (slow), 2014-07-12 01:06:49]

The higher level question I have is why we need an abstract class for
SystemUIResourceManager and an interface for UIResourceProvider? Do we need them
to be mockable? Or do we anticipate supporting multiple implementations?
Otherwise I'd just as soon the definition live by the declaration in ui/ (or
maybe I'm just missing some other reasons for having the implementation live in
content/?).

[powei, 2014-07-12 06:11:32]

On 2014/07/12 01:06:49, jdduke wrote:
> The higher level question I have is why we need an abstract class for
> SystemUIResourceManager and an interface for UIResourceProvider? Do we need
them
> to be mockable? Or do we anticipate supporting multiple implementations?
> Otherwise I'd just as soon the definition live by the declaration in ui/ (or
> maybe I'm just missing some other reasons for having the implementation live
in
> content/?).

The problem is ui cannot depend on cc.  That's the main reason for so many
abstract classes.

[powei, 2014-07-16 17:10:52]

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
File content/browser/android/overscroll_glow.h (right):

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.h:28: class OverscrollGlowLayerWrapper {
On 2014/07/12 01:01:09, jdduke wrote:
> Could we have EdgeEffect define this interface (in edge_effect.h)?
> EdgeEffectLayerWrapper? I'd like to keep EdgeEffect from having an
> OverscrollGlow dependency.

Done.

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.h:30: virtual scoped_refptr<cc::Layer>
layer() = 0;
On 2014/07/12 01:01:09, jdduke wrote:
> What about just returning a naked pointer?

Done.

https://codereview.chromium.org/377013002/diff/90001/content/browser/android/...
content/browser/android/overscroll_glow.h:40: // Create a new effect.
WindowAndroid provides the resource for the effect.
On 2014/07/12 01:01:09, jdduke wrote:
> Hmm, did you mean to reference WindowAndroid instead of
SystemUIResourceManager?
> Maybe put a brief note that |resource_manager| must outlive the effect?

Done.

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
File ui/base/android/system_ui_resource.h (right):

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource.h:23: virtual cc::UIResourceId id() = 0;
On 2014/07/12 01:01:09, jdduke wrote:
> Do you anticipate having multiple implementations of this? Otherwise I wonder
if
> we should just implement it here? 

Done.  Moved SystemUIResource to content

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
File ui/base/android/system_ui_resource_manager.h (right):

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource_manager.h:27: void
SetUIResourceProvider(ui::UIResourceProvider* provider) {
On 2014/07/12 01:01:09, jdduke wrote:
> Could we instead make this an argument to the constructor so there's no
> confusion about lifetimes?

Done.  SystemUIResource is now owned by UIResourceProviderImpl

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource_manager.h:34: virtual
~SystemUIResourceManager() {}
On 2014/07/12 01:01:09, jdduke wrote:
> Nit: Let's move the destructor up below the constructor.

Done.

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/system_u...
ui/base/android/system_ui_resource_manager.h:37: virtual void
BuildResource(ui::SystemUIResource::Type type) = 0;
On 2014/07/12 01:01:09, jdduke wrote:
> Can you explain why this class needs to be abstract?

Acknowledged.

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/window_a...
File ui/base/android/window_android.cc (right):

https://codereview.chromium.org/377013002/diff/90001/ui/base/android/window_a...
ui/base/android/window_android.cc:13: #include
"ui/base/android/system_ui_resource_layer.h"
On 2014/07/12 01:01:09, jdduke wrote:
> Do we still need these inclusions?

Done.  Removed

[powei, 2014-07-16 17:54:36]

cc aelias@

[jdduke (slow), 2014-07-16 19:01:51]

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.cc (right):

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:60: if
(GetData(type)->HasResource())
Should this be DCHECK(!GetData(type)->HasResource());

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:129:
DCHECK(resource);
also DCHECK(!resource_)?

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:141:
FOR_EACH_OBSERVER(ui::SystemUIResourceLayer,
Shouldn't the loading trigger a call to |OnUIResourceIdChanged()| only if its
necessary? Pushing the ID on every subscriber seems unfortunate if we can avoid
it?

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.h (right):

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:42: class Data :
public SystemUIResourceClient {
Could we just forward declare Data here as an inner class then define it in the
.cc file? Otherwise it's a good amount of noise in the header.

[powei, 2014-07-16 23:33:51]

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.cc (right):

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:60: if
(GetData(type)->HasResource())
On 2014/07/16 19:01:51, jdduke wrote:
> Should this be DCHECK(!GetData(type)->HasResource());

Done.

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:129:
DCHECK(resource);
On 2014/07/16 19:01:51, jdduke wrote:
> also DCHECK(!resource_)?

Done.

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.cc:141:
FOR_EACH_OBSERVER(ui::SystemUIResourceLayer,
On 2014/07/16 19:01:51, jdduke wrote:
> Shouldn't the loading trigger a call to |OnUIResourceIdChanged()| only if its
> necessary? Pushing the ID on every subscriber seems unfortunate if we can
avoid
> it?

Done.  Now this only happens when the LTH returns.

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.h (right):

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:42: class Data :
public SystemUIResourceClient {
On 2014/07/16 19:01:51, jdduke wrote:
> Could we just forward declare Data here as an inner class then define it in
the
> .cc file? Otherwise it's a good amount of noise in the header.

Seems like I can't forward declare and use it with ScopedPtrHashMap at the same
time.  ScopedPtrHashMap requires a valid destructor.  Any way to get around
that?

[jdduke (slow), 2014-07-17 00:02:18]

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.h (right):

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:42: class Data :
public SystemUIResourceClient {
On 2014/07/16 23:33:51, powei wrote:
> On 2014/07/16 19:01:51, jdduke wrote:
> > Could we just forward declare Data here as an inner class then define it in
> the
> > .cc file? Otherwise it's a good amount of noise in the header.
> 
> Seems like I can't forward declare and use it with ScopedPtrHashMap at the
same
> time.  ScopedPtrHashMap requires a valid destructor.  Any way to get around
> that?

Try moving the SystemUIResourceManagerImpl destructor definition into the .cc
file.

[jdduke (slow), 2014-07-17 19:56:51]

Is SystemUIResourceManagerImpl anything we could unit test?

https://codereview.chromium.org/377013002/diff/210001/content/browser/android...
File content/browser/android/edge_effect.h (right):

https://codereview.chromium.org/377013002/diff/210001/content/browser/android...
content/browser/android/edge_effect.h:35: virtual void AddToLayer(cc::Layer*
parent) = 0;
Looks like you can also just forward decl this class privately (below before the
scoped_ptr declarations), and put the complete (non-virtual) implementation in
the .cc source.

https://codereview.chromium.org/377013002/diff/210001/content/browser/android...
content/browser/android/edge_effect.h:64: void AddEffectsToLayer(cc::Layer*
parent);
Nitty nit: Let's either call this |AddToLayer()|, or perhaps |SetParent()|.

[powei, 2014-07-21 21:25:59]

added some simple unit tests. Ptal.

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.h (right):

https://codereview.chromium.org/377013002/diff/170001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:42: class Data :
public SystemUIResourceClient {
On 2014/07/17 00:02:18, jdduke wrote:
> On 2014/07/16 23:33:51, powei wrote:
> > On 2014/07/16 19:01:51, jdduke wrote:
> > > Could we just forward declare Data here as an inner class then define it
in
> > the
> > > .cc file? Otherwise it's a good amount of noise in the header.
> > 
> > Seems like I can't forward declare and use it with ScopedPtrHashMap at the
> same
> > time.  ScopedPtrHashMap requires a valid destructor.  Any way to get around
> > that?
> 
> Try moving the SystemUIResourceManagerImpl destructor definition into the .cc
> file.

Done.  That worked.  Thanks.

https://codereview.chromium.org/377013002/diff/210001/content/browser/android...
File content/browser/android/edge_effect.h (right):

https://codereview.chromium.org/377013002/diff/210001/content/browser/android...
content/browser/android/edge_effect.h:35: virtual void AddToLayer(cc::Layer*
parent) = 0;
On 2014/07/17 19:56:51, jdduke wrote:
> Looks like you can also just forward decl this class privately (below before
the
> scoped_ptr declarations), and put the complete (non-virtual) implementation in
> the .cc source.

Done.

https://codereview.chromium.org/377013002/diff/210001/content/browser/android...
content/browser/android/edge_effect.h:64: void AddEffectsToLayer(cc::Layer*
parent);
On 2014/07/17 19:56:51, jdduke wrote:
> Nitty nit: Let's either call this |AddToLayer()|, or perhaps |SetParent()|.

Done.

https://codereview.chromium.org/377013002/diff/230001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/377013002/diff/230001/content/content_tests.g...
content/content_tests.gypi:399:
'browser/android/system_ui_resource_manager_impl_unittest.cc',
Seems like there were no content_unittests for android before this?  Not sure if
this is the right place for it.

+tedchoc@ for comments.

[Ted C, 2014-07-21 23:17:05]

https://codereview.chromium.org/377013002/diff/230001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/377013002/diff/230001/content/content_tests.g...
content/content_tests.gypi:399:
'browser/android/system_ui_resource_manager_impl_unittest.cc',
On 2014/07/21 21:25:58, powei wrote:
> Seems like there were no content_unittests for android before this?  Not sure
if
> this is the right place for it.
> 
> +tedchoc@ for comments.

I think there is a:
'OS == "android"', {

section below that might be at least more consistent with the other android test
files.

[powei, 2014-07-23 18:00:55]

https://codereview.chromium.org/377013002/diff/230001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/377013002/diff/230001/content/content_tests.g...
content/content_tests.gypi:399:
'browser/android/system_ui_resource_manager_impl_unittest.cc',
On 2014/07/21 23:17:04, Ted C wrote:
> On 2014/07/21 21:25:58, powei wrote:
> > Seems like there were no content_unittests for android before this?  Not
sure
> if
> > this is the right place for it.
> > 
> > +tedchoc@ for comments.
> 
> I think there is a:
> 'OS == "android"', {
> 
> section below that might be at least more consistent with the other android
test
> files.

Done.

[jdduke (slow), 2014-07-23 18:17:45]

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/edge_effect.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/edge_effect.h:40:
EdgeEffect(ui::SystemUIResourceManager* resource_manager);
Explicit.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.cc:166: for (size_t i = 0; i <
EdgeEffect::EDGE_COUNT; ++i) {
Nit: Remove braces.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.cc:183: for (size_t i = 0; i <
EdgeEffect::EDGE_COUNT; ++i) {
Nit: Remove braces.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/overscroll_glow.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.h:31: // the effect will remain
deactivated until explicitly enabled.
You'll want to update the comment (specifically about |enabled_|).

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.h:33: ui::SystemUIResourceManager*
resource_manager);
It looks like the factory method doesn't really serve much purpose anymore as it
will always return something non-null.  However it's probably fine to keep it.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.h:73:
OverscrollGlow(ui::SystemUIResourceManager* resource_manager);
Explicit.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/system_ui_resource.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:16: class SystemUIResourceClient {
Perhaps a comment explaining why the client is necessary (or what a typical
OnUIResourceIdChanged() response might do).

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:21: 
Maybe a brief comment or two about the purpose for this class?

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:36: cc::UIResourceId id();
Looks like |id()| can be inline and const. I would move it below other public
methods.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:37: void Load();
It's not clear at first glance when |Load()| should be called? Or should this
method actually be private? If it should be public, maybe a note about what it
does and when to call it.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:38: void
SetClient(SystemUIResourceClient* client);
Will the client change? Can it be set to NULL?

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:11: #include
"base/observer_list.h"
Is observer_list.h used in the header? Could it be pulled in through the .cc?

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:12: #include
"content/browser/android/system_ui_resource.h"
Do we need the include? Is the forward declaration of SystemUIResource below not
sufficient?

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:36: void
RefreshResources();
A comment about what |RefreshResources()| accomplishes and when to call it would
be helpful.

[powei, 2014-07-23 21:56:09]

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/edge_effect.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/edge_effect.h:40:
EdgeEffect(ui::SystemUIResourceManager* resource_manager);
On 2014/07/23 18:17:44, jdduke wrote:
> Explicit.

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.cc:166: for (size_t i = 0; i <
EdgeEffect::EDGE_COUNT; ++i) {
On 2014/07/23 18:17:44, jdduke wrote:
> Nit: Remove braces.

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.cc:183: for (size_t i = 0; i <
EdgeEffect::EDGE_COUNT; ++i) {
On 2014/07/23 18:17:44, jdduke wrote:
> Nit: Remove braces.

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/overscroll_glow.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.h:31: // the effect will remain
deactivated until explicitly enabled.
On 2014/07/23 18:17:45, jdduke wrote:
> You'll want to update the comment (specifically about |enabled_|).

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.h:33: ui::SystemUIResourceManager*
resource_manager);
On 2014/07/23 18:17:44, jdduke wrote:
> It looks like the factory method doesn't really serve much purpose anymore as
it
> will always return something non-null.  However it's probably fine to keep it.

Acknowledged.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/overscroll_glow.h:73:
OverscrollGlow(ui::SystemUIResourceManager* resource_manager);
On 2014/07/23 18:17:45, jdduke wrote:
> Explicit.

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/system_ui_resource.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:16: class SystemUIResourceClient {
On 2014/07/23 18:17:45, jdduke wrote:
> Perhaps a comment explaining why the client is necessary (or what a typical
> OnUIResourceIdChanged() response might do).

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:21: 
On 2014/07/23 18:17:45, jdduke wrote:
> Maybe a brief comment or two about the purpose for this class?

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:36: cc::UIResourceId id();
On 2014/07/23 18:17:45, jdduke wrote:
> Looks like |id()| can be inline and const. I would move it below other public
> methods.

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:37: void Load();
On 2014/07/23 18:17:45, jdduke wrote:
> It's not clear at first glance when |Load()| should be called? Or should this
> method actually be private? If it should be public, maybe a note about what it
> does and when to call it.

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource.h:38: void
SetClient(SystemUIResourceClient* client);
On 2014/07/23 18:17:45, jdduke wrote:
> Will the client change? Can it be set to NULL?

The client will not change.  It can be set to NULL when the client goes out of
scope.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
File content/browser/android/system_ui_resource_manager_impl.h (right):

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:11: #include
"base/observer_list.h"
On 2014/07/23 18:17:45, jdduke wrote:
> Is observer_list.h used in the header? Could it be pulled in through the .cc?

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:12: #include
"content/browser/android/system_ui_resource.h"
On 2014/07/23 18:17:45, jdduke wrote:
> Do we need the include? Is the forward declaration of SystemUIResource below
not
> sufficient?

Done.

https://codereview.chromium.org/377013002/diff/250001/content/browser/android...
content/browser/android/system_ui_resource_manager_impl.h:36: void
RefreshResources();
On 2014/07/23 18:17:45, jdduke wrote:
> A comment about what |RefreshResources()| accomplishes and when to call it
would
> be helpful.

Done.

[jdduke (slow), 2014-07-28 20:45:38]

content/browser/android/edge_effect.{cc,h} and
content/browser/android/overscroll_glow.{cc,h} lgtm. You might want another pair
of eyes to review SystemUIResourceManagerImpl as well.

https://codereview.chromium.org/377013002/diff/310001/content/browser/android...
File content/browser/android/edge_effect.cc (right):

https://codereview.chromium.org/377013002/diff/310001/content/browser/android...
content/browser/android/edge_effect.cc:172:
parent->AddChild(ui_resource_layer_);
Do we need a null check on |parent|? Otherwise we should DCHECK.

https://codereview.chromium.org/377013002/diff/310001/content/browser/android...
File content/browser/android/system_ui_resource.cc (right):

https://codereview.chromium.org/377013002/diff/310001/content/browser/android...
content/browser/android/system_ui_resource.cc:41: id_ = 0;
Is early return worthwhile here?

if (!id_)
  return;

https://codereview.chromium.org/377013002/diff/310001/content/browser/android...
content/browser/android/system_ui_resource.cc:57: client_ = client;
What if the client is set after Load has been called?

[powei, 2014-07-28 23:14:57]

Closing this as jdduke will pick up the remaining work.  Thanks!