Update the nine patch layer to use UI resources

Update the nine patch layer to use UI resources

The old nine-patch layer used priority resource manager for requesting textures.
This patch updates the nine-patch layer to use the UI resource manager. 
this patch clarifies the semantics of the aperture in both image and layer
The new semantics corresponds to existing logic on the android-side.

Changes have been made to UIResourceBitmap to use SkPixelRef as ref-counted
of the bitmap content.

The android-side changes:
https://gerrit-int.chromium.org/#/c/43103/

[Update 8/13/2013] Reverted due to "regression"
( https://code.google.com/p/chromium/issues/detail?id=290215 ): 
https://codereview.chromium.org/23620047/

[Update 8/13/2013] Reverted due to failed mac_gpu bot:
https://codereview.chromium.org/23740010/


BUG=276482, 276487, 290215
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222732

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223162

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224270

[powei, 2013-08-19 22:39:16]

Upstream changes for adding UI resources to nine-patch (clank_patch) layer. 
Please take a look.  Thanks.

[jdduke (slow), 2013-08-19 23:13:25]

https://codereview.chromium.org/22870016/diff/1/content/public/browser/androi...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/22870016/diff/1/content/public/browser/androi...
content/public/browser/android/compositor.h:88: // Generates a UIResource and
returns a UIResourceId.  May return 0
Why not have it take a UIResourceBitmap directly? That gives us a bit more
flexibility in how we use the interface; the JavaBitmap -> UIResourceBitmap
conversion can go in some other conveniently common place.

[powei, 2013-08-20 19:59:34]

+danakj, ccameron

PTAL

Upstreamed android-side code.  I probably need to work on test changes next. 
And rebase after ccameron's CL (https://codereview.chromium.org/22911024/)
lands.

Question to ccameron and danakj: can this land without handling tab
backgrounding for now?

Thanks all.

https://codereview.chromium.org/22870016/diff/1/content/public/browser/androi...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/22870016/diff/1/content/public/browser/androi...
content/public/browser/android/compositor.h:88: // Generates a UIResource and
returns a UIResourceId.  May return 0
On 2013/08/19 23:13:25, jdduke wrote:
> Why not have it take a UIResourceBitmap directly? That gives us a bit more
> flexibility in how we use the interface; the JavaBitmap -> UIResourceBitmap
> conversion can go in some other conveniently common place.

Very true.  Done.

[danakj, 2013-08-20 20:03:52]

chris can answer to backgrounding, but it sounded like no?

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:61: void NinePatchLayer::SetBitmap(UIResourceId
resource_id,
Please don't use function overloading for this, use different method names.

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:75: return resource_.get() && resource_->id();
&& Layer::DrawsContent()

Should/can we check the size of the resource here?

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:16: class ResourceUpdateQueue;
can remove this

[ccameron, 2013-08-20 20:07:45]

On 2013/08/20 20:03:52, danakj wrote:
> chris can answer to backgrounding, but it sounded like no?

It's really early in the release cycle, so as long as we agree to something and
that something seems simple enough to be at-least-mostly-done by the end of the
week, then I think it will be fine (the regression caused by this resource will
be so small that it won't be picked up -- that won't be the case for the linen
texture, but that's not ready yet).

[aelias_OOO_until_Jul13, 2013-08-22 04:55:52]

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:35: void SetBitmap(const SkBitmap& skbitmap,
gfx::Rect aperture);
Are there any users of this method?  If not, can we delete it?

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:40: bool fill_center,
Why is this fill_center value only provided for SetSharedBitmap and not the
other?

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource....
cc/resources/ui_resource.h:19: virtual gfx::Size GetSize() = 0;
Could we instead add a GetUIResourceSize(UIResourceID id) method to
LayerTreeHost and get rid of both these new UIResource and SharedUIResource
classes?

[powei, 2013-08-22 17:58:47]

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:61: void NinePatchLayer::SetBitmap(UIResourceId
resource_id,
On 2013/08/20 20:03:53, danakj wrote:
> Please don't use function overloading for this, use different method names.

Done.

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:75: return resource_.get() && resource_->id();
On 2013/08/20 20:03:53, danakj wrote:
> && Layer::DrawsContent()
> 
> Should/can we check the size of the resource here?

Done (on && Layer::DrawsContent()). I put in some dchecks to make sure that
empty bitmaps cannot be passed to UIResource.  Requesting empty textures crashes
the resource provider  I think.

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:16: class ResourceUpdateQueue;
On 2013/08/20 20:03:53, danakj wrote:
> can remove this

Done.

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:35: void SetBitmap(const SkBitmap& skbitmap,
gfx::Rect aperture);
On 2013/08/22 04:55:52, aelias wrote:
> Are there any users of this method?  If not, can we delete it?

This function has been hooked into webkit's compositor_bindings.  So there might
be or will be other users who expect this function.  

Removing this will quite a few files.  Maybe I can add a note that it's being
deprecated and prepare another CL to remove it?

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:40: bool fill_center,
On 2013/08/22 04:55:52, aelias wrote:
> Why is this fill_center value only provided for SetSharedBitmap and not the
> other?

See the reasoning for the SetBitmap function.  Default value for fill_center is
false since this functionality did not exist before.

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource....
cc/resources/ui_resource.h:19: virtual gfx::Size GetSize() = 0;
On 2013/08/22 04:55:52, aelias wrote:
> Could we instead add a GetUIResourceSize(UIResourceID id) method to
> LayerTreeHost and get rid of both these new UIResource and SharedUIResource
> classes?

I added this function.  But we might need to keep around these two classes for
now  because of the reason mentioned in the SetBitmap comment.

[danakj, 2013-08-22 18:06:10]

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:75: return resource_.get() && resource_->id();
On 2013/08/22 17:58:47, powei wrote:
> On 2013/08/20 20:03:53, danakj wrote:
> > && Layer::DrawsContent()
> > 
> > Should/can we check the size of the resource here?
> 
> Done (on && Layer::DrawsContent()). I put in some dchecks to make sure that
> empty bitmaps cannot be passed to UIResource.  Requesting empty textures
crashes
> the resource provider  I think.

Ok, but when DrawsContent() is false we can early out of some various
computation later on, which is helpful. If the resources are empty sized, then
this does not actually produce any quads, so we could set false here?

[danakj, 2013-08-22 18:11:59]

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:35: void SetBitmap(const SkBitmap& skbitmap,
gfx::Rect aperture);
On 2013/08/22 17:58:47, powei wrote:
> On 2013/08/22 04:55:52, aelias wrote:
> > Are there any users of this method?  If not, can we delete it?
> 
> This function has been hooked into webkit's compositor_bindings.  So there
might
> be or will be other users who expect this function.  
> 
> Removing this will quite a few files.  Maybe I can add a note that it's being
> deprecated and prepare another CL to remove it?

That'd be what ccameron recently did, if we can have the blink side use the new
stuff immediately, that'd save an iteration on the code there.

[danakj, 2013-08-22 18:12:47]

+ccameron

[ccameron, 2013-08-22 18:24:17]

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:35: void SetBitmap(const SkBitmap& skbitmap,
gfx::Rect aperture);
On 2013/08/22 18:12:00, danakj wrote:
> On 2013/08/22 17:58:47, powei wrote:
> > On 2013/08/22 04:55:52, aelias wrote:
> > > Are there any users of this method?  If not, can we delete it?
> > 
> > This function has been hooked into webkit's compositor_bindings.  So there
> might
> > be or will be other users who expect this function.  
> > 
> > Removing this will quite a few files.  Maybe I can add a note that it's
being
> > deprecated and prepare another CL to remove it?
> 
> That'd be what ccameron recently did, if we can have the blink side use the
new
> stuff immediately, that'd save an iteration on the code there.

Please leave this here for now.

[powei, 2013-08-23 01:37:15]

Updated the tests.  I opened a bug to start the discussion on releasing ui
resources for the nine patch layer, as suggested by ccameron@.  

https://code.google.com/p/chromium/issues/detail?id=276647

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:75: return resource_.get() && resource_->id();
On 2013/08/22 18:06:11, danakj wrote:
> On 2013/08/22 17:58:47, powei wrote:
> > On 2013/08/20 20:03:53, danakj wrote:
> > > && Layer::DrawsContent()
> > > 
> > > Should/can we check the size of the resource here?
> > 
> > Done (on && Layer::DrawsContent()). I put in some dchecks to make sure that
> > empty bitmaps cannot be passed to UIResource.  Requesting empty textures
> crashes
> > the resource provider  I think.
> 
> Ok, but when DrawsContent() is false we can early out of some various
> computation later on, which is helpful. If the resources are empty sized, then
> this does not actually produce any quads, so we could set false here?

I guess my assumption here is that either there is no resource
(resource_.get()==NULL) or the resource would be non-empty since creating an
empty UIResource should not be allowed.

[danakj, 2013-08-27 15:42:47]

Hi powei, I went through the patch and have some comments on it.

I'd like to understand the reasoning for the Shared vs Scoped UIResource
classes, I asked for some comments on the classes which I hope should help out.

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/11001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:75: return resource_.get() && resource_->id();
On 2013/08/23 01:37:15, powei wrote:
> On 2013/08/22 18:06:11, danakj wrote:
> > On 2013/08/22 17:58:47, powei wrote:
> > > On 2013/08/20 20:03:53, danakj wrote:
> > > > && Layer::DrawsContent()
> > > > 
> > > > Should/can we check the size of the resource here?
> > > 
> > > Done (on && Layer::DrawsContent()). I put in some dchecks to make sure
that
> > > empty bitmaps cannot be passed to UIResource.  Requesting empty textures
> > crashes
> > > the resource provider  I think.
> > 
> > Ok, but when DrawsContent() is false we can early out of some various
> > computation later on, which is helpful. If the resources are empty sized,
then
> > this does not actually produce any quads, so we could set false here?
> 
> I guess my assumption here is that either there is no resource
> (resource_.get()==NULL) or the resource would be non-empty since creating an
> empty UIResource should not be allowed.
> 

Ah okay, that makes sense, thanks!

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:72: return resource_.get() && resource_->id() &&
Layer::DrawsContent();
nit: no .get() needed for scoped_ptr as bool

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:81: return;
You can't early out, since we needs_push_properties_ = true at the bottom.

Can you instead do if {} else {}?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:53: gfx::Rect border, bool fill_center) {
One argument per line, lining up with the ( unless you wrap right after the (.
Or just clang-format maybe?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:65: fill_center_ = fill_center;
Should this function also cause damage so it is redrawn on the screen?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:77: if (!ui_resource_id_ ||
!layer_tree_impl())
layer_tree_impl is never null, it's set non-null in the layer's constructor and
never changed.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:95: // NinePatch border widths in layer space
nit: Period.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:96: int left_width = border_.x();
maybe prefixing all these variables with layer_ or image_ would help a lot?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:349: list->AppendInteger(border_.width());
Border is a rect, why not save it all? Also we have MathUtil::AsValue for
various geometry things.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.h:31: void set_ui_resource_id(UIResourceId uid)
{ ui_resource_id_ = uid; }
Should this be causing damage so the layer is drawn again?
NoteLayerPropertyChanged() when the id is changed?

Then, SetUIResourceId()?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl_unittest.cc:87: layer->SetLayout(bitmap_size,
aperture_rect, border, false);
should fill center be a parameter of NinePatchLayerLayoutTest so you can test it
too?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl_unittest.cc:133: TEST(NinePatchLayerImplTest,
VerifyDrawQuads) {
How about a few more tests that check different image/layer bound relationships,
and test fillcenter?

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_unittest.cc (left):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_unittest.cc:58: TEST_F(NinePatchLayerTest,
TriggerFullUploadOnceWhenChangingBitmap) {
It seems like this test should still be testing the upload stuff until that code
doesn't exist anymore?

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
File cc/resources/scoped_ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:19: class CC_EXPORT ScopedUIResource : public
UIResourceClient, public UIResource {
Can you add a comment explaining what this class is?

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:26: // UIResourceClient
// UIResourceClient implementation.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:30: 
// UIResource implementation.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
File cc/resources/shared_ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
cc/resources/shared_ui_resource.h:18: class CC_EXPORT SharedUIResource : public
UIResource {
And can you add a comment here as well, such that reading this and the
ScopedUIResource comment, it would be clear when to use which one and why both
exist?

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
cc/resources/shared_ui_resource.h:24: // UIResource
// UIResource implementation.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
cc/resources/shared_ui_resource.h:25: virtual UIResourceId id() OVERRIDE {
return id_; }
since this is virtual, the definition needs to go in a .cc doesn't it?

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource....
cc/resources/ui_resource.h:10: #include "cc/resources/ui_resource_bitmap.h"
This is a lot of headers for one pure virtual function that returns a
UIReourceId. Can you trim these down to what you need?

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource_...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource_...
cc/resources/ui_resource_bitmap.h:50: CC_EXPORT scoped_refptr<UIResourceBitmap>
CreateUIResourceBitmapFromSkBitmap(
Can this be a static method in the UIResourceBitmap class? Could just be
CreateFromSkBitmap()?

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1158: // Deletes a UI resource.  May safely be
called more than once.
Remove this?

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1160: UIResourceClientMap::iterator iter =
ui_resource_client_map_.find(uid);
nit: s/iter/it/

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1168: iter->second->GetBitmap(uid, resource_lost);
can you use a local variable const& or non-const& of the it->second value so we
can see it's type explicitly here?

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:272: // Need to
recreate the UI resources because a new LTH has been created.
LTH -> LayerTreeHost

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.h:126: typedef
base::hash_map<cc::UIResourceId, cc::ScopedUIResource*>
Can you use cc/base/scoped_ptr_hash_map for this? That'd give you more safety.

https://codereview.chromium.org/22870016/diff/44001/content/public/browser/an...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/22870016/diff/44001/content/public/browser/an...
content/public/browser/android/compositor.h:89: // Generates a UIResource and
returns a UIResourceId.  May return 0
nit: Period.

[aelias_OOO_until_Jul13, 2013-08-27 18:40:49]

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource....
cc/resources/ui_resource.h:19: virtual gfx::Size GetSize() = 0;
On 2013/08/22 17:58:47, powei wrote:
> On 2013/08/22 04:55:52, aelias wrote:
> > Could we instead add a GetUIResourceSize(UIResourceID id) method to
> > LayerTreeHost and get rid of both these new UIResource and SharedUIResource
> > classes?
> 
> I added this function.  But we might need to keep around these two classes for
> now  because of the reason mentioned in the SetBitmap comment.

I think we should delete UIResource and SharedUIResource, they're entirely
redundant abstractions.  If it needs a Blink patch to adjust the interface on
that end then so be it.

[powei, 2013-08-27 19:04:57]

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource....
cc/resources/ui_resource.h:19: virtual gfx::Size GetSize() = 0;
On 2013/08/27 18:40:49, aelias wrote:
> On 2013/08/22 17:58:47, powei wrote:
> > On 2013/08/22 04:55:52, aelias wrote:
> > > Could we instead add a GetUIResourceSize(UIResourceID id) method to
> > > LayerTreeHost and get rid of both these new UIResource and
SharedUIResource
> > > classes?
> > 
> > I added this function.  But we might need to keep around these two classes
for
> > now  because of the reason mentioned in the SetBitmap comment.
> 
> I think we should delete UIResource and SharedUIResource, they're entirely
> redundant abstractions.  If it needs a Blink patch to adjust the interface on
> that end then so be it.

Yes, I agree they seem to be redundant classes.   I will delete them.  But would
you object to me moving some of these abstraction to private classes within
NinePatchLayer?  

My main goal in creating these two classes is to have a single resource variable
(resource_) in NinePatchLayer for two resource-attaching interfaces (SetBitmap &
SetSharedBitmap) instead of introducing more logic to keep around both a
UIResourceId and a ScopedUIResource.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:72: return resource_.get() && resource_->id() &&
Layer::DrawsContent();
On 2013/08/27 15:42:48, danakj wrote:
> nit: no .get() needed for scoped_ptr as bool

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:81: return;
On 2013/08/27 15:42:48, danakj wrote:
> You can't early out, since we needs_push_properties_ = true at the bottom.
> 
> Can you instead do if {} else {}?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:53: gfx::Rect border, bool fill_center) {
On 2013/08/27 15:42:48, danakj wrote:
> One argument per line, lining up with the ( unless you wrap right after the (.
> Or just clang-format maybe?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:65: fill_center_ = fill_center;
On 2013/08/27 15:42:48, danakj wrote:
> Should this function also cause damage so it is redrawn on the screen?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.h:31: void set_ui_resource_id(UIResourceId uid)
{ ui_resource_id_ = uid; }
On 2013/08/27 15:42:48, danakj wrote:
> Should this be causing damage so the layer is drawn again?
> NoteLayerPropertyChanged() when the id is changed?
> 
> Then, SetUIResourceId()?

Done.  I remember scrollbar_layer_impl did not have this requirement.  Was this
a recent change?

[danakj, 2013-08-27 19:07:12]

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.h:31: void set_ui_resource_id(UIResourceId uid)
{ ui_resource_id_ = uid; }
On 2013/08/27 19:04:57, powei wrote:
> On 2013/08/27 15:42:48, danakj wrote:
> > Should this be causing damage so the layer is drawn again?
> > NoteLayerPropertyChanged() when the id is changed?
> > 
> > Then, SetUIResourceId()?
> 
> Done.  I remember scrollbar_layer_impl did not have this requirement.  Was
this
> a recent change?

No, it was a bug in scrollbar layer :)

[aelias_OOO_until_Jul13, 2013-08-27 19:07:23]

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/22001/cc/resources/ui_resource....
cc/resources/ui_resource.h:19: virtual gfx::Size GetSize() = 0;
On 2013/08/27 19:04:57, powei wrote:
> On 2013/08/27 18:40:49, aelias wrote:
> > On 2013/08/22 17:58:47, powei wrote:
> > > On 2013/08/22 04:55:52, aelias wrote:
> > > > Could we instead add a GetUIResourceSize(UIResourceID id) method to
> > > > LayerTreeHost and get rid of both these new UIResource and
> SharedUIResource
> > > > classes?
> > > 
> > > I added this function.  But we might need to keep around these two classes
> for
> > > now  because of the reason mentioned in the SetBitmap comment.
> > 
> > I think we should delete UIResource and SharedUIResource, they're entirely
> > redundant abstractions.  If it needs a Blink patch to adjust the interface
on
> > that end then so be it.
> 
> Yes, I agree they seem to be redundant classes.   I will delete them.  But
would
> you object to me moving some of these abstraction to private classes within
> NinePatchLayer?  
> 
> My main goal in creating these two classes is to have a single resource
variable
> (resource_) in NinePatchLayer for two resource-attaching interfaces (SetBitmap
&
> SetSharedBitmap) instead of introducing more logic to keep around both a
> UIResourceId and a ScopedUIResource.  

Yeah, it's probably more acceptable as a private class.

[powei, 2013-08-27 22:39:27]

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:77: if (!ui_resource_id_ ||
!layer_tree_impl())
On 2013/08/27 15:42:48, danakj wrote:
> layer_tree_impl is never null, it's set non-null in the layer's constructor
and
> never changed.

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:95: // NinePatch border widths in layer space
On 2013/08/27 15:42:48, danakj wrote:
> nit: Period.

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:96: int left_width = border_.x();
On 2013/08/27 15:42:48, danakj wrote:
> maybe prefixing all these variables with layer_ or image_ would help a lot?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:349: list->AppendInteger(border_.width());
On 2013/08/27 15:42:48, danakj wrote:
> Border is a rect, why not save it all? Also we have MathUtil::AsValue for
> various geometry things.

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl_unittest.cc:87: layer->SetLayout(bitmap_size,
aperture_rect, border, false);
On 2013/08/27 15:42:48, danakj wrote:
> should fill center be a parameter of NinePatchLayerLayoutTest so you can test
it
> too?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_unittest.cc (left):

https://codereview.chromium.org/22870016/diff/44001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_unittest.cc:58: TEST_F(NinePatchLayerTest,
TriggerFullUploadOnceWhenChangingBitmap) {
On 2013/08/27 15:42:48, danakj wrote:
> It seems like this test should still be testing the upload stuff until that
code
> doesn't exist anymore?

The previous upload checks no longer apply.   I'm not sure what a test on upload
would contain.  Do you mean I should check whether LTH/LTHI did indeed create a
resource for each SetBitmap call?

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
File cc/resources/scoped_ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:19: class CC_EXPORT ScopedUIResource : public
UIResourceClient, public UIResource {
On 2013/08/27 15:42:48, danakj wrote:
> Can you add a comment explaining what this class is?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:26: // UIResourceClient
On 2013/08/27 15:42:48, danakj wrote:
> // UIResourceClient implementation.

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:30: 
On 2013/08/27 15:42:48, danakj wrote:
> // UIResource implementation.

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
File cc/resources/shared_ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
cc/resources/shared_ui_resource.h:18: class CC_EXPORT SharedUIResource : public
UIResource {
On 2013/08/27 15:42:48, danakj wrote:
> And can you add a comment here as well, such that reading this and the
> ScopedUIResource comment, it would be clear when to use which one and why both
> exist?

Done.  Removed.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
cc/resources/shared_ui_resource.h:24: // UIResource
On 2013/08/27 15:42:48, danakj wrote:
> // UIResource implementation.

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/shared_ui_re...
cc/resources/shared_ui_resource.h:25: virtual UIResourceId id() OVERRIDE {
return id_; }
On 2013/08/27 15:42:48, danakj wrote:
> since this is virtual, the definition needs to go in a .cc doesn't it?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource.h
File cc/resources/ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource....
cc/resources/ui_resource.h:10: #include "cc/resources/ui_resource_bitmap.h"
On 2013/08/27 15:42:48, danakj wrote:
> This is a lot of headers for one pure virtual function that returns a
> UIReourceId. Can you trim these down to what you need?

Done.  Removed.

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource_...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/44001/cc/resources/ui_resource_...
cc/resources/ui_resource_bitmap.h:50: CC_EXPORT scoped_refptr<UIResourceBitmap>
CreateUIResourceBitmapFromSkBitmap(
On 2013/08/27 15:42:48, danakj wrote:
> Can this be a static method in the UIResourceBitmap class? Could just be
> CreateFromSkBitmap()?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1158: // Deletes a UI resource.  May safely be
called more than once.
On 2013/08/27 15:42:48, danakj wrote:
> Remove this?

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1160: UIResourceClientMap::iterator iter =
ui_resource_client_map_.find(uid);
On 2013/08/27 15:42:48, danakj wrote:
> nit: s/iter/it/

Done.

https://codereview.chromium.org/22870016/diff/44001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1168: iter->second->GetBitmap(uid, resource_lost);
On 2013/08/27 15:42:48, danakj wrote:
> can you use a local variable const& or non-const& of the it->second value so
we
> can see it's type explicitly here?

Done.

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:272: // Need to
recreate the UI resources because a new LTH has been created.
On 2013/08/27 15:42:48, danakj wrote:
> LTH -> LayerTreeHost

Done.

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.h:126: typedef
base::hash_map<cc::UIResourceId, cc::ScopedUIResource*>
On 2013/08/27 15:42:48, danakj wrote:
> Can you use cc/base/scoped_ptr_hash_map for this? That'd give you more safety.

Including scoped_ptr_hash_map violated a dep rule in content/DEPS.  There's a
comment that says to move scoped_ptr_hash_map to base.  Does anyone know if this
is safe to do?  Might have to start a new CL to do that.

https://codereview.chromium.org/22870016/diff/44001/content/public/browser/an...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/22870016/diff/44001/content/public/browser/an...
content/public/browser/android/compositor.h:89: // Generates a UIResource and
returns a UIResourceId.  May return 0
On 2013/08/27 15:42:48, danakj wrote:
> nit: Period.

Done.

[danakj, 2013-08-27 22:43:50]

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.h:126: typedef
base::hash_map<cc::UIResourceId, cc::ScopedUIResource*>
On 2013/08/27 22:39:28, powei wrote:
> On 2013/08/27 15:42:48, danakj wrote:
> > Can you use cc/base/scoped_ptr_hash_map for this? That'd give you more
safety.
> 
> Including scoped_ptr_hash_map violated a dep rule in content/DEPS.  There's a
> comment that says to move scoped_ptr_hash_map to base.  Does anyone know if
this
> is safe to do?  Might have to start a new CL to do that.

Oh, clever DEPS rules. We should move it, just no one has. The hash map should
be perhaps the least controversial of them.

https://code.google.com/p/chromium/issues/detail?id=156199

[powei, 2013-08-28 20:38:05]

PTAL.  I deleted UIResource and SharedUIResource and moved some of the code to
NinePatchLayer.  Also updated the impl test to include fill_center.  Also added
more DCHECK to ensure that degenerate cases, where the border touches the
bounds, or where the apertures touches the image boundary, cannot happen.  

The switch to ScopedPtrHashMap will have to wait a little longer until
https://codereview.chromium.org/23653002/ lands.  

Thanks.

On 2013/08/27 22:43:50, danakj wrote:
>
https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
> File content/browser/renderer_host/compositor_impl_android.h (right):
> 
>
https://codereview.chromium.org/22870016/diff/44001/content/browser/renderer_...
> content/browser/renderer_host/compositor_impl_android.h:126: typedef
> base::hash_map<cc::UIResourceId, cc::ScopedUIResource*>
> On 2013/08/27 22:39:28, powei wrote:
> > On 2013/08/27 15:42:48, danakj wrote:
> > > Can you use cc/base/scoped_ptr_hash_map for this? That'd give you more
> safety.
> > 
> > Including scoped_ptr_hash_map violated a dep rule in content/DEPS.  There's
a
> > comment that says to move scoped_ptr_hash_map to base.  Does anyone know if
> this
> > is safe to do?  Might have to start a new CL to do that.
> 
> Oh, clever DEPS rules. We should move it, just no one has. The hash map should
> be perhaps the least controversial of them.
> 
> https://code.google.com/p/chromium/issues/detail?id=156199

[aelias_OOO_until_Jul13, 2013-08-28 22:28:28]

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:82: image_aperture_ = aperture;
Please add an early return if all three of the arguments are identical to
previously.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:92: return resource_&& resource_->id() &&
Layer::DrawsContent();
nit: space between resource_&&

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:110: // NinePatchLayer must push properties every
commit to make sure
Can you fix this problem as part of this patch?

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:20: class Resource {
Rename this UIResourceHolder and put it in the below private: area.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:22: virtual UIResourceId id() = 0;
This should have a virtual destructor as well.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:25: class ScopedResource : public Resource {
These two implementation classes can go in the .cc file in an anonymous
namespace.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:55: void SetBorder(gfx::Rect border);
Please write a comment explaining this.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:65: void SetSharedBitmap(UIResourceId resource_id,
Rename to SetUIResourceId

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:92: bool damaged = false;
Please reformulate this as:

if (image_bounds_ == image_bounds && image_aperture_ == aperture && border_ ==
border && fill_center_ == fill_center)
  return;

Then unconditionally assign all and NoteLayerPropertyChanged.

[powei, 2013-08-30 05:17:15]

New patch with some comments addressed.  Others need further input.  Thanks.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:82: image_aperture_ = aperture;
On 2013/08/28 22:28:29, aelias wrote:
> Please add an early return if all three of the arguments are identical to
> previously.

Done.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:92: return resource_&& resource_->id() &&
Layer::DrawsContent();
On 2013/08/28 22:28:29, aelias wrote:
> nit: space between resource_&&

Done.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:110: // NinePatchLayer must push properties every
commit to make sure
On 2013/08/28 22:28:29, aelias wrote:
> Can you fix this problem as part of this patch?

Not 100% on this, but I assume needs_push_properties_ only needs to be set when
parameters have changed.  I called SetNeedsPushProperties() in both SetBitmap
and SetUIResourceId.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:20: class Resource {
On 2013/08/28 22:28:29, aelias wrote:
> Rename this UIResourceHolder and put it in the below private: area.

Renamed it.  

Correct me if I'm wrong, but if the two implementations are to be contained in
the .cc, then UIResourceHolder needs to be public.  Or all three class
definitions can be private as before.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:22: virtual UIResourceId id() = 0;
On 2013/08/28 22:28:29, aelias wrote:
> This should have a virtual destructor as well.

Done.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:25: class ScopedResource : public Resource {
On 2013/08/28 22:28:29, aelias wrote:
> These two implementation classes can go in the .cc file in an anonymous
> namespace.

Done.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:55: void SetBorder(gfx::Rect border);
On 2013/08/28 22:28:29, aelias wrote:
> Please write a comment explaining this.

Done.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:65: void SetSharedBitmap(UIResourceId resource_id,
On 2013/08/28 22:28:29, aelias wrote:
> Rename to SetUIResourceId

Done.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer_impl.cc:92: bool damaged = false;
On 2013/08/28 22:28:29, aelias wrote:
> Please reformulate this as:
> 
> if (image_bounds_ == image_bounds && image_aperture_ == aperture && border_ ==
> border && fill_center_ == fill_center)
>   return;
> 
> Then unconditionally assign all and NoteLayerPropertyChanged.

Done.

[aelias_OOO_until_Jul13, 2013-08-30 06:20:40]

Getting there.  My most substantive comment below is to switch UIResourceBitmap
to hold an SkPixelRef, which I now think is what my original design should've
been in the first place.

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/74018/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:110: // NinePatchLayer must push properties every
commit to make sure
On 2013/08/30 05:17:15, powei wrote:
> On 2013/08/28 22:28:29, aelias wrote:
> > Can you fix this problem as part of this patch?
> 
> Not 100% on this, but I assume needs_push_properties_ only needs to be set
when
> parameters have changed.  I called SetNeedsPushProperties() in both SetBitmap
> and SetUIResourceId.

Looks good.  I think just some ResourceProvider subtleties caused this in the
old implementation, but we avoid those now.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:17: class ScopedUIResourceHolder : public
cc::NinePatchLayer::UIResourceHolder {
I'm pretty sure you can put the namespace {} inside the namespace cc {} and then
you can avoid all these cc:: boilerplate.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:58: NinePatchLayer::NinePatchLayer() :
fill_center_(true) {}
Should this default to false?  You're setting it to false in SetBitmap.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:47: bool fill_center);
Put this in a separate call SetFillCenter(bool); so it can be used with
SetBitmap as well (without needing to edit the Blink API for an existing
function which I understand is annoying to do).

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:63: scoped_ptr<UIResourceHolder> resource_;
Rename this to ui_resource_holder_

https://codereview.chromium.org/22870016/diff/87001/cc/resources/scoped_ui_re...
File cc/resources/scoped_ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:22: // initially given bitmap, regardless of
whether the resquest was because of
typo: "resquest"

https://codereview.chromium.org/22870016/diff/87001/cc/resources/ui_resource_...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/resources/ui_resource_...
cc/resources/ui_resource_bitmap.cc:39: memcpy(dst_pixels, src_pixels,
size.GetArea() * 4);
This isn't correct because there may be unused pixels at the end of each row of
the SkBitmap.  Using SkBitmap::copyPixelsTo would address this.

I don't like unnecessary copies in the first place though... looking again at
Skia includes, I think we can switch to holding an SkPixelRef* instead of a
uint_8[].  SkPixelRef looks extensible enough that we'll be able to put an ETC1
texture in there.  Then callers will be able to just do SkBitmap::getPixelRef().
 Could you try making that change?

Then UIResourceBitmap would no longer be refcounted itself because Skia takes
care of it.

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1166: client->GetBitmap(uid, resource_lost);
This operation may be expensive, I don't think we should call it for a
cheap-sounding getter.  Could you store a struct in the client_map_ instead
containing the size as well as the pointer?

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:286: virtual gfx::Size GetUIResourceSize(UIResourceId
id);
const

https://codereview.chromium.org/22870016/diff/87001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/22870016/diff/87001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:241:
scoped_ptr<cc::ScopedUIResource> resource(it->second);
Yuck.  Now I see why you depended on scoped_ptr_hash_map :).  Please switch this
code over now that it's landed.

https://codereview.chromium.org/22870016/diff/87001/content/public/browser/an...
File content/public/browser/android/compositor_client.h (right):

https://codereview.chromium.org/22870016/diff/87001/content/public/browser/an...
content/public/browser/android/compositor_client.h:29: // Invalidates the UI
Resources in the client.
I don't understand what "Invalidate" means in this context, could you explain?

[aelias_OOO_until_Jul13, 2013-08-30 18:09:54]

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:51: virtual UIResourceId id() = 0;
Thinking about it further I think you're on to something with this and we should
make it public to avoid every layer type needing to reimplement this themselves.

How about this: get rid of ScopedUIResource and have this instead:

class UIResource : public base::RefCounted<UIResource> {
  public:
    virtual UIResourceId id() = 0;
    virtual ~UIResource();
};

class ManagedUIResource : public UIResource {
  // basically same implementation as ScopedUIResource
};

class UnmanagedUIResource : public UIResource {
  // just holds an ID
};

Then NinePatchLayer would just have void SetUIResource(scoped_refptr<UIResource>
uiresource, gfx::Rect, bool);  .  And change the webkit/compositor_bindings
NinePatchLayer shim to call that instead of SetBitmap.

[aelias_OOO_until_Jul13, 2013-08-30 19:02:40]

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:51: virtual UIResourceId id() = 0;
On 2013/08/30 18:09:54, aelias wrote:
> Thinking about it further I think you're on to something with this and we
should
> make it public to avoid every layer type needing to reimplement this
themselves.
> 
> How about this: get rid of ScopedUIResource and have this instead:
> 
> class UIResource : public base::RefCounted<UIResource> {
>   public:
>     virtual UIResourceId id() = 0;
>     virtual ~UIResource();
> };
> 
> class ManagedUIResource : public UIResource {
>   // basically same implementation as ScopedUIResource
> };
> 
> class UnmanagedUIResource : public UIResource {
>   // just holds an ID
> };
> 
> Then NinePatchLayer would just have void
SetUIResource(scoped_refptr<UIResource>
> uiresource, gfx::Rect, bool);  .  And change the webkit/compositor_bindings
> NinePatchLayer shim to call that instead of SetBitmap.

Powei pointed out offline that this has problems with LayerTreeHost changes, so
let's not do this yet.

[powei, 2013-09-03 23:36:05]

new patch.  PTAL.  thanks.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:17: class ScopedUIResourceHolder : public
cc::NinePatchLayer::UIResourceHolder {
On 2013/08/30 06:20:41, aelias wrote:
> I'm pretty sure you can put the namespace {} inside the namespace cc {} and
then
> you can avoid all these cc:: boilerplate.

Done.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.cc:58: NinePatchLayer::NinePatchLayer() :
fill_center_(true) {}
On 2013/08/30 06:20:41, aelias wrote:
> Should this default to false?  You're setting it to false in SetBitmap.

Done.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_layer.h
File cc/layers/nine_patch_layer.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:47: bool fill_center);
On 2013/08/30 06:20:41, aelias wrote:
> Put this in a separate call SetFillCenter(bool); so it can be used with
> SetBitmap as well (without needing to edit the Blink API for an existing
> function which I understand is annoying to do).

Done.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:51: virtual UIResourceId id() = 0;
On 2013/08/30 19:02:40, aelias wrote:
> On 2013/08/30 18:09:54, aelias wrote:
> > Thinking about it further I think you're on to something with this and we
> should
> > make it public to avoid every layer type needing to reimplement this
> themselves.
> > 
> > How about this: get rid of ScopedUIResource and have this instead:
> > 
> > class UIResource : public base::RefCounted<UIResource> {
> >   public:
> >     virtual UIResourceId id() = 0;
> >     virtual ~UIResource();
> > };
> > 
> > class ManagedUIResource : public UIResource {
> >   // basically same implementation as ScopedUIResource
> > };
> > 
> > class UnmanagedUIResource : public UIResource {
> >   // just holds an ID
> > };
> > 
> > Then NinePatchLayer would just have void
> SetUIResource(scoped_refptr<UIResource>
> > uiresource, gfx::Rect, bool);  .  And change the webkit/compositor_bindings
> > NinePatchLayer shim to call that instead of SetBitmap.
> 
> Powei pointed out offline that this has problems with LayerTreeHost changes,
so
> let's not do this yet.

Left it as is for now.

https://codereview.chromium.org/22870016/diff/87001/cc/layers/nine_patch_laye...
cc/layers/nine_patch_layer.h:63: scoped_ptr<UIResourceHolder> resource_;
On 2013/08/30 06:20:41, aelias wrote:
> Rename this to ui_resource_holder_

Done.

https://codereview.chromium.org/22870016/diff/87001/cc/resources/scoped_ui_re...
File cc/resources/scoped_ui_resource.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/resources/scoped_ui_re...
cc/resources/scoped_ui_resource.h:22: // initially given bitmap, regardless of
whether the resquest was because of
On 2013/08/30 06:20:41, aelias wrote:
> typo: "resquest"

Done.

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1166: client->GetBitmap(uid, resource_lost);
On 2013/08/30 06:20:41, aelias wrote:
> This operation may be expensive, I don't think we should call it for a
> cheap-sounding getter.  Could you store a struct in the client_map_ instead
> containing the size as well as the pointer?

Done.

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/22870016/diff/87001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:286: virtual gfx::Size GetUIResourceSize(UIResourceId
id);
On 2013/08/30 06:20:41, aelias wrote:
> const

Done.

https://codereview.chromium.org/22870016/diff/87001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/22870016/diff/87001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:241:
scoped_ptr<cc::ScopedUIResource> resource(it->second);
On 2013/08/30 06:20:41, aelias wrote:
> Yuck.  Now I see why you depended on scoped_ptr_hash_map :).  Please switch
this
> code over now that it's landed.

Done.

https://codereview.chromium.org/22870016/diff/87001/content/public/browser/an...
File content/public/browser/android/compositor_client.h (right):

https://codereview.chromium.org/22870016/diff/87001/content/public/browser/an...
content/public/browser/android/compositor_client.h:29: // Invalidates the UI
Resources in the client.
On 2013/08/30 06:20:41, aelias wrote:
> I don't understand what "Invalidate" means in this context, could you explain?

Done. Renamed to UIResourcesAreInvalid

[aelias_OOO_until_Jul13, 2013-09-04 03:29:31]

https://codereview.chromium.org/22870016/diff/87001/cc/resources/ui_resource_...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/resources/ui_resource_...
cc/resources/ui_resource_bitmap.cc:39: memcpy(dst_pixels, src_pixels,
size.GetArea() * 4);
On 2013/08/30 06:20:41, aelias wrote:
> This isn't correct because there may be unused pixels at the end of each row
of
> the SkBitmap.  Using SkBitmap::copyPixelsTo would address this.
> 
> I don't like unnecessary copies in the first place though... looking again at
> Skia includes, I think we can switch to holding an SkPixelRef* instead of a
> uint_8[].  SkPixelRef looks extensible enough that we'll be able to put an
ETC1
> texture in there.  Then callers will be able to just do
SkBitmap::getPixelRef().
>  Could you try making that change?
> 
> Then UIResourceBitmap would no longer be refcounted itself because Skia takes
> care of it.

You didn't address this comment in your last update, please follow up on it.

[powei, 2013-09-05 18:16:46]

UIResourceBitmap is changed to hold SkPixelRef.  Rebasing is needed but waiting
on android-side to roll. PTAL in the meantime.  thanks.

https://codereview.chromium.org/22870016/diff/87001/cc/resources/ui_resource_...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/87001/cc/resources/ui_resource_...
cc/resources/ui_resource_bitmap.cc:39: memcpy(dst_pixels, src_pixels,
size.GetArea() * 4);
On 2013/09/04 03:29:32, aelias wrote:
> On 2013/08/30 06:20:41, aelias wrote:
> > This isn't correct because there may be unused pixels at the end of each row
> of
> > the SkBitmap.  Using SkBitmap::copyPixelsTo would address this.
> > 
> > I don't like unnecessary copies in the first place though... looking again
at
> > Skia includes, I think we can switch to holding an SkPixelRef* instead of a
> > uint_8[].  SkPixelRef looks extensible enough that we'll be able to put an
> ETC1
> > texture in there.  Then callers will be able to just do
> SkBitmap::getPixelRef().
> >  Could you try making that change?
> > 
> > Then UIResourceBitmap would no longer be refcounted itself because Skia
takes
> > care of it.
> 
> You didn't address this comment in your last update, please follow up on it.

Done.  Sorry about the delay.  Got lost in the fold after the long weekend.

[jdduke (slow), 2013-09-05 18:39:53]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:50: SkPixelRef* pixel_ref_;
Hmm, if we're going to use an SkPixelRef backing, can we not simply go ahead and
use an SkBitmap, rather than a naked ref? Copying the SkBitmap will do the right
thing if the source is immutable, and if not we can make the copy (then set
immutable)... I'm not sure, but it might make some of the
bookkeeping/refcounting simpler.

[aelias_OOO_until_Jul13, 2013-09-05 18:42:31]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:50: SkPixelRef* pixel_ref_;
On 2013/09/05 18:39:53, jdduke wrote:
> Hmm, if we're going to use an SkPixelRef backing, can we not simply go ahead
and
> use an SkBitmap, rather than a naked ref? Copying the SkBitmap will do the
right
> thing if the source is immutable, and if not we can make the copy (then set
> immutable)... I'm not sure, but it might make some of the
> bookkeeping/refcounting simpler.

No, because we intend to support ETC1 compressed bitmaps.  These aren't
supported by SkBitmap and it would be unnecessarily hard for them to be (e.g.
the implementation of getPixel(x, y) would be complicated).

[jdduke (slow), 2013-09-05 18:51:04]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:50: SkPixelRef* pixel_ref_;
On 2013/09/05 18:42:32, aelias wrote:
> On 2013/09/05 18:39:53, jdduke wrote:
> > Hmm, if we're going to use an SkPixelRef backing, can we not simply go ahead
> and
> > use an SkBitmap, rather than a naked ref? Copying the SkBitmap will do the
> right
> > thing if the source is immutable, and if not we can make the copy (then set
> > immutable)... I'm not sure, but it might make some of the
> > bookkeeping/refcounting simpler.
> 
> No, because we intend to support ETC1 compressed bitmaps.  These aren't
> supported by SkBitmap and it would be unnecessarily hard for them to be (e.g.
> the implementation of getPixel(x, y) would be complicated).

I see, though I wasn't proposing we expose the SkBitmap, rather simply use it as
the ref-counting handle.  The user would still supply an SkPixelRef, but
internally it would be contained by an SkBitmap... maybe not worth it?

[danakj, 2013-09-05 19:23:37]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:33: UIResourceBitmap(SkPixelRef* pixel_ref,
Is this passing ownership with a raw pointer? If so, please don't.

[powei, 2013-09-05 20:37:34]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:33: UIResourceBitmap(SkPixelRef* pixel_ref,
On 2013/09/05 19:23:38, danakj wrote:
> Is this passing ownership with a raw pointer? If so, please don't.

SkPixelRef is ref-counted, so this is not taking ownership but increasing the
ref-count.  I could mention in a comment that SkPixelRef passed in here should
already be marked immutable.  There's a DCHECK on this already.

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:50: SkPixelRef* pixel_ref_;
On 2013/09/05 18:51:05, jdduke wrote:
> On 2013/09/05 18:42:32, aelias wrote:
> > On 2013/09/05 18:39:53, jdduke wrote:
> > > Hmm, if we're going to use an SkPixelRef backing, can we not simply go
ahead
> > and
> > > use an SkBitmap, rather than a naked ref? Copying the SkBitmap will do the
> > right
> > > thing if the source is immutable, and if not we can make the copy (then
set
> > > immutable)... I'm not sure, but it might make some of the
> > > bookkeeping/refcounting simpler.
> > 
> > No, because we intend to support ETC1 compressed bitmaps.  These aren't
> > supported by SkBitmap and it would be unnecessarily hard for them to be
(e.g.
> > the implementation of getPixel(x, y) would be complicated).
> 
> I see, though I wasn't proposing we expose the SkBitmap, rather simply use it
as
> the ref-counting handle.  The user would still supply an SkPixelRef, but
> internally it would be contained by an SkBitmap... maybe not worth it?

I'm a little worried that might cause some confusion down the road (using
SkBitmap not for its intended purpose but as a container for SkPixelRef).  That
said, please check the actual implementation to make sure that my
ref-counting/bookkeeping is correct.

[danakj, 2013-09-05 20:38:56]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:33: UIResourceBitmap(SkPixelRef* pixel_ref,
On 2013/09/05 20:37:35, powei wrote:
> On 2013/09/05 19:23:38, danakj wrote:
> > Is this passing ownership with a raw pointer? If so, please don't.
> 
> SkPixelRef is ref-counted, so this is not taking ownership but increasing the
> ref-count.  I could mention in a comment that SkPixelRef passed in here should
> already be marked immutable.  There's a DCHECK on this already.

Please use skia::RefPtr, and skia::AdoptRef() a reference as soon as you see the
raw pointer. Never store/pass raw pointers to skia ref counted things in
chromium.

[powei, 2013-09-05 21:34:07]

Patch up with skia::RefPtr changes.

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:33: UIResourceBitmap(SkPixelRef* pixel_ref,
On 2013/09/05 20:38:57, danakj wrote:
> On 2013/09/05 20:37:35, powei wrote:
> > On 2013/09/05 19:23:38, danakj wrote:
> > > Is this passing ownership with a raw pointer? If so, please don't.
> > 
> > SkPixelRef is ref-counted, so this is not taking ownership but increasing
the
> > ref-count.  I could mention in a comment that SkPixelRef passed in here
should
> > already be marked immutable.  There's a DCHECK on this already.
> 
> Please use skia::RefPtr, and skia::AdoptRef() a reference as soon as you see
the
> raw pointer. Never store/pass raw pointers to skia ref counted things in
> chromium.

Done.  Thanks for letting me know.  This is much better.

[aelias_OOO_until_Jul13, 2013-09-06 04:17:37]

Looking pretty good, I'm glad the SkPixelRef approach works out.

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:37: DCHECK_EQ(skbitmap.config(),
SkBitmap::kARGB_8888_Config);
Please also DCHECK(skbitmap.width(), skbitmap.rowBytesAsPixels());

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:43: skbitmap.copyTo(&skbitmap_copy,
skbitmap.config());
Please DCHECK that the SkBitmap is immutable instead.  Most users of UIResource
will be able to mark them immutable after creation and would prefer not to incur
the copy.  And add a comment in the header saying that the bitmap must be RGBA8
and marked immutable.

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:50: SkPixelRef* pixel_ref_;
On 2013/09/05 18:51:05, jdduke wrote:
> On 2013/09/05 18:42:32, aelias wrote:
> > On 2013/09/05 18:39:53, jdduke wrote:
> > > Hmm, if we're going to use an SkPixelRef backing, can we not simply go
ahead
> > and
> > > use an SkBitmap, rather than a naked ref? Copying the SkBitmap will do the
> > right
> > > thing if the source is immutable, and if not we can make the copy (then
set
> > > immutable)... I'm not sure, but it might make some of the
> > > bookkeeping/refcounting simpler.
> > 
> > No, because we intend to support ETC1 compressed bitmaps.  These aren't
> > supported by SkBitmap and it would be unnecessarily hard for them to be
(e.g.
> > the implementation of getPixel(x, y) would be complicated).
> 
> I see, though I wasn't proposing we expose the SkBitmap, rather simply use it
as
> the ref-counting handle.  The user would still supply an SkPixelRef, but
> internally it would be contained by an SkBitmap... maybe not worth it?

I think SkPixelRef is the right level of abstraction for what we're doing here. 
If you're suggesting use an SkBitmap and lie to Skia about it being RGBA8,
that's confusing and may have hidden pitfalls based on assumptions that Skia
makes about its properties.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer.cc:91: SetNeedsPushProperties();
You can delete this line since SetNeedsCommit does it already.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer.cc:108: SetNeedsPushProperties();
likewise delete

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer.cc:117: SetNeedsPushProperties();
likewise delete

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer_impl.cc:64: // This check imposes an ordering on the
call sequence.  An UIResource must
How about DCHECKing it instead?

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer_impl.cc:113: if (!ui_resource_id_)
Can you move these early returns back down below the AppendDebugBorderQuad? 
That will make it easier to see what's going on if there's ever a bug.

https://codereview.chromium.org/22870016/diff/128001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/128001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:40: explicit UIResourceBitmap();
Can we get rid of this default constructor and the concept of "INVALID_FORMAT"? 
You'll just need to use a constructor to initialize UIResourceRequest instead of
initializing it by setting fields.

https://codereview.chromium.org/22870016/diff/128001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/128001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:95: static SkBitmap
ConvertToSkBitmap(ScopedJavaLocalRef<jobject> jbitmap) {
Could you get rid of this method by making CreateJavaBitmapFromResource return a
JavaBitmap instead?

[powei, 2013-09-09 17:57:17]

ready for another look.  thanks.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer.cc:91: SetNeedsPushProperties();
On 2013/09/06 04:17:38, aelias wrote:
> You can delete this line since SetNeedsCommit does it already.

Done.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer.cc:108: SetNeedsPushProperties();
On 2013/09/06 04:17:38, aelias wrote:
> likewise delete

Done.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer.cc:117: SetNeedsPushProperties();
On 2013/09/06 04:17:38, aelias wrote:
> likewise delete

Done.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer_impl.cc:64: // This check imposes an ordering on the
call sequence.  An UIResource must
On 2013/09/06 04:17:38, aelias wrote:
> How about DCHECKing it instead?

Done.

https://codereview.chromium.org/22870016/diff/128001/cc/layers/nine_patch_lay...
cc/layers/nine_patch_layer_impl.cc:113: if (!ui_resource_id_)
On 2013/09/06 04:17:38, aelias wrote:
> Can you move these early returns back down below the AppendDebugBorderQuad? 
> That will make it easier to see what's going on if there's ever a bug.

Done.

https://codereview.chromium.org/22870016/diff/128001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/128001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:40: explicit UIResourceBitmap();
On 2013/09/06 04:17:38, aelias wrote:
> Can we get rid of this default constructor and the concept of
"INVALID_FORMAT"? 
> You'll just need to use a constructor to initialize UIResourceRequest instead
of
> initializing it by setting fields.

Done.

https://codereview.chromium.org/22870016/diff/128001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/128001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:95: static SkBitmap
ConvertToSkBitmap(ScopedJavaLocalRef<jobject> jbitmap) {
On 2013/09/06 04:17:38, aelias wrote:
> Could you get rid of this method by making CreateJavaBitmapFromResource return
a
> JavaBitmap instead?

Done.  Consolidated into fewer functions.

[aelias_OOO_until_Jul13, 2013-09-09 21:43:44]

Could you update the title and write a detailed description of what this patch
does?  (You can do that using "Edit Issue" in Rietveld, but amending your local
git commit won't have an effect.)

Also, will this need gardener intervention if it lands before the gerrit side?

[powei, 2013-09-09 22:42:13]

On 2013/09/09 21:43:44, aelias wrote:
> Could you update the title and write a detailed description of what this patch
> does?  (You can do that using "Edit Issue" in Rietveld, but amending your
local
> git commit won't have an effect.)

Title and description have been updated.  

> 
> Also, will this need gardener intervention if it lands before the gerrit side?

No, this won't require gardener intervention.  All of the old logic still exist,
and this patch does not change any existing behavior.

[aelias_OOO_until_Jul13, 2013-09-09 23:11:09]

lgtm.  Adding yfriedman@ for OWNERS review of ui/gfx/android/.  Please also wait
for ccameron@ to take another look to make sure everything works for the Mac use
case before you land.

https://codereview.chromium.org/22870016/diff/163001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/163001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:24: DCHECK(pixel_ref &&
pixel_ref->isImmutable());
Please separate these into 4 DCHECKs.  In general, && in DCHECKs is best avoided
because it makes them harder to interpret when they trigger.

https://codereview.chromium.org/22870016/diff/163001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:33: DCHECK_EQ(skbitmap.config(),
SkBitmap::kARGB_8888_Config);
Please also DCHECK(skbitmap.width(), skbitmap.rowBytesAsPixels());
I made this request in #36; not a big deal but please come up with a systematic
workflow for not skipping over code review comments in the future.  (I use the
trick that if there's an odd number of comments, it usually means there's one I
didn't reply "Done" to.)

[aelias_OOO_until_Jul13, 2013-09-09 23:13:50]

The first line of your commit description should be the same as the title. 
Also, please try to restrict it to the 80-character limit (I know this is
annoying from the edit box).

[Yaron, 2013-09-10 00:28:38]

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:95: SkBitmap bitmap =
CreateSkBitmapFromJavaBitmap(jbitmap);
You dropped the SkBitmap null-check. Intentional?

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.h:47: UI_EXPORT SkBitmap
CreateSkBitmapFromJavaBitmap(JavaBitmap& jbitmap);
Why is this exported/declared here? I may be missing where it's being called
from.

[powei, 2013-09-10 00:42:43]

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:37: DCHECK_EQ(skbitmap.config(),
SkBitmap::kARGB_8888_Config);
On 2013/09/06 04:17:37, aelias wrote:
> Please also DCHECK(skbitmap.width(), skbitmap.rowBytesAsPixels());

Done.

https://codereview.chromium.org/22870016/diff/117001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:43: skbitmap.copyTo(&skbitmap_copy,
skbitmap.config());
On 2013/09/06 04:17:37, aelias wrote:
> Please DCHECK that the SkBitmap is immutable instead.  Most users of
UIResource
> will be able to mark them immutable after creation and would prefer not to
incur
> the copy.  And add a comment in the header saying that the bitmap must be
RGBA8
> and marked immutable.

Done.

https://codereview.chromium.org/22870016/diff/163001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/163001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:24: DCHECK(pixel_ref &&
pixel_ref->isImmutable());
On 2013/09/09 23:11:11, aelias wrote:
> Please separate these into 4 DCHECKs.  In general, && in DCHECKs is best
avoided
> because it makes them harder to interpret when they trigger.

Done.

https://codereview.chromium.org/22870016/diff/163001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:33: DCHECK_EQ(skbitmap.config(),
SkBitmap::kARGB_8888_Config);
On 2013/09/09 23:11:11, aelias wrote:
> Please also DCHECK(skbitmap.width(), skbitmap.rowBytesAsPixels());
> I made this request in #36; not a big deal but please come up with a
systematic
> workflow for not skipping over code review comments in the future.  (I use the
> trick that if there's an odd number of comments, it usually means there's one
I
> didn't reply "Done" to.)

Yes.  This was a problem.  I will try to be more careful.

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:95: SkBitmap bitmap =
CreateSkBitmapFromJavaBitmap(jbitmap);
On 2013/09/10 00:28:39, Yaron wrote:
> You dropped the SkBitmap null-check. Intentional?

yes, in the old code, bitmap.isNull() is true when jbitmap (which is jobj in the
new code) is null.  This check is now at line 91.  In both the old and new, a
default constructed SkBitmap is returned.  

When CreateSkBitmapFromJavaBitmap is called, we can be sure that the result
SkBitmap is not null.

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.h:47: UI_EXPORT SkBitmap
CreateSkBitmapFromJavaBitmap(JavaBitmap& jbitmap);
On 2013/09/10 00:28:39, Yaron wrote:
> Why is this exported/declared here? I may be missing where it's being called
> from.

Called in java_bitmap.cc and also in LayerDecorationCache on the android side.

[Yaron, 2013-09-10 01:23:02]

lgtm

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.h (right):

https://codereview.chromium.org/22870016/diff/163001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.h:47: UI_EXPORT SkBitmap
CreateSkBitmapFromJavaBitmap(JavaBitmap& jbitmap);
On 2013/09/10 00:42:44, powei wrote:
> On 2013/09/10 00:28:39, Yaron wrote:
> > Why is this exported/declared here? I may be missing where it's being called
> > from.
> 
> Called in java_bitmap.cc and also in LayerDecorationCache on the android side.

Missed the LayerDecorationCache one. Thanks.

[ccameron, 2013-09-10 20:26:02]

I applied this locally, and this causes the overhang shadow nine-patch layer to
disappear on the Mac.

I'm looking at the patch to see what might be causing this.

[powei, 2013-09-10 20:29:01]

On 2013/09/10 20:26:02, ccameron1 wrote:
> I applied this locally, and this causes the overhang shadow nine-patch layer
to
> disappear on the Mac.
> 
> I'm looking at the patch to see what might be causing this.

You might want to take a look at how the layout of the nine-patch layer has
changed.  In particular, there's a |border_| variable that needs to be defined
and is not in the old nine-patch.

[ccameron, 2013-09-10 21:18:29]

It's that and some lifetime issues. I'll drop by to figure out how to sort this
out.

https://chromiumcodereview.appspot.com/22870016/diff/167001/cc/layers/nine_pa...
File cc/layers/nine_patch_layer.cc (right):

https://chromiumcodereview.appspot.com/22870016/diff/167001/cc/layers/nine_pa...
cc/layers/nine_patch_layer.cc:86: return;
This (and not setting the border) are causing the Mac failures. This call comes
in while layer_tree_host is NULL, where RenderLayerCompositor in Blink sets up
the overscroll layers.

[powei, 2013-09-11 19:59:10]

On 2013/09/10 21:18:29, ccameron1 wrote:
> It's that and some lifetime issues. I'll drop by to figure out how to sort
this
> out.
> 
>
https://chromiumcodereview.appspot.com/22870016/diff/167001/cc/layers/nine_pa...
> File cc/layers/nine_patch_layer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/22870016/diff/167001/cc/layers/nine_pa...
> cc/layers/nine_patch_layer.cc:86: return;
> This (and not setting the border) are causing the Mac failures. This call
comes
> in while layer_tree_host is NULL, where RenderLayerCompositor in Blink sets up
> the overscroll layers.

Chris, would you mind taking a quick look to see if I merged your fix correctly?
 After you ok, I'm going to try and land this.

[ccameron, 2013-09-11 20:06:10]

This lgtm -- thanks!!

[powei, 2013-09-11 20:07:31]

On 2013/09/11 20:06:10, ccameron1 wrote:
> This lgtm -- thanks!!

Thanks everyone, for your help and feedback.

[commit-bot: I haz the power, 2013-09-11 20:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/22870016/184001

[commit-bot: I haz the power, 2013-09-11 20:16:08]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-12 02:51:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/22870016/184001

[commit-bot: I haz the power, 2013-09-12 06:18:21]

Change committed as 222732

[powei, 2013-09-13 17:42:01]

PTAL.

Only changes are the updated perftest files:
cc/test/layer_tree_json_parser.cc
cc/test/data/10_10_layer_tree.json

And one bug that I introduced but now have corrected (see embedded comment).  

Reopening this issue due to:
https://code.google.com/p/chromium/issues/detail?id=290215
and a subsequent revert: https://codereview.chromium.org/23620047/

This CL caused an improvement that was misinterpreted as a regression.  But that
led me to update the perftest to pass the DCHECKS I added.  

Now issue 290215 has been updated to relanding this.

https://chromiumcodereview.appspot.com/22870016/diff/184001/cc/layers/nine_pa...
File cc/layers/nine_patch_layer_impl.cc (right):

https://chromiumcodereview.appspot.com/22870016/diff/184001/cc/layers/nine_pa...
cc/layers/nine_patch_layer_impl.cc:91: if (image_bounds_ == image_bounds &&
image_aperture_ != aperture &&
this is incorrect.  fixed in next patch.

[aelias_OOO_until_Jul13, 2013-09-13 18:11:48]

lgtm

[powei, 2013-09-13 19:32:00]

On 2013/09/13 18:11:48, aelias wrote:
> lgtm

thanks.  I'm going to land this, but note that I manually changed the
cc/test/data/10_10_layer_tree.json.  Ideally, the layout should come from an
actual layout from a phone test (as suggested by jamesr@), but to avoid causing
any more unexpected, drastic changes in the perftest, this temp solution will do
for now.  I will likely open another CL to update the 10_10 test and notify the
perf team that the test needs to be rebaselined.

[commit-bot: I haz the power, 2013-09-13 19:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/22870016/189001

[commit-bot: I haz the power, 2013-09-13 23:25:12]

Change committed as 223162

[powei, 2013-09-19 00:40:27]

On 2013/09/13 23:25:12, I haz the power (commit-bot) wrote:
> Change committed as 223162

This was reverted again due to failing the mac_gpu bot.  After discussing with
ccameron, I changed nine_patch_layer_impl's DCHECKs to workaround the failure. 
mac_gpu bot is happy now.  Will try landing once more.

Chris, can you take one more quick look?  Thanks very much.

[ccameron, 2013-09-19 19:08:23]

This works locally for me. lgtm. Sorry for the delay!

[commit-bot: I haz the power, 2013-09-19 20:15:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/22870016/256001

[commit-bot: I haz the power, 2013-09-20 03:34:29]

Change committed as 224270