Refactor LauncherItemController and LauncherItemDelegate

Refactor LauncherItemController and LauncherItemDelegate

* Remove LauncherItem parameter of LauncherItemDelegate method
* LauncherItemController subclasses LauncherItemDelegate
* Register/Unregister when LauncherItemDelegate is created/removed
* LauncherItemDelegateManager handles LauncherItemDelegate by LauncherID
* LauncherItemDelegateManager take ownership of all LauncherItemDelegate

R=sky@chromium.org, skuhne@chromium.org
BUG=279105
TEST=unit_tests, browser_tests, ash_unittests, ash_shell

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226547

[simonhong_, 2013-09-05 20:42:07]

Dear sky,
Please take a look.
Sorry for this big patch.
Thank you.
Simon.

[simonhong_, 2013-09-09 22:06:55]

kindly ping..

[sky, 2013-09-11 01:42:56]

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.h (right):

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.h:15: // LauncherItemDelegate for
TYPE_APP_LIST. Life-cycle of this class is handled
nit: life-cycle... -> Owned by LauncherItemDelegateManager.

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:21:
LauncherIDToItemDelegateMap::const_iterator iter =
STLDeleteContainerPairSecondPointers.

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:25: ++unregister_count_;
What value does this add beyond the DHECKs you have?

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:73: void
LauncherItemDelegateManager::UnregisterAllLauncherItemDelegateForTest() {
I don't like this being public. Instead put this logic in a class that is a
friend of LauncherItemDelegateManager. There are many examples of this pattern
in ash.

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:19: // LauncherItemDelegate based
on LauncherID. When LauncherItem is created, its
How about: LauncherItemDelegateManager manages the set of LauncherItemDelegates
for the launcher. LauncherItemDelegateManager does not create
LauncherItemDelegates, rather it is expected that someone else invokes
RegisterLauncherItemDelegate appropriately. On the other hand
LauncherItemDelegateManager destroys LauncherItemDelegates when the
corresponding item from the model is removed.

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:35: ash::LauncherItemDelegate*
item_delegate);
Can this take a scoped_ptr so that ownership is clear?

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:38: void
UpdateLauncherItemDelegate(ash::LauncherID id,
Why do we need this?

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_util.h
File ash/launcher/launcher_util.h (right):

https://codereview.chromium.org/23606016/diff/33001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.h:20: ASH_EXPORT ash::LauncherID GetBrowserItemID(
How about a single method that takes the type?

https://codereview.chromium.org/23606016/diff/33001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/23606016/diff/33001/ash/shell.cc#newcode863
ash/shell.cc:863: // Create AppListLauncherItemDelegate and register.
This comment describes exactly waht the code does and isn't helpful. Nuke it.

https://codereview.chromium.org/23606016/diff/33001/ash/shell/launcher_item_d...
File ash/shell/launcher_item_delegate_impl.h (right):

https://codereview.chromium.org/23606016/diff/33001/ash/shell/launcher_item_d...
ash/shell/launcher_item_delegate_impl.h:17: // Implementation of
ash::LauncherItemDelegate.
This needs a better description.

[simonhong_, 2013-09-11 09:05:26]

Dear sky,
I addressed your comments.
Please check again!

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/app_l...
File ash/launcher/app_list_launcher_item_delegate.h (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/app_l...
ash/launcher/app_list_launcher_item_delegate.h:15: // LauncherItemDelegate for
TYPE_APP_LIST. Life-cycle of this class is handled
On 2013/09/11 01:42:57, sky wrote:
> nit: life-cycle... -> Owned by LauncherItemDelegateManager.

Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.cc:21:
LauncherIDToItemDelegateMap::const_iterator iter =
On 2013/09/11 01:42:57, sky wrote:
> STLDeleteContainerPairSecondPointers.

Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.cc:25: ++unregister_count_;
On 2013/09/11 01:42:57, sky wrote:
> What value does this add beyond the DHECKs you have?

It is useless.
Removed.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.cc:73: void
LauncherItemDelegateManager::UnregisterAllLauncherItemDelegateForTest() {
On 2013/09/11 01:42:57, sky wrote:
> I don't like this being public. Instead put this logic in a class that is a
> friend of LauncherItemDelegateManager. There are many examples of this pattern
> in ash.

Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.h:19: // LauncherItemDelegate based
on LauncherID. When LauncherItem is created, its
On 2013/09/11 01:42:57, sky wrote:
> How about: LauncherItemDelegateManager manages the set of
LauncherItemDelegates
> for the launcher. LauncherItemDelegateManager does not create
> LauncherItemDelegates, rather it is expected that someone else invokes
> RegisterLauncherItemDelegate appropriately. On the other hand
> LauncherItemDelegateManager destroys LauncherItemDelegates when the
> corresponding item from the model is removed.

Thank you for suggesting good description!
Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.h:35: ash::LauncherItemDelegate*
item_delegate);
On 2013/09/11 01:42:57, sky wrote:
> Can this take a scoped_ptr so that ownership is clear?

Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.h:38: void
UpdateLauncherItemDelegate(ash::LauncherID id,
On 2013/09/11 01:42:57, sky wrote:
> Why do we need this?

For the PlatformApp LauncherItem, AppShortcutLauncherItemController isn't used
anymore when it is activated. Instead, ShellWindowLauncherItemController is
created for that Launcheritem.
At this time, LauncherItemDelegate is updated with existing LauncherItem.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
File ash/launcher/launcher_util.h (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_util.h:20: ASH_EXPORT ash::LauncherID GetBrowserItemID(
On 2013/09/11 01:42:57, sky wrote:
> How about a single method that takes the type?

Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/shell.cc
File ash/shell.cc (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/shell.cc#newco...
ash/shell.cc:863: // Create AppListLauncherItemDelegate and register.
On 2013/09/11 01:42:57, sky wrote:
> This comment describes exactly waht the code does and isn't helpful. Nuke it.

Done.

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/shell/launcher...
File ash/shell/launcher_item_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/shell/launcher...
ash/shell/launcher_item_delegate_impl.h:17: // Implementation of
ash::LauncherItemDelegate.
On 2013/09/11 01:42:57, sky wrote:
> This needs a better description.

Done.

[sky, 2013-09-12 20:40:13]

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.h:38: void
UpdateLauncherItemDelegate(ash::LauncherID id,
On 2013/09/11 09:05:26, Simon YoungKi Hong wrote:
> On 2013/09/11 01:42:57, sky wrote:
> > Why do we need this?
> 
> For the PlatformApp LauncherItem, AppShortcutLauncherItemController isn't used
> anymore when it is activated. Instead, ShellWindowLauncherItemController is
> created for that Launcheritem.
> At this time, LauncherItemDelegate is updated with existing LauncherItem.

Why can't it just call Register again?

[simonhong_, 2013-09-12 21:06:23]

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://chromiumcodereview.appspot.com/23606016/diff/33001/ash/launcher/launc...
ash/launcher/launcher_item_delegate_manager.h:38: void
UpdateLauncherItemDelegate(ash::LauncherID id,
On 2013/09/12 20:40:13, sky wrote:
> On 2013/09/11 09:05:26, Simon YoungKi Hong wrote:
> > On 2013/09/11 01:42:57, sky wrote:
> > > Why do we need this?
> > 
> > For the PlatformApp LauncherItem, AppShortcutLauncherItemController isn't
used
> > anymore when it is activated. Instead, ShellWindowLauncherItemController is
> > created for that Launcheritem.
> > At this time, LauncherItemDelegate is updated with existing LauncherItem.
> 
> Why can't it just call Register again?


Ah.. yes, I have a mistake.
It is same as Unregister... and Register
I removed this..

[simonhong_, 2013-09-13 02:18:29]

Dear sky,
I made UnregisterLauncherItemDelegate private method.
To do this, RegisterLauncherItemDelegate can overwrite if another
LauncherItemDelegate is existing for item.
That means, item delegate is destroyed only when LauncherItem is removed from
launcher model or new delegate is registered with existing LauncherItem.
Please check again.

[sky, 2013-09-13 17:04:27]

Is it possible the chrome side of the changes can be done in a separate patch?

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate.h:47: // handling might happen. The behavior
varies by the number of windows
This comment shouldn't be here, its an implementation detail of the actual
subclasses.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:15:
Shell::GetInstance()->launcher_model()->AddObserver(this);
Can you make the constructor take the LauncherModel?

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:37:
DCHECK(id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end());
Should this really be a DCHECK? Seems legal that this can be invoked with no
LauncherItemDelegate set.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:35: void
RegisterLauncherItemDelegate(
Name this SetLauncherItemDelegate as that it was it does (and update class
description).

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:58: void
UnregisterLauncherItemDelegate(ash::LauncherID id);
Unregister->Remove (same comment 61).

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
File ash/launcher/launcher_util.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.cc:20: ash::LauncherID
GetLauncherItemIDForType(ash::LauncherItemType type,
Add a test for this. Also, would be nice if you could fork into its own patch in
hopes of keeping the size of this smaller.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.cc:21: const LauncherModel& launcher_model) {
indentation is wrong.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.cc:22: DCHECK(type == ash::TYPE_APP_LIST || type ==
ash::TYPE_BROWSER_SHORTCUT);
Why does this need to be restricted to these types?

https://codereview.chromium.org/23606016/diff/51001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/shell.cc#newcode310
ash/shell.cc:310: launcher_item_delegate_manager_.reset();
If order is important (it is), document it.

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
File ash/shell/launcher_item_delegate_impl.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
ash/shell/launcher_item_delegate_impl.cc:28:
ash::wm::MoveWindowToEventRoot(window, event);
Why do we need this?

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
File ash/shell/launcher_item_delegate_impl.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
ash/shell/launcher_item_delegate_impl.h:17: // LauncherItemDelegateImpl handles
requests from Launcher for a LauncherItem in
This description is confusing. I think you want:

LauncherItemDelegate implementation used by WindowWatcher.

Also, a better name for this is WindowWatcherLaunchItemDelegate.

https://codereview.chromium.org/23606016/diff/51001/ash/test/launcher_item_de...
File ash/test/launcher_item_delegate_manager_test_api.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/test/launcher_item_de...
ash/test/launcher_item_delegate_manager_test_api.h:22: void
UnregisterAllLauncherItemDelegateForTest();
Unregister->Remove

https://codereview.chromium.org/23606016/diff/51001/ash/test/test_launcher_it...
File ash/test/test_launcher_item_delegate.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/test/test_launcher_it...
ash/test/test_launcher_item_delegate.cc:23:
ash::wm::MoveWindowToEventRoot(window_, event);
Again, why do we need this?

[simonhong_, 2013-09-13 20:07:17]

Dear sky,
I addressed your comments.
I think separating chrome-side modification into another patch is difficult
because LauncherItemDelegate interface is changed and getting
LauncherItemDelegate is based on LauncherID not LauncherItemType.
Please check again.
Thank you for reviewing this big patch.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate.h:47: // handling might happen. The behavior
varies by the number of windows
On 2013/09/13 17:04:28, sky wrote:
> This comment shouldn't be here, its an implementation detail of the actual
> subclasses.

Removed.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:15:
Shell::GetInstance()->launcher_model()->AddObserver(this);
On 2013/09/13 17:04:28, sky wrote:
> Can you make the constructor take the LauncherModel?

Done.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:37:
DCHECK(id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end());
On 2013/09/13 17:04:28, sky wrote:
> Should this really be a DCHECK? Seems legal that this can be invoked with no
> LauncherItemDelegate set.

Yes, it shouldn't be checked though all LauncherItems have its
LauncherItemDelegate.
I think some unittests don't need to have LauncherItemDelegate.
Done.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:35: void
RegisterLauncherItemDelegate(
On 2013/09/13 17:04:28, sky wrote:
> Name this SetLauncherItemDelegate as that it was it does (and update class
> description).

Done.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:58: void
UnregisterLauncherItemDelegate(ash::LauncherID id);
On 2013/09/13 17:04:28, sky wrote:
> Unregister->Remove (same comment 61).

Done.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
File ash/launcher/launcher_util.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.cc:20: ash::LauncherID
GetLauncherItemIDForType(ash::LauncherItemType type,
On 2013/09/13 17:04:28, sky wrote:
> Add a test for this. Also, would be nice if you could fork into its own patch
in
> hopes of keeping the size of this smaller.

Yep. I removed these modification from this CL.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.cc:21: const LauncherModel& launcher_model) {
On 2013/09/13 17:04:28, sky wrote:
> indentation is wrong.

Will be handled in a separated CL, too.

https://codereview.chromium.org/23606016/diff/51001/ash/launcher/launcher_uti...
ash/launcher/launcher_util.cc:22: DCHECK(type == ash::TYPE_APP_LIST || type ==
ash::TYPE_BROWSER_SHORTCUT);
On 2013/09/13 17:04:28, sky wrote:
> Why does this need to be restricted to these types?

Because only these two types have unique LauncherItem.
Other items can have multiple items for same type.
Will be handled in a separated CL, too.

https://codereview.chromium.org/23606016/diff/51001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/shell.cc#newcode310
ash/shell.cc:310: launcher_item_delegate_manager_.reset();
On 2013/09/13 17:04:28, sky wrote:
> If order is important (it is), document it.

Done.

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
File ash/shell/launcher_item_delegate_impl.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
ash/shell/launcher_item_delegate_impl.cc:28:
ash::wm::MoveWindowToEventRoot(window, event);
On 2013/09/13 17:04:28, sky wrote:
> Why do we need this?

I just copied this from original implementation.
I think this can be removed because this was already handled by
ScopedTargetRootWindow().
Removed.

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
File ash/shell/launcher_item_delegate_impl.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/shell/launcher_item_d...
ash/shell/launcher_item_delegate_impl.h:17: // LauncherItemDelegateImpl handles
requests from Launcher for a LauncherItem in
On 2013/09/13 17:04:28, sky wrote:
> This description is confusing. I think you want:
> 
> LauncherItemDelegate implementation used by WindowWatcher.
> 
> Also, a better name for this is WindowWatcherLaunchItemDelegate.

Done.

https://codereview.chromium.org/23606016/diff/51001/ash/test/launcher_item_de...
File ash/test/launcher_item_delegate_manager_test_api.h (right):

https://codereview.chromium.org/23606016/diff/51001/ash/test/launcher_item_de...
ash/test/launcher_item_delegate_manager_test_api.h:22: void
UnregisterAllLauncherItemDelegateForTest();
On 2013/09/13 17:04:28, sky wrote:
> Unregister->Remove

Done.

https://codereview.chromium.org/23606016/diff/51001/ash/test/test_launcher_it...
File ash/test/test_launcher_item_delegate.cc (right):

https://codereview.chromium.org/23606016/diff/51001/ash/test/test_launcher_it...
ash/test/test_launcher_item_delegate.cc:23:
ash::wm::MoveWindowToEventRoot(window_, event);
On 2013/09/13 17:04:28, sky wrote:
> Again, why do we need this?

I think this also can be removed because this was already handled by
ScopedTargetRootWindow().
Removed.

[sky, 2013-09-16 16:52:06]

Please reupload, your patch didn't make it.

[simonhong_, 2013-09-16 18:23:08]

On 2013/09/16 16:52:06, sky wrote:
> Please reupload, your patch didn't make it.

Dear sky,
I rebased.
Please take a look.

[sky, 2013-09-16 18:46:11]

https://codereview.chromium.org/23606016/diff/67001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/23606016/diff/67001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:32: if
(id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
No reason for if, Remove ignores requests to remove when not present.

https://codereview.chromium.org/23606016/diff/67001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:72: void
LauncherItemDelegateManager::RemoveAllLauncherItemDelegateForTest() {
This should be in the test helper, not here.

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:262: // This check
needs for win7_aura. If Shell is existing, |item->second| is
Same comment as with shell. If you need this check, it means something else is
wrong.

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:54: // All
LauncherItemControllers are owned by ash::LauncherItemDelegateManager.
This is the wrong place for this check. OnShellWindowAdded should only do work
if the window is in the shell.

[simonhong_, 2013-09-16 19:54:12]

https://codereview.chromium.org/23606016/diff/67001/ash/launcher/launcher_ite...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/23606016/diff/67001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:32: if
(id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
On 2013/09/16 18:46:11, sky wrote:
> No reason for if, Remove ignores requests to remove when not present.

Done.

https://codereview.chromium.org/23606016/diff/67001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:72: void
LauncherItemDelegateManager::RemoveAllLauncherItemDelegateForTest() {
On 2013/09/16 18:46:11, sky wrote:
> This should be in the test helper, not here.

Done.

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:54: // All
LauncherItemControllers are owned by ash::LauncherItemDelegateManager.
On 2013/09/16 18:46:11, sky wrote:
> This is the wrong place for this check. OnShellWindowAdded should only do work
> if the window is in the shell.

Do you mean that we should not create LauncherItem and LauncherItemController
when shell doesn't exist like win7_aura?
If so, how about this as a separate patch?
It was simple work in this class but needed some refactoring at
ChromeLauncherController.

[sky, 2013-09-16 19:57:57]

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc (right):

https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:54: // All
LauncherItemControllers are owned by ash::LauncherItemDelegateManager.
On 2013/09/16 19:54:13, Simon YoungKi Hong wrote:
> On 2013/09/16 18:46:11, sky wrote:
> > This is the wrong place for this check. OnShellWindowAdded should only do
work
> > if the window is in the shell.
> 
> Do you mean that we should not create LauncherItem and LauncherItemController
> when shell doesn't exist like win7_aura?
> If so, how about this as a separate patch?
> It was simple work in this class but needed some refactoring at
> ChromeLauncherController.

win-aura can have windows that exist in the shell and windows that don't. The
code needs to deal with that. You can't do this in a separate patch as AFAICT
your patch would break on win-aura now (ownership is wrong once shell gets
launched).

[simonhong_, 2013-09-16 20:07:41]

On 2013/09/16 19:57:57, sky wrote:
>
https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
> File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc
(right):
> 
>
https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
> chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:54: // All
> LauncherItemControllers are owned by ash::LauncherItemDelegateManager.
> On 2013/09/16 19:54:13, Simon YoungKi Hong wrote:
> > On 2013/09/16 18:46:11, sky wrote:
> > > This is the wrong place for this check. OnShellWindowAdded should only do
> work
> > > if the window is in the shell.
> > 
> > Do you mean that we should not create LauncherItem and
LauncherItemController
> > when shell doesn't exist like win7_aura?
> > If so, how about this as a separate patch?
> > It was simple work in this class but needed some refactoring at
> > ChromeLauncherController.
> 
> win-aura can have windows that exist in the shell and windows that don't. The
> code needs to deal with that. You can't do this in a separate patch as AFAICT
> your patch would break on win-aura now (ownership is wrong once shell gets
> launched).

The reason I added this check routine is for ChromeLauncherControllerTest.
BrowserWithTestWindowTest, a base class of ChromeLauncherControllerTest, doesn't
create a shell instance.
It only creates a shell under chromeos.
If win-arua creates shell in real object(I mean not in unit tests), I think
BrowserWithTestWindowTest also should create Shell.

[sky, 2013-09-16 20:13:56]

ChromeLauncherController should have the logic that it ends up owning
the delegate if necessary.

  -Scott

On Mon, Sep 16, 2013 at 1:07 PM,  <simon.hong81@gmail.com> wrote:
> On 2013/09/16 19:57:57, sky wrote:
>
>
https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
>>
>> File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc
>
> (right):
>
>
>
https://codereview.chromium.org/23606016/diff/67001/chrome/browser/ui/ash/lau...
>>
>> chrome/browser/ui/ash/launcher/shell_window_launcher_controller.cc:54: //
>> All
>> LauncherItemControllers are owned by ash::LauncherItemDelegateManager.
>> On 2013/09/16 19:54:13, Simon YoungKi Hong wrote:
>> > On 2013/09/16 18:46:11, sky wrote:
>> > > This is the wrong place for this check. OnShellWindowAdded should only
>> > > do
>> work
>> > > if the window is in the shell.
>> >
>> > Do you mean that we should not create LauncherItem and
>
> LauncherItemController
>>
>> > when shell doesn't exist like win7_aura?
>> > If so, how about this as a separate patch?
>> > It was simple work in this class but needed some refactoring at
>> > ChromeLauncherController.
>
>
>> win-aura can have windows that exist in the shell and windows that don't.
>> The
>> code needs to deal with that. You can't do this in a separate patch as
>> AFAICT
>> your patch would break on win-aura now (ownership is wrong once shell gets
>> launched).
>
>
> The reason I added this check routine is for ChromeLauncherControllerTest.
> BrowserWithTestWindowTest, a base class of ChromeLauncherControllerTest,
> doesn't
> create a shell instance.
> It only creates a shell under chromeos.
> If win-arua creates shell in real object(I mean not in unit tests), I think
> BrowserWithTestWindowTest also should create Shell.
>
> https://codereview.chromium.org/23606016/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong_, 2013-09-16 20:21:58]

On 2013/09/16 20:13:56, sky wrote:
> ChromeLauncherController should have the logic that it ends up owning
> the delegate if necessary.
> 
>   -Scott

ChromeLauncherController already handles ownership both cases(shell is existed
or not.)
If Shell is existed, delegate is owned by LauncherItemDelegateManager by
SetLauncherItemDelegate().
If not, delegate is owned by ChromeLauncherController. (It is destoryed by
OnRemoved()).

[sky, 2013-09-17 15:44:12]

https://codereview.chromium.org/23606016/diff/71001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23606016/diff/71001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:138: //
AppShortcutLauncherItemController is owned by LauncherItemDelegateManager.
I don't get this. Why would we create an AppShortcutLauncherItemController if
there is no shell? Same comment for all these.

[simonhong_, 2013-09-17 22:10:48]

https://codereview.chromium.org/23606016/diff/71001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23606016/diff/71001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:138: //
AppShortcutLauncherItemController is owned by LauncherItemDelegateManager.
On 2013/09/17 15:44:12, sky wrote:
> I don't get this. Why would we create an AppShortcutLauncherItemController if
> there is no shell? Same comment for all these.

As I know, check of shell existence is only for unittest(that doesn't create
shell).
If so, how about using mockup LauncherItemDelegateManager in the unittest?
with this mockup object, we should guarantee ownership of LauncherItemController
and don't need check like this.
Also OnRemoved() can be removed.
How about this?

[simonhong_, 2013-09-17 23:21:39]

Dear sky,
I uploaded new patch set.
In this patch, I used mockup LauncherItemDelegateManager when shell is not
existed.
I assume that LauncherItemController is only used when shell is existing(except
unit_tests)
Based on this assumption I made new patch.
If my assumption is not true, I will re-implement LauncherItemController as you
suggested.
Please check again.

[sky, 2013-09-17 23:52:22]

Have you this through the asan bots (you can run asan locally too) to make sure
there are no new leaks/errors?

[simonhong_, 2013-09-18 00:00:01]

I didn't checked with asan bot, yet. (Actually, I don't know howto test for
now)
Do you mean using try-server w/o commit check or local my linux machine?
If I can run it in my local linux machine, I will check it (maybe I can
find in the chromium developer site) and let you the result.


On Wed, Sep 18, 2013 at 8:52 AM, <sky@chromium.org> wrote:

> Have you this through the asan bots (you can run asan locally too) to make
> sure
> there are no new leaks/errors?
>
>
https://codereview.chromium.**org/23606016/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong_, 2013-09-18 13:42:23]

Dear sky,
I checked only ash_unittests with asan.
When I run the LauncherViewTest, it didn't show any leak messages.
So I think LauncherItemDelegateManager manages ownership of LauncherItemDelegate
well.
Unfortunately, unit_tests didn't run in my local machine. (I don't know why...)

[sky, 2013-09-18 17:14:55]

Did you try a release build and filter to only the effected tests?

On Wed, Sep 18, 2013 at 6:42 AM,  <simon.hong81@gmail.com> wrote:
> Dear sky,
> I checked only ash_unittests with asan.
> When I run the LauncherViewTest, it didn't show any leak messages.
> So I think LauncherItemDelegateManager manages ownership of
> LauncherItemDelegate
> well.
> Unfortunately, unit_tests didn't run in my local machine. (I don't know
> why...)
>
> https://codereview.chromium.org/23606016/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong_, 2013-09-18 19:06:54]

On 2013/09/18 17:14:55, sky wrote:
> Did you try a release build and filter to only the effected tests?
> 
> On Wed, Sep 18, 2013 at 6:42 AM,  <mailto:simon.hong81@gmail.com> wrote:
> > Dear sky,
> > I checked only ash_unittests with asan.
> > When I run the LauncherViewTest, it didn't show any leak messages.
> > So I think LauncherItemDelegateManager manages ownership of
> > LauncherItemDelegate
> > well.
> > Unfortunately, unit_tests didn't run in my local machine. (I don't know
> > why...)
> >
> > https://codereview.chromium.org/23606016/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

For ash_unittests, I ran below command and all tests are passed.
out/Release/ash_unittests --gtest_filter=Launcher*.* 2>&1 |
tools/valgrind/asan/asan_symbolize.py | c++filt
For unit_tests, I tried below command. (Not worked.)
out/Release/unit_tests --gtest_filter=ChromeLauncherControllerTest.* 2>&1 |
tools/valgrind/asan/asan_symbolize.py | c++filt

[sky, 2013-09-18 21:23:12]

On 2013/09/17 22:10:48, Simon YoungKi Hong wrote:
>
https://codereview.chromium.org/23606016/diff/71001/chrome/browser/ui/ash/lau...
> File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
> (right):
> 
>
https://codereview.chromium.org/23606016/diff/71001/chrome/browser/ui/ash/lau...
> chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:138:
//
> AppShortcutLauncherItemController is owned by LauncherItemDelegateManager.
> On 2013/09/17 15:44:12, sky wrote:
> > I don't get this. Why would we create an AppShortcutLauncherItemController
if
> > there is no shell? Same comment for all these.
> 
> As I know, check of shell existence is only for unittest(that doesn't create
> shell).
> If so, how about using mockup LauncherItemDelegateManager in the unittest?
> with this mockup object, we should guarantee ownership of
LauncherItemController
> and don't need check like this.
> Also OnRemoved() can be removed.
> How about this?

It seems as though you still have ShellWindowLauncherController creating
ShellWindowLauncherItemControllers when there is no shell. Why do that?

[sky, 2013-09-18 21:25:33]

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:213:
#if defined(OS_CHROMEOS)
nit: no indentation.

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:219:
else
nit: no else.

[simonhong_, 2013-09-19 12:20:30]

Dear sky,
With mockup LauncherItemDelegateManager, I think current implementation looks
fine instead of adding more codes for no shell in unit_tests.
The mockup manager can also handle ownership of all item delegate.
Please check again.
Thank you.

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:213:
#if defined(OS_CHROMEOS)
On 2013/09/18 21:25:33, sky wrote:
> nit: no indentation.

Done.

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23606016/diff/83001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/shell_window_launcher_item_controller.cc:219:
else
On 2013/09/18 21:25:33, sky wrote:
> nit: no else.

Done.

[sky, 2013-09-19 16:10:55]

You can do the scoped_ptr in a follow up pass.

LGTM

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h:33:
// Retrieve the list of running applications.
This comment is out of date now, right?

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:474:
ash::LauncherItemDelegate* item_delegate);
Make this take a scoped_ptr.

[Mr4D (OOO till 08-26), 2013-09-19 19:35:11]

(Odd system - am I now a reviewer or not? R= above: No, on the right side yes.
Will look also over it tonight.)

But either way - Please do not land this before the M31 branch point. We are too
close and this CL is quite big.

[simonhong_, 2013-09-19 19:36:11]

Dear sky,
Thank you for review!

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h
(right):

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h:33:
// Retrieve the list of running applications.
On 2013/09/19 16:10:56, sky wrote:
> This comment is out of date now, right?

Yep. I missed this.
Removed.

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/23606016/diff/91001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:474:
ash::LauncherItemDelegate* item_delegate);
On 2013/09/19 16:10:56, sky wrote:
> Make this take a scoped_ptr.

Yep. Inserted this to my todo list.

[simonhong_, 2013-09-19 19:42:57]

On 2013/09/19 19:35:11, Mr4D wrote:
> (Odd system - am I now a reviewer or not? R= above: No, on the right side yes.
> Will look also over it tonight.)
> 
> But either way - Please do not land this before the M31 branch point. We are
too
> close and this CL is quite big.

Dear skunhe,
Yep, I'll landing this after the M31 branch point.
I also added you as a reviewer, too.(sorry, I didn't update CL description.)
Please give any comments for this.
Thank you.
Simon.

[Mr4D (OOO till 08-26), 2013-09-20 16:20:51]

Impressive refactor!

Only a few nits, lgtm, but as said before: Please do not land yet!

https://codereview.chromium.org/23606016/diff/97001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/23606016/diff/97001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:218:
ash::Shell::GetInstance()->launcher_item_delegate_manager();
Is there a particular reason why you remember this and not grab it always from
here?

https://codereview.chromium.org/23606016/diff/97001/ash/shell/window_watcher_...
File ash/shell/window_watcher_launcher_item_delegate.cc (right):

https://codereview.chromium.org/23606016/diff/97001/ash/shell/window_watcher_...
ash/shell/window_watcher_launcher_item_delegate.cc:19: DCHECK(watcher_);
If checking for watcher_ - why not also checking that id is >0?

https://codereview.chromium.org/23606016/diff/97001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/23606016/diff/97001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:208: 
Can you delete one line?

[simonhong_, 2013-09-25 22:23:43]

On 2013/09/20 16:20:51, Mr4D wrote:
> Impressive refactor!
> 
> Only a few nits, lgtm, but as said before: Please do not land yet!
> 
>
https://codereview.chromium.org/23606016/diff/97001/ash/launcher/launcher_vie...
> File ash/launcher/launcher_view_unittest.cc (right):
> 
>
https://codereview.chromium.org/23606016/diff/97001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view_unittest.cc:218:
> ash::Shell::GetInstance()->launcher_item_delegate_manager();
> Is there a particular reason why you remember this and not grab it always from
> here?

I stored because it is referenced in many places when shortcut is created.
> 
>
https://codereview.chromium.org/23606016/diff/97001/ash/shell/window_watcher_...
> File ash/shell/window_watcher_launcher_item_delegate.cc (right):
> 
>
https://codereview.chromium.org/23606016/diff/97001/ash/shell/window_watcher_...
> ash/shell/window_watcher_launcher_item_delegate.cc:19: DCHECK(watcher_);
> If checking for watcher_ - why not also checking that id is >0?

done
> 
>
https://codereview.chromium.org/23606016/diff/97001/chrome/browser/ui/ash/lau...
> File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/23606016/diff/97001/chrome/browser/ui/ash/lau...
> chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:208: 
> Can you delete one line?

done.

Dear stefan,
I rebased.
I remove many patcheset I added commented inline.

[Mr4D (OOO till 08-26), 2013-09-25 23:13:22]

Simon,

it appears that I need to have another pass over the CL since we are
currently adding the multi user capabilities and as such the profile needs
to be taken into account. As such - please let me have another look and see
if there are problems. I will do that somewhen tomorrow.

-Stefan


On Wed, Sep 25, 2013 at 3:23 PM, <simon.hong81@gmail.com> wrote:

> On 2013/09/20 16:20:51, Mr4D wrote:
>
>> Impressive refactor!
>>
>
>  Only a few nits, lgtm, but as said before: Please do not land yet!
>>
>
>
> https://codereview.chromium.**org/23606016/diff/97001/ash/**
>
launcher/launcher_view_**unittest.cc<https://codereview.chromium.org/23606016/diff/97001/ash/launcher/launcher_view_unittest.cc>
>
>> File ash/launcher/launcher_view_**unittest.cc (right):
>>
>
>
> https://codereview.chromium.**org/23606016/diff/97001/ash/**
>
launcher/launcher_view_**unittest.cc#newcode218<https://codereview.chromium.org/23606016/diff/97001/ash/launcher/launcher_view_unittest.cc#newcode218>
>
>> ash/launcher/launcher_view_**unittest.cc:218:
>> ash::Shell::GetInstance()->**launcher_item_delegate_**manager();
>> Is there a particular reason why you remember this and not grab it always
>> from
>> here?
>>
>
> I stored because it is referenced in many places when shortcut is created.
>
>
>
> https://codereview.chromium.**org/23606016/diff/97001/ash/**
>
shell/window_watcher_launcher_**item_delegate.cc<https://codereview.chromium.org/23606016/diff/97001/ash/shell/window_watcher_launcher_item_delegate.cc>
>
>> File ash/shell/window_watcher_**launcher_item_delegate.cc (right):
>>
>
>
> https://codereview.chromium.**org/23606016/diff/97001/ash/**
>
shell/window_watcher_launcher_**item_delegate.cc#newcode19<https://codereview.chromium.org/23606016/diff/97001/ash/shell/window_watcher_launcher_item_delegate.cc#newcode19>
>
>> ash/shell/window_watcher_**launcher_item_delegate.cc:19:
>> DCHECK(watcher_);
>> If checking for watcher_ - why not also checking that id is >0?
>>
>
> done
>
>
>
> https://codereview.chromium.**org/23606016/diff/97001/**
>
chrome/browser/ui/ash/**launcher/chrome_launcher_**controller_unittest.cc<https://codereview.chromium.org/23606016/diff/97001/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc>
>
>> File chrome/browser/ui/ash/**launcher/chrome_launcher_**
>> controller_unittest.cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/23606016/diff/97001/**
> chrome/browser/ui/ash/**launcher/chrome_launcher_**controller_unittest.cc#
>
**newcode208<https://codereview.chromium.org/23606016/diff/97001/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc#newcode208>
>
>> chrome/browser/ui/ash/**launcher/chrome_launcher_**
>> controller_unittest.cc:208:
>> Can you delete one line?
>>
>
> done.
>
> Dear stefan,
> I rebased.
> I remove many patcheset I added commented inline.
>
>
https://codereview.chromium.**org/23606016/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong_, 2013-09-25 23:22:02]

Yep. Thank you!

[simonhong_, 2013-09-30 14:06:03]

Dear stefan,
I checked that this cl doesn't affect multi-profile support.
But, I found BrowserShortcutLauncherItemController store profile pointer when it
is created.
I handled it in a separate cl(already sent to you!).
Please check this cl again.

[commit-bot: I haz the power, 2013-10-02 04:32:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/188001

[commit-bot: I haz the power, 2013-10-02 04:33:00]

Failed to apply patch for ash/ash.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/ash.gyp
  Hunk #1 succeeded at 610 (offset -4 lines).
  Hunk #2 FAILED at 626.
  Hunk #3 succeeded at 738 (offset -2 lines).
  Hunk #4 succeeded at 908 (offset -3 lines).
  1 out of 4 hunks FAILED -- saving rejects to file ash/ash.gyp.rej

Patch:       ash/ash.gyp
Index: ash/ash.gyp
diff --git a/ash/ash.gyp b/ash/ash.gyp
index
62d2c76a2f75bd2cc0cd6d22eb463b7fa21b1fca..5fab5c6e3408786374eff0b7524b76bdc5dc85fb
100644
--- a/ash/ash.gyp
+++ b/ash/ash.gyp
@@ -614,6 +614,8 @@
         'test/launcher_view_test_api.h',
         'test/display_manager_test_api.cc',
         'test/display_manager_test_api.h',
+        'test/launcher_item_delegate_manager_test_api.cc',
+        'test/launcher_item_delegate_manager_test_api.h',
         'test/mirror_window_test_api.cc',
         'test/mirror_window_test_api.h',
         'test/shell_test_api.cc',
@@ -624,8 +626,10 @@
         'test/test_user_wallpaper_delegate.h',
         'test/test_launcher_delegate.cc',
         'test/test_launcher_delegate.h',
+        'test/test_launcher_item_delegate.cc',
+        'test/test_launcher_item_delegate.h',
         'test/test_session_state_delegate.cc',
-        'test/test_session_state_delegate.cc',
+        'test/test_session_state_delegate.h',
         'test/test_shell_delegate.cc',
         'test/test_shell_delegate.h',
         'test/test_suite.cc',
@@ -738,14 +742,21 @@
         'shell/context_menu.cc',
         'shell/context_menu.h',
         'shell/launcher_delegate_impl.cc',
+        'shell/launcher_delegate_impl.h',
         'shell/lock_view.cc',
         'shell/panel_window.cc',
+        'shell/panel_window.h',
         'shell/shell_delegate_impl.cc',
         'shell/shell_delegate_impl.h',
         'shell/toplevel_window.cc',
+        'shell/toplevel_window.h',
         'shell/widgets.cc',
         'shell/window_type_launcher.cc',
+        'shell/window_type_launcher.h',
         'shell/window_watcher.cc',
+        'shell/window_watcher.h',
+        'shell/window_watcher_launcher_item_delegate.cc',
+        'shell/window_watcher_launcher_item_delegate.h',
         'shell/window_watcher_unittest.cc',
         'system/chromeos/managed/tray_locally_managed_user_unittest.cc',
         'system/chromeos/network/network_state_notifier_unittest.cc',
@@ -902,6 +913,8 @@
         'shell/window_type_launcher.h',
         'shell/window_watcher.cc',
         'shell/window_watcher.h',
+        'shell/window_watcher_launcher_item_delegate.cc',
+        'shell/window_watcher_launcher_item_delegate.h',
         '../content/app/startup_helper_win.cc',
         '../ui/views/test/test_views_delegate.cc',
       ],

[commit-bot: I haz the power, 2013-10-02 04:48:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/193001

[commit-bot: I haz the power, 2013-10-02 05:24:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 05:39:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/217001

[commit-bot: I haz the power, 2013-10-02 07:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/231001

[commit-bot: I haz the power, 2013-10-02 07:26:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/242001

[commit-bot: I haz the power, 2013-10-02 08:22:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/259001

[commit-bot: I haz the power, 2013-10-02 14:35:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/230001

[commit-bot: I haz the power, 2013-10-02 15:37:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/281001

[commit-bot: I haz the power, 2013-10-02 15:45:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-10-02 15:47:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/281001

[commit-bot: I haz the power, 2013-10-02 17:29:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23606016/299001

[commit-bot: I haz the power, 2013-10-02 20:22:09]

Change committed as 226547

[dcheng, 2013-10-02 22:12:34]

On 2013/10/02 20:22:09, I haz the power (commit-bot) wrote:
> Change committed as 226547

This appears to be causing unit_test failures on the waterfall. See
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...

Sample failure:
[ RUN      ] ChromeLauncherControllerTest.PersistLauncherItemPositions
Backtrace:
	std::_Iterator_base12::_Adopt [0x5C449B05+197]
	std::_Vector_const_iterator<std::_Vector_val<ash::LauncherModelObserver
*,std::allocator<ash::LauncherModelObserver *> >
>::_Vector_const_iterator<std::_Vector_val<ash::LauncherModelObserver
*,std::allocator<ash::LauncherModelObserver *> > > [0x5C4D2BE1+81]
	std::_Vector_iterator<std::_Vector_val<ash::LauncherModelObserver
*,std::allocator<ash::LauncherModelObserver *> >
>::_Vector_iterator<std::_Vector_val<ash::LauncherModelObserver
*,std::allocator<ash::LauncherModelObserver *> > > [0x5C4D24BE+30]
	std::vector<ash::LauncherModelObserver
*,std::allocator<ash::LauncherModelObserver *> >::end [0x5C4D1101+49]
	ObserverListBase<ash::LauncherModelObserver>::RemoveObserver [0x5C4CF8C2+82]
	ash::LauncherModel::RemoveObserver [0x5C4CE9DD+29]
	ash::LauncherItemDelegateManager::~LauncherItemDelegateManager [0x5C4DA62E+94]
	ash::LauncherItemDelegateManager::`vector deleting destructor' [0x01E4671A+26]
	ChromeLauncherControllerTest::TearDown [0x01E44EAA+90]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
[0x04BC253F+319]
	testing::Test::Run [0x04BAF415+229]
	testing::TestInfo::Run [0x04BAFDAD+221]
	testing::TestCase::Run [0x04BB057F+239]
	testing::internal::UnitTestImpl::RunAllTests [0x04BB714D+701]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
[0x04BC2F37+327]
	testing::UnitTest::Run [0x04BB59B0+192]
	base::TestSuite::Run [0x069A69A0+240]
	content::UnitTestTestSuite::Run [0x068FCEFD+29]
	base::internal::RunnableAdapter<int (__thiscall
content::UnitTestTestSuite::*)(void)>::Run [0x065F3FCB+27]
	base::internal::InvokeHelper<0,int,base::internal::RunnableAdapter<int
(__thiscall content::UnitTestTestSuite::*)(void)>,void
__cdecl(content::UnitTestTestSuite *)>::MakeItSo [0x065F3F7A+26]
	base::internal::Invoker<1,base::internal::BindState<base::internal::RunnableAdapter<int
(__thiscall content::UnitTestTestSuite::*)(void)>,int
__cdecl(content::UnitTestTestSuite *),void
__cdecl(base::internal::UnretainedWrapper<content::UnitTestTestSuite>)
[0x065F3EFA+74]
	base::Callback<int __cdecl(void)>::Run [0x03A4C51F+47]
	base::LaunchUnitTests [0x0699C5C9+489]
	main [0x065F3B1B+203]
	__tmainCRTStartup [0x062F9B1F+447]
(f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c:555)
	mainCRTStartup [0x062F994F+15]
(f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c:371)
	BaseThreadInitThunk [0x75EB339A+18]
	RtlInitializeExceptionChain [0x779B9EF2+99]
	RtlInitializeExceptionChain [0x779B9EC5+54]