Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds a BrowserWindow::IsToolbarShowing() function to indicate
whether the toolbar is showing up on the screen. BrowserWindowCocoa needs to
override this function, which has not been done yet in this change.

BUG=680809, 702251
Review-Url: https://codereview.chromium.org/2781633003
Cr-Commit-Position: refs/heads/master@{#462942}
Committed: https://chromium.googlesource.com/chromium/src/+/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b

[Hzj_jie, 2017-03-27 22:24:43]

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

[commit-bot: I haz the power, 2017-03-27 22:25:33]

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-03-27 22:25:45]

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

[commit-bot: I haz the power, 2017-03-27 22:26:09]

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-03-27 22:26:32]

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

[commit-bot: I haz the power, 2017-03-27 22:26:39]

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-03-27 22:26:46]

Patchset #2 (id:20001) has been deleted

[Hzj_jie, 2017-03-27 22:26:57]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-27 22:47:18]

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

[commit-bot: I haz the power, 2017-03-27 22:47:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hzj_jie, 2017-03-27 23:03:22]

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

[Hzj_jie, 2017-03-27 23:03:31]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-27 23:04:11]

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-03-27 23:20:50]

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

[commit-bot: I haz the power, 2017-03-27 23:21:05]

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-03-27 23:40:04]

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

[commit-bot: I haz the power, 2017-03-27 23:40:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hzj_jie, 2017-03-27 23:41:15]

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

[Hzj_jie, 2017-03-27 23:41:34]

Patchset #2 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-27 23:41:43]

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-03-27 23:41:44]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-28 00:00:13]

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

[commit-bot: I haz the power, 2017-03-28 00:00:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hzj_jie, 2017-03-28 00:39:40]

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

[Hzj_jie, 2017-03-28 00:40:10]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-28 00:40:13]

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-03-28 01:20:41]

zijiehe@chromium.org changed reviewers:
+ dominickn@chromium.org, msw@chromium.org

[Hzj_jie, 2017-03-28 01:38:11]

Description was changed from

==========
Redo browser keyboard lock

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms, so the behavior keeps consistent.

BUG=680809, 702251
==========

to

==========
Redo browser keyboard lock

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

BUG=680809, 702251
==========

[commit-bot: I haz the power, 2017-03-28 01:39:50]

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

[commit-bot: I haz the power, 2017-03-28 01:39:50]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-03-28 02:01:39]

Can you please change the title to something more descriptive, e.g. Allow
browser shortcuts to be captured in chrome-less fullscreen on desktop.

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:201: if
(window()->IsToolbarVisible()) {
I'm not sure this works correctly on on Mac, where the toolbar slides in and out
without actually changing the value of IsToolbarVisible().

 * BrowserWindowCocoa::IsToolbarVisible() calls
Browser::SupportsWindowFeature(Browser::FEATURE_TOOLBAR)

 * Browser::SupportsWindowFeature() calls
Browser::SupportsWindowFeatureImpl(TOOLBAR, true);

 * Browser::SupportsWindowFeatureImpl() looks like it will always return
FEATURE_TOOLBAR, since BrowserWindowCocoa::ShouldHideUIForFullscreen() always
returns false (and you're almost always in a tabbed browser since that's
constant).

Have you tested this on Mac? If it works, then great (and my understanding of
the call sequence must be wrong). Otherwise, I think you need to add a new
method on BrowserWindowCocoa that the FullscreenToolbarController (possibly
calling mustShowFullscreenToolbar).

We should also add explicit tests on Mac, possibly based on existing fullscreen
toolbar tests there.

[Hzj_jie, 2017-03-28 21:23:12]

Description was changed from

==========
Redo browser keyboard lock

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

BUG=680809, 702251
==========

to

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

BUG=680809, 702251
==========

[Hzj_jie, 2017-03-29 00:47:54]

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:201: if
(window()->IsToolbarVisible()) {
On 2017/03/28 02:01:38, dominickn wrote:
> I'm not sure this works correctly on on Mac, where the toolbar slides in and
out
> without actually changing the value of IsToolbarVisible().
> 
>  * BrowserWindowCocoa::IsToolbarVisible() calls
> Browser::SupportsWindowFeature(Browser::FEATURE_TOOLBAR)
> 
>  * Browser::SupportsWindowFeature() calls
> Browser::SupportsWindowFeatureImpl(TOOLBAR, true);
> 
>  * Browser::SupportsWindowFeatureImpl() looks like it will always return
> FEATURE_TOOLBAR, since BrowserWindowCocoa::ShouldHideUIForFullscreen() always
> returns false (and you're almost always in a tabbed browser since that's
> constant).
> 
> Have you tested this on Mac? If it works, then great (and my understanding of
> the call sequence must be wrong). Otherwise, I think you need to add a new
> method on BrowserWindowCocoa that the FullscreenToolbarController (possibly
> calling mustShowFullscreenToolbar).
> 
> We should also add explicit tests on Mac, possibly based on existing
fullscreen
> toolbar tests there.

Thank you Dominick. Unfortunately, I do not even have a Mac. I am blindly
changing the Mac specific logic.
But yes, you are correct. The name of the function is a little bit confusing. It
really means CanToolbarBeVisible(). Even in BrowserView implementation, it does
not indicate the visibility of the toolbar.

I will need more time to work on this change.

[Hzj_jie, 2017-03-29 00:48:21]

zijiehe@chromium.org changed reviewers:
- dominickn@chromium.org, msw@chromium.org

[Hzj_jie, 2017-03-30 21:17:59]

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

[commit-bot: I haz the power, 2017-03-30 21:19:23]

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-03-30 22:12:49]

Description was changed from

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

BUG=680809, 702251
==========

to

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds platform independent changes to achieve. On Mac OS, we need
extra work to return the state of toolbar.

BUG=680809, 702251
==========

[Hzj_jie, 2017-03-30 22:13:06]

Description was changed from

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds platform independent changes to achieve. On Mac OS, we need
extra work to return the state of toolbar.

BUG=680809, 702251
==========

to

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds platform independent changes to achieve. On Mac OS, we
need extra work to return the state of toolbar.

BUG=680809, 702251
==========

[Hzj_jie, 2017-03-30 22:13:24]

Description was changed from

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds platform independent changes to achieve. On Mac OS, we
need extra work to return the state of toolbar.

BUG=680809, 702251
==========

to

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds platform independent changes to achieve. On Mac OS, we need
extra work to return the state of toolbar.

BUG=680809, 702251
==========

[commit-bot: I haz the power, 2017-03-30 22:31:47]

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

[commit-bot: I haz the power, 2017-03-30 22:31:49]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-03-30 23:59:11]

Description was changed from

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds platform independent changes to achieve. On Mac OS, we need
extra work to return the state of toolbar.

BUG=680809, 702251
==========

to

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds a BrowserWindow::IsToolbarShowing() function to indicate
whether the toolbar is showing up on the screen. BrowserWindowCocoa needs to
override this function, which has not been done yet in this change.

BUG=680809, 702251
==========

[Hzj_jie, 2017-03-30 23:59:11]

zijiehe@chromium.org changed reviewers:
+ dominickn@chromium.org, msw@chromium.org

[Hzj_jie, 2017-03-31 00:00:00]

A new API has been added to BrowserWindow. Please kindly include the right
person to review the change.

Thank you.

[Hzj_jie, 2017-04-03 22:06:14]

Hi, Mike and Dominick,
Do you have time to have a look at this change?

[dominickn, 2017-04-04 05:48:18]

Seems fine to me, but I'll defer to msw as I'm a non-OWNER here.

[dominickn, 2017-04-04 05:50:13]

On 2017/04/04 05:48:18, dominickn wrote:
> Seems fine to me, but I'll defer to msw as I'm a non-OWNER here.

I'll note that the description and subject should now be something like:

"Expose whether or not the toolbar is visible to the user on BrowserWindow"

[Hzj_jie, 2017-04-04 18:29:21]

On 2017/04/04 05:50:13, dominickn wrote:
> On 2017/04/04 05:48:18, dominickn wrote:
> > Seems fine to me, but I'll defer to msw as I'm a non-OWNER here.
> 
> I'll note that the description and subject should now be something like:
> 
> "Expose whether or not the toolbar is visible to the user on BrowserWindow"

Do you mean this statement?
"This change adds a BrowserWindow::IsToolbarShowing() function to indicate
whether the toolbar is showing up on the screen."

[msw, 2017-04-04 19:32:48]

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:194: // This behavior is
different on Mac OS, which has a unique user-initiated
nit: move this comment into the OS_MACOSX block, consider revising it to
something like:
"On Mac, allow the browser to handle commands when the toolbar is visible. See
crbug.com/702251"

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:202: if (command_id ==
IDC_FULLSCREEN)
Why not handle IDC_EXIT here?

https://codereview.chromium.org/2781633003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/2781633003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:330: // TODO(zijiehe): Retrieve
the visibility of toolbar from
Is this okay to leave as a TODO? It seems like that's why this was reverted
previously? Please seek reviewers for this aspect of the change before seeking
owners review.

[msw, 2017-04-04 19:33:28]

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:194: // This behavior is
different on Mac OS, which has a unique user-initiated
On 2017/04/04 19:32:48, msw wrote:
> nit: move this comment into the OS_MACOSX block, consider revising it to
> something like:
> "On Mac, allow the browser to handle commands when the toolbar is visible. See
> crbug.com/702251"

Ignore this, sorry.

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:202: if (command_id ==
IDC_FULLSCREEN)
On 2017/04/04 19:32:48, msw wrote:
> Why not handle IDC_EXIT here?

Ignore this, sorry.

[Hzj_jie, 2017-04-04 20:23:39]

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

[commit-bot: I haz the power, 2017-04-04 20:24:29]

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-04-04 20:54:43]

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:194: // This behavior is
different on Mac OS, which has a unique user-initiated
On 2017/04/04 19:33:27, msw wrote:
> On 2017/04/04 19:32:48, msw wrote:
> > nit: move this comment into the OS_MACOSX block, consider revising it to
> > something like:
> > "On Mac, allow the browser to handle commands when the toolbar is visible.
See
> > crbug.com/702251"
> 
> Ignore this, sorry.

I think this comment is still relevant. I moved the comments regarding to Mac OS
into the #if defined(OS_MACOSX).

https://codereview.chromium.org/2781633003/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:202: if (command_id ==
IDC_FULLSCREEN)
On 2017/04/04 19:33:27, msw wrote:
> On 2017/04/04 19:32:48, msw wrote:
> > Why not handle IDC_EXIT here?
> 
> Ignore this, sorry.

I think this is by-design. The IDC_EXIT is covered by is_exit_fullscreen boolean
value.

https://codereview.chromium.org/2781633003/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/2781633003/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:330: // TODO(zijiehe): Retrieve
the visibility of toolbar from
On 2017/04/04 19:32:48, msw wrote:
> Is this okay to leave as a TODO? It seems like that's why this was reverted
> previously? Please seek reviewers for this aspect of the change before seeking
> owners review.

As long as this function returns true, the behavior should keep unchanged.
I have not received my Mac workstation yet. So unfortunately I cannot implement
it blindly now.

[commit-bot: I haz the power, 2017-04-04 21:16:06]

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

[commit-bot: I haz the power, 2017-04-04 21:16:07]

Dry run: This issue passed the CQ dry run.

[msw, 2017-04-04 22:46:39]

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:193: // https://goo.gl/4tJ32G.
I still don't think that we should use URL shortener links in our codebase; use
the full link, or better yet, link to a crbug.com/# issue.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:199: // the shortcuts will be
reserved once the tab strip / Chrome toolbar is
nit: "the commands should be reserved for browser-side handling if the browser
window's toolbar is visible."

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:283: // Setters
nit: remove this comment, it doesn't help.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:284: void ShowToolbar()
{ toolbar_showing_ = true; }
nit: make this set_toolbar_showing(bool showing) { toolbar_showing_ = showing; }

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:342:
EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
Test IDC_EXIT (and others) here?

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:369:
EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
Test IDC_EXIT (and others) here?

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:372: // shortcuts
should be delivered to the web page. See https://goo.gl/4tJ32G.
Ditto, use the full url, or a crbug.com/# addy.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:373:
EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
nit: it would be nice to reduce the boilerplate for all these checks, to make
the behavior of the tests/expectations more readily apparent. Consider one or
both of these suggestions:
(1) creating command id array(s) with corresponding expectations (ie. const
int[] commands_reserved_in_fullscreen = { IDC..., IDC... }; ), or an array of
structs with bool values for the expected (per-fullscreen?) reserved/enabled
state, which you can then loop over like RenderTextTest:
  struct CommandExpectations { int command_id; bool reserved; bool enabled; };
  CommandExpectations fullscreen_expectations[] = { {IDC..., true, true}, ... };
  for (size_t i = 0; i < arraysize(fullscreen_expectations); ++i) {
    SCOPED_TRACE(base::StringPrintf("Testing i=[%" PRIuS "] '%d'", i,
fullscreen_expectations[i].command_id));
    EXPECT_EQ(fullscreen_expectations[i].enabled,
chrome::IsCommandEnabled(browser(), fullscreen_expectations[i].command_id));
If the enabled state always matches the reserved state, combine those bools
(2) Caching the BrowserCommandController and NativeWebKeyboardEvent like:
  BrowserCommandController* controller = browser()->command_controller();
  content::NativeWebKeyboardEvent event(blink::WebInputEvent::TypeFirst, 0, 0);
  EXPECT_FALSE(controller->IsReservedCommandOrKey(IDC_CLOSE_TAB, event);

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:395: IDC_EXIT,
Test IDC_FULLSCREEN here?

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:398: // Exit
fullscreen.
If this block is just a duplicate of the one at the top, it'd be extra nice to
use command arrays and loops, instead of copy-pasting this whole thing.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:463: // Defaults for a
tabbed browser.
If this is a duplicate of the top of UpdateCommandsForFullscreenMode, it'd be
nice to use the array approach. It'd be even better to merge this test with that
above, with either some extra loops for Mac, or as a parameterized fixture
TEST_P() with a bool value for set_toolbar_showing or similar.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:223: // Returns whether the toolbar is
accessible or not. It's called "Visible()"
Rename this function something more representative of the toolbar's state or
return an enum like ToolbarVisibility { VISIBLE, HIDDEN, AUTOHIDE_HIDDEN,
AUTOHIDE_REVEALED } (perhaps something like this exists or there are better
existing names for this behavior/state). If you just rename this function,
'Accessible' is not a great name, evokes thoughts about a11y, maybe
IsToolbarAvailable or IsToolbarPresent (or something better...). At the very
least, add a TODO to rename and revise the comment to something like "Returns
whether the toolbar is potentially visible. This function may return true even
if the toolbar is temporarily hidden, but generally available to the user (ie.
revealed when the cursor hovers the top of the screen)." You can optionally look
at the similar ash shelf auto-hide behavior/state code (but that's admittedly
not an optimal design).

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:326: if (!IsFullscreen()) {
nit: curlies not needed

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:327: return true;
optional nit: remove this conditional and just always return true for now.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:330: // TODO(zijiehe): Retrieve
the visibility of toolbar from
nit: cite a crbug.com/#

[Hzj_jie, 2017-04-05 00:33:09]

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

[commit-bot: I haz the power, 2017-04-05 00:33:50]

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-04-05 00:34:14]

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

[commit-bot: I haz the power, 2017-04-05 00:35:19]

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-04-05 01:21:05]

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

[Hzj_jie, 2017-04-05 01:21:14]

Patchset #4 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-04-05 01:21:36]

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-04-05 02:08:43]

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

[commit-bot: I haz the power, 2017-04-05 02:08:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hzj_jie, 2017-04-05 02:28:02]

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

[Hzj_jie, 2017-04-05 02:28:11]

Patchset #4 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-04-05 02:28:36]

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-04-05 02:46:30]

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

[commit-bot: I haz the power, 2017-04-05 02:46:31]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Hzj_jie, 2017-04-05 18:09:17]

It looks like the failures of both ios and chromeos builds do not relate to my
change.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:193: // https://goo.gl/4tJ32G.
On 2017/04/04 22:46:39, msw wrote:
> I still don't think that we should use URL shortener links in our codebase;
use
> the full link, or better yet, link to a crbug.com/# issue.

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:199: // the shortcuts will be
reserved once the tab strip / Chrome toolbar is
On 2017/04/04 22:46:39, msw wrote:
> nit: "the commands should be reserved for browser-side handling if the browser
> window's toolbar is visible."

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:283: // Setters
On 2017/04/04 22:46:39, msw wrote:
> nit: remove this comment, it doesn't help.

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:284: void ShowToolbar()
{ toolbar_showing_ = true; }
On 2017/04/04 22:46:39, msw wrote:
> nit: make this set_toolbar_showing(bool showing) { toolbar_showing_ = showing;
}

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:342:
EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
On 2017/04/04 22:46:39, msw wrote:
> Test IDC_EXIT (and others) here?

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:369:
EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
On 2017/04/04 22:46:39, msw wrote:
> Test IDC_EXIT (and others) here?

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:372: // shortcuts
should be delivered to the web page. See https://goo.gl/4tJ32G.
On 2017/04/04 22:46:39, msw wrote:
> Ditto, use the full url, or a crbug.com/# addy.

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:373:
EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
On 2017/04/04 22:46:39, msw wrote:
> nit: it would be nice to reduce the boilerplate for all these checks, to make
> the behavior of the tests/expectations more readily apparent. Consider one or
> both of these suggestions:
> (1) creating command id array(s) with corresponding expectations (ie. const
> int[] commands_reserved_in_fullscreen = { IDC..., IDC... }; ), or an array of
> structs with bool values for the expected (per-fullscreen?) reserved/enabled
> state, which you can then loop over like RenderTextTest:
>   struct CommandExpectations { int command_id; bool reserved; bool enabled; };
>   CommandExpectations fullscreen_expectations[] = { {IDC..., true, true}, ...
};
>   for (size_t i = 0; i < arraysize(fullscreen_expectations); ++i) {
>     SCOPED_TRACE(base::StringPrintf("Testing i=[%" PRIuS "] '%d'", i,
> fullscreen_expectations[i].command_id));
>     EXPECT_EQ(fullscreen_expectations[i].enabled,
> chrome::IsCommandEnabled(browser(), fullscreen_expectations[i].command_id));
> If the enabled state always matches the reserved state, combine those bools
> (2) Caching the BrowserCommandController and NativeWebKeyboardEvent like:
>   BrowserCommandController* controller = browser()->command_controller();
>   content::NativeWebKeyboardEvent event(blink::WebInputEvent::TypeFirst, 0,
0);
>   EXPECT_FALSE(controller->IsReservedCommandOrKey(IDC_CLOSE_TAB, event);

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:395: IDC_EXIT,
On 2017/04/04 22:46:39, msw wrote:
> Test IDC_FULLSCREEN here?

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:398: // Exit
fullscreen.
On 2017/04/04 22:46:39, msw wrote:
> If this block is just a duplicate of the one at the top, it'd be extra nice to
> use command arrays and loops, instead of copy-pasting this whole thing.

Yes, updated.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:463: // Defaults for a
tabbed browser.
On 2017/04/04 22:46:39, msw wrote:
> If this is a duplicate of the top of UpdateCommandsForFullscreenMode, it'd be
> nice to use the array approach. It'd be even better to merge this test with
that
> above, with either some extra loops for Mac, or as a parameterized fixture
> TEST_P() with a bool value for set_toolbar_showing or similar.

Merged into the previous test.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:223: // Returns whether the toolbar is
accessible or not. It's called "Visible()"
On 2017/04/04 22:46:39, msw wrote:
> Rename this function something more representative of the toolbar's state or
> return an enum like ToolbarVisibility { VISIBLE, HIDDEN, AUTOHIDE_HIDDEN,
> AUTOHIDE_REVEALED } (perhaps something like this exists or there are better
> existing names for this behavior/state). If you just rename this function,
> 'Accessible' is not a great name, evokes thoughts about a11y, maybe
> IsToolbarAvailable or IsToolbarPresent (or something better...). At the very
> least, add a TODO to rename and revise the comment to something like "Returns
> whether the toolbar is potentially visible. This function may return true even
> if the toolbar is temporarily hidden, but generally available to the user (ie.
> revealed when the cursor hovers the top of the screen)." You can optionally
look
> at the similar ash shelf auto-hide behavior/state code (but that's admittedly
> not an optimal design).

I used to rename it to IsToolbarAvailable(), which is a very reasonable name
IMO. But I found some other functions, such as IsTabStripVisible(), which are
also called "Visible", even they are returning "Available". So I doubt whether
this is a naming convention.
Returning an enum increases the complexity to the users, which, I believe, will
become a huge change. I am happy to add a TODO here to state the plan of
renaming these functions. But I definitely do not want to do it in this change
:)

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:326: if (!IsFullscreen()) {
On 2017/04/04 22:46:39, msw wrote:
> nit: curlies not needed

Done.

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:327: return true;
On 2017/04/04 22:46:39, msw wrote:
> optional nit: remove this conditional and just always return true for now.

I'd prefer to keep this just in case I forgot it later :)

https://codereview.chromium.org/2781633003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:330: // TODO(zijiehe): Retrieve
the visibility of toolbar from
On 2017/04/04 22:46:39, msw wrote:
> nit: cite a crbug.com/#

Done.

[msw, 2017-04-05 23:55:12]

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:192: // be delivered to the web
page. See, intent to implement and ship can be
nit: grammar, consider "The intent to implement and ship can be found in", "See
the intent to implement and ship mentioned in", or just "See details in"

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:321: const int
commands_enabled_in_tab[] = {
I have a slight preference for a struct with bool values for each of the
relevant expectations. That way we'd have a single array and all of its commands
are tested for each of the conditions. Otherwise, we're prone to some incomplete
testing (ie. we never check if IDC_OPEN_CURRENT_URL is reserved or not). Bonus
points for a nicely formatted table:

struct {
  int command_id;
  bool enabled_in_tab;
  bool reserved_in_tab;
  bool enabled_in_fullscreen;
  bool reserved_in_fullscreen;
} commands[] = {
    //       Command ID       |      in-tab       |     fullscreen    |
    //                        | enabled, reserved | enabled, reserved |
    { IDC_FAKE_COMMAND_1,       true,    false,     true,    false    },
    { IDC_FAKE_COMMAND_EXAMPLE, false,   true,      false,   true     },
};

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:322:
IDC_OPEN_CURRENT_URL,
nit: use a consistent indentation level for these arrays.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:419: EXPECT_TRUE(
Have you checked the output for a failure in this test? My guess is that it's
not informative (ie. doesn't state the command id) It'd be nice to have some
SCOPED_TRACE statements to give the command id value (and maybe the array
index?).

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:441: // Once the
toolbar is not showing, the behavior on Mac OS should be
nit: 'When the'

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:442: // consistent as
other platforms.
nit: 'consistent with'

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:464: // If the toolbar
is visible, all the keyboard shortcuts should be preserved
nit: "When the toolbar is showing, commands should be reserved as if the content
were in a tab; IDC_FULLSCREEN should also be reserved."

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:475: if
(commands_unreserved_in_tab[i] != IDC_FULLSCREEN) {
aside: This is a bit odd, since the only value in |commands_unreserved_in_tab|
is IDC_FULLSCREEN... But I guess that's future-proofing for other items to be
added to that array?

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:330: //
FullscreenToolbarController. See http://crbug.com/702251.
This looks like the wrong bug? "Event.preventDefault() blocks ⌘W" is fixed?

[Hzj_jie, 2017-04-06 00:58:12]

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

[Hzj_jie, 2017-04-06 00:58:42]

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

[commit-bot: I haz the power, 2017-04-06 00:59:41]

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-04-06 01:49:28]

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

[commit-bot: I haz the power, 2017-04-06 01:49:29]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-04-06 02:37:59]

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:192: // be delivered to the web
page. See, intent to implement and ship can be
On 2017/04/05 23:55:10, msw wrote:
> nit: grammar, consider "The intent to implement and ship can be found in",
"See
> the intent to implement and ship mentioned in", or just "See details in"

Done.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:321: const int
commands_enabled_in_tab[] = {
On 2017/04/05 23:55:11, msw wrote:
> I have a slight preference for a struct with bool values for each of the
> relevant expectations. That way we'd have a single array and all of its
commands
> are tested for each of the conditions. Otherwise, we're prone to some
incomplete
> testing (ie. we never check if IDC_OPEN_CURRENT_URL is reserved or not). Bonus
> points for a nicely formatted table:
> 
> struct {
>   int command_id;
>   bool enabled_in_tab;
>   bool reserved_in_tab;
>   bool enabled_in_fullscreen;
>   bool reserved_in_fullscreen;
> } commands[] = {
>     //       Command ID       |      in-tab       |     fullscreen    |
>     //                        | enabled, reserved | enabled, reserved |
>     { IDC_FAKE_COMMAND_1,       true,    false,     true,    false    },
>     { IDC_FAKE_COMMAND_EXAMPLE, false,   true,      false,   true     },
> };

Done.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:322:
IDC_OPEN_CURRENT_URL,
On 2017/04/05 23:55:11, msw wrote:
> nit: use a consistent indentation level for these arrays.

Weird, git cl format does not format all the lines.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:419: EXPECT_TRUE(
On 2017/04/05 23:55:11, msw wrote:
> Have you checked the output for a failure in this test? My guess is that it's
> not informative (ie. doesn't state the command id) It'd be nice to have some
> SCOPED_TRACE statements to give the command id value (and maybe the array
> index?).

Done.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:441: // Once the
toolbar is not showing, the behavior on Mac OS should be
On 2017/04/05 23:55:10, msw wrote:
> nit: 'When the'

Done.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:442: // consistent as
other platforms.
On 2017/04/05 23:55:10, msw wrote:
> nit: 'consistent with'

Done.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:464: // If the toolbar
is visible, all the keyboard shortcuts should be preserved
On 2017/04/05 23:55:11, msw wrote:
> nit: "When the toolbar is showing, commands should be reserved as if the
content
> were in a tab; IDC_FULLSCREEN should also be reserved."

Done.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:475: if
(commands_unreserved_in_tab[i] != IDC_FULLSCREEN) {
On 2017/04/05 23:55:11, msw wrote:
> aside: This is a bit odd, since the only value in |commands_unreserved_in_tab|
> is IDC_FULLSCREEN... But I guess that's future-proofing for other items to be
> added to that array?

Now all commands are in one structure, we do not have this issue anymore.

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/2781633003/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:330: //
FullscreenToolbarController. See http://crbug.com/702251.
On 2017/04/05 23:55:11, msw wrote:
> This looks like the wrong bug? "Event.preventDefault() blocks ⌘W" is fixed?

Maybe I can add both bugs to the comment. The closed bug regards to the behavior
of Mac OS. But the issue mentioned in this bug has been resolved as we have
reverted the last change. Another bug crbug.com/680809 is for a generic
implementation, instead of a Mac OS specific one.

[msw, 2017-04-06 03:03:49]

lgtm with nits; thanks for all the improvements!

https://codereview.chromium.org/2781633003/diff/240001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2781633003/diff/240001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:372:
SCOPED_TRACE(testing::Message()
nit: add SCOPED_TRACE messages for the other loops too; bonus if you can shorten
them (only the command number really matters)

https://codereview.chromium.org/2781633003/diff/240001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:388: #if
defined(OS_MACOSX)
nit: skip |#if defined(OS_MACOSX)|, and just do this for all platforms:
  // All platforms behavior similarly in fullscreen when the toolbar is hidden.
  EXPECT_FALSE(window()->IsToolbarShowing());

[Hzj_jie, 2017-04-06 17:42:08]

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

[commit-bot: I haz the power, 2017-04-06 17:42:52]

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-04-06 18:34:24]

The test failure is DownloadExtensionTest.DownloadExtensionTest_SearchState; it
definitely does not relate to my change.

https://codereview.chromium.org/2781633003/diff/240001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller_unittest.cc (right):

https://codereview.chromium.org/2781633003/diff/240001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:372:
SCOPED_TRACE(testing::Message()
On 2017/04/06 03:03:48, msw wrote:
> nit: add SCOPED_TRACE messages for the other loops too; bonus if you can
shorten
> them (only the command number really matters)

Oh, sorry, seems I have not copied these lines to other loops.

https://codereview.chromium.org/2781633003/diff/240001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller_unittest.cc:388: #if
defined(OS_MACOSX)
On 2017/04/06 03:03:48, msw wrote:
> nit: skip |#if defined(OS_MACOSX)|, and just do this for all platforms:
>   // All platforms behavior similarly in fullscreen when the toolbar is
hidden.
>   EXPECT_FALSE(window()->IsToolbarShowing());

Done.
Since this EXPECT_FALSE expects the IsToolbarShowing() returns false, I have
added more comments to explain the default behavior of toolbar in fullscreen
mode.

[Hzj_jie, 2017-04-06 18:45:40]

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

[Hzj_jie, 2017-04-06 18:45:55]

Patchset #6 (id:260001) has been deleted

[Hzj_jie, 2017-04-06 18:46:48]

zijiehe@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, sky@chromium.org

[commit-bot: I haz the power, 2017-04-06 18:47:37]

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

[msw, 2017-04-06 18:51:47]

Nice! lgtm

[Hzj_jie, 2017-04-06 18:56:50]

Will Scott lgtm for test_browser_window.* change, or I can TBR him? IMO, it's a
tiny change to the TestBrowserWindow class.

[msw, 2017-04-06 19:05:10]

On 2017/04/06 18:56:50, Hzj_jie wrote:
> Will Scott lgtm for test_browser_window.* change, or I can TBR him? IMO, it's
a
> tiny change to the TestBrowserWindow class.

Please get his review; TBR is not entirely appropriate.

[Hzj_jie, 2017-04-06 19:06:59]

On 2017/04/06 19:05:10, msw wrote:
> On 2017/04/06 18:56:50, Hzj_jie wrote:
> > Will Scott lgtm for test_browser_window.* change, or I can TBR him? IMO,
it's
> a
> > tiny change to the TestBrowserWindow class.
> 
> Please get his review; TBR is not entirely appropriate.

Sure. (I thought you can also approve the changes though you are not the owner,
since he is your boss :)

[commit-bot: I haz the power, 2017-04-06 19:46:08]

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

[commit-bot: I haz the power, 2017-04-06 19:46:09]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-06 20:38:12]

https://codereview.chromium.org/2781633003/diff/260002/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:232: virtual bool IsToolbarShowing() const =
0;
Will this be renamed to IsToolbarVisible() once you rename what is
IsToolbarVisible to IsToolbarAvailable?

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:147: return IsToolbarVisible();
Is there a reason to have this call IsToolbarVisible? I prefer it return false.

[Hzj_jie, 2017-04-06 20:41:42]

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

[Hzj_jie, 2017-04-06 20:42:09]

https://codereview.chromium.org/2781633003/diff/260002/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:232: virtual bool IsToolbarShowing() const =
0;
On 2017/04/06 20:38:10, sky wrote:
> Will this be renamed to IsToolbarVisible() once you rename what is
> IsToolbarVisible to IsToolbarAvailable?

I think so. TODO has been added.

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:147: return IsToolbarVisible();
On 2017/04/06 20:38:10, sky wrote:
> Is there a reason to have this call IsToolbarVisible? I prefer it return
false.

In platform other than Mac OS, the toolbar is showing up when it's available.

[commit-bot: I haz the power, 2017-04-06 20:42:30]

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-04-06 21:37:05]

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

[commit-bot: I haz the power, 2017-04-06 21:37:06]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-06 21:43:56]

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:147: return IsToolbarVisible();
On 2017/04/06 20:42:09, Hzj_jie wrote:
> On 2017/04/06 20:38:10, sky wrote:
> > Is there a reason to have this call IsToolbarVisible? I prefer it return
> false.
> 
> In platform other than Mac OS, the toolbar is showing up when it's available.

This is a test class though, can't the implementations that need it override to
return whatever they want?

[Hzj_jie, 2017-04-06 21:48:11]

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:147: return IsToolbarVisible();
On 2017/04/06 21:43:56, sky wrote:
> On 2017/04/06 20:42:09, Hzj_jie wrote:
> > On 2017/04/06 20:38:10, sky wrote:
> > > Is there a reason to have this call IsToolbarVisible? I prefer it return
> > false.
> > 
> > In platform other than Mac OS, the toolbar is showing up when it's
available.
> 
> This is a test class though, can't the implementations that need it override
to
> return whatever they want?

Yes, BrowserWindowCocoa overrides this function and return the visibility of the
toolbar by querying FullscreenToolbarController. Another production
implementation (BrowserView) also returns IsToolbarVisible() in
IsToolbarShowing() to match the cases on other platforms.

[sky, 2017-04-06 23:26:00]

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
File chrome/test/base/test_browser_window.cc (right):

https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
chrome/test/base/test_browser_window.cc:147: return IsToolbarVisible();
On 2017/04/06 21:48:11, Hzj_jie wrote:
> On 2017/04/06 21:43:56, sky wrote:
> > On 2017/04/06 20:42:09, Hzj_jie wrote:
> > > On 2017/04/06 20:38:10, sky wrote:
> > > > Is there a reason to have this call IsToolbarVisible? I prefer it return
> > > false.
> > > 
> > > In platform other than Mac OS, the toolbar is showing up when it's
> available.
> > 
> > This is a test class though, can't the implementations that need it override
> to
> > return whatever they want?
> 
> Yes, BrowserWindowCocoa overrides this function and return the visibility of
the
> toolbar by querying FullscreenToolbarController. Another production
> implementation (BrowserView) also returns IsToolbarVisible() in
> IsToolbarShowing() to match the cases on other platforms.

Sorry, I'm confused. Why can't this return false? If you need some test to
override it, then test can do that, right?

[Hzj_jie, 2017-04-06 23:27:29]

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

[Hzj_jie, 2017-04-06 23:27:42]

On 2017/04/06 23:26:00, sky wrote:
>
https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
> File chrome/test/base/test_browser_window.cc (right):
> 
>
https://codereview.chromium.org/2781633003/diff/260002/chrome/test/base/test_...
> chrome/test/base/test_browser_window.cc:147: return IsToolbarVisible();
> On 2017/04/06 21:48:11, Hzj_jie wrote:
> > On 2017/04/06 21:43:56, sky wrote:
> > > On 2017/04/06 20:42:09, Hzj_jie wrote:
> > > > On 2017/04/06 20:38:10, sky wrote:
> > > > > Is there a reason to have this call IsToolbarVisible? I prefer it
return
> > > > false.
> > > > 
> > > > In platform other than Mac OS, the toolbar is showing up when it's
> > available.
> > > 
> > > This is a test class though, can't the implementations that need it
override
> > to
> > > return whatever they want?
> > 
> > Yes, BrowserWindowCocoa overrides this function and return the visibility of
> the
> > toolbar by querying FullscreenToolbarController. Another production
> > implementation (BrowserView) also returns IsToolbarVisible() in
> > IsToolbarShowing() to match the cases on other platforms.
> 
> Sorry, I'm confused. Why can't this return false? If you need some test to
> override it, then test can do that, right?

No, I just thought returning IsToolbarVisible() from IsToolbarShowing() is the
common case.
Updated.

[commit-bot: I haz the power, 2017-04-06 23:28:24]

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-04-07 00:18:05]

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

[commit-bot: I haz the power, 2017-04-07 00:18:07]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-07 14:18:23]

Thanks, LGTM - also, in the future please include chromium-reviews on the cc
list. It's usually added automatically.

[Hzj_jie, 2017-04-07 18:42:26]

On 2017/04/07 14:18:23, sky wrote:
> Thanks, LGTM - also, in the future please include chromium-reviews on the cc
> list. It's usually added automatically.

Sure, added.

Thank you guys for the reviewing. Go ahead to submit this change.

[Hzj_jie, 2017-04-07 18:42:50]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2017-04-07 18:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2781633003/#ps310001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2017-04-07 18:43:35]

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-04-07 18:50:34]

CQ is committing da patch.

Bot data: {"patchset_id": 310001, "attempt_start_ts": 1491590570272330,
"parent_rev": "c2a05c72389f8afb3c55e9a0aa338fb833bbf546", "commit_rev":
"68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b"}

[commit-bot: I haz the power, 2017-04-07 18:51:19]

Description was changed from

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds a BrowserWindow::IsToolbarShowing() function to indicate
whether the toolbar is showing up on the screen. BrowserWindowCocoa needs to
override this function, which has not been done yet in this change.

BUG=680809, 702251
==========

to

==========
Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in http://crbug.com/702251, we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds a BrowserWindow::IsToolbarShowing() function to indicate
whether the toolbar is showing up on the screen. BrowserWindowCocoa needs to
override this function, which has not been done yet in this change.

BUG=680809, 702251

Review-Url: https://codereview.chromium.org/2781633003
Cr-Commit-Position: refs/heads/master@{#462942}
Committed:
https://chromium.googlesource.com/chromium/src/+/68cd3dc22ddbfddfdcf252f42b12...
==========

[commit-bot: I haz the power, 2017-04-07 18:51:21]

Committed patchset #8 (id:310001) as
https://chromium.googlesource.com/chromium/src/+/68cd3dc22ddbfddfdcf252f42b12...