[Aura] Added Support for rendering software compositor frames as cc::TextureLayers.

[Aura] Added Support for rendering software compositor frames as cc::TextureLayers through cc::TextureMailbox.

BUG=161008
R=piman
TBR=jschuh,ben

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

[slavi, 2013-05-21 21:38:29]

This is still WIP patch and I would like to get some early reviewer's feedback.

I extended TextureMailbox to support either mailbox-ed textures or bitmaps
stored in SharedMemory, referenced by a SharedMemoryHandle.

The effect in the context of the s/w compositor is that now the composited frame
lives in TextureLayer in the browser (instead of in ContentLayer) and we don't
do an extra copy from shared memory to the ContentLayer's contents.

[danakj, 2013-05-23 15:34:32]

Very cool! :) Some thoughts on cleaning up the CL and making things more
consistent or hopefully more robust.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:194: context_lost_ = client_->Context3d() &&
Can you DCHECK that Context3d() is not NULL when the texture_mailbox_
IsTexture()?

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:221:
texture_layer->SetTextureMailbox(TextureMailbox());
DCHECK that the texture_mailbox_ is not valid?

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.h:88: const bool uses_mailbox_;
const things should be kFooBar. I'd prefer to not const this.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.h:101: TextureMailbox mailbox_;
Prefer to keep this variable name as is. It's not a gpu::Mailbox, it's a
TextureMailbox, which is why it's called as such.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:68: void FreeMailbox();
Keep this name as TextureMailbox as well for the same reasons please.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:70: const bool uses_mailbox_;
no const, no need to move this variable either - just diff churn

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:80: TextureMailbox mailbox_;
keep the old name

https://codereview.chromium.org/15001027/diff/177001/cc/output/compositor_fra...
File cc/output/compositor_frame_ack.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/output/compositor_fra...
cc/output/compositor_frame_ack.h:22: int last_frame_id;
Is this id only valid for software frames? (The outgoing id is in the software
frame data?)

If so, can you name it last_software_frame_id?

https://codereview.chromium.org/15001027/diff/177001/cc/output/software_outpu...
File cc/output/software_output_device.cc (right):

https://codereview.chromium.org/15001027/diff/177001/cc/output/software_outpu...
cc/output/software_output_device.cc:37: frame_data->id = 0;
This frame id seems to be completely unused right now. Can we add it in the CL
that will make use of it instead?

https://codereview.chromium.org/15001027/diff/177001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/resources/resource_pr...
cc/resources/resource_provider.h:347: base::SharedMemory* shared_memory;
Why is this not a scoped_ptr?

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.cc:90: return Equals(other.name()) ||
Equals(other.handle());
check that IsTexture() and IsSharedMemory() also match?

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:18: class CC_EXPORT TextureMailbox {
Add a TODO here to rename this class please.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:20: typedef base::Callback<void(unsigned
sync_point,
The values on this callback don't really make sense for software.

What if we have two callbacks, a ReleaseTextureCallback and a
ReleaseSharedMemoryCallback?

Then we could avoid exposing the callback directly from this class, ie. remove
|callback()|, and add some methods to fire the appropriate callbacks
RunReleaseTextureCallback(unsigned,bool) and
RunReleaseSharedMemoryCallback(void). We can DCHECK in these methods that we're
calling the right method for the data in the TextureMailbox.

WDYT? Would that clean things up for make them more complicated?

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:35: gfx::Size size,
For textures we always pass along the size outside of this class right now.
Would it be nice to be consistent here and do the same for software bitmaps?
It's a bit odd that this class holds a size in some modes and not in others.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:46: bool Equals(const gpu::Mailbox&) const;
This is the wrong name for this method now. How about Contains()?

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:47: bool Equals(base::SharedMemoryHandle handle)
const;
Same here

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:52: const gpu::Mailbox& name() const { return
name_; }
I think I'd like to see all the gpu::Mailbox related methods/variables here
moved to another class, and have a pointer+getter method on this class for
accessing them.

Similar for the SharedMemory related stuff.

But that can be done in a followup, maybe at the time we rename this class.

[slavi, 2013-05-28 18:55:13]

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:194: context_lost_ = client_->Context3d() &&
On 2013/05/23 15:34:32, danakj wrote:
> Can you DCHECK that Context3d() is not NULL when the texture_mailbox_
> IsTexture()?

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:221:
texture_layer->SetTextureMailbox(TextureMailbox());
On 2013/05/23 15:34:33, danakj wrote:
> DCHECK that the texture_mailbox_ is not valid?

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.h:88: const bool uses_mailbox_;
On 2013/05/23 15:34:33, danakj wrote:
> const things should be kFooBar. I'd prefer to not const this.

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer....
cc/layers/texture_layer.h:101: TextureMailbox mailbox_;
On 2013/05/23 15:34:33, danakj wrote:
> Prefer to keep this variable name as is. It's not a gpu::Mailbox, it's a
> TextureMailbox, which is why it's called as such.

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:68: void FreeMailbox();
On 2013/05/23 15:34:33, danakj wrote:
> Keep this name as TextureMailbox as well for the same reasons please.

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:70: const bool uses_mailbox_;
On 2013/05/23 15:34:33, danakj wrote:
> no const, no need to move this variable either - just diff churn

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:80: TextureMailbox mailbox_;
On 2013/05/23 15:34:33, danakj wrote:
> keep the old name

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/output/compositor_fra...
File cc/output/compositor_frame_ack.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/output/compositor_fra...
cc/output/compositor_frame_ack.h:22: int last_frame_id;
On 2013/05/23 15:34:33, danakj wrote:
> Is this id only valid for software frames? (The outgoing id is in the software
> frame data?)
> 
> If so, can you name it last_software_frame_id?

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/output/software_outpu...
File cc/output/software_output_device.cc (right):

https://codereview.chromium.org/15001027/diff/177001/cc/output/software_outpu...
cc/output/software_output_device.cc:37: frame_data->id = 0;
On 2013/05/23 15:34:33, danakj wrote:
> This frame id seems to be completely unused right now. Can we add it in the CL
> that will make use of it instead?

It gets overriden in 
content/renderer/gpu/compositor_software_output_device.cc
CompositorSoftwareOutputDevice::EndPaint

Do you mean, leave the base (this one) implementation empty?

https://codereview.chromium.org/15001027/diff/177001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/resources/resource_pr...
cc/resources/resource_provider.h:347: base::SharedMemory* shared_memory;
On 2013/05/23 15:34:33, danakj wrote:
> Why is this not a scoped_ptr?

I tried that first but the Resource struct needs to be copyable since it lives
in a STL container. A scoped_refptr would be nice but base::SharedMemory is not
ref counted.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.cc:90: return Equals(other.name()) ||
Equals(other.handle());
On 2013/05/23 15:34:33, danakj wrote:
> check that IsTexture() and IsSharedMemory() also match?

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.h (right):

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:18: class CC_EXPORT TextureMailbox {
On 2013/05/23 15:34:33, danakj wrote:
> Add a TODO here to rename this class please.

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:46: bool Equals(const gpu::Mailbox&) const;
On 2013/05/23 15:34:33, danakj wrote:
> This is the wrong name for this method now. How about Contains()?

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:47: bool Equals(base::SharedMemoryHandle handle)
const;
On 2013/05/23 15:34:33, danakj wrote:
> Same here

Done.

https://codereview.chromium.org/15001027/diff/177001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:52: const gpu::Mailbox& name() const { return
name_; }
On 2013/05/23 15:34:33, danakj wrote:
> I think I'd like to see all the gpu::Mailbox related methods/variables here
> moved to another class, and have a pointer+getter method on this class for
> accessing them.
> 
> Similar for the SharedMemory related stuff.
> 
> But that can be done in a followup, maybe at the time we rename this class.

Let's do that on a second pass when this guy gets renames.

[piman, 2013-05-28 20:58:57]

https://codereview.chromium.org/15001027/diff/221001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/221001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:29: if (!callback.is_null())
nit: braces needed.
I'm not sure that this new form (binding PostCallbackToThread to a NULL
callback) is better than the old one...

https://codereview.chromium.org/15001027/diff/221001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:227: }
It seems that this whole block could be simplified by using the copy constructor
for TextureMailbox and adding a SetCallback. Alternatively, introducing a
TextureMailbox::CopyWithNewCallback.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/221001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:282: if (mailbox.IsTexture()) {
nit: I think we should DCHECK that the mailbox IsValid() and then if it's not a
texture DCHECK that mailbox.IsSharedMemory()
I don't think we should allow giving the RP an invalid resource, I suspect bad
things will happen in other places.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:915: // TODO(skaslev) Implement this path for
shared memory resources.
This needs to be at least a DCHECK or something. Right now, it would fall into
the !source->mailbox.IsTexture() path and crash on a NULL context3d or
something. Better to be explicit about it upfront - and the test right below
would be less confusing.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
cc/resources/texture_mailbox.cc:108: handle_ = base::SharedMemory::NULLHandle();
Maybe we should DCHECK that it is NULL? I don't think we should freely switch
from software to GL - TextureMailbox is really meant to be immutable.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.h (right):

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:50: bool Contains(base::SharedMemoryHandle
handle) const;
nit: no overloading.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:63: gfx::Size size() const { return size_; }
Maybe name this software_size, or shared_memory_size, to indicate that it's only
meaningful for the software path.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1499: if (!dib ||
dib->size() < expected_size) {
We need to keep this test.
Without it, ResourceProvider::LockForRead would CHECK, i.e. it's possible for a
malicious renderer to crash the browser.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:371: void
ReleaseMailbox(MessageLoop *msg_loop, base::Callback<void()> callback,
The callback is always called on the UI thread. This shouldn't be necessary.

If it was, we'd need a MessageLoopProxy rather than a raw MessageLoop.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1507:
SendSoftwareFrameAck(frame_data->id);
We still want to keep the new frame around.
The case is for the renderer to swap into a view that's disconnected from the
tree.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:562:
cc::TextureMailbox current_frame_;
nit: as is, it looks like it's used for the GL path. Maybe rename as
current_software_frame_ ?

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.cc:73: if (id == 0)
So, here you're relying on int overflow which is undefined behavior (i.e. the
compiler can assume it never happens).
For example, the compiler can determine that next_buffer_id_ starts positive and
is only incremented, so it can never be 0 and elide this code.

I think you need to change your ids into unsigned ints (where overflow is
defined).

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.h (right):

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.h:51: void
SetParent(Buffer* parent, const gfx::Rect& damage);
The "parenting" relationship could use an explanation (that it's about damage
tracking).

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:535: }
So, one missing piece here is Window::RecreateLayer. It's used in various UI
animations, for example the cross-fade as we maximize in Ash.
In GL mode, it duplicates the texture.
In the old software mode, it keeps the old layer without causing damage, and
creates a new layer which has the delegate.

As is, the new layer will get the RWHVA as a delegate, but nothing will set the
texture mailbox on it, so it will create a ContentLayer and try to paint it.

One way to go about it is to explicitly duplicate the shm and set it to the new
layer, with proper care wrt destruction.

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:254: void SetTextureMailbox(const cc::TextureMailbox&
mailbox,
nit: comment.

[slavi, 2013-05-29 18:31:47]

https://codereview.chromium.org/15001027/diff/221001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/221001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:29: if (!callback.is_null())
On 2013/05/28 20:58:57, piman wrote:
> nit: braces needed.
> I'm not sure that this new form (binding PostCallbackToThread to a NULL
> callback) is better than the old one...

Done.

https://codereview.chromium.org/15001027/diff/221001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:227: }
On 2013/05/28 20:58:57, piman wrote:
> It seems that this whole block could be simplified by using the copy
constructor
> for TextureMailbox and adding a SetCallback. Alternatively, introducing a
> TextureMailbox::CopyWithNewCallback.

Done.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/221001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:282: if (mailbox.IsTexture()) {
On 2013/05/28 20:58:57, piman wrote:
> nit: I think we should DCHECK that the mailbox IsValid() and then if it's not
a
> texture DCHECK that mailbox.IsSharedMemory()
> I don't think we should allow giving the RP an invalid resource, I suspect bad
> things will happen in other places.

Done.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:915: // TODO(skaslev) Implement this path for
shared memory resources.
On 2013/05/28 20:58:57, piman wrote:
> This needs to be at least a DCHECK or something. Right now, it would fall into
> the !source->mailbox.IsTexture() path and crash on a NULL context3d or
> something. Better to be explicit about it upfront - and the test right below
> would be less confusing.

Done.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
cc/resources/texture_mailbox.cc:108: handle_ = base::SharedMemory::NULLHandle();
On 2013/05/28 20:58:57, piman wrote:
> Maybe we should DCHECK that it is NULL? I don't think we should freely switch
> from software to GL - TextureMailbox is really meant to be immutable.

Done.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.h (right):

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:50: bool Contains(base::SharedMemoryHandle
handle) const;
On 2013/05/28 20:58:57, piman wrote:
> nit: no overloading.

Done.

https://codereview.chromium.org/15001027/diff/221001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:63: gfx::Size size() const { return size_; }
On 2013/05/28 20:58:57, piman wrote:
> Maybe name this software_size, or shared_memory_size, to indicate that it's
only
> meaningful for the software path.

Done.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1499: if (!dib ||
dib->size() < expected_size) {
On 2013/05/28 20:58:57, piman wrote:
> We need to keep this test.
> Without it, ResourceProvider::LockForRead would CHECK, i.e. it's possible for
a
> malicious renderer to crash the browser.

If the size is wrong (too big), SharedMemory::Map shall fail. I'm handling this
case in the resource provider, since ShareMemory::Map may fail for other reasons
as well and this is a problem that will persist as we move toward ubercomp where
SharedMemory will be used for tile content.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:371: void
ReleaseMailbox(MessageLoop *msg_loop, base::Callback<void()> callback,
On 2013/05/28 20:58:57, piman wrote:
> The callback is always called on the UI thread. This shouldn't be necessary.
> 
> If it was, we'd need a MessageLoopProxy rather than a raw MessageLoop.

Done.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1507:
SendSoftwareFrameAck(frame_data->id);
On 2013/05/28 20:58:57, piman wrote:
> We still want to keep the new frame around.
> The case is for the renderer to swap into a view that's disconnected from the
> tree.

Done.

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/15001027/diff/221001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:562:
cc::TextureMailbox current_frame_;
On 2013/05/28 20:58:57, piman wrote:
> nit: as is, it looks like it's used for the GL path. Maybe rename as
> current_software_frame_ ?

Done.

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.cc:73: if (id == 0)
On 2013/05/28 20:58:57, piman wrote:
> So, here you're relying on int overflow which is undefined behavior (i.e. the
> compiler can assume it never happens).
> For example, the compiler can determine that next_buffer_id_ starts positive
and
> is only incremented, so it can never be 0 and elide this code.
> 
> I think you need to change your ids into unsigned ints (where overflow is
> defined).

Done.

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.h (right):

https://codereview.chromium.org/15001027/diff/221001/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.h:51: void
SetParent(Buffer* parent, const gfx::Rect& damage);
On 2013/05/28 20:58:57, piman wrote:
> The "parenting" relationship could use an explanation (that it's about damage
> tracking).

Done.

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:535: }
On 2013/05/28 20:58:57, piman wrote:
> So, one missing piece here is Window::RecreateLayer. It's used in various UI
> animations, for example the cross-fade as we maximize in Ash.
> In GL mode, it duplicates the texture.
> In the old software mode, it keeps the old layer without causing damage, and
> creates a new layer which has the delegate.
> 
> As is, the new layer will get the RWHVA as a delegate, but nothing will set
the
> texture mailbox on it, so it will create a ContentLayer and try to paint it.
> 
> One way to go about it is to explicitly duplicate the shm and set it to the
new
> layer, with proper care wrt destruction.

Done.

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/15001027/diff/221001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:254: void SetTextureMailbox(const cc::TextureMailbox&
mailbox,
On 2013/05/28 20:58:57, piman wrote:
> nit: comment.

Done.

[piman, 2013-05-29 20:00:10]

https://codereview.chromium.org/15001027/diff/156002/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/156002/cc/resources/resource_pr...
cc/resources/resource_provider.cc:290: } else if (mailbox.IsSharedMemory()) {
the second if is not needed anymore, but maybe can be a DCHECK.

https://codereview.chromium.org/15001027/diff/156002/cc/resources/resource_pr...
cc/resources/resource_provider.cc:534: resource->mailbox.handle(), false);
Actually, I just realized because of the RecreateLayer logic, there is an
ownership problem.
There's no guarantee that this will be called (e.g. if the tab is hidden), so no
guarantee that a SharedMemory will be created, and then destroyed, which will
close the handle/FD.

For that matter, there's no guarantee that we will commit the TextureLayer (it
could get deleted before the commit), so no guarantee that we will
CreateResourceFromTextureMailbox.
I think the RWHVA needs to be responsible for the lifetime of the handle,
closing it when it gets the release callback (or, actually, another case, if we
skip the frame).

Now, for the RecreateLayer logic, it also needs to dup and own the handle. It
can't use the same handle because it could get closed before the other layer
starts trying to map it.

https://codereview.chromium.org/15001027/diff/156002/cc/resources/texture_mai...
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/15001027/diff/156002/cc/resources/texture_mai...
cc/resources/texture_mailbox.cc:126: return TextureMailbox();
How about:
TextureMailbox copy(*this);
copy.callback = callback;
return copy;

Then no need for if tests...

[slavi, 2013-06-03 22:14:16]

I think it's ready for landing. PTAL.

https://codereview.chromium.org/15001027/diff/156002/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/156002/cc/resources/resource_pr...
cc/resources/resource_provider.cc:290: } else if (mailbox.IsSharedMemory()) {
On 2013/05/29 20:00:10, piman wrote:
> the second if is not needed anymore, but maybe can be a DCHECK.

Done.

https://codereview.chromium.org/15001027/diff/156002/cc/resources/resource_pr...
cc/resources/resource_provider.cc:534: resource->mailbox.handle(), false);
On 2013/05/29 20:00:10, piman wrote:
> Actually, I just realized because of the RecreateLayer logic, there is an
> ownership problem.
> There's no guarantee that this will be called (e.g. if the tab is hidden), so
no
> guarantee that a SharedMemory will be created, and then destroyed, which will
> close the handle/FD.
> 
> For that matter, there's no guarantee that we will commit the TextureLayer (it
> could get deleted before the commit), so no guarantee that we will
> CreateResourceFromTextureMailbox.
> I think the RWHVA needs to be responsible for the lifetime of the handle,
> closing it when it gets the release callback (or, actually, another case, if
we
> skip the frame).
> 
> Now, for the RecreateLayer logic, it also needs to dup and own the handle. It
> can't use the same handle because it could get closed before the other layer
> starts trying to map it.

Done.

https://codereview.chromium.org/15001027/diff/156002/cc/resources/texture_mai...
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/15001027/diff/156002/cc/resources/texture_mai...
cc/resources/texture_mailbox.cc:126: return TextureMailbox();
On 2013/05/29 20:00:10, piman wrote:
> How about:
> TextureMailbox copy(*this);
> copy.callback = callback;
> return copy;
> 
> Then no need for if tests...

Done.

[piman, 2013-06-05 00:30:55]

Could we also have tests for the new features in cc? In particular:
- callback logic for the software case in TextureLayer
- ResourceProvider logic for CreateResourceFromTextureMailbox in software case?

https://codereview.chromium.org/15001027/diff/234003/base/memory/shared_memor...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/15001027/diff/234003/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:106: result.fd = dup(handle.fd);
nit: HANDLE_EINTR

https://codereview.chromium.org/15001027/diff/234003/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:107: result.auto_close = false;
nit: make that true
It doesn't have an effect in most of the cases, except when you send it through
IPC, but in those cases you most likely want it true.

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (left):

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer....
cc/layers/texture_layer.cc:146: DCHECK(mailbox.IsEmpty() ||
!mailbox.Equals(texture_mailbox_));
As mentioned in person, I think we can (and should) leave this in.

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (left):

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:37: DCHECK(mailbox.IsEmpty() ||
!mailbox.Equals(texture_mailbox_));
Same here - leave this in?

https://codereview.chromium.org/15001027/diff/234003/cc/output/compositor_fra...
File cc/output/compositor_frame_ack.h (right):

https://codereview.chromium.org/15001027/diff/234003/cc/output/compositor_fra...
cc/output/compositor_frame_ack.h:22: int last_software_frame_id;
unsigned int.

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_frame...
File cc/output/software_frame_data.h (right):

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_frame...
cc/output/software_frame_data.h:19: int id;
unsigned int.

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_outpu...
File cc/output/software_output_device.h (right):

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_outpu...
cc/output/software_output_device.h:41: virtual void ReclaimSoftwareFrame(int
id);
unsigned int.

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1563:
base::Bind(ReleaseMailbox, shared_memory.release(),
nit: you can use Passed(new_buffer), and make ReleaseMailbox take a scoped_ptr
and this makes it safer if ever the callback is destroyed without being called
because the message loop is gone or something.

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:485: void
SendSoftwareFrameAck(int software_frame_id);
unsigned int.

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.cc:71: int id =
next_buffer_id_++;
unsigned int.

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.h (right):

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.h:43: explicit Buffer(int
id, scoped_ptr<base::SharedMemory> mem);
This id and all other ones in this class should be unsigned ints.

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.h:81: int GetNextId();
unsigned int.

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc#newcod...
ui/aura/window.cc:175: else if (old_mailbox.IsValid()) {
nit: both sides of the else should have brackets if one side has them.

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc#newcod...
ui/aura/window.cc:184: base::SharedMemory old_buffer(old_handle, false);
What if, instead of just the handle, we also passed the pointer into the
TextureMailbox: we could have the RWHVA Map the shm, and do the size check
(rather than deep inside the ResourceProvider), and we wouldn't have to
duplicate yet-once-more here, just to be able to access the data - it'd already
be mapped.
After all, the only reason we need the handle in the RP is to be able to send it
to the parent compositor, there's no fundamental reason to have the RP be the
one that maps.

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc#newcod...
ui/aura/window.cc:189: base::Bind(MailboxReleaseCallback, new_buffer.release());
nit: you can use Passed(new_buffer), and make MailboxReleaseCallback take a
scoped_ptr and this makes it safer if ever the callback is destroyed without
being called because the message loop is gone or something.

[slavi, 2013-06-06 23:02:46]

https://codereview.chromium.org/15001027/diff/234003/base/memory/shared_memor...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/15001027/diff/234003/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:106: result.fd = dup(handle.fd);
On 2013/06/05 00:30:55, piman wrote:
> nit: HANDLE_EINTR

Done.

https://codereview.chromium.org/15001027/diff/234003/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:107: result.auto_close = false;
On 2013/06/05 00:30:55, piman wrote:
> nit: make that true
> It doesn't have an effect in most of the cases, except when you send it
through
> IPC, but in those cases you most likely want it true.

Done.

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (left):

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer....
cc/layers/texture_layer.cc:146: DCHECK(mailbox.IsEmpty() ||
!mailbox.Equals(texture_mailbox_));
On 2013/06/05 00:30:55, piman wrote:
> As mentioned in person, I think we can (and should) leave this in.

Done.

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (left):

https://codereview.chromium.org/15001027/diff/234003/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:37: DCHECK(mailbox.IsEmpty() ||
!mailbox.Equals(texture_mailbox_));
On 2013/06/05 00:30:55, piman wrote:
> Same here - leave this in?

Done.

https://codereview.chromium.org/15001027/diff/234003/cc/output/compositor_fra...
File cc/output/compositor_frame_ack.h (right):

https://codereview.chromium.org/15001027/diff/234003/cc/output/compositor_fra...
cc/output/compositor_frame_ack.h:22: int last_software_frame_id;
On 2013/06/05 00:30:55, piman wrote:
> unsigned int.

Done.

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_frame...
File cc/output/software_frame_data.h (right):

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_frame...
cc/output/software_frame_data.h:19: int id;
On 2013/06/05 00:30:55, piman wrote:
> unsigned int.

Done.

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_outpu...
File cc/output/software_output_device.h (right):

https://codereview.chromium.org/15001027/diff/234003/cc/output/software_outpu...
cc/output/software_output_device.h:41: virtual void ReclaimSoftwareFrame(int
id);
On 2013/06/05 00:30:55, piman wrote:
> unsigned int.

Done.

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1563:
base::Bind(ReleaseMailbox, shared_memory.release(),
On 2013/06/05 00:30:55, piman wrote:
> nit: you can use Passed(new_buffer), and make ReleaseMailbox take a scoped_ptr
> and this makes it safer if ever the callback is destroyed without being called
> because the message loop is gone or something.

Done.

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/15001027/diff/234003/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.h:485: void
SendSoftwareFrameAck(int software_frame_id);
On 2013/06/05 00:30:55, piman wrote:
> unsigned int.

Done.

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.cc:71: int id =
next_buffer_id_++;
On 2013/06/05 00:30:55, piman wrote:
> unsigned int.

Done.

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
File content/renderer/gpu/compositor_software_output_device.h (right):

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.h:43: explicit Buffer(int
id, scoped_ptr<base::SharedMemory> mem);
On 2013/06/05 00:30:55, piman wrote:
> This id and all other ones in this class should be unsigned ints.

Done.

https://codereview.chromium.org/15001027/diff/234003/content/renderer/gpu/com...
content/renderer/gpu/compositor_software_output_device.h:81: int GetNextId();
On 2013/06/05 00:30:55, piman wrote:
> unsigned int.

Done.

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc#newcod...
ui/aura/window.cc:175: else if (old_mailbox.IsValid()) {
On 2013/06/05 00:30:55, piman wrote:
> nit: both sides of the else should have brackets if one side has them.

Done.

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc#newcod...
ui/aura/window.cc:184: base::SharedMemory old_buffer(old_handle, false);
On 2013/06/05 00:30:55, piman wrote:
> What if, instead of just the handle, we also passed the pointer into the
> TextureMailbox: we could have the RWHVA Map the shm, and do the size check
> (rather than deep inside the ResourceProvider), and we wouldn't have to
> duplicate yet-once-more here, just to be able to access the data - it'd
already
> be mapped.
> After all, the only reason we need the handle in the RP is to be able to send
it
> to the parent compositor, there's no fundamental reason to have the RP be the
> one that maps.

Done.

https://codereview.chromium.org/15001027/diff/234003/ui/aura/window.cc#newcod...
ui/aura/window.cc:189: base::Bind(MailboxReleaseCallback, new_buffer.release());
On 2013/06/05 00:30:55, piman wrote:
> nit: you can use Passed(new_buffer), and make MailboxReleaseCallback take a
> scoped_ptr and this makes it safer if ever the callback is destroyed without
> being called because the message loop is gone or something.

Done.

[piman, 2013-06-07 01:26:27]

I would also like to see tests for the ResourceProvider features:
- create a resource through CreateResourceFromTextureMailbox in software cases.
Can you ScopedReadLockSoftware it? Does it have the right data, the right size?
- There's various tests that check behavior with
CreateResourceFromTextureMailbox, you extended that to work with software. It
should be tested in that case too. You don't support exporting those yet, so
those tests don't apply yet, but there's others.

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:151: mailbox.IsSharedMemory() ||
In which case do you need to call SetTextureMailbox with the same shared memory?
Are you sure those cases are properly handled by the corresponding callback?

That's the reason this DCHECK exists for the texture case - it's not that it
can't be handled through cc, it's that most likely it is not correctly handled
by the caller side (it's indeed not trivial to do so without refcounting and
other things).

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:313: 0, false))
nit: indent

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:294: resources_[id] = resource;
nit: can we share the setting of external/allocated/mailbox and the map entry
between the 2 cases?

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:529: resource->pixels =
reinterpret_cast<uint8_t*>(shared_memory->memory());
nit: we could set resource->pixels directly in CreateResourceFromTextureMailbox
and avoid this logic altogether.

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:352:
shared_memory.reset(NULL);
nit: you actually don't need this, it'll go away by itself when going out of
scope.
Feel free to leave a comment explaining it if you think that makes it clearer.

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1538: if
(!compositor) {
I believe I mentioned that earlier. GetCompositor will return NULL if the RWHVA
is not in the tree. In that case we still want to get the new frame, so that
when the RWHVA is put back into the tree, we can show the latest renderer frame,
not the one from eons ago before the RWHVA was removed from the tree.

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1580:
base::Bind(ReleaseMailbox, Passed(shared_memory.Pass()),
nit: Passed(shared_memory) should work.

https://codereview.chromium.org/15001027/diff/398001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/15001027/diff/398001/ui/aura/window.cc#newcode41
ui/aura/window.cc:41: shared_memory.reset(NULL);
nit: same as in RWHVA you actually don't need this, it'll go away by itself when
going out of scope.

[danakj, 2013-06-07 14:56:42]

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.h (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:71: bool uses_mailbox_;
nit: no need to move this

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:911: if (!source->mailbox.IsTexture()) {
This if looks awkward now, as the else implicitly thinks that IsTexture() is
true, but it won't have to be. What if we invert the if/else here?

The the else can become an elseif in the future.

https://codereview.chromium.org/15001027/diff/398001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.h (right):

https://codereview.chromium.org/15001027/diff/398001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:37: gfx::Size size,
I feel like including the size inside the TextureMailbox is going to be awkward.

When I want to give you a texture mailbox, if it contains a texture, I'll have
to pass a size along with it. If it contains a shared memory, then I don't. But
I'll still have a size parameter there to handle the texture case, it's just
unused?

Like HereIsATextureMailbox(TextureMailbox& mailbox, gfx::Size
size_if_mailbox_is_a_texture). That's pretty awkward. Since the hardware path
needs a size passed along with it, would it make sense to remove the size from
here for the shared memory case and require it to be passed separately as well?

[slavi, 2013-06-07 21:49:21]

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:151: mailbox.IsSharedMemory() ||
On 2013/06/07 01:26:28, piman wrote:
> In which case do you need to call SetTextureMailbox with the same shared
memory?
> Are you sure those cases are properly handled by the corresponding callback?
> 
> That's the reason this DCHECK exists for the texture case - it's not that it
> can't be handled through cc, it's that most likely it is not correctly handled
> by the caller side (it's indeed not trivial to do so without refcounting and
> other things).

This check was failing after tab detach/reattach. Changed it to only check if
it's the owner of the mailbox.

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.h (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.h:71: bool uses_mailbox_;
On 2013/06/07 14:56:42, danakj wrote:
> nit: no need to move this

Done.

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:313: 0, false))
On 2013/06/07 01:26:28, piman wrote:
> nit: indent

Done.

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:294: resources_[id] = resource;
On 2013/06/07 01:26:28, piman wrote:
> nit: can we share the setting of external/allocated/mailbox and the map entry
> between the 2 cases?

Done.

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:529: resource->pixels =
reinterpret_cast<uint8_t*>(shared_memory->memory());
On 2013/06/07 01:26:28, piman wrote:
> nit: we could set resource->pixels directly in
CreateResourceFromTextureMailbox
> and avoid this logic altogether.

Done.

https://codereview.chromium.org/15001027/diff/398001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:911: if (!source->mailbox.IsTexture()) {
On 2013/06/07 14:56:42, danakj wrote:
> This if looks awkward now, as the else implicitly thinks that IsTexture() is
> true, but it won't have to be. What if we invert the if/else here?
> 
> The the else can become an elseif in the future.

I agree, hence the DCHECK and TODO comment above.

I can make the DCHECK above a CHECK until s/w compositing for ubercomp is not
ready.

https://codereview.chromium.org/15001027/diff/398001/cc/resources/texture_mai...
File cc/resources/texture_mailbox.h (right):

https://codereview.chromium.org/15001027/diff/398001/cc/resources/texture_mai...
cc/resources/texture_mailbox.h:37: gfx::Size size,
On 2013/06/07 14:56:42, danakj wrote:
> I feel like including the size inside the TextureMailbox is going to be
awkward.
> 
> When I want to give you a texture mailbox, if it contains a texture, I'll have
> to pass a size along with it. If it contains a shared memory, then I don't.
But
> I'll still have a size parameter there to handle the texture case, it's just
> unused?
> 
> Like HereIsATextureMailbox(TextureMailbox& mailbox, gfx::Size
> size_if_mailbox_is_a_texture). That's pretty awkward. Since the hardware path
> needs a size passed along with it, would it make sense to remove the size from
> here for the shared memory case and require it to be passed separately as
well?

We talked this over offline.

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:352:
shared_memory.reset(NULL);
On 2013/06/07 01:26:28, piman wrote:
> nit: you actually don't need this, it'll go away by itself when going out of
> scope.
> Feel free to leave a comment explaining it if you think that makes it clearer.

I knew that. I thought leaving it makes the code clearer. Gone.

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1538: if
(!compositor) {
On 2013/06/07 01:26:28, piman wrote:
> I believe I mentioned that earlier. GetCompositor will return NULL if the
RWHVA
> is not in the tree. In that case we still want to get the new frame, so that
> when the RWHVA is put back into the tree, we can show the latest renderer
frame,
> not the one from eons ago before the RWHVA was removed from the tree.

Done.

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1580:
base::Bind(ReleaseMailbox, Passed(shared_memory.Pass()),
On 2013/06/07 01:26:28, piman wrote:
> nit: Passed(shared_memory) should work.

It doesn't.

https://codereview.chromium.org/15001027/diff/398001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/15001027/diff/398001/ui/aura/window.cc#newcode41
ui/aura/window.cc:41: shared_memory.reset(NULL);
On 2013/06/07 01:26:28, piman wrote:
> nit: same as in RWHVA you actually don't need this, it'll go away by itself
when
> going out of scope.

Done.

[piman, 2013-06-07 23:29:59]

LGTM, thanks for the new tests.

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/15001027/diff/398001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:151: mailbox.IsSharedMemory() ||
On 2013/06/07 21:49:21, slavi wrote:
> On 2013/06/07 01:26:28, piman wrote:
> > In which case do you need to call SetTextureMailbox with the same shared
> memory?
> > Are you sure those cases are properly handled by the corresponding callback?
> > 
> > That's the reason this DCHECK exists for the texture case - it's not that it
> > can't be handled through cc, it's that most likely it is not correctly
handled
> > by the caller side (it's indeed not trivial to do so without refcounting and
> > other things).
> 
> This check was failing after tab detach/reattach. Changed it to only check if
> it's the owner of the mailbox.

What this means is that detaching/reattaching a tab will draw blank until you
receive a new frame from the renderer. We'll need to fix that. Can you file a
bug?

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/15001027/diff/398001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1580:
base::Bind(ReleaseMailbox, Passed(shared_memory.Pass()),
On 2013/06/07 21:49:21, slavi wrote:
> On 2013/06/07 01:26:28, piman wrote:
> > nit: Passed(shared_memory) should work.
> 
> It doesn't.

ok, Passed(&shared_memory).

[commit-bot: I haz the power, 2013-06-07 23:56:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/15001027/306002

[commit-bot: I haz the power, 2013-06-08 00:17:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-08 00:18:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/15001027/306002

[commit-bot: I haz the power, 2013-06-08 04:38:53]

Change committed as 205010

[piman, 2013-06-11 19:40:17]

jschuh/ben, can you do a post-facto review? I don't think this should have TBR'd

[jschuh, 2013-06-11 23:35:20]

On 2013/06/11 19:40:17, piman wrote:
> jschuh/ben, can you do a post-facto review? I don't think this should have
TBR'd

Yeah, it shouldn't have been TBR'd. But the handle passing looks correct on the
ipc security front, so retroactive lgtm.

[Ben Goodger (Google), 2013-06-12 17:34:18]

aura lgtm