Use Ash's ShelfWindowWatcher for app panel windows.

Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Extend and use ShelfWindowWatcher to add shelf items for panels.
Add delegate panel support; track windows with items; refactor.

Load icons in ExtensionAppWindowLauncherController::RegisterApp.
Add ShelfItem::app_id; taken from the window property.
Add/update tests; let ShelfModel remove all items (used in tests).

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS panel shelf item regressions.
R=jamescook@chromium.org

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

[msw, 2016-11-01 00:28:28]

Patchset #5 (id:80001) has been deleted

[msw, 2016-11-01 00:57:55]

Description was changed from

==========
Try using ShelfWindowWatcher for panels.

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

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncherController with panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

TODO: Add unit tests, address TODO comment drafts, etc.

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=
==========

[msw, 2016-11-01 22:27:32]

Description was changed from

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncherController with panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

TODO: Add unit tests, address TODO comment drafts, etc.

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=
==========

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

Generalize ChromeLauncherControllerImpl::IsOpen|Close helpers.
Move IsOpen() to ShelfItemDelegate for Close context items.

Remove unused/forked ash::wm::MoveWindowToEventRoot.
TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=jamescook@chromium.org
==========

[msw, 2016-11-01 22:27:32]

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

[msw, 2016-11-01 22:29:42]

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

[commit-bot: I haz the power, 2016-11-01 22:30:37]

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

[commit-bot: I haz the power, 2016-11-01 22:55:00]

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

[commit-bot: I haz the power, 2016-11-01 22:55:01]

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

[msw, 2016-11-01 23:06:06]

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

[commit-bot: I haz the power, 2016-11-01 23:06:36]

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

[msw, 2016-11-01 23:06:50]

Description was changed from

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

Generalize ChromeLauncherControllerImpl::IsOpen|Close helpers.
Move IsOpen() to ShelfItemDelegate for Close context items.

Remove unused/forked ash::wm::MoveWindowToEventRoot.
TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=jamescook@chromium.org
==========

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

Generalize ChromeLauncherControllerImpl::IsOpen|Close helpers.
Move IsOpen() to ShelfItemDelegate for Close context items.
Fork MoveWindowToEventRoot for ShelfWindowWatcherItemDelegate.

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-11-01 23:57:53]

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

[commit-bot: I haz the power, 2016-11-01 23:57:54]

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

[msw, 2016-11-02 00:04:08]

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

[commit-bot: I haz the power, 2016-11-02 00:04:38]

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

[msw, 2016-11-02 00:04:39]

Description was changed from

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

Generalize ChromeLauncherControllerImpl::IsOpen|Close helpers.
Move IsOpen() to ShelfItemDelegate for Close context items.
Fork MoveWindowToEventRoot for ShelfWindowWatcherItemDelegate.

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=jamescook@chromium.org
==========

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

Generalize ChromeLauncherControllerImpl::IsOpen|Close helpers.
Move IsOpen() to ShelfItemDelegate for Close context items.
Fork MoveWindowToEventRoot for ShelfWindowWatcherItemDelegate.

Add a ShelfWindowWatcherTest fixture for panel windows.
Remove ChromeLauncherControllerImplTest's panel use.

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=jamescook@chromium.org
==========

[msw, 2016-11-02 00:05:07]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2016-11-02 01:31:35]

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

[commit-bot: I haz the power, 2016-11-02 01:31: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_...)

[James Cook, 2016-11-02 17:53:44]

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.cc:69: // A new window was created in the
default container.
nit: update comment

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.cc:191: }
Maybe DCHECK that window is not in managed_user_windows_ anymore? At first I
thought we weren't cleaning up windows when they close.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher.h (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.h:25: // even if the window is transiently
reparented (e.g. during a drag).
nit: update comment to say it handles panels also

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.h:112: std::set<WmWindow*>
managed_user_windows_;
I'm OK with this name, but what about user_windows_with_items_? I was a little
confused by the code to cleanup windows when they close.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_item_delegate.cc (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_item_delegate.cc:30: bool
MoveWindowToEventRoot(WmWindow* window, const ui::Event& event) {
I think there's another copy of this in TestShelfItemDelegate. I'm OK with one
forked copy for mustash, but not two. Can you refactor this to share the code?
Maybe just make it a public static method?

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:279: WmWindow* window_1 =
WmLookup::Get()->GetWindowForWidget(widget1.get());
super nit: maybe window1 here or widget_1 above?

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:293: }
Thanks for writing a test.

Is it possible to write a similar (additional?) test that uses a more typical
panel creation code path, rather than tweaking the properties directly? Or do we
have coverage for that elsewhere?

https://codereview.chromium.org/2462753002/diff/200001/ash/common/wm_window_p...
File ash/common/wm_window_property.h (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/wm_window_p...
ash/common/wm_window_property.h:10: enum class WmWindowProperty {
nit: Add a comment pointing readers to ui/aura/client/aura_constants.h for
detailed descriptions of the items. Or document more below.

https://codereview.chromium.org/2462753002/diff/200001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2462753002/diff/200001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3322:
}
Hrm, kinda like this. Except this lives in chrome. Hmm. Would it work to write a
test in chrome that uses typical panel-creation code paths, then asserts that
the right window properties are set? Then that test, plus a test in ash that ash
shelf does the right thing when the window properties are correct, would cover
the total system.

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

https://codereview.chromium.org/2462753002/diff/200001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:37:
if (app_window->show_in_shelf()) {
Can you tell me more about this? This sounds like some stuff that was added by
Intel contributors for remote desktop apps. Are you sure it's OK to not check
for panels?

[msw, 2016-11-04 08:16:08]

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

[commit-bot: I haz the power, 2016-11-04 08:16: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, 2016-11-04 08:23:42]

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

[commit-bot: I haz the power, 2016-11-04 08:23:44]

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

[msw, 2016-11-04 21:25:03]

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

[commit-bot: I haz the power, 2016-11-04 21:25:54]

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

[commit-bot: I haz the power, 2016-11-04 23:08:05]

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

[commit-bot: I haz the power, 2016-11-04 23:08:06]

Dry run: This issue passed the CQ dry run.

[msw, 2016-11-10 17:20:17]

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

[commit-bot: I haz the power, 2016-11-10 17:20:36]

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

[commit-bot: I haz the power, 2016-11-10 18:25:15]

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

[commit-bot: I haz the power, 2016-11-10 18:25:16]

Dry run: This issue passed the CQ dry run.

[msw, 2016-11-10 20:44:27]

Description was changed from

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Observe the panel container and windows in ShelfWindowWatcher.
Add items for panels; extend ShelfWindowWatcherItemDelegate.
Track the managed window set to avoid modifying unrelated items.

Expose aura::Window's window and app icon properties.
Expose similar functions on WmWindow; Mus is not implemented.

Generalize ChromeLauncherControllerImpl::IsOpen|Close helpers.
Move IsOpen() to ShelfItemDelegate for Close context items.
Fork MoveWindowToEventRoot for ShelfWindowWatcherItemDelegate.

Add a ShelfWindowWatcherTest fixture for panel windows.
Remove ChromeLauncherControllerImplTest's panel use.

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS app panel window regressions.
R=jamescook@chromium.org
==========

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Extend and use ShelfWindowWatcher to add shelf items for panels.
Add delegate panel support; track windows with items; refactor.

Load icons in ExtensionAppWindowLauncherController::RegisterApp.
Add ShelfItem::app_id; taken from the window property.
Add/update tests; let ShelfModel remove all items (used in tests).

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS panel shelf item regressions.
R=jamescook@chromium.org
==========

[msw, 2016-11-10 20:50:42]

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

[commit-bot: I haz the power, 2016-11-10 20:51:05]

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

[msw, 2016-11-10 21:07:47]

Hey James, please take a fresh look; thanks!

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.cc:69: // A new window was created in the
default container.
On 2016/11/02 17:53:43, James Cook wrote:
> nit: update comment

Done.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.cc:191: }
On 2016/11/02 17:53:43, James Cook wrote:
> Maybe DCHECK that window is not in managed_user_windows_ anymore? At first I
> thought we weren't cleaning up windows when they close.

Done.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher.h (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.h:25: // even if the window is transiently
reparented (e.g. during a drag).
On 2016/11/02 17:53:44, James Cook wrote:
> nit: update comment to say it handles panels also

Done.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.h:112: std::set<WmWindow*>
managed_user_windows_;
On 2016/11/02 17:53:43, James Cook wrote:
> I'm OK with this name, but what about user_windows_with_items_? I was a little
> confused by the code to cleanup windows when they close.

Done.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_item_delegate.cc (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_item_delegate.cc:30: bool
MoveWindowToEventRoot(WmWindow* window, const ui::Event& event) {
On 2016/11/02 17:53:44, James Cook wrote:
> I think there's another copy of this in TestShelfItemDelegate. I'm OK with one
> forked copy for mustash, but not two. Can you refactor this to share the code?
> Maybe just make it a public static method?

Done (moved to WmWindow in a prior CL).

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:279: WmWindow* window_1 =
WmLookup::Get()->GetWindowForWidget(widget1.get());
On 2016/11/02 17:53:44, James Cook wrote:
> super nit: maybe window1 here or widget_1 above?

Done.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:293: }
On 2016/11/02 17:53:44, James Cook wrote:
> Thanks for writing a test.
> 
> Is it possible to write a similar (additional?) test that uses a more typical
> panel creation code path, rather than tweaking the properties directly? Or do
we
> have coverage for that elsewhere?

I added LauncherPlatformAppBrowserTest.AppPanel, restored and updated
ChromeLauncherControllerImplTest.AppPanels, and added some testing here with
ash_shell_lib's PanelWindow. There's also
LauncherPlatformAppBrowserTest.AppPanelClickBehavior.

https://codereview.chromium.org/2462753002/diff/200001/ash/common/wm_window_p...
File ash/common/wm_window_property.h (right):

https://codereview.chromium.org/2462753002/diff/200001/ash/common/wm_window_p...
ash/common/wm_window_property.h:10: enum class WmWindowProperty {
On 2016/11/02 17:53:44, James Cook wrote:
> nit: Add a comment pointing readers to ui/aura/client/aura_constants.h for
> detailed descriptions of the items. Or document more below.

Done (in a prior CL).

https://codereview.chromium.org/2462753002/diff/200001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2462753002/diff/200001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3322:
}
On 2016/11/02 17:53:44, James Cook wrote:
> Hrm, kinda like this. Except this lives in chrome. Hmm. Would it work to write
a
> test in chrome that uses typical panel-creation code paths, then asserts that
> the right window properties are set? Then that test, plus a test in ash that
ash
> shelf does the right thing when the window properties are correct, would cover
> the total system.

I restored and update this; it took a bit of work*, but that seems worthwhile.
See also the new LauncherPlatformAppBrowserTest.AppPanel and expanded
ShelfWindowWatcherTest.PanelWindow.

* I made ChromeLauncherControllerImplTest use the actual ShelfModel owned by
ash::WmShell's ShelfController,
and I also made ChromeNativeAppWindowViewsAuraAsh::OnBeforePanelWidgetInit set a
default context to avoid AshTestViewsDelegate::OnBeforeWidgetInit from adding
the panel to the root window, and instead let wm::GetDefaultParent supply the
panel container.

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

https://codereview.chromium.org/2462753002/diff/200001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:37:
if (app_window->show_in_shelf()) {
On 2016/11/02 17:53:44, James Cook wrote:
> Can you tell me more about this? This sounds like some stuff that was added by
> Intel contributors for remote desktop apps. Are you sure it's OK to not check
> for panels?

I aimed to remove all panel use/control from this class, but I reverted this
change to support an existing unit test check, etc.

[commit-bot: I haz the power, 2016-11-10 21:54:16]

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

[commit-bot: I haz the power, 2016-11-10 21:54:18]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-11-10 22:32:46]

https://codereview.chromium.org/2462753002/diff/380001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/BUILD.gn#newcode1360
ash/BUILD.gn:1360: ":ash_shell_lib",
Why is this needed? ash_shell should just be the little ash runtime environment.
I wouldn't expect it to be needed for tests.

Is there a stub that needs to be moved into shared test code?

https://codereview.chromium.org/2462753002/diff/380001/ash/common/DEPS
File ash/common/DEPS (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/DEPS#newcode17
ash/common/DEPS:17: "+ash/shell",
ditto

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.cc:138: model_->AddAt(item.type ==
TYPE_APP_PANEL ? 0 : model_->item_count(), item);
Shouldn't this be to the left of the leftmost existing panel, not to the left of
everything?

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_item_delegate.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_item_delegate.cc:28: : id_(id),
window_(window) {}
DCHECK that id is valid and window isn't null

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:16: #include
"ash/shell/panel_window.h"
Ah, this might be the reason. I would prefer you either move PanelWindow to
ash/test or create your own inline PanelWindow for this test.

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:304: }
Nice test.

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:930: return
controller ? controller->app_id() : model_->ItemByID(id)->app_id;
Is ItemByID() always guaranteed to return a valid iterator here? If so, can you
DCHECK something to tell the reader this is safe?

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:779:
ash::ShelfItemDelegate* item1_delegate =
optional: I would be OK with "using ash::ShelfItemDelegate" if it made the
wrapping better.

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:798:
}
+1 for increased test coverage.

[msw, 2016-11-10 23:30:52]

Comments addressed, please take another look; thanks!

https://codereview.chromium.org/2462753002/diff/380001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/BUILD.gn#newcode1360
ash/BUILD.gn:1360: ":ash_shell_lib",
On 2016/11/10 22:32:46, James Cook wrote:
> Why is this needed? ash_shell should just be the little ash runtime
environment.
> I wouldn't expect it to be needed for tests.
> 
> Is there a stub that needs to be moved into shared test code?

Reverted; this let ShelfWindowWatcherTest use PanelWindow.

https://codereview.chromium.org/2462753002/diff/380001/ash/common/DEPS
File ash/common/DEPS (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/DEPS#newcode17
ash/common/DEPS:17: "+ash/shell",
On 2016/11/10 22:32:46, James Cook wrote:
> ditto

Reverted.

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher.cc:138: model_->AddAt(item.type ==
TYPE_APP_PANEL ? 0 : model_->item_count(), item);
On 2016/11/10 22:32:46, James Cook wrote:
> Shouldn't this be to the left of the leftmost existing panel, not to the left
of
> everything?

Sending 0 will do the right thing. The shelf code orders items by type.
(this usage was moved from ChromeLauncherControllerImpl::CreateAppLauncherItem)

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_item_delegate.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_item_delegate.cc:28: : id_(id),
window_(window) {}
On 2016/11/10 22:32:46, James Cook wrote:
> DCHECK that id is valid and window isn't null

Done.

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_window_watcher_unittest.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:16: #include
"ash/shell/panel_window.h"
On 2016/11/10 22:32:46, James Cook wrote:
> Ah, this might be the reason. I would prefer you either move PanelWindow to
> ash/test or create your own inline PanelWindow for this test.

Yup, this was the reason. I tested an inlined panel-type widget here instead.

https://codereview.chromium.org/2462753002/diff/380001/ash/common/shelf/shelf...
ash/common/shelf/shelf_window_watcher_unittest.cc:304: }
On 2016/11/10 22:32:46, James Cook wrote:
> Nice test.

Acknowledged.

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:930: return
controller ? controller->app_id() : model_->ItemByID(id)->app_id;
On 2016/11/10 22:32:46, James Cook wrote:
> Is ItemByID() always guaranteed to return a valid iterator here? If so, can
you
> DCHECK something to tell the reader this is safe?

Ah, this wasn't technically safe (but also isn't checked elsewhere); fixed.

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:779:
ash::ShelfItemDelegate* item1_delegate =
On 2016/11/10 22:32:46, James Cook wrote:
> optional: I would be OK with "using ash::ShelfItemDelegate" if it made the
> wrapping better.

Eh, it's a minor tradeoff, left as-is.

https://codereview.chromium.org/2462753002/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:798:
}
On 2016/11/10 22:32:46, James Cook wrote:
> +1 for increased test coverage.

Acknowledged.

[msw, 2016-11-10 23:30:59]

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

[commit-bot: I haz the power, 2016-11-10 23:32:01]

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

[James Cook, 2016-11-10 23:38:31]

LGTM. Nice patch.

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

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

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

Dry run: This issue passed the CQ dry run.

[msw, 2016-11-11 00:29:38]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-11-11 00:30:14]

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

[commit-bot: I haz the power, 2016-11-11 00:37:15]

Description was changed from

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Extend and use ShelfWindowWatcher to add shelf items for panels.
Add delegate panel support; track windows with items; refactor.

Load icons in ExtensionAppWindowLauncherController::RegisterApp.
Add ShelfItem::app_id; taken from the window property.
Add/update tests; let ShelfModel remove all items (used in tests).

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS panel shelf item regressions.
R=jamescook@chromium.org
==========

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Extend and use ShelfWindowWatcher to add shelf items for panels.
Add delegate panel support; track windows with items; refactor.

Load icons in ExtensionAppWindowLauncherController::RegisterApp.
Add ShelfItem::app_id; taken from the window property.
Add/update tests; let ShelfModel remove all items (used in tests).

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS panel shelf item regressions.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-11-11 00:37:17]

Committed patchset #20 (id:400001)

[commit-bot: I haz the power, 2016-11-11 00:42:17]

Description was changed from

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Extend and use ShelfWindowWatcher to add shelf items for panels.
Add delegate panel support; track windows with items; refactor.

Load icons in ExtensionAppWindowLauncherController::RegisterApp.
Add ShelfItem::app_id; taken from the window property.
Add/update tests; let ShelfModel remove all items (used in tests).

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS panel shelf item regressions.
R=jamescook@chromium.org
==========

to

==========
Use Ash's ShelfWindowWatcher for app panel windows.

Do not use ExtensionAppWindowLauncher[Item]Controller for panels.
Extend and use ShelfWindowWatcher to add shelf items for panels.
Add delegate panel support; track windows with items; refactor.

Load icons in ExtensionAppWindowLauncherController::RegisterApp.
Add ShelfItem::app_id; taken from the window property.
Add/update tests; let ShelfModel remove all items (used in tests).

TODO: Support window/app icons for ui::Window/WmWindowMus?

BUG=557406
TEST=No Chrome OS panel shelf item regressions.
R=jamescook@chromium.org

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

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

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