Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689383002:
Commands should have consistent behaviors across different modes
DescriptionCommands should have a consistent behavior across different modes
Change https://chromium.googlesource.com/chromium/src/+/3c7af99a93f4b4837b2fbee5cb66697f66ccf241 makes command_updater_ updates in inappropriate
modes.
BUG=691677
Patch Set 1 #
Total comments: 14
Patch Set 2 : Resolve review comments #
Total comments: 8
Patch Set 3 : Resolve review comments #
Messages
Total messages: 58 (50 generated)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Commands should have a consistent behavior across different modes Change https://goo.gl/RM1oat makes command_updater_ updates in inappropriate modes. BUG=691677 ========== to ========== Commands should have a consistent behavior across different modes Change https://goo.gl/RM1oat makes command_updater_ updates in inappropriate modes. BUG=691677 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
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 checked by zijiehe@chromium.org to run a CQ dry run
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zijiehe@chromium.org changed reviewers: + dominickn@chromium.org, msw@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1500: menuState_.get(), lastProfile_, false); How do you know the relevant window is not fullscreen here? (ditto below) Should UpdateSharedCommandsForIncognitoAvailability be non-static and determine |is_fullscreen| internally, like UpdateCommandsForIncognitoAvailability? Or can we get the relevant browser and check that here (via chrome::FindLastActiveWithProfile, chrome::GetLastActiveBrowser(), or like menuNeedsUpdate)? You might want to ping a knowledgable cocoa reviewer; I don't know our Mac code too well. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:894: command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, false); aside: these two commands (IDC_IMPORT_SETTINGS and IDC_OPTIONS) are already redundantly/conflictingly set in UpdateSharedCommandsForIncognitoAvailability https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1063: command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled); These two commands (IDC_OPTIONS and IDC_IMPORT_SETTINGS) are already redundantly/conflictingly set in UpdateCommandsForIncognitoAvailability and UpdateSharedCommandsForIncognitoAvailability https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.h (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.h:73: // Update commands whose state depends on incognito mode availability and that nit: update comment https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_unittest.cc (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_unittest.cc:164: browser()->command_controller()->command_updater(), testprofile, false); If we keep this flag, we should have test coverage for passing true.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
zijiehe@chromium.org changed reviewers: + noms@chromium.org, thakis@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1500: menuState_.get(), lastProfile_, false); On 2017/02/14 22:34:50, msw wrote: > How do you know the relevant window is not fullscreen here? (ditto below) > Should UpdateSharedCommandsForIncognitoAvailability be non-static and determine > |is_fullscreen| internally, like UpdateCommandsForIncognitoAvailability? Or can > we get the relevant browser and check that here (via > chrome::FindLastActiveWithProfile, chrome::GetLastActiveBrowser(), or like > menuNeedsUpdate)? You might want to ping a knowledgable cocoa reviewer; I don't > know our Mac code too well. AFAIK, menuState_ here is the state of menu items. i.e. Even we do not reserve new-incognitor-window function, we still need this menu item to be available. I will definitely add the original owner of the logic into the review. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:894: command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, false); On 2017/02/14 22:34:50, msw wrote: > aside: these two commands (IDC_IMPORT_SETTINGS and IDC_OPTIONS) are already > redundantly/conflictingly set in UpdateSharedCommandsForIncognitoAvailability I think this is by design, since UpdateSharedCommandsForIncognitoAvailability() is also used in app_controller_mac.mm, it won't cover the IsShowingMainUI() scenario. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1063: command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled); On 2017/02/14 22:34:50, msw wrote: > These two commands (IDC_OPTIONS and IDC_IMPORT_SETTINGS) are already > redundantly/conflictingly set in UpdateCommandsForIncognitoAvailability and > UpdateSharedCommandsForIncognitoAvailability These several lines are duplicated with UpdateCommandsForIncognitoAvailability(), but have slightly difference: Whether IDC_OPTIONS should be enabled in guest_session. I tend to believe logic in UpdateSharedCommandsForIncognitoAvailability() is correct, since it matches test cases. I have removed these lines. Thank you for the reminding. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.h (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.h:73: // Update commands whose state depends on incognito mode availability and that On 2017/02/14 22:34:50, msw wrote: > nit: update comment Done. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_unittest.cc (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_unittest.cc:164: browser()->command_controller()->command_updater(), testprofile, false); On 2017/02/14 22:34:50, msw wrote: > If we keep this flag, we should have test coverage for passing true. Sure, more test cases are added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Please replace the url-shortener link in the CL description with the full code review link. Please don't use url-shorteners for chromium bugs or code reviews, unless it's really necessary. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1500: menuState_.get(), lastProfile_, false); On 2017/02/15 02:00:49, Hzj_jie wrote: > On 2017/02/14 22:34:50, msw wrote: > > How do you know the relevant window is not fullscreen here? (ditto below) > > Should UpdateSharedCommandsForIncognitoAvailability be non-static and > determine > > |is_fullscreen| internally, like UpdateCommandsForIncognitoAvailability? Or > can > > we get the relevant browser and check that here (via > > chrome::FindLastActiveWithProfile, chrome::GetLastActiveBrowser(), or like > > menuNeedsUpdate)? You might want to ping a knowledgable cocoa reviewer; I > don't > > know our Mac code too well. > > AFAIK, menuState_ here is the state of menu items. i.e. Even we do not reserve > new-incognitor-window function, we still need this menu item to be available. > I will definitely add the original owner of the logic into the review. Sorry, but I don't understand how your reply answers my primary question: How do you know the relevant window is not fullscreen here? This now always disables IDC_NEW_INCOGNITO_WINDOW and IDC_NEW_WINDOW... https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:894: command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, false); On 2017/02/15 02:00:49, Hzj_jie wrote: > On 2017/02/14 22:34:50, msw wrote: > > aside: these two commands (IDC_IMPORT_SETTINGS and IDC_OPTIONS) are already > > redundantly/conflictingly set in UpdateSharedCommandsForIncognitoAvailability > > I think this is by design, since UpdateSharedCommandsForIncognitoAvailability() > is also used in app_controller_mac.mm, it won't cover the IsShowingMainUI() > scenario. It seems like the pattern here is to call UpdateSharedCommandsForIncognitoAvailability without state information, and then use state information to disable some commands that would otherwise be enabled. For example, UpdateSharedCommandsForIncognitoAvailability might enable IDC_IMPORT_SETTINGS, and then this function would disable it if !IsShowingMainUI(). Perhaps we should do the same for the commands that need to be disabled during fullscreen. So, you should probably remove the |is_fullscreen| argument to UpdateSharedCommandsForIncognitoAvailability and at the end of this function do: if (window() && window()->IsFullscreen()) { command_updater->UpdateCommandEnabled(IDC_NEW_WINDOW, false); command_updater->UpdateCommandEnabled(IDC_NEW_INCOGNITO_WINDOW, false); } https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1058: command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled); Now that I'm looking closer, I'm not sure this is identical to line 883 + line 895. Line 883 enables IDC_OPTIONS in guest sessions, but this doesn't... You should seek a reviewer (or PM/UX) that knows the intended behavior. https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1059: command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, Similarly, this disables IDC_IMPORT_SETTINGS for guest sessions, but line 882 + line 894 would have potentially allowed this command to work for guest sessions... Please seek a more savvy reviewer. https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:859: !is_fullscreen && incognito_availability != IncognitoModePrefs::FORCED); I should have asked earlier, but do we actually need to enable and disable these commands when entering/exiting fullscreen? Is it possible that your earlier changes to IsReservedCommandOrKey would have been sufficient to let those accelerators be sent to the web content first? It might even be okay if they were handled as browser/tab commands if they weren't handled by the web content.
Description was changed from ========== Commands should have a consistent behavior across different modes Change https://goo.gl/RM1oat makes command_updater_ updates in inappropriate modes. BUG=691677 ========== to ========== Commands should have a consistent behavior across different modes Change https://chromium.googlesource.com/chromium/src/+/3c7af99a93f4b4837b2fbee5cb66... makes command_updater_ updates in inappropriate modes. BUG=691677 ==========
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 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 checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zijiehe@chromium.org changed reviewers: - noms@chromium.org, thakis@chromium.org
https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1500: menuState_.get(), lastProfile_, false); On 2017/02/15 18:40:51, msw wrote: > On 2017/02/15 02:00:49, Hzj_jie wrote: > > On 2017/02/14 22:34:50, msw wrote: > > > How do you know the relevant window is not fullscreen here? (ditto below) > > > Should UpdateSharedCommandsForIncognitoAvailability be non-static and > > determine > > > |is_fullscreen| internally, like UpdateCommandsForIncognitoAvailability? Or > > can > > > we get the relevant browser and check that here (via > > > chrome::FindLastActiveWithProfile, chrome::GetLastActiveBrowser(), or like > > > menuNeedsUpdate)? You might want to ping a knowledgable cocoa reviewer; I > > don't > > > know our Mac code too well. > > > > AFAIK, menuState_ here is the state of menu items. i.e. Even we do not reserve > > new-incognitor-window function, we still need this menu item to be available. > > I will definitely add the original owner of the logic into the review. > > Sorry, but I don't understand how your reply answers my primary question: > How do you know the relevant window is not fullscreen here? > This now always disables IDC_NEW_INCOGNITO_WINDOW and IDC_NEW_WINDOW... No, I do not know, and I do not think we are talking about a window here. When is_fullscreen is false, the logic is consistent with existing one. Monica (noms@) wrote this logic 2+ years ago, but she is not working on another project now. I am still looking for her replacement. But no matter, since I have reverted the change to UpdateSharedCommandsForIncognitoAvailability() function, we do not need to change this file anymore. https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:894: command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, false); On 2017/02/15 18:40:51, msw wrote: > On 2017/02/15 02:00:49, Hzj_jie wrote: > > On 2017/02/14 22:34:50, msw wrote: > > > aside: these two commands (IDC_IMPORT_SETTINGS and IDC_OPTIONS) are already > > > redundantly/conflictingly set in > UpdateSharedCommandsForIncognitoAvailability > > > > I think this is by design, since > UpdateSharedCommandsForIncognitoAvailability() > > is also used in app_controller_mac.mm, it won't cover the IsShowingMainUI() > > scenario. > > It seems like the pattern here is to call > UpdateSharedCommandsForIncognitoAvailability without state information, and then > use state information to disable some commands that would otherwise be enabled. > For example, UpdateSharedCommandsForIncognitoAvailability might enable > IDC_IMPORT_SETTINGS, and then this function would disable it if > !IsShowingMainUI(). Perhaps we should do the same for the commands that need to > be disabled during fullscreen. So, you should probably remove the > |is_fullscreen| argument to UpdateSharedCommandsForIncognitoAvailability and at > the end of this function do: > > if (window() && window()->IsFullscreen()) { > command_updater->UpdateCommandEnabled(IDC_NEW_WINDOW, false); > command_updater->UpdateCommandEnabled(IDC_NEW_INCOGNITO_WINDOW, false); > } Good suggestion. Thank you. https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1058: command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled); On 2017/02/15 18:40:51, msw wrote: > Now that I'm looking closer, I'm not sure this is identical to line 883 + line > 895. Line 883 enables IDC_OPTIONS in guest sessions, but this doesn't... You > should seek a reviewer (or PM/UX) that knows the intended behavior. Yes, as I have replied in the previous comment, we have a very tiny difference here. Personally I believe the one between line 880 & 890 is correct, since it matches the test cases. But, yes, I should find a PM/UX to make the decision. Do you happen to know who is the PM owner of this functionality? https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1059: command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, On 2017/02/15 18:40:51, msw wrote: > Similarly, this disables IDC_IMPORT_SETTINGS for guest sessions, but line 882 + > line 894 would have potentially allowed this command to work for guest > sessions... Please seek a more savvy reviewer. Ditto. https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:859: !is_fullscreen && incognito_availability != IncognitoModePrefs::FORCED); On 2017/02/15 18:40:51, msw wrote: > I should have asked earlier, but do we actually need to enable and disable these > commands when entering/exiting fullscreen? Is it possible that your earlier > changes to IsReservedCommandOrKey would have been sufficient to let those > accelerators be sent to the web content first? It might even be okay if they > were handled as browser/tab commands if they weren't handled by the web content. Yes, patch set 2 of change https://codereview.chromium.org/2636013002/#ps20001 is enough for the functionality in the description. The only concern is whether inconsistent states of CommandObserver and BrowserCommandController may introduce unexpected issues. If you think it should be fine. I am happy to revert the change to https://codereview.chromium.org/2636013002/#ps20001.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This looks much better, but I'm keen on pursuing a change that restores the old command-enabled behavior and simply returns false for IsReservedCommandOrKey. If that doesn't seem promising, let's make sure that IDC_OPTIONS and IDC_IMPORT_SETTINGS have the correct enabled state for forced-incognito and guest sessions (might be good to clean that up either way). https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:1058: command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled); On 2017/02/15 21:20:16, Hzj_jie wrote: > On 2017/02/15 18:40:51, msw wrote: > > Now that I'm looking closer, I'm not sure this is identical to line 883 + line > > 895. Line 883 enables IDC_OPTIONS in guest sessions, but this doesn't... You > > should seek a reviewer (or PM/UX) that knows the intended behavior. > > Yes, as I have replied in the previous comment, we have a very tiny difference > here. Personally I believe the one between line 880 & 890 is correct, since it > matches the test cases. > But, yes, I should find a PM/UX to make the decision. Do you happen to know who > is the PM owner of this functionality? If it matches the test expectations, then it's probably correct, but it would be good to follow up with ewald@ (CC'ed), he should hopefully be able to point you in the right direction. https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller.cc:859: !is_fullscreen && incognito_availability != IncognitoModePrefs::FORCED); On 2017/02/15 21:20:16, Hzj_jie wrote: > On 2017/02/15 18:40:51, msw wrote: > > I should have asked earlier, but do we actually need to enable and disable > these > > commands when entering/exiting fullscreen? Is it possible that your earlier > > changes to IsReservedCommandOrKey would have been sufficient to let those > > accelerators be sent to the web content first? It might even be okay if they > > were handled as browser/tab commands if they weren't handled by the web > content. > > Yes, patch set 2 of change https://codereview.chromium.org/2636013002/#ps20001 > is enough for the functionality in the description. The only concern is whether > inconsistent states of CommandObserver and BrowserCommandController may > introduce unexpected issues. > If you think it should be fine. I am happy to revert the change to > https://codereview.chromium.org/2636013002/#ps20001. My naive understanding is that it would be safe to leave the commands enabled, but return false for IsReservedCommandOrKey... it's worth looking closer, in order to simplify the code, and to match the intended behavior change (we don't necessarily want to disable new window creation in fullscreen, we simply want to offer the relevant input event handling to the fullscreen web content).
On 2017/02/15 22:38:19, msw wrote: > This looks much better, but I'm keen on pursuing a change that restores the old > command-enabled behavior and simply returns false for IsReservedCommandOrKey. If > that doesn't seem promising, let's make sure that IDC_OPTIONS and > IDC_IMPORT_SETTINGS have the correct enabled state for forced-incognito and > guest sessions (might be good to clean that up either way). > > https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... > File chrome/browser/ui/browser_command_controller.cc (left): > > https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... > chrome/browser/ui/browser_command_controller.cc:1058: > command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled); > On 2017/02/15 21:20:16, Hzj_jie wrote: > > On 2017/02/15 18:40:51, msw wrote: > > > Now that I'm looking closer, I'm not sure this is identical to line 883 + > line > > > 895. Line 883 enables IDC_OPTIONS in guest sessions, but this doesn't... You > > > should seek a reviewer (or PM/UX) that knows the intended behavior. > > > > Yes, as I have replied in the previous comment, we have a very tiny difference > > here. Personally I believe the one between line 880 & 890 is correct, since it > > matches the test cases. > > But, yes, I should find a PM/UX to make the decision. Do you happen to know > who > > is the PM owner of this functionality? > > If it matches the test expectations, then it's probably correct, but it would be > good to follow up with ewald@ (CC'ed), he should hopefully be able to point you > in the right direction. > > https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... > File chrome/browser/ui/browser_command_controller.cc (right): > > https://codereview.chromium.org/2689383002/diff/140001/chrome/browser/ui/brow... > chrome/browser/ui/browser_command_controller.cc:859: !is_fullscreen && > incognito_availability != IncognitoModePrefs::FORCED); > On 2017/02/15 21:20:16, Hzj_jie wrote: > > On 2017/02/15 18:40:51, msw wrote: > > > I should have asked earlier, but do we actually need to enable and disable > > these > > > commands when entering/exiting fullscreen? Is it possible that your earlier > > > changes to IsReservedCommandOrKey would have been sufficient to let those > > > accelerators be sent to the web content first? It might even be okay if they > > > were handled as browser/tab commands if they weren't handled by the web > > content. > > > > Yes, patch set 2 of change https://codereview.chromium.org/2636013002/#ps20001 > > is enough for the functionality in the description. The only concern is > whether > > inconsistent states of CommandObserver and BrowserCommandController may > > introduce unexpected issues. > > If you think it should be fine. I am happy to revert the change to > > https://codereview.chromium.org/2636013002/#ps20001. > > My naive understanding is that it would be safe to leave the commands enabled, > but return false for IsReservedCommandOrKey... it's worth looking closer, in > order to simplify the code, and to match the intended behavior change (we don't > necessarily want to disable new window creation in fullscreen, we simply want to > offer the relevant input event handling to the fullscreen web content). That's definitely a very safe change. I will follow up with Eli about the right behavior of the CommandUpdater. But before we finally make any decision, I will revert the changes to the https://codereview.chromium.org/2636013002/#ps20001.
This change is deprecated. A new one will be prepared soon or later to correct the inconsistency of IDC_OPTIONS and IDC_IMPORT_SETTINGS between UpdateCommandsForFullscreenMode() and UpdateSharedCommandsForIncognitoAvailability().
zijiehe@chromium.org changed reviewers: - dominickn@chromium.org, msw@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
