Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
775763007:
Use event init dictionary in modules/device_light (Closed)
DescriptionUse event init dictionary in modules/device_light
This CL replaces manually-written DeviceLightEventInit with
IDL dictionary. fast/dom/DeviceLight/create-event.html covers this change.
BUG=433179
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187214
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 28 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
bashi@chromium.org changed reviewers: + haraken@chromium.org, rijubrata.bhaumik@intel.com, timvolodine@chromium.org
PTAL?
https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:27: , m_value(std::numeric_limits<double>::infinity()) Doesn't this change existing behavior? I guess the default value was 0 before this CL.
https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:27: , m_value(std::numeric_limits<double>::infinity()) On 2014/12/03 04:31:06, haraken wrote: > > Doesn't this change existing behavior? I guess the default value was 0 before > this CL. This CL changes "bubbles" behavior, but doesn't changes "value" behavior. It is (and was) initialized with positive infinity. The spec also says it should be initialized with positive infinity. "If the implementation is unable to report the current light level, initialize the value attribute to positive Infinity, otherwise initialize the attribute to the current light level."
https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:27: , m_value(std::numeric_limits<double>::infinity()) On 2014/12/03 04:31:06, haraken wrote: > > Doesn't this change existing behavior? I guess the default value was 0 before > this CL. This CL changes "bubbles" behavior, but doesn't changes "value" behavior. It is (and was) initialized with positive infinity. The spec also says it should be initialized with positive infinity. "If the implementation is unable to report the current light level, initialize the value attribute to positive Infinity, otherwise initialize the attribute to the current light level."
LGTM https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:27: , m_value(std::numeric_limits<double>::infinity()) On 2014/12/03 04:36:48, bashi1 wrote: > On 2014/12/03 04:31:06, haraken wrote: > > > > Doesn't this change existing behavior? I guess the default value was 0 before > > this CL. > > This CL changes "bubbles" behavior, but doesn't changes "value" behavior. It is > (and was) initialized with positive infinity. The spec also says it should be > initialized with positive infinity. > > "If the implementation is unable to report the current light level, initialize > the value attribute to positive Infinity, otherwise initialize the attribute to > the current light level." ah, makes sense. Thanks.
https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... File LayoutTests/fast/dom/DeviceLight/create-event.html (right): https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... LayoutTests/fast/dom/DeviceLight/create-event.html:34: shouldBeFalse("defaultEvent.bubbles"); unless I am missing something the spec explicitly states that the event should bubble, see http://w3c.github.io/ambient-light/.. https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:29: if (initializer.hasValue()) is this always false now? https://codereview.chromium.org/775763007/diff/60001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:30: m_value = initializer.value(); can this be initialized directly in the member initialization list?
https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... File LayoutTests/fast/dom/DeviceLight/create-event.html (right): https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... LayoutTests/fast/dom/DeviceLight/create-event.html:34: shouldBeFalse("defaultEvent.bubbles"); On 2014/12/03 12:37:50, timvolodine wrote: > unless I am missing something the spec explicitly states that the event should > bubble, see http://w3c.github.io/ambient-light/.. It seems that I forgot to put a comment to describe this change (I wrote it in the initial PS, but deleted it before actual review). Sorry for the confusion. I think it should be configurable by scripts, as all other interfaces which inherit from Event allow to do so. The spec says: "When a user agent is required to fire a device light event, the user agent must run the following steps: 1. Create an event that uses the DeviceLightEvent interface, with the name devicelight, which bubbles, is not cancelable, and has no default action, that also meets the following conditions ..." However, EventInits are used by scripts(not UA), and they should be able to control 'bubbles' and 'cancelable'. Note that you can force DeviceEvent's bubbles always to be true when you create it in Blink, with DeviceEventInit.bubbles = true, or by calling another constructor. If the spec really wants to make the event's bubbles be always true, I'll revise the CL.
timvolodine@, ping? I'd happy to revert the behavior change of bubbles. (We can revisit it later)
https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... File LayoutTests/fast/dom/DeviceLight/create-event.html (right): https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... LayoutTests/fast/dom/DeviceLight/create-event.html:34: shouldBeFalse("defaultEvent.bubbles"); On 2014/12/04 23:40:52, bashi1 wrote: > On 2014/12/03 12:37:50, timvolodine wrote: > > unless I am missing something the spec explicitly states that the event should > > bubble, see http://w3c.github.io/ambient-light/.. > > It seems that I forgot to put a comment to describe this change (I wrote it in > the initial PS, but deleted it before actual review). Sorry for the confusion. > > I think it should be configurable by scripts, as all other interfaces which > inherit from Event allow to do so. > > The spec says: > > "When a user agent is required to fire a device light event, the user agent must > run the following steps: > > 1. Create an event that uses the DeviceLightEvent interface, with the name > devicelight, which bubbles, is not cancelable, and has no default action, that > also meets the following conditions ..." > > However, EventInits are used by scripts(not UA), and they should be able to > control 'bubbles' and 'cancelable'. Note that you can force DeviceEvent's > bubbles always to be true when you create it in Blink, with > DeviceEventInit.bubbles = true, or by calling another constructor. > > If the spec really wants to make the event's bubbles be always true, I'll revise > the CL. I did not write the spec :/, I was looking at http://w3c.github.io/ambient-light/#idl-def-DeviceLightEvent. so according to that bubbles should be true. you can contact the authors if in doubt: Anssi Kostiainen who is at intel would be a good guess I think. btw a developer who implemented Device Light API is one of the reviewers: riju_@ what do you think?
https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... File LayoutTests/fast/dom/DeviceLight/create-event.html (right): https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... LayoutTests/fast/dom/DeviceLight/create-event.html:34: shouldBeFalse("defaultEvent.bubbles"); On 2014/12/09 14:49:29, timvolodine wrote: > On 2014/12/04 23:40:52, bashi1 wrote: > > On 2014/12/03 12:37:50, timvolodine wrote: > > > unless I am missing something the spec explicitly states that the event > should > > > bubble, see http://w3c.github.io/ambient-light/.. > > > > It seems that I forgot to put a comment to describe this change (I wrote it in > > the initial PS, but deleted it before actual review). Sorry for the confusion. > > > > I think it should be configurable by scripts, as all other interfaces which > > inherit from Event allow to do so. > > > > The spec says: > > > > "When a user agent is required to fire a device light event, the user agent > must > > run the following steps: > > > > 1. Create an event that uses the DeviceLightEvent interface, with the name > > devicelight, which bubbles, is not cancelable, and has no default action, that > > also meets the following conditions ..." > > > > However, EventInits are used by scripts(not UA), and they should be able to > > control 'bubbles' and 'cancelable'. Note that you can force DeviceEvent's > > bubbles always to be true when you create it in Blink, with > > DeviceEventInit.bubbles = true, or by calling another constructor. > > > > If the spec really wants to make the event's bubbles be always true, I'll > revise > > the CL. > > I did not write the spec :/, I was looking at > http://w3c.github.io/ambient-light/#idl-def-DeviceLightEvent. > so according to that bubbles should be true. > > you can contact the authors if in doubt: Anssi Kostiainen who is at intel would > be a good guess I think. btw a developer who implemented Device Light API is one > of the reviewers: > riju_@ what do you think? Hmm, my point was "a user agent" is different from "scripts". It seems that it would be better to keep the behavior, so I revised the CL. Could you take another look? Thanks!
On 2014/12/09 14:49:29, timvolodine wrote: > https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... > File LayoutTests/fast/dom/DeviceLight/create-event.html (right): > > https://codereview.chromium.org/775763007/diff/60001/LayoutTests/fast/dom/Dev... > LayoutTests/fast/dom/DeviceLight/create-event.html:34: > shouldBeFalse("defaultEvent.bubbles"); > On 2014/12/04 23:40:52, bashi1 wrote: > > On 2014/12/03 12:37:50, timvolodine wrote: > > > unless I am missing something the spec explicitly states that the event > should > > > bubble, see http://w3c.github.io/ambient-light/.. > > > > It seems that I forgot to put a comment to describe this change (I wrote it in > > the initial PS, but deleted it before actual review). Sorry for the confusion. > > > > I think it should be configurable by scripts, as all other interfaces which > > inherit from Event allow to do so. > > > > The spec says: > > > > "When a user agent is required to fire a device light event, the user agent > must > > run the following steps: > > > > 1. Create an event that uses the DeviceLightEvent interface, with the name > > devicelight, which bubbles, is not cancelable, and has no default action, that > > also meets the following conditions ..." > > > > However, EventInits are used by scripts(not UA), and they should be able to > > control 'bubbles' and 'cancelable'. Note that you can force DeviceEvent's > > bubbles always to be true when you create it in Blink, with > > DeviceEventInit.bubbles = true, or by calling another constructor. > > > > If the spec really wants to make the event's bubbles be always true, I'll > revise > > the CL. > > I did not write the spec :/, I was looking at > http://w3c.github.io/ambient-light/#idl-def-DeviceLightEvent. > so according to that bubbles should be true. > > you can contact the authors if in doubt: Anssi Kostiainen who is at intel would > be a good guess I think. btw a developer who implemented Device Light API is one > of the reviewers: > riju_@ what do you think? @bashi1 thanks for the patch. Anssi is on vacation, so did not have a chance to speak with him. I think as of now we keep the the "bubbles" behavior consistent with the spec as Firefox already ships with this behaviour. Patchest #2, looks fine to me. We can revisit this "bubbles", "cancelable" configuration discussion later. LGTM.
https://codereview.chromium.org/775763007/diff/80001/LayoutTests/fast/dom/Dev... File LayoutTests/fast/dom/DeviceLight/create-event.html (right): https://codereview.chromium.org/775763007/diff/80001/LayoutTests/fast/dom/Dev... LayoutTests/fast/dom/DeviceLight/create-event.html:34: // FIXME: Maybe bubbles should be configurable. fixme looks very vague: either remove or add something like "consider making bubbles property configurable" https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:27: , m_value(std::numeric_limits<double>::infinity()) you could do: m_value(initializer.hasValue() ? initializer.value() : std::numeric_limits<double>::infinity()) https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEventInit.idl (right): https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... Source/modules/device_light/DeviceLightEventInit.idl:4: the diff on the left seems unrelated, could you upload using --no-find-copies?
Thank you for review! https://codereview.chromium.org/775763007/diff/80001/LayoutTests/fast/dom/Dev... File LayoutTests/fast/dom/DeviceLight/create-event.html (right): https://codereview.chromium.org/775763007/diff/80001/LayoutTests/fast/dom/Dev... LayoutTests/fast/dom/DeviceLight/create-event.html:34: // FIXME: Maybe bubbles should be configurable. On 2014/12/11 18:38:46, timvolodine wrote: > fixme looks very vague: either remove or add something like "consider making > bubbles property configurable" Done. https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEvent.cpp (right): https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... Source/modules/device_light/DeviceLightEvent.cpp:27: , m_value(std::numeric_limits<double>::infinity()) On 2014/12/11 18:38:46, timvolodine wrote: > you could do: > m_value(initializer.hasValue() ? initializer.value() : > std::numeric_limits<double>::infinity()) Done. https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... File Source/modules/device_light/DeviceLightEventInit.idl (right): https://codereview.chromium.org/775763007/diff/80001/Source/modules/device_li... Source/modules/device_light/DeviceLightEventInit.idl:4: On 2014/12/11 18:38:46, timvolodine wrote: > the diff on the left seems unrelated, could you upload using --no-find-copies? Done.
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/775763007/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41040)
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/775763007/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41112)
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/775763007/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187214 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
