Simplify FakeGpuProcess* video accelerator tests

Simplify FakeGpuProcess* video accelerator tests

Make DispatchToGpuPlatformSupportTask and DispatchToGpuPlatformSupportHostTask
statics, removing the weak ptrs (and failing DCHECKS in that code) altogether.

BUG=447798
Committed: https://crrev.com/72b1eacd439fcb0bcf45ae7de2d8a9f578804ac6
Cr-Commit-Position: refs/heads/master@{#321212}

[vignatti (out of this project), 2015-03-17 17:50:49]

tiago.vignatti@intel.com changed reviewers:
+ dnicoara@chromium.org, spang@chromium.org

[vignatti (out of this project), 2015-03-17 17:50:49]

tiago.vignatti@intel.com changed required reviewers:
+ spang@chromium.org

[vignatti (out of this project), 2015-03-17 17:51:17]

dnicoara and spang, PTAL.

[dnicoara, 2015-03-17 18:15:16]

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
File ui/ozone/public/ozone_gpu_test_helper.cc (right):

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
ui/ozone/public/ozone_gpu_test_helper.cc:85:
->OnChannelEstablished(kGpuProcessHostId, gpu_task_runner_, sender);
Grr, this is where the actual issue is. Can't use a weak_ptr on multiple
threads.

Off the top of my head I want to say that the DispatchToGpuPlatformSupport*()
methods could just be static functions. 

Perhaps that's enough, though one thing that worries me is that 
OnChannelDestroyed() is never called when the FakeGpuProcess* objects go away.
Perhaps that will be needed as well.

[vignatti (out of this project), 2015-03-17 18:59:05]

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
File ui/ozone/public/ozone_gpu_test_helper.cc (right):

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
ui/ozone/public/ozone_gpu_test_helper.cc:85:
->OnChannelEstablished(kGpuProcessHostId, gpu_task_runner_, sender);
On 2015/03/17 18:15:16, dnicoara wrote:
> Grr, this is where the actual issue is. Can't use a weak_ptr on multiple
> threads.

oh, I thought this snip was correct?

 
> Off the top of my head I want to say that the DispatchToGpuPlatformSupport*()
> methods could just be static functions. 
> 
> Perhaps that's enough, though one thing that worries me is that 
> OnChannelDestroyed() is never called when the FakeGpuProcess* objects go away.
> Perhaps that will be needed as well.

hmm I think we can call OnChannelDestroyed at ~FakeGpuProcess but I don't think
for the video tests there are observers or handlers installed at the moment. So
there wouldn't be any effect on doing this.

[dnicoara, 2015-03-17 21:28:56]

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
File ui/ozone/public/ozone_gpu_test_helper.cc (right):

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
ui/ozone/public/ozone_gpu_test_helper.cc:85:
->OnChannelEstablished(kGpuProcessHostId, gpu_task_runner_, sender);
On 2015/03/17 18:59:05, vignatti wrote:
> On 2015/03/17 18:15:16, dnicoara wrote:
> > Grr, this is where the actual issue is. Can't use a weak_ptr on multiple
> > threads.
> 
> oh, I thought this snip was correct?

No, this is actually where this is very very wrong. The |sender| is going to be
called on the |gpu_task_runner_|. Since it is created on the main thread, the
weak_ptr is only guaranteed to work correctly on the main thread.

> 
>  
> > Off the top of my head I want to say that the
DispatchToGpuPlatformSupport*()
> > methods could just be static functions. 
> > 
> > Perhaps that's enough, though one thing that worries me is that 
> > OnChannelDestroyed() is never called when the FakeGpuProcess* objects go
away.
> > Perhaps that will be needed as well.
> 
> hmm I think we can call OnChannelDestroyed at ~FakeGpuProcess but I don't
think
> for the video tests there are observers or handlers installed at the moment.
So
> there wouldn't be any effect on doing this.

I don't think it would matter for the video tests either. Just wanted to point
it out in case there are other issues.

[vignatti (out of this project), 2015-03-18 15:38:29]

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
File ui/ozone/public/ozone_gpu_test_helper.cc (right):

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
ui/ozone/public/ozone_gpu_test_helper.cc:85:
->OnChannelEstablished(kGpuProcessHostId, gpu_task_runner_, sender);
On 2015/03/17 21:28:56, dnicoara wrote:
> On 2015/03/17 18:59:05, vignatti wrote:
> > On 2015/03/17 18:15:16, dnicoara wrote:
> > > Grr, this is where the actual issue is. Can't use a weak_ptr on multiple
> > > threads.
> > 
> > oh, I thought this snip was correct?
> 
> No, this is actually where this is very very wrong. The |sender| is going to
be
> called on the |gpu_task_runner_|. Since it is created on the main thread, the
> weak_ptr is only guaranteed to work correctly on the main thread.

I actually think that the way you used weak_ptr here is fine for the purpose
wanted:

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

i.e., we bound weak_ptr to be used solely through |gpu_task_runner_|, so there's
no problem with that. We don't use that in the main thread. Or am I missing
something?

[dnicoara, 2015-03-18 17:19:28]

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
File ui/ozone/public/ozone_gpu_test_helper.cc (right):

https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
ui/ozone/public/ozone_gpu_test_helper.cc:85:
->OnChannelEstablished(kGpuProcessHostId, gpu_task_runner_, sender);
On 2015/03/18 15:38:29, vignatti wrote:
> On 2015/03/17 21:28:56, dnicoara wrote:
> > On 2015/03/17 18:59:05, vignatti wrote:
> > > On 2015/03/17 18:15:16, dnicoara wrote:
> > > > Grr, this is where the actual issue is. Can't use a weak_ptr on multiple
> > > > threads.
> > > 
> > > oh, I thought this snip was correct?
> > 
> > No, this is actually where this is very very wrong. The |sender| is going to
> be
> > called on the |gpu_task_runner_|. Since it is created on the main thread,
the
> > weak_ptr is only guaranteed to work correctly on the main thread.
> 
> I actually think that the way you used weak_ptr here is fine for the purpose
> wanted:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> 
> i.e., we bound weak_ptr to be used solely through |gpu_task_runner_|, so
there's
> no problem with that. We don't use that in the main thread. Or am I missing
> something?

You're right, I'm getting confused by the code :(

Though we still can't do this by posting a task to invalidate the weak ptrs and
wait since |gpu_task_runner == ui_task_runner| should be supported.

I'm tempted to say we might need another object to act as the sender and take
the weak_ptr to that. Then in the dtor we could post a task with
DeletePointer(...). spang@ what do you think?

[spang, 2015-03-18 17:48:53]

On 2015/03/18 17:19:28, dnicoara wrote:
>
https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
> File ui/ozone/public/ozone_gpu_test_helper.cc (right):
> 
>
https://codereview.chromium.org/1017673003/diff/1/ui/ozone/public/ozone_gpu_t...
> ui/ozone/public/ozone_gpu_test_helper.cc:85:
> ->OnChannelEstablished(kGpuProcessHostId, gpu_task_runner_, sender);
> On 2015/03/18 15:38:29, vignatti wrote:
> > On 2015/03/17 21:28:56, dnicoara wrote:
> > > On 2015/03/17 18:59:05, vignatti wrote:
> > > > On 2015/03/17 18:15:16, dnicoara wrote:
> > > > > Grr, this is where the actual issue is. Can't use a weak_ptr on
multiple
> > > > > threads.
> > > > 
> > > > oh, I thought this snip was correct?
> > > 
> > > No, this is actually where this is very very wrong. The |sender| is going
to
> > be
> > > called on the |gpu_task_runner_|. Since it is created on the main thread,
> the
> > > weak_ptr is only guaranteed to work correctly on the main thread.
> > 
> > I actually think that the way you used weak_ptr here is fine for the purpose
> > wanted:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> > 
> > i.e., we bound weak_ptr to be used solely through |gpu_task_runner_|, so
> there's
> > no problem with that. We don't use that in the main thread. Or am I missing
> > something?
> 
> You're right, I'm getting confused by the code :(
> 
> Though we still can't do this by posting a task to invalidate the weak ptrs
and
> wait since |gpu_task_runner == ui_task_runner| should be supported.
> 
> I'm tempted to say we might need another object to act as the sender and take
> the weak_ptr to that. Then in the dtor we could post a task with
> DeletePointer(...). spang@ what do you think?

This usage is definitely haywire. We should make
DispatchToGpuPlatformSupportTask & DispatchToGpuPlatformSupportHostTask static
and remove the weak ptrs altogether.

[vignatti (out of this project), 2015-03-18 20:26:35]

On 2015/03/18 17:48:53, spang wrote:
> This usage is definitely haywire. We should make
> DispatchToGpuPlatformSupportTask & DispatchToGpuPlatformSupportHostTask static
> and remove the weak ptrs altogether.

I think this idea made everything much simpler after all. PTAL in #2. Thanks.

[spang, 2015-03-18 21:00:22]

lgtm

[vignatti (out of this project), 2015-03-18 21:02:53]

The CQ bit was checked by tiago.vignatti@intel.com

[commit-bot: I haz the power, 2015-03-18 21:03:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017673003/20001

[commit-bot: I haz the power, 2015-03-18 21:40:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-18 21:41:12]

Patchset 2 (id:??) landed as
https://crrev.com/72b1eacd439fcb0bcf45ae7de2d8a9f578804ac6
Cr-Commit-Position: refs/heads/master@{#321212}