Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865793002:
Remove the BackspaceGoesBack flag. (Closed)
DescriptionRemove the BackspaceGoesBack flag.
This has shipped for months and won't be rolled back. We don't
need to keep this code around.
BUG=699133
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2865793002
Cr-Commit-Position: refs/heads/master@{#475637}
Committed: https://chromium.googlesource.com/chromium/src/+/0fe1505aa01556438f01db7850034050509cbbc1
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove unused includes. #Patch Set 3 : merge to ToT #
Total comments: 4
Patch Set 4 : remove some more unused includes #Patch Set 5 : Add TODO #
Total comments: 6
Patch Set 6 : Remove backspace from keyboard overlay #Patch Set 7 : Remove backspace from keyboard overlay #Patch Set 8 : Fix alt keyboard shortcuts #Patch Set 9 : merge to trunk #
Messages
Total messages: 43 (24 generated)
Description was changed from ========== Remove the BackspaceGoesBack flag. This has shipped for months and won't be rolled back. We don't need to keep this code around. BUG=699133 ========== to ========== Remove the BackspaceGoesBack flag. This has shipped for months and won't be rolled back. We don't need to keep this code around. BUG=699133 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by ojan@google.com 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.
ojan@google.com changed reviewers: + pkasting@chromium.org
ojan@google.com changed reviewers: - ojan@google.com
LGTM https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/keyboard_overlay.js:231: delete shortcutDataCache['backspace<>SHIFT']; Do we need these lines anymore? https://codereview.chromium.org/2865793002/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_window_views_browsertest.cc (left): https://codereview.chromium.org/2865793002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_window_views_browsertest.cc:206: scoped_feature_list.InitAndEnableFeature(features::kBackspaceGoesBackFeature); Looks like we can remove some #includes if we remove this block, e.g. scoped_feature_list.h
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2865793002/#ps40001 (title: "merge to ToT")
https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/keyboard_overlay.js:231: delete shortcutDataCache['backspace<>SHIFT']; On 2017/05/06 at 04:56:13, Peter Kasting wrote: > Do we need these lines anymore? I don't know. I was just maintaining behavior with the flag off. https://codereview.chromium.org/2865793002/diff/1/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_window_views_browsertest.cc (left): https://codereview.chromium.org/2865793002/diff/1/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_window_views_browsertest.cc:206: scoped_feature_list.InitAndEnableFeature(features::kBackspaceGoesBackFeature); On 2017/05/06 at 04:56:13, Peter Kasting wrote: > Looks like we can remove some #includes if we remove this block, e.g. scoped_feature_list.h Done
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ojan@chromium.org changed reviewers: + michaelpg@chromium.org
Michael, can you take a look at the chromeos change here. a) I need your owners approval and b) maybe you can help answer Peter's question on whether we can just delete these two lines.
https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/keyboard_overlay.js:231: delete shortcutDataCache['backspace<>SHIFT']; On 2017/05/10 18:07:06, ojan wrote: > On 2017/05/06 at 04:56:13, Peter Kasting wrote: > > Do we need these lines anymore? > > I don't know. I was just maintaining behavior with the flag off. shortcutDataCache starts with these keys populated, so yes, the CL as-is needs these lines. (If you remove these lines and build, you'll see that the overlay shows the removed shortcuts again. You can also set a breakpoint here at chrome://keyboardoverlay to see the existing values at these keys.) We should remove them, though, by removing the same keys from the data it's drawing from. shortcutDataCache comes from: https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/keyboa... which says it's a generated file -- I'd look there to see how to remove these entries. (BTW, because the equivalent alt+arrow shortcuts have the same strings, you don't need to worry about removing them.) https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:312: GoBack(browser_, disposition); can remove chrome_features.h #include here, too https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (left): https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:321: base::FeatureList::IsEnabled(features::kBackspaceGoesBackFeature)); Looks like you can remove some #include's, as there are no other uses of the features namespace or base::FeatureList in this file: - chrome_features.h - feature_list.h Check the other files in this CL for the same thing.
On 2017/05/10 at 23:41:51, michaelpg wrote: > https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/keyboard_overlay.js (right): > > https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/keyboard_overlay.js:231: delete shortcutDataCache['backspace<>SHIFT']; > On 2017/05/10 18:07:06, ojan wrote: > > On 2017/05/06 at 04:56:13, Peter Kasting wrote: > > > Do we need these lines anymore? > > > > I don't know. I was just maintaining behavior with the flag off. > > shortcutDataCache starts with these keys populated, so yes, the CL as-is needs these lines. (If you remove these lines and build, you'll see that the overlay shows the removed shortcuts again. You can also set a breakpoint here at chrome://keyboardoverlay to see the existing values at these keys.) > > We should remove them, though, by removing the same keys from the data it's drawing from. shortcutDataCache comes from: > > https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/keyboa... > > which says it's a generated file -- I'd look there to see how to remove these entries. > > (BTW, because the equivalent alt+arrow shortcuts have the same strings, you don't need to worry about removing them.) I can't figure out how to run this script and there doesn't seem to be any documentation or OWNERS. I put a TODO and filed crbug.com/721141 for now. SG? https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:312: GoBack(browser_, disposition); On 2017/05/10 at 23:41:51, michaelpg wrote: > can remove chrome_features.h #include here, too done https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (left): https://codereview.chromium.org/2865793002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:321: base::FeatureList::IsEnabled(features::kBackspaceGoesBackFeature)); On 2017/05/10 at 23:41:51, michaelpg wrote: > Looks like you can remove some #include's, as there are no other uses of the features namespace or base::FeatureList in this file: > - chrome_features.h > - feature_list.h > > Check the other files in this CL for the same thing. Pretty sure you caught the two files left that I missed. Sorry about that.
The CQ bit was checked by ojan@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 ========== Remove the BackspaceGoesBack flag. This has shipped for months and won't be rolled back. We don't need to keep this code around. BUG=699133 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Remove the BackspaceGoesBack flag. This has shipped for months and won't be rolled back. We don't need to keep this code around. BUG=699133 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
afakhry: Do you know how to remove entries from the generated keyboard overlay data file, so we can remove these exceptions? https://codereview.chromium.org/2865793002/diff/1/chrome/browser/resources/ch... I found: https://sites.google.com/a/google.com/chromeos-team-tok/past-projects/ui/keyb... but neither of my accounts has access to the hotkey spreadsheet. Would it be evil to just delete those two entries from the JS data file manually?
afakhry@chromium.org changed reviewers: + afakhry@chromium.org
Re: michaelpg: We've been manually modifying keyboard_overlay_data.js for a more than a couple of years already, so no it's not evil. https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/keyboard_overlay.js:234: delete shortcutDataCache['backspace<>SHIFT']; Please remove these lines as well as the corresponding entries in keyboard_overlay_data.js. https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right): https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:182: {"keyboardOverlayGoForward", IDS_KEYBOARD_OVERLAY_GO_FORWARD}, Please remove the above two entries if you don't need them anymore. Also remove the corresponding IDS_* from the chromeos_strings.grdp
On 2017/05/11 01:19:35, afakhry wrote: > Re: michaelpg: We've been manually modifying keyboard_overlay_data.js for a more > than a couple of years already, so no it's not evil. Thanks! Ojan, can you go ahead and remove those entries from the data file and the corresponding exceptions, instead of adding that TODO?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Removed the keyboard overlay bits. I don't really know how to test this though. chrome:keyboardoverlay doesn't do anything for me in a default Chrome build. Also, as a followup, should we remove the line about he keyboard_overlay_data.js file being generated and delete the script? Presumably someone trying to run that script would then override all the manual changes that have been made in the past couple years? https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/keyboard_overlay.js:234: delete shortcutDataCache['backspace<>SHIFT']; On 2017/05/11 at 01:19:35, afakhry wrote: > Please remove these lines as well as the corresponding entries in keyboard_overlay_data.js. done https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right): https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:182: {"keyboardOverlayGoForward", IDS_KEYBOARD_OVERLAY_GO_FORWARD}, On 2017/05/11 at 01:19:35, afakhry wrote: > Please remove the above two entries if you don't need them anymore. > Also remove the corresponding IDS_* from the chromeos_strings.grdp Done
On 2017/05/11 18:25:25, ojan wrote: > Removed the keyboard overlay bits. I don't really know how to test this though. > chrome:keyboardoverlay doesn't do anything for me in a default Chrome build. Building with chromeos = true and running ./out/Release/chrome --user-data-dir=/tmp/foo should be sufficient. Can you try this, to confirm that what you have in this patchset gives you errors, and then try again after fixing? Ctrl+Alt+? will also show the keyboard overlay in chromeos builds, but you'll need to use remote debugging (you should still see the JS errors logged to the console, though, I think). > > Also, as a followup, should we remove the line about he keyboard_overlay_data.js > file being generated and delete the script? Presumably someone trying to run > that script would then override all the manual changes that have been made in > the past couple years? Yeah. Or add a TODO explaining the situation. https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right): https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:182: {"keyboardOverlayGoForward", IDS_KEYBOARD_OVERLAY_GO_FORWARD}, On 2017/05/11 01:19:35, afakhry wrote: > Please remove the above two entries if you don't need them anymore. > Also remove the corresponding IDS_* from the chromeos_strings.grdp (These strings are still used by the alt keys, so they should stay.) https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:182: {"keyboardOverlayGoForward", IDS_KEYBOARD_OVERLAY_GO_FORWARD}, On 2017/05/11 18:25:25, ojan wrote: > On 2017/05/11 at 01:19:35, afakhry wrote: > > Please remove the above two entries if you don't need them anymore. > > Also remove the corresponding IDS_* from the chromeos_strings.grdp > > Done Oops, my drafts didn't get published. Ojan: these are still used, grep keyboard_overlay_data.js for 'keyboardOverlayGoBack' and 'keyboardOverlayGoForward'.
OK. All fixed and actually manually tested this time. On 2017/05/11 at 19:10:14, michaelpg wrote: > On 2017/05/11 18:25:25, ojan wrote: > > Removed the keyboard overlay bits. I don't really know how to test this though. > > chrome:keyboardoverlay doesn't do anything for me in a default Chrome build. > > Building with chromeos = true I think you meant target_os="chromeos". :) It's easier to do this properly now that I know how to actually verify correctness of the code I'm modifying. Thanks for being patient. I've somehow never had to build or test chromeos before. > https://codereview.chromium.org/2865793002/diff/80001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:182: {"keyboardOverlayGoForward", IDS_KEYBOARD_OVERLAY_GO_FORWARD}, > On 2017/05/11 18:25:25, ojan wrote: > > On 2017/05/11 at 01:19:35, afakhry wrote: > > > Please remove the above two entries if you don't need them anymore. > > > Also remove the corresponding IDS_* from the chromeos_strings.grdp > > > > Done > > Oops, my drafts didn't get published. Ojan: these are still used, grep keyboard_overlay_data.js for 'keyboardOverlayGoBack' and 'keyboardOverlayGoForward'. I did actually code search for these before removing them but didn't notice there were two entries in this file. Sorry for being sloppy.
LGTM, thanks Ojan
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2865793002/#ps140001 (title: "Fix alt keyboard shortcuts")
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by ojan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2865793002/#ps160001 (title: "merge to trunk")
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": 160001, "attempt_start_ts": 1496165965621590,
"parent_rev": "0c645f037a2eb0950b6447c206c6fdda3e6bf32a", "commit_rev":
"0fe1505aa01556438f01db7850034050509cbbc1"}
Message was sent while issue was closed.
Description was changed from ========== Remove the BackspaceGoesBack flag. This has shipped for months and won't be rolled back. We don't need to keep this code around. BUG=699133 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Remove the BackspaceGoesBack flag. This has shipped for months and won't be rolled back. We don't need to keep this code around. BUG=699133 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2865793002 Cr-Commit-Position: refs/heads/master@{#475637} Committed: https://chromium.googlesource.com/chromium/src/+/0fe1505aa01556438f01db785003... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0fe1505aa01556438f01db785003... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
