gpu: Move GpuWatchdogThread into //gpu/ipc/service from content.

gpu: Move GpuWatchdogThread into //gpu/ipc/service from content.

GpuWatchdogThread doesn't have any dependency on content. So move it into
the gpu service component, so that it can be used by both content-gpu and
mus-gpu.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/2fb7e150d959023d9a793a74e30cea42798c56b4
Cr-Commit-Position: refs/heads/master@{#415149}

[sadrul, 2016-08-28 04:52:42]

Description was changed from

==========
gpu: Move GpuWatchdogThread into //gpu/ipc/service from content.

GpuWatchdogThread doesn't have any dependency on content. So move it into
the gpu service component, so that it can be used by both content-gpu and
mus-gpu.

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

to

==========
gpu: Move GpuWatchdogThread into //gpu/ipc/service from content.

GpuWatchdogThread doesn't have any dependency on content. So move it into
the gpu service component, so that it can be used by both content-gpu and
mus-gpu.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sadrul, 2016-08-28 04:52:46]

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

[commit-bot: I haz the power, 2016-08-28 04:52:52]

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, 2016-08-28 05:32:54]

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

[commit-bot: I haz the power, 2016-08-28 05:32:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sadrul, 2016-08-28 12:59:00]

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

[commit-bot: I haz the power, 2016-08-28 12:59:07]

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

[sadrul, 2016-08-28 13:01:57]

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

[sadrul, 2016-08-28 13:01:58]

https://codereview.chromium.org/2286063003/diff/20001/gpu/ipc/service/gpu_wat...
File gpu/ipc/service/gpu_watchdog_thread.h (right):

https://codereview.chromium.org/2286063003/diff/20001/gpu/ipc/service/gpu_wat...
gpu/ipc/service/gpu_watchdog_thread.h:36: NON_EXPORTED_BASE(public GpuWatchdog),
There's no other subclass of GpuWatchdog, and now that both the interface and
the only implementation is in gpu, and there are no users of GpuWatchdog from
out of gpu, maybe I should get rid of GpuWatchdog interface?

[commit-bot: I haz the power, 2016-08-28 14:16:55]

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

[commit-bot: I haz the power, 2016-08-28 14:16:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sadrul, 2016-08-29 15:47:56]

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

[sadrul, 2016-08-29 15:47:56]

+fsamuel@ for services/ui/gpu

[Fady Samuel, 2016-08-29 16:41:37]

lgtm + nits.

https://codereview.chromium.org/2286063003/diff/20001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (left):

https://codereview.chromium.org/2286063003/diff/20001/content/gpu/gpu_main.cc...
content/gpu/gpu_main.cc:356: if (enable_watchdog && delayed_watchdog_enable) {
nit: get rid of braces.

https://codereview.chromium.org/2286063003/diff/20001/gpu/ipc/service/gpu_wat...
File gpu/ipc/service/gpu_watchdog_thread.h (right):

https://codereview.chromium.org/2286063003/diff/20001/gpu/ipc/service/gpu_wat...
gpu/ipc/service/gpu_watchdog_thread.h:36: NON_EXPORTED_BASE(public GpuWatchdog),
On 2016/08/28 13:01:58, sadrul wrote:
> There's no other subclass of GpuWatchdog, and now that both the interface and
> the only implementation is in gpu, and there are no users of GpuWatchdog from
> out of gpu, maybe I should get rid of GpuWatchdog interface?

Since this isn't used in tests, this seems like a reasonable cleanup.

[piman, 2016-08-29 18:43:04]

LGTM. It feels somewhat out of place (it's more "we need a place out of
content/" rather than "it belongs in gpu/ipc"), but I don't feel that strongly,
so let's go with this for now.

One thing, I know GPU hangs are treated specially on the crash server, and I
suspect (but I have no idea) this is done based on the crash location / stack
trace, which now changes. So we may need to do something somewhere (¯\_(ツ)_/¯)
to fix it up.

[sadrul, 2016-08-29 19:22:09]

On 2016/08/29 18:43:04, piman OOO back 2016-08-29 wrote:
> LGTM. It feels somewhat out of place (it's more "we need a place out of
> content/" rather than "it belongs in gpu/ipc"), but I don't feel that
strongly,
> so let's go with this for now.

Indeed. //gpu/ipc feels a bit awkward. I considered //gpu/core/service, but that
doesn't feel much better either, to be honest. I plan on moving more things into
gpu (e.g. https://codereview.chromium.org/2289553002/), so I would be interested
if you have suggestions for a better place.

> 
> One thing, I know GPU hangs are treated specially on the crash server, and I
> suspect (but I have no idea) this is done based on the crash location / stack
> trace, which now changes. So we may need to do something somewhere (¯\_(ツ)_/¯)
> to fix it up.

Interesting. Thanks for the heads up! I will see if someone from
chrome-stability@ can confirm.

[sadrul, 2016-08-29 19:22:36]

Oh, in the meantime, any objections to removing GpuWatchdog interface, and just
using GpuWatchdogThread everywhere?

[piman, 2016-08-29 19:51:44]

On Mon, Aug 29, 2016 at 12:22 PM, <sadrul@chromium.org> wrote:

> Oh, in the meantime, any objections to removing GpuWatchdog interface, and
> just
> using GpuWatchdogThread everywhere?
>

Nope, go ahead.


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

-- 
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-08-29 20:00:32]

https://codereview.chromium.org/2286063003/diff/20001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (left):

https://codereview.chromium.org/2286063003/diff/20001/content/gpu/gpu_main.cc...
content/gpu/gpu_main.cc:356: if (enable_watchdog && delayed_watchdog_enable) {
On 2016/08/29 16:41:37, Fady Samuel wrote:
> nit: get rid of braces.

Done.

https://codereview.chromium.org/2286063003/diff/20001/gpu/ipc/service/gpu_wat...
File gpu/ipc/service/gpu_watchdog_thread.h (right):

https://codereview.chromium.org/2286063003/diff/20001/gpu/ipc/service/gpu_wat...
gpu/ipc/service/gpu_watchdog_thread.h:36: NON_EXPORTED_BASE(public GpuWatchdog),
On 2016/08/29 16:41:37, Fady Samuel wrote:
> On 2016/08/28 13:01:58, sadrul wrote:
> > There's no other subclass of GpuWatchdog, and now that both the interface
and
> > the only implementation is in gpu, and there are no users of GpuWatchdog
from
> > out of gpu, maybe I should get rid of GpuWatchdog interface?
> 
> Since this isn't used in tests, this seems like a reasonable cleanup.

Done.

[sadrul, 2016-08-29 20:00:41]

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

[commit-bot: I haz the power, 2016-08-29 20:21:31]

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

[piman, 2016-08-29 20:59:45]

lgtm

[sadrul, 2016-08-29 22:45:36]

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

[sadrul, 2016-08-29 23:28:51]

The CQ bit was unchecked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-08-29 23:28:59]

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

[sadrul, 2016-08-30 03:51:15]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-08-30 03:51:15]

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

[commit-bot: I haz the power, 2016-08-30 03:54:54]

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-08-30 05:22:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 05:24:31]

Description was changed from

==========
gpu: Move GpuWatchdogThread into //gpu/ipc/service from content.

GpuWatchdogThread doesn't have any dependency on content. So move it into
the gpu service component, so that it can be used by both content-gpu and
mus-gpu.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Move GpuWatchdogThread into //gpu/ipc/service from content.

GpuWatchdogThread doesn't have any dependency on content. So move it into
the gpu service component, so that it can be used by both content-gpu and
mus-gpu.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/2fb7e150d959023d9a793a74e30cea42798c56b4
Cr-Commit-Position: refs/heads/master@{#415149}
==========

[commit-bot: I haz the power, 2016-08-30 05:24:32]

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