Fix an inconsistent behavior in BrowserCommandController

Fix an inconsistent behavior in BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change fixes the issue, and adds a test case to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=694331
Review-Url: https://codereview.chromium.org/2701973002
Cr-Commit-Position: refs/heads/master@{#452703}
Committed: https://chromium.googlesource.com/chromium/src/+/db473d55d6bc0c1caca424616a788ca0c19a4d69

[Hzj_jie, 2017-02-18 00:36:21]

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

[commit-bot: I haz the power, 2017-02-18 00:37:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 02:39:47]

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

[commit-bot: I haz the power, 2017-02-18 02:39:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[msw, 2017-02-20 01:01:12]

Some minor comments, and I look forward to seeing the fix.

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller_unittest.cc:451:
TEST_F(BrowserCommandControllerTest, DISABLED_OptionsConsistency) {
File a bug, add a comment here that cites the bug #, and use that same # for the
CL's BUG=

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller_unittest.cc:452: TestingProfile*
testprofile = browser()->profile()->AsTestingProfile();
nit: |profile| or |test_profile| to fit naming conventions

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller_unittest.cc:471:
EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
So, this check fails if you enabled the test right now? Maybe enable the test,
but leave this line commented out or EXPECT_FALSE with a comment.

[Hzj_jie, 2017-02-20 20:39:37]

Description was changed from

==========
Add a test case to indicate the inconsistent behavior in
BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change adds a DISABLED_ test cases to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=
==========

to

==========
Add a test case to indicate the inconsistent behavior in
BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change adds a DISABLED_ test cases to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=694331
==========

[Hzj_jie, 2017-02-20 20:47:40]

Description was changed from

==========
Add a test case to indicate the inconsistent behavior in
BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change adds a DISABLED_ test cases to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=694331
==========

to

==========
Add a test case to indicate the inconsistent behavior in
BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change adds a test case to indicate this inconsistent behavior, and make it
more "visible".

R=msw

BUG=694331
==========

[Hzj_jie, 2017-02-20 20:47:55]

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

[commit-bot: I haz the power, 2017-02-20 20:48:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-02-20 21:15:40]

Thank you for the review, Mike.
Do you prefer to include the fix also in this change or in a separated change?

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller_unittest.cc:451:
TEST_F(BrowserCommandControllerTest, DISABLED_OptionsConsistency) {
On 2017/02/20 01:01:12, msw wrote:
> File a bug, add a comment here that cites the bug #, and use that same # for
the
> CL's BUG=

Done.

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller_unittest.cc:452: TestingProfile*
testprofile = browser()->profile()->AsTestingProfile();
On 2017/02/20 01:01:12, msw wrote:
> nit: |profile| or |test_profile| to fit naming conventions

Done.

https://codereview.chromium.org/2701973002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller_unittest.cc:471:
EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
On 2017/02/20 01:01:12, msw wrote:
> So, this check fails if you enabled the test right now? Maybe enable the test,
> but leave this line commented out or EXPECT_FALSE with a comment.

Yes, the two EXPECT_TRUEs fail because of the code defect.
Updated.

[commit-bot: I haz the power, 2017-02-20 21:22:17]

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

[commit-bot: I haz the power, 2017-02-20 21:22:18]

Dry run: This issue passed the CQ dry run.

[msw, 2017-02-21 05:15:19]

On 2017/02/20 21:15:40, Hzj_jie wrote:
> Thank you for the review, Mike.
> Do you prefer to include the fix also in this change or in a separated change?

Including the fix in this cl would be great!

[Hzj_jie, 2017-02-21 22:36:32]

On 2017/02/21 05:15:19, msw (vacation feb 21 maybe 22) wrote:
> On 2017/02/20 21:15:40, Hzj_jie wrote:
> > Thank you for the review, Mike.
> > Do you prefer to include the fix also in this change or in a separated
change?
> 
> Including the fix in this cl would be great!

Definitely, I am working on it.

[Hzj_jie, 2017-02-21 22:55:12]

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

[commit-bot: I haz the power, 2017-02-21 22:56:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-02-21 23:12:04]

Description was changed from

==========
Add a test case to indicate the inconsistent behavior in
BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change adds a test case to indicate this inconsistent behavior, and make it
more "visible".

R=msw

BUG=694331
==========

to

==========
Fix an inconsistent behavior in BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change fixes the issue, and adds a test case to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=694331
==========

[commit-bot: I haz the power, 2017-02-22 00:59:07]

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

[commit-bot: I haz the power, 2017-02-22 00:59:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Hzj_jie, 2017-02-22 19:29:39]

Updated.

[msw, 2017-02-23 19:07:28]

lgtm with a nit; thanks!

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:1070:
UpdateCommandsForIncognitoAvailability();
I was slightly worried that this updates the enabled state of additional
commands, beyond IDC_OPTIONS and IDC_IMPORT_SETTINGS (ie. IDC_NEW_WINDOW,
IDC_NEW_INCOGNITO_WINDOW, IDC_SHOW_BOOKMARK_MANAGER, IDC_MANAGE_EXTENSIONS, and
IDC_SHOW_SIGNIN), but since I don't see any other calls in this file to update
those commands with separate (and potentially conflicting) logic, it seems like
this should be okay.

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_unittest.cc:451: // Code defect is
tracked by bug, http://crbug.com/694331.
nit: replace this comment with something like: "Ensure that the logic for
enabling IDC_OPTIONS is consistent, regardless of the order of entering
fullscreen and incognito modes. See crbug.com/694331"

[Hzj_jie, 2017-02-23 21:59:53]

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

[Hzj_jie, 2017-02-23 22:00:09]

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:1070:
UpdateCommandsForIncognitoAvailability();
On 2017/02/23 19:07:28, msw wrote:
> I was slightly worried that this updates the enabled state of additional
> commands, beyond IDC_OPTIONS and IDC_IMPORT_SETTINGS (ie. IDC_NEW_WINDOW,
> IDC_NEW_INCOGNITO_WINDOW, IDC_SHOW_BOOKMARK_MANAGER, IDC_MANAGE_EXTENSIONS,
and
> IDC_SHOW_SIGNIN), but since I don't see any other calls in this file to update
> those commands with separate (and potentially conflicting) logic, it seems
like
> this should be okay.

I think a simple rule to avoid the conflict is to ensure every command is set in
exactly one code path. Say, IDC_OPTIONS is set only in
UpdateSharedCommandsForIncognitoAvailability() and
UpdateCommandsForIncognitoAvailability(). So no matter how many times one
command is set, for one state, we always ensure the consistency.

But before this change, the rule has been broken, IDC_OPTIONS is set in both
UpdateCommandsForIncognitoAvailability() code path and
UpdateCommandsForFullscreenMode() code path.

Should we add this into the comment of this file?

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller_unittest.cc:451: // Code defect is
tracked by bug, http://crbug.com/694331.
On 2017/02/23 19:07:28, msw wrote:
> nit: replace this comment with something like: "Ensure that the logic for
> enabling IDC_OPTIONS is consistent, regardless of the order of entering
> fullscreen and incognito modes. See crbug.com/694331"

Done.

[commit-bot: I haz the power, 2017-02-23 22:00:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-02-23 22:19:43]

still lgtm

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2701973002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:1070:
UpdateCommandsForIncognitoAvailability();
On 2017/02/23 22:00:09, Hzj_jie wrote:
> On 2017/02/23 19:07:28, msw wrote:
> > I was slightly worried that this updates the enabled state of additional
> > commands, beyond IDC_OPTIONS and IDC_IMPORT_SETTINGS (ie. IDC_NEW_WINDOW,
> > IDC_NEW_INCOGNITO_WINDOW, IDC_SHOW_BOOKMARK_MANAGER, IDC_MANAGE_EXTENSIONS,
> and
> > IDC_SHOW_SIGNIN), but since I don't see any other calls in this file to
update
> > those commands with separate (and potentially conflicting) logic, it seems
> like
> > this should be okay.
> 
> I think a simple rule to avoid the conflict is to ensure every command is set
in
> exactly one code path. Say, IDC_OPTIONS is set only in
> UpdateSharedCommandsForIncognitoAvailability() and
> UpdateCommandsForIncognitoAvailability(). So no matter how many times one
> command is set, for one state, we always ensure the consistency.
> 
> But before this change, the rule has been broken, IDC_OPTIONS is set in both
> UpdateCommandsForIncognitoAvailability() code path and
> UpdateCommandsForFullscreenMode() code path.
> 
> Should we add this into the comment of this file?

I generally agree, but doubt the effectiveness of a comment. I think it's fine
to land as-is.

[commit-bot: I haz the power, 2017-02-23 22:58:58]

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

[commit-bot: I haz the power, 2017-02-23 22:58:59]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-02-24 01:06:42]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2017-02-24 01:07:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 01:13:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487898402110720,
"parent_rev": "fc4476007d75bde5fa57c50efd4300d36dd33885", "commit_rev":
"db473d55d6bc0c1caca424616a788ca0c19a4d69"}

[commit-bot: I haz the power, 2017-02-24 01:15:12]

Description was changed from

==========
Fix an inconsistent behavior in BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change fixes the issue, and adds a test case to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=694331
==========

to

==========
Fix an inconsistent behavior in BrowserCommandController

An inconsistent behavior has been detected in BrowserCommandController, which
disables IDC_OPTIONS when entering and exiting fullscreen mode unexpectedly.
This change fixes the issue, and adds a test case to indicate this inconsistent
behavior, and make it more "visible".

R=msw

BUG=694331

Review-Url: https://codereview.chromium.org/2701973002
Cr-Commit-Position: refs/heads/master@{#452703}
Committed:
https://chromium.googlesource.com/chromium/src/+/db473d55d6bc0c1caca424616a78...
==========

[commit-bot: I haz the power, 2017-02-24 01:15:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/db473d55d6bc0c1caca424616a78...