Add mash shelf application id support.

Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER; a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org,jam@chromium.org

Committed: https://crrev.com/5dce124e0dcc9e781378f48a3246ca5ede44dafb
Cr-Commit-Position: refs/heads/master@{#389508}

[msw, 2016-04-22 01:25:07]

Description was changed from

==========
Add support for mash window app ids.

BUG=557406
==========

to

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org
==========

[msw, 2016-04-22 01:25:07]

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

[msw, 2016-04-22 01:26:31]

Patchset #5 (id:80001) has been deleted

[msw, 2016-04-22 01:27:01]

Hey Scott, please take a look; thanks!

[msw, 2016-04-22 01:27:10]

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

[commit-bot: I haz the power, 2016-04-22 01:27:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899323002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899323002/100001

[commit-bot: I haz the power, 2016-04-22 01:29:56]

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

[commit-bot: I haz the power, 2016-04-22 01:29:57]

Dry run: Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[msw, 2016-04-22 01:37:09]

The CQ bit was checked by msw@chromium.org

[msw, 2016-04-22 01:37:10]

The CQ bit was unchecked by msw@chromium.org

[msw, 2016-04-22 01:37:15]

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

[commit-bot: I haz the power, 2016-04-22 01:37:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899323002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899323002/120001

[commit-bot: I haz the power, 2016-04-22 01:42:41]

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

[commit-bot: I haz the power, 2016-04-22 01:42:42]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-04-22 15:51:19]

It seems like a security issue to allow clients to supply their own app_id.
Could the WS supply this information to the WM when top level windows are
created?

I'm suggesting that the WS remembers the remote identity of connections made to
it, and forwards the app id through to WmCreateTopLevelWindow. This only works
if apps are connecting to mus directly via WindowTreeFactory and not being
embedded, which I believe is the case. Would this work?

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
File ash/mus/shelf_delegate_mus.cc (right):

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
ash/mus/shelf_delegate_mus.cc:72: for (auto window : item_delegate_->windows())
const auto&

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
ash/mus/shelf_delegate_mus.cc:133: std::map<uint32_t, base::string16> windows_;
This name is mildly confusing as as it's really window id to title. I'm ok with
windows, but at least document what the mapping is.

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
ash/mus/shelf_delegate_mus.cc:297:
DCHECK(app_id_to_shelf_id_.count(app_id.To<std::string>()));
Don't trust a client to do the right thing with mojoms. In other words ignore
the client if the app_id is bogus (and possibly log).

Also, do we want to allow one client to pin/unpin another clients items?

https://codereview.chromium.org/1899323002/diff/120001/mash/wm/public/interfa...
File mash/wm/public/interfaces/user_window_controller.mojom (right):

https://codereview.chromium.org/1899323002/diff/120001/mash/wm/public/interfa...
mash/wm/public/interfaces/user_window_controller.mojom:12: string window_app_id;
Document this. And while you're at it, document window_id (it's not the same as
the internal id used the ws).

https://codereview.chromium.org/1899323002/diff/120001/services/shell/public/...
File services/shell/public/cpp/shell_connection.h (right):

https://codereview.chromium.org/1899323002/diff/120001/services/shell/public/...
services/shell/public/cpp/shell_connection.h:70: private:
nit: fix this line too.

[msw, 2016-04-22 19:02:18]

On 2016/04/22 15:51:19, sky wrote:
> It seems like a security issue to allow clients to supply their own app_id.
> Could the WS supply this information to the WM when top level windows are
> created?
> 
> I'm suggesting that the WS remembers the remote identity of connections made
to
> it, and forwards the app id through to WmCreateTopLevelWindow. This only works
> if apps are connecting to mus directly via WindowTreeFactory and not being
> embedded, which I believe is the case. Would this work?

How would extension app windows work in that case? I figure that some cases will
need to override the window property for legitimate reasons. Also, I'm not sure
if this is really a security issue. I could rename the property "window group
id" or similar so that it doesn't convey the authority of an app id, since it's
just used for grouping/associating windows (at least for now). Happy to discuss
more.

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
File ash/mus/shelf_delegate_mus.cc (right):

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
ash/mus/shelf_delegate_mus.cc:72: for (auto window : item_delegate_->windows())
On 2016/04/22 15:51:19, sky wrote:
> const auto&

Done.

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
ash/mus/shelf_delegate_mus.cc:133: std::map<uint32_t, base::string16> windows_;
On 2016/04/22 15:51:19, sky wrote:
> This name is mildly confusing as as it's really window id to title. I'm ok
with
> windows, but at least document what the mapping is.

Renamed, clarity in identifier names is better than explanatory comments.

https://codereview.chromium.org/1899323002/diff/120001/ash/mus/shelf_delegate...
ash/mus/shelf_delegate_mus.cc:297:
DCHECK(app_id_to_shelf_id_.count(app_id.To<std::string>()));
On 2016/04/22 15:51:19, sky wrote:
> Don't trust a client to do the right thing with mojoms. In other words ignore
> the client if the app_id is bogus (and possibly log).
> 
> Also, do we want to allow one client to pin/unpin another clients items?

Added early return. I think this is probably okay for now; I don't anticipate
bad mojo actors in the chromium codebase making their way into the shelf
controller interface (ie. we don't protect against
Shell::GetInstance()->shelf_model()->RemoveItemAt(index)). If we do want some
added security, maybe we could require a ShelfItemDelegateAssociatedPtrInfo
(like PinItem) and require that the it be the same object? (not sure if that's
actually possible)

https://codereview.chromium.org/1899323002/diff/120001/mash/wm/public/interfa...
File mash/wm/public/interfaces/user_window_controller.mojom (right):

https://codereview.chromium.org/1899323002/diff/120001/mash/wm/public/interfa...
mash/wm/public/interfaces/user_window_controller.mojom:12: string window_app_id;
On 2016/04/22 15:51:19, sky wrote:
> Document this. And while you're at it, document window_id (it's not the same
as
> the internal id used the ws).

Done.

https://codereview.chromium.org/1899323002/diff/120001/services/shell/public/...
File services/shell/public/cpp/shell_connection.h (right):

https://codereview.chromium.org/1899323002/diff/120001/services/shell/public/...
services/shell/public/cpp/shell_connection.h:70: private:
On 2016/04/22 15:51:19, sky wrote:
> nit: fix this line too.

Done.

[msw, 2016-04-22 19:02:28]

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

[commit-bot: I haz the power, 2016-04-22 19:03:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899323002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899323002/140001

[sky, 2016-04-22 19:15:53]

LGTM

[commit-bot: I haz the power, 2016-04-22 21:55:29]

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

[commit-bot: I haz the power, 2016-04-22 21:55:30]

Dry run: This issue passed the CQ dry run.

[msw, 2016-04-22 22:03:48]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-04-22 22:04:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899323002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899323002/140001

[commit-bot: I haz the power, 2016-04-22 22:15:08]

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

[commit-bot: I haz the power, 2016-04-22 22:15:09]

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

[msw, 2016-04-22 22:21:18]

Description was changed from

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org
==========

to

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org,jam@chromium.org
==========

[msw, 2016-04-22 22:21:19]

msw@chromium.org changed reviewers:
+ jam@chromium.org

[msw, 2016-04-22 22:21:43]

Hey John, please take a look at content/*; thanks!

[jam, 2016-04-25 15:38:02]

lgtm

[msw, 2016-04-25 16:43:30]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-04-25 16:43:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899323002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899323002/140001

[commit-bot: I haz the power, 2016-04-25 17:44:14]

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

[commit-bot: I haz the power, 2016-04-25 17:44:16]

Failed to commit the patch.

[commit-bot: I haz the power, 2016-04-25 17:44:27]

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

[msw, 2016-04-25 18:07:52]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-04-25 18:08:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899323002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899323002/140001

[commit-bot: I haz the power, 2016-04-25 18:16:56]

Description was changed from

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org,jam@chromium.org
==========

to

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org,jam@chromium.org
==========

[commit-bot: I haz the power, 2016-04-25 18:16:57]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-04-25 18:18:01]

Description was changed from

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org,jam@chromium.org
==========

to

==========
Add mash shelf application id support.

Add an app_id mus window property; used to group shelf items, etc.
Populate app_id from the identity name via WindowManagerConnection.

Track multiple windows in ShelfDelegateMus & ShelfItemDelegateMus.
Show new ShelfMenuModelMus with titles on multi-window item click.

Rename ShelfController::[Pin|Unpin]Item (was [Add|Remove]Item).
Plumb identity; remove incorrect window_property.h comment.

TODO: Support dynamic app_id or custom init, for extensions, etc.?
TODO: Support window application titles (eg. 'Chrome' vs. 'New Tab')

BUG=557406
TEST=In "mojo_runner mojo:mash_session", type 'quick_launch' and hit CTRL+ENTER;
a second quick launch window should open and share the existing shelf item.
R=sky@chromium.org,jam@chromium.org

Committed: https://crrev.com/5dce124e0dcc9e781378f48a3246ca5ede44dafb
Cr-Commit-Position: refs/heads/master@{#389508}
==========

[commit-bot: I haz the power, 2016-04-25 18:18:03]

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