Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1584733003:
[Cleanup] Remove some expired omnibox field trials. (Closed)
|
Created:
4 years, 11 months ago by Ilya Sherman Modified:
4 years, 11 months ago CC:
chromium-reviews, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cleanup] Remove some expired omnibox field trials.
BUG=none
TEST=none
R=mpearson@chromium.org
Committed: https://crrev.com/938488a71970e186877a608c3c4b5b6e8cb2bd06
Cr-Commit-Position: refs/heads/master@{#370734}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Only remove the dynamic trials #
Total comments: 4
Patch Set 3 : Remove no longer needed #includes #Patch Set 4 : Fix test compile #Patch Set 5 : Remove code paths from iOS as well #Patch Set 6 : Rebase #Patch Set 7 : Fix a unit test #
Messages
Total messages: 35 (14 generated)
This CL removes a couple of easy pickings. It would be nice if someone more familiar with the trials could check whether there are any other dead code paths that could be removed.
The dynamic trials can go. I'd prefer if the stop timer one did not. Note that you were too aggressive at removing code that relates to the dynamic trials, as described in changelist comments below. There are no other dead code paths. The dynamic trial was merely used to pipe chrome to send trials to the suggest server; it didn't affect any other local code. --mark https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/autocomplete_controller.cc:180: stop_timer_duration_(base::TimeDelta::FromMilliseconds(1500)), I wouldn't be surprised if we decide to experiment with this again. Is there any convincing reason to remove this lightweight code? https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... File components/omnibox/browser/base_search_provider.cc (left): https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/base_search_provider.cc:155: OmniboxFieldTrial::GetActiveSuggestFieldTrialHashes(&field_trial_hashes); This code cannot be removed because it's part of the omnibox bundled field trial. See my comment elsewhere. https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... File components/omnibox/browser/omnibox_field_trial.cc (left): https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/omnibox_field_trial.cc:144: void OmniboxFieldTrial::GetActiveSuggestFieldTrialHashes( The last part of this function still needs to remain because of the kBundledExperimentFieldTrialName, which is currently in use. The dynamic stuff can go. https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/omnibox_field_trial.cc:158: base::TimeDelta OmniboxFieldTrial::StopTimerFieldTrialDuration() { I'd prefer is this trial remains.
On 2016/01/14 05:31:02, Mark P wrote: > The dynamic trials can go. I'd prefer if the stop timer one did not. Note that > you were too aggressive at removing code that relates to the dynamic trials, as > described in changelist comments below. Thanks for catching my careless mistake, Mark! I had also forgotten to update the header file... oops. Now fixed. > There are no other dead code paths. The dynamic trial was merely used to pipe > chrome to send trials to the suggest server; it didn't affect any other local > code. By dead code paths, I meant code supporting any other obsolete field trials. https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/autocomplete_controller.cc:180: stop_timer_duration_(base::TimeDelta::FromMilliseconds(1500)), On 2016/01/14 05:31:02, Mark P wrote: > I wouldn't be surprised if we decide to experiment with this again. Is there > any convincing reason to remove this lightweight code? I'd argue that it's easy enough to restore, but I don't feel strongly either way. https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... File components/omnibox/browser/base_search_provider.cc (left): https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/base_search_provider.cc:155: OmniboxFieldTrial::GetActiveSuggestFieldTrialHashes(&field_trial_hashes); On 2016/01/14 05:31:02, Mark P wrote: > This code cannot be removed because it's part of the omnibox bundled field > trial. See my comment elsewhere. Done. https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... File components/omnibox/browser/omnibox_field_trial.cc (left): https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/omnibox_field_trial.cc:144: void OmniboxFieldTrial::GetActiveSuggestFieldTrialHashes( On 2016/01/14 05:31:02, Mark P wrote: > The last part of this function still needs to remain because of the > kBundledExperimentFieldTrialName, which is currently in use. The dynamic stuff > can go. Whoops! Thanks for catching that. https://codereview.chromium.org/1584733003/diff/1/components/omnibox/browser/... components/omnibox/browser/omnibox_field_trial.cc:158: base::TimeDelta OmniboxFieldTrial::StopTimerFieldTrialDuration() { On 2016/01/14 05:31:02, Mark P wrote: > I'd prefer is this trial remains. Done.
lgtm baring minor comments below On 2016/01/14 05:40:41, Ilya Sherman wrote: > > There are no other dead code paths. The dynamic trial was merely used to pipe > > chrome to send trials to the suggest server; it didn't affect any other local > > code. > > By dead code paths, I meant code supporting any other obsolete field trials. I'm not aware of any other obsolete field trials. --mark https://codereview.chromium.org/1584733003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/1584733003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:16: #include "components/omnibox/browser/omnibox_field_trial.h" This can probably be removed. https://codereview.chromium.org/1584733003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/1584733003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:15: #include "base/strings/stringprintf.h" Probably don't need this anymore.
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1584733003/#ps40001 (title: "Remove no longer needed #includes")
isherman@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei, PTAL at chrome/browser/chrome_browser_field_trials.cc https://codereview.chromium.org/1584733003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/1584733003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:16: #include "components/omnibox/browser/omnibox_field_trial.h" On 2016/01/14 18:38:47, Mark P wrote: > This can probably be removed. Done. https://codereview.chromium.org/1584733003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/1584733003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:15: #include "base/strings/stringprintf.h" On 2016/01/14 18:38:47, Mark P wrote: > Probably don't need this anymore. Done.
The CQ bit was unchecked by isherman@chromium.org
The CQ bit was checked by isherman@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/1584733003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1584733003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by isherman@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/1584733003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1584733003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
isherman@chromium.org changed reviewers: + rohitrao@chromium.org
Rohit, PTAL at the //ios change
The CQ bit was checked by isherman@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/1584733003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1584733003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chrome_browser_field_trials.cc lgtm
ios/ changes lgtm
Mark, it looks like the SearchProviderTest.FieldTrialTriggeredParsing test relied on the dynamic field trials being present. Should I remove the test, or somehow modify it to be based on a different field trial? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
On Wed, Jan 20, 2016 at 6:27 PM, <isherman@chromium.org> wrote: > Mark, it looks like the SearchProviderTest.FieldTrialTriggeredParsing test > relied on the dynamic field trials being present. Should I remove the > test, or > somehow modify it to be based on a different field trial? > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut... This is a test of behavior that's important for analysis. Can you keep the test? I _think_ it should pass if you active the omnibox bundled field trials before the test case starts (i.e., have at least one of the dynamic field trials triggered). --mark > > > https://codereview.chromium.org/1584733003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/21 04:46:56, Mark P wrote: > On Wed, Jan 20, 2016 at 6:27 PM, <mailto:isherman@chromium.org> wrote: > > > Mark, it looks like the SearchProviderTest.FieldTrialTriggeredParsing test > > relied on the dynamic field trials being present. Should I remove the > > test, or > > somehow modify it to be based on a different field trial? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut... > > > This is a test of behavior that's important for analysis. Can you keep the > test? I _think_ it should pass if you active the omnibox bundled field > trials before the test case starts (i.e., have at least one of the dynamic > field trials triggered). > > --mark Okay, I updated the test so that it once again passes. PTAL and let me know if the test changes look reasonable.
still lgtm
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/1584733003/#ps120001 (title: "Fix a unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1584733003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1584733003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Cleanup] Remove some expired omnibox field trials. BUG=none TEST=none R=mpearson@chromium.org ========== to ========== [Cleanup] Remove some expired omnibox field trials. BUG=none TEST=none R=mpearson@chromium.org Committed: https://crrev.com/938488a71970e186877a608c3c4b5b6e8cb2bd06 Cr-Commit-Position: refs/heads/master@{#370734} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/938488a71970e186877a608c3c4b5b6e8cb2bd06 Cr-Commit-Position: refs/heads/master@{#370734} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
