Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(138)

Issue 2636013002: Disable browser key reservation in fullscreen mode (Closed)

Created:
3 years, 11 months ago by Hzj_jie
Modified:
3 years, 10 months ago
Reviewers:
dominickn, msw
CC:
chromium-reviews, Jamie, garykac
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Disable browser key reservation in fullscreen mode In intent to implement https://goo.gl/4tJ32G, we decided to deliver browser shortcuts to the websites, so a web page can provide almost consistent user experience as a native application. To ensure there is no security risk, a simple and safe approach is to allow websites to capture browser reserved keyboard combinations in full screen mode only. This does not apply to the keys to exit fullscreen (F11) or exit the application (ESC). BUG=680809 Review-Url: https://codereview.chromium.org/2636013002 Cr-Commit-Position: refs/heads/master@{#449896} Committed: https://chromium.googlesource.com/chromium/src/+/3c7af99a93f4b4837b2fbee5cb66697f66ccf241

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add test cases #

Total comments: 4

Patch Set 3 : Resolve review comments #

Total comments: 14

Patch Set 4 : Resolve review comments #

Patch Set 5 : Resolve review comments #

Total comments: 12

Patch Set 6 : Resolve review comments #

Total comments: 7

Patch Set 7 : Resolve review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -38 lines) Patch
M chrome/browser/ui/browser_command_controller.cc View 1 2 3 4 5 6 6 chunks +38 lines, -38 lines 0 comments Download
M chrome/browser/ui/browser_command_controller_unittest.cc View 1 2 2 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 64 (37 generated)
Hzj_jie
3 years, 11 months ago (2017-01-16 21:04:02 UTC) #7
dominickn
Hey, it's a little early in the process to post this. :) We need an ...
3 years, 11 months ago (2017-01-16 22:13:39 UTC) #8
Hzj_jie
On 2017/01/16 22:13:39, dominickn wrote: > Hey, it's a little early in the process to ...
3 years, 11 months ago (2017-01-17 23:19:01 UTC) #11
Hzj_jie
https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_command_controller.cc#newcode204 chrome/browser/ui/browser_command_controller.cc:204: // the web page. This has been discussed in ...
3 years, 11 months ago (2017-01-17 23:19:10 UTC) #12
dominickn
Could you put the call to CommandUpdater in chrome::ToggleFullscreenMode()? Maybe verify that chrome::ToggleFullscreenMode is the ...
3 years, 10 months ago (2017-01-30 01:01:20 UTC) #15
Hzj_jie
I have just realized that BrowserCommandController has an UpdateCommandsForFullscreenMode(), which is useful in our scenario. ...
3 years, 10 months ago (2017-02-08 00:24:07 UTC) #18
dominickn
Please update the CL description to link to the intent to implement thread on blink-dev, ...
3 years, 10 months ago (2017-02-08 00:42:38 UTC) #21
Hzj_jie
https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/browser_command_controller.cc#oldcode307 chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); On 2017/02/08 00:42:37, dominickn wrote: > Why did ...
3 years, 10 months ago (2017-02-08 18:29:38 UTC) #27
dominickn
https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/browser_command_controller.cc#oldcode307 chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); Intents to implement are only required for web-facing ...
3 years, 10 months ago (2017-02-08 23:40:22 UTC) #28
Hzj_jie
https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/browser_command_controller.cc#oldcode307 chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); On 2017/02/08 23:40:21, dominickn wrote: > Intents to ...
3 years, 10 months ago (2017-02-09 02:04:51 UTC) #33
dominickn
Thanks for this. lgtm. You'll need to find a chrome/browser/ui OWNER (perhaps msw@) to approve ...
3 years, 10 months ago (2017-02-09 02:56:30 UTC) #34
Hzj_jie
Thank you Dominick for the review. Involve Mike.
3 years, 10 months ago (2017-02-09 17:25:51 UTC) #36
msw
Your CL description should mention that this doesn't apply to the keys to exit fullscreen ...
3 years, 10 months ago (2017-02-09 18:01:13 UTC) #37
dominickn
On 2017/02/09 18:01:13, msw wrote: > Your CL description should mention that this doesn't apply ...
3 years, 10 months ago (2017-02-09 21:21:19 UTC) #38
msw
On 2017/02/09 21:21:19, dominickn wrote: > FYI I'm the Security Enamel person on this. :) ...
3 years, 10 months ago (2017-02-09 23:33:06 UTC) #39
Hzj_jie
The test failures on Windows should not relate to my change. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (right): ...
3 years, 10 months ago (2017-02-10 01:14:23 UTC) #45
msw
https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc#newcode199 chrome/browser/ui/browser_command_controller.cc:199: if (window()->IsFullscreen()) { Can we replace this block with: ...
3 years, 10 months ago (2017-02-10 02:03:39 UTC) #46
Hzj_jie
https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc#newcode199 chrome/browser/ui/browser_command_controller.cc:199: if (window()->IsFullscreen()) { On 2017/02/10 02:03:39, msw wrote: > ...
3 years, 10 months ago (2017-02-10 19:18:08 UTC) #48
dominickn
On 2017/02/09 23:33:06, msw wrote: > On 2017/02/09 21:21:19, dominickn wrote: > > FYI I'm ...
3 years, 10 months ago (2017-02-10 21:26:59 UTC) #52
msw
https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc#newcode199 chrome/browser/ui/browser_command_controller.cc:199: if (window()->IsFullscreen()) { On 2017/02/10 19:18:08, Hzj_jie wrote: > ...
3 years, 10 months ago (2017-02-10 23:56:14 UTC) #53
Hzj_jie
On 2017/02/10 23:56:14, msw wrote: > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc > File chrome/browser/ui/browser_command_controller.cc (right): > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc#newcode199 > ...
3 years, 10 months ago (2017-02-11 00:16:48 UTC) #54
msw
On 2017/02/11 00:16:48, Hzj_jie wrote: > On 2017/02/10 23:56:14, msw wrote: > > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/browser_command_controller.cc ...
3 years, 10 months ago (2017-02-11 00:44:53 UTC) #55
Hzj_jie
On 2017/02/11 00:44:53, msw wrote: > On 2017/02/11 00:16:48, Hzj_jie wrote: > > On 2017/02/10 ...
3 years, 10 months ago (2017-02-11 00:50:50 UTC) #56
msw
On 2017/02/11 00:50:50, Hzj_jie wrote: > On 2017/02/11 00:44:53, msw wrote: > > On 2017/02/11 ...
3 years, 10 months ago (2017-02-11 01:50:57 UTC) #57
Hzj_jie
On 2017/02/11 01:50:57, msw wrote: > On 2017/02/11 00:50:50, Hzj_jie wrote: > > On 2017/02/11 ...
3 years, 10 months ago (2017-02-12 20:14:46 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2636013002/120001
3 years, 10 months ago (2017-02-12 20:15:12 UTC) #61
commit-bot: I haz the power
3 years, 10 months ago (2017-02-12 21:01:05 UTC) #64
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3c7af99a93f4b4837b2fbee5cb66...

Powered by Google App Engine
This is Rietveld 408576698