[sensors]: Introduce the Generic Sensor API.

[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

Intent to Implement :
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TkfdVqYAYiE

BUG=606766
Committed: https://crrev.com/ea88054ae4e7cd154177bb23426271fc0d6906ab
Cr-Commit-Position: refs/heads/master@{#396707}

[riju_, 2016-05-03 10:54:44]

Patchset #2 (id:20001) has been deleted

[riju_, 2016-05-03 12:32:27]

Patchset #2 (id:40001) has been deleted

[riju_, 2016-05-04 06:51:58]

rijubrata.bhaumik@intel.com changed reviewers:
+ timvolodine@chromium.org

[riju_, 2016-05-04 06:51:59]

Hi Tim, 
Please take a look.

[timvolodine, 2016-05-06 17:04:30]

Thanks for the patch, I've left some initial comments. I realize some of them
are pretty general, so I posted some issues on the github spec as well.

While reviewing this patch I tried to look at your other more general patch as
well and realized I had some questions regarding the derivation of AmbientLight
from Sensor. Not sure if it would make sense to include the concrete
implementation of AmbientLight in this patch at some stage. It would make this
patch heavy of course, so we better keep it as is for now. We could just iterate
incrementally not sure what's easier..

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
DocumentCreateEventSensorErrorEvent = 1334,
doesn't seem to be used anywhere in this patch unless I am missing something?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:9: #include
"core/dom/DOMException.h"
not used?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:19:
Sensor::Sensor(ExecutionContext* executionContext, const SensorOptions&
sensorOptions)
the class doesn't seem to do much at this stage.. is there code that can be put
here?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:24: 
no state() implementation as you have in the other patch
https://codereview.chromium.org/1892083002 ?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:28: return
*m_sensorReading.get();
I understand reading can be nullable, how is that going to be handled?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:32: {
will these remain empty? otherwise add todo

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:45: return false;
I assume this needs to be overriden in deriving classes, so maybe make it pure
virtual?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.idl:12: "errored"
should there be something corresponding to 'there is no relevant hardware
sensor'? also, would "idle" be better called "uninitialized"?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorErrorEventInit.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorErrorEventInit.idl:9: //Error
error;
should this be here?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorOptions.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorOptions.idl:1: // Copyright 2015
The Chromium Authors. All rights reserved.
2016 ;)

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorOptions.idl:9: double? frequency;
I think we generally use 4sp indent in idls

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.cpp:10: :
m_timeStamp(0.0)
hmm should we allow this? I guess this would correspond to 'nullable', but maybe
then should be -1 or something like that.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.h:12: #include
"platform/heap/Handle.h"
are Handle and AtomicString really needed here?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.idl:12: readonly
attribute DOMHighResTimeStamp timeStamp;
I assume concrete sensors would derive from SensorReading. However do we always
need a timeStamp? looks like can be potentially overhead for very hi-frequency
sensors. Maybe it should be nullable or not there at all?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:11: readonly
attribute SensorReading reading;
is there a reason to have both SensorReadingEvent and SensorReading? would
SensorReadingEvent be sufficient?

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71972: +  <int value="1334"
label="DocumentCreateEventSensorErrorEvent"/>
is this used anywhere in this patch?

[timvolodine, 2016-05-06 17:07:04]

oh, on a related note: do you have intent to implement on blink-dev for this?

[riju_, 2016-05-12 14:10:11]

Description was changed from

==========
[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

BUG=606766
==========

to

==========
[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

Intent to Implement : 
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TkfdVqYAYiE

BUG=606766
==========

[riju_, 2016-05-12 14:11:51]

On 2016/05/06 17:07:04, timvolodine wrote:
> oh, on a related note: do you have intent to implement on blink-dev for this?

Yes. Now I have updated the description.

[riju_, 2016-05-12 14:12:46]

Thanks Tim, Fixed most of the issues, for spec related ones, lets find consensus
on github.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
DocumentCreateEventSensorErrorEvent = 1334,
On 2016/05/06 17:04:29, timvolodine wrote:
> doesn't seem to be used anywhere in this patch unless I am missing something?

Acknowledged. But I am creating a SensorErrorEvent idl, and the generated files
tell me UseCounter is needed. I guess we need to use this in a subsequent CL
when we construct a ALS sensor and then find out that there is no hardware. So
yes, its not used in this CL, but its part of general Sensor infrastructure,
hence i am keeping this in this CL.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:9: #include
"core/dom/DOMException.h"
On 2016/05/06 17:04:29, timvolodine wrote:
> not used?

Acknowledged.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:19:
Sensor::Sensor(ExecutionContext* executionContext, const SensorOptions&
sensorOptions)
On 2016/05/06 17:04:29, timvolodine wrote:
> the class doesn't seem to do much at this stage.. is there code that can be
put
> here?

Only thing i am adding is m_sensorState(SensorState::Idle), so that when
concrete sensors are created, the state is idle by default.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:24: 
On 2016/05/06 17:04:29, timvolodine wrote:
> no state() implementation as you have in the other patch
> https://codereview.chromium.org/1892083002 ?

I was adding the "state" part in the next CL, but I will put it here now.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:28: return
*m_sensorReading.get();
On 2016/05/06 17:04:29, timvolodine wrote:
> I understand reading can be nullable, how is that going to be handled?

Acknowledged.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:32: {
On 2016/05/06 17:04:29, timvolodine wrote:
> will these remain empty? otherwise add todo

As suggested by you
(https://codereview.chromium.org/1892083002/diff/1/third_party/WebKit/Source/m...
Line 119), 
I could add the " m_hasEventListener = false;" 
But then Sensor class should inherit PlatformEventController whose primary job
is to be a 
Base controller class for registering controllers with a dispatcher.

Concrete sensors should have their own Dispatchers, hence this design.
I am adding a comment saying they will we overriden by deriving classes.
Sounds Ok?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:45: return false;
On 2016/05/06 17:04:29, timvolodine wrote:
> I assume this needs to be overriden in deriving classes, so maybe make it pure
> virtual?

yes. removing it all together from here. In ActiveScriptWrappable,
hasPendingActivity is pure virtual, so Sensor does inherit that.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.idl:12: "errored"
On 2016/05/06 17:04:30, timvolodine wrote:
> should there be something corresponding to 'there is no relevant hardware
> sensor'? also, would "idle" be better called "uninitialized"?

Spec issues: Let us resolve them in the spec discussions.
https://github.com/w3c/sensors/issues/102
https://github.com/w3c/sensors/issues/104

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorErrorEventInit.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorErrorEventInit.idl:9: //Error
error;
On 2016/05/06 17:04:30, timvolodine wrote:
> should this be here?

Done.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorOptions.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorOptions.idl:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/05/06 17:04:30, timvolodine wrote:
> 2016 ;)

;)

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorOptions.idl:9: double? frequency;
On 2016/05/06 17:04:30, timvolodine wrote:
> I think we generally use 4sp indent in idls

Done.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.cpp:10: :
m_timeStamp(0.0)
On 2016/05/06 17:04:30, timvolodine wrote:
> hmm should we allow this? I guess this would correspond to 'nullable', but
maybe
> then should be -1 or something like that.

Making it -1 now.
Wider spec issue : https://github.com/w3c/sensors/issues/105

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.h:12: #include
"platform/heap/Handle.h"
On 2016/05/06 17:04:30, timvolodine wrote:
> are Handle and AtomicString really needed here?

Done.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.idl:12: readonly
attribute DOMHighResTimeStamp timeStamp;
On 2016/05/06 17:04:30, timvolodine wrote:
> I assume concrete sensors would derive from SensorReading. However do we
always
> need a timeStamp? looks like can be potentially overhead for very hi-frequency
> sensors. Maybe it should be nullable or not there at all?

Spec issue raised it in https://github.com/w3c/sensors/issues/105

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:11: readonly
attribute SensorReading reading;
On 2016/05/06 17:04:30, timvolodine wrote:
> is there a reason to have both SensorReadingEvent and SensorReading? would
> SensorReadingEvent be sufficient?

Spec issue : https://github.com/w3c/sensors/issues/103

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71972: +  <int value="1334"
label="DocumentCreateEventSensorErrorEvent"/>
On 2016/05/06 17:04:30, timvolodine wrote:
> is this used anywhere in this patch?

No. Not in this patch but I am creating a SensorErrorEvent idl, and the
generated files
tell me UseCounter is needed. I guess we need to use this in a subsequent CL
when we construct a ALS sensor and then find out that there is no hardware. So
yes, its not used in this CL, but its part of general Sensor infrastructure,
hence i am keeping this in this CL.

[timvolodine, 2016-05-17 17:44:40]

Thanks Riju for following up on the issues, some more comments/questions below

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
DocumentCreateEventSensorErrorEvent = 1334,
On 2016/05/12 14:12:45, riju_ wrote:
> On 2016/05/06 17:04:29, timvolodine wrote:
> > doesn't seem to be used anywhere in this patch unless I am missing
something?
> 
> Acknowledged. But I am creating a SensorErrorEvent idl, and the generated
files
> tell me UseCounter is needed. I guess we need to use this in a subsequent CL
> when we construct a ALS sensor and then find out that there is no hardware. So
> yes, its not used in this CL, but its part of general Sensor infrastructure,
> hence i am keeping this in this CL.

hmm weird, do you mean the compiler complains about usecounter. Do we now need a
constant for every idl we make? seems unnecessary to measure this since the
sensor classes are meant to be subclassed aren't they?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:32: {
On 2016/05/12 14:12:45, riju_ wrote:
> On 2016/05/06 17:04:29, timvolodine wrote:
> > will these remain empty? otherwise add todo
> 
> As suggested by you
>
(https://codereview.chromium.org/1892083002/diff/1/third_party/WebKit/Source/m...
> Line 119), 
> I could add the " m_hasEventListener = false;" 
> But then Sensor class should inherit PlatformEventController whose primary job
> is to be a 
> Base controller class for registering controllers with a dispatcher.
> 
> Concrete sensors should have their own Dispatchers, hence this design.
> I am adding a comment saying they will we overriden by deriving classes.
> Sounds Ok?

so yes, maybe Sensor should inherit from PlatformEventController? That way it
would listen to page visibility as well, and that is something we would want for
sensors I believe.

also, if these methods are empty anyway could they be removed altogether? (and
maybe keep some kind of comment)

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.cpp:10: :
m_timeStamp(0.0)
On 2016/05/12 14:12:46, riju_ wrote:
> On 2016/05/06 17:04:30, timvolodine wrote:
> > hmm should we allow this? I guess this would correspond to 'nullable', but
> maybe
> > then should be -1 or something like that.
> 
> Making it -1 now.
> Wider spec issue : https://github.com/w3c/sensors/issues/105

hmm, I think I would actually prefer it to be nullable..

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:11: readonly
attribute SensorReading reading;
On 2016/05/12 14:12:46, riju_ wrote:
> On 2016/05/06 17:04:30, timvolodine wrote:
> > is there a reason to have both SensorReadingEvent and SensorReading? would
> > SensorReadingEvent be sufficient?
> 
> Spec issue : https://github.com/w3c/sensors/issues/103

yeah may be possible to drop the reading here.. would simplify implementation
down the road quite a bit

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71972: +  <int value="1334"
label="DocumentCreateEventSensorErrorEvent"/>
On 2016/05/12 14:12:46, riju_ wrote:
> On 2016/05/06 17:04:30, timvolodine wrote:
> > is this used anywhere in this patch?
> 
> No. Not in this patch but I am creating a SensorErrorEvent idl, and the
> generated files
> tell me UseCounter is needed. I guess we need to use this in a subsequent CL
> when we construct a ALS sensor and then find out that there is no hardware. So
> yes, its not used in this CL, but its part of general Sensor infrastructure,
> hence i am keeping this in this CL.

confused here, I thought we use histograms.xml for things we actually record the
statics on?

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:29: case SensorState::Idle:
indentation

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:46: return
m_sensorReading.get();
could you just return m_sensorReading.get() here without the if above?

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:51: void Sensor::suspend()
if the methods inherited from ActiveDOMObject are empty here anyway why bother
overriding them?

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/sensor_state_type.h (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/sensor_state_type.h:16: ENUM_MAX_VALUE
= Errored
is this common practice in blink? where is it needed?

[haraken, 2016-05-17 23:42:46]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-17 23:42:48]

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:51: void Sensor::suspend()
On 2016/05/17 17:44:39, timvolodine wrote:
> if the methods inherited from ActiveDOMObject are empty here anyway why bother
> overriding them?

If you don't have a plan to do something on suspend() and resume(), you don't
need to inherit from ActiveDOMObject.

In common cases, inheriting from ContextLifecycleObserver would be enough (then
you can observe contextDestroyed()).

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.idl (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.idl:17: DependentLifetime,

Remove this.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorErrorEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorErrorEvent.idl:9:
Constructor(DOMString type, SensorErrorEventInit errorEventInitDict)]

] should be on the next line.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:9:
Constructor(DOMString type, SensorReadingEventInit eventInitDict)]

Ditto.

[riju_, 2016-05-19 13:26:02]

Patchset #5 (id:120001) has been deleted

[riju_, 2016-05-20 11:20:11]

Thanks Tim and Haraken for the comments, 
@Tim : outstanding issue : https://github.com/w3c/sensors/issues/103

Can we progress on this CL assuming current spec stays, and when we get the
consensus in spec, make changes in a later CL?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
DocumentCreateEventSensorErrorEvent = 1334,
On 2016/05/17 17:44:39, timvolodine wrote:
> On 2016/05/12 14:12:45, riju_ wrote:
> > On 2016/05/06 17:04:29, timvolodine wrote:
> > > doesn't seem to be used anywhere in this patch unless I am missing
> something?
> > 
> > Acknowledged. But I am creating a SensorErrorEvent idl, and the generated
> files
> > tell me UseCounter is needed. I guess we need to use this in a subsequent CL
> > when we construct a ALS sensor and then find out that there is no hardware.
So
> > yes, its not used in this CL, but its part of general Sensor infrastructure,
> > hence i am keeping this in this CL.
> 
> hmm weird, do you mean the compiler complains about usecounter. Do we now need
a
> constant for every idl we make? seems unnecessary to measure this since the
> sensor classes are meant to be subclassed aren't they?

SensorErrorEvent and SensorReadEvent are not subclassed.

Could be wrong, but, maybe things changed after
https://codereview.chromium.org/1673243002 ?

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:32: {
On 2016/05/17 17:44:39, timvolodine wrote:
> On 2016/05/12 14:12:45, riju_ wrote:
> > On 2016/05/06 17:04:29, timvolodine wrote:
> > > will these remain empty? otherwise add todo
> > 
> > As suggested by you
> >
>
(https://codereview.chromium.org/1892083002/diff/1/third_party/WebKit/Source/m...
> > Line 119), 
> > I could add the " m_hasEventListener = false;" 
> > But then Sensor class should inherit PlatformEventController whose primary
job
> > is to be a 
> > Base controller class for registering controllers with a dispatcher.
> > 
> > Concrete sensors should have their own Dispatchers, hence this design.
> > I am adding a comment saying they will we overriden by deriving classes.
> > Sounds Ok?
> 
> so yes, maybe Sensor should inherit from PlatformEventController? That way it
> would listen to page visibility as well, and that is something we would want
for
> sensors I believe.
> 
> also, if these methods are empty anyway could they be removed altogether? (and
> maybe keep some kind of comment)

Done.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReading.cpp (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReading.cpp:10: :
m_timeStamp(0.0)
On 2016/05/17 17:44:39, timvolodine wrote:
> On 2016/05/12 14:12:46, riju_ wrote:
> > On 2016/05/06 17:04:30, timvolodine wrote:
> > > hmm should we allow this? I guess this would correspond to 'nullable', but
> > maybe
> > > then should be -1 or something like that.
> > 
> > Making it -1 now.
> > Wider spec issue : https://github.com/w3c/sensors/issues/105
> 
> hmm, I think I would actually prefer it to be nullable..

Done.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:11: readonly
attribute SensorReading reading;
On 2016/05/17 17:44:39, timvolodine wrote:
> On 2016/05/12 14:12:46, riju_ wrote:
> > On 2016/05/06 17:04:30, timvolodine wrote:
> > > is there a reason to have both SensorReadingEvent and SensorReading? would
> > > SensorReadingEvent be sufficient?
> > 
> > Spec issue : https://github.com/w3c/sensors/issues/103
> 
> yeah may be possible to drop the reading here.. would simplify implementation
> down the road quite a bit

Can we keep this for the time being? 
I am working with the spec authors and others to find consensus.
We can change the code, when the spec changes.

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1942663003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71972: +  <int value="1334"
label="DocumentCreateEventSensorErrorEvent"/>
On 2016/05/17 17:44:39, timvolodine wrote:
> On 2016/05/12 14:12:46, riju_ wrote:
> > On 2016/05/06 17:04:30, timvolodine wrote:
> > > is this used anywhere in this patch?
> > 
> > No. Not in this patch but I am creating a SensorErrorEvent idl, and the
> > generated files
> > tell me UseCounter is needed. I guess we need to use this in a subsequent CL
> > when we construct a ALS sensor and then find out that there is no hardware.
So
> > yes, its not used in this CL, but its part of general Sensor infrastructure,
> > hence i am keeping this in this CL.
> 
> confused here, I thought we use histograms.xml for things we actually record
the
> statics on?

Maybe things changed after
https://codereview.chromium.org/1673243002

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:29: case SensorState::Idle:
On 2016/05/17 17:44:39, timvolodine wrote:
> indentation

check-webkit-style does not complain
https://www.chromium.org/blink/coding-style 
A case label should line up with its switch statement. The case statement is
indented.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:46: return
m_sensorReading.get();
On 2016/05/17 17:44:40, timvolodine wrote:
> could you just return m_sensorReading.get() here without the if above?

Done.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:51: void Sensor::suspend()
On 2016/05/17 17:44:39, timvolodine wrote:
> if the methods inherited from ActiveDOMObject are empty here anyway why bother
> overriding them?

These are not empty any more.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.idl (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.idl:17: DependentLifetime,
On 2016/05/17 23:42:48, haraken wrote:
> 
> Remove this.

Done.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorErrorEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorErrorEvent.idl:9:
Constructor(DOMString type, SensorErrorEventInit errorEventInitDict)]
On 2016/05/17 23:42:48, haraken wrote:
> 
> ] should be on the next line.

Done.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:9:
Constructor(DOMString type, SensorReadingEventInit eventInitDict)]
On 2016/05/17 23:42:48, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/sensor_state_type.h (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/sensor_state_type.h:16: ENUM_MAX_VALUE
= Errored
On 2016/05/17 17:44:40, timvolodine wrote:
> is this common practice in blink? where is it needed?

not a practise in Blink, removing it

[timvolodine, 2016-05-24 15:30:36]

> Can we progress on this CL assuming current spec stays, and when we get the
consensus in spec, make changes in a later CL?

sounds reasonable :)

looks good in general thanks! a few more comments below

also, pls check indentation in *.idl for consistency.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
DocumentCreateEventSensorErrorEvent = 1334,
On 2016/05/20 11:20:11, riju_ wrote:
> On 2016/05/17 17:44:39, timvolodine wrote:
> > On 2016/05/12 14:12:45, riju_ wrote:
> > > On 2016/05/06 17:04:29, timvolodine wrote:
> > > > doesn't seem to be used anywhere in this patch unless I am missing
> > something?
> > > 
> > > Acknowledged. But I am creating a SensorErrorEvent idl, and the generated
> > files
> > > tell me UseCounter is needed. I guess we need to use this in a subsequent
CL
> > > when we construct a ALS sensor and then find out that there is no
hardware.
> So
> > > yes, its not used in this CL, but its part of general Sensor
infrastructure,
> > > hence i am keeping this in this CL.
> > 
> > hmm weird, do you mean the compiler complains about usecounter. Do we now
need
> a
> > constant for every idl we make? seems unnecessary to measure this since the
> > sensor classes are meant to be subclassed aren't they?
> 
> SensorErrorEvent and SensorReadEvent are not subclassed.
> 
> Could be wrong, but, maybe things changed after
> https://codereview.chromium.org/1673243002 ?

yeah the patch you are refereing to makes sense.. don't see histograms.xml there
though, not sure if it's needed in this patch (?) but it's fine with me

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:11: readonly
attribute SensorReading reading;
On 2016/05/20 11:20:11, riju_ wrote:
> On 2016/05/17 17:44:39, timvolodine wrote:
> > On 2016/05/12 14:12:46, riju_ wrote:
> > > On 2016/05/06 17:04:30, timvolodine wrote:
> > > > is there a reason to have both SensorReadingEvent and SensorReading?
would
> > > > SensorReadingEvent be sufficient?
> > > 
> > > Spec issue : https://github.com/w3c/sensors/issues/103
> > 
> > yeah may be possible to drop the reading here.. would simplify
implementation
> > down the road quite a bit
> 
> Can we keep this for the time being? 
> I am working with the spec authors and others to find consensus.
> We can change the code, when the spec changes.

sgtm ;)

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:29: case SensorState::Idle:
On 2016/05/20 11:20:11, riju_ wrote:
> On 2016/05/17 17:44:39, timvolodine wrote:
> > indentation
> 
> check-webkit-style does not complain
> https://www.chromium.org/blink/coding-style 
> A case label should line up with its switch statement. The case statement is
> indented.

yes sorry you are correct, this should follow the blink-style

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:78:
dispatchEvent(Event::create(EventTypeNames::statechange));
this may need a bit of thinking regarding re-entrancy, is this safe? e.g.
imagine a user executing sensor.start and sensor.stop inside 'onstatechange'
event handler.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:96: stopUpdating();
clear m_sensorReading?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:9: #include
"bindings/core/v8/ScriptPromise.h"
is this needed?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:17: #include
"modules/sensor/SensorReading.h"
you are forward declaring it below? just remove this include?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:28: : public
EventTargetWithInlineData
I think we usually put commas on previous line ?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:67: Member<SensorReading>
m_sensorReading;
probably also protected?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:68: SensorOptions
m_sensorOptions;
and this?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReading.h:31:
DECLARE_VIRTUAL_TRACE();
for consistency best to put as below as last statement in public section

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReading.h:37: SensorReading();
private or protected? since we have create methods..

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:41:
DECLARE_VIRTUAL_TRACE();
nit: put below

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:43: SensorReading*
reading() { return m_reading; }
const method? and what about returning pointer to const reading?
the latter question also applies to sensor.reading

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:46:
SensorReadingEvent();
should constructors be protected/private?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:9:
Constructor(DOMString type, SensorReadingEventInit eventInitDict)
indentation consistency with other idls?

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEventInit.idl
(right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEventInit.idl:9:
SensorReading reading;
indentation as with other idls? I think it should be 4sp

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/sensor_state_type.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/sensor_state_type.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
should the file name follow the /modules names e.g. SensorState.h ?

[riju_, 2016-05-25 12:17:38]

Patchset #7 (id:180001) has been deleted

[riju_, 2016-05-25 15:07:16]

rijubrata.bhaumik@intel.com changed reviewers:
+ isherman@chromium.org

[riju_, 2016-05-25 15:07:18]

Thanks Tim for the review. Comments inline.

@Ilya : histograms.xml owner.

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1942663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1152:
DocumentCreateEventSensorErrorEvent = 1334,
On 2016/05/24 15:30:35, timvolodine wrote:
> On 2016/05/20 11:20:11, riju_ wrote:
> > On 2016/05/17 17:44:39, timvolodine wrote:
> > > On 2016/05/12 14:12:45, riju_ wrote:
> > > > On 2016/05/06 17:04:29, timvolodine wrote:
> > > > > doesn't seem to be used anywhere in this patch unless I am missing
> > > something?
> > > > 
> > > > Acknowledged. But I am creating a SensorErrorEvent idl, and the
generated
> > > files
> > > > tell me UseCounter is needed. I guess we need to use this in a
subsequent
> CL
> > > > when we construct a ALS sensor and then find out that there is no
> hardware.
> > So
> > > > yes, its not used in this CL, but its part of general Sensor
> infrastructure,
> > > > hence i am keeping this in this CL.
> > > 
> > > hmm weird, do you mean the compiler complains about usecounter. Do we now
> need
> > a
> > > constant for every idl we make? seems unnecessary to measure this since
the
> > > sensor classes are meant to be subclassed aren't they?
> > 
> > SensorErrorEvent and SensorReadEvent are not subclassed.
> > 
> > Could be wrong, but, maybe things changed after
> > https://codereview.chromium.org/1673243002 ?
> 
> yeah the patch you are refereing to makes sense.. don't see histograms.xml
there
> though, not sure if it's needed in this patch (?) but it's fine with me

After this CL, https://codereview.chromium.org/1745913002/
if there is a change in Feature enum, we need to update histogram, else clang
format complains.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:78:
dispatchEvent(Event::create(EventTypeNames::statechange));
On 2016/05/24 15:30:35, timvolodine wrote:
> this may need a bit of thinking regarding re-entrancy, is this safe? e.g.
> imagine a user executing sensor.start and sensor.stop inside 'onstatechange'
> event handler.
Adding,
if (m_sensorState == newState)
   return;

and with the state checks in start() and stop(), it will throw exceptions

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:96: stopUpdating();
On 2016/05/24 15:30:35, timvolodine wrote:
> clear m_sensorReading?

for some sensors say geolocation, it is useful to have a
requestReading(cache:true) or just reading() method, asking for a reading from
cache, if its not stale for say more than 5 minutes. 
So within start() and stop(), i.e. sensor is active, preferred way is
event.reading.illuminance
and for a one-shot reading or when the sensor is not active (last useful
reading) , sensor.reading
That's why I did not want to clear sensorReading.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:9: #include
"bindings/core/v8/ScriptPromise.h"
On 2016/05/24 15:30:35, timvolodine wrote:
> is this needed?

No. Sorry, an older version used requestReading() which returned a promise.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:17: #include
"modules/sensor/SensorReading.h"
On 2016/05/24 15:30:35, timvolodine wrote:
> you are forward declaring it below? just remove this include?

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:28: : public
EventTargetWithInlineData
On 2016/05/24 15:30:35, timvolodine wrote:
> I think we usually put commas on previous line ?

Chromium style "clang-format" puts commas on previous line, but in Blink
"check-webkit-style", this is the way.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:67: Member<SensorReading>
m_sensorReading;
On 2016/05/24 15:30:35, timvolodine wrote:
> probably also protected?

In a previous patchset, i made it private to restrict access, reverting it back.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:68: SensorOptions
m_sensorOptions;
On 2016/05/24 15:30:35, timvolodine wrote:
> and this?

same

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReading.h:31:
DECLARE_VIRTUAL_TRACE();
On 2016/05/24 15:30:35, timvolodine wrote:
> for consistency best to put as below as last statement in public section

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReading.h:37: SensorReading();
On 2016/05/24 15:30:35, timvolodine wrote:
> private or protected? since we have create methods..

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:41:
DECLARE_VIRTUAL_TRACE();
On 2016/05/24 15:30:35, timvolodine wrote:
> nit: put below

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:43: SensorReading*
reading() { return m_reading; }
On 2016/05/24 15:30:35, timvolodine wrote:
> const method? and what about returning pointer to const reading?
> the latter question also applies to sensor.reading
Const method yes. Checked with the other getters, and looks like this is the
norm.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:46:
SensorReadingEvent();
On 2016/05/24 15:30:35, timvolodine wrote:
> should constructors be protected/private?

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.idl:9:
Constructor(DOMString type, SensorReadingEventInit eventInitDict)
On 2016/05/24 15:30:35, timvolodine wrote:
> indentation consistency with other idls?

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEventInit.idl
(right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEventInit.idl:9:
SensorReading reading;
On 2016/05/24 15:30:35, timvolodine wrote:
> indentation as with other idls? I think it should be 4sp

Done.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/sensor_state_type.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/sensor_state_type.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/05/24 15:30:36, timvolodine wrote:
> should the file name follow the /modules names e.g. SensorState.h ?

Done.

[timvolodine, 2016-05-25 15:40:22]

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:78:
dispatchEvent(Event::create(EventTypeNames::statechange));
On 2016/05/25 15:07:17, riju_ wrote:
> On 2016/05/24 15:30:35, timvolodine wrote:
> > this may need a bit of thinking regarding re-entrancy, is this safe? e.g.
> > imagine a user executing sensor.start and sensor.stop inside 'onstatechange'
> > event handler.
> Adding,
> if (m_sensorState == newState)
>    return;
> 
> and with the state checks in start() and stop(), it will throw exceptions

don't think this is sufficient.
if I am not overlooking anything the situation would be as follows: stop is
executed in 'onstatechange' it will call stopUpdating and then 'return' and
proceed with Sensor::start calling startUpdating()...

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:96: stopUpdating();
On 2016/05/25 15:07:17, riju_ wrote:
> On 2016/05/24 15:30:35, timvolodine wrote:
> > clear m_sensorReading?
> 
> for some sensors say geolocation, it is useful to have a
> requestReading(cache:true) or just reading() method, asking for a reading from
> cache, if its not stale for say more than 5 minutes. 
> So within start() and stop(), i.e. sensor is active, preferred way is
> event.reading.illuminance
> and for a one-shot reading or when the sensor is not active (last useful
> reading) , sensor.reading
> That's why I did not want to clear sensorReading.

caching is nice, but I was thinking in terms of usage which can be confusing
IMO:
sensorA.stop() then after x hours the user does sensorA.reading... (forgetting
to sensorA.start) and gets stale data..

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:43: SensorReading*
reading() { return m_reading; }
On 2016/05/25 15:07:18, riju_ wrote:
> On 2016/05/24 15:30:35, timvolodine wrote:
> > const method? and what about returning pointer to const reading?
> > the latter question also applies to sensor.reading
> Const method yes. Checked with the other getters, and looks like this is the
> norm.
> 

alright, could you file a crbug and put a TODO to consider making it return
pointer to const? generally don't think we want the users be able to change
m_reading.

[Ilya Sherman, 2016-05-25 17:15:58]

histograms.xml lgtm

[riju_, 2016-05-26 09:04:08]

Patchset #8 (id:220001) has been deleted

[riju_, 2016-05-26 11:05:33]

Patchset #8 (id:240001) has been deleted

[riju_, 2016-05-26 13:21:02]

hi Tim, fixed the re entrancy issue you pointed out. PTAL.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:78:
dispatchEvent(Event::create(EventTypeNames::statechange));
On 2016/05/25 15:40:22, timvolodine wrote:
> On 2016/05/25 15:07:17, riju_ wrote:
> > On 2016/05/24 15:30:35, timvolodine wrote:
> > > this may need a bit of thinking regarding re-entrancy, is this safe? e.g.
> > > imagine a user executing sensor.start and sensor.stop inside
'onstatechange'
> > > event handler.
> > Adding,
> > if (m_sensorState == newState)
> >    return;
> > 
> > and with the state checks in start() and stop(), it will throw exceptions
> 
> don't think this is sufficient.
> if I am not overlooking anything the situation would be as follows: stop is
> executed in 'onstatechange' it will call stopUpdating and then 'return' and
> proceed with Sensor::start calling startUpdating()...

Yes, you are right.
Now I have checked with adding start() / stop() in onstatechange event handler.
It throws an exception and halts.

https://codereview.chromium.org/1942663003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:96: stopUpdating();
On 2016/05/25 15:40:22, timvolodine wrote:
> On 2016/05/25 15:07:17, riju_ wrote:
> > On 2016/05/24 15:30:35, timvolodine wrote:
> > > clear m_sensorReading?
> > 
> > for some sensors say geolocation, it is useful to have a
> > requestReading(cache:true) or just reading() method, asking for a reading
from
> > cache, if its not stale for say more than 5 minutes. 
> > So within start() and stop(), i.e. sensor is active, preferred way is
> > event.reading.illuminance
> > and for a one-shot reading or when the sensor is not active (last useful
> > reading) , sensor.reading
> > That's why I did not want to clear sensorReading.
> 
> caching is nice, but I was thinking in terms of usage which can be confusing
> IMO:
> sensorA.stop() then after x hours the user does sensorA.reading... (forgetting
> to sensorA.start) and gets stale data..

Ok, I am clearing the reading in sensor.stop() now.

[timvolodine, 2016-05-26 15:49:02]

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:69:
updateState(SensorState::Active);
is this correct? startUpdating is async, so it could result in
SensorState::Errored. SensorState::Active should probably be set in
didUpdateData in deriving class.

there are also some implementation considerations depending on the spec:
startUpdating() may trigger a one-shot update if there is data already so one of
the questions could be whether onstatechange(active) should be guaranteed to
come before ondatachange..

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:81: if (m_sensorState ==
SensorState::Activating) {
so this means that we cannot stop the sensor while it's "activating". what does
the spec say about this?

regarding reentrancy it may be easier to 'postTask' the state change dispatch,
e.g. like we do in PlatformEventController using a timer.

[riju_, 2016-05-26 17:32:21]

thanks tim, postTasked now.

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:69:
updateState(SensorState::Active);
On 2016/05/26 15:49:01, timvolodine wrote:
> is this correct? startUpdating is async, so it could result in
> SensorState::Errored. SensorState::Active should probably be set in
> didUpdateData in deriving class.
> 

No, removing the active statechange from here.
> there are also some implementation considerations depending on the spec:
> startUpdating() may trigger a one-shot update if there is data already so one
of
> the questions could be whether onstatechange(active) should be guaranteed to
> come before ondatachange..
enforcing event order. need to check this with spec author, before we do the
onstatechange(active).

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:81: if (m_sensorState ==
SensorState::Activating) {
On 2016/05/26 15:49:01, timvolodine wrote:
> so this means that we cannot stop the sensor while it's "activating". what
does
> the spec say about this?
Wrongly done , fixing it.

> regarding reentrancy it may be easier to 'postTask' the state change dispatch,
> e.g. like we do in PlatformEventController using a timer.

[timvolodine, 2016-05-27 18:40:57]

lgtm % comments

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:69:
updateState(SensorState::Active);
On 2016/05/26 17:32:21, riju_ wrote:
> On 2016/05/26 15:49:01, timvolodine wrote:
> > is this correct? startUpdating is async, so it could result in
> > SensorState::Errored. SensorState::Active should probably be set in
> > didUpdateData in deriving class.
> > 
> 
> No, removing the active statechange from here.
> > there are also some implementation considerations depending on the spec:
> > startUpdating() may trigger a one-shot update if there is data already so
one
> of
> > the questions could be whether onstatechange(active) should be guaranteed to
> > come before ondatachange..
> enforcing event order. need to check this with spec author, before we do the
> onstatechange(active).

ok, could you pls add a TODO regarding this in order to not forget in the
future; something like "verify the correct order of onstatechange(active) and
the first onchange(event)"

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:58: m_hasEventListener =
false;
Don't think this is needed? if sensor state is idle/errored m_hasEventListener
should have already been set accordignly and no need to call stopUpdating.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:91: m_sensorState =
newState;
Would be nice to have a state transitions check and some comments on the valid
transitions if that makes sense, like e.g. is done in AbstractAudioContext.cpp.
Could be done in a follow-up patch once the spec is clear on that. Maybe add a
TODO for this.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.h (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:39: void notifyStateChange();
does it need to be public?

[haraken, 2016-05-28 00:09:23]

In terms of implementation, LGTM.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorErrorEvent.h (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorErrorEvent.h:41:
SensorErrorEvent(const AtomicString& eventType);

Add explicit.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:52:
SensorReadingEvent(const AtomicString& eventType);

Add explicit.

[riju_, 2016-05-30 08:26:15]

Patchset #10 (id:300001) has been deleted

[riju_, 2016-05-30 08:27:55]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-30 08:28:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942663003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942663003/320001

[commit-bot: I haz the power, 2016-05-30 10:08:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-30 10:08:28]

Dry run: This issue passed the CQ dry run.

[riju_, 2016-05-30 10:15:14]

Thanks Tim and haraken.

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:69:
updateState(SensorState::Active);
On 2016/05/27 18:40:57, timvolodine wrote:
> On 2016/05/26 17:32:21, riju_ wrote:
> > On 2016/05/26 15:49:01, timvolodine wrote:
> > > is this correct? startUpdating is async, so it could result in
> > > SensorState::Errored. SensorState::Active should probably be set in
> > > didUpdateData in deriving class.
> > > 
> > 
> > No, removing the active statechange from here.
> > > there are also some implementation considerations depending on the spec:
> > > startUpdating() may trigger a one-shot update if there is data already so
> one
> > of
> > > the questions could be whether onstatechange(active) should be guaranteed
to
> > > come before ondatachange..
> > enforcing event order. need to check this with spec author, before we do the
> > onstatechange(active).
> 
> ok, could you pls add a TODO regarding this in order to not forget in the
> future; something like "verify the correct order of onstatechange(active) and
> the first onchange(event)"

Done.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:58: m_hasEventListener =
false;
On 2016/05/27 18:40:57, timvolodine wrote:
> Don't think this is needed? if sensor state is idle/errored m_hasEventListener
> should have already been set accordignly and no need to call stopUpdating.

Done.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:91: m_sensorState =
newState;
On 2016/05/27 18:40:57, timvolodine wrote:
> Would be nice to have a state transitions check and some comments on the valid
> transitions if that makes sense, like e.g. is done in
AbstractAudioContext.cpp.
> Could be done in a follow-up patch once the spec is clear on that. Maybe add a
> TODO for this.

Done.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.h (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:39: void notifyStateChange();
On 2016/05/27 18:40:57, timvolodine wrote:
> does it need to be public?

Done.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorErrorEvent.h (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorErrorEvent.h:41:
SensorErrorEvent(const AtomicString& eventType);
On 2016/05/28 00:09:23, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h (right):

https://codereview.chromium.org/1942663003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingEvent.h:52:
SensorReadingEvent(const AtomicString& eventType);
On 2016/05/28 00:09:23, haraken wrote:
> 
> Add explicit.

Done.

[riju_, 2016-05-30 10:15:24]

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

[riju_, 2016-05-30 10:15:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, haraken@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/1942663003/#ps320001
(title: "fix final comments")

[commit-bot: I haz the power, 2016-05-30 10:15:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942663003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942663003/320001

[commit-bot: I haz the power, 2016-05-30 10:19:45]

Description was changed from

==========
[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

Intent to Implement : 
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TkfdVqYAYiE

BUG=606766
==========

to

==========
[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

Intent to Implement : 
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TkfdVqYAYiE

BUG=606766
==========

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

Committed patchset #10 (id:320001)

[commit-bot: I haz the power, 2016-05-30 10:21:40]

Description was changed from

==========
[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

Intent to Implement : 
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TkfdVqYAYiE

BUG=606766
==========

to

==========
[sensors]: Introduce the Generic Sensor API.

The Generic Sensor API defines a framework for exposing sensor data to
the Open Web Platform. This CL introduces the IDLs for Sensor* interface
and adds the supporting .h|cpp files.

The ED for the Generic Sensor API is here -
https://w3c.github.io/sensors/

Intent to Implement :
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TkfdVqYAYiE

BUG=606766

Committed: https://crrev.com/ea88054ae4e7cd154177bb23426271fc0d6906ab
Cr-Commit-Position: refs/heads/master@{#396707}
==========

[commit-bot: I haz the power, 2016-05-30 10:21:41]

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

[timvolodine, 2016-05-31 13:59:49]

On 2016/05/30 10:21:41, commit-bot: I haz the power wrote:
> Patchset 10 (id:??) landed as
> https://crrev.com/ea88054ae4e7cd154177bb23426271fc0d6906ab
> Cr-Commit-Position: refs/heads/master@{#396707}

actually forgot to mention that somebody should double-check the oilpan/gc part
of this patch; however looks like haraken@ is the right expert for this so
assuming everything is fine ;)

[haraken, 2016-05-31 14:35:03]

On 2016/05/31 13:59:49, timvolodine wrote:
> On 2016/05/30 10:21:41, commit-bot: I haz the power wrote:
> > Patchset 10 (id:??) landed as
> > https://crrev.com/ea88054ae4e7cd154177bb23426271fc0d6906ab
> > Cr-Commit-Position: refs/heads/master@{#396707}
> 
> actually forgot to mention that somebody should double-check the oilpan/gc
part
> of this patch; however looks like haraken@ is the right expert for this so
> assuming everything is fine ;)

Yes, I think this CL looks good.

[timvolodine, 2016-05-31 14:39:51]

On 2016/05/31 14:35:03, haraken wrote:
> On 2016/05/31 13:59:49, timvolodine wrote:
> > On 2016/05/30 10:21:41, commit-bot: I haz the power wrote:
> > > Patchset 10 (id:??) landed as
> > > https://crrev.com/ea88054ae4e7cd154177bb23426271fc0d6906ab
> > > Cr-Commit-Position: refs/heads/master@{#396707}
> > 
> > actually forgot to mention that somebody should double-check the oilpan/gc
> part
> > of this patch; however looks like haraken@ is the right expert for this so
> > assuming everything is fine ;)
> 
> Yes, I think this CL looks good.

Thanks haraken!

[sof, 2016-06-27 09:30:59]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-06-27 09:31:00]

Just something noticed while looking over other code details.

https://codereview.chromium.org/1942663003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.h (right):

https://codereview.chromium.org/1942663003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.h:29: , public
PlatformEventController {
Where are the implementations of the pure virtual methods that
PlatformEventController declares?