Refactor LauncherItemController and LauncherItemDelegate

Attempt 2: Refactor LauncherItemController and LauncherItemDelegate

Previous CL was reverted (https://codereview.chromium.org/23606016/) because LauncherItemDelegateManager holds destoryed LauncherModel pointer (in win7_aura trybot).
To handle this, destroy LauncherItemDelegateManager and recreates a new one with new LauncherModel.

Original description.

* 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=228595

[simonhong_, 2013-10-03 01:20:44]

Dear sky,
Previous cl was reverted.
I addressed that issue at patchset #2.
Please check again.
Thank you.
Simon.

[simonhong_, 2013-10-08 23:15:48]

Kindly ping..

[Mr4D (OOO till 08-26), 2013-10-09 00:38:22]

Please see comments!

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

https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:825: // FinalizeRipOffDrag().
Actually that is a problem: There are two views
a. the view inside the LauncherView and 
b. the dragged view.

Even if they are kept on top of each other, they would - as long as they stay
within the view's boundaries - add alpha transparency due to double blending.
This will be noticeable in alpha containing regions. Furthermore, when you leave
the bounding (layer) view the pixels outside the area will not be double blended
and the one inside will be -> which gives another artifact.

=> Which will not look good.

As such it might look correct on a first glimpse to keep both visible, but you
will see artifacts.

https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1822: LOG(ERROR) << __FUNCTION__;
I guess this can be removed?

https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1825: LOG(ERROR) << __FUNCTION__ << " " <<
IsShowingMenu();
And this?

https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1832: // 
owner_overflow_bubble_->HideBubbleAndRefreshButton();
I think this can go?

[simonhong_, 2013-10-09 00:56:33]

On 2013/10/09 00:38:22, Mr4D wrote:
> Please see comments!
> 
>
https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
> File ash/launcher/launcher_view.cc (right):
> 
>
https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view.cc:825: // FinalizeRipOffDrag().
> Actually that is a problem: There are two views
> a. the view inside the LauncherView and 
> b. the dragged view.
> 
> Even if they are kept on top of each other, they would - as long as they stay
> within the view's boundaries - add alpha transparency due to double blending.
> This will be noticeable in alpha containing regions. Furthermore, when you
leave
> the bounding (layer) view the pixels outside the area will not be double
blended
> and the one inside will be -> which gives another artifact.
> 
> => Which will not look good.
> 
> As such it might look correct on a first glimpse to keep both visible, but you
> will see artifacts.
> 
>
https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view.cc:1822: LOG(ERROR) << __FUNCTION__;
> I guess this can be removed?
> 
>
https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view.cc:1825: LOG(ERROR) << __FUNCTION__ << " " <<
> IsShowingMenu();
> And this?
> 
>
https://codereview.chromium.org/25823002/diff/36001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view.cc:1832: // 
> owner_overflow_bubble_->HideBubbleAndRefreshButton();
> I think this can go?

Oops!
Sorry, there was a problem during the rebase.
Previous patch set was mixed with other another one.
I rebased again.
I assume that your comment is for my another
CL(https://codereview.chromium.org/26320007/).

[Mr4D (OOO till 08-26), 2013-10-09 16:14:08]

It is a bit confusing towards what is now from the patch, what is new and what
is from the old patch.

Please address commments.

(Under which circumstances could there be no item_delegate?)

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

https://codereview.chromium.org/25823002/diff/57001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1880:
item_manager_->GetLauncherItemDelegate(item->id);
Is it possible to have an item with an assigned id which has no item delegate?

https://codereview.chromium.org/25823002/diff/57001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/25823002/diff/57001/ash/shell.cc#newcode502
ash/shell.cc:502: // TODO(simon.hong81): Make function for this in shelf_util.h
Would be nice to add a comment here like:
// Finding the launcher models location of the app list and setting its
LauncherItemDelegate.

[simonhong_, 2013-10-09 22:14:10]

Dear stefan,
Please check my comments.

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

https://codereview.chromium.org/25823002/diff/57001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1880:
item_manager_->GetLauncherItemDelegate(item->id);
On 2013/10/09 16:14:08, Mr4D wrote:
> Is it possible to have an item with an assigned id which has no item delegate?

In production code, all item have its item delegate.
But some items in unittests don't have.

https://codereview.chromium.org/25823002/diff/57001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/25823002/diff/57001/ash/shell.cc#newcode502
ash/shell.cc:502: // TODO(simon.hong81): Make function for this in shelf_util.h
On 2013/10/09 16:14:08, Mr4D wrote:
> Would be nice to add a comment here like:
> // Finding the launcher models location of the app list and setting its
> LauncherItemDelegate.

Done.

[Mr4D (OOO till 08-26), 2013-10-10 04:20:14]

Can you do a quick investigation for my comment in line 1880? Beside that I
guess the patch would be okay with the few changes you did. 

I am always trying to minimize the impact of test code onto normal functionality
and in this case it is possible that there is in fact a problem in production
code if the pointer would get NULL. So if we could address that that would be
great!

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

https://codereview.chromium.org/25823002/diff/57001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1880:
item_manager_->GetLauncherItemDelegate(item->id);
I just looked and there are many checks for this.

Since this can only be a problem for unit tests and not for production code it
would be ideal to be able to replace that with DCHECK's instead since it could
indicate a real problem.

Would it be possible to change the unit tests accordingly? Would that be much
work?

[simonhong_, 2013-10-10 04:32:50]

On 2013/10/10 04:20:14, Mr4D wrote:
> Can you do a quick investigation for my comment in line 1880? Beside that I
> guess the patch would be okay with the few changes you did. 
> 
> I am always trying to minimize the impact of test code onto normal
functionality
> and in this case it is possible that there is in fact a problem in production
> code if the pointer would get NULL. So if we could address that that would be
> great!
> 
>
https://codereview.chromium.org/25823002/diff/57001/ash/launcher/launcher_vie...
> File ash/launcher/launcher_view.cc (right):
> 
>
https://codereview.chromium.org/25823002/diff/57001/ash/launcher/launcher_vie...
> ash/launcher/launcher_view.cc:1880:
> item_manager_->GetLauncherItemDelegate(item->id);
> I just looked and there are many checks for this.
> 
> Since this can only be a problem for unit tests and not for production code it
> would be ideal to be able to replace that with DCHECK's instead since it could
> indicate a real problem.
> 
> Would it be possible to change the unit tests accordingly? Would that be much
> work?

Thank you for review.
Replacing with DCHECK and modifying current unittest is not much work.
I'll update and notify again!

[simonhong_, 2013-10-10 07:44:47]

Dear stefan,
I inserted NULL check routine into
LauncherItemDelegateManager::GetLauncherItemDelegate().
Please check again!

[Mr4D (OOO till 08-26), 2013-10-10 14:23:46]

Many thanks for doing this!

lgtm

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

https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:40: // When LauncherItem exists,
its LauncherItemDelegate must be existed, too.
What about this:

// Each LauncherItem has to have a LauncherItemDelegate.

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

Dear sky,
I need owners check.
Most of part are not changed previous version.
Please check again.

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

https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
ash/launcher/launcher_item_delegate_manager.cc:40: // When LauncherItem exists,
its LauncherItemDelegate must be existed, too.
On 2013/10/10 14:23:47, Mr4D wrote:
> What about this:
> 
> // Each LauncherItem has to have a LauncherItemDelegate.

Done.

[sky, 2013-10-10 20:26:16]

What specific files do you need me to look at? If I've already
reviewed large parts of this, what versions do I diff between?

On Thu, Oct 10, 2013 at 11:09 AM,  <simon.hong81@gmail.com> wrote:
> Dear sky,
> I need owners check.
> Most of part are not changed previous version.
> Please check again.
>
>
>
>
https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
> File ash/launcher/launcher_item_delegate_manager.cc (right):
>
>
https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
> ash/launcher/launcher_item_delegate_manager.cc:40: // When LauncherItem
> exists, its LauncherItemDelegate must be existed, too.
> On 2013/10/10 14:23:47, Mr4D wrote:
>>
>> What about this:
>
>
>> // Each LauncherItem has to have a LauncherItemDelegate.
>
>
> Done.
>
> https://codereview.chromium.org/25823002/

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

[simonhong_, 2013-10-10 21:55:36]

On 2013/10/10 20:26:16, sky wrote:
> What specific files do you need me to look at? If I've already
> reviewed large parts of this, what versions do I diff between?
> 
> On Thu, Oct 10, 2013 at 11:09 AM,  <mailto:simon.hong81@gmail.com> wrote:
> > Dear sky,
> > I need owners check.
> > Most of part are not changed previous version.
> > Please check again.
> >
> >
> >
> >
>
https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
> > File ash/launcher/launcher_item_delegate_manager.cc (right):
> >
> >
>
https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
> > ash/launcher/launcher_item_delegate_manager.cc:40: // When LauncherItem
> > exists, its LauncherItemDelegate must be existed, too.
> > On 2013/10/10 14:23:47, Mr4D wrote:
> >>
> >> What about this:
> >
> >
> >> // Each LauncherItem has to have a LauncherItemDelegate.
> >
> >
> > Done.
> >
> > https://codereview.chromium.org/25823002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Dear sky,
Patch set #1 is original patch that was reverted and you reviewed.

Following files are modified.
ash/launcher/launcher_unittest.cc
ash/launcher/launcher_item_delegate_manager.cc
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

Please check again.
Thank you.

[simonhong_, 2013-10-10 21:57:10]

On 2013/10/10 21:55:36, Simon YoungKi Hong wrote:
> On 2013/10/10 20:26:16, sky wrote:
> > What specific files do you need me to look at? If I've already
> > reviewed large parts of this, what versions do I diff between?
> > 
> > On Thu, Oct 10, 2013 at 11:09 AM,  <mailto:simon.hong81@gmail.com> wrote:
> > > Dear sky,
> > > I need owners check.
> > > Most of part are not changed previous version.
> > > Please check again.
> > >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
> > > File ash/launcher/launcher_item_delegate_manager.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/25823002/diff/75001/ash/launcher/launcher_ite...
> > > ash/launcher/launcher_item_delegate_manager.cc:40: // When LauncherItem
> > > exists, its LauncherItemDelegate must be existed, too.
> > > On 2013/10/10 14:23:47, Mr4D wrote:
> > >>
> > >> What about this:
> > >
> > >
> > >> // Each LauncherItem has to have a LauncherItemDelegate.
> > >
> > >
> > > Done.
> > >
> > > https://codereview.chromium.org/25823002/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Dear sky,
> Patch set #1 is original patch that was reverted and you reviewed.
> 
> Following files are modified.
> ash/launcher/launcher_unittest.cc
> ash/launcher/launcher_item_delegate_manager.cc
> chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
> 
> Please check again.
> Thank you.

Ah! these files are also modified.
ash/launcher/launcher.cc
ash/launcher/launcher_view.cc

[sky, 2013-10-14 13:24:43]

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

https://codereview.chromium.org/25823002/diff/99001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:796: if (bounds) {
I don't like the NULL checks here. This is error prone. How about making all
call sites pass in non-null?

https://codereview.chromium.org/25823002/diff/99001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1356: const int preferred_size =
shelf->GetPreferredShelfSize();
Why does the Launcher need to care about the shelf?

[simonhong_, 2013-10-14 21:17:31]

Dear sky,
Please check again.

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

https://codereview.chromium.org/25823002/diff/99001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:796: if (bounds) {
On 2013/10/14 13:24:43, sky wrote:
> I don't like the NULL checks here. This is error prone. How about making all
> call sites pass in non-null?

This was changed by another CL because some caller didn't use overflow button's
bounds.
If you don't want this, I'll revert  this in a separate CL.

https://codereview.chromium.org/25823002/diff/99001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1356: const int preferred_size =
shelf->GetPreferredShelfSize();
On 2013/10/14 13:24:43, sky wrote:
> Why does the Launcher need to care about the shelf?

IMO, different launcher icon size is used depends on whether
AlternateShelfLayout is used or not.

[sky, 2013-10-15 00:17:58]

LGTM

[commit-bot: I haz the power, 2013-10-15 00:20:16]

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

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

Change committed as 228595