Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532933002:
predictors: Remove unused field trial configuration parsing. (Closed)
Descriptionpredictors: Remove unused field trial configuration parsing.
The code is unused, will likely not come back in the same form, and
forces needless verbosity in forthcoming CLs.
BUG=631966
Committed: https://crrev.com/47c7372e5364d228656e61e79fb1ed91a4e3a4d3
Cr-Commit-Position: refs/heads/master@{#434694}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 10
Patch Set 4 : Address comments. #
Messages
Total messages: 25 (16 generated)
The CQ bit was checked by lizeb@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...
lizeb@chromium.org changed reviewers: + pasko@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lizeb@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...
ignoring for now as per offline request from lizeb@, let me know when to look again
On 2016/11/28 15:07:03, pasko wrote: > ignoring for now as per offline request from lizeb@, let me know when to look > again PTAL, thanks !
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a bunch of nits and suggestions for later, feel free to do them now if you want, but then ask for another look thanks https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:20: using std::string; nit: the only line using it is going away with this change :) https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:21: using std::vector; not needed? https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common_unittest.cc (right): https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common_unittest.cc:104: TEST_F(ResourcePrefetchCommonTest, IsDisabledByDefault) { On related note: I am not sure the clutter around these BlahEnabled() functions is worth the benefits. Testing the string parsing separately may be better. For example, there is ResourcePrefetchPredictorConfig::IsLearningEnabled() that is used in two places: 1. tests (7 mentions) 2. DCHECK (1 mention) worth it? If you feel the same way, let's nuke these too? (no pressure though, can certainly wait until maintaining them becomes more of a burden) https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common_unittest.cc:108: TestIsPrefetchDisabled(config); This is becoming the single callsite for this function, which makes the purpose of the function unclear. Let's inline such functions in the tests themselves? https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common_unittest.cc:129: // Verifies whether prefetching in the field trial is disabled according to Should we remove the mention of the field trial?
Thanks! PTAL https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:20: using std::string; On 2016/11/28 16:31:13, pasko wrote: > nit: the only line using it is going away with this change :) Done. https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:21: using std::vector; On 2016/11/28 16:31:13, pasko wrote: > not needed? Done. https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common_unittest.cc (right): https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common_unittest.cc:104: TEST_F(ResourcePrefetchCommonTest, IsDisabledByDefault) { On 2016/11/28 16:31:13, pasko wrote: > On related note: I am not sure the clutter around these BlahEnabled() functions > is worth the benefits. Testing the string parsing separately may be better. > > For example, there is ResourcePrefetchPredictorConfig::IsLearningEnabled() that > is used in two places: > 1. tests (7 mentions) > 2. DCHECK (1 mention) > > worth it? > > If you feel the same way, let's nuke these too? (no pressure though, can > certainly wait until maintaining them becomes more of a burden) This will become a bit more complex with the external only mode being added later on, so let's keep these for now. https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common_unittest.cc:108: TestIsPrefetchDisabled(config); On 2016/11/28 16:31:13, pasko wrote: > This is becoming the single callsite for this function, which makes the purpose > of the function unclear. Let's inline such functions in the tests themselves? Done. https://codereview.chromium.org/2532933002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common_unittest.cc:129: // Verifies whether prefetching in the field trial is disabled according to On 2016/11/28 16:31:13, pasko wrote: > Should we remove the mention of the field trial? Done.
The CQ bit was checked by lizeb@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lizeb@chromium.org
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": 60001, "attempt_start_ts": 1480356716238440,
"parent_rev": "315d5cb13b99a98213790029540d88332425d320", "commit_rev":
"a73b0bd4e5bfe127f0ad68a7cd87c59a36a2ae32"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Remove unused field trial configuration parsing. The code is unused, will likely not come back in the same form, and forces needless verbosity in forthcoming CLs. BUG=631966 ========== to ========== predictors: Remove unused field trial configuration parsing. The code is unused, will likely not come back in the same form, and forces needless verbosity in forthcoming CLs. BUG=631966 Committed: https://crrev.com/47c7372e5364d228656e61e79fb1ed91a4e3a4d3 Cr-Commit-Position: refs/heads/master@{#434694} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/47c7372e5364d228656e61e79fb1ed91a4e3a4d3 Cr-Commit-Position: refs/heads/master@{#434694} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
