The "Update Chrome" menu item should appear in addition to the About menu.

The "Update Chrome" menu item should appear in addition to the About menu.

It should not replace it. This patch modifications to the GTK and Cocoa ports to make the update chrome item appear when an update is available. On win/chromeos, the menu item is always there but disabled, since I'm having some problems figuring out the views custom menu implementation.

BUG=46221
TEST=The upgrade item should now appear under instead of replacing the about command.

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

[sky, 2010-08-27 22:38:24]

LGTM.
Let me know when you land and I'll fix windows.

  -Scott

[Evan Stade, 2010-08-27 23:16:11]

http://codereview.chromium.org/3143046/diff/2001/3001
File app/menus/menu_model.h (right):

http://codereview.chromium.org/3143046/diff/2001/3001#newcode98
app/menus/menu_model.h:98: virtual bool IsVisibleAt(int index) const = 0;
why don't you put the default here? This class is already not pure virtual

http://codereview.chromium.org/3143046/diff/2001/3003
File app/menus/simple_menu_model.h (right):

http://codereview.chromium.org/3143046/diff/2001/3003#newcode29
app/menus/simple_menu_model.h:29: virtual bool IsCommandIdVisible(int
command_id) const { return true; }
implementation in cc? (if for no other reason than to match the rest of the
class)

http://codereview.chromium.org/3143046/diff/2001/3007
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3143046/diff/2001/3007#newcode1106
chrome/browser/browser.cc:1106: IDC_UPGRADE_DIALOG,
how does this get updated when the upgrade detector notices an upgrade?

http://codereview.chromium.org/3143046/diff/2001/3014
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/3143046/diff/2001/3014#newcode233
chrome/browser/gtk/menu_gtk.cc:233: if (model) {
add note about why this is necessary?

nit: if (model && model->IsVisibleAt)

also, no {} for single line conditionals

[Robert Sesek, 2010-08-29 18:38:42]

http://codereview.chromium.org/3143046/diff/2001/3001
File app/menus/menu_model.h (right):

http://codereview.chromium.org/3143046/diff/2001/3001#newcode98
app/menus/menu_model.h:98: virtual bool IsVisibleAt(int index) const = 0;
On 2010/08/27 23:16:12, Evan Stade wrote:
> why don't you put the default here? This class is already not pure virtual

I'd also prefer this.

http://codereview.chromium.org/3143046/diff/2001/3007
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3143046/diff/2001/3007#newcode1106
chrome/browser/browser.cc:1106: IDC_UPGRADE_DIALOG,
On 2010/08/27 23:16:12, Evan Stade wrote:
> how does this get updated when the upgrade detector notices an upgrade?

It won't.  You need to subscribe to the notification.  Or could this always be
enabled since we only show it when it's available?

http://codereview.chromium.org/3143046/diff/2001/3013
File chrome/browser/cocoa/menu_controller.mm (right):

http://codereview.chromium.org/3143046/diff/2001/3013#newcode147
chrome/browser/cocoa/menu_controller.mm:147: [(id)item setHidden:(visible ?
FALSE : TRUE)];
Make this:
  [(id)item setHidden:model->IsVisibleAt(modelIndex)];

http://codereview.chromium.org/3143046/diff/2001/3016
File chrome/browser/wrench_menu_model.cc (right):

http://codereview.chromium.org/3143046/diff/2001/3016#newcode372
chrome/browser/wrench_menu_model.cc:372: // On Mac, there is no About item
unless it is replaced with the update
Update this comment.

[Elliot Glaysher, 2010-08-30 18:23:42]

TAL.

A question: how do I test this? Now that I'm waiting on the upgrade detector to
send a notification to the browser, how can I manually test that everything
works correctly? The code *should* work, but what can I do to verify?

http://codereview.chromium.org/3143046/diff/2001/3003
File app/menus/simple_menu_model.h (right):

http://codereview.chromium.org/3143046/diff/2001/3003#newcode29
app/menus/simple_menu_model.h:29: virtual bool IsCommandIdVisible(int
command_id) const { return true; }
On 2010/08/27 23:16:12, Evan Stade wrote:
> implementation in cc? (if for no other reason than to match the rest of the
> class)

Thank you for nitting about this. I wish everyone nitted about this. It would
make my current cleanup project easier. :(

http://codereview.chromium.org/3143046/diff/2001/3007
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3143046/diff/2001/3007#newcode1106
chrome/browser/browser.cc:1106: IDC_UPGRADE_DIALOG,
On 2010/08/29 18:38:43, rsesek wrote:
> On 2010/08/27 23:16:12, Evan Stade wrote:
> > how does this get updated when the upgrade detector notices an upgrade?
> 
> It won't.  You need to subscribe to the notification.

Done.

> Or could this always be
> enabled since we only show it when it's available?

I can do that later, but not now. There's going to be a period of time when
cocoa/gtk do proper hiding of the menu item, but views doesn't and relies on the
disabling.

http://codereview.chromium.org/3143046/diff/2001/3014
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/3143046/diff/2001/3014#newcode233
chrome/browser/gtk/menu_gtk.cc:233: if (model) {
On 2010/08/27 23:16:12, Evan Stade wrote:
> add note about why this is necessary?
> also, no {} for single line conditionals

Done.

> nit: if (model && model->IsVisibleAt)

That isn't logically equivalent to what I'm testing. In the case where there is
a model and the resulting isn't visible, show shouldn't be called.

[Evan Stade, 2010-08-30 18:52:18]

On 2010/08/30 18:23:42, Elliot Glaysher wrote:
> TAL.
> 
> A question: how do I test this? Now that I'm waiting on the upgrade detector
to
> send a notification to the browser, how can I manually test that everything
> works correctly? The code *should* work, but what can I do to verify?

what I have done, clunky as it may be, is:

1) change the frequency that the version is checked
2) run chrome
3) edit common/chrome_version_info_posix.h.version so that the version is newer
than real version (so that when chrome checks it, it will 
think the version is new)
4) recompile

It would probably be nice to make it just accept a command line flag though (of
course, it is important to test for the case where the upgrade has already been
detected as well as the case where it gets detected and the notification is
sent).

 
> That isn't logically equivalent to what I'm testing. In the case where there
is
> a model and the resulting isn't visible, show shouldn't be called.

oh right. my bad.

if (!(model && !model->IsVisibleAt()))
  show()

that doesn't look too elegant either :(

[Robert Sesek, 2010-08-30 20:36:23]

LGTM

[Elliot Glaysher, 2010-08-30 23:33:31]

Evan, TAL?

Testing showed that listening to the notification wasn't working. I'm going to
take rsesek's suggestion to default to enabled.

[Evan Stade, 2010-08-31 02:03:58]

lgtm

http://codereview.chromium.org/3143046/diff/9002/15002
File app/menus/menu_model.h (right):

http://codereview.chromium.org/3143046/diff/9002/15002#newcode97
app/menus/menu_model.h:97: // Returns true the menu item is visible.
grammar

http://codereview.chromium.org/3143046/diff/9002/15007
File chrome/browser/back_forward_menu_model.h (right):

http://codereview.chromium.org/3143046/diff/9002/15007#newcode59
chrome/browser/back_forward_menu_model.h:59: virtual bool IsVisibleAt(int)
const;
no longer needed