mash: Support ShelfModel access in Chrome.

mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Extend ShelfController interface to support ShelfModel modifications.
Extend ShelfObserver interface to report ShelfModel modifications.

Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and async load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add AppList item in model ctor and its delegate in controller ctor.
(avoids item ordering problems on init, Ash owns item delegate)

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add and update shelf controller, model, and CLC tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,

Review-Url: https://codereview.chromium.org/2833173002
Cr-Commit-Position: refs/heads/master@{#476166}
Committed: https://chromium.googlesource.com/chromium/src/+/19b30c2c383d894f472343c308f1e2f71b98a790

[msw, 2017-05-23 18:13:40]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

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

to

==========
mash: Support ShelfModel access in Chrome.

This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to just own ash::ShelfItemDelegate instances)



Add a stream output operator for ShelfID testing.

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

[msw, 2017-05-23 22:07:44]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to just own ash::ShelfItemDelegate instances)



Add a stream output operator for ShelfID testing.

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

to

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)

This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Restore deleted ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)

Remove Mash's workaround code in ShelfWindowWatcher.
TODO: Ensure that windows like QuickLaunch get shelf items.

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing.

TODO: Fix context menus, AppList's PinToShelf, etc.

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

[msw, 2017-05-25 20:21:19]

Patchset #2 (id:20001) has been deleted

[msw, 2017-05-25 20:21:26]

Patchset #2 (id:40001) has been deleted

[msw, 2017-05-25 20:21:34]

Patchset #3 (id:80001) has been deleted

[msw, 2017-05-25 21:40:25]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)

This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Restore deleted ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)

Remove Mash's workaround code in ShelfWindowWatcher.
TODO: Ensure that windows like QuickLaunch get shelf items.

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing.

TODO: Fix context menus, AppList's PinToShelf, etc.

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

to

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and delayed load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add ShelfController unittests; new CLC unit and browser tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
==========

[msw, 2017-05-25 23:30:55]

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

[commit-bot: I haz the power, 2017-05-25 23:32:02]

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-26 04:16:43]

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

[commit-bot: I haz the power, 2017-05-26 04:16:44]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-26 06:26:33]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and delayed load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add ShelfController unittests; new CLC unit and browser tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
==========

to

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and delayed load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add ShelfController unittests; new CLC unit and browser tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org
==========

[msw, 2017-05-26 06:26:33]

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

[msw, 2017-05-26 06:30:02]

Hey James, please take a look if you're feeling well enough; thanks!
I could split out some small bits, but it's mostly cohesive.
I'll ask Scott to review ash_config.cc, maybe a little more too.

[sky, 2017-05-26 19:42:00]

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

[sky, 2017-05-26 19:42:01]

I started on this, and have a question about the design choice you made. Happy
to talk in person next week if you prefer, or over IM.

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

https://codereview.chromium.org/2833173002/diff/180001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:69: LinkShelfModels(ShelfModelObserver
observer) => (ShelfModelObserver observer,
Is there a reason you don't want LinkShelfModels to take both observers rather
than using a callback? The second one would be a ShelfModelObserver&. That said,
is there a reason you prefer an observer in both directions rather than explicit
functions on ShelfController for the mutation? Using observers in both
directions muddies who the real owner of the state is. If you instead have
explicit functions on ShelfController for the mutation and an observer in only
one direction it's clear who owns what.

[msw, 2017-05-27 00:03:51]

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

[commit-bot: I haz the power, 2017-05-27 00:04:08]

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-27 00:04:39]

Thanks for the feedback; work in progress. I'll ping back next week.

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

https://codereview.chromium.org/2833173002/diff/180001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:69: LinkShelfModels(ShelfModelObserver
observer) => (ShelfModelObserver observer,
On 2017/05/26 19:42:01, sky wrote:
> Is there a reason you don't want LinkShelfModels to take both observers rather
> than using a callback? The second one would be a ShelfModelObserver&. That
said,
> is there a reason you prefer an observer in both directions rather than
explicit
> functions on ShelfController for the mutation? Using observers in both
> directions muddies who the real owner of the state is. If you instead have
> explicit functions on ShelfController for the mutation and an observer in only
> one direction it's clear who owns what.

I've started pursuing changes like what you suggest (adding explicit ShelfModel
client functions on ShelfController, dropping Ash's 'observation' of chrome, and
consolidating ShelfModelObserver functions onto the associated ShelfObserver
interface). I suppose using observers on both sides had a somewhat nice symmetry
(the impls on either sides was nearly identical) and that pattern punts the
issue of ownership (the model is simply replicated, not 'owned' on one side and
'cached' on the other), but ultimately, Ash *should* own the model, and Chrome
should be a client/observer of it, so your suggestions are worth pursuing.

I'll need to refine the new initialization sequence next week, after fixing one
issue (disagreement about indices amid init), I revised the client interface to
allow [temporary] index disagreement between the models, but now I get the
profile-pinned apps out of order on the shelf. I'll ping here when I've
addressed that defect (etc.), but feel free to make other high-level suggestions
or do a preliminary review pass.

[commit-bot: I haz the power, 2017-05-27 00:06:20]

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

[commit-bot: I haz the power, 2017-05-27 00:06:21]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2017-05-30 15:00:10]

msw, I can take over this review. Ping me when you want me to look.

[msw, 2017-05-30 18:58:39]

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

[commit-bot: I haz the power, 2017-05-30 18:59:32]

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-30 20:27:39]

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

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

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

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

[commit-bot: I haz the power, 2017-05-30 21:39:31]

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-30 21:46:25]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Add ShelfModelObserver and ShelfController::LinkShelfModels to mojom.
Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and delayed load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add ShelfController unittests; new CLC unit and browser tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org
==========

to

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Extend ShelfController interface to support ShelfModel modifications.
Extend ShelfObserver interface to report ShelfModel modifications.

Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and async load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add AppList item in model ctor and its delegate in controller ctor.
(avoids item ordering problems on init, Ash owns item delegate)

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add and update shelf controller, model, and CLC tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org
==========

[msw, 2017-05-30 21:47:06]

James, please take a look; thanks!

[commit-bot: I haz the power, 2017-05-30 22:17:36]

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

[commit-bot: I haz the power, 2017-05-30 22:17:36]

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-30 22:28:26]

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

[commit-bot: I haz the power, 2017-05-30 22:29:08]

Dry run: CQ is trying da patch.

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

[James Cook, 2017-05-30 22:47:48]

Approach looks good.

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

https://codereview.chromium.org/2833173002/diff/260001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:69: MoveShelfItem(ShelfID id, int32
target_index);
nit: document what happens for negative/out-of-range index. Also, I presume
"target_index" means "make this the index value" not "put it after |target|".

Also, I presume 0 is invalid, due to app list icon being there?

https://codereview.chromium.org/2833173002/diff/260001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:91: OnShelfItemMoved(int32 start_index, int32
target_index);
naively it seems like this should pass an ID to verify the destination really
has the right item in the right location

https://codereview.chromium.org/2833173002/diff/260001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:92: OnShelfItemChanged(int32 index, ShelfItem
item);
It's a little odd that this takes an index. I would expect an ID. I think it
needs docs explaining what counts as a change (does changing index mean a
change? I'm guessing no)

Also, should this be OnShelfItemUpdated() to correspond with UpdateShelfItem, or
is it something different?

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_contro...
File ash/shelf/shelf_controller_unittest.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_contro...
ash/shelf/shelf_controller_unittest.cc:105: // The ShelfModel should be
initialized with a single item for the AppList.
I think this test would be clearer if you took the code from here on down and
copy/pasted it into two tests, one that only runs in mash and one that only runs
in non-mash.

Coincidentally, the current TotT has an issue about conditionals in tests. <_< 
>_>

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_contro...
ash/shelf/shelf_controller_unittest.cc:148: }
Thanks for adding tests.

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:480: ui::Event::Clone(*passed_event), display_id,
LAUNCH_FROM_UNKNOWN,
Can you avoid cloning the event twice? Maybe move the Clone() inside the if()
and/or add an else?

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_window...
File ash/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher_unittest.cc:327: EXPECT_EQ(2,
model_->item_count());
Was this supposed to look for an empty icon? If not, update the comment.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:7: #include
"ash/shelf/shelf_model.h"
still needed?

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

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1327: if
(delegate.is_bound())
nit: curlies

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1341:
chromeos::GetAshConfig() == ash::Config::MASH) {
This (and the blocks below) is a little odd. So CLC is observing changes to its
own model and using that to report changes to ash? Naively I would expect code
in chrome to call functions on CLC, which would both update its model and send
mojo messages to ash.

Is that because code in chrome directly accesses CLC's ShelfModel instance? Or
because of the multiple updates done below?

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1383:
base::AutoReset<bool> reset(&applying_remote_shelf_model_changes_, false);
Aside: It's kind of a bummer that adding an item can result in 3 IPCs from
chrome to ash.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:399:
ash::ShelfModel* model_;
nit: document what this means in mash vs. non-mash

Also, I think you can clean up the TODO at
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/ash/chrome_brows...

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(left):

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:3450:
TEST_F(ChromeLauncherControllerTest, AppPanels) {
I presume this is going away because we're getting rid of panels? Or is it
tested somewhere else?

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

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:4269:
EXPECT_EQ(is_mash ? 1u : 0u, shelf_controller->added_count());
As before, I think this would be clearer with two tests, one mash and one
non-mash. Each could early-exit if it detected the wrong config.

[commit-bot: I haz the power, 2017-05-30 23:12:36]

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

[commit-bot: I haz the power, 2017-05-30 23:12:37]

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_...)

[sky, 2017-05-30 23:21:56]

Thanks for adding explicit mutation functions and having the observer only
in one direction. Much easier to understand! Thanks.

  -Scott

On Tue, May 30, 2017 at 2:47 PM, <msw@chromium.org> wrote:

> James, please take a look; thanks!
>
> https://codereview.chromium.org/2833173002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2017-05-31 16:54:29]

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

[msw, 2017-05-31 16:54:38]

Patchset #12 (id:280001) has been deleted

[commit-bot: I haz the power, 2017-05-31 16:54:52]

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-31 16:55:22]

Comments addressed. Please take another look, thanks!

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

https://codereview.chromium.org/2833173002/diff/260001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:69: MoveShelfItem(ShelfID id, int32
target_index);
On 2017/05/30 22:47:47, James Cook wrote:
> nit: document what happens for negative/out-of-range index. Also, I presume
> "target_index" means "make this the index value" not "put it after |target|".
> 
> Also, I presume 0 is invalid, due to app list icon being there?

Done.

https://codereview.chromium.org/2833173002/diff/260001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:91: OnShelfItemMoved(int32 start_index, int32
target_index);
On 2017/05/30 22:47:47, James Cook wrote:
> naively it seems like this should pass an ID to verify the destination really
> has the right item in the right location

I'll pass the item's id instead of the original index.

https://codereview.chromium.org/2833173002/diff/260001/ash/public/interfaces/...
ash/public/interfaces/shelf.mojom:92: OnShelfItemChanged(int32 index, ShelfItem
item);
On 2017/05/30 22:47:47, James Cook wrote:
> It's a little odd that this takes an index. I would expect an ID. I think it
> needs docs explaining what counts as a change (does changing index mean a
> change? I'm guessing no)
> 
> Also, should this be OnShelfItemUpdated() to correspond with UpdateShelfItem,
or
> is it something different?

I removed that index param, fixed the name, added comments throughout.

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_contro...
File ash/shelf/shelf_controller_unittest.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_contro...
ash/shelf/shelf_controller_unittest.cc:105: // The ShelfModel should be
initialized with a single item for the AppList.
On 2017/05/30 22:47:47, James Cook wrote:
> I think this test would be clearer if you took the code from here on down and
> copy/pasted it into two tests, one that only runs in mash and one that only
runs
> in non-mash.
> 
> Coincidentally, the current TotT has an issue about conditionals in tests. <_<

> >_>

Done.

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_contro...
ash/shelf/shelf_controller_unittest.cc:148: }
On 2017/05/30 22:47:47, James Cook wrote:
> Thanks for adding tests.

Acknowledged.

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:480: ui::Event::Clone(*passed_event), display_id,
LAUNCH_FROM_UNKNOWN,
On 2017/05/30 22:47:47, James Cook wrote:
> Can you avoid cloning the event twice? Maybe move the Clone() inside the if()
> and/or add an else?
> 

Done. This miserable client code is a sign of poor infrastructure. I pinged
Sadrul and Hadi about mus not supporting mouse/touch events, but that seems to
be their intended design for now.

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_window...
File ash/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/ash/shelf/shelf_window...
ash/shelf/shelf_window_watcher_unittest.cc:327: EXPECT_EQ(2,
model_->item_count());
On 2017/05/30 22:47:48, James Cook wrote:
> Was this supposed to look for an empty icon? If not, update the comment.

Done.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:7: #include
"ash/shelf/shelf_model.h"
On 2017/05/30 22:47:48, James Cook wrote:
> still needed?

Done.

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

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1327: if
(delegate.is_bound())
On 2017/05/30 22:47:48, James Cook wrote:
> nit: curlies

Done.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1341:
chromeos::GetAshConfig() == ash::Config::MASH) {
On 2017/05/30 22:47:48, James Cook wrote:
> This (and the blocks below) is a little odd. So CLC is observing changes to
its
> own model and using that to report changes to ash? Naively I would expect code
> in chrome to call functions on CLC, which would both update its model and send
> mojo messages to ash.
> 
> Is that because code in chrome directly accesses CLC's ShelfModel instance? Or
> because of the multiple updates done below?

Yes, code in chrome directly accesses CLC's ShelfModel instance, some even still
uses ash::Shell::shelf_model() . I'll try to refine that later on by making
those Chrome callsites use CLC/ShelfController instead.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1383:
base::AutoReset<bool> reset(&applying_remote_shelf_model_changes_, false);
On 2017/05/30 22:47:48, James Cook wrote:
> Aside: It's kind of a bummer that adding an item can result in 3 IPCs from
> chrome to ash.

Perhaps worth following up.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:399:
ash::ShelfModel* model_;
On 2017/05/30 22:47:48, James Cook wrote:
> nit: document what this means in mash vs. non-mash
> 
> Also, I think you can clean up the TODO at
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/ash/chrome_brows...

Done.

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(left):

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:3450:
TEST_F(ChromeLauncherControllerTest, AppPanels) {
On 2017/05/30 22:47:48, James Cook wrote:
> I presume this is going away because we're getting rid of panels? Or is it
> tested somewhere else?

Replaced by ShelfWindowWatcherTest.ItemIcon.

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

https://codereview.chromium.org/2833173002/diff/260001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:4269:
EXPECT_EQ(is_mash ? 1u : 0u, shelf_controller->added_count());
On 2017/05/30 22:47:48, James Cook wrote:
> As before, I think this would be clearer with two tests, one mash and one
> non-mash. Each could early-exit if it detected the wrong config.

This test is actually pretty pointless in classic ash, so I made it mash-only.

[commit-bot: I haz the power, 2017-05-31 16:57:45]

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

[commit-bot: I haz the power, 2017-05-31 16:57:47]

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

[msw, 2017-05-31 16:59:29]

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

[commit-bot: I haz the power, 2017-05-31 16:59:46]

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-31 18:36:27]

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

[commit-bot: I haz the power, 2017-05-31 18:36:28]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-05-31 20:02:31]

LGTM with nits. Epic CL.

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

https://codereview.chromium.org/2833173002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1287:
DCHECK_LE(index, model_->item_count()) << "Index out of bounds; clamping";
super-nit: I don't think the word "clamping" helps here or elow -- if we have
dchecks on we assert out, and if we have dchecks off nothing prints.

https://codereview.chromium.org/2833173002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1289: index =
std::min<int32_t>(std::max<int32_t>(index, 1), model_->item_count());
nit: Could this (and elsewhere) be std::min() without a type? I think everything
is an int, so it should be able to infer.

Aside: std::clamp() is coming in C++17!

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

https://codereview.chromium.org/2833173002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:4277:
EXPECT_EQ(2u, shelf_controller->added_count());
optional: This test might be slightly easier to read if you reset the
added_count and removed_count between each block.

[msw, 2017-05-31 20:31:34]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Extend ShelfController interface to support ShelfModel modifications.
Extend ShelfObserver interface to report ShelfModel modifications.

Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and async load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add AppList item in model ctor and its delegate in controller ctor.
(avoids item ordering problems on init, Ash owns item delegate)

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add and update shelf controller, model, and CLC tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org
==========

to

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Extend ShelfController interface to support ShelfModel modifications.
Extend ShelfObserver interface to report ShelfModel modifications.

Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and async load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add AppList item in model ctor and its delegate in controller ctor.
(avoids item ordering problems on init, Ash owns item delegate)

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add and update shelf controller, model, and CLC tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,
==========

[msw, 2017-05-31 20:31:34]

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

[msw, 2017-05-31 20:32:26]

Tom, please review ash/public/interfaces/*
Scott, please review chrome/browser/chromeos/*
(we discussed ash_config.cc offline last week, the arc change is simple)

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

https://codereview.chromium.org/2833173002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1287:
DCHECK_LE(index, model_->item_count()) << "Index out of bounds; clamping";
On 2017/05/31 20:02:30, James Cook wrote:
> super-nit: I don't think the word "clamping" helps here or elow -- if we have
> dchecks on we assert out, and if we have dchecks off nothing prints.

Done.

https://codereview.chromium.org/2833173002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1289: index =
std::min<int32_t>(std::max<int32_t>(index, 1), model_->item_count());
On 2017/05/31 20:02:30, James Cook wrote:
> nit: Could this (and elsewhere) be std::min() without a type? I think
everything
> is an int, so it should be able to infer.
> 
> Aside: std::clamp() is coming in C++17!

Done. Good to know!

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

https://codereview.chromium.org/2833173002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:4277:
EXPECT_EQ(2u, shelf_controller->added_count());
On 2017/05/31 20:02:30, James Cook wrote:
> optional: This test might be slightly easier to read if you reset the
> added_count and removed_count between each block.

I'm going to leave it as-is, the benefit doesn't outweigh the cost, IMO, albeit
the stakes seem rather low for either outcome.

[msw, 2017-05-31 20:32:37]

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

[commit-bot: I haz the power, 2017-05-31 20:33:48]

Dry run: CQ is trying da patch.

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

[Tom Sepez, 2017-05-31 21:14:27]

https://codereview.chromium.org/2833173002/diff/340001/ash/public/cpp/remote_...
File ash/public/cpp/remote_shelf_item_delegate.h (right):

https://codereview.chromium.org/2833173002/diff/340001/ash/public/cpp/remote_...
ash/public/cpp/remote_shelf_item_delegate.h:14: class ASH_PUBLIC_EXPORT
RemoteShelfItemDelegate : public ShelfItemDelegate {
This is WM -> browser or other trusted components, right?

[msw, 2017-05-31 21:36:55]

https://codereview.chromium.org/2833173002/diff/340001/ash/public/cpp/remote_...
File ash/public/cpp/remote_shelf_item_delegate.h (right):

https://codereview.chromium.org/2833173002/diff/340001/ash/public/cpp/remote_...
ash/public/cpp/remote_shelf_item_delegate.h:14: class ASH_PUBLIC_EXPORT
RemoteShelfItemDelegate : public ShelfItemDelegate {
On 2017/05/31 21:14:27, Tom Sepez (ooo after 6-2) wrote:
> This is WM -> browser or other trusted components, right?

Yes, this is WM <-> browser only for now. Ash constructs these when Chrome calls
[ash::mojom::]ShelfController::SetShelfItemDelegate, and Chrome constructs these
when Ash calls
[ash::mojom::ShelfObserver|ChromeLauncherController]::OnShelfItemDelegateChanged(),
as per Chrome's use of [ash::mojom::]ShelfController::AddObserver(). This CL
should not allow any other components to connect to either interface, no less
inject their own ShelfItemDelegates.

[commit-bot: I haz the power, 2017-05-31 22:07:07]

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

[commit-bot: I haz the power, 2017-05-31 22:07:08]

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

[Tom Sepez, 2017-05-31 22:38:52]

lgtm

[msw, 2017-05-31 23:05:55]

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

[commit-bot: I haz the power, 2017-05-31 23:06:19]

Dry run: CQ is trying da patch.

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

[sky, 2017-06-01 02:55:47]

LGTM

[commit-bot: I haz the power, 2017-06-01 02:57:42]

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

[commit-bot: I haz the power, 2017-06-01 02:57:43]

Dry run: This issue passed the CQ dry run.

[msw, 2017-06-01 03:16:23]

The CQ bit was checked by msw@chromium.org

[msw, 2017-06-01 03:16:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2833173002/#ps340001
(title: "Address comments; fix test failures.")

[commit-bot: I haz the power, 2017-06-01 03:16:47]

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-06-01 03:21:43]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1496286983849370,
"parent_rev": "aa1f66653b1719a83595604aa450ebb5cba59c25", "commit_rev":
"19b30c2c383d894f472343c308f1e2f71b98a790"}

[commit-bot: I haz the power, 2017-06-01 03:21:55]

Description was changed from

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Extend ShelfController interface to support ShelfModel modifications.
Extend ShelfObserver interface to report ShelfModel modifications.

Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and async load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add AppList item in model ctor and its delegate in controller ctor.
(avoids item ordering problems on init, Ash owns item delegate)

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add and update shelf controller, model, and CLC tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,
==========

to

==========
mash: Support ShelfModel access in Chrome.

Sync ShelfModels between ShelfController and ChromeLauncherController.
(allows synchronous access to ShelfModel/ShelfItem in Ash and Chrome)
This brings us much closer to a functional Shelf in Mash.
Implements step (2Bd) of my Mash Shelf Model and Control plan:
  docs.google.com/document/d/1wD0W2VKAEyeslXxc4L-QpOxZlIyaSZ74oSMndmrF120

Extend ShelfController interface to support ShelfModel modifications.
Extend ShelfObserver interface to report ShelfModel modifications.

Add/restore old ash::ShelfModelObserver::ShelfItemDelegateChanged.
Make mojom::ShelfItem::image optional (for tests and async load).
Make ShelfController and ChromeLauncherController observe and sync.
Avoid cyclically reporting remote changes with a flag.

Add RemoteShelfItemDelegate; to delegate between Ash and Chrome.
(allows ShelfModel to own/use ash::ShelfItemDelegate instances)
Add a binding and helper function on ash::ShelfItemDelegate.

Remove Mash's workaround code in ShelfWindowWatcher.
Remove unused/deprecated ash::mojom::ShelfController functions.

Add AppList item in model ctor and its delegate in controller ctor.
(avoids item ordering problems on init, Ash owns item delegate)

Add missing type mapping for ash::ShelfID and ash.mojom.ShelfID.
Add a stream output operator for ShelfID testing/logging.

Change chromeos::GetAshConfig to support --mash and --mus flags.
(This allows unit_tests to run in simulated mus/mash configs).

Add and update shelf controller, model, and CLC tests.
Replace a CLC panel test with ShelfWindowWatcherTest.ItemIcon.

TODO: Fix context menus, AppList's PinToShelf, etc.
TODO: Ensure that windows like QuickLaunch get shelf items.

BUG=557406
TEST=No Chrome OS changes; mash shelf shows pinned items, etc.
R=jamescook@chromium.org,sky@chromium.org,tsepez@chromium.org,

Review-Url: https://codereview.chromium.org/2833173002
Cr-Commit-Position: refs/heads/master@{#476166}
Committed:
https://chromium.googlesource.com/chromium/src/+/19b30c2c383d894f472343c308f1...
==========

[commit-bot: I haz the power, 2017-06-01 03:21:57]

Committed patchset #14 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/19b30c2c383d894f472343c308f1...