Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(127)

Issue 18572014: Implement Android shared memory data fetcher for Device Motion. (Closed)

Created:
7 years, 5 months ago by timvolodine
Modified:
7 years, 5 months ago
Reviewers:
bulach, piman
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@renderer-sync-12June-tryASYNC-2-bis-tryRebase-6
Visibility:
Public.

Description

Implement Android shared memory data fetcher for Device Motion. This CL implements the shared memory data fetcher for devices running on Android. The data fetchers acquires data from sensors (e.g. accelerometer, gyroscope) and puts it into a shared memory buffer. Because the sensor data is push-based on Android there is no need for an extra polling thread to fetch the data at regular intervals. This CL features: - interface for shared memory data fetchers for Device Motion. - implementation of the Android shared memory data fetcher. - unit tests for the Android shared memory data fetcher. - implementation of the empty data fetcher for other platforms. - modification of the existing Android data fetcher to be a singleton, such that it is compatible with the current Device Orientation implementation. BUG=135804 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211539

Patch Set 1 #

Patch Set 2 : added OWNERS file #

Total comments: 22

Patch Set 3 : fixed comments #

Patch Set 4 : fixed enums for received_motion_data_ #

Patch Set 5 : added device_motion_provider #

Patch Set 6 : refactored to use split-implementation platform approach #

Patch Set 7 : fixed some includes #

Total comments: 28

Patch Set 8 : fixed comments, added proper singleton implementation and shutdown. #

Total comments: 14

Patch Set 9 : refactored #

Patch Set 10 : modified for merge #

Patch Set 11 : added data_fetcher_orientation_android #

Patch Set 12 : moved jni registration to a separate CL #

Patch Set 13 : fixed some includes and removed instance() from data_fetcher_shared_memory_default #

Total comments: 18

Patch Set 14 : fixed comments #

Total comments: 12

Patch Set 15 : fixed comments and unit tests #

Total comments: 2

Patch Set 16 : fixed comment, added CONTENT_EXPORT + rebased #

Patch Set 17 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+456 lines, -54 lines) Patch
M content/browser/device_orientation/OWNERS View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/device_orientation/data_fetcher_impl_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +40 lines, -19 lines 0 comments Download
M content/browser/device_orientation/data_fetcher_impl_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +99 lines, -23 lines 0 comments Download
A content/browser/device_orientation/data_fetcher_impl_android_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +103 lines, -0 lines 0 comments Download
A content/browser/device_orientation/data_fetcher_orientation_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +32 lines, -0 lines 0 comments Download
A content/browser/device_orientation/data_fetcher_orientation_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +37 lines, -0 lines 0 comments Download
A content/browser/device_orientation/data_fetcher_shared_memory.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +48 lines, -0 lines 0 comments Download
A content/browser/device_orientation/data_fetcher_shared_memory_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +36 lines, -0 lines 0 comments Download
A content/browser/device_orientation/data_fetcher_shared_memory_default.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +39 lines, -0 lines 0 comments Download
M content/browser/device_orientation/device_motion_provider.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -4 lines 0 comments Download
M content/browser/device_orientation/device_motion_provider.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -4 lines 0 comments Download
M content/browser/device_orientation/provider.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +7 lines, -1 line 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 23 (0 generated)
timvolodine
7 years, 5 months ago (2013-07-03 18:58:39 UTC) #1
bulach
thanks tim, good stuff! some comments below, I think the last one will hopefully simplify ...
7 years, 5 months ago (2013-07-04 08:23:35 UTC) #2
timvolodine
thanks Marcus, replies below. https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/device_orientation/data_fetcher_impl_android.cc File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://chromiumcodereview.appspot.com/18572014/diff/1001/content/browser/device_orientation/data_fetcher_impl_android.cc#newcode25 content/browser/device_orientation/data_fetcher_impl_android.cc:25: : device_motion_buffer_(0), On 2013/07/04 08:23:36, ...
7 years, 5 months ago (2013-07-04 12:02:52 UTC) #3
bulach
lgtm thanks tim! question: the instance() method should be implemented by the platform-specific code in ...
7 years, 5 months ago (2013-07-05 11:42:44 UTC) #4
bulach
sorry, I didn't mean lgtm :) would like to discuss this a bit further, just ...
7 years, 5 months ago (2013-07-05 11:43:14 UTC) #5
timvolodine
On 2013/07/05 11:43:14, bulach wrote: > sorry, I didn't mean lgtm :) would like to ...
7 years, 5 months ago (2013-07-05 12:26:41 UTC) #6
timvolodine
On 2013/07/05 11:42:44, bulach wrote: > lgtm > > thanks tim! question: the instance() method ...
7 years, 5 months ago (2013-07-05 13:22:43 UTC) #7
timvolodine
refactored the platform dependent code to use the suggested split-implementation approach. please have a look!
7 years, 5 months ago (2013-07-08 13:40:38 UTC) #8
bulach
good stuff, thanks! just some nits below and some questions about the "instance()" methods: https://codereview.chromium.org/18572014/diff/24001/content/browser/device_orientation/data_fetcher_impl_android.cc ...
7 years, 5 months ago (2013-07-09 09:03:29 UTC) #9
timvolodine
https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/device_orientation/data_fetcher_impl_android.cc File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://chromiumcodereview.appspot.com/18572014/diff/24001/content/browser/device_orientation/data_fetcher_impl_android.cc#newcode54 content/browser/device_orientation/data_fetcher_impl_android.cc:54: DataFetcherImplAndroid::~DataFetcherImplAndroid() { On 2013/07/09 09:03:29, bulach wrote: > question: ...
7 years, 5 months ago (2013-07-09 18:59:56 UTC) #10
bulach
thanks tim! this patch is highlighting a few more issues with the existing code in ...
7 years, 5 months ago (2013-07-10 16:59:41 UTC) #11
timvolodine
thanks Marcus! replies below. https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/browser_main_loop.cc File content/browser/browser_main_loop.cc (right): https://chromiumcodereview.appspot.com/18572014/diff/37001/content/browser/browser_main_loop.cc#newcode469 content/browser/browser_main_loop.cc:469: DataFetcherImplAndroid::Register(base::android::AttachCurrentThread()); On 2013/07/10 16:59:41, bulach ...
7 years, 5 months ago (2013-07-11 14:31:34 UTC) #12
bulach
thanks tim! a few questions below, let me know your thoughts: https://codereview.chromium.org/18572014/diff/23003/content/browser/device_orientation/data_fetcher_impl_android.cc File content/browser/device_orientation/data_fetcher_impl_android.cc (right): ...
7 years, 5 months ago (2013-07-12 14:00:41 UTC) #13
timvolodine
https://codereview.chromium.org/18572014/diff/23003/content/browser/device_orientation/data_fetcher_impl_android.cc File content/browser/device_orientation/data_fetcher_impl_android.cc (right): https://codereview.chromium.org/18572014/diff/23003/content/browser/device_orientation/data_fetcher_impl_android.cc#newcode7 content/browser/device_orientation/data_fetcher_impl_android.cc:7: #include <string.h> On 2013/07/12 14:00:41, bulach wrote: > nit: ...
7 years, 5 months ago (2013-07-12 15:22:17 UTC) #14
bulach
lgtm, thanks tim! just some nits and suggestions for the follow up.. https://codereview.chromium.org/18572014/diff/59001/content/browser/device_orientation/data_fetcher_impl_android.h File content/browser/device_orientation/data_fetcher_impl_android.h ...
7 years, 5 months ago (2013-07-12 16:07:43 UTC) #15
timvolodine
thanks Marcus! https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/device_orientation/data_fetcher_impl_android.h File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://chromiumcodereview.appspot.com/18572014/diff/59001/content/browser/device_orientation/data_fetcher_impl_android.h#newcode28 content/browser/device_orientation/data_fetcher_impl_android.h:28: // once Device Orientation switches to shared ...
7 years, 5 months ago (2013-07-12 17:43:56 UTC) #16
timvolodine
extra approval needed for content/content_browser.gypi, content/content_tests/gypi
7 years, 5 months ago (2013-07-12 17:45:33 UTC) #17
piman
LGTM+nit https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/device_orientation/data_fetcher_impl_android.h File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/device_orientation/data_fetcher_impl_android.h#newcode87 content/browser/device_orientation/data_fetcher_impl_android.h:87: int number_active_device_motion_sensors_; new field needs initialization.
7 years, 5 months ago (2013-07-12 18:06:45 UTC) #18
timvolodine
thanks! https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/device_orientation/data_fetcher_impl_android.h File content/browser/device_orientation/data_fetcher_impl_android.h (right): https://chromiumcodereview.appspot.com/18572014/diff/67001/content/browser/device_orientation/data_fetcher_impl_android.h#newcode87 content/browser/device_orientation/data_fetcher_impl_android.h:87: int number_active_device_motion_sensors_; On 2013/07/12 18:06:45, piman wrote: > ...
7 years, 5 months ago (2013-07-12 18:33:45 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/18572014/78001
7 years, 5 months ago (2013-07-12 19:57:54 UTC) #20
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=175347
7 years, 5 months ago (2013-07-12 21:26:01 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/18572014/78001
7 years, 5 months ago (2013-07-12 23:57:29 UTC) #22
commit-bot: I haz the power
7 years, 5 months ago (2013-07-13 04:23:09 UTC) #23
Message was sent while issue was closed.
Change committed as 211539

Powered by Google App Engine
This is Rietveld 408576698