Reland r206537 - cc: Don't return mailboxes if the TextureLayer is removed from the tree.

Reland r206537 - cc: Don't return mailboxes if the TextureLayer is removed from the tree.

When the TextureLayer is removed from the tree, the TextureLayerImpl is
destroyed, so it will return the mailbox to the main thread. Prior to this patch
we would return the mailbox to the client. When re-attaching the TextureLayer,
we would then not push the mailbox to the new TextureLayerImpl, and stop drawing
the layer.
Instead, keep the mailbox on the TextureLayer. If the TextureLayer is re-attached to the tree, we
push the mailbox again (which is ok to do now).
Only after the impl side has returned the mailbox *and* the mailbox is replaced
on the TextureLayer (or it is destroyed), will we call the callback.

To do that we add a refcounted MailboxHolder whose job is to call the callback
when it's destroyed. It's referenced by the TextureLayer itself, while being the
current mailbox, and the closures we give to the impl thread.
    
BUG=249535
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207081

[piman, 2013-06-17 23:10:40]

dana: PTAL
PS1 is the original issue (after rebase). PS2 has the fix.

[danakj, 2013-06-18 00:53:24]

https://codereview.chromium.org/16888015/diff/2001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/16888015/diff/2001/cc/layers/texture_layer.cc...
cc/layers/texture_layer.cc:261: message_loop_->PostTask(FROM_HERE, base::Bind(
I guess the downside here is we can't DCHECK that we called this function, we
would just leak the MailboxHolder if we failed to call it.

Maybe we could get around that by keeping track in here if the impl thread has a
ref on the object, and if it does when we get to the ~ then we post task in the
~ to the main thread, otherwise run the callback immediately.
GetCallbackForImplThread() would just have to set a bool ref_on_impl_thread_ =
true;

Is that idea crazy or helpful, I'm not sure?

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

https://codereview.chromium.org/16888015/diff/2001/cc/layers/texture_layer.h#...
cc/layers/texture_layer.h:113: scoped_refptr<base::MessageLoopProxy> const
message_loop_;
nit: const first (why const?)

[piman, 2013-06-18 02:10:58]

PTAL

https://codereview.chromium.org/16888015/diff/2001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/16888015/diff/2001/cc/layers/texture_layer.cc...
cc/layers/texture_layer.cc:261: message_loop_->PostTask(FROM_HERE, base::Bind(
On 2013/06/18 00:53:24, danakj wrote:
> I guess the downside here is we can't DCHECK that we called this function, we
> would just leak the MailboxHolder if we failed to call it.
> 
> Maybe we could get around that by keeping track in here if the impl thread has
a
> ref on the object, and if it does when we get to the ~ then we post task in
the
> ~ to the main thread, otherwise run the callback immediately.
> GetCallbackForImplThread() would just have to set a bool ref_on_impl_thread_ =
> true;
> 
> Is that idea crazy or helpful, I'm not sure?

Following our discussion, see the new version. Not entirely sure whether the
MainThreadReference is overkill or not...

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

https://codereview.chromium.org/16888015/diff/2001/cc/layers/texture_layer.h#...
cc/layers/texture_layer.h:113: scoped_refptr<base::MessageLoopProxy> const
message_loop_;
On 2013/06/18 00:53:24, danakj wrote:
> nit: const first (why const?)

Done.

[danakj, 2013-06-18 02:20:20]

Cool, LGTM

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:160: if (!host && uses_mailbox_ && holder_ref_.get())
nit: don't need .get() for scoped_ptr

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:166: return (client_ || texture_id_ ||
holder_ref_.get()) &&
nit: don't need .get() for scoped_ptr

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:239: internal_references_(1),
nit: start with 0 here, and have MainThreadRef constructor do InternalAddRef()?
Or add comment here that this is for the MainThreadReference.

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:264: DCHECK(internal_references_);
nit: DCHECK_GT or DCHECK_NE 0?

[piman, 2013-06-18 02:53:01]

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:160: if (!host && uses_mailbox_ && holder_ref_.get())
On 2013/06/18 02:20:20, danakj wrote:
> nit: don't need .get() for scoped_ptr

Done.

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:166: return (client_ || texture_id_ ||
holder_ref_.get()) &&
On 2013/06/18 02:20:20, danakj wrote:
> nit: don't need .get() for scoped_ptr

Done.

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:239: internal_references_(1),
On 2013/06/18 02:20:20, danakj wrote:
> nit: start with 0 here, and have MainThreadRef constructor do
InternalAddRef()?
> Or add comment here that this is for the MainThreadReference.

Done.

https://codereview.chromium.org/16888015/diff/16001/cc/layers/texture_layer.c...
cc/layers/texture_layer.cc:264: DCHECK(internal_references_);
On 2013/06/18 02:20:20, danakj wrote:
> nit: DCHECK_GT or DCHECK_NE 0?

Done.

[danakj, 2013-06-18 03:00:03]

LGTM!

[commit-bot: I haz the power, 2013-06-18 18:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/16888015/26001

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

Change committed as 207081