Refactor LauncherDelegate

Refactor LauncherDelegate

(This CL is splitted from https://codereview.chromium.org/11513005/ prior to adding LauncherItem for dialog)

Refactor LauncherItem bits in LauncherDelegate into LauncherItemDelegate.
LauncherItemDelegateManager helps Launcher/LauncherView to choose right LauncherItemDelegate based on LauncherItemType.

* AppListLauncherItemDelegate is added for TYPE_APP_LIST.
* LauncherItemManager (subclass of LauncherItemDelegate) will be added for TYPE_DIALOG in the separate CL.

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

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

[simonhong_, 2013-08-10 06:44:37]

Dear jamescook,
Please take a look.
Thank you.
Simon.

[James Cook, 2013-08-12 21:45:27]

What is the purpose of this change?  Why do we need it?  It seems to be
splitting an interface into two interfaces, but I don't understand the
motivation.

Also, would this be easier to do after the
chrome_launcher_controller_per_browser.h/cc code is removed?

[simonhong_, 2013-08-12 22:40:21]

On 2013/08/12 21:45:27, James Cook wrote:
> What is the purpose of this change?  Why do we need it?  It seems to be
> splitting an interface into two interfaces, but I don't understand the
> motivation.
Think about LauncherItemController. It handles LauncherItem specific bits. This
kind of things are needed when LauncherItem should be handled in ash.
TYPE_DIALOG will be introduced and it can handle in ash. There is more item(such
as app list) that can be handled in AshLauncherDelegate. When new item handler
is needed, it can implement by implementing LauncherItemDelegate.
> 
> Also, would this be easier to do after the
> chrome_launcher_controller_per_browser.h/cc code is removed?
Yes, it will be more easier.

[James Cook, 2013-08-13 17:33:38]

On 2013/08/12 22:40:21, Simon YoungKi Hong wrote:
> On 2013/08/12 21:45:27, James Cook wrote:
> > What is the purpose of this change?  Why do we need it?  It seems to be
> > splitting an interface into two interfaces, but I don't understand the
> > motivation.

I talked to skuhne - it sounds like this patch is split out from
https://codereview.chromium.org/11513005/ and is meant to help with the task
manager not showing in the shelf app menus crbug.com/121242.  If you have a
change split out from another patch, please mention that patch in the commit
description as it helps your reviewer know what is going on.  Likewise, you
should put BUG=121242 in this issue's commit description.

[Mr4D (OOO till 08-26), 2013-08-13 18:18:24]

I have found a few bugs for you to fix.

Beside that:

The "ProxyLauncherDelegate" feels really odd. Did sky ask for that?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
?? Does this actually work already?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:972: model_->items()[start_drag_index_]);
Should you do this assignment if start_drag_index_ == -1?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:996: model_->items()[current_index]);
Should you do this assignment if start_drag_index_ == -1?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1392:
delegate_->GetLauncherItemDelegate(model_->items()[index]);
Should you do this assignment if start_drag_index_ == -1?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1477: default:
Please do not use default here. Reason: If we are using a "switch" statement we
want to be informed by the compiler when a case was not covered.

That said: Here was never a default which means that in all cases you do the
same code. As such I think that you can get rid of the switch statement
alltogether?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1524: // Fallthrough
I don't think so! Using this fall through you record incorrectly the UMA about
the applist button.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1550:
menu_model.reset(item_delegate->CreateApplicationMenu(item, event.flags()));
And what else? It can be that another menu is still shown and the
menu_model.get() will result in != NULL. You have to menu_model.reset() here to
enforce any opened menus to close in that case.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1722: const LauncherItem* item =
LauncherItemForView(view);
Shouldn't you check here if (!item) return true?

https://codereview.chromium.org/22429004/diff/13001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/shell.cc#newcode853
ash/shell.cc:853: LauncherDelegate* Shell::GetLauncherDelegate() {
Somehow I need to think about this for a while. Did sky ask for this? It feels
rather odd to have "proxy functions" for an interface which should never be
called - or did I miss something here?

https://codereview.chromium.org/22429004/diff/13001/ash/shell.cc#newcode855
ash/shell.cc:855: DCHECK(!launcher_model_);
I assume you mean "!launcher_model_.get()" ?

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:111:
DCHECK(controller_);
Isn't there a DCHECK already in the instance function? If not so we should
probably have there one.

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1033: if
(HasItemController(id))
Do you know why this is necessary? We shouldn't come here in the first place if
an item is not valid.

[simonhong_, 2013-08-13 19:54:56]

Dear Stefan,
sky outlined what I should do and didn't reviewed specific implementation
details yet.
I made it as a Proxy to hide multiple LauncherDelegate from
Launcher/LauncherView and ShelfWidget.
Please check again!
Thank you.
Simon.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
On 2013/08/13 18:18:24, Mr4D wrote:
> ?? Does this actually work already?

Yes, AppList button works well with this ItemDelegate.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:972: model_->items()[start_drag_index_]);
On 2013/08/13 18:18:24, Mr4D wrote:
> Should you do this assignment if start_drag_index_ == -1?

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:996: model_->items()[current_index]);
On 2013/08/13 18:18:24, Mr4D wrote:
> Should you do this assignment if start_drag_index_ == -1?

I think you mean current_index not start_drag_index_.
"current_index == -1" can be removed because it is already checked by
above DCHECK_NE(-1, current_index).

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1392:
delegate_->GetLauncherItemDelegate(model_->items()[index]);
On 2013/08/13 18:18:24, Mr4D wrote:
> Should you do this assignment if start_drag_index_ == -1?

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1477: default:
On 2013/08/13 18:18:24, Mr4D wrote:
> Please do not use default here. Reason: If we are using a "switch" statement
we
> want to be informed by the compiler when a case was not covered.
> 
> That said: Here was never a default which means that in all cases you do the
> same code. As such I think that you can get rid of the switch statement
> alltogether?

Removed switch statement.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1524: // Fallthrough
On 2013/08/13 18:18:24, Mr4D wrote:
> I don't think so! Using this fall through you record incorrectly the UMA about
> the applist button.

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1550:
menu_model.reset(item_delegate->CreateApplicationMenu(item, event.flags()));
On 2013/08/13 18:18:24, Mr4D wrote:
> And what else? It can be that another menu is still shown and the
> menu_model.get() will result in != NULL. You have to menu_model.reset() here
to
> enforce any opened menus to close in that case.

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1722: const LauncherItem* item =
LauncherItemForView(view);
On 2013/08/13 18:18:24, Mr4D wrote:
> Shouldn't you check here if (!item) return true?

Done.
Q) Is "true" default value for showing tooltip?

https://codereview.chromium.org/22429004/diff/13001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/shell.cc#newcode853
ash/shell.cc:853: LauncherDelegate* Shell::GetLauncherDelegate() {
On 2013/08/13 18:18:24, Mr4D wrote:
> Somehow I need to think about this for a while. Did sky ask for this? It feels
> rather odd to have "proxy functions" for an interface which should never be
> called - or did I miss something here?

sky didn't tell specific implementation details for this.
As you may know, this function is called by ShelfWidget.

https://codereview.chromium.org/22429004/diff/13001/ash/shell.cc#newcode855
ash/shell.cc:855: DCHECK(!launcher_model_);
On 2013/08/13 18:18:24, Mr4D wrote:
> I assume you mean "!launcher_model_.get()" ?

Done.
But, I think DCHECK(!launcher_delegate_) also same meaning because it returns 1
if its managed object is not NULL otherwise returns 0.

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:111:
DCHECK(controller_);
On 2013/08/13 18:18:24, Mr4D wrote:
> Isn't there a DCHECK already in the instance function? If not so we should
> probably have there one.

Yes, it is already checked.
Removed.

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1033: if
(HasItemController(id))
On 2013/08/13 18:18:24, Mr4D wrote:
> Do you know why this is necessary? We shouldn't come here in the first place
if
> an item is not valid.

This is needed because we have two LauncherDelegate.
LauncherModel notifies to its observers regardless of the changing source.
When AshLauncherDelegate change LauncherItem (ex, app list item), this
notification also reaches to here.

[Mr4D (OOO till 08-26), 2013-08-14 15:52:18]

Please see my comments!

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
Why then this comment? (that makes it sound like that it does not work
otherwise).

Maybe something like 

// TODO(simon.hong81): This works for the moment, but the AppList LauncherItem
creation should move here.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:27: // root window.
I don't understand what this comment means. Maybe:

// Pass NULL here to show the app list in the currently active RootWindow.

?

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:48: // AppList don't show
application menu.
An AppList does not show an application menu.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1722: const LauncherItem* item =
LauncherItemForView(view);
Look at the old code:

return (!item || ...);

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1033: if
(HasItemController(id))
Which 2? Are you talking about the "Ash" and the "Chrome" portion above? If so -
please read my comments above.

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
Just thinking - couldn't this be created as all the other LauncherItemDelegates
in the realm of the browser and thus removing the need for this proxy/dispatch
class altogether? That would be far cleaner and better understandable.

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/ash_launcher...
File ash/launcher/ash_launcher_delegate.h (right):

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/ash_launcher...
ash/launcher/ash_launcher_delegate.h:18: // LauncherDelegate in Ash.
IF there is a reason to not follow my comment in
app_list_launcher_item_delegate.cc:

This is confusing. Why are you not simply putting the 2 needed lines of this
class into your "Proxy" class and rename the "Proxy" class into something like
"LauncherDelegateDispatcher" or so? That would make it much easier to understand
what is going on here.

[simonhong_, 2013-08-14 23:47:51]

Dear Stefan,
Please check again!
Thank you for review.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
On 2013/08/14 15:52:19, Mr4D wrote:
> Why then this comment? (that makes it sound like that it does not work
> otherwise).
> 
> Maybe something like 
> 
> // TODO(simon.hong81): This works for the moment, but the AppList LauncherItem
> creation should move here.

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:27: // root window.
On 2013/08/14 15:52:19, Mr4D wrote:
> I don't understand what this comment means. Maybe:
> 
> // Pass NULL here to show the app list in the currently active RootWindow.
> 
> ?

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:48: // AppList don't show
application menu.
On 2013/08/14 15:52:19, Mr4D wrote:
> An AppList does not show an application menu.

Done.

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/13001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1722: const LauncherItem* item =
LauncherItemForView(view);
On 2013/08/14 15:52:19, Mr4D wrote:
> Look at the old code:
> 
> return (!item || ...);

Done.

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1033: if
(HasItemController(id))
On 2013/08/14 15:52:19, Mr4D wrote:
> Which 2? Are you talking about the "Ash" and the "Chrome" portion above? If so
-
> please read my comments above.

Sorry, I wrote wrong answer.
Yes, currently we can't reach here and also with this CL.
But, think about LauncherItem for dialog (for TaskManager).
When LauncherItem is changed by LauncherModel::Set(), LauncherModel call
LauncherItemChanged() from observer list.
Then, we can reach here.
I think DCHECK should be removed because we will add LauncherItem that not
handled by ChromeLauncherController.

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
On 2013/08/14 15:52:19, Mr4D wrote:
> Just thinking - couldn't this be created as all the other
LauncherItemDelegates
> in the realm of the browser and thus removing the need for this proxy/dispatch
> class altogether? That would be far cleaner and better understandable.

Proxy/dispatch class is introduced for dialog launcher item (TaskManager).
Its item is handled only in ash(by LauncherItemManager - implemented in adding
TaskManager CL)
To handle request from Launcher/LauncherView for dialog item,
LauncherItemManager need a LauncherDelegate interface.
After some thinking, I think LauncherItemManager don't need whole the
LauncherDelegate interface,
it only need LauncherItem specific bits of LauncherDelegate(interfaces in
LauncherItemDelegate).
Sky also agreed that design and requested doing this refactoring in the separate
CL(this CL).
During the refactoring, AppListLauncherItemDelegate is introduced prior to
adding LaunherItemDelegate for dialog item(It will added in another CL)

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/ash_launcher...
File ash/launcher/ash_launcher_delegate.h (right):

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/ash_launcher...
ash/launcher/ash_launcher_delegate.h:18: // LauncherDelegate in Ash.
On 2013/08/14 15:52:19, Mr4D wrote:
> IF there is a reason to not follow my comment in
> app_list_launcher_item_delegate.cc:
> 
> This is confusing. Why are you not simply putting the 2 needed lines of this
> class into your "Proxy" class and rename the "Proxy" class into something like
> "LauncherDelegateDispatcher" or so? That would make it much easier to
understand
> what is going on here.

The reason why I another LauncherDelegate in Ash is GetIDByWindow().
Most of other interfaces are only served by ChromeLauncherDelegate.
So I also thought that LauncherDelegateProxy just return ItemDelegate of applist
or dialog
not via AshLauncherDelegate.
Of course GetIDByWindow() can search by using pointer of each ItemController in
ash.
But, I think doing this via LauncherDelegate is more cleaner design.
I assume that AshLauncherDelegate get more role in the future.
Which is more better approach?

[Mr4D (OOO till 08-26), 2013-08-15 15:53:18]

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/22429004/diff/13001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1033: if
(HasItemController(id))
So we plan that we do not have a ChromeLauncherController for some items? And as
such we will run into many new special cases after you have spent so much time
to make everything nice and clean? Why not?

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:17: // TODO: Create AppList
LauncherItem here.
Okay - so more are coming, but that somehow does not mean that this has to be
within ash?

What LauncherItemManager?

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/ash_launcher...
File ash/launcher/ash_launcher_delegate.h (right):

https://codereview.chromium.org/22429004/diff/22001/ash/launcher/ash_launcher...
ash/launcher/ash_launcher_delegate.h:18: // LauncherDelegate in Ash.
If it is only the one function you are interested in: Wouldn't it be better to
have a central registration place which can "register" a handler (and which
could be queried then)? Something like "AddGetIDByWindowHandler(..)". That way
the LauncherDelegate could then query all known handlers and return an ID.

[Mr4D (OOO till 08-26), 2013-08-15 20:25:45]

Simon,

please stop working on this for the moment since we need to circle back with sky
- once he is back from vacation.

Thanks,
Stefan

Short side notes:

You said that the main reason for the Delegate split as it is done is
GetIDByWindows - but..
- that is used only in Launcher in 2 places.
- if we would have LauncherItemControllers for special objects (like a task
manager, dialogs, ..) this would solve itself "magically".
- looking into other architectures I have found a "WindowWatcher" which could be
implemented to see when "special applications" get created and create items for
it automatically.

If there is a reason to have separate LauncherItemDelegates as suggested by sky,
we should at least remove the functions from LauncherDelegate which are defined
in LauncherItemDelegates to avoid the confusing double definition.

[simonhong_, 2013-08-15 22:53:55]

On 2013/08/15 20:25:45, Mr4D wrote:
> Simon,
> 
> please stop working on this for the moment since we need to circle back with
sky
> - once he is back from vacation.
> 
> Thanks,
> Stefan
> 
> Short side notes:
> 
> You said that the main reason for the Delegate split as it is done is
> GetIDByWindows - but..
> - that is used only in Launcher in 2 places.
> - if we would have LauncherItemControllers for special objects (like a task
> manager, dialogs, ..) this would solve itself "magically".

At first time, I implemented with LauncherItemController.
But, sky suggested that there is no reason to add codes in chrome side for this
because it can be done by monitoring interested window creation/deletion by
setting WindowProperty.
So, LauncherItemManager is implemented. It has similar architecture with
WindowWatcher
and some more duties for doing interactions with Launcher/LauncherView (like
clicking a LauncherItem).

> - looking into other architectures I have found a "WindowWatcher" which could
be
> implemented to see when "special applications" get created and create items
for
> it automatically.
> 
> If there is a reason to have separate LauncherItemDelegates as suggested by
sky,
> we should at least remove the functions from LauncherDelegate which are
defined
> in LauncherItemDelegates to avoid the confusing double definition.

New LauncherDelegate don't have duplicated interfaces with
LauncherItemDelegate...
I moved LauncherItem related interfaces into LauncherItemDelegate from
LauncherDelegate.

[Mr4D (OOO till 08-26), 2013-08-19 20:17:44]

As discussed - since sky was guiding this CL for several months already we let
him drive the changes again.

[James Cook, 2013-08-19 21:15:09]

-me, since sky is an owner here.  Re-add me if you need me

[simonhong_, 2013-08-20 08:25:29]

Dear sky,
I'll send rebased patch after removing PerBrowserLauncher.
Thank you.
Simon.

[simonhong_, 2013-08-23 07:41:38]

Dear Sky,
I updated this CL at patch set #8.
Please take a look.
Thank you.
Simon.

[sky, 2013-08-23 20:23:14]

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:112: LauncherItemDelegate* item_delegate =
Do we ever have a NULL delegate?

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher_del...
File ash/launcher/launcher_delegate_proxy.h (right):

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher_del...
ash/launcher/launcher_delegate_proxy.h:19: class LauncherDelegateProxy : public
ash::LauncherDelegate {
I think what you need is a class that lets you register LauncherItemDelegates
with. The AppListLauncherItemDelegate would be registered with it and the new
manager would register with it too. I'm fine with this class doing that, but I
think calling it LauncherDelegateProxy is a bit misleading. Long term we should
have only one delegate and getting the LauncherItemDelegate should not be on it.
We should just have a way to register the ItemDelegate. Maybe a better name is
LauncherItemDelegateManager.

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher_del...
ash/launcher/launcher_delegate_proxy.h:36: scoped_ptr<LauncherDelegate>
chrome_launcher_delegate_;
This shouldn't be named chrome.

[simonhong_, 2013-08-23 21:53:20]

Dear sky,
How about using AshLauncherDelegate?
It handles LauncherItemDelegate in ash and return right LauncherItemDelegate for
Launcher/LauncherView.
I think another LauncherDelegate is needed in ash to implement GetIDByWindow().

[sky, 2013-08-23 22:00:49]

On 2013/08/23 21:53:20, Simon YoungKi Hong wrote:
> Dear sky,
> How about using AshLauncherDelegate?
> It handles LauncherItemDelegate in ash and return right LauncherItemDelegate
for
> Launcher/LauncherView.
> I think another LauncherDelegate is needed in ash to implement
GetIDByWindow().

Long term I don't think it'll be a LauncherDelegate, so I would avoid using that
name.

[simonhong_, 2013-08-23 22:06:33]

AshLauncherDelegate is another LauncherDelegate not rename of
LauncherDelegateProxy.
I mean multiple LauncherDelegate in chrome and ash.
LauncherDelegateProxy is proxy for two LauncherDelegate from
Launcher/LauncherView.


On Sat, Aug 24, 2013 at 7:00 AM, <sky@chromium.org> wrote:

> On 2013/08/23 21:53:20, Simon YoungKi Hong wrote:
>
>> Dear sky,
>> How about using AshLauncherDelegate?
>> It handles LauncherItemDelegate in ash and return right
>> LauncherItemDelegate
>>
> for
>
>> Launcher/LauncherView.
>> I think another LauncherDelegate is needed in ash to implement
>>
> GetIDByWindow().
>
> Long term I don't think it'll be a LauncherDelegate, so I would avoid
> using that
> name.
>
>
https://codereview.chromium.**org/22429004/<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.

[sky, 2013-08-23 22:38:42]

I still like LauncherItemDelegateManager better than what you're suggesting.

On Fri, Aug 23, 2013 at 3:06 PM, Simon YoungKi Hong
<simon.hong81@gmail.com> wrote:
> AshLauncherDelegate is another LauncherDelegate not rename of
> LauncherDelegateProxy.
> I mean multiple LauncherDelegate in chrome and ash.
> LauncherDelegateProxy is proxy for two LauncherDelegate from
> Launcher/LauncherView.
>
>
> On Sat, Aug 24, 2013 at 7:00 AM, <sky@chromium.org> wrote:
>>
>> On 2013/08/23 21:53:20, Simon YoungKi Hong wrote:
>>>
>>> Dear sky,
>>> How about using AshLauncherDelegate?
>>> It handles LauncherItemDelegate in ash and return right
>>> LauncherItemDelegate
>>
>> for
>>>
>>> Launcher/LauncherView.
>>> I think another LauncherDelegate is needed in ash to implement
>>
>> GetIDByWindow().
>>
>> Long term I don't think it'll be a LauncherDelegate, so I would avoid
>> using that
>> name.
>>
>> https://codereview.chromium.org/22429004/
>
>

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

[simonhong_, 2013-08-26 08:47:09]

Dear sky,
LauncherItemDelegateManager is created.
Please check again.
Thank you.
Simon.

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:112: LauncherItemDelegate* item_delegate =
On 2013/08/23 20:23:14, sky wrote:
> Do we ever have a NULL delegate?

item_delegate is always not NULL.

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher_del...
File ash/launcher/launcher_delegate_proxy.h (right):

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher_del...
ash/launcher/launcher_delegate_proxy.h:19: class LauncherDelegateProxy : public
ash::LauncherDelegate {
On 2013/08/23 20:23:14, sky wrote:
> I think what you need is a class that lets you register LauncherItemDelegates
> with. The AppListLauncherItemDelegate would be registered with it and the new
> manager would register with it too. I'm fine with this class doing that, but I
> think calling it LauncherDelegateProxy is a bit misleading. Long term we
should
> have only one delegate and getting the LauncherItemDelegate should not be on
it.
> We should just have a way to register the ItemDelegate. Maybe a better name is
> LauncherItemDelegateManager.

Done.

https://codereview.chromium.org/22429004/diff/60002/ash/launcher/launcher_del...
ash/launcher/launcher_delegate_proxy.h:36: scoped_ptr<LauncherDelegate>
chrome_launcher_delegate_;
On 2013/08/23 20:23:14, sky wrote:
> This shouldn't be named chrome.

Removed.

[sky, 2013-08-27 00:03:46]

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:18: // TODO(simon.hong81): This
works for the moment, but the AppList LauncherItem
nit: indent comments here and 25/26.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:111: const ash::LauncherItem item =
launcher_view_->model()->items()[index];
const ash::LauncherItem&

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

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:17: class ASH_EXPORT
LauncherItemDelegateManager {
Add a description.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:22: // Returns
LauncherItemDelegate for |item_type|.
Document ownership.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:27: void
RegisterLauncherItemDelegate(
Document ownership. I think in the long term I would like to see this class take
ownership of the LauncherItemDelegate, ownership is clearer that way.

Wouldn't it be better if we register per id?

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1833:
item_manager_->GetLauncherItemDelegate(item->type);
You don't NULL every where. How come?

[simonhong_, 2013-08-27 01:05:29]

Dear sky,
Please check again.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/app_list_lau...
File ash/launcher/app_list_launcher_item_delegate.cc (right):

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/app_list_lau...
ash/launcher/app_list_launcher_item_delegate.cc:18: // TODO(simon.hong81): This
works for the moment, but the AppList LauncherItem
On 2013/08/27 00:03:47, sky wrote:
> nit: indent comments here and 25/26.

Done.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher.cc#...
ash/launcher/launcher.cc:111: const ash::LauncherItem item =
launcher_view_->model()->items()[index];
On 2013/08/27 00:03:47, sky wrote:
> const ash::LauncherItem&

Done.

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

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:17: class ASH_EXPORT
LauncherItemDelegateManager {
On 2013/08/27 00:03:47, sky wrote:
> Add a description.

Done.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:22: // Returns
LauncherItemDelegate for |item_type|.
On 2013/08/27 00:03:47, sky wrote:
> Document ownership.

Done.

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.h:27: void
RegisterLauncherItemDelegate(
On 2013/08/27 00:03:47, sky wrote:
> Document ownership. I think in the long term I would like to see this class
take
> ownership of the LauncherItemDelegate, ownership is clearer that way.
Yes, I think so.
This needs some refactoring of LauncherItemController and
ChromeLauncherController.
I'll add it my todo list.
> 
> Wouldn't it be better if we register per id?

Yes, registering with id is more better.
Todo this, some works are needed in ChromeLauncherController.
It will be achieved by registering LauncherItemDelegate when LauncherItem is
created and added.
(I added it in my todo list, too)

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/82001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1833:
item_manager_->GetLauncherItemDelegate(item->type);
On 2013/08/27 00:03:47, sky wrote:
> You don't NULL every where. How come?

GetLauncherItemDelegate() don't return NULL.
This function dchecked the existence of delegate .
For now, all types of LauncherItemDelegate must be registered by itself.
When we register with LauncherID, all LauncherItem will also exist when
LauncherItem is added.

[sky, 2013-08-27 03:56:39]

LGTM

[commit-bot: I haz the power, 2013-08-27 04:55:32]

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

[commit-bot: I haz the power, 2013-08-27 05:12:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-27 05:27:45]

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

[commit-bot: I haz the power, 2013-08-27 05:27:52]

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 FAILED at 138.
  Hunk #2 succeeded at 141 (offset -4 lines).
  1 out of 2 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
807097494fe1af7a7bd376f50b7bc614ce771dff..b6606f6b86e37ad2807a0ea40af98206ea848b08
100644
--- a/ash/ash.gyp
+++ b/ash/ash.gyp
@@ -138,6 +138,8 @@
         'launcher/alternate_app_list_button.h',
         'launcher/app_list_button.cc',
         'launcher/app_list_button.h',
+        'launcher/app_list_launcher_item_delegate.cc',
+        'launcher/app_list_launcher_item_delegate.h',
         'launcher/launcher.cc',
         'launcher/launcher.h',
         'launcher/launcher_alignment_menu.cc',
@@ -145,7 +147,10 @@
         'launcher/launcher_button.cc',
         'launcher/launcher_button.h',
         'launcher/launcher_delegate.h',
+        'launcher/launcher_item_delegate_manager.cc',
+        'launcher/launcher_item_delegate_manager.h',
         'launcher/launcher_icon_observer.h',
+        'launcher/launcher_item_delegate.h',
         'launcher/launcher_model.cc',
         'launcher/launcher_model.h',
         'launcher/launcher_model_observer.h',

[commit-bot: I haz the power, 2013-08-27 07:22:47]

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

[commit-bot: I haz the power, 2013-08-27 12:07:40]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-27 12:23:26]

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

[Mr4D (OOO till 08-26), 2013-08-27 15:00:52]

Much better then the original version! Thanks for doing this!

lgtm

(I suppose it is too late for addressing my comments - but maybe you can address
it later)

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
ash/launcher/launcher_item_delegate_manager.cc:17: // When new |item_delegate|
is registered with exsiting |type|, overwrites
//  When a new |item_delegate| is registered with an existing |type|, it will
get overwritten.

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
ash/launcher/launcher_item_delegate_manager.h:29: // This class don't own each
LauncherItemDelegate for now.
.. doesn't own ..

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
ash/launcher/launcher_item_delegate_manager.h:34: // For now, This class don't
own |item_delegate|.
.. This class doesn't own ..

https://codereview.chromium.org/22429004/diff/137001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/22429004/diff/137001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:248: if
(ash::Shell::HasInstance())
This is odd. Since the ChromeLauncherController gets created by the shell this
test shouldn't be necessary. I suppose it is done for unit tests? Did you try
without?

[simonhong_, 2013-08-27 15:08:54]

https://codereview.chromium.org/22429004/diff/137001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/22429004/diff/137001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:248: if
(ash::Shell::HasInstance())
On 2013/08/27 15:00:52, Mr4D wrote:
> This is odd. Since the ChromeLauncherController gets created by the shell this
> test shouldn't be necessary. I suppose it is done for unit tests? Did you try
> without?

Without this, all ChromeLauncherControllerTest is failed by win7_aura trybot.
(Of course, all above tests are passed by linux_chromeos)
I think it is strange why ChromeLauncherControllerTest is tested by win7_aura.

[Mr4D (OOO till 08-26), 2013-08-27 15:25:07]

Hmm. Well - in that case the least you should do is adding a comment there which
tests fail if the test isn't there.

And again - many thanks for your time and work!

[simonhong_, 2013-08-27 21:51:59]

Dear stefan,
Would you please check again?
I uploaded new patchset to pass OpenBrowserUsingShelfOnOhterDisplay.

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
File ash/launcher/launcher_item_delegate_manager.cc (right):

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
ash/launcher/launcher_item_delegate_manager.cc:17: // When new |item_delegate|
is registered with exsiting |type|, overwrites
On 2013/08/27 15:00:52, Mr4D wrote:
> //  When a new |item_delegate| is registered with an existing |type|, it will
> get overwritten.

Done.

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
File ash/launcher/launcher_item_delegate_manager.h (right):

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
ash/launcher/launcher_item_delegate_manager.h:29: // This class don't own each
LauncherItemDelegate for now.
On 2013/08/27 15:00:52, Mr4D wrote:
> .. doesn't own ..

Done.

https://codereview.chromium.org/22429004/diff/137001/ash/launcher/launcher_it...
ash/launcher/launcher_item_delegate_manager.h:34: // For now, This class don't
own |item_delegate|.
On 2013/08/27 15:00:52, Mr4D wrote:
> .. This class doesn't own ..

Done.

https://codereview.chromium.org/22429004/diff/160001/ash/launcher/launcher_vi...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/160001/ash/launcher/launcher_vi...
ash/launcher/launcher_view.cc:1648:
ShowListMenuForView(model_->items()[view_index], sender, event);
I moved this refactoring code into ScopedTargetRootWindow scoped_target
parentheses.
WindowSizerTest.OpenBrowserUsingShelfOnOhterDisplay was fail because of above
code.

[commit-bot: I haz the power, 2013-08-27 23:40:52]

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

[Mr4D (OOO till 08-26), 2013-08-28 00:06:06]

Oshima, could you please have a quick look at the latest change from Simon in
launcher_view.cc, line 1648, latest patch set?

I think this should be okay, but I don't see a reason why this unit test is
suddenly failing with his patch so that this modification is necessary and since
you just added this I would rather like to run this by you.

https://codereview.chromium.org/22429004/diff/160001/ash/launcher/launcher_vi...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/160001/ash/launcher/launcher_vi...
ash/launcher/launcher_view.cc:1648:
ShowListMenuForView(model_->items()[view_index], sender, event);
Sorry, but did you test why this was failing?

This makes me worried. Because Oshima was working on this just a few weeks ago.
From my understanding of that change I think this should be fine, but he should
have a quick look at this.

[commit-bot: I haz the power, 2013-08-28 00:32:11]

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

[simonhong_, 2013-08-28 00:33:58]

https://codereview.chromium.org/22429004/diff/160001/ash/launcher/launcher_vi...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22429004/diff/160001/ash/launcher/launcher_vi...
ash/launcher/launcher_view.cc:1648:
ShowListMenuForView(model_->items()[view_index], sender, event);
On 2013/08/28 00:06:06, Mr4D wrote:
> Sorry, but did you test why this was failing?
> 
> This makes me worried. Because Oshima was working on this just a few weeks
ago.
> From my understanding of that change I think this should be fine, but he
should
> have a quick look at this.

Yes, I tested and found failing reason.
Failing reason is calling ItemSelected() outside of ScopedTargetRootWindow
scope.

[commit-bot: I haz the power, 2013-08-28 05:22:43]

Change committed as 219924

[oshima, 2013-08-28 05:27:45]

https://chromiumcodereview.appspot.com/22429004/diff/160001/ash/launcher/laun...
File ash/launcher/launcher_view.cc (right):

https://chromiumcodereview.appspot.com/22429004/diff/160001/ash/launcher/laun...
ash/launcher/launcher_view.cc:1648:
ShowListMenuForView(model_->items()[view_index], sender, event);
On 2013/08/28 00:33:58, Simon YoungKi Hong wrote:
> On 2013/08/28 00:06:06, Mr4D wrote:
> > Sorry, but did you test why this was failing?
> > 
> > This makes me worried. Because Oshima was working on this just a few weeks
> ago.
> > From my understanding of that change I think this should be fine, but he
> should
> > have a quick look at this.
> 
> Yes, I tested and found failing reason.
> Failing reason is calling ItemSelected() outside of ScopedTargetRootWindow
> scope.

Yes, that's correct fix.