Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2965753002:
[Cleanup] Migrate the FileMetricsProvider to use the Task Scheduler. (Closed)
|
Created:
3 years, 5 months ago by Ilya Sherman Modified:
3 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Cleanup] Migrate the FileMetricsProvider to use the Task Scheduler.
BUG=667892
TEST=none
R=asvitkine@chromium.org, bcwhite@chromium.org
Review-Url: https://codereview.chromium.org/2965753002
Cr-Commit-Position: refs/heads/master@{#484813}
Committed: https://chromium.googlesource.com/chromium/src/+/69b1f42c4c8d2cd53f1c04c5a9df3d2ffa3d914f
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebase #Patch Set 3 : Skip on shutdown (rather than continue) #
Total comments: 2
Patch Set 4 : Change DCHECK syntax #
Messages
Total messages: 29 (17 generated)
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/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.
lgtm https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:109: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}); I don't think there's anything in here that needs "continue". They can all be "skip". https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:217: g_task_runner_for_testing = task_runner.get(); Delete the existing one if set.
Thanks! https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:109: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}); On 2017/07/04 14:50:31, bcwhite wrote: > I don't think there's anything in here that needs "continue". They can all be > "skip". Done. https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:217: g_task_runner_for_testing = task_runner.get(); On 2017/07/04 14:50:31, bcwhite wrote: > Delete the existing one if set. There's no ownership, so it doesn't make sense to delete anything. I did add a DCHECK, though, that this is never set twice in the same test.
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/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2965753002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:217: g_task_runner_for_testing = task_runner.get(); On 2017/07/05 18:15:56, Ilya Sherman wrote: > On 2017/07/04 14:50:31, bcwhite wrote: > > Delete the existing one if set. > > There's no ownership, so it doesn't make sense to delete anything. I did add a > DCHECK, though, that this is never set twice in the same test. Acknowledged. https://codereview.chromium.org/2965753002/diff/40001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2965753002/diff/40001/components/metrics/file... components/metrics/file_metrics_provider.cc:217: DCHECK(g_task_runner_for_testing == nullptr); DCHECK(!g_task_runner_for_testing);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2965753002/#ps60001 (title: "Change DCHECK syntax")
https://codereview.chromium.org/2965753002/diff/40001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2965753002/diff/40001/components/metrics/file... components/metrics/file_metrics_provider.cc:217: DCHECK(g_task_runner_for_testing == nullptr); On 2017/07/05 18:31:51, bcwhite wrote: > DCHECK(!g_task_runner_for_testing); 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: ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2965753002/#ps60001 (title: "Change DCHECK syntax")
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: 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 isherman@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": 1499387374255620,
"parent_rev": "43528ec69cbfa04acb0e67906d7b53e7bcd064e6", "commit_rev":
"69b1f42c4c8d2cd53f1c04c5a9df3d2ffa3d914f"}
Message was sent while issue was closed.
Description was changed from ========== [Cleanup] Migrate the FileMetricsProvider to use the Task Scheduler. BUG=667892 TEST=none R=asvitkine@chromium.org, bcwhite@chromium.org ========== to ========== [Cleanup] Migrate the FileMetricsProvider to use the Task Scheduler. BUG=667892 TEST=none R=asvitkine@chromium.org, bcwhite@chromium.org Review-Url: https://codereview.chromium.org/2965753002 Cr-Commit-Position: refs/heads/master@{#484813} Committed: https://chromium.googlesource.com/chromium/src/+/69b1f42c4c8d2cd53f1c04c5a9df... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/69b1f42c4c8d2cd53f1c04c5a9df... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
