4 years, 3 months ago
(2016-09-21 09:08:38 UTC)
#4
Dry run: This issue passed the CQ dry run.
shalamov
Please take a look. I was trying to figure out what was the root cause ...
4 years, 3 months ago
(2016-09-21 10:05:24 UTC)
#5
Please take a look.
I was trying to figure out what was the root cause for the failure in previous
CL and I hope I solved it in https://codereview.chromium.org/2358513002/
Is there a way to test CL on failed bots? They are missing from trybots list.
Tim, I've made following changes:
* Merged idl-AmbientLightSensorReading.html with idl-AmbientLightSensor.html
* Used "'AmbientLightSensor' in window" to check for interfaces.
> third_party/WebKit/Source/modules/sensor/AmbientLightSensorReading.cpp:31:
return 0.0;
> is this according to spec?
Spec doesn't say what should be expected behaviour. I've created issue for
ambient light sensor specification
https://github.com/w3c/ambient-light/issues/15
>third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensor.html:1: <!DOCTYPE
html>
>no expectations txt files anywhere?
No expectations needed for testharness based tests that are passing (NOTE: Tests
which use testharness.js do not have expected result files if all test cases
pass.).
https://www.chromium.org/developers/testing/webkit-layout-tests
> third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:188:
> Just a couple of ideas for future consideration:
> - seems like nice to add some 'classical' layout test at some point. e.g.
> DeviceOrientation/window-property.html or
> WebKit/LayoutTests/battery-status/api-defined.html
> - maybe something trivial that shows how to use the API (thinking of e.g.
> WebKit/LayoutTests/battery-status/promise-with-eventlisteners.html). I
> understand it's covered by the tests here but seem a bit harder to read.
> - is there a test for special cases e.g. if there is no sensor, or when
onerror
> is expected..?
Mikhail is adding https / main frame / frequency cap functionality and adding
more tests in (https://codereview.chromium.org/2353493002/) and also
implementing onerror event and will upload new CL soon.
Mikhail
lgtm
4 years, 3 months ago
(2016-09-21 14:05:10 UTC)
#6
lgtm
haraken
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h#newcode19 third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public GarbageCollectedMixin Why do you need to make ...
4 years, 3 months ago
(2016-09-21 14:27:51 UTC)
#7
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h#newcode19 third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public GarbageCollectedMixin On 2016/09/21 14:27:51, haraken wrote: > ...
4 years, 3 months ago
(2016-09-22 06:33:39 UTC)
#8
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public
GarbageCollectedMixin
On 2016/09/21 14:27:51, haraken wrote:
>
> Why do you need to make this a GC mixin? Can you just keep it as
> GarbageCollected<>? Then AmbientLightSensorReading doesn't need to inherit
from
> GarbageCollected<>.
Member AmbientLightSensorReadingInit mAmbientLightSensorReadingInit; in
AmbientLightSensorReading requires finalization, that was the reason why I made
base class a mixin. All other concrete sensors would also have similar members,
so that the reading can be constructed from JS context.
haraken
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h#newcode19 third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public GarbageCollectedMixin On 2016/09/22 06:33:39, shalamov wrote: > ...
4 years, 3 months ago
(2016-09-22 13:55:05 UTC)
#9
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public
GarbageCollectedMixin
On 2016/09/22 06:33:39, shalamov wrote:
> On 2016/09/21 14:27:51, haraken wrote:
> >
> > Why do you need to make this a GC mixin? Can you just keep it as
> > GarbageCollected<>? Then AmbientLightSensorReading doesn't need to inherit
> from
> > GarbageCollected<>.
>
> Member AmbientLightSensorReadingInit mAmbientLightSensorReadingInit; in
> AmbientLightSensorReading requires finalization, that was the reason why I
made
> base class a mixin. All other concrete sensors would also have similar
members,
> so that the reading can be constructed from JS context.
Can you just make SensorReading GarbageCollectedFinalized?
A GC mixin makes things complicated and slower, so we should avoid using it
unless necessary.
shalamov
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h File third_party/WebKit/Source/modules/sensor/SensorReading.h (right): https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/modules/sensor/SensorReading.h#newcode19 third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public GarbageCollectedMixin On 2016/09/22 13:55:05, haraken wrote: > ...
4 years, 3 months ago
(2016-09-22 15:15:09 UTC)
#10
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public
GarbageCollectedMixin
On 2016/09/22 13:55:05, haraken wrote:
> On 2016/09/22 06:33:39, shalamov wrote:
> > On 2016/09/21 14:27:51, haraken wrote:
> > >
> > > Why do you need to make this a GC mixin? Can you just keep it as
> > > GarbageCollected<>? Then AmbientLightSensorReading doesn't need to inherit
> > from
> > > GarbageCollected<>.
> >
> > Member AmbientLightSensorReadingInit mAmbientLightSensorReadingInit; in
> > AmbientLightSensorReading requires finalization, that was the reason why I
> made
> > base class a mixin. All other concrete sensors would also have similar
> members,
> > so that the reading can be constructed from JS context.
>
> Can you just make SensorReading GarbageCollectedFinalized?
>
> A GC mixin makes things complicated and slower, so we should avoid using it
> unless necessary.
Done.
4 years, 3 months ago
(2016-09-23 08:16:22 UTC)
#11
On 2016/09/22 15:15:09, shalamov wrote:
>
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/sensor/SensorReading.h (right):
>
>
https://codereview.chromium.org/2356133002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/sensor/SensorReading.h:19: : public
> GarbageCollectedMixin
> On 2016/09/22 13:55:05, haraken wrote:
> > On 2016/09/22 06:33:39, shalamov wrote:
> > > On 2016/09/21 14:27:51, haraken wrote:
> > > >
> > > > Why do you need to make this a GC mixin? Can you just keep it as
> > > > GarbageCollected<>? Then AmbientLightSensorReading doesn't need to
inherit
> > > from
> > > > GarbageCollected<>.
> > >
> > > Member AmbientLightSensorReadingInit mAmbientLightSensorReadingInit; in
> > > AmbientLightSensorReading requires finalization, that was the reason why I
> > made
> > > base class a mixin. All other concrete sensors would also have similar
> > members,
> > > so that the reading can be constructed from JS context.
> >
> > Can you just make SensorReading GarbageCollectedFinalized?
> >
> > A GC mixin makes things complicated and slower, so we should avoid using it
> > unless necessary.
>
> Done.
haraken@ I made SensorReading class GarbageCollectedFinalized, is this
sufficient?
haraken
Sorry for the review delay -- this slipped off my review list... LGTM! https://codereview.chromium.org/2356133002/diff/20001/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h File ...
4 years, 2 months ago
(2016-09-27 13:32:23 UTC)
#12
Issue 2356133002: Reland of [sensors] Ambient light sensor bindings implementation
(Closed)
Created 4 years, 3 months ago by shalamov
Modified 4 years, 2 months ago
Reviewers: jochen (gone - plz use gerrit), haraken, Mikhail, timvolodine
Base URL:
Comments: 5
Chromium Code Reviews
Issue
2356133002:
Reland of [sensors] Ambient light sensor bindings implementation (Closed)
