Fix key navigation to cut/copy/paste zoom button on wrench menu

Fix key navigation to cut/copy/paste zoom button on wrench menu
* Fix button class detection (added CustomButton::AsCustomButtton)
* Removed obsolete touch specific color scheme. Instead, use
 NativeTheme as that's what MenuItemView is using.

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

[oshima, 2013-11-25 21:59:01]

https://codereview.chromium.org/82113004/diff/160001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/82113004/diff/160001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:308: canvas->FillRect(bounds,
BackgroundColor(view, state));
I wonder if this is correct/intentional. MenuItemview doesn't seem to be drawing
the background for non hover state.

[oshima, 2013-11-25 22:38:41]

uploaded new patch set that fixes the fullscreen image for hover state.

[sky, 2013-11-26 00:01:42]

Since this code is easy to break and not realize it, can you write a unit test
that verifies the cut/copy/paste buttons (and any others) are actually
CustomButtons?

[oshima, 2013-11-26 00:29:53]

Added CustomButtonTest::AsCustomButton. PTAL.

[sky, 2013-11-26 03:49:30]

I was thinking more of tests for the cut/copy/paste buttons created by
WrenchMenu. Is it possible to write a test that verifies AsCustomButton works
for them?

[oshima, 2013-11-26 05:16:44]

On 2013/11/26 03:49:30, sky wrote:
> I was thinking more of tests for the cut/copy/paste buttons created by
> WrenchMenu. Is it possible to write a test that verifies AsCustomButton works
> for them?

I'd just add DCHECK(CustomButton::AsCustomButton(cut));
I think that's better than unit test. PTAL.

[sky, 2013-12-02 14:28:01]

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:205: if (use_new_menu_)
This shouldn't be necessary sine handled on 199, right?

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:283: NOTREACHED();
Why the NOTREACHED and then a valid lookup?

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:427:
DCHECK(CustomButton::AsCustomButton(button));
Add a comment as to why this is needed.

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:513: 
nit: remove newline.

https://codereview.chromium.org/82113004/diff/300001/ui/views/controls/button...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/82113004/diff/300001/ui/views/controls/button...
ui/views/controls/button/custom_button.cc:39: const char* classname =
view->GetClassName();
check null?

[oshima, 2013-12-02 20:00:58]

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:205: if (use_new_menu_)
On 2013/12/02 14:28:01, sky wrote:
> This shouldn't be necessary sine handled on 199, right?

oops, thank you for the catch. done.

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:283: NOTREACHED();
On 2013/12/02 14:28:01, sky wrote:
> Why the NOTREACHED and then a valid lookup?

I have to return something, otherwise it won't compile,
and I simply used what's been done before. I can change to fixed color (such as
black or white, red). Please let me know which way you want.

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:427:
DCHECK(CustomButton::AsCustomButton(button));
On 2013/12/02 14:28:01, sky wrote:
> Add a comment as to why this is needed.

Done.

https://codereview.chromium.org/82113004/diff/300001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:513: 
On 2013/12/02 14:28:01, sky wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/82113004/diff/300001/ui/views/controls/button...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/82113004/diff/300001/ui/views/controls/button...
ui/views/controls/button/custom_button.cc:39: const char* classname =
view->GetClassName();
On 2013/12/02 14:28:01, sky wrote:
> check null?

I believe the expectation is this will never return NULL.
(views::GetClassName() returns "View").
I checked all other use of GetClassName() and they assume the same.

https://code.google.com/p/chromium/codesearch#search/&q=%22%3EGetClassName%22...

[sky, 2013-12-02 22:10:35]

LGTM

[commit-bot: I haz the power, 2013-12-02 22:13:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/82113004/320001

[commit-bot: I haz the power, 2013-12-03 00:29:44]

Change committed as 238241