mash: Replace int ShelfIDs with AppLaunchID strings.

mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsNull() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,reveman@chromium.org

Review-Url: https://codereview.chromium.org/2860503002
Cr-Commit-Position: refs/heads/master@{#469537}
Committed: https://chromium.googlesource.com/chromium/src/+/84b8a5f1c2f1c62aadd4c943f3f093860cdc470d

[msw, 2017-05-03 23:41:03]

Patchset #3 (id:40001) has been deleted

[msw, 2017-05-04 02:56:05]

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

[commit-bot: I haz the power, 2017-05-04 02:56:29]

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, 2017-05-04 03:00:15]

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

[commit-bot: I haz the power, 2017-05-04 03:00:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[msw, 2017-05-04 03:40:42]

Description was changed from

==========
mash: Replace ShelfIDs with AppLaunchID strings.

BUG=
==========

to

==========
mash: Replace ShelfIDs with AppLaunchID strings.

Rename AppLaunchId to ShelfID and use that instead of uint32_t.
(ShelfModel no longer assigns incrementing number ids to items)
Add ShelfID comparison operators and IsEmpty() helper function.
Allow empty app_id strings (iff the launch_id string is also empty).

Remove ShelfItem::app_launch_id, use ShelfItem::id instead.

Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
Register kShelfIdKey for property conversion and mirroring.

TODO: Worth enforcing unique ShelfIDs added to ShelfModel?
(should hold in production, requires lots of test changes)

TODO: Remove ShelfModel id conversion functions; audit users.
(some users call these to check if an item exists in the model)

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

[msw, 2017-05-04 03:40:43]

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

[msw, 2017-05-04 04:17:28]

Description was changed from

==========
mash: Replace ShelfIDs with AppLaunchID strings.

Rename AppLaunchId to ShelfID and use that instead of uint32_t.
(ShelfModel no longer assigns incrementing number ids to items)
Add ShelfID comparison operators and IsEmpty() helper function.
Allow empty app_id strings (iff the launch_id string is also empty).

Remove ShelfItem::app_launch_id, use ShelfItem::id instead.

Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
Register kShelfIdKey for property conversion and mirroring.

TODO: Worth enforcing unique ShelfIDs added to ShelfModel?
(should hold in production, requires lots of test changes)

TODO: Remove ShelfModel id conversion functions; audit users.
(some users call these to check if an item exists in the model)

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

to

==========
mash: Replace ShelfIDs with AppLaunchID strings.

Rename AppLaunchId to ShelfID and use that instead of uint32_t.
(ShelfModel no longer assigns incrementing number ids to items)
Add ShelfID comparison operators and IsEmpty() helper function.
Allow empty app_id strings (iff the launch_id string is also empty).
Add a corresponding ShelfID mojo struct with traits and test.

Remove ShelfItem::app_launch_id, use ShelfItem::id instead.
Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
Move kShelfIDKey to ash/public/cpp to support Chrome usage.
Register kShelfIdKey for property conversion and mirroring.

DCHECK unique non-empty ShelfIDs in ShelfModel (avoid odd behavior).
DCHECK ShelfItemDelegate ShelfIDs match their ShelfItem's IDs.
(should hold in production, many test changes, TODO: CHECK?)
Add reasonable non-empty ids for AppList, Task Manager, Settings.

Require a kShelfIDKey property for ShelfWindowWatcher item creation.

TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

[msw, 2017-05-04 08:41:51]

Description was changed from

==========
mash: Replace ShelfIDs with AppLaunchID strings.

Rename AppLaunchId to ShelfID and use that instead of uint32_t.
(ShelfModel no longer assigns incrementing number ids to items)
Add ShelfID comparison operators and IsEmpty() helper function.
Allow empty app_id strings (iff the launch_id string is also empty).
Add a corresponding ShelfID mojo struct with traits and test.

Remove ShelfItem::app_launch_id, use ShelfItem::id instead.
Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
Move kShelfIDKey to ash/public/cpp to support Chrome usage.
Register kShelfIdKey for property conversion and mirroring.

DCHECK unique non-empty ShelfIDs in ShelfModel (avoid odd behavior).
DCHECK ShelfItemDelegate ShelfIDs match their ShelfItem's IDs.
(should hold in production, many test changes, TODO: CHECK?)
Add reasonable non-empty ids for AppList, Task Manager, Settings.

Require a kShelfIDKey property for ShelfWindowWatcher item creation.

TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

to

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Rename AppLaunchId to ShelfID and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem:
-Add comparison operators and IsEmpty() helper function.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Add a corresponding ShelfID mojo struct with traits and test.
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.

ShelfModel:
-DCHECK unique non-empty ShelfIDs (avoid odd behavior).
-DCHECK ShelfItemDelegate ShelfIDs match their items' IDs.

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep AppLaunchId name for now, change in followup.
OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

[msw, 2017-05-04 08:56:33]

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

[commit-bot: I haz the power, 2017-05-04 08:56:42]

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, 2017-05-04 09:37:10]

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

[commit-bot: I haz the power, 2017-05-04 09:37:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2017-05-04 10:53:23]

Description was changed from

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Rename AppLaunchId to ShelfID and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem:
-Add comparison operators and IsEmpty() helper function.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Add a corresponding ShelfID mojo struct with traits and test.
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.

ShelfModel:
-DCHECK unique non-empty ShelfIDs (avoid odd behavior).
-DCHECK ShelfItemDelegate ShelfIDs match their items' IDs.

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep AppLaunchId name for now, change in followup.
OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

to

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsEmpty() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

[msw, 2017-05-04 10:55:53]

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

[commit-bot: I haz the power, 2017-05-04 10:56:06]

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, 2017-05-04 11:56:52]

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

[commit-bot: I haz the power, 2017-05-04 11:56:53]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-04 14:17:30]

Hey James, please take a look, thanks!

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.cc:7: #include "base/logging.h"
self nit: removed

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:9: #include <string>
self nit: removed

[James Cook, 2017-05-04 16:38:50]

This is a big step forward. Thanks for tackling it.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
File ash/public/cpp/app_launch_id.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
ash/public/cpp/app_launch_id.cc:17: AppLaunchId::AppLaunchId(const std::string&
app_id) : app_id_(app_id) {}
optional: = default (and below)

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
File ash/public/cpp/app_launch_id.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
ash/public/cpp/app_launch_id.h:36: bool IsEmpty() const;
naming question: Should this be IsValid()? Or have the comment say something
about what it means when both are empty? It's not clear from just this header
what empty is used for.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_i...
File ash/public/cpp/shelf_item.h (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_i...
ash/public/cpp/shelf_item.h:29: ShelfID id = kInvalidShelfID;
Hooray, less duplicate data!

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
File ash/public/cpp/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
ash/public/cpp/shelf_struct_traits.cc:24: *out = ash::ShelfID(app_id,
launch_id);
Reading a ShelfID looks like it allocates and copies strings at least twice. Is
it possible to friend the structtraits in ShelfID / AppLaunchID and then read
the data directly into out->app_id_ and out->launch_id_ ?

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.cc:7: #include "base/logging.h"
On 2017/05/04 14:17:30, msw wrote:
> self nit: removed

Huh?

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:9: #include <string>
On 2017/05/04 14:17:30, msw wrote:
> self nit: removed

Huh?

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/window_...
File ash/public/cpp/window_properties.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/window_...
ash/public/cpp/window_properties.cc:16:
DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(ShelfID, kShelfIDKey, nullptr);
dumb question: Since ShelfID has copy and assignment constructors, do you have
to store it as an owned pointer? Could it just be a value? Or do those have to
be plain-old-data?

https://codereview.chromium.org/2860503002/diff/150001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:121: string app_id;            // An app id
string, used to match app windows.
nit: maybe call out that "app id" is usually an extension id? and that there are
special hard-coded ones for things like browser, task mgr, etc?

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/app_list_she...
File ash/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/app_list_she...
ash/shelf/app_list_shelf_item_delegate.cc:19: static constexpr const char
kAppListAppID[] = "AppList";
"static constexpr const"? Maybe just const or just constexpr.

optional: "AppListId" or "AppListAppId"

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model....
ash/shelf/shelf_model.cc:148: if (item.status == ash::STATUS_CLOSED) {
I think it's really nice that you clean up these sorts of things as you see
them.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model....
ash/shelf/shelf_model.cc:54: ShelfModel::ShelfModel() {}
optional: = default

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model.h
File ash/shelf/shelf_model.h (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model....
ash/shelf/shelf_model.h:123: ShelfID next_id_;
Thank goodness this is going away.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model_...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model_...
ash/shelf/shelf_model_unittest.cc:72: item.id = ShelfID("AppList");
ditto

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
File ash/shelf/shelf_window_watcher.cc (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher.cc:180: window->SetProperty(kShelfIDKey,
kInvalidShelfID);
Nice that we don't have to wipe the shelf ID anymore.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher.cc:110: if (key == aura::client::kAppIconKey ||
key == aura::client::kWindowIconKey ||
super nit: is there anything special about the order here? if not, consider
alphabetizing

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher.cc:226: if (window->GetProperty(kShelfIDKey))
maybe move this up and/or into the test for TYPE_UNDEFINED?

Will we be in a good state if a buggy app unexpectedly wipes its kShelfID window
property?

https://codereview.chromium.org/2860503002/diff/150001/ash/shell/window_watch...
File ash/shell/window_watcher.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shell/window_watch...
ash/shell/window_watcher.h:37: aura::Window* GetWindowByID(const ash::ShelfID&
id);
nit: ash:: not needed?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_launcher_prefs.h:28: namespace ash {
Hey, while you're here can you take this file out of namespace ash? I don't
think we should be using namespace ash in chrome code.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:356:
ash::ShelfItemDelegate* delegate = GetShelfItemDelegate(app_id);
Huh, I'm surprised this compiles, since there's no {} scope for the case.
Something new in C++11?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:373: LaunchApp(id,
ash::LAUNCH_FROM_UNKNOWN, event_flags);
Double-checking: We don't need to copy the ID anymore? Who owns the |id| when
this is called?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:598:
app_list.id = ash::ShelfID("AppList");
ditto

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:121:
const ash::ShelfID shelf_id = GetShelfId(app_window);
nit: DCHECK that it is valid

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/settings_window_observer.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/settings_window_observer.cc:56: static constexpr
const char kSettingsAppID[] = "Settings";
Are all of "static constexpr const" needed here?

Also, maybe "SettingsId" or "SettingsAppId" for the string?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
(left):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:134:
window->SetProperty(aura::client::kAppIdKey,
Just for my understanding: Where does the window pick up the shelfid property in
this case?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/task_manager_view.cc:107: static constexpr const char
kTaskManagerAppID[] = "Task Manager";
ditto

[msw, 2017-05-04 19:02:55]

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

[commit-bot: I haz the power, 2017-05-04 19:03:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-05-04 19:05:55]

msw@chromium.org changed reviewers:
+ sadrul@chromium.org, tsepez@chromium.org

[msw, 2017-05-04 19:05:58]

Comments addressed, please take another look; thanks!
Tom, please take a look at shelf.mojom.
Sadrul, please take a look at ui/aura/*.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
File ash/public/cpp/app_launch_id.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
ash/public/cpp/app_launch_id.cc:17: AppLaunchId::AppLaunchId(const std::string&
app_id) : app_id_(app_id) {}
On 2017/05/04 16:38:48, James Cook wrote:
> optional: = default (and below)

Doneish, it works for the empty-arg default ctor and the dtor, but not this one.
Here's the compile error: "only special member functions may be defaulted"
I also moved the "= default;" defs here from the header, per Elliot's advice.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
File ash/public/cpp/app_launch_id.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
ash/public/cpp/app_launch_id.h:36: bool IsEmpty() const;
On 2017/05/04 16:38:48, James Cook wrote:
> naming question: Should this be IsValid()? Or have the comment say something
> about what it means when both are empty? It's not clear from just this header
> what empty is used for.

Hmm, it would be nice to invert the boolean, but I didn't want to claim that an
ID is "valid" simply because one or both of the strings are not empty. Validity
might seem to imply more than that. It'd change many callsites, but I'll switch
to IsValid or maybe IsViable or similar if you prefer that (left as-is for now).

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_i...
File ash/public/cpp/shelf_item.h (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_i...
ash/public/cpp/shelf_item.h:29: ShelfID id = kInvalidShelfID;
On 2017/05/04 16:38:48, James Cook wrote:
> Hooray, less duplicate data!

Acknowledged.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
File ash/public/cpp/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
ash/public/cpp/shelf_struct_traits.cc:24: *out = ash::ShelfID(app_id,
launch_id);
On 2017/05/04 16:38:48, James Cook wrote:
> Reading a ShelfID looks like it allocates and copies strings at least twice.
Is
> it possible to friend the structtraits in ShelfID / AppLaunchID and then read
> the data directly into out->app_id_ and out->launch_id_ ?

WDYT about instead making AppLaunchId a struct with public |app_id| and
|launch_id| (in a followup or now)? It seems a bit odd that ShelfItem is such a
struct, but the simple two-string id it contains is a class with accessors.
Friending traits just adds complexity on top of unnecessary complexity.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.cc:7: #include "base/logging.h"
On 2017/05/04 16:38:48, James Cook wrote:
> On 2017/05/04 14:17:30, msw wrote:
> > self nit: removed
> 
> Huh?

ditto; maybe I should have written "nit: remove" or "note to self: remove this"?

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_t...
ash/public/cpp/shelf_types.h:9: #include <string>
On 2017/05/04 16:38:48, James Cook wrote:
> On 2017/05/04 14:17:30, msw wrote:
> > self nit: removed
> 
> Huh?

Ha, sorry, I was up really late working on this and reviewing it myself. I
remove this unused include locally, but hadn't yet uploaded a revised patch set.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/window_...
File ash/public/cpp/window_properties.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/window_...
ash/public/cpp/window_properties.cc:16:
DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(ShelfID, kShelfIDKey, nullptr);
On 2017/05/04 16:38:49, James Cook wrote:
> dumb question: Since ShelfID has copy and assignment constructors, do you have
> to store it as an owned pointer? Could it just be a value? Or do those have to
> be plain-old-data?

That'd be great, but properties are stored as int64_t, so strings won't work.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/interfaces/...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:121: string app_id;            // An app id
string, used to match app windows.
On 2017/05/04 16:38:49, James Cook wrote:
> nit: maybe call out that "app id" is usually an extension id? and that there
are
> special hard-coded ones for things like browser, task mgr, etc?

Done.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/app_list_she...
File ash/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/app_list_she...
ash/shelf/app_list_shelf_item_delegate.cc:19: static constexpr const char
kAppListAppID[] = "AppList";
On 2017/05/04 16:38:49, James Cook wrote:
> "static constexpr const"? Maybe just const or just constexpr.
> 
> optional: "AppListId" or "AppListAppId"

Done, I changed the identifier (but left the 'k' prefix) and contents.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model....
ash/shelf/shelf_model.cc:148: if (item.status == ash::STATUS_CLOSED) {
On 2017/05/04 16:38:49, James Cook wrote:
> I think it's really nice that you clean up these sorts of things as you see
> them.

Acknowledged.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model....
ash/shelf/shelf_model.cc:54: ShelfModel::ShelfModel() {}
On 2017/05/04 16:38:49, James Cook wrote:
> optional: = default

Done.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model.h
File ash/shelf/shelf_model.h (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model....
ash/shelf/shelf_model.h:123: ShelfID next_id_;
On 2017/05/04 16:38:49, James Cook wrote:
> Thank goodness this is going away.

Acknowledged.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model_...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_model_...
ash/shelf/shelf_model_unittest.cc:72: item.id = ShelfID("AppList");
On 2017/05/04 16:38:49, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
File ash/shelf/shelf_window_watcher.cc (left):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher.cc:180: window->SetProperty(kShelfIDKey,
kInvalidShelfID);
On 2017/05/04 16:38:49, James Cook wrote:
> Nice that we don't have to wipe the shelf ID anymore.

Agreed.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher.cc:110: if (key == aura::client::kAppIconKey ||
key == aura::client::kWindowIconKey ||
On 2017/05/04 16:38:49, James Cook wrote:
> super nit: is there anything special about the order here? if not, consider
> alphabetizing

I put the two icons together to wrap a bit nicer, but I can alphabetize if you
prefer. I don't see this list growing by much, if that matters.

https://codereview.chromium.org/2860503002/diff/150001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher.cc:226: if (window->GetProperty(kShelfIDKey))
On 2017/05/04 16:38:49, James Cook wrote:
> maybe move this up and/or into the test for TYPE_UNDEFINED?
> 
> Will we be in a good state if a buggy app unexpectedly wipes its kShelfID
window
> property?

Yeah, I guess if the id is cleared we should probably destroy the item.

https://codereview.chromium.org/2860503002/diff/150001/ash/shell/window_watch...
File ash/shell/window_watcher.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/shell/window_watch...
ash/shell/window_watcher.h:37: aura::Window* GetWindowByID(const ash::ShelfID&
id);
On 2017/05/04 16:38:49, James Cook wrote:
> nit: ash:: not needed?

Done here and in the cc file.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_launcher_prefs.h:28: namespace ash {
On 2017/05/04 16:38:49, James Cook wrote:
> Hey, while you're here can you take this file out of namespace ash? I don't
> think we should be using namespace ash in chrome code.

I 100% agree, but I'll do it in a followup, this CL is too big already.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:356:
ash::ShelfItemDelegate* delegate = GetShelfItemDelegate(app_id);
On 2017/05/04 16:38:49, James Cook wrote:
> Huh, I'm surprised this compiles, since there's no {} scope for the case.
> Something new in C++11?

Why do you think the brackets are needed?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:373: LaunchApp(id,
ash::LAUNCH_FROM_UNKNOWN, event_flags);
On 2017/05/04 16:38:49, James Cook wrote:
> Double-checking: We don't need to copy the ID anymore? Who owns the |id| when
> this is called?

This should be fine. The one caller, LauncherContextMenu::ExecuteCommand, passes
in the id from a ShelfItem that it owns, which should not be destroyed here.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:598:
app_list.id = ash::ShelfID("AppList");
On 2017/05/04 16:38:49, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:121:
const ash::ShelfID shelf_id = GetShelfId(app_window);
On 2017/05/04 16:38:49, James Cook wrote:
> nit: DCHECK that it is valid

Done.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/settings_window_observer.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/settings_window_observer.cc:56: static constexpr
const char kSettingsAppID[] = "Settings";
On 2017/05/04 16:38:49, James Cook wrote:
> Are all of "static constexpr const" needed here?
> 
> Also, maybe "SettingsId" or "SettingsAppId" for the string?

Done.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
(left):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:134:
window->SetProperty(aura::client::kAppIdKey,
On 2017/05/04 16:38:49, James Cook wrote:
> Just for my understanding: Where does the window pick up the shelfid property
in
> this case?

It's done by ExtensionAppWindowLauncherController::RegisterApp. I could possibly
do it redundantly (or just) here... That might help if I eventually ever remove
the [extension/arc] launcher controllers and if this can support both cases, but
for now, RegisterApp seems like the best home for this logic?

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/task_manager_view.cc:107: static constexpr const char
kTaskManagerAppID[] = "Task Manager";
On 2017/05/04 16:38:49, James Cook wrote:
> ditto

Done.

[sky, 2017-05-04 19:28:45]

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

[sky, 2017-05-04 19:28:46]

ui/aura LGTM

[James Cook, 2017-05-04 19:45:08]

LGTM with nits. Epic CL - thanks for fixing this.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
File ash/public/cpp/app_launch_id.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
ash/public/cpp/app_launch_id.h:36: bool IsEmpty() const;
On 2017/05/04 19:05:56, msw wrote:
> On 2017/05/04 16:38:48, James Cook wrote:
> > naming question: Should this be IsValid()? Or have the comment say something
> > about what it means when both are empty? It's not clear from just this
header
> > what empty is used for.
> 
> Hmm, it would be nice to invert the boolean, but I didn't want to claim that
an
> ID is "valid" simply because one or both of the strings are not empty.
Validity
> might seem to imply more than that. It'd change many callsites, but I'll
switch
> to IsValid or maybe IsViable or similar if you prefer that (left as-is for
now).

OK. Please update the comment to say that empty is often used as a null or
invalid value.

highly optional: What do you think about IsNull() vs. IsEmpty()?

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
File ash/public/cpp/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
ash/public/cpp/shelf_struct_traits.cc:24: *out = ash::ShelfID(app_id,
launch_id);
On 2017/05/04 19:05:56, msw wrote:
> On 2017/05/04 16:38:48, James Cook wrote:
> > Reading a ShelfID looks like it allocates and copies strings at least twice.
> Is
> > it possible to friend the structtraits in ShelfID / AppLaunchID and then
read
> > the data directly into out->app_id_ and out->launch_id_ ?
> 
> WDYT about instead making AppLaunchId a struct with public |app_id| and
> |launch_id| (in a followup or now)? It seems a bit odd that ShelfItem is such
a
> struct, but the simple two-string id it contains is a class with accessors.
> Friending traits just adds complexity on top of unnecessary complexity.

Yeah, I think making ShelfID / AppLaunchId a struct would be better. Do that.

If you think it will touch many of the lines that you're touching in this CL I
would do it in this one. Otherwise a followup would be fine.

I would also check here that if the launch_id is non-empty that the app_id is
non-empty too.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_launcher_prefs.h:28: namespace ash {
On 2017/05/04 19:05:57, msw wrote:
> On 2017/05/04 16:38:49, James Cook wrote:
> > Hey, while you're here can you take this file out of namespace ash? I don't
> > think we should be using namespace ash in chrome code.
> 
> I 100% agree, but I'll do it in a followup, this CL is too big already.

SGTM

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:356:
ash::ShelfItemDelegate* delegate = GetShelfItemDelegate(app_id);
On 2017/05/04 19:05:57, msw wrote:
> On 2017/05/04 16:38:49, James Cook wrote:
> > Huh, I'm surprised this compiles, since there's no {} scope for the case.
> > Something new in C++11?
> 
> Why do you think the brackets are needed?

Because you're declaring a new variable but not adding a new scope. I think this
only works because there is no trailing case label. Otherwise I think you'll get
something like:

foo.cc: In function ‘int main()’:
foo.cc:13:10: error: jump to case label [-fpermissive]
     case 3:
          ^
foo.cc:10:11: error:   crosses initialization of ‘int b’
       int b = 3;

I would add braces.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
(left):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:134:
window->SetProperty(aura::client::kAppIdKey,
On 2017/05/04 19:05:58, msw wrote:
> On 2017/05/04 16:38:49, James Cook wrote:
> > Just for my understanding: Where does the window pick up the shelfid
property
> in
> > this case?
> 
> It's done by ExtensionAppWindowLauncherController::RegisterApp. I could
possibly
> do it redundantly (or just) here... That might help if I eventually ever
remove
> the [extension/arc] launcher controllers and if this can support both cases,
but
> for now, RegisterApp seems like the best home for this logic?

This SGTM.

[commit-bot: I haz the power, 2017-05-04 20:09:45]

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

[commit-bot: I haz the power, 2017-05-04 20:09:47]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-04 20:25:42]

Description was changed from

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsEmpty() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org
==========

to

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsNull() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org
==========

[msw, 2017-05-04 20:36:22]

Description was changed from

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsNull() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsNull() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,reveman@chromiu...
==========

[msw, 2017-05-04 20:36:22]

msw@chromium.org changed reviewers:
+ reveman@chromium.org
- sadrul@chromium.org

[msw, 2017-05-04 20:39:08]

tsepez, please review shelf.mojom (and shelf_struct_traits*?)
reveman, please review components/exo/shell_surface.cc

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
File ash/public/cpp/app_launch_id.h (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/app_lau...
ash/public/cpp/app_launch_id.h:36: bool IsEmpty() const;
On 2017/05/04 19:45:07, James Cook wrote:
> On 2017/05/04 19:05:56, msw wrote:
> > On 2017/05/04 16:38:48, James Cook wrote:
> > > naming question: Should this be IsValid()? Or have the comment say
something
> > > about what it means when both are empty? It's not clear from just this
> header
> > > what empty is used for.
> > 
> > Hmm, it would be nice to invert the boolean, but I didn't want to claim that
> an
> > ID is "valid" simply because one or both of the strings are not empty.
> Validity
> > might seem to imply more than that. It'd change many callsites, but I'll
> switch
> > to IsValid or maybe IsViable or similar if you prefer that (left as-is for
> now).
> 
> OK. Please update the comment to say that empty is often used as a null or
> invalid value.
> 
> highly optional: What do you think about IsNull() vs. IsEmpty()?

Done.

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
File ash/public/cpp/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/ash/public/cpp/shelf_s...
ash/public/cpp/shelf_struct_traits.cc:24: *out = ash::ShelfID(app_id,
launch_id);
On 2017/05/04 19:45:07, James Cook wrote:
> On 2017/05/04 19:05:56, msw wrote:
> > On 2017/05/04 16:38:48, James Cook wrote:
> > > Reading a ShelfID looks like it allocates and copies strings at least
twice.
> > Is
> > > it possible to friend the structtraits in ShelfID / AppLaunchID and then
> read
> > > the data directly into out->app_id_ and out->launch_id_ ?
> > 
> > WDYT about instead making AppLaunchId a struct with public |app_id| and
> > |launch_id| (in a followup or now)? It seems a bit odd that ShelfItem is
such
> a
> > struct, but the simple two-string id it contains is a class with accessors.
> > Friending traits just adds complexity on top of unnecessary complexity.
> 
> Yeah, I think making ShelfID / AppLaunchId a struct would be better. Do that.
> 
> If you think it will touch many of the lines that you're touching in this CL I
> would do it in this one. Otherwise a followup would be fine.
> 
> I would also check here that if the launch_id is non-empty that the app_id is
> non-empty too.

Done.

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2860503002/diff/150001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:356:
ash::ShelfItemDelegate* delegate = GetShelfItemDelegate(app_id);
On 2017/05/04 19:45:08, James Cook wrote:
> On 2017/05/04 19:05:57, msw wrote:
> > On 2017/05/04 16:38:49, James Cook wrote:
> > > Huh, I'm surprised this compiles, since there's no {} scope for the case.
> > > Something new in C++11?
> > 
> > Why do you think the brackets are needed?
> 
> Because you're declaring a new variable but not adding a new scope. I think
this
> only works because there is no trailing case label. Otherwise I think you'll
get
> something like:
> 
> foo.cc: In function ‘int main()’:
> foo.cc:13:10: error: jump to case label [-fpermissive]
>      case 3:
>           ^
> foo.cc:10:11: error:   crosses initialization of ‘int b’
>        int b = 3;
> 
> I would add braces.

Done.

[msw, 2017-05-04 20:39:21]

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

[commit-bot: I haz the power, 2017-05-04 20:40:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reveman, 2017-05-04 20:40:09]

components/exo lgtm

[commit-bot: I haz the power, 2017-05-04 21:56:25]

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

[commit-bot: I haz the power, 2017-05-04 21:56:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom Sepez, 2017-05-04 22:09:35]

lgtm

[James Cook, 2017-05-04 22:14:09]

https://codereview.chromium.org/2860503002/diff/180001/ash/public/cpp/shelf_s...
File ash/public/cpp/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2860503002/diff/180001/ash/public/cpp/shelf_s...
ash/public/cpp/shelf_struct_traits.cc:21: return data.ReadAppId(&out->app_id) &&
!data.ReadLaunchId(&out->launch_id) &&
extra "!" here?

Also, this might be clearer if it was more than one line (like first do the
reads, then check the empties)?

[msw, 2017-05-04 23:06:50]

Thanks! Landing

https://codereview.chromium.org/2860503002/diff/180001/ash/public/cpp/shelf_s...
File ash/public/cpp/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2860503002/diff/180001/ash/public/cpp/shelf_s...
ash/public/cpp/shelf_struct_traits.cc:21: return data.ReadAppId(&out->app_id) &&
!data.ReadLaunchId(&out->launch_id) &&
On 2017/05/04 22:14:08, James Cook wrote:
> extra "!" here?
> 
> Also, this might be clearer if it was more than one line (like first do the
> reads, then check the empties)?

Done. Good catch!

[msw, 2017-05-04 23:07:00]

The CQ bit was checked by msw@chromium.org

[msw, 2017-05-04 23:07:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org, tsepez@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2860503002/#ps220001
(title: "Fix struct traits typo.")

[commit-bot: I haz the power, 2017-05-04 23:07: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, 2017-05-05 00:13:40]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493939220463520,
"parent_rev": "b5affd8856bc45e32552223e5e41f7ae51a96220", "commit_rev":
"9c0f17e8b234b6d2fec38861a47b559660b7c53c"}

[commit-bot: I haz the power, 2017-05-05 00:14:09]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493939220463520,
"parent_rev": "8e3d93ee57c9f7e45f4570aff1957e4ea645738c", "commit_rev":
"84b8a5f1c2f1c62aadd4c943f3f093860cdc470d"}

[commit-bot: I haz the power, 2017-05-05 00:14:24]

Description was changed from

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsNull() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,reveman@chromiu...
==========

to

==========
mash: Replace int ShelfIDs with AppLaunchID strings.

Map ShelfID to AppLaunchId and use that instead of uint32_t.
ShelfModel no longer assigns int ids, clients supply unique strings.

ShelfID/ShelfItem/ShelfModel:
-Add comparison operators and IsNull() helper function.
-Add a corresponding ShelfID mojo struct with traits and test.
-Allow empty app_id strings (iff the launch_id string is also empty).
-Nix ShelfItem's AppLaunchId, use its ShelfID |id| instead.
-DCHECK unique non-empty ShelfIDs (avoid odd ShelfModel behavior).

Properties:
-Remove aura::client::kAppIdKey, use ash::kShelfIdKey instead.
-Move kShelfIDKey to ash/public/cpp to approve Chrome usage.
-Register kShelfIdKey for mus/mash conversion and mirroring.
-Require a kShelfIDKey property for ShelfWindowWatcher item creation.

Misc/Chrome:
-Set non-empty ShelfIDs for AppList, Task Manager, Settings.
-Apply ExtensionWindow* ShelfIDs for panel windows too.
-Use ShelfID in ExtensionAppWindowLauncherController maps.

TODO: Rename AppLaunchId to ShelfID (can do in this CL if desired)
TODO: Remove ShelfModel id conversion functions; audit users.
(also used to check item presence in ShelfModel, map ARC ids...)

OPTIONAL: Keep kShelfIDKey in ash/wm/window_properties for now?
OPTIONAL: CHECK in ShelfModel for prod/arc? Loosen checks for tests?

BUG=557406
TEST=No Chrome OS shelf or app regressions.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,reveman@chromiu...

Review-Url: https://codereview.chromium.org/2860503002
Cr-Commit-Position: refs/heads/master@{#469537}
Committed:
https://chromium.googlesource.com/chromium/src/+/84b8a5f1c2f1c62aadd4c943f3f0...
==========

[commit-bot: I haz the power, 2017-05-05 00:14:25]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/84b8a5f1c2f1c62aadd4c943f3f0...