TouchView Mojom

A new mojom so that chrome/browser can learn of touchview state changes which occur on ash.

TEST=manually tested login screen in normal CrOS builds as well as with the mash flag
BUG=648753
Committed: https://crrev.com/c85c81962fc4e37368c1562978fce94bfba6cd92
Cr-Commit-Position: refs/heads/master@{#435961}

[jonross, 2016-11-18 00:17:13]

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

[commit-bot: I haz the power, 2016-11-18 00:19:08]

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-11-18 01:37:41]

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

[commit-bot: I haz the power, 2016-11-18 01:37:42]

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

[jonross, 2016-11-30 16:58:34]

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

[commit-bot: I haz the power, 2016-11-30 16:59:12]

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-11-30 17:02:49]

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

[commit-bot: I haz the power, 2016-11-30 17:02:49]

Dry run: 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...)

[jonross, 2016-11-30 18:32:23]

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

[commit-bot: I haz the power, 2016-11-30 18:32:51]

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-11-30 19:53:55]

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

[commit-bot: I haz the power, 2016-11-30 19:53:56]

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

[jonross, 2016-12-01 14:38:39]

jonross@chromium.org changed reviewers:
+ jamescook@chromium.org

[jonross, 2016-12-01 14:38:40]

Hey James,

Could you take a look at this change? I'm adding a new mojom to handle
chrome-ash communication of the touch view state.

Thanks,
Jon

[jonross, 2016-12-01 15:02:56]

Description was changed from

==========
WIP TouchView Mojom
WIP TouchView mojom so that chrome/browser can learn of touchview state changes.
==========

to

==========
A new mojom so that chrome/browser can learn of touchview state changes which
occur on ash.


TEST=manually tested login screen in normal CrOS builds as well as with the mash
flag
BUG=648753
==========

[James Cook, 2016-12-01 16:55:10]

Little things and questions

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
File ash/public/interfaces/touch_view.mojom (right):

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
ash/public/interfaces/touch_view.mojom:9: OnTouchViewChanged(bool enabled);
nit: document when this is fired (e.g. before or after). I presume it's after,
but since mojom is our permanent interface between ash and chrome I prefer
everything to be obsessively documented.

optional: This also could be called OnTouchViewEnabled(bool enabled) or
OnTouchViewToggled(bool enabled) which would make calls like
OnTouchViewChanged(false) self-documenting. Or you could keep separate
Started/Ended methods.

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
ash/public/interfaces/touch_view.mojom:14: interface TouchViewManager {
Just curious, any particular reason for Manager vs. Controller vs. just
TouchView? I'm OK with any of them, and I like that you're calling everything
TouchView, it just seemed a little weird that mojom::TouchViewManager is
implemented by MaximizeModeController.

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
ash/public/interfaces/touch_view.mojom:15: AddObserver(TouchViewObserver
client);
Just curious, there's no need to remove?

Also, you might move (some of or all of) the interface comment to this method,
just to emphasize that calling the method triggers an initial notification.

https://codereview.chromium.org/2505403003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (left):

https://codereview.chromium.org/2505403003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:298:
NOTIMPLEMENTED();
Hooray!

https://codereview.chromium.org/2505403003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://codereview.chromium.org/2505403003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:495:
mojo::Binding<ash::mojom::TouchViewObserver> touch_view_binding_;
I like that you called this touch_view_binding_ and not just binding_. :-)

[jonross, 2016-12-01 19:20:57]

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
File ash/public/interfaces/touch_view.mojom (right):

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
ash/public/interfaces/touch_view.mojom:9: OnTouchViewChanged(bool enabled);
On 2016/12/01 16:55:09, James Cook wrote:
> nit: document when this is fired (e.g. before or after). I presume it's after,
> but since mojom is our permanent interface between ash and chrome I prefer
> everything to be obsessively documented.
> 
> optional: This also could be called OnTouchViewEnabled(bool enabled) or
> OnTouchViewToggled(bool enabled) which would make calls like
> OnTouchViewChanged(false) self-documenting. Or you could keep separate
> Started/Ended methods.

Added docs on timing. Going with the suggested OnTouchViewToggled(bool enabled)
I like methods to be as self-documenting as possible.

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
ash/public/interfaces/touch_view.mojom:14: interface TouchViewManager {
On 2016/12/01 16:55:09, James Cook wrote:
> Just curious, any particular reason for Manager vs. Controller vs. just
> TouchView? I'm OK with any of them, and I like that you're calling everything
> TouchView, it just seemed a little weird that mojom::TouchViewManager is
> implemented by MaximizeModeController.

I went with Manager as this interface provides no way to make changes. To me
there was nothing to request from a "Controller". I'm open to suggestions on
this name though, so I could switch it to just TouchView if desired.

Yeah I realized it's weird with MaximizeModeController, but really that should
be renamed at some point to TouchView* as its now does more than simply maximize
windows.

https://codereview.chromium.org/2505403003/diff/60001/ash/public/interfaces/t...
ash/public/interfaces/touch_view.mojom:15: AddObserver(TouchViewObserver
client);
On 2016/12/01 16:55:09, James Cook wrote:
> Just curious, there's no need to remove?
> 
> Also, you might move (some of or all of) the interface comment to this method,
> just to emphasize that calling the method triggers an initial notification.

No need to remove, the mojo::InterfacePtrSet clears our ptrs as connections
close. So when the signin screen is destructed the ash side clears out the
observer.

[James Cook, 2016-12-01 20:34:29]

LGTM

[jonross, 2016-12-01 20:46:31]

jonross@chromium.org changed reviewers:
+ xiyuan@chromium.org

[jonross, 2016-12-01 20:46:31]

xiyuan@chromium.org: Please review changes in chrome/browser/ui/webui/chromeos/*

I've added a new mojom to handle notifying chrome of touch view changes
occurring in ash.

Thanks,
Jon

[xiyuan, 2016-12-01 21:28:39]

lgtm

https://codereview.chromium.org/2505403003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/2505403003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:271:
touch_view_enabled_(false),
nit: move initializer to header ?

https://codereview.chromium.org/2505403003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:301:
connector->ConnectToInterface("content_browser", &touch_view_manager_ptr_);
nit: "content_browser" -> content::mojom::kBrowserServiceName

[jonross, 2016-12-01 22:01:17]

https://codereview.chromium.org/2505403003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/2505403003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:271:
touch_view_enabled_(false),
On 2016/12/01 21:28:39, xiyuan wrote:
> nit: move initializer to header ?

Done.

https://codereview.chromium.org/2505403003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:301:
connector->ConnectToInterface("content_browser", &touch_view_manager_ptr_);
On 2016/12/01 21:28:39, xiyuan wrote:
> nit: "content_browser" -> content::mojom::kBrowserServiceName

Done.

[jonross, 2016-12-01 22:03:44]

jonross@chromium.org changed reviewers:
+ dcheng@chromium.org

[jonross, 2016-12-01 22:03:45]

Hey dcheng@,

Could you provide the security owners review to this change?

I'm adding a new interface in ash/public/interfaces/touch_view.mojom to replace
the current observer pattern used by chrome to listen to ash.

I've also update chrome/browser/chrome_content_browser_manifest_overlay.json to
define the new service being provided.

Thanks,
Jon

[dcheng, 2016-12-02 02:59:50]

mojo lgtm

https://codereview.chromium.org/2505403003/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/touch_view.mojom (right):

https://codereview.chromium.org/2505403003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/touch_view.mojom:18: AddObserver(TouchViewObserver
client);
Nit: shouldn't the parameter be called observer =)

[jonross, 2016-12-02 14:45:59]

https://codereview.chromium.org/2505403003/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/touch_view.mojom (right):

https://codereview.chromium.org/2505403003/diff/100001/ash/public/interfaces/...
ash/public/interfaces/touch_view.mojom:18: AddObserver(TouchViewObserver
client);
On 2016/12/02 02:59:50, dcheng wrote:
> Nit: shouldn't the parameter be called observer =)

Done.

[jonross, 2016-12-02 14:46:04]

The CQ bit was checked by jonross@chromium.org

[jonross, 2016-12-02 14:46:05]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
jamescook@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2505403003/#ps120001
(title: "Minor param rename")

[commit-bot: I haz the power, 2016-12-02 14:46: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-12-02 17:18:50]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480689964646320,
"parent_rev": "9d2b22f16e75fd209d909d98143e41f22eebe552", "commit_rev":
"28bd102185f2a06f777db63ec4143d7bf1ea7504"}

[commit-bot: I haz the power, 2016-12-02 17:19:26]

Description was changed from

==========
A new mojom so that chrome/browser can learn of touchview state changes which
occur on ash.


TEST=manually tested login screen in normal CrOS builds as well as with the mash
flag
BUG=648753
==========

to

==========
A new mojom so that chrome/browser can learn of touchview state changes which
occur on ash.


TEST=manually tested login screen in normal CrOS builds as well as with the mash
flag
BUG=648753
==========

[commit-bot: I haz the power, 2016-12-02 17:19:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-02 17:22:06]

Description was changed from

==========
A new mojom so that chrome/browser can learn of touchview state changes which
occur on ash.


TEST=manually tested login screen in normal CrOS builds as well as with the mash
flag
BUG=648753
==========

to

==========
A new mojom so that chrome/browser can learn of touchview state changes which
occur on ash.

TEST=manually tested login screen in normal CrOS builds as well as with the mash
flag
BUG=648753

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

[commit-bot: I haz the power, 2016-12-02 17:22:07]

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