Finalizing the 'OnClick' behavior of the Launcher items.

Finalizing the 'OnClick' behavior of the Launcher items.


BUG=173233
TEST=visual


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

[Mr4D (OOO till 08-26), 2013-01-31 19:48:44]

As discussed - I added the system color and a color override since the item
itself is not select-able (since it is neither a checkbox nor a radiobutton).

Please have a look!

[sky, 2013-01-31 21:15:02]

https://codereview.chromium.org/12096094/diff/2002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/12096094/diff/2002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:1212: if (event.IsShiftDown())
This code is fragile (I know I've added some too), can you create a scoping
object whose constructor turns it on and destructor unsets.

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_delegate.h (right):

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_delegate.h:79: SkColor* override_color) const;
This shouldn't take is_hovered, instead it should only be invoked if the item
isn't selected. That means you can get rid of colors you're adding to native
theme.

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_item_view.cc:784: canvas->DrawColor(override_color,
SkXfermode::kSrc_Mode);
I don't think you need kSrc_Mode here, meaning DrawColor(override_color) should
be enough.

[Mr4D (OOO till 08-26), 2013-01-31 22:38:52]

Addressed - please have another look!

https://codereview.chromium.org/12096094/diff/2002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/12096094/diff/2002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:1212: if (event.IsShiftDown())
On 2013/01/31 21:15:02, sky wrote:
> This code is fragile (I know I've added some too), can you create a scoping
> object whose constructor turns it on and destructor unsets.

Done.

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_delegate.h (right):

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_delegate.h:79: SkColor* override_color) const;
Actually... The background colors are different from the normal menu colors for
the hovered AND the non hovered state. So we need both separately.

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_item_view.cc:784: canvas->DrawColor(override_color,
SkXfermode::kSrc_Mode);
Did it as it was done in line 794 - but done as requested.

[sky, 2013-02-01 00:17:33]

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_delegate.h (right):

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_delegate.h:79: SkColor* override_color) const;
On 2013/01/31 22:38:52, Mr4D wrote:
> Actually... The background colors are different from the normal menu colors
for
> the hovered AND the non hovered state. So we need both separately.

Ugh.
Ok, in that case the names you've gone with are inconsistant and confusing here.
is_hovered->selected
kColorId_SelectedMenuItemBackground->HightedMenuItemBackground
kColorId_FocusedSelectedMenuItemBackground->SelectedHighlightMenuItemBackground

And be sure and add a description above these in native_theme since it is not
obvious.

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

https://codereview.chromium.org/12096094/diff/18001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:77: class SlowAnimations {
ScopedAnimationSetter -> and this should be promoted some where else, but that
can happen later.

https://codereview.chromium.org/12096094/diff/18001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/12096094/diff/18001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_item_view.cc:782: IsSelected(),
IsSelected()->render_selection.

[Mr4D (OOO till 08-26), 2013-02-01 15:48:28]

Addressed! Please have another look!

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_delegate.h (right):

https://codereview.chromium.org/12096094/diff/2002/ui/views/controls/menu/men...
ui/views/controls/menu/menu_delegate.h:79: SkColor* override_color) const;
As discussed: The names are correct. The function GetBackgroundColor called from
MenuViewItem is "universally usable" and gets the state of the mouse hover. The
implementing function (in my case the running applications list) has a very
special sate: The "Application is active" state - which does not translate into
any other menu state. However, it can also be "highlighted" via hover.

But since this is so confusing - and there is currently only one incident where
this is used, I remove the system colors and we can re-introduce them with a
better name if needed.
Done.

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

https://codereview.chromium.org/12096094/diff/18001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:77: class SlowAnimations {
On 2013/02/01 00:17:33, sky wrote:
> ScopedAnimationSetter -> and this should be promoted some where else, but that
> can happen later.

Done.

https://codereview.chromium.org/12096094/diff/18001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_item_view.cc (right):

https://codereview.chromium.org/12096094/diff/18001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_item_view.cc:782: IsSelected(),
On 2013/02/01 00:17:33, sky wrote:
> IsSelected()->render_selection.

Done.

[sky, 2013-02-01 17:04:09]

LGTM

[commit-bot: I haz the power, 2013-02-01 17:54:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/12096094/23001

[commit-bot: I haz the power, 2013-02-01 17:58:20]

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=android_cl...

[commit-bot: I haz the power, 2013-02-01 18:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/12096094/23001

[commit-bot: I haz the power, 2013-02-01 23:51:33]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-02 00:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/12096094/23001

[commit-bot: I haz the power, 2013-02-02 04:47:51]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-02 18:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/12096094/23001

[commit-bot: I haz the power, 2013-02-04 15:13:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/12096094/23001

[commit-bot: I haz the power, 2013-02-04 16:26:49]

Change committed as 180421