|
|
DescriptionDisable 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 #
Messages
Total messages: 64 (37 generated)
Description was changed from ========== Disable browser key reservation in fullscreen mode In feature proposal "Allowing sites to intercept keyboard shortcuts" at http://shortn/_8CfN9KaMe6, a simple and safe approach is to allow websites to capture browser reserved keyboard combinations in full screen mode. BUG=680809 ========== to ========== Disable browser key reservation in fullscreen mode In feature proposal "Allowing sites to intercept keyboard shortcuts" at http://shortn/_8CfN9KaMe6, a simple and safe approach is to allow websites to capture browser reserved keyboard combinations in full screen mode. BUG=680809 ==========
zijiehe@chromium.org changed reviewers: + dominickn@chromium.org
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey, it's a little early in the process to post this. :) We need an intent to implement and ship first. https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_command_controller.cc:204: // the web page. This has been discussed in the document Allowing sites to Since this is a web-facing change, this cannot be landed until an intent to implement and ship is posted to blink-dev and approved. I think garykac@ is in charge of that. When it's been posted and approved, this CL should link to the blink-dev discussion, not to an internal doc. A test should also be added for the changes in this method, probably in browser_command_controller_unittest.cc
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/16 22:13:39, dominickn wrote: > Hey, it's a little early in the process to post this. :) > > We need an intent to implement and ship first. > > https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_c... > File chrome/browser/ui/browser_command_controller.cc (right): > > https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_c... > chrome/browser/ui/browser_command_controller.cc:204: // the web page. This has > been discussed in the document Allowing sites to > Since this is a web-facing change, this cannot be landed until an intent to > implement and ship is posted to blink-dev and approved. I think garykac@ is in > charge of that. > > When it's been posted and approved, this CL should link to the blink-dev > discussion, not to an internal doc. > > A test should also be added for the changes in this method, probably in > browser_command_controller_unittest.cc Thank you Dominic, I believe this change is just a very good start for me to understand the process of blink development. It looks like the change is far more than two lines. Technically said, this change has already implemented the logic to pass the preserved key combinations to the website in fullscreen mode. But CommandUpdater has not been correctly notified. You may find the strange test expectations in browser_command_controller_unittest.cc. To achieve this, a callback, maybe OnWindowStateChange, needs to be added to BaseWindow, then we can attach the callback to execute the CommandUpdater, which will make this change larger.
https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_command_controller.cc:204: // the web page. This has been discussed in the document Allowing sites to On 2017/01/16 22:13:39, dominickn wrote: > Since this is a web-facing change, this cannot be landed until an intent to > implement and ship is posted to blink-dev and approved. I think garykac@ is in > charge of that. > > When it's been posted and approved, this CL should link to the blink-dev > discussion, not to an internal doc. > > A test should also be added for the changes in this method, probably in > browser_command_controller_unittest.cc I have received the intent of implementation from Gary, since it's not still in discussion stage, I will update it once it is approved.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you put the call to CommandUpdater in chrome::ToggleFullscreenMode()? Maybe verify that chrome::ToggleFullscreenMode is the method called when entering fullscreen via F11 and via the JavaScript API. If it is, that seems like a natural place to do the updating. https://codereview.chromium.org/2636013002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller_unittest.cc (right): https://codereview.chromium.org/2636013002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_unittest.cc:362: EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey( Nit: add a newline and comment above this saying: // In fullscreen, only the exit fullscreen command is reserved. All other shortcuts are unreserved. See <link to approved intent to implement and ship> https://codereview.chromium.org/2636013002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_unittest.cc:385: content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0))); Can you EXPECT_TRUE that IDC_FULLSCREEN is reserved?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have just realized that BrowserCommandController has an UpdateCommandsForFullscreenMode(), which is useful in our scenario. p.s. According to the Intent to Implement, we are targeting eight commands in total, which does not include IDC_EXIT. I actively listed all in IsReservedCommandOrKey() function. https://codereview.chromium.org/2636013002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller_unittest.cc (right): https://codereview.chromium.org/2636013002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_unittest.cc:362: EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey( On 2017/01/30 01:01:19, dominickn wrote: > Nit: add a newline and comment above this saying: > > // In fullscreen, only the exit fullscreen command is reserved. All other > shortcuts are unreserved. See <link to approved intent to implement and ship> Done. https://codereview.chromium.org/2636013002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_unittest.cc:385: content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0))); On 2017/01/30 01:01:19, dominickn wrote: > Can you EXPECT_TRUE that IDC_FULLSCREEN is reserved? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please update the CL description to link to the intent to implement thread on blink-dev, not an internal link. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); Why did you change these? It seems unrelated to me. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:404: ExecuteVisitDesktopCommand(id, browser_->window()->GetNativeWindow()); This seems unrelated https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:410: PrefService* prefs = browser_->profile()->GetPrefs(); Unrelated? https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:193: // Allowing sites to intercept keyboard shortcuts at https://goo.gl/r6XmNt. Change this comment to be: In fullscreen, all keys except Esc and F11 should be delivered to the web page. See <link to blink-dev intent to implement and ship thread> https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1027: const bool is_not_fullscreen = !is_fullscreen; I don't think you need bools for is_fullscreen and is_not_fullscreen. The only thing you use is_fullscreen for is computing is_not_fullscreen. Just use: bool is_not_fullscreen = (window() && !window()->IsFullscreen()); And remove is_fullscreen.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Disable browser key reservation in fullscreen mode In feature proposal "Allowing sites to intercept keyboard shortcuts" at http://shortn/_8CfN9KaMe6, a simple and safe approach is to allow websites to capture browser reserved keyboard combinations in full screen mode. BUG=680809 ========== to ========== 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. BUG=680809 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); On 2017/02/08 00:42:37, dominickn wrote: > Why did you change these? It seems unrelated to me. I have done a full document replacement to replace browser_->window() and browser_->profile() with window() and profile(), which impacts both existing code and my new code. It looks like we always need to have an Intent to Implement for each change in Chrome. I cannot quite tell how the shortcuts (window() and profile()) can be used eventually, they have already 4.5 years old. Personally I believe these changes should be covered by any newly added code should always use these shortcuts. But unfortunately, they were not. Since I am touching this file, this cleanup looks safe, and it also helps to keep the consistency. But anyway, if you have a strong opinion, or the policy in Chrome does not allow this kind of cleanup to be merged with other changes. I would remove them. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:404: ExecuteVisitDesktopCommand(id, browser_->window()->GetNativeWindow()); On 2017/02/08 00:42:38, dominickn wrote: > This seems unrelated Explained above. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:410: PrefService* prefs = browser_->profile()->GetPrefs(); On 2017/02/08 00:42:38, dominickn wrote: > Unrelated? Ditto. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:193: // Allowing sites to intercept keyboard shortcuts at https://goo.gl/r6XmNt. On 2017/02/08 00:42:38, dominickn wrote: > Change this comment to be: > > In fullscreen, all keys except Esc and F11 should be delivered to the web page. > See <link to blink-dev intent to implement and ship thread> Done. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1027: const bool is_not_fullscreen = !is_fullscreen; On 2017/02/08 00:42:38, dominickn wrote: > I don't think you need bools for is_fullscreen and is_not_fullscreen. The only > thing you use is_fullscreen for is computing is_not_fullscreen. Just use: > > bool is_not_fullscreen = (window() && !window()->IsFullscreen()); > > And remove is_fullscreen. is_fullscreen is also used when calculating fullscreen_enabled. See line 1081.
https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); Intents to implement are only required for web-facing behaviour changes. We needed one to change the keys available in fullscreen because that changes the behaviour that web developers will see. Internal Chrome cleanups don't require intents to implement (though they may need a design document if they are complex). In this case, this isn't complex, but it is usually a good idea to put cleanups like this in separate CLs. The main reason is that they make the patch bigger, and can make it more complicated if a patch has to be reverted. Plus, you don't mention this cleanup in the CL description (making it hard to find later). As a general rule: smaller, focused CLs are better. So my advice is to remove these changes, and submit them separately. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1027: const bool is_not_fullscreen = !is_fullscreen; It just feels a little wasteful to have two boolean variables that are opposites of one another (one boolean should suffice). Is there something wrong with using !is_not_fullscreen at line 1081? There's a bunch of code already that looks like (!not_something).
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:307: browser_->window()->MaybeShowNewBackShortcutBubble(false); On 2017/02/08 23:40:21, dominickn wrote: > Intents to implement are only required for web-facing behaviour changes. We > needed one to change the keys available in fullscreen because that changes the > behaviour that web developers will see. > > Internal Chrome cleanups don't require intents to implement (though they may > need a design document if they are complex). In this case, this isn't complex, > but it is usually a good idea to put cleanups like this in separate CLs. The > main reason is that they make the patch bigger, and can make it more complicated > if a patch has to be reverted. Plus, you don't mention this cleanup in the CL > description (making it hard to find later). > > As a general rule: smaller, focused CLs are better. So my advice is to remove > these changes, and submit them separately. Oh, that would be good. I will revert these changes. https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1027: const bool is_not_fullscreen = !is_fullscreen; On 2017/02/08 23:40:21, dominickn wrote: > It just feels a little wasteful to have two boolean variables that are opposites > of one another (one boolean should suffice). Is there something wrong with using > !is_not_fullscreen at line 1081? There's a bunch of code already that looks > like (!not_something). No, nothing could be wrong. I would prefer to remove is_not_fullscreen, and use is_fullscreen & !is_fullscreen everywhere. Double negative is bad.
Thanks for this. lgtm. You'll need to find a chrome/browser/ui OWNER (perhaps msw@) to approve so you can land this. :)
zijiehe@chromium.org changed reviewers: + msw@chromium.org
Thank you Dominick for the review. Involve Mike.
Your CL description should mention that this doesn't apply to the keys to exit fullscreen or exit the application. The bug should probably get review from ux and security folks before this CL lands. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:191: // In fullscreen, all keys except Esc and F11 should be delivered to the web nit: maybe describe this in terms of commands, not keys, as you do in the unit test? Also, this comment should mention that IDC_EXIT (exiting the app) is still reserved in fullscreen. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:193: if (command_id == IDC_FULLSCREEN) { If the comment above is correct; why can't we just do: return command_id == IDC_FULLSCREEN; Do we want to still respect 'content editing hotkey' below in fullscreen? https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:195: } else if (command_id == IDC_CLOSE_TAB || nit: no else after return https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:196: command_id == IDC_CLOSE_WINDOW || nit: since most of this block is duplicated below, maybe cache the check as: bool is_browser_command = command_id == IDC_CLOSE_TAB || ...; (or maybe you'd prefer the name |is_tab_or_window_command|) for use here and in the last return statement. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1025: const bool is_fullscreen = (window() && window()->IsFullscreen()); nit: parens not needed https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1089: command_updater_.UpdateCommandEnabled(IDC_CLOSE_TAB, !is_fullscreen); Does this CL need security/UX review beyond the 'intent to implement' thread? (it seems high-impact and worth checking if security/UX has any concerns)
On 2017/02/09 18:01:13, msw wrote: > Your CL description should mention that this doesn't apply to the keys to exit > fullscreen or exit the application. The bug should probably get review from ux > and security folks before this CL lands. FYI I'm the Security Enamel person on this. :)
On 2017/02/09 21:21:19, dominickn wrote: > FYI I'm the Security Enamel person on this. :) Oh cool! Should the bug have any tags for review/approval?
Description was changed from ========== 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. BUG=680809 ========== to ========== 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 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The test failures on Windows should not relate to my change. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:191: // In fullscreen, all keys except Esc and F11 should be delivered to the web On 2017/02/09 18:01:13, msw wrote: > nit: maybe describe this in terms of commands, not keys, as you do in the unit > test? Also, this comment should mention that IDC_EXIT (exiting the app) is still > reserved in fullscreen. Done. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:193: if (command_id == IDC_FULLSCREEN) { On 2017/02/09 18:01:13, msw wrote: > If the comment above is correct; why can't we just do: > return command_id == IDC_FULLSCREEN; > Do we want to still respect 'content editing hotkey' below in fullscreen? I have updated the comment. IDC_EXIT should also be excluded. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:195: } else if (command_id == IDC_CLOSE_TAB || On 2017/02/09 18:01:13, msw wrote: > nit: no else after return Done. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:196: command_id == IDC_CLOSE_WINDOW || On 2017/02/09 18:01:13, msw wrote: > nit: since most of this block is duplicated below, maybe cache the check as: > bool is_browser_command = command_id == IDC_CLOSE_TAB || ...; > (or maybe you'd prefer the name |is_tab_or_window_command|) > for use here and in the last return statement. Done. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1025: const bool is_fullscreen = (window() && window()->IsFullscreen()); On 2017/02/09 18:01:13, msw wrote: > nit: parens not needed Done. https://codereview.chromium.org/2636013002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1089: command_updater_.UpdateCommandEnabled(IDC_CLOSE_TAB, !is_fullscreen); On 2017/02/09 18:01:13, msw wrote: > Does this CL need security/UX review beyond the 'intent to implement' thread? > (it seems high-impact and worth checking if security/UX has any concerns) Dominick has reviewed this already, though I cannot tell whether there are extra steps we need to take care.
https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:199: if (window()->IsFullscreen()) { Can we replace this block with: if (window()->IsFullscreen()) return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; That seems to be a more concise implementation of the intended behavior, and matches what's described in the comment. (if so, you can revert the |is_tab_or_window_command| change) https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:202: if (command_id == IDC_FULLSCREEN) { nit: curlies not needed https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:205: if (is_tab_or_window_command) { nit: curlies not needed
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:199: if (window()->IsFullscreen()) { On 2017/02/10 02:03:39, msw wrote: > Can we replace this block with: > if (window()->IsFullscreen()) > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > That seems to be a more concise implementation of the intended behavior, and > matches what's described in the comment. (if so, you can revert the > |is_tab_or_window_command| change) It looks like your solution changes the behavior on Linux. The following ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing ESC in fullscreen. https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:202: if (command_id == IDC_FULLSCREEN) { On 2017/02/10 02:03:39, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:205: if (is_tab_or_window_command) { On 2017/02/10 02:03:39, msw wrote: > nit: curlies not needed Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 23:33:06, msw wrote: > On 2017/02/09 21:21:19, dominickn wrote: > > FYI I'm the Security Enamel person on this. :) > > Oh cool! Should the bug have any tags for review/approval? The OWP Security folks have seen this and not had an issue, and I'm stamping from the Enamel side. :)
https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:199: if (window()->IsFullscreen()) { On 2017/02/10 19:18:08, Hzj_jie wrote: > On 2017/02/10 02:03:39, msw wrote: > > Can we replace this block with: > > if (window()->IsFullscreen()) > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > That seems to be a more concise implementation of the intended behavior, and > > matches what's described in the comment. (if so, you can revert the > > |is_tab_or_window_command| change) > > It looks like your solution changes the behavior on Linux. The following > ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing ESC in > fullscreen. Can you elaborate? It looks like the Linux block only ever returns false, and afaict, MatchEvent() doesn't have any side-effects; so the only conditions under which fullscreen would return true below line 190 is if the command is IDC_FULLSCREEN or IDC_EXIT. So, my suggested code should keep the intended behavior, right? Please let me know if I'm missing something. if (window()->IsFullscreen()) return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT;
On 2017/02/10 23:56:14, msw wrote: > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... > File chrome/browser/ui/browser_command_controller.cc (right): > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... > chrome/browser/ui/browser_command_controller.cc:199: if > (window()->IsFullscreen()) { > On 2017/02/10 19:18:08, Hzj_jie wrote: > > On 2017/02/10 02:03:39, msw wrote: > > > Can we replace this block with: > > > if (window()->IsFullscreen()) > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > That seems to be a more concise implementation of the intended behavior, and > > > matches what's described in the comment. (if so, you can revert the > > > |is_tab_or_window_command| change) > > > > It looks like your solution changes the behavior on Linux. The following > > ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing ESC in > > fullscreen. > > Can you elaborate? It looks like the Linux block only ever returns false, and > afaict, MatchEvent() doesn't have any side-effects; so the only conditions under > which fullscreen would return true below line 190 is if the command is > IDC_FULLSCREEN or IDC_EXIT. So, my suggested code should keep the intended > behavior, right? Please let me know if I'm missing something. > if (window()->IsFullscreen()) > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; Sorry about lacking of knowledge about TextEditKeyBindingDelegateAuraLinux. But I see two potential issues with the change you suggested. 1. When command_id is IDC_EXIT. In your change, this function always returns true in fullscreen. But current implementation may return false if TextEditKeyBindingDelegateAuraLinux::MatchEvent() returns true. This behavior is consistent with the version without this patch. 2. I have no idea what TextEditKeyBindingDelegateAuraLinux::MatchEvent() will do, but it's not a const function, which means it may have side effect. Say, Gtk2KeyBindingsHandler::MatchEvent().
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/brow... > > File chrome/browser/ui/browser_command_controller.cc (right): > > > > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... > > chrome/browser/ui/browser_command_controller.cc:199: if > > (window()->IsFullscreen()) { > > On 2017/02/10 19:18:08, Hzj_jie wrote: > > > On 2017/02/10 02:03:39, msw wrote: > > > > Can we replace this block with: > > > > if (window()->IsFullscreen()) > > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > That seems to be a more concise implementation of the intended behavior, > and > > > > matches what's described in the comment. (if so, you can revert the > > > > |is_tab_or_window_command| change) > > > > > > It looks like your solution changes the behavior on Linux. The following > > > ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing ESC in > > > fullscreen. > > > > Can you elaborate? It looks like the Linux block only ever returns false, and > > afaict, MatchEvent() doesn't have any side-effects; so the only conditions > under > > which fullscreen would return true below line 190 is if the command is > > IDC_FULLSCREEN or IDC_EXIT. So, my suggested code should keep the intended > > behavior, right? Please let me know if I'm missing something. > > if (window()->IsFullscreen()) > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > Sorry about lacking of knowledge about TextEditKeyBindingDelegateAuraLinux. But > I see two potential issues with the change you suggested. > 1. When command_id is IDC_EXIT. In your change, this function always returns > true in fullscreen. But current implementation may return false if > TextEditKeyBindingDelegateAuraLinux::MatchEvent() returns true. This behavior is > consistent with the version without this patch. > 2. I have no idea what TextEditKeyBindingDelegateAuraLinux::MatchEvent() will > do, but it's not a const function, which means it may have side effect. Say, > Gtk2KeyBindingsHandler::MatchEvent(). Hmm, if TextEditKeyBindingDelegateAuraLinux::MatchEvent() matches IDC_EXIT or IDC_FULLSCREEN as a user-registered content editing hotkey, wouldn't that mean that users could get stuck in fullscreen? That would be troubling; we should know if it's a real possibility before landing this change. Can you investigate that so we have a clear answer? If Dominick says this is fine as-is, I can give you a rubber stamp.
On 2017/02/11 00:44:53, msw wrote: > 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/brow... > > > File chrome/browser/ui/browser_command_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... > > > chrome/browser/ui/browser_command_controller.cc:199: if > > > (window()->IsFullscreen()) { > > > On 2017/02/10 19:18:08, Hzj_jie wrote: > > > > On 2017/02/10 02:03:39, msw wrote: > > > > > Can we replace this block with: > > > > > if (window()->IsFullscreen()) > > > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > > That seems to be a more concise implementation of the intended behavior, > > and > > > > > matches what's described in the comment. (if so, you can revert the > > > > > |is_tab_or_window_command| change) > > > > > > > > It looks like your solution changes the behavior on Linux. The following > > > > ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing ESC > in > > > > fullscreen. > > > > > > Can you elaborate? It looks like the Linux block only ever returns false, > and > > > afaict, MatchEvent() doesn't have any side-effects; so the only conditions > > under > > > which fullscreen would return true below line 190 is if the command is > > > IDC_FULLSCREEN or IDC_EXIT. So, my suggested code should keep the intended > > > behavior, right? Please let me know if I'm missing something. > > > if (window()->IsFullscreen()) > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > Sorry about lacking of knowledge about TextEditKeyBindingDelegateAuraLinux. > But > > I see two potential issues with the change you suggested. > > 1. When command_id is IDC_EXIT. In your change, this function always returns > > true in fullscreen. But current implementation may return false if > > TextEditKeyBindingDelegateAuraLinux::MatchEvent() returns true. This behavior > is > > consistent with the version without this patch. > > 2. I have no idea what TextEditKeyBindingDelegateAuraLinux::MatchEvent() will > > do, but it's not a const function, which means it may have side effect. Say, > > Gtk2KeyBindingsHandler::MatchEvent(). > > Hmm, if TextEditKeyBindingDelegateAuraLinux::MatchEvent() matches IDC_EXIT or > IDC_FULLSCREEN as a user-registered content editing hotkey, wouldn't that mean > that users could get stuck in fullscreen? That would be troubling; we should > know if it's a real possibility before landing this change. Can you investigate > that so we have a clear answer? If Dominick says this is fine as-is, I can give > you a rubber stamp. Emm, interesting. My gut feeling is no, since current implementation matches existing behavior for this scenario. The only change by this cl is delivering tab or window commands to the web page in full screen as described. I am still investigating the possibilities.
On 2017/02/11 00:50:50, Hzj_jie wrote: > On 2017/02/11 00:44:53, msw wrote: > > 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/brow... > > > > File chrome/browser/ui/browser_command_controller.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... > > > > chrome/browser/ui/browser_command_controller.cc:199: if > > > > (window()->IsFullscreen()) { > > > > On 2017/02/10 19:18:08, Hzj_jie wrote: > > > > > On 2017/02/10 02:03:39, msw wrote: > > > > > > Can we replace this block with: > > > > > > if (window()->IsFullscreen()) > > > > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > > > That seems to be a more concise implementation of the intended > behavior, > > > and > > > > > > matches what's described in the comment. (if so, you can revert the > > > > > > |is_tab_or_window_command| change) > > > > > > > > > > It looks like your solution changes the behavior on Linux. The following > > > > > ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing > ESC > > in > > > > > fullscreen. > > > > > > > > Can you elaborate? It looks like the Linux block only ever returns false, > > and > > > > afaict, MatchEvent() doesn't have any side-effects; so the only conditions > > > under > > > > which fullscreen would return true below line 190 is if the command is > > > > IDC_FULLSCREEN or IDC_EXIT. So, my suggested code should keep the intended > > > > behavior, right? Please let me know if I'm missing something. > > > > if (window()->IsFullscreen()) > > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > > > Sorry about lacking of knowledge about TextEditKeyBindingDelegateAuraLinux. > > But > > > I see two potential issues with the change you suggested. > > > 1. When command_id is IDC_EXIT. In your change, this function always returns > > > true in fullscreen. But current implementation may return false if > > > TextEditKeyBindingDelegateAuraLinux::MatchEvent() returns true. This > behavior > > is > > > consistent with the version without this patch. > > > 2. I have no idea what TextEditKeyBindingDelegateAuraLinux::MatchEvent() > will > > > do, but it's not a const function, which means it may have side effect. Say, > > > Gtk2KeyBindingsHandler::MatchEvent(). > > > > Hmm, if TextEditKeyBindingDelegateAuraLinux::MatchEvent() matches IDC_EXIT or > > IDC_FULLSCREEN as a user-registered content editing hotkey, wouldn't that mean > > that users could get stuck in fullscreen? That would be troubling; we should > > know if it's a real possibility before landing this change. Can you > investigate > > that so we have a clear answer? If Dominick says this is fine as-is, I can > give > > you a rubber stamp. > > Emm, interesting. My gut feeling is no, since current implementation matches > existing behavior for this scenario. The only change by this cl is delivering > tab or window commands to the web page in full screen as described. > > I am still investigating the possibilities. Ah, that's true, this doesn't seem to introduce any other behavior changes than what's intended. In that case, LGTM. But it would be nice to check for a safeguard, and if it is indeed impossible for IDC_EXIT and IDC_FULLSCREEN to be registered as content editing hotkeys, then simplifying the code would be nice. Thanks for being thorough and bearing with me.
On 2017/02/11 01:50:57, msw wrote: > On 2017/02/11 00:50:50, Hzj_jie wrote: > > On 2017/02/11 00:44:53, msw wrote: > > > 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/brow... > > > > > File chrome/browser/ui/browser_command_controller.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2636013002/diff/100001/chrome/browser/ui/brow... > > > > > chrome/browser/ui/browser_command_controller.cc:199: if > > > > > (window()->IsFullscreen()) { > > > > > On 2017/02/10 19:18:08, Hzj_jie wrote: > > > > > > On 2017/02/10 02:03:39, msw wrote: > > > > > > > Can we replace this block with: > > > > > > > if (window()->IsFullscreen()) > > > > > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > > > > That seems to be a more concise implementation of the intended > > behavior, > > > > and > > > > > > > matches what's described in the comment. (if so, you can revert the > > > > > > > |is_tab_or_window_command| change) > > > > > > > > > > > > It looks like your solution changes the behavior on Linux. The > following > > > > > > ui::TextEditKeyBindingDelegateAuraLinux will be ignored when pressing > > ESC > > > in > > > > > > fullscreen. > > > > > > > > > > Can you elaborate? It looks like the Linux block only ever returns > false, > > > and > > > > > afaict, MatchEvent() doesn't have any side-effects; so the only > conditions > > > > under > > > > > which fullscreen would return true below line 190 is if the command is > > > > > IDC_FULLSCREEN or IDC_EXIT. So, my suggested code should keep the > intended > > > > > behavior, right? Please let me know if I'm missing something. > > > > > if (window()->IsFullscreen()) > > > > > return command_id == IDC_FULLSCREEN || command_id == IDC_EXIT; > > > > > > > > Sorry about lacking of knowledge about > TextEditKeyBindingDelegateAuraLinux. > > > But > > > > I see two potential issues with the change you suggested. > > > > 1. When command_id is IDC_EXIT. In your change, this function always > returns > > > > true in fullscreen. But current implementation may return false if > > > > TextEditKeyBindingDelegateAuraLinux::MatchEvent() returns true. This > > behavior > > > is > > > > consistent with the version without this patch. > > > > 2. I have no idea what TextEditKeyBindingDelegateAuraLinux::MatchEvent() > > will > > > > do, but it's not a const function, which means it may have side effect. > Say, > > > > Gtk2KeyBindingsHandler::MatchEvent(). > > > > > > Hmm, if TextEditKeyBindingDelegateAuraLinux::MatchEvent() matches IDC_EXIT > or > > > IDC_FULLSCREEN as a user-registered content editing hotkey, wouldn't that > mean > > > that users could get stuck in fullscreen? That would be troubling; we should > > > know if it's a real possibility before landing this change. Can you > > investigate > > > that so we have a clear answer? If Dominick says this is fine as-is, I can > > give > > > you a rubber stamp. > > > > Emm, interesting. My gut feeling is no, since current implementation matches > > existing behavior for this scenario. The only change by this cl is delivering > > tab or window commands to the web page in full screen as described. > > > > I am still investigating the possibilities. > > Ah, that's true, this doesn't seem to introduce any other behavior changes than > what's intended. In that case, LGTM. But it would be nice to check for a > safeguard, and if it is indeed impossible for IDC_EXIT and IDC_FULLSCREEN to be > registered as content editing hotkeys, then simplifying the code would be nice. > Thanks for being thorough and bearing with me. It looks like Gtk2KeyBindingsHandler referred by GtkUi is the only implementation of TextEditKeyBindingsDelegateAuraLinux. But I cannot quite understand its logic, AFAICT, it always returns false, which looks like a bug to me. The logic was written by Elliot (erg@). I will talk with him. Hopefully he can still remember the logic, which is almost three years old. Otherwise, it should be safe to assume TextEditKeyBindingsDelegateAuraLinux::MatchEvent() won't return true if command_id is IDC_EXIT. To ensure we are safe, I will submit the change as-is, and talk with Elliot before making any further change.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2636013002/#ps120001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486930501950060, "parent_rev": "1a8d5f3805dbae5242cf7dce14df3807344dae37", "commit_rev": "3c7af99a93f4b4837b2fbee5cb66697f66ccf241"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/3c7af99a93f4b4837b2fbee5cb66... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3c7af99a93f4b4837b2fbee5cb66... |