Convert device_sensors to use mojo.

Convert device_sensors to use mojo.

BUG=577685
Committed: https://crrev.com/413d6473ff0d73fe335a5ed3e2d475cab6d868d5
Cr-Commit-Position: refs/heads/master@{#403749}

[Sam McNally, 2016-06-02 05:32:36]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-06-02 05:48:15]

Patchset #2 (id:40001) has been deleted

[Sam McNally, 2016-06-02 05:57:00]

Patchset #2 (id:60001) has been deleted

[Sam McNally, 2016-06-02 06:51:48]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-06-06 05:01:13]

Patchset #4 (id:140001) has been deleted

[Sam McNally, 2016-06-06 05:01:31]

Patchset #3 (id:120001) has been deleted

[Sam McNally, 2016-06-06 05:01:34]

Patchset #2 (id:100001) has been deleted

[Sam McNally, 2016-06-06 05:01:36]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-06-07 00:16:14]

Patchset #1 (id:160001) has been deleted

[Sam McNally, 2016-06-09 04:01:39]

Patchset #1 (id:180001) has been deleted

[Sam McNally, 2016-06-09 04:03:24]

Patchset #1 (id:200001) has been deleted

[Sam McNally, 2016-06-09 05:28:49]

Patchset #1 (id:220001) has been deleted

[Sam McNally, 2016-06-09 09:34:39]

sammc@chromium.org changed reviewers:
+ timvolodine@chromium.org

[timvolodine, 2016-06-09 17:43:47]

Thanks Sam, think it looks great! especially the delta of the number of lines is
nice :)

just a couple of questions below:

https://codereview.chromium.org/2037513002/diff/240001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:97: typedef
std::map<ConsumerType, mojo::ScopedSharedBufferMapping>
why do we need two maps now? is there a way to just store the
ConsumerType->Handle map?

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
File content/renderer/device_sensors/device_motion_event_pump.cc (right):

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
content/renderer/device_sensors/device_motion_event_pump.cc:23: if
(!using_fake_data_for_testing_ && reader_->GetLatestData(&data) &&
don't think this should be called in testing mode (?)

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
File content/renderer/device_sensors/device_motion_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
content/renderer/device_sensors/device_motion_event_pump.h:41: bool
using_fake_data_for_testing_ = false;
why is this needed?

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
File content/renderer/device_sensors/device_orientation_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
content/renderer/device_sensors/device_orientation_event_pump.h:25: class
CONTENT_EXPORT DeviceOrientationEventPumpBase
do we still need to CONTENT_EXPORT this?

[Sam McNally, 2016-06-10 01:07:51]

https://codereview.chromium.org/2037513002/diff/240001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:97: typedef
std::map<ConsumerType, mojo::ScopedSharedBufferMapping>
On 2016/06/09 17:43:47, timvolodine wrote:
> why do we need two maps now? is there a way to just store the
> ConsumerType->Handle map?

Mojo shared memory doesn't have anything that holds both a shared memory handle
and a mapping of that shared memory. The first map holds the handles to
duplicate to share with clients; the second manages lifetime of the mappings in
the browser process. There's no reason they need to be separate maps though.

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
File content/renderer/device_sensors/device_motion_event_pump.cc (right):

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
content/renderer/device_sensors/device_motion_event_pump.cc:23: if
(!using_fake_data_for_testing_ && reader_->GetLatestData(&data) &&
On 2016/06/09 17:43:47, timvolodine wrote:
> don't think this should be called in testing mode (?)

Done. Previously, I hadn't replicated the testing mode behaviour in the mojo IPC
mixin.

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
File content/renderer/device_sensors/device_motion_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
content/renderer/device_sensors/device_motion_event_pump.h:41: bool
using_fake_data_for_testing_ = false;
On 2016/06/09 17:43:47, timvolodine wrote:
> why is this needed?

Removed it.

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
File content/renderer/device_sensors/device_orientation_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/renderer/devic...
content/renderer/device_sensors/device_orientation_event_pump.h:25: class
CONTENT_EXPORT DeviceOrientationEventPumpBase
On 2016/06/09 17:43:47, timvolodine wrote:
> do we still need to CONTENT_EXPORT this?

Yes, the unit test still needs this.

[timvolodine, 2016-06-15 15:54:47]

ok thanks, I see you fixed the two maps issue and the bots, so lgtm % comment
below on my side.

https://codereview.chromium.org/2037513002/diff/240001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/2037513002/diff/240001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:97: typedef
std::map<ConsumerType, mojo::ScopedSharedBufferMapping>
On 2016/06/10 01:07:51, Sam McNally wrote:
> On 2016/06/09 17:43:47, timvolodine wrote:
> > why do we need two maps now? is there a way to just store the
> > ConsumerType->Handle map?
> 
> Mojo shared memory doesn't have anything that holds both a shared memory
handle
> and a mapping of that shared memory. The first map holds the handles to
> duplicate to share with clients; the second manages lifetime of the mappings
in
> the browser process. There's no reason they need to be separate maps though.

I see, was hoping mojo would provide something equivalent to SharedMemory..

https://codereview.chromium.org/2037513002/diff/320001/content/renderer/devic...
File content/renderer/device_sensors/device_sensor_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/320001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:47:
RenderThreadImpl::current()->layout_test_mode()) {
Probably even better to put this in SendStartMessage/SendStopMessage above to
avoid executing unnecessary mojo calls in layout tests. This can happen in a
follow-up patch though.

[Sam McNally, 2016-06-16 00:48:37]

sammc@chromium.org changed reviewers:
+ dcheng@chromium.org, rockot@chromium.org, sievers@chromium.org

[Sam McNally, 2016-06-16 00:48:38]

+dcheng for messages and mojoms

+rockot for //device

+sievers for //content outside device_sensors

[Sam McNally, 2016-06-16 00:51:04]

https://codereview.chromium.org/2037513002/diff/320001/content/renderer/devic...
File content/renderer/device_sensors/device_sensor_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/320001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:47:
RenderThreadImpl::current()->layout_test_mode()) {
On 2016/06/15 15:54:47, timvolodine wrote:
> Probably even better to put this in SendStartMessage/SendStopMessage above to
> avoid executing unnecessary mojo calls in layout tests. This can happen in a
> follow-up patch though.

Done.

[Ken Rockot(use gerrit already), 2016-06-16 07:22:45]

device lgtm

[dcheng, 2016-06-16 08:57:32]

I'm at BlinkOn so I probably won't be able to take a look at this until Friday
or Monday. If it's urgent, feel free to find another reviewer.

[Sam McNally, 2016-06-29 01:34:52]

dcheng, sievers: ping

[no sievers, 2016-06-30 21:46:37]

On 2016/06/16 00:48:38, Sam McNally wrote:
> +dcheng for messages and mojoms
> 
> +rockot for //device
> 
> +sievers for //content outside device_sensors

lgtm

[dcheng, 2016-07-04 03:39:50]

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:97:
SharedMemoryMap;
Nit: since we're changing this, let's update it to use the preferred using
SharedMemoryMap = std::map<...>; syntax

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
File content/browser/device_sensors/device_sensor_host.cc (right):

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
content/browser/device_sensors/device_sensor_host.cc:27:
DeviceInertialSensorService::GetInstance()->RemoveConsumer(consumer_type);
Is the name for this still accurate? It looks like 'light sensors' are an
explicit instantiation of this template, and in my mind, that's not an inertial
sensor.

(I guess this is from pre-existing code, but perhaps it would be nice to clean
it up in a followup maybe?)

https://codereview.chromium.org/2037513002/diff/380001/content/common/device_...
File content/common/device_sensors/OWNERS (right):

https://codereview.chromium.org/2037513002/diff/380001/content/common/device_...
content/common/device_sensors/OWNERS:4: per-file
*_messages*.h=file://ipc/SECURITY_OWNERS
If you rebase, do you still need these? I think we fixed a bug where the
presubmit was making you add these rules for deleted files. See
https://codereview.chromium.org/2090653003/

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
File content/renderer/device_sensors/device_orientation_event_pump_unittest.cc
(right):

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_orientation_event_pump_unittest.cc:112:
return mojo::WrapSharedMemoryHandle(
Here and elsewhere, would it make sense to just use the mojo shared buffer
interface uniformly throughout instead of wrapping/unwrapping to/from
base::SharedMemory? It feels like it might be slightly less verbose.

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
File content/renderer/device_sensors/device_sensor_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:28:
base::Bind(&DeviceSensorMojoClientMixin<Base, MojoInterface>::DidStart,
Please include bind.h and bind_helpers.h.

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:34: MojoInterface&
GetMojoInterface() {
Why does this happen in a GetMojoInterface() helper rather than in the ctor?

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:43:
!RenderThreadImpl::current()->layout_test_mode()) {
Is this check still needed with Mojo? My understanding is that layout tests can
use JS mocks for Mojo stuff now: https://codereview.chromium.org/1836203002/

That would obsolete this, right?

(A followup for this is fine, I'm just trying to make sure I understand the
current state of affairs)

[Sam McNally, 2016-07-04 05:30:38]

Patchset #10 (id:420001) has been deleted

[Sam McNally, 2016-07-04 05:32:43]

Patchset #10 (id:440001) has been deleted

[Sam McNally, 2016-07-04 08:31:39]

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:97:
SharedMemoryMap;
On 2016/07/04 03:39:49, dcheng wrote:
> Nit: since we're changing this, let's update it to use the preferred using
> SharedMemoryMap = std::map<...>; syntax

Done.

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
File content/browser/device_sensors/device_sensor_host.cc (right):

https://codereview.chromium.org/2037513002/diff/380001/content/browser/device...
content/browser/device_sensors/device_sensor_host.cc:27:
DeviceInertialSensorService::GetInstance()->RemoveConsumer(consumer_type);
On 2016/07/04 03:39:49, dcheng wrote:
> Is the name for this still accurate? It looks like 'light sensors' are an
> explicit instantiation of this template, and in my mind, that's not an
inertial
> sensor.

I think you're right.
> 
> (I guess this is from pre-existing code, but perhaps it would be nice to clean
> it up in a followup maybe?)

I'll rename it in a followup.

https://codereview.chromium.org/2037513002/diff/380001/content/common/device_...
File content/common/device_sensors/OWNERS (right):

https://codereview.chromium.org/2037513002/diff/380001/content/common/device_...
content/common/device_sensors/OWNERS:4: per-file
*_messages*.h=file://ipc/SECURITY_OWNERS
On 2016/07/04 03:39:49, dcheng wrote:
> If you rebase, do you still need these? I think we fixed a bug where the
> presubmit was making you add these rules for deleted files. See
> https://codereview.chromium.org/2090653003/

Done.

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
File content/renderer/device_sensors/device_orientation_event_pump_unittest.cc
(right):

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_orientation_event_pump_unittest.cc:112:
return mojo::WrapSharedMemoryHandle(
On 2016/07/04 03:39:49, dcheng wrote:
> Here and elsewhere, would it make sense to just use the mojo shared buffer
> interface uniformly throughout instead of wrapping/unwrapping to/from
> base::SharedMemory? It feels like it might be slightly less verbose.

Done.

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
File content/renderer/device_sensors/device_sensor_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:28:
base::Bind(&DeviceSensorMojoClientMixin<Base, MojoInterface>::DidStart,
On 2016/07/04 03:39:49, dcheng wrote:
> Please include bind.h and bind_helpers.h.

Done.

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:34: MojoInterface&
GetMojoInterface() {
On 2016/07/04 03:39:49, dcheng wrote:
> Why does this happen in a GetMojoInterface() helper rather than in the ctor?

Done.

https://codereview.chromium.org/2037513002/diff/380001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:43:
!RenderThreadImpl::current()->layout_test_mode()) {
On 2016/07/04 03:39:50, dcheng wrote:
> Is this check still needed with Mojo? My understanding is that layout tests
can
> use JS mocks for Mojo stuff now: https://codereview.chromium.org/1836203002/
> 
> That would obsolete this, right?
> 
> (A followup for this is fine, I'm just trying to make sure I understand the
> current state of affairs)

Yes, the layout tests haven't been converted to that style. To convert those
layout tests, we'd need to expose mojo shared buffers to the JS API.

[dcheng, 2016-07-05 02:51:54]

LGTM with nits

https://codereview.chromium.org/2037513002/diff/500001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.cc (right):

https://codereview.chromium.org/2037513002/diff/500001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.cc:187: return
mojo::ScopedSharedBufferHandle();
Btw, does reaching this state indicate a logic flaw? It would mean that we
didn't call StartFetchingDeviceData first or it failed, right? Do you think it'd
make sense to DCHECK() instead?

(Just trying to get a sense of the right thing to do here, but if this is a
control path that is exceptional, it might be nice to highlight it as such)

https://codereview.chromium.org/2037513002/diff/500001/content/renderer/devic...
File content/renderer/device_sensors/device_sensor_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/500001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:32: // the other end
of the mojo pipe to anything.
Nit: annotate with a TODO to remove once we have the proper JS layout test
machinery hooked up for this.

https://codereview.chromium.org/2037513002/diff/500001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:48: MojoInterface&
GetMojoInterface() {
I think this is now unnecessary: it might be a bit simpler to just have
SendStartMessage / SendStopMessage just use mojo_interface_-> directly.

[Sam McNally, 2016-07-05 03:03:36]

https://codereview.chromium.org/2037513002/diff/500001/content/browser/device...
File content/browser/device_sensors/data_fetcher_shared_memory_base.cc (right):

https://codereview.chromium.org/2037513002/diff/500001/content/browser/device...
content/browser/device_sensors/data_fetcher_shared_memory_base.cc:187: return
mojo::ScopedSharedBufferHandle();
On 2016/07/05 02:51:54, dcheng wrote:
> Btw, does reaching this state indicate a logic flaw? It would mean that we
> didn't call StartFetchingDeviceData first or it failed, right? Do you think
it'd
> make sense to DCHECK() instead?
> 
> (Just trying to get a sense of the right thing to do here, but if this is a
> control path that is exceptional, it might be nice to highlight it as such)

Done.

https://codereview.chromium.org/2037513002/diff/500001/content/renderer/devic...
File content/renderer/device_sensors/device_sensor_event_pump.h (right):

https://codereview.chromium.org/2037513002/diff/500001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:32: // the other end
of the mojo pipe to anything.
On 2016/07/05 02:51:54, dcheng wrote:
> Nit: annotate with a TODO to remove once we have the proper JS layout test
> machinery hooked up for this.

Done.

https://codereview.chromium.org/2037513002/diff/500001/content/renderer/devic...
content/renderer/device_sensors/device_sensor_event_pump.h:48: MojoInterface&
GetMojoInterface() {
On 2016/07/05 02:51:54, dcheng wrote:
> I think this is now unnecessary: it might be a bit simpler to just have
> SendStartMessage / SendStopMessage just use mojo_interface_-> directly.

Done.

[Sam McNally, 2016-07-05 07:36:55]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-07-05 07:36:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
timvolodine@chromium.org, rockot@chromium.org, sievers@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2037513002/#ps520001
(title: " ")

[commit-bot: I haz the power, 2016-07-05 07:37:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 07:43:55]

Committed patchset #13 (id:520001)

[commit-bot: I haz the power, 2016-07-05 07:46:24]

Description was changed from

==========
Convert device_sensors to use mojo.

BUG=577685
==========

to

==========
Convert device_sensors to use mojo.

BUG=577685

Committed: https://crrev.com/413d6473ff0d73fe335a5ed3e2d475cab6d868d5
Cr-Commit-Position: refs/heads/master@{#403749}
==========

[commit-bot: I haz the power, 2016-07-05 07:46:30]

Patchset 13 (id:??) landed as
https://crrev.com/413d6473ff0d73fe335a5ed3e2d475cab6d868d5
Cr-Commit-Position: refs/heads/master@{#403749}