Fixed logic for display of upgrade menu item.

Fixed logic for display of upgrade menu item.

Moves all logic for deciding the availability of the upgrade menu item into the visibility check. Also extends a test to prevent future regressions.

BUG=475971
Committed: https://crrev.com/44babbed5ac4beea3f2244b00d8ddbc359c1d13d
Cr-Commit-Position: refs/heads/master@{#327079}

[Albert Bodenhamer, 2015-04-23 20:36:20]

The CQ bit was checked by abodenha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 20:36:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061663003/1

[commit-bot: I haz the power, 2015-04-23 21:38:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 21:38:02]

Dry run: This issue passed the CQ dry run.

[Albert Bodenhamer, 2015-04-23 21:39:02]

abodenha@chromium.org changed reviewers:
+ msw@chromium.org

[Albert Bodenhamer, 2015-04-23 21:42:20]

abodenha@chromium.org changed reviewers:
+ thestig@chromium.org

[Albert Bodenhamer, 2015-04-23 21:46:30]

PTAL when you get a chance.
+thestig for upgrade_detector.h
+msw for everything else

I'd appreciate feedback on the structure for the test. I'm not fond of creating
that friend relationship, but at the same time creating and injecting a mock for
this seems like overkill.

[msw, 2015-04-23 22:06:43]

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:133: if
(browser_defaults::kShowUpgradeMenuItem)
nit: EXPECT_EQ(browser_defaults::kShowUpgradeMenuItem,
model.IsCommandIdVisible(IDC_UPGRADE_DIALOG))

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
chrome/browser/upgrade_detector.h:135:
FRIEND_TEST_ALL_PREFIXES(WrenchMenuModelTest, Basics);
Why does this need to be a friend test? It looks like it just uses public API.

[Lei Zhang, 2015-04-23 22:07:45]

chrome/browser/upgrade_detector.h lgtm

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:133: if
(browser_defaults::kShowUpgradeMenuItem)
Maybe reduce the if/else to:

EXPECT_EQ(browser_defaults::kShowUpgradeMenuItem,
          model.IsCommandIdVisible(IDC_UPGRADE_DIALOG));

[Lei Zhang, 2015-04-23 22:08:22]

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
chrome/browser/upgrade_detector.h:135:
FRIEND_TEST_ALL_PREFIXES(WrenchMenuModelTest, Basics);
On 2015/04/23 22:06:42, msw wrote:
> Why does this need to be a friend test? It looks like it just uses public API.

The methods are protected, not public.

[msw, 2015-04-23 22:11:28]

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
chrome/browser/upgrade_detector.h:135:
FRIEND_TEST_ALL_PREFIXES(WrenchMenuModelTest, Basics);
On 2015/04/23 22:08:22, Lei Zhang wrote:
> On 2015/04/23 22:06:42, msw wrote:
> > Why does this need to be a friend test? It looks like it just uses public
API.
> 
> The methods are protected, not public.

Ah, missed that, sorry!

[Albert Bodenhamer, 2015-04-23 22:26:48]

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:133: if
(browser_defaults::kShowUpgradeMenuItem)
On 2015/04/23 22:06:41, msw wrote:
> nit: EXPECT_EQ(browser_defaults::kShowUpgradeMenuItem,
> model.IsCommandIdVisible(IDC_UPGRADE_DIALOG))

Done.

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/ui/toolbar/w...
chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:133: if
(browser_defaults::kShowUpgradeMenuItem)
On 2015/04/23 22:06:41, msw wrote:
> nit: EXPECT_EQ(browser_defaults::kShowUpgradeMenuItem,
> model.IsCommandIdVisible(IDC_UPGRADE_DIALOG))

Done.

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/1061663003/diff/1/chrome/browser/upgrade_dete...
chrome/browser/upgrade_detector.h:135:
FRIEND_TEST_ALL_PREFIXES(WrenchMenuModelTest, Basics);
On 2015/04/23 22:11:28, msw wrote:
> On 2015/04/23 22:08:22, Lei Zhang wrote:
> > On 2015/04/23 22:06:42, msw wrote:
> > > Why does this need to be a friend test? It looks like it just uses public
> API.
> > 
> > The methods are protected, not public.
> 
> Ah, missed that, sorry!

Yep. Protected. There were a bunch of ways to go here. This seemed simplest, but
I'm never a fan of friend.

[msw, 2015-04-23 22:57:25]

lgtm

[Albert Bodenhamer, 2015-04-27 16:56:01]

The CQ bit was checked by abodenha@chromium.org

[Albert Bodenhamer, 2015-04-27 16:56:01]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1061663003/#ps20001
(title: "Fix nit.")

[commit-bot: I haz the power, 2015-04-27 16:56:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061663003/20001

[commit-bot: I haz the power, 2015-04-27 18:31:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-27 18:31:49]

Patchset 2 (id:??) landed as
https://crrev.com/44babbed5ac4beea3f2244b00d8ddbc359c1d13d
Cr-Commit-Position: refs/heads/master@{#327079}