Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2452083002:
[Device Sensors] Remove BrowserThread knowledge (Closed)
|
Created:
4 years, 1 month ago by blundell Modified:
4 years, 1 month ago Reviewers:
timvolodine CC:
chromium-reviews, riju_, jam, darin-cc_chromium.org, timvolodine, mlamouri+watch-sensors_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Device Sensors] Remove BrowserThread knowledge
Following the restructuring of Device Sensors Mojo communication to
occur on the UI thread on Android, it is now totally straightforward
to remove content::BrowserThread knowledge from the Device Sensors code.
This change will facilitate moving //content/browser/device_sensors to
//device/sensors in a followup as part of the servicification of
//content's device-related code.
BUG=612322
Committed: https://crrev.com/8a86a47aa16fcc5236bef6f4e99e1880c180fd4f
Cr-Commit-Position: refs/heads/master@{#432825}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Response to review #Patch Set 4 : Fix build #
Total comments: 2
Patch Set 5 : Rebase #
Depends on Patchset: Messages
Total messages: 33 (22 generated)
The CQ bit was checked by blundell@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.
blundell@chromium.org changed reviewers: + timvolodine@chromium.org
ping :)
https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... File content/browser/device_sensors/sensor_manager_android.cc (right): https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... content/browser/device_sensors/sensor_manager_android.cc:60: DCHECK(thread_checker_.CalledOnValidThread()); can we add an explicit UI thread check here? to avoid somebody potentially runnig this on a non-UI thread.. https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... File content/browser/device_sensors/sensor_manager_android.h (right): https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... content/browser/device_sensors/sensor_manager_android.h:151: base::ThreadChecker thread_checker_; maybe add a comment that this class should live on UI thread https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... File content/browser/device_sensors/sensor_manager_android_unittest.cc (right): https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... content/browser/device_sensors/sensor_manager_android_unittest.cc:69: base::MessageLoop message_loop_; is this needed anywhere?
The CQ bit was checked by blundell@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by blundell@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.
Thanks! https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... File content/browser/device_sensors/sensor_manager_android.cc (right): https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... content/browser/device_sensors/sensor_manager_android.cc:60: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/10/31 15:52:46, timvolodine wrote: > can we add an explicit UI thread check here? to avoid somebody potentially > runnig this on a non-UI thread.. Done in GetInstance(), the initial entrypoint. https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... File content/browser/device_sensors/sensor_manager_android.h (right): https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... content/browser/device_sensors/sensor_manager_android.h:151: base::ThreadChecker thread_checker_; On 2016/10/31 15:52:47, timvolodine wrote: > maybe add a comment that this class should live on UI thread It's present on the GetInstance() declaration above. https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... File content/browser/device_sensors/sensor_manager_android_unittest.cc (right): https://codereview.chromium.org/2452083002/diff/1/content/browser/device_sens... content/browser/device_sensors/sensor_manager_android_unittest.cc:69: base::MessageLoop message_loop_; On 2016/10/31 15:52:47, timvolodine wrote: > is this needed anywhere? It's implicitly needed by the fact that SensorManagerAndroid needs there to be a current message loop, which there isn't by default in unittests.
lgtm (with a question). thanks! https://codereview.chromium.org/2452083002/diff/60001/content/browser/device_... File content/browser/device_sensors/device_sensor_host.cc (left): https://codereview.chromium.org/2452083002/diff/60001/content/browser/device_... content/browser/device_sensors/device_sensor_host.cc:8: #include "content/public/browser/browser_thread.h" actually a question: so we can depend on content/public/browser in components/ but not in device/ ?
https://codereview.chromium.org/2452083002/diff/60001/content/browser/device_... File content/browser/device_sensors/device_sensor_host.cc (left): https://codereview.chromium.org/2452083002/diff/60001/content/browser/device_... content/browser/device_sensors/device_sensor_host.cc:8: #include "content/public/browser/browser_thread.h" On 2016/11/10 16:43:04, timvolodine wrote: > actually a question: so we can depend on content/public/browser in components/ > but not in device/ ? Yes, essentially. Individual components make their own decisions on whether it's legal for them to depend on //content depending on their use case (e.g., are they intended for use on iOS, which doesn't use //content). All of //device is lower-level than //content.
The CQ bit was checked by blundell@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.
The CQ bit was checked by blundell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2452083002/#ps80001 (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_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 blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Device Sensors] Remove BrowserThread knowledge Following the restructuring of Device Sensors Mojo communication to occur on the UI thread on Android, it is now totally straightforward to remove content::BrowserThread knowledge from the Device Sensors code. This change will facilitate moving //content/browser/device_sensors to //device/sensors in a followup as part of the servicification of //content's device-related code. BUG=612322 ========== to ========== [Device Sensors] Remove BrowserThread knowledge Following the restructuring of Device Sensors Mojo communication to occur on the UI thread on Android, it is now totally straightforward to remove content::BrowserThread knowledge from the Device Sensors code. This change will facilitate moving //content/browser/device_sensors to //device/sensors in a followup as part of the servicification of //content's device-related code. BUG=612322 Committed: https://crrev.com/8a86a47aa16fcc5236bef6f4e99e1880c180fd4f Cr-Commit-Position: refs/heads/master@{#432825} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8a86a47aa16fcc5236bef6f4e99e1880c180fd4f Cr-Commit-Position: refs/heads/master@{#432825} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
