|
|
Descriptioncros: Replace BrowserThread::FILE in InputServiceProxy
BrowserThread::FILE is deprecated in favor of the post_task API.
BUG=689520
Review-Url: https://codereview.chromium.org/2937253003
Cr-Commit-Position: refs/heads/master@{#481228}
Committed: https://chromium.googlesource.com/chromium/src/+/164abed19ba6d4882dc3b66028bcbfd24399b177
Patch Set 1 #
Total comments: 4
Patch Set 2 : update dependent code to fix tests #
Total comments: 11
Patch Set 3 : for #2 #
Total comments: 16
Patch Set 4 : for #3 #Patch Set 5 : rebase #
Total comments: 1
Messages
Total messages: 44 (29 generated)
The CQ bit was checked by xiyuan@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...
xiyuan@chromium.org changed reviewers: + achuith@chromium.org
Could you help to review this? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
LGTM. We have 63 instances of chromeos migrations in this doc: https://docs.google.com/spreadsheets/d/18x9PGMlfgWcBr4fDz2SEEtIwTpSjcBFT2Puib... I'm wondering if we should file a separate bug for these for the the chromeos UI team? https://codereview.chromium.org/2937253003/diff/1/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device/input_service_proxy.h (right): https://codereview.chromium.org/2937253003/diff/1/chrome/browser/chromeos/dev... chrome/browser/chromeos/device/input_service_proxy.h:49: // Should be called once before any InputServiceProxy instance is created. Should we move this comment after the using declaration? https://codereview.chromium.org/2937253003/diff/1/chrome/browser/chromeos/dev... chrome/browser/chromeos/device/input_service_proxy.h:51: base::Callback<scoped_refptr<base::TaskRunner>(void)>; I'm curious if this is the standard pattern now for switching between UI and background threads? It's a bit tricky with pointers to callbacks that bind lambda's.
Filing separate bugs and assign to the team makes sense to me. I was just following other helpers that use the existing bug. And I need to revise the CL because it is tangled with WizardController, InputServiceTestHelper etc. Looks like I need to update all of them in one CL. Will ping back when new PS is ready for review. Thanks. https://codereview.chromium.org/2937253003/diff/1/chrome/browser/chromeos/dev... File chrome/browser/chromeos/device/input_service_proxy.h (right): https://codereview.chromium.org/2937253003/diff/1/chrome/browser/chromeos/dev... chrome/browser/chromeos/device/input_service_proxy.h:49: // Should be called once before any InputServiceProxy instance is created. On 2017/06/15 23:04:59, achuithb wrote: > Should we move this comment after the using declaration? will do https://codereview.chromium.org/2937253003/diff/1/chrome/browser/chromeos/dev... chrome/browser/chromeos/device/input_service_proxy.h:51: base::Callback<scoped_refptr<base::TaskRunner>(void)>; On 2017/06/15 23:04:59, achuithb wrote: > I'm curious if this is the standard pattern now for switching between UI and > background threads? It's a bit tricky with pointers to callbacks that bind > lambda's. Not a standard pattern. I used this because it fits in with the current test code. I could also make it a boolean though since there are only two tests use this and both want UI thread.
The CQ bit was checked by xiyuan@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 xiyuan@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by xiyuan@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.
PTAL. Thanks.
xiyuan@chromium.org changed reviewers: + fdoray@chromium.org
+fdoray, please review lazy_task_runner.h change. Thanks.
lgtm. Sorry you had to rewrite the code that generates a callback pointer via a lambda - it was quite clever but I think this approach has a readability advantage. Thanks! https://codereview.chromium.org/2937253003/diff/60001/base/task_scheduler/laz... File base/task_scheduler/lazy_task_runner.h (right): https://codereview.chromium.org/2937253003/diff/60001/base/task_scheduler/laz... base/task_scheduler/lazy_task_runner.h:108: ALLOW_UNUSED_TYPE constexpr base::SingleThreadTaskRunnerThreadMode \ Should this be a separate CL? https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.cc (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.cc:18: typedef device::InputServiceLinux::InputDeviceInfo InputDeviceInfo; Does it make sense to move this into the anonymous namespace? Maybe it doesn't make a difference https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.h (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.h:52: static scoped_refptr<base::TaskRunner> GetInputServiceTaskRunner(); Should we include base/memory/ref_counted.h or is weak_ptr.h sufficient? I'm not sure. https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/bluetooth_host_pairing_browsertest.cc (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/bluetooth_host_pairing_browsertest.cc:70: InputServiceProxy::SetUseUIThreadForTesting(true); Do you know why we use the UI thread here (and in the other browser test) instead of a blocking thread? https://codereview.chromium.org/2937253003/diff/60001/components/pairing/blue... File components/pairing/bluetooth_host_pairing_controller.cc (right): https://codereview.chromium.org/2937253003/diff/60001/components/pairing/blue... components/pairing/bluetooth_host_pairing_controller.cc:90: : current_stage_(STAGE_NONE), I don't know if you have any interest in moving this initialization to the header. Ignore it if you'd rather not.
https://codereview.chromium.org/2937253003/diff/60001/base/task_scheduler/laz... File base/task_scheduler/lazy_task_runner.h (right): https://codereview.chromium.org/2937253003/diff/60001/base/task_scheduler/laz... base/task_scheduler/lazy_task_runner.h:108: ALLOW_UNUSED_TYPE constexpr base::SingleThreadTaskRunnerThreadMode \ On 2017/06/19 22:34:42, achuithb wrote: > Should this be a separate CL? Done. Moved it to https://codereview.chromium.org/2946823002/. https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.cc (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.cc:18: typedef device::InputServiceLinux::InputDeviceInfo InputDeviceInfo; On 2017/06/19 22:34:42, achuithb wrote: > Does it make sense to move this into the anonymous namespace? Maybe it doesn't > make a difference Moved and changed typedef -> using. https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.h (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.h:52: static scoped_refptr<base::TaskRunner> GetInputServiceTaskRunner(); On 2017/06/19 22:34:42, achuithb wrote: > Should we include base/memory/ref_counted.h or is weak_ptr.h sufficient? I'm not > sure. You are right that we should include ref_counted.h. Fixed. https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/bluetooth_host_pairing_browsertest.cc (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/bluetooth_host_pairing_browsertest.cc:70: InputServiceProxy::SetUseUIThreadForTesting(true); On 2017/06/19 22:34:42, achuithb wrote: > Do you know why we use the UI thread here (and in the other browser test) > instead of a blocking thread? I did not look closely. Think it makes it easier to manipulate fake devices in tests. InputDeviceLinux can only be accessed from one thread. This call essentially creates InputDeviceLinux on the UI thread. Then test can call AddDeviceForTesting directly without the need to ping-pong with a background thread. https://codereview.chromium.org/2937253003/diff/60001/components/pairing/blue... File components/pairing/bluetooth_host_pairing_controller.cc (right): https://codereview.chromium.org/2937253003/diff/60001/components/pairing/blue... components/pairing/bluetooth_host_pairing_controller.cc:90: : current_stage_(STAGE_NONE), On 2017/06/19 22:34:42, achuithb wrote: > I don't know if you have any interest in moving this initialization to the > header. Ignore it if you'd rather not. I am a fan of in-place initializers. Done.
The CQ bit was checked by xiyuan@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. Thanks for taking care of the comments! https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/bluetooth_host_pairing_browsertest.cc (right): https://codereview.chromium.org/2937253003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/bluetooth_host_pairing_browsertest.cc:70: InputServiceProxy::SetUseUIThreadForTesting(true); On 2017/06/19 23:00:26, xiyuan wrote: > On 2017/06/19 22:34:42, achuithb wrote: > > Do you know why we use the UI thread here (and in the other browser test) > > instead of a blocking thread? > > I did not look closely. Think it makes it easier to manipulate fake devices in > tests. InputDeviceLinux can only be accessed from one thread. This call > essentially creates InputDeviceLinux on the UI thread. Then test can call > AddDeviceForTesting directly without the need to ping-pong with a background > thread. Ok, I'm just wondering if we can get away from using this on the UI thread at some point and just massaging the test. But it's not really important for the purpose of this CL (which is to migrate away from BrowserThread::FILE). https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... File components/pairing/bluetooth_host_pairing_controller.cc (right): https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... components/pairing/bluetooth_host_pairing_controller.cc:90: : proto_decoder_(new ProtoDecoder(this)), Does base::MakeUnique not work here?
https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.cc (right): https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.cc:111: FROM_HERE, base::Bind(&InputServiceProxy::ServiceObserver::Initialize, PostTask() takes a OnceClosure as argument https://chromium.googlesource.com/chromium/src/+/b30e6bf3abc9c86a5fdfa612bdf0.... You should use BindOnce instead of Bind to avoid a conversion. You don't have to change this as part of this CL, but it's a good thing to know. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.h (right): https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.h:53: static scoped_refptr<base::TaskRunner> GetInputServiceTaskRunner(); Since it's important for callers that tasks posted to the returned TaskRunner run in sequence, use SequencedTaskRunner as the return type. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_test_helper.cc (right): https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_test_helper.cc:21: void InitInputServiceOnInputServiceThread() { InitInputServiceOnInputServiceSequence Since you plan to change the LazySingleThreadTaskRunner to a LazySequencedTaskRunner and a thread is a special kind of sequence. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_test_helper.cc:25: void AddDeviceOnInputServiceThread(const InputDeviceInfo& device) { AddDeviceOnInputServiceSequence https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_test_helper.cc:31: void RemoveDeviceOnInputServiceThread(const std::string& id) { RemoveDeviceOnInputServiceSequence https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... File components/pairing/bluetooth_host_pairing_controller.h (right): https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... components/pairing/bluetooth_host_pairing_controller.h:65: const scoped_refptr<base::TaskRunner>& input_service_task_runner); Pass by value instead of by const reference and std::move to the member https://crbug.com/589043 https://codereview.chromium.org/2937253003/diff/80001/components/pairing/shar... File components/pairing/shark_connection_listener.h (right): https://codereview.chromium.org/2937253003/diff/80001/components/pairing/shar... components/pairing/shark_connection_listener.h:32: const scoped_refptr<base::TaskRunner>& input_service_task_runner, Pass by value instead of by const reference and std::move to the member https://crbug.com/589043
The CQ bit was checked by xiyuan@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...
https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.cc (right): https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.cc:111: FROM_HERE, base::Bind(&InputServiceProxy::ServiceObserver::Initialize, On 2017/06/20 12:42:39, fdoray wrote: > PostTask() takes a OnceClosure as argument > https://chromium.googlesource.com/chromium/src/+/b30e6bf3abc9c86a5fdfa612bdf0.... > You should use BindOnce instead of Bind to avoid a conversion. You don't have to > change this as part of this CL, but it's a good thing to know. Done. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_proxy.h (right): https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_proxy.h:53: static scoped_refptr<base::TaskRunner> GetInputServiceTaskRunner(); On 2017/06/20 12:42:39, fdoray wrote: > Since it's important for callers that tasks posted to the returned TaskRunner > run in sequence, use SequencedTaskRunner as the return type. Done. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/device/input_service_test_helper.cc (right): https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_test_helper.cc:21: void InitInputServiceOnInputServiceThread() { On 2017/06/20 12:42:39, fdoray wrote: > InitInputServiceOnInputServiceSequence > > Since you plan to change the LazySingleThreadTaskRunner to a > LazySequencedTaskRunner and a thread is a special kind of sequence. Done. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_test_helper.cc:25: void AddDeviceOnInputServiceThread(const InputDeviceInfo& device) { On 2017/06/20 12:42:39, fdoray wrote: > AddDeviceOnInputServiceSequence Done. https://codereview.chromium.org/2937253003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/device/input_service_test_helper.cc:31: void RemoveDeviceOnInputServiceThread(const std::string& id) { On 2017/06/20 12:42:39, fdoray wrote: > RemoveDeviceOnInputServiceSequence Done. https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... File components/pairing/bluetooth_host_pairing_controller.cc (right): https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... components/pairing/bluetooth_host_pairing_controller.cc:90: : proto_decoder_(new ProtoDecoder(this)), On 2017/06/20 08:36:48, achuithb wrote: > Does base::MakeUnique not work here? Done. https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... File components/pairing/bluetooth_host_pairing_controller.h (right): https://codereview.chromium.org/2937253003/diff/80001/components/pairing/blue... components/pairing/bluetooth_host_pairing_controller.h:65: const scoped_refptr<base::TaskRunner>& input_service_task_runner); On 2017/06/20 12:42:39, fdoray wrote: > Pass by value instead of by const reference and std::move to the member > https://crbug.com/589043 Done. https://codereview.chromium.org/2937253003/diff/80001/components/pairing/shar... File components/pairing/shark_connection_listener.h (right): https://codereview.chromium.org/2937253003/diff/80001/components/pairing/shar... components/pairing/shark_connection_listener.h:32: const scoped_refptr<base::TaskRunner>& input_service_task_runner, On 2017/06/20 12:42:39, fdoray wrote: > Pass by value instead of by const reference and std::move to the member > https://crbug.com/589043 Done.
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 xiyuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org Link to the patchset: https://codereview.chromium.org/2937253003/#ps120001 (title: "rebase")
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 xiyuan@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": 120001, "attempt_start_ts": 1498063190426020, "parent_rev": "a6ce44474ee162592d04a31cf0b243c31a53f57d", "commit_rev": "164abed19ba6d4882dc3b66028bcbfd24399b177"}
Message was sent while issue was closed.
Description was changed from ========== cros: Replace BrowserThread::FILE in InputServiceProxy BrowserThread::FILE is deprecated in favor of the post_task API. BUG=689520 ========== to ========== cros: Replace BrowserThread::FILE in InputServiceProxy BrowserThread::FILE is deprecated in favor of the post_task API. BUG=689520 Review-Url: https://codereview.chromium.org/2937253003 Cr-Commit-Position: refs/heads/master@{#481228} Committed: https://chromium.googlesource.com/chromium/src/+/164abed19ba6d4882dc3b66028bc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/164abed19ba6d4882dc3b66028bc...
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2937253003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/device/input_service_proxy.cc (right): https://codereview.chromium.org/2937253003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/device/input_service_proxy.cc:27: // are updated to check on sequence instead of thread. Create bug to make InputServiceLinux and friends sequence-friendly. Add TODO and reference to bug here. |