Make Mac menu code obey incognito availability.

Make Mac menu code obey incognito availability.

BUG=170053
TEST=as in bug


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

[Avi (use Gerrit), 2013-01-15 22:19:05]

Robert: Mac stuff
Rustem: Are you the correct person to ask for a review for the incognito mode
side?

[Robert Sesek, 2013-01-15 22:27:18]

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/app_control...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/app_control...
chrome/browser/app_controller_mac.mm:744: if (menuState_ &&  // null in tests
.

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller.cc:963: if (!show_main_ui) {
Why this change? You should probably be updating the command state
unconditionally.

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller_base.h (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller_base.h:13: class
BrowserCommandControllerBase {
Why split this up? And it's not really a Base class.

[rustema, 2013-01-15 22:32:41]

The change is looking good to me conceptually. However, I haven't been working
on Chrome for over a year. So I'd probably ask someone from Munich to take a
closer look.

Markus, can you suggest a good candidate to review this CL?

[rustema, 2013-01-15 22:33:39]

lgtm

[Avi (use Gerrit), 2013-01-15 22:35:06]

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/app_control...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/app_control...
chrome/browser/app_controller_mac.mm:744: if (menuState_ &&  // null in tests
On 2013/01/15 22:27:18, rsesek wrote:
> .

Done.

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller.cc:963: if (!show_main_ui) {
On 2013/01/15 22:27:18, rsesek wrote:
> Why this change? You should probably be updating the command state
> unconditionally.

This is not a change.

The old code enabled the commands for show_main_ui && command_enabled (i.e. both
had to be true).
BrowserCommandControllerBase::UpdateCommandsForIncognitoAvailability enables the
commands only for command_enabled. Therefore, to maintain equivalence, we need
to disable them for !show_main_ui.

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller_base.h (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller_base.h:13: class
BrowserCommandControllerBase {
On 2013/01/15 22:27:18, rsesek wrote:
> Why split this up? And it's not really a Base class.

I need to split this because AppController(Mac) needs to have the same logic for
enabling/disabling commands as the BrowserCommandController. So I factored out
that logic.

It is a "Base" class conceptually in that both AppController(Mac) and
BrowserCommandController use it. If you have a different name suggestion I'd be
happy to use it.

[Avi (use Gerrit), 2013-01-15 22:36:29]

Rustem, my apologies then. I saw you were working on a CL in this area
(https://codereview.chromium.org/11471027/) so I thought you would be a good
reviewer.

[Robert Sesek, 2013-01-15 22:44:37]

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller.cc:963: if (!show_main_ui) {
On 2013/01/15 22:35:06, Avi wrote:
> On 2013/01/15 22:27:18, rsesek wrote:
> > Why this change? You should probably be updating the command state
> > unconditionally.
> 
> This is not a change.
> 
> The old code enabled the commands for show_main_ui && command_enabled (i.e.
both
> had to be true).
> BrowserCommandControllerBase::UpdateCommandsForIncognitoAvailability enables
the
> commands only for command_enabled. Therefore, to maintain equivalence, we need
> to disable them for !show_main_ui. 

Ah, got it.

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller_base.h (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller_base.h:13: class
BrowserCommandControllerBase {
On 2013/01/15 22:35:06, Avi wrote:
> On 2013/01/15 22:27:18, rsesek wrote:
> > Why split this up? And it's not really a Base class.
> 
> I need to split this because AppController(Mac) needs to have the same logic
for
> enabling/disabling commands as the BrowserCommandController. So I factored out
> that logic.
> 
> It is a "Base" class conceptually in that both AppController(Mac) and
> BrowserCommandController use it. If you have a different name suggestion I'd
be
> happy to use it.

Why can't they just be static on the main BrowserCommandController?

[Avi (use Gerrit), 2013-01-15 22:49:30]

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_command_controller_base.h (right):

https://codereview.chromium.org/11906008/diff/7001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_command_controller_base.h:13: class
BrowserCommandControllerBase {
It felt awkward to have two random things that AppController poked
BrowserCommandController for. I guess this is just as awkward. I'll switch it
back.

[Avi (use Gerrit), 2013-01-16 03:46:35]

Robert, please take a look.

[Robert Sesek, 2013-01-16 18:24:50]

LGTM

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:528:
UpdateSharedCommandsForIncognitoAvailability(menuState_.get(), lastProfile_);
nit: indent 4

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:534:
UpdateSharedCommandsForIncognitoAvailability,
nit: indent 4

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.h (right):

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.h:76: // only depend on the
profile.
nit: dependS?

[Avi (use Gerrit), 2013-01-16 19:04:37]

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:528:
UpdateSharedCommandsForIncognitoAvailability(menuState_.get(), lastProfile_);
On 2013/01/16 18:24:50, rsesek wrote:
> nit: indent 4

Done.

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:534:
UpdateSharedCommandsForIncognitoAvailability,
On 2013/01/16 18:24:50, rsesek wrote:
> nit: indent 4

Done.

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.h (right):

https://codereview.chromium.org/11906008/diff/21001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.h:76: // only depend on the
profile.
On 2013/01/16 18:24:50, rsesek wrote:
> nit: dependS?

The commands depend on the profile. Awkward constructions ftw.

[Avi (use Gerrit), 2013-01-16 19:05:37]

Markus: thoughts?

[markusheintz_, 2013-01-17 15:13:08]

Sorry for the late response. My email filters did not label this email as
Review. That's way I haven't seen it earlier.

Thanks a lot for fixing this issue Avi!!

LGTM

I only have two questions.

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:619: prefs::kAllowFileSelectionDialogs,
Is this related to this issue? Sounds like it deals with the file selection
dialog?

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.h (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.h:72: // Updates the open-file
state (Mac-only).
If this is for Mac should use IF DEFS ?

[Avi (use Gerrit), 2013-01-17 18:35:11]

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:619: prefs::kAllowFileSelectionDialogs,
On 2013/01/17 15:13:08, markusheintz_ wrote:
> Is this related to this issue? Sounds like it deals with the file selection
> dialog?

If you look at the IDC_* values that menuState_ controls and that
BrowserCommandController turns on/off, you'll see the incog settings, and file
selection dialogs. I figured that while I was in here fixing the fact that the
incog menu items weren't getting updated, I might as well fix up everything else
that's supposed to be getting turned on/off and wasn't.

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.h (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.h:72: // Updates the open-file
state (Mac-only).
On 2013/01/17 15:13:08, markusheintz_ wrote:
> If this is for Mac should use IF DEFS ?

We have IDC_OPEN_FILE on other platforms; looks like a bad (or at least out of
date) comment to me.

[markusheintz_, 2013-01-18 12:54:07]

Still LGTM.

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:619: prefs::kAllowFileSelectionDialogs,
If disabling any other menu entries that should be is not too much work, then
adding it to this CL sounds good.

But I'm also ok with the current state of the CL.

On 2013/01/17 18:35:11, Avi wrote:
> On 2013/01/17 15:13:08, markusheintz_ wrote:
> > Is this related to this issue? Sounds like it deals with the file selection
> > dialog?
> 
> If you look at the IDC_* values that menuState_ controls and that
> BrowserCommandController turns on/off, you'll see the incog settings, and file
> selection dialogs. I figured that while I was in here fixing the fact that the
> incog menu items weren't getting updated, I might as well fix up everything
else
> that's supposed to be getting turned on/off and wasn't.

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.h (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.h:72: // Updates the open-file
state (Mac-only).
I see. Could you update the comment and remove the "(Mac-only)"? Thanks :)

On 2013/01/17 18:35:11, Avi wrote:
> On 2013/01/17 15:13:08, markusheintz_ wrote:
> > If this is for Mac should use IF DEFS ?
> 
> We have IDC_OPEN_FILE on other platforms; looks like a bad (or at least out of
> date) comment to me.

[markusheintz_, 2013-01-18 13:21:05]

cc Joao

[Joao da Silva, 2013-01-18 13:24:50]

Looks good, thanks!

[Avi (use Gerrit), 2013-01-18 19:19:23]

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:619: prefs::kAllowFileSelectionDialogs,
Nope, that's it. All the other menu items are not switched on/off by the
BrowserCommandController.

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_command_controller.h (right):

https://codereview.chromium.org/11906008/diff/29001/chrome/browser/ui/browser...
chrome/browser/ui/browser_command_controller.h:72: // Updates the open-file
state (Mac-only).
That's the plan; I'm OOO now and will do so when I return.

[commit-bot: I haz the power, 2013-01-22 14:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11906008/34001

[commit-bot: I haz the power, 2013-01-22 14:38:10]

Presubmit check for 11906008-34001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui

Presubmit checks took 1.2s to calculate.

[Avi (use Gerrit), 2013-01-22 17:06:35]

Nico, OWNER?

[Nico, 2013-01-22 17:09:52]

lgtm

(no test?)

[commit-bot: I haz the power, 2013-01-22 17:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11906008/34001

[commit-bot: I haz the power, 2013-01-22 21:09:12]

Change committed as 178093