Splitting the Mus demo code out of a service.

Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
Also making the BitmapUploader use the async version of the GPU
service EstablishGpuChannel API to prevent a deadlock when embedded.

This is in preparation of running the Mus demo from Android content shell.

BUG=657499
Committed: https://crrev.com/87303d73f481bfa8acb3508180070ca70cecd4ca
Cr-Commit-Position: refs/heads/master@{#426959}

[Jay Civelli, 2016-10-19 16:45:29]

Description was changed from

==========
Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
This is so it can be used from the Android content shell.

BUG=657499
==========

to

==========
Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
Also making the BitmapUploader use the async version of the GPU
service EstablishGpuChannel API to prevent a deadlock when embedded.

This is in preparation of running the Mus demo from Android content shell.

BUG=657499
==========

[Jay Civelli, 2016-10-19 16:46:42]

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

[commit-bot: I haz the power, 2016-10-19 16:47:11]

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-10-19 17:55:23]

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

[commit-bot: I haz the power, 2016-10-19 17:55:24]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2016-10-19 17:58:43]

jcivelli@chromium.org changed reviewers:
+ rjkroege@chromium.org

[tonikitoo, 2016-10-19 19:34:06]

tonikitoo@igalia.com changed reviewers:
+ tonikitoo@igalia.com

[tonikitoo, 2016-10-19 19:34:07]

driven-by review.

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/BUILD.gn
File services/ui/demo/BUILD.gn (right):

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/BUILD.gn#n...
services/ui/demo/BUILD.gn:23: "//services/ui/public/interfaces",
do we need to duplicate most of the dependencies in mus_demo and mus_demo_lib
targets?

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/DEPS
File services/ui/demo/DEPS (right):

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/DEPS#newcode2
services/ui/demo/DEPS:2: "+content/shell/browser",
unrelated to this change?

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/mus_demo.h
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/mus_demo.h...
services/ui/demo/mus_demo.h:28: // A simple MUS Demo. It connects to the
mojo:ui, creates a new window and
since you are touch this comment, maybe replace "mojo:ui" by "service:ui"?

https://codereview.chromium.org/2390013002

[Jay Civelli, 2016-10-19 20:00:13]

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/BUILD.gn
File services/ui/demo/BUILD.gn (right):

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/BUILD.gn#n...
services/ui/demo/BUILD.gn:23: "//services/ui/public/interfaces",
On 2016/10/19 19:34:07, tonikitoo wrote:
> do we need to duplicate most of the dependencies in mus_demo and mus_demo_lib
> targets?

Good point, simplified.

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/DEPS
File services/ui/demo/DEPS (right):

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/DEPS#newcode2
services/ui/demo/DEPS:2: "+content/shell/browser",
On 2016/10/19 19:34:07, tonikitoo wrote:
> unrelated to this change?

Good catch, removed.

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/mus_demo.h
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2440453002/diff/1/services/ui/demo/mus_demo.h...
services/ui/demo/mus_demo.h:28: // A simple MUS Demo. It connects to the
mojo:ui, creates a new window and
On 2016/10/19 19:34:07, tonikitoo wrote:
> since you are touch this comment, maybe replace "mojo:ui" by "service:ui"?
> 
> https://codereview.chromium.org/2390013002

Done.

[Jay Civelli, 2016-10-19 20:02:48]

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

[commit-bot: I haz the power, 2016-10-19 20:03:41]

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-10-19 21:36:59]

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

[commit-bot: I haz the power, 2016-10-19 21:36:59]

Dry run: This issue passed the CQ dry run.

[fwang, 2016-10-20 13:05:03]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com

[rjkroege, 2016-10-20 21:44:33]

lgtm with micro-nits

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
services/ui/demo/mus_demo.cc:188: #if defined(OS_ANDROID)
feel free to add a todo here to make this not require an #ifdef

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo_service.h (right):

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
services/ui/demo/mus_demo_service.h:24: // shell::Service:
tiny nit: Overrides shell::Service?

[Jay Civelli, 2016-10-21 05:32:42]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2016-10-21 05:32:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2440453002/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-10-21 05:32:59]

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-10-21 06:03:53]

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

[commit-bot: I haz the power, 2016-10-21 06:03:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jay Civelli, 2016-10-21 17:16:11]

Adding @sky for owner approval for services/ui/public/cpp/BUILD.gn

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
services/ui/demo/mus_demo.cc:188: #if defined(OS_ANDROID)
On 2016/10/20 21:44:32, rjkroege wrote:
> feel free to add a todo here to make this not require an #ifdef

Done.

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo_service.h (right):

https://codereview.chromium.org/2440453002/diff/20001/services/ui/demo/mus_de...
services/ui/demo/mus_demo_service.h:24: // shell::Service:
On 2016/10/20 21:44:33, rjkroege wrote:
> tiny nit: Overrides shell::Service?
> 

Done.

[Jay Civelli, 2016-10-21 19:16:22]

jcivelli@chromium.org changed reviewers:
+ sky@chromium.org

[tonikitoo, 2016-10-21 22:13:05]

informal l g t m

[sky, 2016-10-21 23:18:23]

LGTM

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
services/ui/demo/mus_demo.h:37: void Start(service_manager::Connector*
connector);
Start->Init

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo_service.cc (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
services/ui/demo/mus_demo_service.cc:12: MusDemoService::MusDemoService() :
mus_demo_(new MusDemo()) {}
We like changing stuff all the time, new way for this is MakeUnique, in
base/memory/ptr_util.h. See threads on chromium-dev for gory details if you
care.

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo_service.h (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
services/ui/demo/mus_demo_service.h:12: 
remove newline (just like you have no newline between 34/35).

https://codereview.chromium.org/2440453002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/BUILD.gn:96: "//services/ui/demo:mus_demo_library",
Now that you moved the files to demo_lib, is the mus_demo_library one still
needed? Nuke if you can.

[Jay Civelli, 2016-10-22 00:00:16]

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
services/ui/demo/mus_demo.h:37: void Start(service_manager::Connector*
connector);
On 2016/10/21 23:18:23, sky wrote:
> Start->Init

Start seems more appropriate since it is really starting the demo.
Let me know if you disagree.

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo_service.cc (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
services/ui/demo/mus_demo_service.cc:12: MusDemoService::MusDemoService() :
mus_demo_(new MusDemo()) {}
On 2016/10/21 23:18:23, sky wrote:
> We like changing stuff all the time, new way for this is MakeUnique, in
> base/memory/ptr_util.h. See threads on chromium-dev for gory details if you
> care.

Done.

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
File services/ui/demo/mus_demo_service.h (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/demo/mus_de...
services/ui/demo/mus_demo_service.h:12: 
On 2016/10/21 23:18:23, sky wrote:
> remove newline (just like you have no newline between 34/35).

Done.

https://codereview.chromium.org/2440453002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2440453002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/BUILD.gn:96: "//services/ui/demo:mus_demo_library",
On 2016/10/21 23:18:23, sky wrote:
> Now that you moved the files to demo_lib, is the mus_demo_library one still
> needed? Nuke if you can.

Good point, removed.

[Jay Civelli, 2016-10-22 00:00:21]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2016-10-22 00:00:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2440453002/#ps80001
(title: "Addressed @sky comments and synced")

[commit-bot: I haz the power, 2016-10-22 00:01:29]

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-10-22 02:51:38]

Description was changed from

==========
Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
Also making the BitmapUploader use the async version of the GPU
service EstablishGpuChannel API to prevent a deadlock when embedded.

This is in preparation of running the Mus demo from Android content shell.

BUG=657499
==========

to

==========
Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
Also making the BitmapUploader use the async version of the GPU
service EstablishGpuChannel API to prevent a deadlock when embedded.

This is in preparation of running the Mus demo from Android content shell.

BUG=657499
==========

[commit-bot: I haz the power, 2016-10-22 02:51:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-22 02:54:51]

Description was changed from

==========
Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
Also making the BitmapUploader use the async version of the GPU
service EstablishGpuChannel API to prevent a deadlock when embedded.

This is in preparation of running the Mus demo from Android content shell.

BUG=657499
==========

to

==========
Splitting the Mus demo code out of a service.

Splitting the Mus demo code so it can be executed from outside of a
service.
Also making the BitmapUploader use the async version of the GPU
service EstablishGpuChannel API to prevent a deadlock when embedded.

This is in preparation of running the Mus demo from Android content shell.

BUG=657499

Committed: https://crrev.com/87303d73f481bfa8acb3508180070ca70cecd4ca
Cr-Commit-Position: refs/heads/master@{#426959}
==========

[commit-bot: I haz the power, 2016-10-22 02:54:52]

Patchset 5 (id:??) landed as
https://crrev.com/87303d73f481bfa8acb3508180070ca70cecd4ca
Cr-Commit-Position: refs/heads/master@{#426959}