New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 596290 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
mus

Blocking:
issue 586365



Sign in to add a comment

content/ (except for content/gpu) should not be able to talk to gpu/ipc/service

Project Member Reported by fsam...@chromium.org, Mar 20 2016

Issue description

Any includes to content/common/gpu from content/browser/* and content/renderer/* today should be moved to gpu/ipc/common.

This way only content/gpu can talk to gpu/ipc/service. We can enforce this through DEPS. Later when Mus needs the service code, we can add that to the DEPS file too.
 
It looks like the following files should move to gpu/ipc/common:

content/common/gpu/gpu_process_launch_causes.h
content/common/gpu/gpu_memory_uma_stats.h
content/common/gpu/gpu_surface_lookup.h
content/common/gpu/gpu_surface_lookup.cc
content/common/gpu/gpu_memory_buffer_factory.h
content/common/gpu/gpu_memory_buffer_factory.cc
content/common/gpu/accelerated_surface_buffers_swapped_params_mac.h
content/common/gpu/accelerated_surface_buffers_swapped_params_mac.cc
content/common/gpu/gpu_memory_buffer_factory_io_surface.h
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc

// This file is weird. It refers to client code (I need to investigate): "content/common/gpu/client/gpu_memory_buffer_impl.h"
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc
content/common/gpu/gpu_memory_buffer_factory_io_surface.h
content/common/gpu/gpu_memory_buffer_factory_surface_texture.h
content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc


Looks like we'll probably need to move content/common/gpu/establish_channel_params.h to gpu/ipc/common (it is used in GpuProcessHost but not included there currently, gpu_host_messages.h pulls it in).
Blocking: 586365

Comment 4 by piman@chromium.org, Mar 21 2016

I don't think establish_channel_params.h should be going to gpu/ipc, it's only used for the browser<->gpu process channel, so should be able to stay in content/
It's used in GpuChannelManager though: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu/gpu_channel_manager.cc&q=establish_channel_params.h&sq=package:chromium&type=cs&l=161

I'm not sure of a nice way to remove references to those params from gpu/ipc/service other than doing type conversion from a content/ type to a gpu/ type in gpu_child_thread which seems icky.
Well, I could just pass in a bunch of parameters too.. that also seems icky. 

Comment 7 by piman@chromium.org, Mar 21 2016

I was thinking just unpacking the parameters in GpuChildThread instead of GpuChannelManager would do.
Sure we'll do that.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27d230cc4675d3b25187e6a0c66b8a8c5f635451

commit 27d230cc4675d3b25187e6a0c66b8a8c5f635451
Author: fsamuel <fsamuel@chromium.org>
Date: Mon Mar 21 19:25:06 2016

Move establish_channel_params.{cc|h} to content/gpu

This is a browser <=> gpu struct and so it really best belongs
there.

BUG= 596290 

Review URL: https://codereview.chromium.org/1823723002

Cr-Commit-Position: refs/heads/master@{#382354}

[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/browser/gpu/gpu_process_host.cc
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/common/gpu/gpu_channel_manager.cc
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/common/gpu/gpu_channel_manager.h
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/common/gpu/gpu_channel_manager_unittest.cc
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/common/gpu/gpu_channel_unittest.cc
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/common/gpu/gpu_memory_uma_stats.h
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/content_common.gypi
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/content_gpu.gypi
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/gpu/BUILD.gn
[rename] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/gpu/establish_channel_params.cc
[rename] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/gpu/establish_channel_params.h
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/gpu/gpu_child_thread.cc
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/gpu/gpu_child_thread.h
[modify] https://crrev.com/27d230cc4675d3b25187e6a0c66b8a8c5f635451/content/gpu/gpu_host_messages.h

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c28d985e1f60cece099c984f1e1f8cd0c33c0e14

commit c28d985e1f60cece099c984f1e1f8cd0c33c0e14
Author: fsamuel <fsamuel@chromium.org>
Date: Tue Mar 22 23:05:02 2016

gpu_process_launch_causes => content/common

This is used to establish the channel and has content-speciifc concepts.
This doesn't seem to belong to content/common/gpu as per piman@'s suggestion.

BUG= 596290 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Review URL: https://codereview.chromium.org/1825183002

Cr-Commit-Position: refs/heads/master@{#382724}

[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/gpu/browser_gpu_channel_host_factory.h
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/gpu/gpu_ipc_browsertests.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/gpu/gpu_process_host.h
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/mojo/mojo_shell_context.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/renderer_host/render_message_filter.h
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/common/child_process_messages.h
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/common/gpu/client/gpu_channel_host.h
[rename] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/common/gpu_process_launch_causes.h
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/content_common.gypi
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/renderer/render_thread_impl.h
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/renderer/render_widget.cc
[modify] https://crrev.com/c28d985e1f60cece099c984f1e1f8cd0c33c0e14/content/renderer/renderer_blink_platform_impl.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efd3ce2aac23c08d4b3296a217359af73edc8629

commit efd3ce2aac23c08d4b3296a217359af73edc8629
Author: fsamuel <fsamuel@chromium.org>
Date: Wed Mar 23 03:41:04 2016

Move more files to gpu/ipc/common

This CL moves:

content/common/gpu/gpu_memory_uma_stats.h
content/common/gpu/gpu_surface_lookup.cc
content/common/gpu/gpu_surface_lookup.h

to gpu/ipc/common.

BUG= 596290 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Review URL: https://codereview.chromium.org/1823763003

Cr-Commit-Position: refs/heads/master@{#382785}

[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/app/android/child_process_service.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/browser/gpu/gpu_process_host.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/browser/gpu/gpu_process_host.h
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/browser/gpu/gpu_surface_tracker.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/browser/gpu/gpu_surface_tracker.h
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu/gpu_channel_manager_delegate.h
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu/gpu_channel_test_common.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu/gpu_channel_test_common.h
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu/gpu_memory_manager.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu/image_transport_surface_android.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/common/gpu_host_messages.h
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/content_common.gypi
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/gpu/gpu_child_thread.cc
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/content/gpu/gpu_child_thread.h
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/gpu/gpu_ipc_common.gypi
[modify] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/gpu/ipc/common/BUILD.gn
[rename] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/gpu/ipc/common/gpu_memory_uma_stats.h
[rename] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/gpu/ipc/common/gpu_surface_lookup.cc
[rename] https://crrev.com/efd3ce2aac23c08d4b3296a217359af73edc8629/gpu/ipc/common/gpu_surface_lookup.h

This patch addresses the GpuMemoryBufferFactory dependency I mentioned earlier:

https://codereview.chromium.org/1831513003/

After a bit more digging it seems accelerated_surface_buffers_swapped_params_mac.{h|cc} is better suited in content/common. I've begun work on that. Once that's done, I'll give the dependencies on content/common/gpu from content/browser and content/renderer another look and if all is good, I'll close this bug.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/521c72b8e01db6716892e82b8fb5d23864766ae7

commit 521c72b8e01db6716892e82b8fb5d23864766ae7
Author: fsamuel <fsamuel@chromium.org>
Date: Thu Mar 24 19:19:32 2016

Move AcceleratedSurfaceBuffersSwappedParams to content/common

This struct is used for browser<=>gpu IPC. As with EstablishChannelParams,
this CL moves this struct to content/common and removes its usage
from within content/common/gpu

BUG= 596290 

Review URL: https://codereview.chromium.org/1830513003

Cr-Commit-Position: refs/heads/master@{#383117}

[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/browser/gpu/gpu_process_host_ui_shim.cc
[rename] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/accelerated_surface_buffers_swapped_params_mac.cc
[rename] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/accelerated_surface_buffers_swapped_params_mac.h
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/gpu/gpu_channel_manager_delegate.h
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/gpu/gpu_channel_test_common.cc
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/gpu/gpu_channel_test_common.h
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/gpu/image_transport_surface_overlay_mac.h
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/gpu/image_transport_surface_overlay_mac.mm
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/common/gpu_host_messages.h
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/content_common.gypi
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/gpu/gpu_child_thread.cc
[modify] https://crrev.com/521c72b8e01db6716892e82b8fb5d23864766ae7/content/gpu/gpu_child_thread.h

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f862957acd612cdb4730f964a7fd07201f841361

commit f862957acd612cdb4730f964a7fd07201f841361
Author: fsamuel <fsamuel@chromium.org>
Date: Thu Mar 24 20:33:21 2016

Move buffer_presented_params_mac to content/common

This is an IPC struct used by gpu_host_messages.h so
it doesn't really belong to content/common/gpu and later
gpu/ipc/common

BUG= 596290 

Review URL: https://codereview.chromium.org/1830193002

Cr-Commit-Position: refs/heads/master@{#383135}

[rename] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/buffer_presented_params_mac.cc
[rename] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/buffer_presented_params_mac.h
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/gpu/gpu_channel_manager.cc
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/gpu/gpu_channel_manager.h
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/gpu/image_transport_surface_overlay_mac.h
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/gpu/image_transport_surface_overlay_mac.mm
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/common/gpu_host_messages.h
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/content_common.gypi
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/gpu/gpu_child_thread.cc
[modify] https://crrev.com/f862957acd612cdb4730f964a7fd07201f841361/content/gpu/gpu_child_thread.h

Status: Fixed (was: Started)
The last couple of patches were fairly simple so I don't expect them to get reverted. Marking as FIXED.

Sign in to add a comment