mash: Add touch-hud app.

mash: Add touch-hud app.

BUG=588311
TEST=mash_unittests + manual

Committed: https://crrev.com/ff996c78860cc2a45211b2f4938061b7c8d299cb
Cr-Commit-Position: refs/heads/master@{#406986}

[riajiang, 2016-07-14 22:36:27]

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

[commit-bot: I haz the power, 2016-07-14 22:37:18]

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

[riajiang, 2016-07-14 23:30:21]

riajiang@chromium.org changed reviewers:
+ jamescook@chromium.org, msw@chromium.org, sadrul@chromium.org,
sky@chromium.org

[sadrul, 2016-07-15 15:01:11]

https://codereview.chromium.org/2150933003/diff/20001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:188: return gfx::Rect(0, 0,
root_->bounds().width(), root_->bounds().height());
Can you add a test for this code here?

Add a TODO to resize these windows if/when |root_| is resized.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/DEPS
File ash/touch_hud/mus/DEPS (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/DEPS#...
ash/touch_hud/mus/DEPS:6: "+services/ui/public",
Add "-ash" and then "+ash/touch_hud", "+ash/public", since this should not use
any other parts of ash.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
File ash/touch_hud/mus/touch_hud_application.cc (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:85: // If the keyboard shortcut for
enabling touch-hud app is used.
Remove the comment.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:93: new
TouchHudUI(views::WindowManagerConnection::Get(), widget_);
window_manager_connection_.get()

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:111: widget_ = nullptr;
Does this app terminate when the widget is closed? Because we should do that.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
File ash/touch_hud/mus/touch_hud_application.h (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.h:48:
mojo::BindingSet<mash::mojom::Launchable> bindings_;
I don't think we need a BindingSet. Use a Binding instead.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.h:51: mojo::TracingImpl tracing_;
Let's start without the tracing for now.

https://codereview.chromium.org/2150933003/diff/20001/mash/BUILD.gn
File mash/BUILD.gn (right):

https://codereview.chromium.org/2150933003/diff/20001/mash/BUILD.gn#newcode14
mash/BUILD.gn:14: "//ash/touch_hud",
ash/touch_hud/mus:touch_hud

[riajiang, 2016-07-15 17:01:28]

https://codereview.chromium.org/2150933003/diff/20001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:188: return gfx::Rect(0, 0,
root_->bounds().width(), root_->bounds().height());
On 2016/07/15 15:01:10, sadrul wrote:
> Can you add a test for this code here?
> 
> Add a TODO to resize these windows if/when |root_| is resized.

Added a test in window_mananger_unittest for creating fullscreen window.

I think this function is only called in RootWindowController::NewTopLevelWindow
when creating new windows so resizing would be elsewhere?

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/DEPS
File ash/touch_hud/mus/DEPS (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/DEPS#...
ash/touch_hud/mus/DEPS:6: "+services/ui/public",
On 2016/07/15 15:01:10, sadrul wrote:
> Add "-ash" and then "+ash/touch_hud", "+ash/public", since this should not use
> any other parts of ash.

Done.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
File ash/touch_hud/mus/touch_hud_application.cc (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:85: // If the keyboard shortcut for
enabling touch-hud app is used.
On 2016/07/15 15:01:10, sadrul wrote:
> Remove the comment.

Done.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:93: new
TouchHudUI(views::WindowManagerConnection::Get(), widget_);
On 2016/07/15 15:01:11, sadrul wrote:
> window_manager_connection_.get()

Done.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:111: widget_ = nullptr;
On 2016/07/15 15:01:10, sadrul wrote:
> Does this app terminate when the widget is closed? Because we should do that.

Added QuitWhenIdle() after closing the widget.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
File ash/touch_hud/mus/touch_hud_application.h (right):

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.h:48:
mojo::BindingSet<mash::mojom::Launchable> bindings_;
On 2016/07/15 15:01:11, sadrul wrote:
> I don't think we need a BindingSet. Use a Binding instead.

Done.

https://codereview.chromium.org/2150933003/diff/20001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.h:51: mojo::TracingImpl tracing_;
On 2016/07/15 15:01:11, sadrul wrote:
> Let's start without the tracing for now.

Done.

https://codereview.chromium.org/2150933003/diff/20001/mash/BUILD.gn
File mash/BUILD.gn (right):

https://codereview.chromium.org/2150933003/diff/20001/mash/BUILD.gn#newcode14
mash/BUILD.gn:14: "//ash/touch_hud",
On 2016/07/15 15:01:11, sadrul wrote:
> ash/touch_hud/mus:touch_hud

Done.

[sadrul, 2016-07-15 18:15:18]

LGTM!

https://codereview.chromium.org/2150933003/diff/40001/ash/touch_hud/mus/touch...
File ash/touch_hud/mus/touch_hud_application.cc (right):

https://codereview.chromium.org/2150933003/diff/40001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:56: views::Widget* widget_;
|widget_| isn't used. You can remove.

[riajiang, 2016-07-15 18:21:40]

Description was changed from

==========
mash: Add touch-hud app.

BUG=588311
==========

to

==========
mash: Add touch-hud app.

BUG=588311
TEST=ash_unittests + manual
==========

[James Cook, 2016-07-15 18:35:23]

LGTM. Nice patch.

In the future, please let me know what you would like me to look at in the
initial review request message (especially if you just need me for certain
directories).

https://codereview.chromium.org/2150933003/diff/40001/ash/mus/window_manager_...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2150933003/diff/40001/ash/mus/window_manager_...
ash/mus/window_manager_unittest.cc:161: TEST_F(WmTestBase, Fullscreen) {
nit: WmTestBase -> WindowManagerTest

[riajiang, 2016-07-18 15:25:01]

riajiang@chromium.org changed required reviewers:
+ sky@chromium.org

[riajiang, 2016-07-20 14:08:53]

riajiang@chromium.org changed required reviewers:
- sky@chromium.org

[riajiang, 2016-07-20 14:16:17]

sky, could you please take a look at chrome/ and mash/? Thanks!

On 2016/07/15 18:35:23, James Cook wrote:
> LGTM. Nice patch.
> 
> In the future, please let me know what you would like me to look at in the
> initial review request message (especially if you just need me for certain
> directories).

Sorry, will do next time!

https://codereview.chromium.org/2150933003/diff/40001/ash/mus/window_manager_...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2150933003/diff/40001/ash/mus/window_manager_...
ash/mus/window_manager_unittest.cc:161: TEST_F(WmTestBase, Fullscreen) {
On 2016/07/15 18:35:23, James Cook wrote:
> nit: WmTestBase -> WindowManagerTest

Done.

https://codereview.chromium.org/2150933003/diff/40001/ash/touch_hud/mus/touch...
File ash/touch_hud/mus/touch_hud_application.cc (right):

https://codereview.chromium.org/2150933003/diff/40001/ash/touch_hud/mus/touch...
ash/touch_hud/mus/touch_hud_application.cc:56: views::Widget* widget_;
On 2016/07/15 18:15:18, sadrul wrote:
> |widget_| isn't used. You can remove.

Done.

[sky, 2016-07-20 15:58:27]

LGTM

[riajiang, 2016-07-21 19:33:11]

Description was changed from

==========
mash: Add touch-hud app.

BUG=588311
TEST=ash_unittests + manual
==========

to

==========
mash: Add touch-hud app.

BUG=588311
TEST=mash_unittests + manual
==========

[riajiang, 2016-07-21 19:34:11]

Made some changes to test. PTAL. Thanks!

[sky, 2016-07-21 20:00:02]

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:188:
static_cast<::ui::mojom::ShowState>(window->GetSharedProperty<int32_t>(
Use GetWindowShowState in property_util

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
File ash/mus/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
ash/mus/root_window_controller_unittest.cc:30: EXPECT_EQ(window->GetBounds(),
root_window->GetBounds());
Format of assertions in expected, actual. I believe you want to reverse these
two.

[riajiang, 2016-07-21 20:13:51]

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:188:
static_cast<::ui::mojom::ShowState>(window->GetSharedProperty<int32_t>(
On 2016/07/21 20:00:02, sky wrote:
> Use GetWindowShowState in property_util

Done.

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
File ash/mus/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2150933003/diff/80001/ash/mus/root_window_con...
ash/mus/root_window_controller_unittest.cc:30: EXPECT_EQ(window->GetBounds(),
root_window->GetBounds());
On 2016/07/21 20:00:02, sky wrote:
> Format of assertions in expected, actual. I believe you want to reverse these
> two.

Done.

[sky, 2016-07-21 21:26:39]

LGTM

[riajiang, 2016-07-21 21:30:47]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-07-21 21:30:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2150933003/#ps100001
(title: "test")

[commit-bot: I haz the power, 2016-07-21 21:31:18]

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-07-21 21:35:31]

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

[commit-bot: I haz the power, 2016-07-21 21:35:35]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[riajiang, 2016-07-21 22:02:42]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-07-21 22:02:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sadrul@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2150933003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-21 22:03:42]

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-07-21 23:22:26]

Description was changed from

==========
mash: Add touch-hud app.

BUG=588311
TEST=mash_unittests + manual
==========

to

==========
mash: Add touch-hud app.

BUG=588311
TEST=mash_unittests + manual
==========

[commit-bot: I haz the power, 2016-07-21 23:22:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-21 23:24:17]

Description was changed from

==========
mash: Add touch-hud app.

BUG=588311
TEST=mash_unittests + manual
==========

to

==========
mash: Add touch-hud app.

BUG=588311
TEST=mash_unittests + manual

Committed: https://crrev.com/ff996c78860cc2a45211b2f4938061b7c8d299cb
Cr-Commit-Position: refs/heads/master@{#406986}
==========

[commit-bot: I haz the power, 2016-07-21 23:24:18]

Patchset 7 (id:??) landed as
https://crrev.com/ff996c78860cc2a45211b2f4938061b7c8d299cb
Cr-Commit-Position: refs/heads/master@{#406986}