|
|
DescriptionUse task runner in version_updater_win.cc.
BUG=667892
Review-Url: https://codereview.chromium.org/2952133002
Cr-Commit-Position: refs/heads/master@{#488160}
Committed: https://chromium.googlesource.com/chromium/src/+/c59edd83ceaeb3b0c4fb8dc62281eb57512dcd60
Patch Set 1 : #
Total comments: 6
Patch Set 2 : address easy comments #Patch Set 3 : remove task runner injection #
Total comments: 4
Patch Set 4 : fix tests #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : address comments #
Total comments: 2
Patch Set 7 : fix nit #
Total comments: 1
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by calamity@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.
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
calamity@chromium.org changed reviewers: + benhenry@chromium.org
PTAL, thanks!
Description was changed from ========== Use task runner in version_updater_win.cc. BUG=667892 ========== to ========== Use task runner in version_updater_win.cc. BUG=667892 ==========
benhenry@google.com changed reviewers: + gab@chromium.org
https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_win.cc (left): https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:89: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); It's fine to keep these (the UI thread isn't going away at this time) https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:105: void VersionUpdaterWin::BeginUpdateCheckOnFileThread( Rename method https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:109: BeginUpdateCheck(base::CreateSingleThreadTaskRunnerWithTraits( Have the leaves of this call obtain the task runner they need directly instead of having it injected : https://chromium.googlesource.com/chromium/src/+/lkcr/docs/threading_and_task...
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_win.cc (left): https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:89: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/07/11 23:40:15, gab (in-out til july17) wrote: > It's fine to keep these (the UI thread isn't going away at this time) Done. https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:105: void VersionUpdaterWin::BeginUpdateCheckOnFileThread( On 2017/07/11 23:40:15, gab (in-out til july17) wrote: > Rename method Done. https://codereview.chromium.org/2952133002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_win.cc:109: BeginUpdateCheck(base::CreateSingleThreadTaskRunnerWithTraits( On 2017/07/11 23:40:15, gab (in-out til july17) wrote: > Have the leaves of this call obtain the task runner they need directly instead > of having it injected : > https://chromium.googlesource.com/chromium/src/+/lkcr/docs/threading_and_task... I gave this a shot. The latest patchset shows my attempt, but the unittests fail, saying the expected method never gets called. Any suggestions or insights? Thanks.
Thanks for working on this, comments below. https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:397: : task_runner_(base::CreateSingleThreadTaskRunnerWithTraits( Does CreateSequencedTaskRunnerWithTraits() work? https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta... https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (left): https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:625: scoped_refptr<base::TestSimpleTaskRunner> task_runner_; TestSimpleTaskRunner ignores delays in delayed tasks which explains why some tasks now fail to run. ScopedTaskEnvironment will eventually provide a mock clock [1] but for now it unfortunately still doesn't (work in progress)... The simplest solution is probably to add UpdateCheckDriver::SetTaskRunnerForTesting() and inject your TestSimpleTaskRunner with a TODO against https://crbug.com/708584 to use ScopedTaskEnvironment when it supports mock time. [1] https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:397: : task_runner_(base::CreateSingleThreadTaskRunnerWithTraits( On 2017/07/17 17:14:40, gab wrote: > Does CreateSequencedTaskRunnerWithTraits() work? > > https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta... Seems to. https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (left): https://codereview.chromium.org/2952133002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:625: scoped_refptr<base::TestSimpleTaskRunner> task_runner_; On 2017/07/17 17:14:40, gab wrote: > TestSimpleTaskRunner ignores delays in delayed tasks which explains why some > tasks now fail to run. > > ScopedTaskEnvironment will eventually provide a mock clock [1] but for now it > unfortunately still doesn't (work in progress)... > > The simplest solution is probably to add > UpdateCheckDriver::SetTaskRunnerForTesting() and inject your > TestSimpleTaskRunner with a TODO against https://crbug.com/708584 to use > ScopedTaskEnvironment when it supports mock time. > > [1] > https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... Done.
lgtm w/ comments https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (left): https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:408: DCHECK(result_runner_->BelongsToCurrentThread()); Why remove these checks? With SequencedTaskRunner you can do result_runner_->RunsTasksInCurrentSequence(); https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:67: scoped_refptr<base::SequencedTaskRunner> g_update_driver_task_runner = nullptr; I think this needs to be a raw pointer to avoid a static initializer. https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.h:17: class SequencedTaskRunner; scoped_refptr<Foo> is only defined if Foo is defined (scoped_refptr needs to know that Foo inherits from base::RefCounted). IWYU states that in order to take Bar by value it must be fully defined. As such foo.h needs to be explicitly included when taking scoped_refptr<Foo> by value.
calamity@chromium.org changed reviewers: + pkasting@chromium.org
I think I did what you recommended. Let me know if I've done something wrong. Thanks. +pkasting for chrome/browser/google/ OWNERS. https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (left): https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:408: DCHECK(result_runner_->BelongsToCurrentThread()); On 2017/07/19 15:15:24, gab wrote: > Why remove these checks? With SequencedTaskRunner you can do > result_runner_->RunsTasksInCurrentSequence(); Done. https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:67: scoped_refptr<base::SequencedTaskRunner> g_update_driver_task_runner = nullptr; On 2017/07/19 15:15:24, gab wrote: > I think this needs to be a raw pointer to avoid a static initializer. Done. https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/2952133002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.h:17: class SequencedTaskRunner; On 2017/07/19 15:15:24, gab wrote: > scoped_refptr<Foo> is only defined if Foo is defined (scoped_refptr needs to > know that Foo inherits from base::RefCounted). > > IWYU states that in order to take Bar by value it must be fully defined. > > As such foo.h needs to be explicitly included when taking scoped_refptr<Foo> by > value. scoped_refptr removed.
LGTM https://codereview.chromium.org/2952133002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/2952133002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:39: Status status = CHECKING; Nit: This variable can now go away entirely, and we can inline UPDATING into the call at the bottom.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
https://codereview.chromium.org/2952133002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/2952133002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:39: Status status = CHECKING; On 2017/07/20 05:28:53, Peter Kasting wrote: > Nit: This variable can now go away entirely, and we can inline UPDATING into the > call at the bottom. Done.
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.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2952133002/#ps220001 (title: "fix nit")
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": 220001, "attempt_start_ts": 1500535006635200, "parent_rev": "146658672ac9fadfa9be2a0124853ac5d1f3db19", "commit_rev": "c59edd83ceaeb3b0c4fb8dc62281eb57512dcd60"}
Message was sent while issue was closed.
Description was changed from ========== Use task runner in version_updater_win.cc. BUG=667892 ========== to ========== Use task runner in version_updater_win.cc. BUG=667892 Review-Url: https://codereview.chromium.org/2952133002 Cr-Commit-Position: refs/heads/master@{#488160} Committed: https://chromium.googlesource.com/chromium/src/+/c59edd83ceaeb3b0c4fb8dc62281... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://chromium.googlesource.com/chromium/src/+/c59edd83ceaeb3b0c4fb8dc62281...
Message was sent while issue was closed.
https://codereview.chromium.org/2952133002/diff/220001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/2952133002/diff/220001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:599: SetUpdateDriverTaskRunnerForTesting(task_runner_.get()); Also set it to nullptr in TearDown() so future tests in the same process don't flake by unintentionally grabbing a freed SequencedTaskRunner* from the global. |