gpu: Some code cleanups.

gpu: Some code cleanups.

. Remove unused BrowserGpuChannelHostFactory::AddFilterOnIO().
. Use a weak-ptr to bind callbacks to GpuProcessHost, instead of looking
  up the GpuProcessHost from the host-id (using GpuProcessHost::FromID()).
. Remove some null checks for |gpu_channel_manager_| in GpuService,
  because it is created during initialization, and destroyed only in the
  dtor. None of the other callbacks should trigger before initialization.
  So the null checks should not be necessary.

BUG=none

Review-Url: https://codereview.chromium.org/2801943002
Cr-Commit-Position: refs/heads/master@{#463049}
Committed: https://chromium.googlesource.com/chromium/src/+/6a900aa3c26f4a4f29477926c8389e29ed8a7eff

[sadrul, 2017-04-06 04:27:15]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 04:27:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 05:19:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 05:19:20]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-04-06 05:56:13]

sadrul@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2017-04-06 20:51:14]

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (left):

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:375:
host->AddFilter(filter.get());
nit: can we remove GpuProcessHost::AddFilter now? I don't think it's used.

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:623:
base::Bind(base::IgnoreResult(&GpuProcessHost::Send),
So, if the weak_ptr is null, the message will be leaked. Do we still need a
trampoline, maybe?

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service.cc
File services/ui/gpu/gpu_service.cc (left):

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service...
services/ui/gpu/gpu_service.cc:235: if (gpu_channel_manager_)
This used to be necessary (in GpuChildThread) because some IPCs could be
pipelined (from the host) after initialization, but initialization could fail
(so we wouldn't create the GpuChannelManager), so the GPU process could see
these messages come in before the process exited.

Is it not possible any more?

[sadrul, 2017-04-07 01:47:55]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 01:48:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-04-07 02:16:12]

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (left):

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:375:
host->AddFilter(filter.get());
On 2017/04/06 20:51:14, piman wrote:
> nit: can we remove GpuProcessHost::AddFilter now? I don't think it's used.

Done.

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:623:
base::Bind(base::IgnoreResult(&GpuProcessHost::Send),
On 2017/04/06 20:51:14, piman wrote:
> So, if the weak_ptr is null, the message will be leaked. Do we still need a
> trampoline, maybe?

Good catch! Done. (... although maybe IPC::Sender::Send() could be updated to
take a unique_ptr<Message>)

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service.cc
File services/ui/gpu/gpu_service.cc (left):

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service...
services/ui/gpu/gpu_service.cc:235: if (gpu_channel_manager_)
On 2017/04/06 20:51:14, piman wrote:
> This used to be necessary (in GpuChildThread) because some IPCs could be
> pipelined (from the host) after initialization, but initialization could fail
> (so we wouldn't create the GpuChannelManager), so the GPU process could see
> these messages come in before the process exited.
> 
> Is it not possible any more?

We tear-down the mojo connection if the initialization fails [1]. So I think
this should now be safe?

[1]
https://cs.chromium.org/chromium/src/content/gpu/gpu_child_thread.cc?type=cs&...

[commit-bot: I haz the power, 2017-04-07 02:43:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 02:43:38]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-04-07 02:53:04]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 02:53:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 04:35:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 04:35:45]

Dry run: This issue passed the CQ dry run.

[piman, 2017-04-07 18:16:41]

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2801943002/diff/1/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:623:
base::Bind(base::IgnoreResult(&GpuProcessHost::Send),
On 2017/04/07 02:16:12, sadrul wrote:
> On 2017/04/06 20:51:14, piman wrote:
> > So, if the weak_ptr is null, the message will be leaked. Do we still need a
> > trampoline, maybe?
> 
> Good catch! Done. (... although maybe IPC::Sender::Send() could be updated to
> take a unique_ptr<Message>)

That would be ideal... but I think there's so many implementations and callers
to fixup, and given that this is essentially deprecated (vs mojo), so usage
should go down over time, it may not be worth it to go and fix.

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service.cc
File services/ui/gpu/gpu_service.cc (left):

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service...
services/ui/gpu/gpu_service.cc:235: if (gpu_channel_manager_)
On 2017/04/07 02:16:12, sadrul wrote:
> On 2017/04/06 20:51:14, piman wrote:
> > This used to be necessary (in GpuChildThread) because some IPCs could be
> > pipelined (from the host) after initialization, but initialization could
fail
> > (so we wouldn't create the GpuChannelManager), so the GPU process could see
> > these messages come in before the process exited.
> > 
> > Is it not possible any more?
> 
> We tear-down the mojo connection if the initialization fails [1]. So I think
> this should now be safe?
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/gpu/gpu_child_thread.cc?type=cs&...

Ok. It does rely on the fact that gpu_service_.reset() happens in the same task
as the gpu_service_->Bind(std::move(request)); there. Worth a comment and/or
moving the dead_on_arrival_ path before the Bind.

[piman, 2017-04-07 18:16:51]

lgtm

[sadrul, 2017-04-07 22:06:50]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2017-04-07 22:07:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-04-07 22:09:00]

The CQ bit was unchecked by sadrul@chromium.org

[sadrul, 2017-04-07 22:13:30]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 22:13:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-04-07 22:57:03]

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service.cc
File services/ui/gpu/gpu_service.cc (left):

https://codereview.chromium.org/2801943002/diff/1/services/ui/gpu/gpu_service...
services/ui/gpu/gpu_service.cc:235: if (gpu_channel_manager_)
On 2017/04/07 18:16:41, piman wrote:
> On 2017/04/07 02:16:12, sadrul wrote:
> > On 2017/04/06 20:51:14, piman wrote:
> > > This used to be necessary (in GpuChildThread) because some IPCs could be
> > > pipelined (from the host) after initialization, but initialization could
> fail
> > > (so we wouldn't create the GpuChannelManager), so the GPU process could
see
> > > these messages come in before the process exited.
> > > 
> > > Is it not possible any more?
> > 
> > We tear-down the mojo connection if the initialization fails [1]. So I think
> > this should now be safe?
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/content/gpu/gpu_child_thread.cc?type=cs&...
> 
> Ok. It does rely on the fact that gpu_service_.reset() happens in the same
task
> as the gpu_service_->Bind(std::move(request)); there. Worth a comment and/or
> moving the dead_on_arrival_ path before the Bind.

Sounds good. Moved Bind() to happen after the check for dead_on_arrival_, and
added a comment.

[sadrul, 2017-04-07 23:01:11]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-04-07 23:01:11]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2801943002/#ps60001
(title: ".")

[commit-bot: I haz the power, 2017-04-07 23:01:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 23:30:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491606071045580,
"parent_rev": "9d705ef39020eddba9be5d940b88d1bfd82b6834", "commit_rev":
"6a900aa3c26f4a4f29477926c8389e29ed8a7eff"}

[commit-bot: I haz the power, 2017-04-07 23:31:34]

Description was changed from

==========
gpu: Some code cleanups.

. Remove unused BrowserGpuChannelHostFactory::AddFilterOnIO().
. Use a weak-ptr to bind callbacks to GpuProcessHost, instead of looking
  up the GpuProcessHost from the host-id (using GpuProcessHost::FromID()).
. Remove some null checks for |gpu_channel_manager_| in GpuService,
  because it is created during initialization, and destroyed only in the
  dtor. None of the other callbacks should trigger before initialization.
  So the null checks should not be necessary.

BUG=none
==========

to

==========
gpu: Some code cleanups.

. Remove unused BrowserGpuChannelHostFactory::AddFilterOnIO().
. Use a weak-ptr to bind callbacks to GpuProcessHost, instead of looking
  up the GpuProcessHost from the host-id (using GpuProcessHost::FromID()).
. Remove some null checks for |gpu_channel_manager_| in GpuService,
  because it is created during initialization, and destroyed only in the
  dtor. None of the other callbacks should trigger before initialization.
  So the null checks should not be necessary.

BUG=none

Review-Url: https://codereview.chromium.org/2801943002
Cr-Commit-Position: refs/heads/master@{#463049}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a900aa3c26f4a4f29477926c838...
==========

[commit-bot: I haz the power, 2017-04-07 23:31:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6a900aa3c26f4a4f29477926c838...