|
|
Created:
6 years, 9 months ago by mlamouri (slow - plz ping) Modified:
6 years, 7 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionBatteryStatus: use 'unrestricted double' instead of 'double'
Even though it seems to be the same as far as our WebIDL
parser is concerned, it might be better to keep the IDL as
close to the specification as we can.
BUG=122593
TBR=haraken
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173079
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Messages
Total messages: 24 (0 generated)
nbarth is probably a better reviewer for this CL.
not LGTM Thanks for your interest in improving Blink IDL usage Mounir! We do not currently support 'unrestricted double'; doing so usefully requires adding type-checking to double. I've opened a bug for this, and it's pretty high on my to-do list, but if you'd like to grab it, please go ahead. IDL: unrestricted float and unrestricted double https://code.google.com/p/chromium/issues/detail?id=354298 This CL is not LGTM b/c it doesn't change behavior, but makes the IDL files *look* like there's a difference. As a rule, nop data should not be included in IDL files: if Blink (bindings generator) ignores an IDL keyword or extended attribute, do not include it, as it suggests a difference in behavior when there is none. If this results in a difference from the spec, this is good, as it makes the difference in behavior visible. More details at: http://www.chromium.org/developers/web-idl-interfaces#TOC-IDL The tracking bug for type-checking is at: IDL bindings should do type checking https://code.google.com/p/chromium/issues/detail?id=321518 Does this make sense? Once we implement 'unrestricted double', I'd be glad to resurrect this CL!
On 2014/03/20 03:30:30, Nils Barth wrote: > not LGTM > > Thanks for your interest in improving Blink IDL usage Mounir! > > We do not currently support 'unrestricted double'; > doing so usefully requires adding type-checking to double. > I've opened a bug for this, and it's pretty high on my to-do > list, but if you'd like to grab it, please go ahead. > > IDL: unrestricted float and unrestricted double > https://code.google.com/p/chromium/issues/detail?id=354298 > > This CL is not LGTM b/c it doesn't change behavior, but makes the IDL files > *look* like there's a difference. > > As a rule, nop data should not be included in IDL files: if Blink (bindings > generator) ignores an IDL keyword or extended attribute, do not include it, as > it suggests a difference in behavior when there is none. If this results in a > difference from the spec, this is good, as it makes the difference in behavior > visible. > > More details at: > http://www.chromium.org/developers/web-idl-interfaces#TOC-IDL > > The tracking bug for type-checking is at: > IDL bindings should do type checking > https://code.google.com/p/chromium/issues/detail?id=321518 > > Does this make sense? > > Once we implement 'unrestricted double', I'd be glad to resurrect this CL! Isn't the IDL bindings considering 'float' and 'double' as 'unrestricted float' and 'unrestricted double' in practice? It seems to me that using 'unrestricted' is fine given that we would behave exactly as expected. If we keep 'double' here we will have to worry about those when 'double' will behave as expected (ie. restricted).
In brief: I'm adding support for this distinction right now; could you hold off until I've done so? Once I have, this would be a great first use case! On 2014/03/20 10:43:27, Mounir Lamouri wrote: > Isn't the IDL bindings considering 'float' and 'double' as 'unrestricted float' > and 'unrestricted double' in practice? Yes, currently it treats float/double as unrestricted. > It seems to me that using 'unrestricted' is fine given that we would behave exactly as expected. Could you hold off a few days, as I'm in the middle of adding explicit support for these? (As stated, we don't want to include nop data; that's why I'm saying "no, not yet".) Also, such an interface changes needs tests anyway, explicitly testing that non-finite values are ok. > If we keep 'double' here we will have to worry about those when 'double' will behave as expected > (ie. restricted). There is no problem with keeping 'double' here, because we are not going to change the behavior without opt-in. There are currently about 200 uses of 'double' in 50 interfaces and 300 uses of 'float' in 70 interfaces, some of which should be unrestricted and some of which should not. These will be converted one interface at a time. Yes, this will take a very long time. However, this is necessary to avoid breakage, exactly as you indicate, because the actual expectations are often not documented, not tested, and the spec may differ from legacy expectations.
If you are working on that, I guess I could just wait, there is no rush to have this patch in. Thank you for working on this and sorry for being a bit pushy :)
On 2014/03/24 14:33:40, Mounir Lamouri wrote: > If you are working on that, I guess I could just wait, there is no rush to have > this patch in. > > Thank you for working on this and sorry for being a bit pushy :) No problem, and thanks for pushing for correct code!
On 2014/03/25 09:51:40, Nils Barth wrote: > On 2014/03/24 14:33:40, Mounir Lamouri wrote: > > If you are working on that, I guess I could just wait, there is no rush to > have > > this patch in. > > > > Thank you for working on this and sorry for being a bit pushy :) > > No problem, and thanks for pushing for correct code! Nils, is that ready to go now?
On 2014/04/30 09:01:39, Mounir Lamouri wrote: > On 2014/03/25 09:51:40, Nils Barth wrote: > > On 2014/03/24 14:33:40, Mounir Lamouri wrote: > > > If you are working on that, I guess I could just wait, there is no rush to > > have > > > this patch in. > > > > > > Thank you for working on this and sorry for being a bit pushy :) > > > > No problem, and thanks for pushing for correct code! > > Nils, is that ready to go now? Yup! (This only has readonly double attributes, so it's no change in behavior, and the compiler now support these.) Could you also add [TypeChecking=Unrestricted] to the interface, which ensures that checking is actually enforced otherwise?
A few IDL nits, otherwise LGTM! https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.idl (right): https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:5: // http://dev.w3.org/2009/dap/system-info/battery-status.html More precise URL? https://dvcs.w3.org/hg/dap/raw-file/tip/battery/Overview.html#batterymanager-... https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:8: RuntimeEnabled=BatteryStatus, Could you add: TypeChecking=Unrestricted, ...here? Thanks! https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:19: Could you remove the empty line? https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:21: ...here too?
https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.idl (right): https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:5: // http://dev.w3.org/2009/dap/system-info/battery-status.html On 2014/05/01 01:19:50, Nils Barth wrote: > More precise URL? > > https://dvcs.w3.org/hg/dap/raw-file/tip/battery/Overview.html#batterymanager-... Done. https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:8: RuntimeEnabled=BatteryStatus, On 2014/05/01 01:19:50, Nils Barth wrote: > Could you add: > TypeChecking=Unrestricted, > ...here? > Thanks! Done. https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:19: On 2014/05/01 01:19:50, Nils Barth wrote: > Could you remove the empty line? Done. https://codereview.chromium.org/203683002/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.idl:21: On 2014/05/01 01:19:50, Nils Barth wrote: > ...here too? Done.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/203683002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/203683002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink
The CQ bit was checked by nbarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/203683002/20001
haraken, could this get an ok? (nop change, fixes to accord with spec)
Message was sent while issue was closed.
Change committed as 173079
Message was sent while issue was closed.
LGTM |