base::Bind() conversion for remaining GPU files.

base::Bind() conversion for remaining GPU files.

BUG=none
TEST=trybots


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111067

[dcheng, 2011-11-18 02:06:07]

This is a fairly straightforward conversion; there's nothing too interesting in
this one.

[apatrick_chromium, 2011-11-18 17:26:58]

My only complaint is that GpuScheduler supports weak pointers to it and
references to it need not be unretained. Though if the conversion preserves the
original semantics then LGTM.

[dcheng, 2011-11-18 23:18:30]

On 2011/11/18 17:26:58, apatrick_chromium wrote:
> My only complaint is that GpuScheduler supports weak pointers to it and
> references to it need not be unretained. Though if the conversion preserves
the
> original semantics then LGTM.

The original NewCallback() did no refcounting of its arguments and also didn't
default to using weak pointers. I see two ways to proceed:

1) Use base::Unretained() and preserve existing semantics. In this case, a
CommandBufferService/GpuScheduler must remain alive as long as there are pending
callbacks referring to it. If it gets deleted, we'll get a crash or a
use-after-free from ASAN, but we'll know about it and be able to fix it.

2) Use AsWeakPtr(). We won't crash, but it's possible there's a callback that
absolutely needs to run and doesn't because the object gets deleted before the
callback runs. With a weak_ptr, we won't even know, because we won't get crashes
and the callback will just silently disappear.

Since you know this code, could you clarify the lifetime guarantees on
CommandBufferService/GpuScheduler? Is it OK for callbacks to be automatically
cancelled when the corresponding object is deleted?

[apatrick_chromium, 2011-11-21 19:57:32]

On 2011/11/18 23:18:30, dcheng wrote:
> On 2011/11/18 17:26:58, apatrick_chromium wrote:
> > My only complaint is that GpuScheduler supports weak pointers to it and
> > references to it need not be unretained. Though if the conversion preserves
> the
> > original semantics then LGTM.
> 
> The original NewCallback() did no refcounting of its arguments and also didn't
> default to using weak pointers. I see two ways to proceed:
> 
> 1) Use base::Unretained() and preserve existing semantics. In this case, a
> CommandBufferService/GpuScheduler must remain alive as long as there are
pending
> callbacks referring to it. If it gets deleted, we'll get a crash or a
> use-after-free from ASAN, but we'll know about it and be able to fix it.
> 
> 2) Use AsWeakPtr(). We won't crash, but it's possible there's a callback that
> absolutely needs to run and doesn't because the object gets deleted before the
> callback runs. With a weak_ptr, we won't even know, because we won't get
crashes
> and the callback will just silently disappear.
> 
> Since you know this code, could you clarify the lifetime guarantees on
> CommandBufferService/GpuScheduler? Is it OK for callbacks to be automatically
> cancelled when the corresponding object is deleted?

I think the lifetime guarantees are such that the callbacks will not outlive the
GpuScheduler so this change is safe. GpuScheduler only got support for WeakPtr
recently. That's why the callbacks are not using it at the moment. LGTM.

[commit-bot: I haz the power, 2011-11-21 20:05:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8586048/13002

[commit-bot: I haz the power, 2011-11-21 20:05:11]

Presubmit check for 8586048-13002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc

Presubmit checks took 3.1s to calculate.

[dcheng, 2011-11-21 20:06:28]

+tony for webkit/* changes.

[tony, 2011-11-21 21:55:43]

LGTM

[commit-bot: I haz the power, 2011-11-21 21:57:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8586048/13002

[commit-bot: I haz the power, 2011-11-21 21:58:05]

Presubmit check for 8586048-13002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc

Presubmit checks took 2.0s to calculate.

[dcheng, 2011-11-21 21:59:25]

As it turns out, I can't read. gpu != glue

[Ken Russell (switch to Gerrit), 2011-11-21 23:05:00]

webkit/gpu LGTM

[commit-bot: I haz the power, 2011-11-21 23:11:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8586048/13002

[commit-bot: I haz the power, 2011-11-22 01:24:53]

Change committed as 111067