For Touch: remove some menu items for experimentation, adjust sizes.

For Touch: remove some menu items for experimentation, adjust sizes.
Also removed painting keyboard accelerators.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99745

[Emmanuel Saint-loubert-Bié, 2011-09-02 00:23:41]

Hi Scott,

Adding this code makes me cringe. If you can think of a quick way to achieve the
same results with less #ifdef I am all ears. This is likely to change soon again
though.

Thanks,

-- Emmanuel

[sky, 2011-09-02 00:43:04]

I don't know if my suggestion for cleaning up WrenchMenuModel will actually make
it better. You'll have to experiment. Elliot might have other suggestions.

  -Scott

http://codereview.chromium.org/7825024/diff/1/chrome/browser/ui/toolbar/wrenc...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

http://codereview.chromium.org/7825024/diff/1/chrome/browser/ui/toolbar/wrenc...
chrome/browser/ui/toolbar/wrench_menu_model.cc:409: #if !defined(TOUCH_UI)
This code is such a mess of ifdefs that it's probably worth moving Build into
platform specific files.

http://codereview.chromium.org/7825024/diff/1/views/controls/menu/menu_item_v...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7825024/diff/1/views/controls/menu/menu_item_v...
views/controls/menu/menu_item_view.cc:434: #if defined(TOUCH_UI)
How about adding to MenuConfig and option as to whether accelerators are used
and setting it to false for touch.

[Emmanuel Saint-loubert-Bié, 2011-09-02 00:58:40]

The one downside of breaking into several files is that it is harder
to keep items in the same order afterwards.

> How about adding to MenuConfig and option as to whether accelerators are
> used and setting it to false for touch.

Thanks that is a good idea. I will do that.

-- Emmanuel

On Thu, Sep 1, 2011 at 5:43 PM,  <sky@chromium.org> wrote:
> I don't know if my suggestion for cleaning up WrenchMenuModel will actually
> make
> it better. You'll have to experiment. Elliot might have other suggestions.
>
>  -Scott
>
>
>
http://codereview.chromium.org/7825024/diff/1/chrome/browser/ui/toolbar/wrenc...
> File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):
>
>
http://codereview.chromium.org/7825024/diff/1/chrome/browser/ui/toolbar/wrenc...
> chrome/browser/ui/toolbar/wrench_menu_model.cc:409: #if
> !defined(TOUCH_UI)
> This code is such a mess of ifdefs that it's probably worth moving Build
> into platform specific files.
>
>
http://codereview.chromium.org/7825024/diff/1/views/controls/menu/menu_item_v...
> File views/controls/menu/menu_item_view.cc (right):
>
>
http://codereview.chromium.org/7825024/diff/1/views/controls/menu/menu_item_v...
> views/controls/menu/menu_item_view.cc:434: #if defined(TOUCH_UI)
> How about adding to MenuConfig and option as to whether accelerators are
> used and setting it to false for touch.
>
> http://codereview.chromium.org/7825024/
>

[sky, 2011-09-02 03:37:37]

On Thu, Sep 1, 2011 at 5:58 PM, Emmanuel Saint-loubert
<saintlou@chromium.org> wrote:
> The one downside of breaking into several files is that it is harder
> to keep items in the same order afterwards.

The flipside is easier to read code.

  -Scott

>> How about adding to MenuConfig and option as to whether accelerators are
>> used and setting it to false for touch.
>
> Thanks that is a good idea. I will do that.
>
> -- Emmanuel
>
> On Thu, Sep 1, 2011 at 5:43 PM,  <sky@chromium.org> wrote:
>> I don't know if my suggestion for cleaning up WrenchMenuModel will actually
>> make
>> it better. You'll have to experiment. Elliot might have other suggestions.
>>
>>  -Scott
>>
>>
>>
http://codereview.chromium.org/7825024/diff/1/chrome/browser/ui/toolbar/wrenc...
>> File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):
>>
>>
http://codereview.chromium.org/7825024/diff/1/chrome/browser/ui/toolbar/wrenc...
>> chrome/browser/ui/toolbar/wrench_menu_model.cc:409: #if
>> !defined(TOUCH_UI)
>> This code is such a mess of ifdefs that it's probably worth moving Build
>> into platform specific files.
>>
>>
http://codereview.chromium.org/7825024/diff/1/views/controls/menu/menu_item_v...
>> File views/controls/menu/menu_item_view.cc (right):
>>
>>
http://codereview.chromium.org/7825024/diff/1/views/controls/menu/menu_item_v...
>> views/controls/menu/menu_item_view.cc:434: #if defined(TOUCH_UI)
>> How about adding to MenuConfig and option as to whether accelerators are
>> used and setting it to false for touch.
>>
>> http://codereview.chromium.org/7825024/
>>
>

[Emmanuel Saint-loubert-Bié, 2011-09-02 22:19:54]

Hi Scott,

For the menu construction separating the Browser and CrOS menus make things a
lot clearer. 
I do not think we should fork the entire file, because there is too much common
code. Subclassing in a separate file and overriding the Build method was
something I tried too, but was a bit awkward. 
Beside what I submitted another existing pattern would be to follow the
menu_item approach where a single class is implementing across 2 files
(menu_item_view.cc + the _linux.cc or _win.cc). In this case the method would
revert to being called Build.

-- Emmanuel

[sky, 2011-09-02 22:28:41]

I like having a wrench_menu_model_chromeos that has the Build method
and the build in wrench_menu_model ifdefd for !chromeos.

  -Scott

On Fri, Sep 2, 2011 at 3:19 PM,  <saintlou@chromium.org> wrote:
>
> Hi Scott,
>
> For the menu construction separating the Browser and CrOS menus make things
> a
> lot clearer.
> I do not think we should fork the entire file, because there is too much
> common
> code. Subclassing in a separate file and overriding the Build method was
> something I tried too, but was a bit awkward.
> Beside what I submitted another existing pattern would be to follow the
> menu_item approach where a single class is implementing across 2 files
> (menu_item_view.cc + the _linux.cc or _win.cc). In this case the method
> would
> revert to being called Build.
>
> -- Emmanuel
>
>
> http://codereview.chromium.org/7825024/
>

[Emmanuel Saint-loubert-Bié, 2011-09-02 22:29:19]

Cool, will do!

-- Emmanuel

On Fri, Sep 2, 2011 at 3:28 PM, Scott Violet <sky@chromium.org> wrote:

> I like having a wrench_menu_model_chromeos that has the Build method
> and the build in wrench_menu_model ifdefd for !chromeos.
>
>  -Scott
>
> On Fri, Sep 2, 2011 at 3:19 PM,  <saintlou@chromium.org> wrote:
> >
> > Hi Scott,
> >
> > For the menu construction separating the Browser and CrOS menus make
> things
> > a
> > lot clearer.
> > I do not think we should fork the entire file, because there is too much
> > common
> > code. Subclassing in a separate file and overriding the Build method was
> > something I tried too, but was a bit awkward.
> > Beside what I submitted another existing pattern would be to follow the
> > menu_item approach where a single class is implementing across 2 files
> > (menu_item_view.cc + the _linux.cc or _win.cc). In this case the method
> > would
> > revert to being called Build.
> >
> > -- Emmanuel
> >
> >
> > http://codereview.chromium.org/7825024/
> >
>

[sky, 2011-09-02 22:30:10]

http://codereview.chromium.org/7825024/diff/7003/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7825024/diff/7003/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:90: #ifdef TOUCH_UI
This should be in MenuConfig, which is the central place to specify the config
for a particular platform.

[Emmanuel Saint-loubert-Bié, 2011-09-02 23:04:37]

Made discussed changes

http://codereview.chromium.org/7825024/diff/7003/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7825024/diff/7003/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:90: #ifdef TOUCH_UI
Thanks, I did not know/realize that.

[sky, 2011-09-02 23:12:30]

LGTM with the following addressed.

http://codereview.chromium.org/7825024/diff/11005/chrome/browser/ui/toolbar/w...
File chrome/browser/ui/toolbar/wrench_menu_model_cros.cc (right):

http://codereview.chromium.org/7825024/diff/11005/chrome/browser/ui/toolbar/w...
chrome/browser/ui/toolbar/wrench_menu_model_cros.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
There seem to be more files named with _chromeos than _cros. So, this should be
_chromeos.

http://codereview.chromium.org/7825024/diff/11005/views/controls/menu/menu_co...
File views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/7825024/diff/11005/views/controls/menu/menu_co...
views/controls/menu/menu_config.cc:38: show_accelerators = false;
Set this in menu_config_linux, where there is already a #ifdef TOUCH_UI section.

[Emmanuel Saint-loubert-Bié, 2011-09-02 23:29:01]

Done (and also renamed the other file that lead me the wrong way).

http://codereview.chromium.org/7825024/diff/11005/chrome/browser/ui/toolbar/w...
File chrome/browser/ui/toolbar/wrench_menu_model_cros.cc (right):

http://codereview.chromium.org/7825024/diff/11005/chrome/browser/ui/toolbar/w...
chrome/browser/ui/toolbar/wrench_menu_model_cros.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Yes I was inspired by a file _cros.cc e.g
chrome/browser/ui/webui/fileicon_source_cros.cc
and you are right there are more with _chromeos

http://codereview.chromium.org/7825024/diff/11005/views/controls/menu/menu_co...
File views/controls/menu/menu_config.cc (right):

http://codereview.chromium.org/7825024/diff/11005/views/controls/menu/menu_co...
views/controls/menu/menu_config.cc:38: show_accelerators = false;
Yes that is better