Implement DeviceLight

Implement DeviceLight

Initial implementation, behind a runtime flag.
This CL is meant for the blink side.
The android and Mac side code will be followed up in subsequent CLs.

Content/Chrome side CL with Android implementation -> https://codereview.chromium.org/152893002/

Spec: https://dvcs.w3.org/hg/dap/raw-file/default/light/Overview.html

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

BUG=336424
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173798

[riju_, 2014-03-04 17:30:11]

Hi Adam,

I put this CL as a monolithic chunk just to get and idea of the technical debt
introduced due to this feature and also have an overall idea.
(+561 lines, -94 lines = 467) - Blink side
(+483 lines, -53 lines = 430) - Chrome side ( +Android implementation) 
So total = 897 LOC.

PTAL

[abarth-chromium, 2014-03-04 18:55:12]

I'm not sure everyone on the blink-dev thread was on board.  Do you think we
should have more discussion before landing the code?  I'll take a look and
provide some technical feedback too.

[abarth-chromium, 2014-03-04 18:59:31]

This CL generally looks fine from a technical point of view.  Below are some
very minor comments.  Let's make sure we have the community on board and then we
can proceed.

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
File Source/modules/devicelight/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
Source/modules/devicelight/DeviceLightController.cpp:24: */
Please use the copyright noticed listed in:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
File Source/modules/devicelight/DeviceLightDispatcher.h (right):

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
Source/modules/devicelight/DeviceLightDispatcher.h:31: #include
"modules/device_orientation/DeviceSensorEventDispatcher.h"
Yeah, we'll need to do that before landing this CL to avoid introducing
cross-module dependencies.

https://codereview.chromium.org/143823004/diff/330001/Source/modules/modules....
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/143823004/diff/330001/Source/modules/modules....
Source/modules/modules.gypi:27: 'devicelight/DeviceLightEvent.idl',
The module probably should be named device_light to match device_orientation

https://codereview.chromium.org/143823004/diff/330001/Source/web/WebRuntimeFe...
File Source/web/WebRuntimeFeatures.cpp (right):

https://codereview.chromium.org/143823004/diff/330001/Source/web/WebRuntimeFe...
Source/web/WebRuntimeFeatures.cpp:90: }
Why is this needed?

https://codereview.chromium.org/143823004/diff/330001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/143823004/diff/330001/public/platform/Platfor...
public/platform/Platform.h:621: 
Two blank lines between sections.

https://codereview.chromium.org/143823004/diff/330001/public/platform/Platfor...
public/platform/Platform.h:629: 
You've got an extra blank line here.

[riju_, 2014-03-05 15:47:27]

Thanks for the comments. Yes I agree we need to get a consensus on blink-dev
before landing this CL. We tried to address all the concerns the community had
regarding this API. We will ask the API owners and other members once again.

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
File Source/modules/devicelight/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
Source/modules/devicelight/DeviceLightController.cpp:24: */
On 2014/03/04 18:59:32, abarth wrote:
> Please use the copyright noticed listed in:
> 
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
File Source/modules/devicelight/DeviceLightDispatcher.h (right):

https://codereview.chromium.org/143823004/diff/330001/Source/modules/deviceli...
Source/modules/devicelight/DeviceLightDispatcher.h:31: #include
"modules/device_orientation/DeviceSensorEventDispatcher.h"
On 2014/03/04 18:59:32, abarth wrote:
> Yeah, we'll need to do that before landing this CL to avoid introducing
> cross-module dependencies.

Yes. I will do this in a separate CL to avoid confusion.

https://codereview.chromium.org/143823004/diff/330001/Source/modules/modules....
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/143823004/diff/330001/Source/modules/modules....
Source/modules/modules.gypi:27: 'devicelight/DeviceLightEvent.idl',
On 2014/03/04 18:59:32, abarth wrote:
> The module probably should be named device_light to match device_orientation

Done.

https://codereview.chromium.org/143823004/diff/330001/Source/web/WebRuntimeFe...
File Source/web/WebRuntimeFeatures.cpp (right):

https://codereview.chromium.org/143823004/diff/330001/Source/web/WebRuntimeFe...
Source/web/WebRuntimeFeatures.cpp:90: }
On 2014/03/04 18:59:32, abarth wrote:
> Why is this needed?

Done.

https://codereview.chromium.org/143823004/diff/330001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/143823004/diff/330001/public/platform/Platfor...
public/platform/Platform.h:621: 
On 2014/03/04 18:59:32, abarth wrote:
> Two blank lines between sections.

Done.

https://codereview.chromium.org/143823004/diff/330001/public/platform/Platfor...
public/platform/Platform.h:629: 
On 2014/03/04 18:59:32, abarth wrote:
> You've got an extra blank line here.

Done.

[Inactive, 2014-03-07 19:29:52]

https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
Source/core/frame/Window.idl:181: [RuntimeEnabled=DeviceLight] attribute
EventHandler ondevicelight;
I don't know that the policy is on this but technically, this could be part of a
partial interface in the new module, to avoid editing Window / DOMWindow in
core/. Adam, what do you think?

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:68: double
DeviceLightDispatcher::latestDeviceLightData()
can be const

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.h (right):

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:20: class
DeviceLightDispatcher : public DeviceSensorEventDispatcher, public
blink::WebDeviceLightListener {
Can be marked as FINAL

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:27: virtual void
didChangeDeviceLight(const double&) OVERRIDE;
const double& -> double?

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:33:
~DeviceLightDispatcher();
virtual

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.cpp (right):

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:40: void
DeviceLightEvent::trace(Visitor* visitor)
Why are we overriding this if we don't do anything more than calling
Event::trace()?

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
File public/platform/WebDeviceLightListener.h (right):

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:2: * Copyright (C) 2014 Intel Inc. All
rights reserved.
Should probably use the new copyright header format?

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:31: class WebDeviceLightListener {
This class should probably have a virtual destructor.

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:34: virtual void
didChangeDeviceLight(const double&) = 0;
const double& -> double?

[abarth-chromium, 2014-03-08 06:44:19]

https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
Source/core/frame/Window.idl:181: [RuntimeEnabled=DeviceLight] attribute
EventHandler ondevicelight;
On 2014/03/07 19:29:52, Chris Dumez wrote:
> I don't know that the policy is on this but technically, this could be part of
a
> partial interface in the new module, to avoid editing Window / DOMWindow in
> core/. Adam, what do you think?

Does that work?  If so, we should apply it to ondevicemotion and
ondeviceorientation, etc.  it might require some changes to the code generator.

[Inactive, 2014-03-08 13:51:48]

On 2014/03/08 06:44:19, abarth wrote:
>
https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
> File Source/core/frame/Window.idl (right):
> 
>
https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
> Source/core/frame/Window.idl:181: [RuntimeEnabled=DeviceLight] attribute
> EventHandler ondevicelight;
> On 2014/03/07 19:29:52, Chris Dumez wrote:
> > I don't know that the policy is on this but technically, this could be part
of
> a
> > partial interface in the new module, to avoid editing Window / DOMWindow in
> > core/. Adam, what do you think?
> 
> Does that work?  If so, we should apply it to ondevicemotion and
> ondeviceorientation, etc.  it might require some changes to the code
generator.

I already did this for the screen orientation module which adds an EventHandler
to Screen via a partial interface. It shouldn't be any different for DOMWindow.

[Inactive, 2014-03-08 15:36:32]

On 2014/03/08 06:44:19, abarth wrote:
>
https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
> File Source/core/frame/Window.idl (right):
> 
>
https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
> Source/core/frame/Window.idl:181: [RuntimeEnabled=DeviceLight] attribute
> EventHandler ondevicelight;
> On 2014/03/07 19:29:52, Chris Dumez wrote:
> > I don't know that the policy is on this but technically, this could be part
of
> a
> > partial interface in the new module, to avoid editing Window / DOMWindow in
> > core/. Adam, what do you think?
> 
> Does that work?  If so, we should apply it to ondevicemotion and
> ondeviceorientation, etc.  it might require some changes to the code
generator.

devicemotion / deviceorientation update:
https://codereview.chromium.org/189413008/

[riju_, 2014-03-10 16:17:25]

Thanks Chris for the comments.

I will wait for [1] to go in. Then I will rebase this CL and fix any other
outstanding issues.

[1]https://codereview.chromium.org/191003009

https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/143823004/diff/350001/Source/core/frame/Windo...
Source/core/frame/Window.idl:181: [RuntimeEnabled=DeviceLight] attribute
EventHandler ondevicelight;
On 2014/03/07 19:29:52, Chris Dumez wrote:
> I don't know that the policy is on this but technically, this could be part of
a
> partial interface in the new module, to avoid editing Window / DOMWindow in
> core/. Adam, what do you think?

Done.

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:68: double
DeviceLightDispatcher::latestDeviceLightData()
On 2014/03/07 19:29:52, Chris Dumez wrote:
> can be const
On Mac try bot it showed -> error: 'const' type qualifier on return type has no
effect.

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.h (right):

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:20: class
DeviceLightDispatcher : public DeviceSensorEventDispatcher, public
blink::WebDeviceLightListener {
On 2014/03/07 19:29:52, Chris Dumez wrote:
> Can be marked as FINAL

Done.

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:27: virtual void
didChangeDeviceLight(const double&) OVERRIDE;
On 2014/03/07 19:29:52, Chris Dumez wrote:
> const double& -> double?

Done.

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:33:
~DeviceLightDispatcher();
On 2014/03/07 19:29:52, Chris Dumez wrote:
> virtual

Done.

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.cpp (right):

https://codereview.chromium.org/143823004/diff/350001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:40: void
DeviceLightEvent::trace(Visitor* visitor)
On 2014/03/07 19:29:52, Chris Dumez wrote:
> Why are we overriding this if we don't do anything more than calling
> Event::trace()?

Done.

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
File public/platform/WebDeviceLightListener.h (right):

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:2: * Copyright (C) 2014 Intel Inc. All
rights reserved.
On 2014/03/07 19:29:52, Chris Dumez wrote:
> Should probably use the new copyright header format?

Done.

https://codereview.chromium.org/143823004/diff/350001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:31: class WebDeviceLightListener {
On 2014/03/07 19:29:52, Chris Dumez wrote:
> This class should probably have a virtual destructor.

Done.

[riju_, 2014-03-18 16:48:55]

Hi Adam, PTAL.

I hope it is OK if I add the Layout tests in a subsequent CL. Mainly because
there is a dependency on TestRunner/ SetMockDeviceLight, which will be fixed
when the content part CL goes in.

[abarth-chromium, 2014-03-18 22:48:53]

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:80: startUpdating();
What happens when visibilityState() changes?  Don't we need to start updating
then?

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.h (right):

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.h:14: 
You've got an extra line here.

https://codereview.chromium.org/143823004/diff/440001/Source/web/WebRuntimeFe...
File Source/web/WebRuntimeFeatures.cpp (right):

https://codereview.chromium.org/143823004/diff/440001/Source/web/WebRuntimeFe...
Source/web/WebRuntimeFeatures.cpp:88: }
There's no need for this function.  It should just be controlled by the
experimental web platform features flag.

https://codereview.chromium.org/143823004/diff/440001/public/web/WebRuntimeFe...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/143823004/diff/440001/public/web/WebRuntimeFe...
public/web/WebRuntimeFeatures.h:57: BLINK_EXPORT static bool
isDeviceLightEnabled();
Please don't export these functions.  They shouldn't be needed.

[riju_, 2014-03-19 12:28:13]

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:80: startUpdating();
On 2014/03/18 22:48:53, abarth wrote:
> What happens when visibilityState() changes?  Don't we need to start updating
> then?
My understanding is that, we update only if the page is visible. That is the
case with the other apis.

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.h (right):

https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.h:14: 
On 2014/03/18 22:48:53, abarth wrote:
> You've got an extra line here.

Done.

https://codereview.chromium.org/143823004/diff/440001/Source/web/WebRuntimeFe...
File Source/web/WebRuntimeFeatures.cpp (right):

https://codereview.chromium.org/143823004/diff/440001/Source/web/WebRuntimeFe...
Source/web/WebRuntimeFeatures.cpp:88: }
On 2014/03/18 22:48:53, abarth wrote:
> There's no need for this function.  It should just be controlled by the
> experimental web platform features flag.

Done.

https://codereview.chromium.org/143823004/diff/440001/public/web/WebRuntimeFe...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/143823004/diff/440001/public/web/WebRuntimeFe...
public/web/WebRuntimeFeatures.h:57: BLINK_EXPORT static bool
isDeviceLightEnabled();
On 2014/03/18 22:48:53, abarth wrote:
> Please don't export these functions.  They shouldn't be needed.

Done.

[abarth-chromium, 2014-03-19 17:11:11]

On 2014/03/19 12:28:13, riju_ wrote:
>
https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
> Source/modules/device_light/DeviceLightController.cpp:80: startUpdating();
> On 2014/03/18 22:48:53, abarth wrote:
> > What happens when visibilityState() changes?  Don't we need to start
updating
> > then?
> My understanding is that, we update only if the page is visible. That is the
> case with the other apis.

My point is that when the page becomes visible, we'll need to call
startUpdating.  Can you show me where that happens?

[riju_, 2014-03-20 11:31:39]

On 2014/03/19 17:11:11, abarth wrote:
> On 2014/03/19 12:28:13, riju_ wrote:
> >
>
https://codereview.chromium.org/143823004/diff/440001/Source/modules/device_l...
> > Source/modules/device_light/DeviceLightController.cpp:80: startUpdating();
> > On 2014/03/18 22:48:53, abarth wrote:
> > > What happens when visibilityState() changes?  Don't we need to start
> updating
> > > then?
> > My understanding is that, we update only if the page is visible. That is the
> > case with the other apis.
> 
> My point is that when the page becomes visible, we'll need to call
> startUpdating.  Can you show me where that happens?

DeviceSensorEventController::pageVisibilityChanged()

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

I took the common part out of this CL and put it in
https://codereview.chromium.org/191003009/

[sof, 2014-03-20 16:27:14]

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.h (right):

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:27: static
PassRefPtrWillBeRawPtr<DeviceLightEvent> create()
Make this PassRefPtr<> only for now; Event hasn't been moved to Oilpan yet.

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:29: return
adoptRefWillBeRefCountedGarbageCollected(new DeviceLightEvent);
Similarly, just adoptRef()

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:31: static
PassRefPtrWillBeRawPtr<DeviceLightEvent> create(const AtomicString& eventType,
const double value)
PassRefPtr<>

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:33: return
adoptRefWillBeRefCountedGarbageCollected(new DeviceLightEvent(eventType,
value));
adoptRef()

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:35: static
PassRefPtrWillBeRawPtr<DeviceLightEvent> create(const AtomicString& eventType,
const DeviceLightEventInit& initializer)
PassRefPtr<>

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:37: return
adoptRefWillBeRefCountedGarbageCollected(new DeviceLightEvent(eventType,
initializer));
adoptRef()

[riju_, 2014-03-20 20:12:28]

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.h (right):

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:27: static
PassRefPtrWillBeRawPtr<DeviceLightEvent> create()
On 2014/03/20 16:27:15, sof wrote:
> Make this PassRefPtr<> only for now; Event hasn't been moved to Oilpan yet.

Done.

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:29: return
adoptRefWillBeRefCountedGarbageCollected(new DeviceLightEvent);
On 2014/03/20 16:27:15, sof wrote:
> Similarly, just adoptRef()

Done.

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:31: static
PassRefPtrWillBeRawPtr<DeviceLightEvent> create(const AtomicString& eventType,
const double value)
On 2014/03/20 16:27:15, sof wrote:
> PassRefPtr<>

Done.

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:33: return
adoptRefWillBeRefCountedGarbageCollected(new DeviceLightEvent(eventType,
value));
On 2014/03/20 16:27:15, sof wrote:
> adoptRef()

Done.

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:35: static
PassRefPtrWillBeRawPtr<DeviceLightEvent> create(const AtomicString& eventType,
const DeviceLightEventInit& initializer)
On 2014/03/20 16:27:15, sof wrote:
> PassRefPtr<>

Done.

https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.h:37: return
adoptRefWillBeRefCountedGarbageCollected(new DeviceLightEvent(eventType,
initializer));
On 2014/03/20 16:27:15, sof wrote:
> adoptRef()

Done.

[sof, 2014-03-20 20:24:21]

On 2014/03/20 20:12:28, riju_ wrote:
>
https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
> File Source/modules/device_light/DeviceLightEvent.h (right):
> 
>
https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
> Source/modules/device_light/DeviceLightEvent.h:27: static
> PassRefPtrWillBeRawPtr<DeviceLightEvent> create()
> On 2014/03/20 16:27:15, sof wrote:
> > Make this PassRefPtr<> only for now; Event hasn't been moved to Oilpan yet.
> 
> Done.
> 

Thanks; should you want to build with Oilpan enabled, add enable_oilpan=1 to
your gyp defines.

[riju_, 2014-03-20 21:20:13]

On 2014/03/20 20:24:21, sof wrote:
> On 2014/03/20 20:12:28, riju_ wrote:
> >
>
https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
> > File Source/modules/device_light/DeviceLightEvent.h (right):
> > 
> >
>
https://codereview.chromium.org/143823004/diff/460001/Source/modules/device_l...
> > Source/modules/device_light/DeviceLightEvent.h:27: static
> > PassRefPtrWillBeRawPtr<DeviceLightEvent> create()
> > On 2014/03/20 16:27:15, sof wrote:
> > > Make this PassRefPtr<> only for now; Event hasn't been moved to Oilpan
yet.
> > 
> > Done.
> > 
> 
> Thanks; should you want to build with Oilpan enabled, add enable_oilpan=1 to
> your gyp defines.

thanks for the review comments

[riju_, 2014-03-21 10:17:39]

Hi Adam, PTAL and let me know if there are any more blockers for this CL.

[abarth-chromium, 2014-03-21 13:04:12]

I'm sorry, but I'm tired of reviewing this CL.  It's getting better, but at this
rate it's going to take a long time to finish.

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/script-tests/TEMPLATE.html (right):

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/script-tests/TEMPLATE.html:8: </html>
This file is not needed.  You can just delete it.

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/script-tests/create-event.js (right):

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/script-tests/create-event.js:15:
shouldBeTrue("typeof event.value == 'number'");
This code should be inlined into the test that uses it.  Having a separate
script-tests directory doesn't serve any purpose anymore.

https://codereview.chromium.org/143823004/diff/500001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/500001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:80: startUpdating();
You still haven't fixed this bug.  If someone calls addEventListener while the
page isn't visible and then the page becomes visible, we won't startUpdating and
the event will never fire.

[riju_, 2014-04-03 17:52:53]

Hi Adam,
Sorry for the delay. I was doing the content side part. A put up a demo on Nexus
5 -> http://youtu.be/rkj1ifmFFQ4

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/script-tests/TEMPLATE.html (right):

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/script-tests/TEMPLATE.html:8: </html>
On 2014/03/21 13:04:13, abarth wrote:
> This file is not needed.  You can just delete it.

Done.

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/script-tests/create-event.js (right):

https://codereview.chromium.org/143823004/diff/500001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/script-tests/create-event.js:15:
shouldBeTrue("typeof event.value == 'number'");
On 2014/03/21 13:04:13, abarth wrote:
> This code should be inlined into the test that uses it.  Having a separate
> script-tests directory doesn't serve any purpose anymore.

Done.

https://codereview.chromium.org/143823004/diff/500001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/500001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:80: startUpdating();
On 2014/03/21 13:04:13, abarth wrote:
> You still haven't fixed this bug.  If someone calls addEventListener while the
> page isn't visible and then the page becomes visible, we won't startUpdating
and
> the event will never fire.

DeviceLightController is inheriting from DeviceSensorEventController, when the
page becomes visible, DeviceSensorEventController::pageVisibilityChanged() is
called which calls the startUpdating().

To illustrate this i tested using this snippet ->

<!DOCTYPE html>
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta charset="utf-8">
<title>Test Page</title>
</head>
<body>

<p>Test Page</p>
<script>
var callback = function () {
  if (document.hidden === false) {
    console.log("=====It's Not Hidden\n");
} else {
    console.log("======It's Hidden\n");
    window.addEventListener("devicelight", function(event) {
      document.querySelector('#content span').innerHTML = event.value; 
    });
}
	
};

document.addEventListener('visibilitychange', callback);
</script>
<div id="content">Lux value = <span></span></div>
</body>
</html>
==================================================================

Tested on Nexus 5, produces the output as expected, printing the lux values
whenever the page is visible.

[timvolodine, 2014-04-08 12:59:55]

a few nits..

https://codereview.chromium.org/143823004/diff/560001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/560001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:49: return
DeviceLightDispatcher::instance().latestDeviceLightData();
can lux sensor value be 0? in that case this would still return false.

https://codereview.chromium.org/143823004/diff/560001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:71: return
!lightEvent->value();
should this be value==+Inf? (as according to spec)

[riju_, 2014-04-10 12:22:10]

Thanks Tim for the comments.

https://codereview.chromium.org/143823004/diff/560001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/560001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:49: return
DeviceLightDispatcher::instance().latestDeviceLightData();
On 2014/04/08 12:59:55, timvolodine wrote:
> can lux sensor value be 0? in that case this would still return false.

Yes. Lux value = 0 is valid. changing this to True only if >=0

https://codereview.chromium.org/143823004/diff/560001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:71: return
!lightEvent->value();
On 2014/04/08 12:59:55, timvolodine wrote:
> should this be value==+Inf? (as according to spec)

Done.

[timvolodine, 2014-04-10 12:43:13]

https://codereview.chromium.org/143823004/diff/600001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.h (right):

https://codereview.chromium.org/143823004/diff/600001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:36: double
m_lastDeviceLightData;
is this initialized somewhere? preferably such that
DeviceLightController::hasLastData() returns false initially..

[riju_, 2014-04-10 14:10:40]

https://codereview.chromium.org/143823004/diff/600001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.h (right):

https://codereview.chromium.org/143823004/diff/600001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.h:36: double
m_lastDeviceLightData;
On 2014/04/10 12:43:13, timvolodine wrote:
> is this initialized somewhere? preferably such that
> DeviceLightController::hasLastData() returns false initially..

Done.
Initializing m_lastDeviceLightData = -1 in the c'stor because 0 is a valid Lux
value.

[riju_, 2014-04-14 14:50:10]

On 2014/04/10 14:10:40, riju_ wrote:
>
https://codereview.chromium.org/143823004/diff/600001/Source/modules/device_l...
> File Source/modules/device_light/DeviceLightDispatcher.h (right):
> 
>
https://codereview.chromium.org/143823004/diff/600001/Source/modules/device_l...
> Source/modules/device_light/DeviceLightDispatcher.h:36: double
> m_lastDeviceLightData;
> On 2014/04/10 12:43:13, timvolodine wrote:
> > is this initialized somewhere? preferably such that
> > DeviceLightController::hasLastData() returns false initially..
> 
> Done.
> Initializing m_lastDeviceLightData = -1 in the c'stor because 0 is a valid Lux
> value.

Hi Tim, Chris 
Thanks for your comments.
Please let me know if anything else needs to be done for this CL.

-Riju.

[Inactive, 2014-04-15 15:56:11]

https://codereview.chromium.org/143823004/diff/620001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/window-property-expected.txt (right):

https://codereview.chromium.org/143823004/diff/620001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/window-property-expected.txt:6: FAIL typeof
window.DeviceLightEvent == 'object' should be true. Was false.
Why all the FAIL lines?

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:49: return
(DeviceLightDispatcher::instance().latestDeviceLightData() >= 0 ) ? true :
false;
return DeviceLightDispatcher::instance().latestDeviceLightData() >= 0;

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:71: return
(lightEvent->value() == std::numeric_limits<double>::infinity()) ? true : false;
return lightEvent->value() == std::numeric_limits<double>::infinity();

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:86: stopUpdating();
This is something that the DeviceOrientation code does but that I never
understood. What if we have several listeners from the 'devicelight' event? The
current code will stop listening as soon as 1 of the listeners is unregistered.
This does not seem right to me but it may simply be me misunderstanding.

I thought we should have a counter that we increment / decrement in didAdd /
didRemove and only stop listening when it becomes zero.

This is more a question for Tim or Adam I guess. Do not change this just yet as
this is consistent with device orientation / motion and I may be
misunderstanding.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:23: m_lastDeviceLightData
= -1;
Please use initializer list.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.idl (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.idl:9: interface DeviceLightEvent :
Event {
We usually put interface on the same line as the closing ']'

[timvolodine, 2014-04-17 14:35:52]

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:86: stopUpdating();
On 2014/04/15 15:56:12, Chris Dumez wrote:
> This is something that the DeviceOrientation code does but that I never
> understood. What if we have several listeners from the 'devicelight' event?
The
> current code will stop listening as soon as 1 of the listeners is
unregistered.
> This does not seem right to me but it may simply be me misunderstanding.
> 
> I thought we should have a counter that we increment / decrement in didAdd /
> didRemove and only stop listening when it becomes zero.
> 
> This is more a question for Tim or Adam I guess. Do not change this just yet
as
> this is consistent with device orientation / motion and I may be
> misunderstanding.

Chris you are right, this is an issue indeed. It appears we can avoid keeping a
counter by simply testing
"window->hasEventListeners(EventTypeNames::devicemotion)" when adding/removing
listeners. I've uploaded https://codereview.chromium.org/240993003/ let me know
what you think.

[riju_, 2014-04-22 13:11:28]

Chris , Tim : thanks for all the help and comments.

I will add the more Layout tests (e.g. multiple-event-listeners) later when i
get the content part in.

-Riju.

https://codereview.chromium.org/143823004/diff/620001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/window-property-expected.txt (right):

https://codereview.chromium.org/143823004/diff/620001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/window-property-expected.txt:6: FAIL typeof
window.DeviceLightEvent == 'object' should be true. Was false.
On 2014/04/15 15:56:12, Chris Dumez wrote:
> Why all the FAIL lines?

Done.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:49: return
(DeviceLightDispatcher::instance().latestDeviceLightData() >= 0 ) ? true :
false;
On 2014/04/15 15:56:12, Chris Dumez wrote:
> return DeviceLightDispatcher::instance().latestDeviceLightData() >= 0;

Done.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:71: return
(lightEvent->value() == std::numeric_limits<double>::infinity()) ? true : false;
On 2014/04/15 15:56:12, Chris Dumez wrote:
> return lightEvent->value() == std::numeric_limits<double>::infinity();

Done.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:86: stopUpdating();
On 2014/04/15 15:56:12, Chris Dumez wrote:
> This is something that the DeviceOrientation code does but that I never
> understood. What if we have several listeners from the 'devicelight' event?
The
> current code will stop listening as soon as 1 of the listeners is
unregistered.
> This does not seem right to me but it may simply be me misunderstanding.
> 
> I thought we should have a counter that we increment / decrement in didAdd /
> didRemove and only stop listening when it becomes zero.
> 
> This is more a question for Tim or Adam I guess. Do not change this just yet
as
> this is consistent with device orientation / motion and I may be
> misunderstanding.

Done.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:23: m_lastDeviceLightData
= -1;
On 2014/04/15 15:56:12, Chris Dumez wrote:
> Please use initializer list.

Done.

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.idl (right):

https://codereview.chromium.org/143823004/diff/620001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.idl:9: interface DeviceLightEvent :
Event {
On 2014/04/15 15:56:12, Chris Dumez wrote:
> We usually put interface on the same line as the closing ']'

Done.

[riju_, 2014-04-28 12:39:01]

Hi All, PTAL.

[timvolodine, 2014-04-29 14:48:40]

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:86: if (eventType !=
EventTypeNames::devicelight ||
window->hasEventListeners(EventTypeNames::devicelight)) {
the logic here is wrong. please look at e.g. DeviceMotionController
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
where this is done correctly.

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:47:
blink::Platform::current()->setDeviceLightListener(0);
you probably want to reset m_lastDeviceLightData here, otherwise it could fire a
stale value on second start.

https://codereview.chromium.org/143823004/diff/770001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/143823004/diff/770001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:50: DeviceLight status=experimental
I think this should be "test" for now. Once the LayoutTests using TestRunner are
submitted the status can be upgraded.

[riju_, 2014-04-29 15:59:41]

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightController.cpp (right):

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
Source/modules/device_light/DeviceLightController.cpp:86: if (eventType !=
EventTypeNames::devicelight ||
window->hasEventListeners(EventTypeNames::devicelight)) {
On 2014/04/29 14:48:41, timvolodine wrote:
> the logic here is wrong. please look at e.g. DeviceMotionController
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> where this is done correctly.

Done.

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/770001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:47:
blink::Platform::current()->setDeviceLightListener(0);
On 2014/04/29 14:48:41, timvolodine wrote:
> you probably want to reset m_lastDeviceLightData here, otherwise it could fire
a
> stale value on second start.

Done.

[riju_, 2014-05-06 11:37:47]

On 2014/04/30 10:43:08, riju_ wrote:

Friendly ping : Tim, PTAL

[timvolodine, 2014-05-06 12:27:48]

lgtm

thanks!
you'll need owners approval to land this.
we'll also need some more Layout tests for this module (probably based on
testrunner in a separate patch).

[Inactive, 2014-05-06 12:58:07]

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/create-event.html (right):

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:1: <html>
Missing DOCTYPE

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:9: var event =
document.createEvent('DeviceLightEvent');
It would be good to test the modern way of creating events too (DeviceLightEvent
constructor).

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:10: 
shouldBe("event.__proto__", "DeviceLightEvent.prototype");

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:18: shouldBeTrue("typeof
event.type == 'string'");
You should also check that type is "devicelight".

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:19: shouldBeTrue("typeof
event.bubbles == 'boolean'");
You should also check that bubbles is true.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:20: shouldBeTrue("typeof
event.cancelable == 'boolean'");
You should also check that cancelable is false.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:21: shouldBeTrue("typeof
event.value == 'number'");
You should check the value as well.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.cpp (right):

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:6: 
No need for this blank line.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:21:
DeviceLightEvent::DeviceLightEvent(const AtomicString& eventType, const double
value)
nit: Why the const argument?

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:22: : Event(eventType, true,
false) // Default event is bubbles, not cancellable.
// The DeviceLightEvent bubbles but is not cancelable.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.idl (right):

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.idl:9:
[InitializedByEventConstructor] readonly attribute double value;
According to the spec, this should be an "unrestricted double".

https://codereview.chromium.org/143823004/diff/800001/public/platform/WebDevi...
File public/platform/WebDeviceLightListener.h (right):

https://codereview.chromium.org/143823004/diff/800001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:13: virtual void
didChangeDeviceLight(const double) = 0;
nit: why the const argument?

[riju_, 2014-05-09 12:00:20]

Thanks Tim ! Yes all the other Layout tests will be added in the next CL.

Chris, thanks for the comments. Hopefully the CL looks better now.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/create-event.html (right):

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:1: <html>
On 2014/05/06 12:58:08, Chris Dumez wrote:
> Missing DOCTYPE

Done.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:9: var event =
document.createEvent('DeviceLightEvent');
On 2014/05/06 12:58:08, Chris Dumez wrote:
> It would be good to test the modern way of creating events too
(DeviceLightEvent
> constructor).

Done.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:10: 
On 2014/05/06 12:58:08, Chris Dumez wrote:
> shouldBe("event.__proto__", "DeviceLightEvent.prototype");

Done.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:18: shouldBeTrue("typeof
event.type == 'string'");
On 2014/05/06 12:58:08, Chris Dumez wrote:
> You should also check that type is "devicelight".

Done.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:19: shouldBeTrue("typeof
event.bubbles == 'boolean'");
On 2014/05/06 12:58:08, Chris Dumez wrote:
> You should also check that bubbles is true.

Done.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:20: shouldBeTrue("typeof
event.cancelable == 'boolean'");
On 2014/05/06 12:58:08, Chris Dumez wrote:
> You should also check that cancelable is false.

Done.

https://codereview.chromium.org/143823004/diff/800001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:21: shouldBeTrue("typeof
event.value == 'number'");
On 2014/05/06 12:58:08, Chris Dumez wrote:
> You should check the value as well.

Done.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.cpp (right):

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:6: 
On 2014/05/06 12:58:08, Chris Dumez wrote:
> No need for this blank line.

Done.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:21:
DeviceLightEvent::DeviceLightEvent(const AtomicString& eventType, const double
value)
On 2014/05/06 12:58:08, Chris Dumez wrote:
> nit: Why the const argument?

Done.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.cpp:22: : Event(eventType, true,
false) // Default event is bubbles, not cancellable.
On 2014/05/06 12:58:08, Chris Dumez wrote:
> // The DeviceLightEvent bubbles but is not cancelable.

Done.

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightEvent.idl (right):

https://codereview.chromium.org/143823004/diff/800001/Source/modules/device_l...
Source/modules/device_light/DeviceLightEvent.idl:9:
[InitializedByEventConstructor] readonly attribute double value;
On 2014/05/06 12:58:08, Chris Dumez wrote:
> According to the spec, this should be an "unrestricted double".

Done.

https://codereview.chromium.org/143823004/diff/800001/public/platform/WebDevi...
File public/platform/WebDeviceLightListener.h (right):

https://codereview.chromium.org/143823004/diff/800001/public/platform/WebDevi...
public/platform/WebDeviceLightListener.h:13: virtual void
didChangeDeviceLight(const double) = 0;
On 2014/05/06 12:58:08, Chris Dumez wrote:
> nit: why the const argument?

Done.

[Inactive, 2014-05-09 13:12:46]

lgtm (with nits) but I am not an OWNER for most of these folders.

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/create-event.html (right):

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:4: <script
src="../../../resources/js-test.js"></script>
nit: this can be moved to the body and you can remove the <head> section.

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/window-property.html (right):

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/window-property.html:4: <script
src="../../../resources/js-test.js"></script>
nit: can be moved to the body and the head section can be removed.

https://codereview.chromium.org/143823004/diff/860001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/860001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:21:
DeviceLightDispatcher::DeviceLightDispatcher() : m_lastDeviceLightData(-1)
nit: Per coding style, I think ": m_lastDeviceLightData(-1)" should be on the
next line.

[riju_, 2014-05-09 13:45:34]

Thanks Chris for all the review comments.

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/create-event.html (right):

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/create-event.html:4: <script
src="../../../resources/js-test.js"></script>
On 2014/05/09 13:12:47, Chris Dumez wrote:
> nit: this can be moved to the body and you can remove the <head> section.

Done.

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
File LayoutTests/fast/dom/DeviceLight/window-property.html (right):

https://codereview.chromium.org/143823004/diff/860001/LayoutTests/fast/dom/De...
LayoutTests/fast/dom/DeviceLight/window-property.html:4: <script
src="../../../resources/js-test.js"></script>
On 2014/05/09 13:12:47, Chris Dumez wrote:
> nit: can be moved to the body and the head section can be removed.

Done.

https://codereview.chromium.org/143823004/diff/860001/Source/modules/device_l...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/143823004/diff/860001/Source/modules/device_l...
Source/modules/device_light/DeviceLightDispatcher.cpp:21:
DeviceLightDispatcher::DeviceLightDispatcher() : m_lastDeviceLightData(-1)
On 2014/05/09 13:12:47, Chris Dumez wrote:
> nit: Per coding style, I think ": m_lastDeviceLightData(-1)" should be on the
> next line.

Done.

Tools/Scripts/check-webkit-style (or cpplint.py) did not complain, but checked
the Blink coding doc and found you are right.

[riju_, 2014-05-09 16:30:24]

hi Adam, i think the CL is in a better shape now.  Please have a look.

[abarth-chromium, 2014-05-10 00:23:08]

lgtm

Looks perfect!  Thanks so much, and sorry for being grump earlier in the review.

[commit-bot: I haz the power, 2014-05-10 00:24:26]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rijubrata.bhaumik@intel.com/143823004/...

[commit-bot: I haz the power, 2014-05-10 00:42:47]

Change committed as 173798

[commit-bot: I haz the power, 2014-05-10 00:43:52]

Failed to apply patch for Source/modules/device_light/DOMWindowDeviceLight.h:
File exist but was about to be overwriten
Patch:   NR 
Source/modules/device_orientation/DOMWindowDeviceMotion.h->Source/modules/device_light/DOMWindowDeviceLight.h
Index: Source/modules/device_light/DOMWindowDeviceLight.h
diff --git a/Source/modules/device_orientation/DOMWindowDeviceMotion.h
b/Source/modules/device_light/DOMWindowDeviceLight.h
similarity index 57%
copy from Source/modules/device_orientation/DOMWindowDeviceMotion.h
copy to Source/modules/device_light/DOMWindowDeviceLight.h
index
e9b96f5bb9c11643437f5f333814a0658b0d8172..bee6f06d0de6ece9bfcec3263322613942556995
100644
--- a/Source/modules/device_orientation/DOMWindowDeviceMotion.h
+++ b/Source/modules/device_light/DOMWindowDeviceLight.h
@@ -2,18 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef DOMWindowDeviceMotion_h
-#define DOMWindowDeviceMotion_h
+#ifndef DOMWindowDeviceLight_h
+#define DOMWindowDeviceLight_h
 
 #include "core/events/EventTarget.h"
 
 namespace WebCore {
-
-class DOMWindowDeviceMotion {
+class DOMWindowDeviceLight {
 public:
-    DEFINE_STATIC_ATTRIBUTE_EVENT_LISTENER(devicemotion);
+    DEFINE_STATIC_ATTRIBUTE_EVENT_LISTENER(devicelight);
 };
-
 } // namespace WebCore
-
-#endif // DOMWindowDeviceMotion_h
+#endif // DOMWindowDeviceLight_h

[riju_, 2014-05-12 10:49:35]

On 2014/05/10 00:23:08, abarth wrote:
> lgtm
> 
> Looks perfect!  Thanks so much, and sorry for being grump earlier in the
review.

No worries :) Thanks all !!