[DeviceLight] Add renderer+common parts

[DeviceLight] Add renderer+common parts

Adding the Renderer and Common part in the content side for the DeviceLight API.
Add Unit tests for deviceLight pump
Add DeviceLight support in renderer_webkitplatformsupport_impl.h|cc

The default polling time interval of DeviceLightEventPump 
is right now set as of 200 milliseconds. 
(device motion/orientation = 50ms)

In Firefox (Android) implementation, DeviceLight's frequency of
update is set as SensorManager.SENSOR_DELAY_NORMAL which in Android
parlance translates to 200 milliseconds delay which is 5 times a second.
Ofcourse the delay is only a suggested delay, system typically uses a
smaller delay.

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

[riju_, 2014-05-14 15:16:12]

Hi Tim,

I have split the monolithic CL [1].
This is the renderer+common part.

Last time we discussed, I did not have an exact value in mind regarding the
frequency of update for device light. I checked the Firefox implementation [2]
and updated the CL's desciption.

Do you think we can just change the DeviceLight's event pump to ~200
milliseconds and keep rest of the infrastructure same ?

[1] https://codereview.chromium.org/152893002/
[2]
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShe...

[timvolodine, 2014-05-21 18:24:12]

yes I think we could use 200ms delay for the pump, maybe even less it depends on
the applications. Anyway doesn't look like it needs 20Hz update frequency like
motion/orientation.

https://codereview.chromium.org/286793002/diff/20001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/286793002/diff/20001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump.cc:43: if
(reader_->GetLatestData(&data))
if you are using the same infrastructure as motion/orientation in
content/browser then I think you'll need a bit in the shared memory to signify
that the buffer has been updated (similarly to the allAvailableSensorsAreActive
for motion/orientation). Otherwise you risk firing un-updated shared memory into
blink.

[riju_, 2014-06-02 15:33:15]

Hi Tim : Please have a look.

[timvolodine, 2014-06-03 16:08:11]

https://codereview.chromium.org/286793002/diff/40001/content/common/device_se...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/40001/content/common/device_se...
content/common/device_sensors/device_light_data.h:13: memset(this, 0,
sizeof(*this));
I would not inline and create a corresponding .cpp file and put the constructor
there, as we did with other similar classes.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump.cc:7: #include
"content/common/device_sensors/device_light_data.h"
not needed if already included in device_light_event_pump.h

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump.h (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump.h:9: #include
"content/common/device_sensors/device_light_data.h"
is this include needed? is it possible to forward declare instead?

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:103:
TEST_F(DeviceLightEventPumpTest, DidStartPolling) {
could you include a test when valusIsUpdated==false pls?

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:116: }
indent -2sp

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:150:
base::LazyInstance<double>::Leaky
I wonder if LazyInstance is really needed here because it's only wrapping a
double?

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:178: // when
setDeviceMotionListener is invoked, return a double value
nit: start with capital letter, end with "."

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:179: static void
SetMockDeviceLightDataForTesting(
fits on one line

[riju_, 2014-06-05 19:50:01]

hi Tim,
forward declaration of DeviceLightData class in  device_light_event_pump.h did
not work for Windows build, but was fine for Linux/Mac. Hence the #include.

https://codereview.chromium.org/286793002/diff/40001/content/common/device_se...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/40001/content/common/device_se...
content/common/device_sensors/device_light_data.h:13: memset(this, 0,
sizeof(*this));
On 2014/06/03 16:08:12, timvolodine wrote:
> I would not inline and create a corresponding .cpp file and put the
constructor
> there, as we did with other similar classes.

Done.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump.cc (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump.cc:7: #include
"content/common/device_sensors/device_light_data.h"
On 2014/06/03 16:08:12, timvolodine wrote:
> not needed if already included in device_light_event_pump.h

Done. Now, I am forward declaring in .h and the include here.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump.h (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump.h:9: #include
"content/common/device_sensors/device_light_data.h"
On 2014/06/03 16:08:12, timvolodine wrote:
> is this include needed? is it possible to forward declare instead?

Forward declaring worked on Linux/Mac. Try Bots complained for Windows build
"error C2027: use of undefined type".

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:103:
TEST_F(DeviceLightEventPumpTest, DidStartPolling) {
On 2014/06/03 16:08:12, timvolodine wrote:
> could you include a test when valusIsUpdated==false pls?

Done.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/device_...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:116: }
On 2014/06/03 16:08:12, timvolodine wrote:
> indent -2sp

Done.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:150:
base::LazyInstance<double>::Leaky
On 2014/06/03 16:08:12, timvolodine wrote:
> I wonder if LazyInstance is really needed here because it's only wrapping a
> double?

Done.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:178: // when
setDeviceMotionListener is invoked, return a double value
On 2014/06/03 16:08:12, timvolodine wrote:
> nit: start with capital letter, end with "."

Done.

https://codereview.chromium.org/286793002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:179: static void
SetMockDeviceLightDataForTesting(
On 2014/06/03 16:08:12, timvolodine wrote:
> fits on one line

Done.

[timvolodine, 2014-06-11 15:30:07]

just a few more comments, looks good for the rest..

https://codereview.chromium.org/286793002/diff/120001/content/common/device_s...
File content/common/device_sensors/device_light_hardware_buffer.h (right):

https://codereview.chromium.org/286793002/diff/120001/content/common/device_s...
content/common/device_sensors/device_light_hardware_buffer.h:13: typedef
SharedMemorySeqLockBuffer<DeviceLightData>
think this fits on one line?

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:7: #include
"base/logging.h"
probably not needed

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:8: #include
"base/memory/scoped_ptr.h"
probably not needed

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:25:
memcpy(&data_, &data, sizeof(data));
you are copying from double into DeviceLightData? better is "data_.value =
data", and rename argument data to value.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:151: double
g_test_device_light_data;
does this compile? should it be static? you probably also want to set it to some
default value.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1064: if
(g_test_device_light_data == 0) {
so you won't be able to test zero-lux value...

[riju_, 2014-06-12 10:34:22]

hi Tim, thanks for the comments. PTAL.

https://codereview.chromium.org/286793002/diff/120001/content/common/device_s...
File content/common/device_sensors/device_light_hardware_buffer.h (right):

https://codereview.chromium.org/286793002/diff/120001/content/common/device_s...
content/common/device_sensors/device_light_hardware_buffer.h:13: typedef
SharedMemorySeqLockBuffer<DeviceLightData>
On 2014/06/11 15:30:06, timvolodine wrote:
> think this fits on one line?

Done.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:7: #include
"base/logging.h"
On 2014/06/11 15:30:06, timvolodine wrote:
> probably not needed

Done.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:8: #include
"base/memory/scoped_ptr.h"
On 2014/06/11 15:30:06, timvolodine wrote:
> probably not needed

Done.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:25:
memcpy(&data_, &data, sizeof(data));
On 2014/06/11 15:30:06, timvolodine wrote:
> you are copying from double into DeviceLightData? better is "data_.value =
> data", and rename argument data to value.

Done.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:151: double
g_test_device_light_data;
On 2014/06/11 15:30:06, timvolodine wrote:
> does this compile? should it be static? you probably also want to set it to
some
> default value.

It is inside an anonymous namespace, so why use "static"?
g_test_device_light_data is anyways used only in this file, as are
g_sandbox_enabled and web_gamepad_listener. I did not remove the "static"
keyword from these 2 as they are not related to DeviceLight, but now i guess its
ok to remove this nit in this CL only.

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1064: if
(g_test_device_light_data == 0) {
On 2014/06/11 15:30:06, timvolodine wrote:
> so you won't be able to test zero-lux value...

Done.

[riju_, 2014-06-12 12:48:46]

+dcheng: ipc + content/common/*_message*

+jochen: content owner

[jochen (gone - plz use gerrit), 2014-06-12 14:33:51]

how will you write layout tests for this?

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_data.cc (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_data.cc:13: memset(this, 0,
sizeof(*this));
don't do this. use proper initialization.

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_data.h:17: double value;
this class doesn't follow the coding style at all.

data members should be private. don't unnecessarily create bitfields. add
disallow copy/assign. add getters/setters

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_hardware_buffer.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_hardware_buffer.h:13: typedef
SharedMemorySeqLockBuffer<DeviceLightData>DeviceLightHardwareBuffer;
missing space after >

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

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:25: static const int
kDefaultLightPumpDelayMillis;
any reason this is part of the public interface?

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:39: virtual void
FireEvent() OVERRIDE;
please add a comment which interface is implemented here.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:46: };
disallow copy/assign

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:18:
memset(&data_, 0, sizeof(data_));
why is this required?

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:20: virtual
~MockDeviceLightListener() { }
iirc chromium style has no space between { }. did you clang-format your cl?

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:76:
memset(buffer_, 0, sizeof(DeviceLightHardwareBuffer));
no memset plz

https://codereview.chromium.org/286793002/diff/140001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.h:142:
blink::WebDeviceLightListener* listener) OVERRIDE;
don't add OVERRIDE to blink api implementations

[timvolodine, 2014-06-12 15:17:28]

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:151: double
g_test_device_light_data;
On 2014/06/12 10:34:22, riju_ wrote:
> On 2014/06/11 15:30:06, timvolodine wrote:
> > does this compile? should it be static? you probably also want to set it to
> some
> > default value.
> 
> It is inside an anonymous namespace, so why use "static"?
> g_test_device_light_data is anyways used only in this file, as are
> g_sandbox_enabled and web_gamepad_listener. I did not remove the "static"
> keyword from these 2 as they are not related to DeviceLight, but now i guess
its
> ok to remove this nit in this CL only.

looks like there is a difference between compilers on this matter. some
compilers seem to apply external linkage if there is no "static" keyword, so
it's probably safer to leave "static" as it is? maybe jochen can comment on this
as well.

[timvolodine, 2014-06-12 15:25:28]

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_data.h:17: double value;
On 2014/06/12 14:33:50, jochen traveling until Jun 23 wrote:
> this class doesn't follow the coding style at all.
> 
> data members should be private. don't unnecessarily create bitfields. add
> disallow copy/assign. add getters/setters

re getters/setters: this class is intentionally POD, because it is used for
shared memory.
Similar classes can be found in third_party/WebKit/public/platform/ e.g.
WebGamepads.h.

probably should add a comment:
// This class is intentionally POD and fixed size so that it can be stored
// in shared memory between the browser and the renderer processes.

[jochen (gone - plz use gerrit), 2014-06-13 16:07:11]

On 2014/06/12 15:17:28, timvolodine wrote:
>
https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
> File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> 
>
https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
> content/renderer/renderer_webkitplatformsupport_impl.cc:151: double
> g_test_device_light_data;
> On 2014/06/12 10:34:22, riju_ wrote:
> > On 2014/06/11 15:30:06, timvolodine wrote:
> > > does this compile? should it be static? you probably also want to set it
to
> > some
> > > default value.
> > 
> > It is inside an anonymous namespace, so why use "static"?
> > g_test_device_light_data is anyways used only in this file, as are
> > g_sandbox_enabled and web_gamepad_listener. I did not remove the "static"
> > keyword from these 2 as they are not related to DeviceLight, but now i guess
> its
> > ok to remove this nit in this CL only.
> 
> looks like there is a difference between compilers on this matter. some
> compilers seem to apply external linkage if there is no "static" keyword, so
> it's probably safer to leave "static" as it is? maybe jochen can comment on
this
> as well.

if it's inside an anonymous namespace, nothing can link against it, so you don't
need static.

[jochen (gone - plz use gerrit), 2014-06-13 16:09:44]

On 2014/06/13 16:07:11, jochen traveling until Jun 23 wrote:
> On 2014/06/12 15:17:28, timvolodine wrote:
> >
>
https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
> > File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/286793002/diff/120001/content/renderer/render...
> > content/renderer/renderer_webkitplatformsupport_impl.cc:151: double
> > g_test_device_light_data;
> > On 2014/06/12 10:34:22, riju_ wrote:
> > > On 2014/06/11 15:30:06, timvolodine wrote:
> > > > does this compile? should it be static? you probably also want to set it
> to
> > > some
> > > > default value.
> > > 
> > > It is inside an anonymous namespace, so why use "static"?
> > > g_test_device_light_data is anyways used only in this file, as are
> > > g_sandbox_enabled and web_gamepad_listener. I did not remove the "static"
> > > keyword from these 2 as they are not related to DeviceLight, but now i
guess
> > its
> > > ok to remove this nit in this CL only.
> > 
> > looks like there is a difference between compilers on this matter. some
> > compilers seem to apply external linkage if there is no "static" keyword, so
> > it's probably safer to leave "static" as it is? maybe jochen can comment on
> this
> > as well.
> 
> if it's inside an anonymous namespace, nothing can link against it, so you
don't
> need static.

a pod class should be a struct, should have an inline ctor that uses initializer
lists and non memset, and no dtor.

[dcheng, 2014-06-13 17:01:35]

https://codereview.chromium.org/286793002/diff/160001/content/common/device_s...
File content/common/device_sensors/device_light_messages.h (right):

https://codereview.chromium.org/286793002/diff/160001/content/common/device_s...
content/common/device_sensors/device_light_messages.h:23:
base::SharedMemoryHandle /* handle */)
Why use a shared memory handle? DeviceLightData is a really small struct anyway.

https://codereview.chromium.org/286793002/diff/160001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/160001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1097: if
(g_test_device_light_data == -1) {
Personally, I'd probably just make this check < 0, since I assume that any
negative light level isn't a valid device light level.

[riju_, 2014-06-13 17:06:26]

hi jochen, thanks for the comments. 

just for context: 
[part 1] This CL.
[part 2] https://codereview.chromium.org/292693004/
[part 3] https://codereview.chromium.org/336823002/

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_data.cc (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_data.cc:13: memset(this, 0,
sizeof(*this));
On 2014/06/12 14:33:50, jochen traveling until Jun 23 wrote:
> don't do this. use proper initialization.

Done.

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_data.h:17: double value;
On 2014/06/12 14:33:50, jochen traveling until Jun 23 wrote:
> this class doesn't follow the coding style at all.
> 
> data members should be private. don't unnecessarily create bitfields. add
> disallow copy/assign. add getters/setters

Done. As Tim suggested, adding a comment why this class does not follow chromium
coding style.

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
File content/common/device_sensors/device_light_hardware_buffer.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/common/device_s...
content/common/device_sensors/device_light_hardware_buffer.h:13: typedef
SharedMemorySeqLockBuffer<DeviceLightData>DeviceLightHardwareBuffer;
On 2014/06/12 14:33:50, jochen traveling until Jun 23 wrote:
> missing space after >

Done.

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

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:25: static const int
kDefaultLightPumpDelayMillis;
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> any reason this is part of the public interface?
Making it protected now.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:39: virtual void
FireEvent() OVERRIDE;
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> please add a comment which interface is implemented here.

Done.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:46: };
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:18:
memset(&data_, 0, sizeof(data_));
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> why is this required?

Done.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:20: virtual
~MockDeviceLightListener() { }
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> iirc chromium style has no space between { }. did you clang-format your cl?

I did not know about clang format. Now I have done "git cl format" . Thanks.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:76:
memset(buffer_, 0, sizeof(DeviceLightHardwareBuffer));
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> no memset plz

Done.

https://codereview.chromium.org/286793002/diff/140001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/286793002/diff/140001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.h:142:
blink::WebDeviceLightListener* listener) OVERRIDE;
On 2014/06/12 14:33:51, jochen traveling until Jun 23 wrote:
> don't add OVERRIDE to blink api implementations

Done.

[riju_, 2014-06-16 09:31:51]

https://codereview.chromium.org/286793002/diff/160001/content/common/device_s...
File content/common/device_sensors/device_light_messages.h (right):

https://codereview.chromium.org/286793002/diff/160001/content/common/device_s...
content/common/device_sensors/device_light_messages.h:23:
base::SharedMemoryHandle /* handle */)
On 2014/06/13 17:01:34, dcheng wrote:
> Why use a shared memory handle? DeviceLightData is a really small struct
anyway.

I was under the impression that for infrequent updates (like 1 second interval,
e.g. geolocation) we should use just pass the data object using IPC, but the
ones that require more frequent updates (DeviceLight 5 times a second), we
should use Shared Memory. Maybe shared memory  might be a bit overkill for this
small struct, but are there any disadvantages for this approach ?

[riju_, 2014-06-16 10:46:30]

https://codereview.chromium.org/286793002/diff/160001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/286793002/diff/160001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1097: if
(g_test_device_light_data == -1) {
On 2014/06/13 17:01:34, dcheng wrote:
> Personally, I'd probably just make this check < 0, since I assume that any
> negative light level isn't a valid device light level.

Done.

[dcheng, 2014-06-16 17:14:08]

On 2014/06/16 at 09:31:51, rijubrata.bhaumik wrote:
>
https://codereview.chromium.org/286793002/diff/160001/content/common/device_s...
> File content/common/device_sensors/device_light_messages.h (right):
> 
>
https://codereview.chromium.org/286793002/diff/160001/content/common/device_s...
> content/common/device_sensors/device_light_messages.h:23:
base::SharedMemoryHandle /* handle */)
> On 2014/06/13 17:01:34, dcheng wrote:
> > Why use a shared memory handle? DeviceLightData is a really small struct
anyway.
> 
> I was under the impression that for infrequent updates (like 1 second
interval, e.g. geolocation) we should use just pass the data object using IPC,
but the ones that require more frequent updates (DeviceLight 5 times a second),
we should use Shared Memory. Maybe shared memory  might be a bit overkill for
this small struct, but are there any disadvantages for this approach ?

It's a lot more complicated. It's easy to get wrong. From what I can tell, we
actually do this for latency reasons:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/sha...

That being said, it looks like this already has the blessing of other security
IPC reviewers, so I just have one other comment about the IPCs.

https://codereview.chromium.org/286793002/diff/220001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/220001/content/common/device_s...
content/common/device_sensors/device_light_data.h:20: bool valueIsUpdated;
I don't understand the purpose of valueIsUpdated. The patch mentions that the
synchronization method will be mentioned in device_light_hardware_buffer.h, but
I don't see any further information about it. I think it would be most natural
to keep a copy of the last seen data and fire the event if data_.value !=
data.value. That way you don't need the bool at all?

Also, this should be named value_is_updated... if it's needed.

[riju_, 2014-06-18 09:26:01]

https://codereview.chromium.org/286793002/diff/220001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/220001/content/common/device_s...
content/common/device_sensors/device_light_data.h:20: bool valueIsUpdated;
On 2014/06/16 17:14:08, dcheng wrote:
> I don't understand the purpose of valueIsUpdated. The patch mentions that the
> synchronization method will be mentioned in device_light_hardware_buffer.h,
but
> I don't see any further information about it. I think it would be most natural
> to keep a copy of the last seen data and fire the event if data_.value !=
> data.value. That way you don't need the bool at all?
> 
> Also, this should be named value_is_updated... if it's needed.

Done.

[dcheng, 2014-06-18 16:51:19]

LGTM with nits addressed

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

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:16: :
DeviceSensorEventPump(kDefaultLightPumpDelayMillis),
Not for this patch, but it would be nice to fix the underlying class to use
base::TimeDelta here in the long run.

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:23: listener_(0),
NULL

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:50: if (data.value !=
last_seen_data_ && did_return_light_data) {
Nit: It probably makes more sense to order the check for did_return_light_data
first.

[timvolodine, 2014-06-18 17:38:36]

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

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:18:
last_seen_data_(-1) {
you probably also want to set to -1 when Stop is called.

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:93:
TEST_F(DeviceLightEventPumpTest, DidStartPolling) {
+ test when values are equal and no change?

[riju_, 2014-06-19 13:15:54]

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

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:16: :
DeviceSensorEventPump(kDefaultLightPumpDelayMillis),
On 2014/06/18 16:51:19, dcheng wrote:
> Not for this patch, but it would be nice to fix the underlying class to use
> base::TimeDelta here in the long run.
Ok. I will put up the new CL once this goes in.

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:18:
last_seen_data_(-1) {
On 2014/06/18 17:38:36, timvolodine wrote:
> you probably also want to set to -1 when Stop is called.

Done.

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:23: listener_(0),
On 2014/06/18 16:51:19, dcheng wrote:
> NULL

Done.

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.cc:50: if (data.value !=
last_seen_data_ && did_return_light_data) {
On 2014/06/18 16:51:19, dcheng wrote:
> Nit: It probably makes more sense to order the check for did_return_light_data
> first.

Done.

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
File content/renderer/device_sensors/device_light_event_pump_unittest.cc
(right):

https://codereview.chromium.org/286793002/diff/240001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump_unittest.cc:93:
TEST_F(DeviceLightEventPumpTest, DidStartPolling) {
On 2014/06/18 17:38:36, timvolodine wrote:
> + test when values are equal and no change?

Done.

[timvolodine, 2014-06-19 13:58:30]

lgtm thanks!

[riju_, 2014-06-25 11:50:51]

jochen : friendly ping. any further comments for this CL ?

[jochen (gone - plz use gerrit), 2014-06-25 13:16:11]

https://codereview.chromium.org/286793002/diff/280001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/280001/content/common/device_s...
content/common/device_sensors/device_light_data.h:17: CONTENT_EXPORT
DeviceLightData() : value(-1) {}
no need for content exporting this, since all is inline

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

https://codereview.chromium.org/286793002/diff/280001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:43: static const int
kDefaultLightPumpDelayMillis;
that value is not used anywhere outside of the .cc file as far as I can see, so
it doesn't need to be on the class at all.

You can just define a const int in an anonymous namespace inside the cc file

https://codereview.chromium.org/286793002/diff/280001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/286793002/diff/280001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.h:168: static void
SetMockGamepadsForTesting(const blink::WebGamepads& pads);
not related to this CL?

https://codereview.chromium.org/286793002/diff/280001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.h:179: static void
SetMockDeviceLightDataForTesting(double data);
can you hook this API up to content_shell / test_runner?

[riju_, 2014-06-25 15:53:11]

https://codereview.chromium.org/286793002/diff/280001/content/common/device_s...
File content/common/device_sensors/device_light_data.h (right):

https://codereview.chromium.org/286793002/diff/280001/content/common/device_s...
content/common/device_sensors/device_light_data.h:17: CONTENT_EXPORT
DeviceLightData() : value(-1) {}
On 2014/06/25 13:16:11, jochen wrote:
> no need for content exporting this, since all is inline

Done.

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

https://codereview.chromium.org/286793002/diff/280001/content/renderer/device...
content/renderer/device_sensors/device_light_event_pump.h:43: static const int
kDefaultLightPumpDelayMillis;
On 2014/06/25 13:16:11, jochen wrote:
> that value is not used anywhere outside of the .cc file as far as I can see,
so
> it doesn't need to be on the class at all.
> 
> You can just define a const int in an anonymous namespace inside the cc file

Done.

https://codereview.chromium.org/286793002/diff/280001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/286793002/diff/280001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.h:168: static void
SetMockGamepadsForTesting(const blink::WebGamepads& pads);
On 2014/06/25 13:16:11, jochen wrote:
> not related to this CL?

Sorry, my mistake in rebasing. Done.

https://codereview.chromium.org/286793002/diff/280001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.h:179: static void
SetMockDeviceLightDataForTesting(double data);
On 2014/06/25 13:16:11, jochen wrote:
> can you hook this API up to content_shell / test_runner?

I have the layout support patch here.
https://codereview.chromium.org/336823002/

[jochen (gone - plz use gerrit), 2014-06-26 14:59:38]

lgtm

[riju_, 2014-06-27 08:14:15]

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

[commit-bot: I haz the power, 2014-06-27 08:15:47]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-06-27 11:51:24]

Change committed as 280306