content: Remove some unused code.

content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place: it is
  used to find the associated GpuProcessHost. However, if it does not
  exist anymore, then the code simply creates a new GpuProcessHost
  instance and uses that. The code can just use GpuProcessHost::Get()
  instead, which returns the existing sandboxed gpu process if it exists,
  and creates a new one otherwise. So using |gpu_process_id_| is not
  actually necessary.

BUG=643746
Committed: https://crrev.com/5196acbe6971a34399bd56a0e5fe801652b2346d
Cr-Commit-Position: refs/heads/master@{#434085}

[sadrul, 2016-11-19 01:20:26]

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

[commit-bot: I haz the power, 2016-11-19 01:20:55]

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

[sadrul, 2016-11-19 01:22:15]

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

[sadrul, 2016-11-19 01:29:09]

Description was changed from

==========
content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place, but
  it's not actually needed. So remove it.

BUG=643746
==========

to

==========
content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place: it is
  used to find the associated GpuProcessHost. However, if it does not
  exist anymore, then the code simply creates a new GpuProcessHost
  instance and uses that. The code can just use GpuProcessHost::Get()
  instead, which returns the existing sandboxed gpu process if it exists,
  and creates a new one otherwise. So using |gpu_process_id_| is not
  actually necessary.

BUG=643746
==========

[commit-bot: I haz the power, 2016-11-19 02:30:27]

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

[commit-bot: I haz the power, 2016-11-19 02:30:28]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-22 00:52:24]

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

[piman, 2016-11-22 00:52:25]

+reveman for the ChildProcessHostImpl changes. I'm not sure if we want to remove
it, if we want to create GMBs from the GPU process (for fsamuel's
InProcessCommandBuffer changes for example), we'd still need something like it.
We could also want to use it from ppapi plugin processes - though at this point
it's unlikely we'll spend time doing it.

If we decide to go ahead, we should move ChildGpuMemoryBufferManager to
content/renderer so that someone doesn't try to use it from arbitrary child
processes.

[sadrul, 2016-11-22 01:34:02]

On 2016/11/22 00:52:25, piman wrote:
> +reveman for the ChildProcessHostImpl changes. I'm not sure if we want to
remove
> it, if we want to create GMBs from the GPU process (for fsamuel's
> InProcessCommandBuffer changes for example), we'd still need something like
it.
> We could also want to use it from ppapi plugin processes - though at this
point
> it's unlikely we'll spend time doing it.
> 
> If we decide to go ahead, we should move ChildGpuMemoryBufferManager to
> content/renderer so that someone doesn't try to use it from arbitrary child
> processes.

I considered doing that. But I am also working on removing
ChildGpuMemoryBufferManager altogether (by changing it to use mojo IPC:
https://codereview.chromium.org/2514923002/). So decided to avoid the churn
here. But I am happy to move this into content/renderer/ as well.

[piman, 2016-11-22 01:51:18]

On Mon, Nov 21, 2016 at 5:34 PM, <sadrul@chromium.org> wrote:

> On 2016/11/22 00:52:25, piman wrote:
> > +reveman for the ChildProcessHostImpl changes. I'm not sure if we want to
> remove
> > it, if we want to create GMBs from the GPU process (for fsamuel's
> > InProcessCommandBuffer changes for example), we'd still need something
> like
> it.
> > We could also want to use it from ppapi plugin processes - though at this
> point
> > it's unlikely we'll spend time doing it.
> >
> > If we decide to go ahead, we should move ChildGpuMemoryBufferManager to
> > content/renderer so that someone doesn't try to use it from arbitrary
> child
> > processes.
>
> I considered doing that. But I am also working on removing
> ChildGpuMemoryBufferManager altogether (by changing it to use mojo IPC:
> https://codereview.chromium.org/2514923002/). So decided to avoid the
> churn
> here. But I am happy to move this into content/renderer/ as well.
>

I see. No need for churn. If GpuService can handle those requests, any
reason not to expose it to all child processes rather than just renderers?


> https://codereview.chromium.org/2514153002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2016-11-22 01:59:46]

On 2016/11/22 01:51:18, piman wrote:
> On Mon, Nov 21, 2016 at 5:34 PM, <mailto:sadrul@chromium.org> wrote:
> 
> > On 2016/11/22 00:52:25, piman wrote:
> > > +reveman for the ChildProcessHostImpl changes. I'm not sure if we want to
> > remove
> > > it, if we want to create GMBs from the GPU process (for fsamuel's
> > > InProcessCommandBuffer changes for example), we'd still need something
> > like
> > it.
> > > We could also want to use it from ppapi plugin processes - though at this
> > point
> > > it's unlikely we'll spend time doing it.
> > >
> > > If we decide to go ahead, we should move ChildGpuMemoryBufferManager to
> > > content/renderer so that someone doesn't try to use it from arbitrary
> > child
> > > processes.
> >
> > I considered doing that. But I am also working on removing
> > ChildGpuMemoryBufferManager altogether (by changing it to use mojo IPC:
> > https://codereview.chromium.org/2514923002/). So decided to avoid the
> > churn
> > here. But I am happy to move this into content/renderer/ as well.
> >
> 
> I see. No need for churn. If GpuService can handle those requests, any
> reason not to expose it to all child processes rather than just renderers?

Nope (just the place that sets up the connection will be different, the relevant
code itself should be the same). I can do that when I hook up GpuService. But
since the chrome-style IPC are handled in two different places right now
(RenderMessageFilter for renderers, ChildProcessHostImpl for non-renderers), and
since one of them isn't actually used, I would prefer to remove it first if that
sgty.

[reveman, 2016-11-22 09:11:01]

ChildProcessHostImpl changes lgtm as long as the new mojo allocation can easily
be extended to support ppapi processes in the future if we want that.

For GpuProcess allocation of GMBs we probably want to use shmem and GMB factory
directly and we can create a GMB manager implementation that does that when
needed.

[sadrul, 2016-11-22 19:49:00]

On 2016/11/22 09:11:01, reveman wrote:
> ChildProcessHostImpl changes lgtm as long as the new mojo allocation can
easily
> be extended to support ppapi processes in the future if we want that.

Yep, that should not be difficult to do.

> For GpuProcess allocation of GMBs we probably want to use shmem and GMB
factory
> directly and we can create a GMB manager implementation that does that when
> needed.

Indeed.

[piman, 2016-11-22 22:54:44]

lgtm

[sadrul, 2016-11-22 23:23:49]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:24:53]

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, 2016-11-23 02:27:29]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1479857029334470, "parent_rev":
"7f4ce97a7e46b9c16281fadc03db893a56302219", "commit_rev":
"09d8fd0c6b17add5b52ddc13770cd968d87b6469"}

[commit-bot: I haz the power, 2016-11-23 02:28:11]

Description was changed from

==========
content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place: it is
  used to find the associated GpuProcessHost. However, if it does not
  exist anymore, then the code simply creates a new GpuProcessHost
  instance and uses that. The code can just use GpuProcessHost::Get()
  instead, which returns the existing sandboxed gpu process if it exists,
  and creates a new one otherwise. So using |gpu_process_id_| is not
  actually necessary.

BUG=643746
==========

to

==========
content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place: it is
  used to find the associated GpuProcessHost. However, if it does not
  exist anymore, then the code simply creates a new GpuProcessHost
  instance and uses that. The code can just use GpuProcessHost::Get()
  instead, which returns the existing sandboxed gpu process if it exists,
  and creates a new one otherwise. So using |gpu_process_id_| is not
  actually necessary.

BUG=643746
==========

[commit-bot: I haz the power, 2016-11-23 02:28:12]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-23 02:30:38]

Description was changed from

==========
content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place: it is
  used to find the associated GpuProcessHost. However, if it does not
  exist anymore, then the code simply creates a new GpuProcessHost
  instance and uses that. The code can just use GpuProcessHost::Get()
  instead, which returns the existing sandboxed gpu process if it exists,
  and creates a new one otherwise. So using |gpu_process_id_| is not
  actually necessary.

BUG=643746
==========

to

==========
content: Remove some unused code.

. ChildGpuMemoryBufferManager is used only by renderer child processes
  (only RenderThreadImpl creates and uses it). So remove the code in the
  browser process that handles messages from it for non-renderer
  processes.

. RenderMessageFilter::gpu_process_id_ is used in only one place: it is
  used to find the associated GpuProcessHost. However, if it does not
  exist anymore, then the code simply creates a new GpuProcessHost
  instance and uses that. The code can just use GpuProcessHost::Get()
  instead, which returns the existing sandboxed gpu process if it exists,
  and creates a new one otherwise. So using |gpu_process_id_| is not
  actually necessary.

BUG=643746

Committed: https://crrev.com/5196acbe6971a34399bd56a0e5fe801652b2346d
Cr-Commit-Position: refs/heads/master@{#434085}
==========

[commit-bot: I haz the power, 2016-11-23 02:30:39]

Patchset 1 (id:??) landed as
https://crrev.com/5196acbe6971a34399bd56a0e5fe801652b2346d
Cr-Commit-Position: refs/heads/master@{#434085}