[webkit-reviews] review denied: [Bug 132460] Enhance IDL compiler so it supports unrestricted float and double : [Attachment 230740] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat May 3 02:04:07 PDT 2014
Dirk Schulze <krit at webkit.org> has denied Rik Cabanier <cabanier at adobe.com>'s
request for review:
Bug 132460: Enhance IDL compiler so it supports unrestricted float and double
https://bugs.webkit.org/show_bug.cgi?id=132460
Attachment 230740: Patch
https://bugs.webkit.org/attachment.cgi?id=230740&action=review
------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=230740&action=review
Wasn't that bad actually. Thought the patch would be bigger. Still some
snippets.
> Source/WebCore/ChangeLog:9
> + Alse updated the IDL files so they use unrestricted float and
Also
> Source/WebCore/bindings/scripts/CodeGenerator.pm:52
> "float" => 1, "double" => 1, "byte" => 1,
> - "octet" => 1);
> + "octet" => 1, "unrestricted float" => 1,
"unrestricted double" => 1);
move "unrestricted float" => 1, "unrestricted double" => 1 right after the line
with float and double, shift "byte" to the line with octet for readability.
> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1722
> + $implIncludes{"<wtf/MathExtras.h>"} = 1; # for isfinite
I wonder if we still use MathExtras.h or some std lib logic instead.
> Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:1191
> + push(@implContent, "#import <wtf/MathExtras.h>\n\n"); # for isfinite
Ditto. Need to check.
> Source/WebCore/bindings/scripts/test/TestTypedefs.idl:45
> + void setShadow(DOUBLE width, DOUBLE height, unrestricted float blur,
[StrictTypeChecking] optional STRING color, optional DOUBLE alpha);
What is DOUBLE?
> Source/WebCore/bindings/scripts/test/TestTypedefs.idl:68
> +typedef unrestricted float DOUBLE;
I see.
> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:65
> + void fillRect(unrestricted float x, unrestricted float y, unrestricted
float width, unrestricted float height);
Interestingly, the Canvas spec uses double everywhere.
> Source/WebCore/html/track/TextTrackCue.idl:29
> + Constructor(unrestricted double startTime, unrestricted double endTime,
DOMString text),
I would make them restricted, see next comment.
> Source/WebCore/html/track/TextTrackCue.idl:41
> + [SetterRaisesException] attribute unrestricted double startTime;
> + [SetterRaisesException] attribute unrestricted double endTime;
according to the spec, these should be restricted, not unrestricted.
> Source/WebCore/page/DOMWindow.idl:111
> + void moveBy([Default=Undefined] optional unrestricted float x,
[Default=Undefined] optional unrestricted float y); // FIXME: this should take
longs not floats.
> + void moveTo([Default=Undefined] optional unrestricted float x,
[Default=Undefined] optional unrestricted float y); // FIXME: this should take
longs not floats.
> + void resizeBy([Default=Undefined] optional unrestricted float x,
[Default=Undefined] optional unrestricted float y); // FIXME: this should take
longs not floats.
> + void resizeTo([Default=Undefined] optional unrestricted float width,
[Default=Undefined] optional unrestricted float height); // FIXME: this should
take longs not floats.
If it should take longs and not floats, the first step might be to use
restricted floats, no?
> Source/WebCore/svg/SVGAnimationElement.idl:31
> + unrestricted float getStartTime();
> + unrestricted float getCurrentTime();
These are tricky. If we restrict for media elements, we might want to restrict
for SVGAnimationElement as well. On the other hand, these are getters so
doesn't matter anyway.
> Source/WebCore/svg/SVGPathElement.idl:30
> + unrestricted float getTotalLength();
We should clarify in the SVG spec where we want to restrict floats and where we
don't. I am fine for doing unrestricted for all SVG element for now. This is
the current behavior anyway.
> LayoutTests/canvas/philip/tests/2d.missingargs-expected.txt:1
> +Failed assertion: expected exception of type TypeError, got: Error:
SyntaxError: DOM Exception 12
Hm. This should really not be a TypeError, no?
> LayoutTests/platform/mac/canvas/philip/tests/2d.missingargs-expected.txt:1
> Failed assertion: expected exception of type TypeError, got: Error:
SyntaxError: DOM Exception 12
Why didn't that go away? Did you add an unrestricted somewhere where it should
be restricted? Is the test incorrect?
More information about the webkit-reviews
mailing list