[DeviceLight] Browser+java part

[DeviceLight] Browser+java part

This is part 2 of the Adding Device Light [content side]
Adding the Browser and Java part in the content side for the
DeviceLight API.

BUG=336424
Committed: https://crrev.com/f07da3973fd2bc8cdd739fed2522634bd475b2d2
Cr-Commit-Position: refs/heads/master@{#295937}

[riju_, 2014-07-15 12:01:30]

Hi Tim, please take a look. I am going to add the browser test part for
DeviceLight in a subsequent CL.

[riju_, 2014-07-16 07:17:59]

+jochen: content owner

[timvolodine, 2014-07-16 15:39:38]

thanks for the patch riju, comments below.

Also, I have a few general concerns:
- currently it looks like the implementation can fire a stale lux value into
blink after a restart of the API.
- what happens if there is no sensor? should the specification do something
special like setting value to infinity? currently it looks like the
device_light_event_pump will continue spinning even when no data can be
provided..

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:63: return
SetLightBuffer(light_buffer_);
the function doesn't appear to do much, you could just return true instead?

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:61: case
CONSUMER_TYPE_LIGHT: {
indentation

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:67: }
break;
break on new line

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:218: //
TODO(riju) : Add light test
I think the browsertest should either be in a separate patch comlpetely or add a
test here.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:161:
kInertialSensorIntervalMillis);
do we need same interval for device light as for the other sensors? i think
ideally we should have a separate interval constant for device light, as on the
renderer/ side.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:212:
is_light_buffer_ready_ = ready;
why do you need this? where is it used?

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:216:
SetLightBufferReadyStatus(false);
should the lux value be set to -1 here?

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java
(right):

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java:522:
* Device Light value from Ambient Light sensors
nit: add "." in the end.

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java
(right):

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java:105:
DeviceSensors.DEVICE_LIGHT, 100);
nit: fits on one line?

[jochen (gone - plz use gerrit), 2014-07-17 07:36:01]

is it worthwhile to use shared memory for 5 IPCs/s ?

[riju_, 2014-07-18 15:59:18]

Tim: thanks for the comments.
In case of No Sensor, like linux desktop, null event should be fired ->
DeviceLightController::isNullEvent()  called which does set the value to
Infinity. [1]

I am also trying to fix your other concerns soon.
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


====

Jochen: Use of shared memory was questioned in the previous CL
(https://codereview.chromium.org/286793002/) also. But dcheng said OK, because i
was re using the motion/orientation infra.
If you still feel strongly against it, then I will try to refactor the
DeviceLightEventPump / DeviceSensorEventPump and see if I can come up with a
"high_latency_variant" which passes the object directly which DeviceLight might
use.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:63: return
SetLightBuffer(light_buffer_);
On 2014/07/16 15:39:38, timvolodine wrote:
> the function doesn't appear to do much, you could just return true instead?

Done.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:61: case
CONSUMER_TYPE_LIGHT: {
On 2014/07/16 15:39:38, timvolodine wrote:
> indentation

Done. I ran git cl format, and clang format did this.
But, lets keep consistency with the previous code.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:67: }
break;
On 2014/07/16 15:39:38, timvolodine wrote:
> break on new line

Done.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:218: //
TODO(riju) : Add light test
On 2014/07/16 15:39:38, timvolodine wrote:
> I think the browsertest should either be in a separate patch comlpetely or add
a
> test here.

Yes. I agree with you. I am renaming the "device_orientation" dir under
content/test/data to "device_sensors", hence did not want to pollute this CL. If
its not too much trouble for you/reviewers lets keep the browsertests in this
CL.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:161:
kInertialSensorIntervalMillis);
On 2014/07/16 15:39:38, timvolodine wrote:
> do we need same interval for device light as for the other sensors? i think
> ideally we should have a separate interval constant for device light, as on
the
> renderer/ side.

Done. Now I have added kInertialLightSensorIntervalMillis=200 for DeviceLight.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:212:
is_light_buffer_ready_ = ready;
On 2014/07/16 15:39:38, timvolodine wrote:
> why do you need this? where is it used?

Done.

https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:216:
SetLightBufferReadyStatus(false);
On 2014/07/16 15:39:38, timvolodine wrote:
> should the lux value be set to -1 here?

Done.

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java
(right):

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java:522:
* Device Light value from Ambient Light sensors
On 2014/07/16 15:39:38, timvolodine wrote:
> nit: add "." in the end.

Done.

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java
(right):

https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java:105:
DeviceSensors.DEVICE_LIGHT, 100);
On 2014/07/16 15:39:38, timvolodine wrote:
> nit: fits on one line?

Done.

[riju_, 2014-07-31 16:50:29]

On 2014/07/17 07:36:01, jochen wrote:
> is it worthwhile to use shared memory for 5 IPCs/s ?

Hi jochen,

I spent some time trying to re factor the code to accommodate a non Shared
Memory variant in the DeviceSensor code.
DeviceSensorEventPump and SensorManagerAndroid are very tightly coupled to
Shared Memory. The native call GotLight already presents
the value in a shared memory. So to pass to renderer process, passing the handle
is easier. I tried to extract the value and then use it for IPC, but it was
very hackish and unacceptable.
At this stage, mixing IPC and shared memory is going to make the code
unnecessarily complex.

How about we try to land the DeviceLight API using the Shared Memory approach
as of now. This CL will kind of complete the implementation.

If there are still concerns and if the non shared memory approach is still
preferred, I will re-implement the API without any Shared Memory,
like say the battery_status.

[jochen (gone - plz use gerrit), 2014-08-01 09:57:41]

since there's a good reason (share infrastructure) to use shared memory, that's
fine with me.

I'm waiting for some device_sensor owner to approve the CL before I look at the
two places outside

[riju_, 2014-08-01 10:10:09]

On 2014/08/01 09:57:41, jochen wrote:
> since there's a good reason (share infrastructure) to use shared memory,
that's
> fine with me.
> 
> I'm waiting for some device_sensor owner to approve the CL before I look at
the
> two places outside

OK, thanks.

[riju_, 2014-08-04 16:45:05]

On 2014/07/16 15:39:38, timvolodine wrote:
> thanks for the patch riju, comments below.
> 
> Also, I have a few general concerns:
> - currently it looks like the implementation can fire a stale lux value into
> blink after a restart of the API.
> - what happens if there is no sensor? should the specification do something
> special like setting value to infinity? currently it looks like the
> device_light_event_pump will continue spinning even when no data can be
> provided..

Hi Tim,

Yes. The device_light_event_pump is spinning needlessly.

On linux desktop, with no sensors, when i run the content_browsertests,
in "data_fetcher_shared_memory_default.cc", DataFetcherSharedMemory c'stor is
called,
but DataFetcherSharedMemory::Start() is not called.
I was thinking of setting
light_buffer_->data.value = std::numeric_limits<double>::infinity()
there and then in DeviceLightEventPump::FireEvent(),
if (data.value == std::numeric_limits<double>::infinity()) {
    Stop();
}

Right now the pump is running because the value returned by GetLatestData() is
always 0, in case of no sensor.
As a hack in DeviceLightEventPump::FireEvent() , I tried -> if (data.value == 0)
Stop();
The pump does stop, but i have to Ctrl+C the test.

Any suggestions where I might be doing it wrong ?

>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
> (right):
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/data_fetcher_shared_memory_default.cc:63:
return
> SetLightBuffer(light_buffer_);
> the function doesn't appear to do much, you could just return true instead?
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/device_inertial_sensor_browsertest.cc:61: case
> CONSUMER_TYPE_LIGHT: {
> indentation
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/device_inertial_sensor_browsertest.cc:67: }
> break;
> break on new line
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/device_inertial_sensor_browsertest.cc:218: //
> TODO(riju) : Add light test
> I think the browsertest should either be in a separate patch comlpetely or add
a
> test here.
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> File content/browser/device_sensors/sensor_manager_android.cc (right):
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/sensor_manager_android.cc:161:
> kInertialSensorIntervalMillis);
> do we need same interval for device light as for the other sensors? i think
> ideally we should have a separate interval constant for device light, as on
the
> renderer/ side.
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/sensor_manager_android.cc:212:
> is_light_buffer_ready_ = ready;
> why do you need this? where is it used?
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/browser/device_...
> content/browser/device_sensors/sensor_manager_android.cc:216:
> SetLightBufferReadyStatus(false);
> should the lux value be set to -1 here?
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java
> (right):
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java:522:
> * Device Light value from Ambient Light sensors
> nit: add "." in the end.
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java
> (right):
> 
>
https://codereview.chromium.org/292693004/diff/120001/content/public/android/...
>
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java:105:
> DeviceSensors.DEVICE_LIGHT, 100);
> nit: fits on one line?

[timvolodine, 2014-08-05 10:39:17]

On 2014/08/04 16:45:05, riju_ wrote:
> On 2014/07/16 15:39:38, timvolodine wrote:
> > thanks for the patch riju, comments below.
> > 
> > Also, I have a few general concerns:
> > - currently it looks like the implementation can fire a stale lux value into
> > blink after a restart of the API.
> > - what happens if there is no sensor? should the specification do something
> > special like setting value to infinity? currently it looks like the
> > device_light_event_pump will continue spinning even when no data can be
> > provided..
> 
> Hi Tim,
> 
> Yes. The device_light_event_pump is spinning needlessly.
> 
> On linux desktop, with no sensors, when i run the content_browsertests,
> in "data_fetcher_shared_memory_default.cc", DataFetcherSharedMemory c'stor is
> called,
> but DataFetcherSharedMemory::Start() is not called.
> I was thinking of setting
> light_buffer_->data.value = std::numeric_limits<double>::infinity()
> there and then in DeviceLightEventPump::FireEvent(),
> if (data.value == std::numeric_limits<double>::infinity()) {
>     Stop();
> }
> 
> Right now the pump is running because the value returned by GetLatestData() is
> always 0, in case of no sensor.
> As a hack in DeviceLightEventPump::FireEvent() , I tried -> if (data.value ==
0)
> Stop();
> The pump does stop, but i have to Ctrl+C the test.
> 
> Any suggestions where I might be doing it wrong ?
> 
Hi Riju,

I understood that when the lux-value is +infinity when there is no sensor.
DeviceOrientation fires a single null-event in such cases, so in your case the
isNullEvent function will be true when +infinity so blink will automatically
call stop once it receives such an event. (by the way this is not very clear in
the current Device Light specification).

One tricky part is that the "pump" needs to keep firing "null" events until
blink decides to unregister (that's what is tested in browsertests). You can
have a look at how DeviceOrientationEventPump::ShouldFireEvent() handles this.
In any case I don't think the "pump" needs to do any stopping on its own.
Hope this helps.

Tim

[riju_, 2014-08-13 11:49:11]

On 2014/08/05 10:39:17, timvolodine wrote:
> On 2014/08/04 16:45:05, riju_ wrote:
> > On 2014/07/16 15:39:38, timvolodine wrote:
> > > thanks for the patch riju, comments below.
> > > 
> > > Also, I have a few general concerns:
> > > - currently it looks like the implementation can fire a stale lux value
into
> > > blink after a restart of the API.
> > > - what happens if there is no sensor? should the specification do
something
> > > special like setting value to infinity? currently it looks like the
> > > device_light_event_pump will continue spinning even when no data can be
> > > provided..
> > 
> > Hi Tim,
> > 
> > Yes. The device_light_event_pump is spinning needlessly.
> > 
> > On linux desktop, with no sensors, when i run the content_browsertests,
> > in "data_fetcher_shared_memory_default.cc", DataFetcherSharedMemory c'stor
is
> > called,
> > but DataFetcherSharedMemory::Start() is not called.
> > I was thinking of setting
> > light_buffer_->data.value = std::numeric_limits<double>::infinity()
> > there and then in DeviceLightEventPump::FireEvent(),
> > if (data.value == std::numeric_limits<double>::infinity()) {
> >     Stop();
> > }
> > 
> > Right now the pump is running because the value returned by GetLatestData()
is
> > always 0, in case of no sensor.
> > As a hack in DeviceLightEventPump::FireEvent() , I tried -> if (data.value
==
> 0)
> > Stop();
> > The pump does stop, but i have to Ctrl+C the test.
> > 
> > Any suggestions where I might be doing it wrong ?
> > 
> Hi Riju,
> 
> I understood that when the lux-value is +infinity when there is no sensor.
> DeviceOrientation fires a single null-event in such cases, so in your case the
> isNullEvent function will be true when +infinity so blink will automatically
> call stop once it receives such an event. (by the way this is not very clear
in
> the current Device Light specification).
> 
> One tricky part is that the "pump" needs to keep firing "null" events until
> blink decides to unregister (that's what is tested in browsertests). You can
> have a look at how DeviceOrientationEventPump::ShouldFireEvent() handles this.
> In any case I don't think the "pump" needs to do any stopping on its own.
> Hope this helps.
> 
> Tim

Hi Tim,

I tried your suggestion in the last patch set. In the light pump i realized we
need
to fire multiple(5 for device light) +inf values before Blink considers that a
NULL
event is fired. (Seemed a bit odd). I got to the point that in Blink,
DeviceLightController the NULL event is called, but I still have to Ctrl+C the
test.
here s the log(info) trace https://gist.github.com/riju/323f18e17bfe6ed756cf

The DeviceInertialSensorBrowserTest.LightTest is passing locally on my linux
desktop,
but timing out on the bot.

any suggestions ?

[timvolodine, 2014-08-19 17:38:46]

https://codereview.chromium.org/292693004/diff/280001/content/test/data/devic...
File content/test/data/device_sensors/device_light_null_test_with_alert.html
(right):

https://codereview.chromium.org/292693004/diff/280001/content/test/data/devic...
content/test/data/device_sensors/device_light_null_test_with_alert.html:6:
return event.value == null;
should this be infinity?

[riju_, 2014-08-20 13:06:25]

Thanks for having a look Tim.

The bot is timing out, because on the blink side, RunTimeEnabled=test.
I was having =stable in my local branch, thats why it was fine for me.

When i put status=experimental, i need to pass
--enable-experimental-web-platform-features to content_browsertests [1]


[1] https://gist.github.com/riju/b2af86f1ccae8a6248c2

https://codereview.chromium.org/292693004/diff/280001/content/test/data/devic...
File content/test/data/device_sensors/device_light_null_test_with_alert.html
(right):

https://codereview.chromium.org/292693004/diff/280001/content/test/data/devic...
content/test/data/device_sensors/device_light_null_test_with_alert.html:6:
return event.value == null;
On 2014/08/19 17:38:45, timvolodine wrote:
> should this be infinity?

stupid me ! thanks Tim.
Done.

[riju_, 2014-09-04 09:02:16]

Hi Tim,
please take a look, hopefully final one. 
All tests pass now all bots are green. (patchset 9)

[timvolodine, 2014-09-04 17:16:58]

> All tests pass now all bots are green. (patchset 9)

cool, comments below:

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:58: return
true;
I assume this is for non-android platforms. Should the API return Inf in that
case? as it is now it looks like javascript will never get a callback.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:72: return
light_buffer_;
so this will return false because light_buffer_==0?

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:63: case
CONSUMER_TYPE_LIGHT:
indent -2

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:70: // Null
event scenario
if you allow UpdateLight to take a value as well this would be simpler:
e.g. UpdateLight(light_buffer, sensor_date_available_ ? 100 : infinity);

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:245: //
TODO(riju): remove command line args later
nit: later -> "when the feature goes stable"

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:247:
switches::kEnableExperimentalWebPlatformFeatures))
indentation?

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:276:
MAYBE_LightNullTestWithAlert) {
the test with alert is kind of involved, maybe just provide a "NullTest" where
an inf value is simply propagated without an alert. That way you can remove the
android ifdef.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:285:
switches::kEnableExperimentalWebPlatformFeatures))
indent

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/device_light_message_filter.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_light_message_filter.cc:13: :
BrowserMessageFilter(DeviceLightMsgStart), is_started_(false) {
I think multiline is preferred here, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_light_message_filter.cc:18: if
(is_started_)
use braces because multiple lines

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/inertial_sensor_consts.h (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:23: const int
kInertialLightSensorIntervalMillis = 199;
why 199?

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:23: const int
kInertialLightSensorIntervalMillis = 199;
maybe also mention where the corresponing renderer/ side constant should be
found.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:189: bool success =
Start(kTypeLight);
should the buffer be set to infinity when !success?

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:205:
device_light_buffer_->data.value = -1;
wrap with writeBegin writeEnd.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android_unittest.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android_unittest.cc:157:
sensorManager.StopFetchingDeviceLightData();
check that buffer->data == -1?

https://codereview.chromium.org/292693004/diff/380001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java
(right):

https://codereview.chromium.org/292693004/diff/380001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java:116:
success = registerSensors(DEVICE_LIGHT_SENSORS, rateInMilliseconds, false);
(..,.., true)?

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:13: const int
kDefaultLightPumpDelayMillis = 199;
why not 200?

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:41: // It seems that
we need to fire multiple times to blink to get 1 NULL
remove comment

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:44: data.value ==
std::numeric_limits<double>::infinity()) {
this does not look right.
you could do reader_->GetLatestData(&data) && ( .. || ..)

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:44: data.value ==
std::numeric_limits<double>::infinity()) {
also I am not sure if negative values could be fired into blink this way, looks
like it's not possible but I am not 100% sure.
to be on the safe side you could define ShouldFireEvent which would filter out
value<0.

https://codereview.chromium.org/292693004/diff/380001/content/test/data/devic...
File content/test/data/device_sensors/device_inertial_sensor_diagnostics.html
(right):

https://codereview.chromium.org/292693004/diff/380001/content/test/data/devic...
content/test/data/device_sensors/device_inertial_sensor_diagnostics.html:4:
<title>Device Motion/Orientation/Light diagnostic measurements</title>
could you please separate this into a separate file, e.g.
device_light_diagnostics.
this file is meant for device orientation/motion only.

[riju_, 2014-09-08 09:26:18]

Thanks Tim, followed most of your comments, apart from NullTest without alert ..
i tried a few times locally on android and it was not flaking .. so keeping the
alert as of now and just removing the #ifdef

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:58: return
true;
On 2014/09/04 17:16:57, timvolodine wrote:
> I assume this is for non-android platforms. Should the API return Inf in that
> case? as it is now it looks like javascript will never get a callback.

Ok, putting inf in the light buffer.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:72: return
light_buffer_;
On 2014/09/04 17:16:57, timvolodine wrote:
> so this will return false because light_buffer_==0?

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:63: case
CONSUMER_TYPE_LIGHT:
On 2014/09/04 17:16:57, timvolodine wrote:
> indent -2

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:70: // Null
event scenario
On 2014/09/04 17:16:57, timvolodine wrote:
> if you allow UpdateLight to take a value as well this would be simpler:
> e.g. UpdateLight(light_buffer, sensor_date_available_ ? 100 : infinity);

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:245: //
TODO(riju): remove command line args later
On 2014/09/04 17:16:57, timvolodine wrote:
> nit: later -> "when the feature goes stable"

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:247:
switches::kEnableExperimentalWebPlatformFeatures))
On 2014/09/04 17:16:57, timvolodine wrote:
> indentation?

git cl format tells me to keep it like this.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:276:
MAYBE_LightNullTestWithAlert) {
On 2014/09/04 17:16:57, timvolodine wrote:
> the test with alert is kind of involved, maybe just provide a "NullTest" where
> an inf value is simply propagated without an alert. That way you can remove
the
> android ifdef.

Let me take out #ifdef android and see its its flaking on the try bots.
Locally the test passes every time.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:285:
switches::kEnableExperimentalWebPlatformFeatures))
On 2014/09/04 17:16:57, timvolodine wrote:
> indent

git cl format tells me to keep it like this.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/device_light_message_filter.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_light_message_filter.cc:13: :
BrowserMessageFilter(DeviceLightMsgStart), is_started_(false) {
On 2014/09/04 17:16:57, timvolodine wrote:
> I think multiline is preferred here, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

1. git cl format tells me to do this way.
2. From the style guide, since it fits in 1 line, this way is ok, When it
requires multiple lines, indent 4 spaces...

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/device_light_message_filter.cc:18: if
(is_started_)
On 2014/09/04 17:16:57, timvolodine wrote:
> use braces because multiple lines

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/inertial_sensor_consts.h (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:23: const int
kInertialLightSensorIntervalMillis = 199;
On 2014/09/04 17:16:57, timvolodine wrote:
> maybe also mention where the corresponing renderer/ side constant should be
> found.

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:189: bool success =
Start(kTypeLight);
On 2014/09/04 17:16:57, timvolodine wrote:
> should the buffer be set to infinity when !success?

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:205:
device_light_buffer_->data.value = -1;
On 2014/09/04 17:16:57, timvolodine wrote:
> wrap with writeBegin writeEnd.

Done.

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android_unittest.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android_unittest.cc:157:
sensorManager.StopFetchingDeviceLightData();
On 2014/09/04 17:16:58, timvolodine wrote:
> check that buffer->data == -1?

Done.

https://codereview.chromium.org/292693004/diff/380001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java
(right):

https://codereview.chromium.org/292693004/diff/380001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/DeviceSensors.java:116:
success = registerSensors(DEVICE_LIGHT_SENSORS, rateInMilliseconds, false);
On 2014/09/04 17:16:58, timvolodine wrote:
> (..,.., true)?

Done.

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:41: // It seems that
we need to fire multiple times to blink to get 1 NULL
On 2014/09/04 17:16:58, timvolodine wrote:
> remove comment

Done.

https://codereview.chromium.org/292693004/diff/380001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:44: data.value ==
std::numeric_limits<double>::infinity()) {
On 2014/09/04 17:16:58, timvolodine wrote:
> this does not look right.
> you could do reader_->GetLatestData(&data) && ( .. || ..)

Done.

https://codereview.chromium.org/292693004/diff/380001/content/test/data/devic...
File content/test/data/device_sensors/device_inertial_sensor_diagnostics.html
(right):

https://codereview.chromium.org/292693004/diff/380001/content/test/data/devic...
content/test/data/device_sensors/device_inertial_sensor_diagnostics.html:4:
<title>Device Motion/Orientation/Light diagnostic measurements</title>
On 2014/09/04 17:16:58, timvolodine wrote:
> could you please separate this into a separate file, e.g.
> device_light_diagnostics.
> this file is meant for device orientation/motion only.

Done. I will add similar thing for Light in the next CL.

[timvolodine, 2014-09-08 15:20:19]

regarding null test with alert, I am pretty sure it will flake on our test
bots... but if you are committed to keep an eye on it and fix if it breaks I am
ok with it.

almost there ;), some more comments below.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:84: return
light_buffer_->data.value >= 0;
for consistency I think return SetLightBuffer(.., -1) is better, otherwise the
buffer is always +inf (after first start).

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:241: //
TODO(riju): remove command line args when the feature goes stable
this looks a bit dense, could you add white line?

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:241: //
TODO(riju): remove command line args when the feature goes stable
nit "." at eol.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:243:
switches::kEnableExperimentalWebPlatformFeatures))
this still looks weird to me, I think indent +4sp would be appropriate here.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:243:
switches::kEnableExperimentalWebPlatformFeatures))
also { } required when multiline

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:271:
fetcher_->SetSensorDataAvailable(false);
nit : add empty line for readability?

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:274:
switches::kEnableExperimentalWebPlatformFeatures))
{ .. }

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/inertial_sensor_consts.h (right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:22: //
|kDefaultLightPumpDelayMillis| in
could you add this comment directly above LightSensorIntervalMillis?

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:25: const int
kInertialLightSensorIntervalMillis = 200;
remove "inertial" from the name.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:187:
SetInternalLightBuffers(-1);
the name is confusing, maybe call it SetLightBufferValue?

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:50: if (lux !=
last_seen_data_ || lux == std::numeric_limits<double>::infinity())
could you add a comment regarding why infinity is special?
maybe something like this looks cleaner:
// ..
if (lux == infinity)
  return true;

return lux != last_seen_data;

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:50: if (lux !=
last_seen_data_ || lux == std::numeric_limits<double>::infinity())
also a unit test for this case would be desirable

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.h (right):

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:45: bool
ShouldFireEvent(double data) const;
does this need to be protected?

[riju_, 2014-09-09 14:01:18]

Tim, please take another look.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/data_fetcher_shared_memory_default.cc
(right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/data_fetcher_shared_memory_default.cc:84: return
light_buffer_->data.value >= 0;
On 2014/09/08 15:20:18, timvolodine wrote:
> for consistency I think return SetLightBuffer(.., -1) is better, otherwise the
> buffer is always +inf (after first start).

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:241: //
TODO(riju): remove command line args when the feature goes stable
On 2014/09/08 15:20:19, timvolodine wrote:
> this looks a bit dense, could you add white line?

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:241: //
TODO(riju): remove command line args when the feature goes stable
On 2014/09/08 15:20:19, timvolodine wrote:
> this looks a bit dense, could you add white line?

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:243:
switches::kEnableExperimentalWebPlatformFeatures))
On 2014/09/08 15:20:19, timvolodine wrote:
> this still looks weird to me, I think indent +4sp would be appropriate here.

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:243:
switches::kEnableExperimentalWebPlatformFeatures))
On 2014/09/08 15:20:19, timvolodine wrote:
> this still looks weird to me, I think indent +4sp would be appropriate here.

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:271:
fetcher_->SetSensorDataAvailable(false);
On 2014/09/08 15:20:19, timvolodine wrote:
> nit : add empty line for readability?

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:274:
switches::kEnableExperimentalWebPlatformFeatures))
On 2014/09/08 15:20:19, timvolodine wrote:
> { .. }

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/inertial_sensor_consts.h (right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:22: //
|kDefaultLightPumpDelayMillis| in
On 2014/09/08 15:20:19, timvolodine wrote:
> could you add this comment directly above LightSensorIntervalMillis?

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/inertial_sensor_consts.h:25: const int
kInertialLightSensorIntervalMillis = 200;
On 2014/09/08 15:20:19, timvolodine wrote:
> remove "inertial" from the name.

Done.

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/292693004/diff/420001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:187:
SetInternalLightBuffers(-1);
On 2014/09/08 15:20:19, timvolodine wrote:
> the name is confusing, maybe call it SetLightBufferValue?

Done.

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:50: if (lux !=
last_seen_data_ || lux == std::numeric_limits<double>::infinity())
On 2014/09/08 15:20:19, timvolodine wrote:
> could you add a comment regarding why infinity is special?
> maybe something like this looks cleaner:
> // ..
> if (lux == infinity)
>   return true;
> 
> return lux != last_seen_data;

Done.

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:50: if (lux !=
last_seen_data_ || lux == std::numeric_limits<double>::infinity())
On 2014/09/08 15:20:19, timvolodine wrote:
> could you add a comment regarding why infinity is special?
> maybe something like this looks cleaner:
> // ..
> if (lux == infinity)
>   return true;
> 
> return lux != last_seen_data;

Done.

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.h (right):

https://codereview.chromium.org/292693004/diff/420001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:45: bool
ShouldFireEvent(double data) const;
On 2014/09/08 15:20:19, timvolodine wrote:
> does this need to be protected?

Done.

[timvolodine, 2014-09-10 13:47:26]

lgtm with a few nits. basically I think there is some confusion with the use of
"null event" for the ambient light api..

jochen@ should have a look at this patch as well..

https://codereview.chromium.org/292693004/diff/460001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/460001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:270: //
expects to get an event with null(Inf) values. This tests that the one-off
you seem to use null events and inf values interchangeably this is confusing,
maybe use "one-off infinity event" as a generic term everywhere.

https://codereview.chromium.org/292693004/diff/460001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java
(right):

https://codereview.chromium.org/292693004/diff/460001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java:184:
DeviceSensors.DEVICE_LIGHT, 100);
i think this fits on one line?

https://codereview.chromium.org/292693004/diff/460001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/292693004/diff/460001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:51: // no data can be
provided, this is a null event.
nit: strictly speaking it's not a null event, something like this is probably
clearer:
-> no sensor data can be provided, fire an infinity event into blink.

https://codereview.chromium.org/292693004/diff/460001/content/test/data/devic...
File content/test/data/device_sensors/device_light_null_test.html (right):

https://codereview.chromium.org/292693004/diff/460001/content/test/data/devic...
content/test/data/device_sensors/device_light_null_test.html:1: <html>
also the file is better called device_light_infinity_test.html

https://codereview.chromium.org/292693004/diff/460001/content/test/data/devic...
content/test/data/device_sensors/device_light_null_test.html:3:
<title>DeviceLight all-null event test with alert</title>
"all-null event" -> one-off infinity event

[riju_, 2014-09-11 11:05:11]

jochen@ : please take another look.

[jochen (gone - plz use gerrit), 2014-09-12 13:57:46]

stuff outside device_sensors lgtm with nit addressed

https://codereview.chromium.org/292693004/diff/460001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/292693004/diff/460001/content/content_browser...
content/content_browser.gypi:478:
'browser/device_sensors/device_light_message_filter.h',
please also update the BUILD.gn file (in content/browser)

[riju_, 2014-09-22 08:22:21]

https://codereview.chromium.org/292693004/diff/460001/content/browser/device_...
File content/browser/device_sensors/device_inertial_sensor_browsertest.cc
(right):

https://codereview.chromium.org/292693004/diff/460001/content/browser/device_...
content/browser/device_sensors/device_inertial_sensor_browsertest.cc:270: //
expects to get an event with null(Inf) values. This tests that the one-off
On 2014/09/10 13:47:25, timvolodine wrote:
> you seem to use null events and inf values interchangeably this is confusing,
> maybe use "one-off infinity event" as a generic term everywhere.

Done.

https://codereview.chromium.org/292693004/diff/460001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/292693004/diff/460001/content/content_browser...
content/content_browser.gypi:478:
'browser/device_sensors/device_light_message_filter.h',
On 2014/09/12 13:57:46, jochen wrote:
> please also update the BUILD.gn file (in content/browser)

If i understood correctly, the content/browser/browser.gni uses the
gypi_to_gn.py script to read the gypi values.
I  checked the gn trybots, the 2 above files are included in the compilation.
Also these 2 files are not excluded in any build. 

I am cq ing this CL now, i can check gn related issues in a subsequent CL.

https://codereview.chromium.org/292693004/diff/460001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java
(right):

https://codereview.chromium.org/292693004/diff/460001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java:184:
DeviceSensors.DEVICE_LIGHT, 100);
On 2014/09/10 13:47:25, timvolodine wrote:
> i think this fits on one line?

No. Its 87 chars if I put in a single line, git cl format tells me to use 2
lines.

https://codereview.chromium.org/292693004/diff/460001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/292693004/diff/460001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:51: // no data can be
provided, this is a null event.
On 2014/09/10 13:47:25, timvolodine wrote:
> nit: strictly speaking it's not a null event, something like this is probably
> clearer:
> -> no sensor data can be provided, fire an infinity event into blink.

Done.

https://codereview.chromium.org/292693004/diff/460001/content/test/data/devic...
File content/test/data/device_sensors/device_light_null_test.html (right):

https://codereview.chromium.org/292693004/diff/460001/content/test/data/devic...
content/test/data/device_sensors/device_light_null_test.html:1: <html>
On 2014/09/10 13:47:25, timvolodine wrote:
> also the file is better called device_light_infinity_test.html

Done.

https://codereview.chromium.org/292693004/diff/460001/content/test/data/devic...
content/test/data/device_sensors/device_light_null_test.html:3:
<title>DeviceLight all-null event test with alert</title>
On 2014/09/10 13:47:25, timvolodine wrote:
> "all-null event" -> one-off infinity event

Done.

[riju_, 2014-09-22 08:24:12]

The CQ bit was checked by rijubrata.bhaumik@intel.com

[commit-bot: I haz the power, 2014-09-22 08:25:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/292693004/500001

[commit-bot: I haz the power, 2014-09-22 08:30:35]

Committed patchset #16 (id:500001) as 26316d740c731639bef927faa64b543f11d16853

[commit-bot: I haz the power, 2014-09-22 08:31:09]

Patchset 16 (id:??) landed as
https://crrev.com/f07da3973fd2bc8cdd739fed2522634bd475b2d2
Cr-Commit-Position: refs/heads/master@{#295937}

[timvolodine, 2014-09-22 11:12:37]

https://codereview.chromium.org/292693004/diff/460001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java
(right):

https://codereview.chromium.org/292693004/diff/460001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/DeviceSensorsTest.java:184:
DeviceSensors.DEVICE_LIGHT, 100);
On 2014/09/22 08:22:21, riju_(OOO) wrote:
> On 2014/09/10 13:47:25, timvolodine wrote:
> > i think this fits on one line?
> 
> No. Its 87 chars if I put in a single line, git cl format tells me to use 2
> lines.

for java files we have a limit of 100 chars