Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1992003002:
Re-add backspace-goes-back as a default disabled finch trial (Closed)
DescriptionRe-add backspace-goes-back as a default disabled finch trial.
This is just so that we have a kill switch in case this makes it
to stable and there is then unexpected outcry.
BUG=610039
Committed: https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df
Cr-Commit-Position: refs/heads/master@{#395141}
Patch Set 1 #Patch Set 2 : fix assert in test #
Total comments: 16
Patch Set 3 : address revieww comments #
Messages
Total messages: 33 (14 generated)
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/patch-status/1992003002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992003002/1
Description was changed from ========== fix formatting now with tests make it a finch trial make backspace go back with new enum ========== to ========== Readd backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG=610039 ==========
The CQ bit was unchecked by ojan@chromium.org
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/patch-status/1992003002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992003002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
ojan@chromium.org changed reviewers: + mpearson@chromium.org, pkasting@chromium.org
I didn't get a finch ambassador for this, but I think it might not be necessary since we're not really doing an experiment here so much as using finch as a way of disabling the feature quickly if twitter blows up. :) Also, I've never done a finch trial before, so I may be doing this all wrong. :)
drive-by https://codereview.chromium.org/1992003002/diff/20001/chrome/app/chrome_comma... File chrome/app/chrome_command_ids.h (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/app/chrome_comma... chrome/app/chrome_command_ids.h:36: #define IDC_BACKSPACE_BACK 33010 Do these need to be added to histograms.xml?
LGTM except for one question https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:333: break; Nit: Slightly shorter, and abides by the comment about keeping the command order exactly matching the function declaration order: case IDC_BACKSPACE_BACK: if (!base::FeatureList::IsEnabled(kBackspaceGoesBackFeature)) break; case IDC_BACK: GoBack(browser_, disposition); break; (& similar) https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:996: command_updater_.UpdateCommandEnabled(IDC_BACKSPACE_FORWARD, Nit: Swap this with next line to keep the backs/forwards together https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_window_views_browsertest.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views_browsertest.cc:190: // Pressing backspace should not navigate back and close the dialog. Nit: "... with the Finch flag disabled"? https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views_browsertest.cc:205: // Pressing backspace should navigate back and close the dialog with the Nit: I'd move this comment up above the previous block, since that's related to what it's talking about. finch -> Finch https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views_browsertest.cc:213: EXPECT_EQ(NULL, dialog->GetWidget()); Nit: nullptr https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:2436: content::RecordAction(UserMetricsAction("Accel_Back_Backspace")); Will this and the bit below even be reached? Won't these trigger IDC_BACKSPACE_[BACK,FORWARD] instead of these command IDs?
Use of base::FeatureList looks fine to me. I assume you tested this change using --enable-features=... and --disable-features=... --mark
The CQ bit was checked by ojan@chromium.org
On 2016/05/19 at 22:49:03, mpearson wrote: > Use of base::FeatureList looks fine to me. > I assume you tested this change using --enable-features=... and --disable-features=... Yup. https://codereview.chromium.org/1992003002/diff/20001/chrome/app/chrome_comma... File chrome/app/chrome_command_ids.h (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/app/chrome_comma... chrome/app/chrome_command_ids.h:36: #define IDC_BACKSPACE_BACK 33010 On 2016/05/19 at 02:04:33, Matt Giuca wrote: > Do these need to be added to histograms.xml? We're not planning to measure anything here. So, I think no? https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:333: break; On 2016/05/19 at 09:28:30, Peter Kasting wrote: > Nit: Slightly shorter, and abides by the comment about keeping the command order exactly matching the function declaration order: > > case IDC_BACKSPACE_BACK: > if (!base::FeatureList::IsEnabled(kBackspaceGoesBackFeature)) > break; > case IDC_BACK: > GoBack(browser_, disposition); > break; > > (& similar) done https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:996: command_updater_.UpdateCommandEnabled(IDC_BACKSPACE_FORWARD, On 2016/05/19 at 09:28:30, Peter Kasting wrote: > Nit: Swap this with next line to keep the backs/forwards together done https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/constrained_window_views_browsertest.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views_browsertest.cc:190: // Pressing backspace should not navigate back and close the dialog. On 2016/05/19 at 09:28:30, Peter Kasting wrote: > Nit: "... with the Finch flag disabled"? done https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views_browsertest.cc:205: // Pressing backspace should navigate back and close the dialog with the On 2016/05/19 at 09:28:30, Peter Kasting wrote: > Nit: I'd move this comment up above the previous block, since that's related to what it's talking about. > > finch -> Finch done https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/constrained_window_views_browsertest.cc:213: EXPECT_EQ(NULL, dialog->GetWidget()); On 2016/05/19 at 09:28:30, Peter Kasting wrote: > Nit: nullptr done https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:2436: content::RecordAction(UserMetricsAction("Accel_Back_Backspace")); On 2016/05/19 at 09:28:30, Peter Kasting wrote: > Will this and the bit below even be reached? Won't these trigger IDC_BACKSPACE_[BACK,FORWARD] instead of these command IDs? Ugh. Yeah. This just just from starting with a revert of the previous patch. Was slopping in cleaning it up apparently. Removed.
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/1992003002/#ps40001 (title: "address revieww comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1992003002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992003002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ojan@chromium.org changed reviewers: + avi@chromium.org, sky@chromium.org
sky/avi can you both OWNERS LGTM. avi: for the .mm files sky: for chrome/app/chrome_command_ids.h and chrome/browser/app_mode/app_mode_utils.cc
Description was changed from ========== Readd backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG=610039 ========== to ========== Re-add backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG=610039 ==========
lgtm
LGTM
The CQ bit was checked by ojan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1992003002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992003002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Re-add backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG=610039 ========== to ========== Re-add backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG=610039 Committed: https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df Cr-Commit-Position: refs/heads/master@{#395141} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df Cr-Commit-Position: refs/heads/master@{#395141}
Message was sent while issue was closed.
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:333: break; On 2016/05/20 16:33:48, ojan wrote: > On 2016/05/19 at 09:28:30, Peter Kasting wrote: > > Nit: Slightly shorter, and abides by the comment about keeping the command > order exactly matching the function declaration order: > > > > case IDC_BACKSPACE_BACK: > > if (!base::FeatureList::IsEnabled(kBackspaceGoesBackFeature)) > > break; > > case IDC_BACK: > > GoBack(browser_, disposition); > > break; > > > > (& similar) > > done Um, this is kind of gross and I'm going to have to put it back the way it was in my follow-up CL (which adds an else clause)... I have never seen a conditional fall-through before.
Message was sent while issue was closed.
https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:333: break; On 2016/05/23 01:12:01, Matt Giuca wrote: > On 2016/05/20 16:33:48, ojan wrote: > > On 2016/05/19 at 09:28:30, Peter Kasting wrote: > > > Nit: Slightly shorter, and abides by the comment about keeping the command > > order exactly matching the function declaration order: > > > > > > case IDC_BACKSPACE_BACK: > > > if (!base::FeatureList::IsEnabled(kBackspaceGoesBackFeature)) > > > break; > > > case IDC_BACK: > > > GoBack(browser_, disposition); > > > break; > > > > > > (& similar) > > > > done > > Um, this is kind of gross and I'm going to have to put it back the way it was in > my follow-up CL (which adds an else clause)... I have never seen a conditional > fall-through before. Sorry, actually I can keep the conditional fall-through if it's desired. Seems bad though. Will discuss on my follow-up CL https://codereview.chromium.org/1983803002/
Message was sent while issue was closed.
On 2016/05/23 at 01:15:40, mgiuca wrote: > https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_command_controller.cc (right): > > https://codereview.chromium.org/1992003002/diff/20001/chrome/browser/ui/brows... > chrome/browser/ui/browser_command_controller.cc:333: break; > On 2016/05/23 01:12:01, Matt Giuca wrote: > > On 2016/05/20 16:33:48, ojan wrote: > > > On 2016/05/19 at 09:28:30, Peter Kasting wrote: > > > > Nit: Slightly shorter, and abides by the comment about keeping the command > > > order exactly matching the function declaration order: > > > > > > > > case IDC_BACKSPACE_BACK: > > > > if (!base::FeatureList::IsEnabled(kBackspaceGoesBackFeature)) > > > > break; > > > > case IDC_BACK: > > > > GoBack(browser_, disposition); > > > > break; > > > > > > > > (& similar) > > > > > > done > > > > Um, this is kind of gross and I'm going to have to put it back the way it was in > > my follow-up CL (which adds an else clause)... I have never seen a conditional > > fall-through before. > > Sorry, actually I can keep the conditional fall-through if it's desired. Seems bad though. Will discuss on my follow-up CL https://codereview.chromium.org/1983803002/ Heh. I agree FWIW. I find conditional fall through to be error prone and ugly, but I didn't really feel like arguing about it given that I expect we'll delete this code entirely in a few releases. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
