ping

This is causing wasted efforts (see for example https://codereview.chromium.org/2723363002/), and blocking the conversion to mojo api.

Looking at the code: GpuProcessHost::ForceShutdown() should be equivalent. Can chromecast/internal usage of StopGpuProcess() be replaced by ForceShutdown()?

+alokp


Hey - sorry we didn't get to this yet.  Moving the full code that uses it is a bigger project.

I was looking just now at whether we could literally make a small helper function in chromecast/ that calls it:
(our internal code) -> helper function -> StopGpuProcess

If we did that, you could just update the helper function to call the new mojo API, and we could possibly add some tests.


However, when I look at doing that, there is another problem, which is that our callsite doesn't go through content/public APIs.  I think we wrote it originally before we had dependency checking on our internal code (and then later added //nogncheck etc when we enabled that checking) ... but this makes it hard to just move into an upstream helper as-is.

Our calling code looks like this, btw:

  if (content::GpuProcessHostUIShim::GetOneInstance()) {
    content::GpuProcessHostUIShim::GetOneInstance()->StopGpuProcess(on_stopped_callback);
  } else {
    on_stopped_callback.Run();
  }


If you want to move quickly, then sure ... an equivalent mojo API that we can copy in would be fine.  But would also appreciate your thoughts on the right way to solve the dependency issue.  Also, I think that the code above has some race conditions, where the GPU process is currently starting, but hasn't finished, so GetOneInstance returns nullptr, and this code assumes it's stopped.  It would be great to eliminate that too :)

OK. So the difference between GpuProcessHost::ForceShutdown() and GpuProcessHost::StopGpuProcess() is that ForceShutdown() does not provide a callback when the gpu-process is actually shut down. And //chromecast/internal seems to use the callback. So it's probably going to be necessary to introduce a mojom API (which remains unused in the tree) after all. Sad! [ :-) ]

Alternatively, and I am probably pushing it a little here, but can //chromecast/internal always just run the callback? i.e. always assume the process is torn down immediately?

/cc+ jbauman@, and piman@ (welcome back!)

The callback is used currently.  It's used for input switching on Cast TV: we have exclusive access to the screen, and when switching away from cast input, we need to destroy all GL contexts to release access for other applications.  We use the callback to notify the system when that's completed.

As I said above, happy to put some time into moving this in tree, but would appreciate some guidance on the best way to do this.  It doesn't seem like our internal code accessing content::GpuProcessHostUIShim directly can be done upstream.

I'd be ok with a specific/scoped public content API (With a test? And explanation of what it's for).

Something like content::StopGpuProcess(base::Closure callback) which internally handles the GpuProcessHostUIShim logic.

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

commit e6f6e1048099d2ce82e0b5d70d69206f5f3077fe
Author: sadrul <sadrul@chromium.org>
Date: Sat Mar 11 01:09:56 2017

gpu: Replace GpuMsg_Finalize ipc with equivalent mojom api

Remove GpuMsg_Finalize, and associated code (e.g.
GpuProcessHost*::StopGpuProcess()). Introduce GpuService::Stop() mojom
api in its place, and content::StopGpuProcess() so that content embedders
(e.g. chromecast) can stop the gpu process when desired.

BUG=639941,  643746 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2723363002
Cr-Commit-Position: refs/heads/master@{#456242}

[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/frame_host/debug_urls.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/gpu/gpu_data_manager_impl_private.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/gpu/gpu_ipc_browsertests.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/gpu/gpu_process_host.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/gpu/gpu_process_host.h
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/gpu/gpu_process_host_ui_shim.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/gpu/gpu_process_host_ui_shim.h
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/common/gpu_host_messages.h
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/gpu/gpu_child_thread.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/gpu/gpu_child_thread.h
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/public/browser/gpu_utils.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/content/public/browser/gpu_utils.h
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/services/ui/gpu/gpu_service.cc
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/services/ui/gpu/gpu_service.h
[modify] https://crrev.com/e6f6e1048099d2ce82e0b5d70d69206f5f3077fe/services/ui/gpu/interfaces/gpu_service.mojom