Bug 77605

The Web IDL spec: http://dev.w3.org/2006/webapi/WebIDL/#Clamp

*** Bug 67463 has been marked as a duplicate of this bug. ***

(In reply to comment #2) > *** Bug 67463 has been marked as a duplicate of this bug. *** Oh, I overlooked the existing bug.

np. I'm sorry for being lazy for a long time on this issue.

Created attachment 154879 [details] wipPatch This patch supports the methods with clamp extended attribute. As per spec says : The [Clamp] extended attribute MUST take no arguments. The [Clamp] extended attribute MUST NOT appear on a read only attribute, or an attribute, operation argument or dictionary member that is not of an integer type. It also MUST NOT be used in conjunction with the [EnforceRange] extended attribute. This patch is based on the current Implementation for WebSockets.idl in JS/V8WebSocketsCustom.cpp::close(). Supporting [Clamp] we should be able to remove these custom bindings. Not marking for review as currently not supports other bindings like (CPP,ObjC,GObject).

Comment on attachment 154879 [details] wipPatch View in context: https://bugs.webkit.org/attachment.cgi?id=154879&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1686 > + $parameterCheckString .= " double $parameterName = $nativeValue;\n"; $parameterCheckString .= " double $parameterName = $nativeValue;\n"; //Should be $parameterCheckString .= " $parameter->type $parameterName = $nativeValue;\n" > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2357 > + code = clampTo(code, minValue, maxValue); In the current JS/V8WebSocketCustom.cpp it again clamps this(code) to integer. code = clampToInteger(x); //http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSWebSocketCustom.cpp?rev=115670#L136 Which is confusing as spec says it should be double value. This is clamped to Integer to match declaration void close(int code, const String& reason, ExceptionCode&);

Comment on attachment 154879 [details] wipPatch View in context: https://bugs.webkit.org/attachment.cgi?id=154879&action=review > Source/WebCore/bindings/scripts/test/TestObj.idl:191 > + void classMethodWithClamp(in [Clamp] long code, in [Clamp] double gode); Per the spec, I think you cannot use [Clamp] for double. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2360 > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); This check should be put just after toInt32(). > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2373 > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); Ditto. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1731 > + double code = toInt32(codeValue); This can throw exception. Please use EXCEPTION_BLOCK(). > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1737 > + code = clampTo(code, minValue, maxValue); - You need to implement clampTo() somewhere. Maybe bindings/generic/GenericBinding.h? - Let's move 'double maxValue = ...; double minValue = ...;' and 'if(isnan(code)) { ... } else { ... }' into clampTo(). - toClampedValue() might be a better name. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1739 > + if (ec) > + return throwError(ec, args.GetIsolate()); Remove this. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1744 > + double maxValue = static_cast<double>(std::numeric_limits<uint16_t>::max()); > + double minValue = static_cast<double>(std::numeric_limits<uint16_t>::min()); You need to avoid duplicate declarations of maxValue and minValue. By moving the logic to clampTo(), you can solve the issue. > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1750 > + if (ec) > + return throwError(ec, args.GetIsolate()); Remove this.

(> Not marking for review as currently not supports other bindings like (CPP,ObjC,GObject). For the time being in this patch, I think it is OK to just skip [Clamp] attributes in CPP, ObjC and GObject (as a bunch of attributes are already skipped in CPP, ObjC and GObject).

(In reply to comment #6) > Which is confusing as spec says it should be double value. This is clamped to Integer to match declaration > void close(int code, const String& reason, ExceptionCode&); Shall we fix it before landing this patch?

Created attachment 154975 [details] Updated Patch (In reply to comment #7) > > Source/WebCore/bindings/scripts/test/TestObj.idl:191 > > + void classMethodWithClamp(in [Clamp] long code, in [Clamp] double gode); > Per the spec, I think you cannot use [Clamp] for double. Yes right because of "not of an integer type". Thanks. > > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2360 > > + if (exec->hadException()) > > + return JSValue::encode(jsUndefined()); > > This check should be put just after toInt32(). Done. > > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1731 > > + double code = toInt32(codeValue); > This can throw exception. Please use EXCEPTION_BLOCK(). Done. > > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1739 > > + if (ec) > > + return throwError(ec, args.GetIsolate()); > > Remove this. Done > > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1737 > > + code = clampTo(code, minValue, maxValue); > > - You need to implement clampTo() somewhere. Maybe bindings/generic/GenericBinding.h? > > - Let's move 'double maxValue = ...; double minValue = ...;' and 'if(isnan(code)) { ... } else { ... }' into clampTo(). > > - toClampedValue() might be a better name. > > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1744 > > + double maxValue = static_cast<double>(std::numeric_limits<uint16_t>::max()); > > + double minValue = static_cast<double>(std::numeric_limits<uint16_t>::min()); > > You need to avoid duplicate declarations of maxValue and minValue. By moving the logic to clampTo(), you can solve the issue. > > > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1750 > > + if (ec) > > + return throwError(ec, args.GetIsolate()); > > Remove this. In current patch I tried to to simplify this and now there shouldn't be problem on duplicate declaration. Still if insist I will move it to GenericBinding.h. (In reply to comment #9) > > Which is confusing as spec says it should be double value. This is clamped to Integer to match declaration > > void close(int code, const String& reason, ExceptionCode&); > > Shall we fix it before landing this patch? I checked with specification again I think we need not to modify anything here as clampTo<T>(...) solves the problem and also matches the spec: http://www.w3.org/TR/WebIDL/#es-unsigned-short (3rd Point)

Comment on attachment 154975 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154975&action=review > Source/WebCore/bindings/scripts/test/TestObj.idl:191 > +#if defined(TESTING_V8) || defined(TESTING_JS) This is a solution to skip ObjC, GObject and CPP for TestObj.idl, but this is not a solution to skip them for real IDL files. So instead of adding this defined macro here, please modify CodeGenerator{ObjC,GObject,CPP}.pm so that they can skip generating code for [Clamp] attributes. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2355 > + objArgsShort = 0.0; 0.0 => 0 > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2365 > + objArgsLong = 0.0; 0.0 => 0 > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1732 > + objArgsShort = 0.0; You need to declare objArgsShort. 0.0 => 0 > Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1737 > + objArgsLong = 0.0; Ditto.

Created attachment 155127 [details] patch Updated Patch.

Comment on attachment 155127 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=155127&action=review Looks OK > Source/WebCore/ChangeLog:14 > + * bindings/scripts/CodeGeneratorCPP.pm: > + (SkipFunction): Skips methods with [Clamp] parameters. Nit: No update in run-bindings-tests results for CPP? (I am not sure but a couple of lines might be generated even if you skipped it. Please just check it.) > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2366 > + if (isnan(objArgsShortNativeValue)) > + objArgsShort = 0; > + else Nit: These three lines could be: if (!isnan(objArgsShortNativeValue)) > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2376 > + if (isnan(objArgsLongNativeValue)) > + objArgsLong = 0; > + else Ditto.

Created attachment 155131 [details] patch

(In reply to comment #13) > (From update of attachment 155127 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155127&action=review > > Looks OK > > > Source/WebCore/ChangeLog:14 > > + * bindings/scripts/CodeGeneratorCPP.pm: > > + (SkipFunction): Skips methods with [Clamp] parameters. > > Nit: No update in run-bindings-tests results for CPP? (I am not sure but a couple of lines might be generated even if you skipped it. Please just check it.) I checked it but as we are Skiping function itself so there should no generated code for these bindings, isn't it ..

Comment on attachment 155127 [details] patch Cleared Kentaro Hara's review+ from obsolete attachment 155127 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 155131 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=155131&action=review Thank you very much for the patch! > Source/WebCore/bindings/scripts/IDLAttributes.txt:28 > +Clamp Please add the explanation to https://trac.webkit.org/wiki/WebKitIDL

Comment on attachment 155131 [details] patch Clearing flags on attachment: 155131 Committed r123962: <http://trac.webkit.org/changeset/123962>

All reviewed patches have been landed. Closing bug.

(In reply to comment #17) > (From update of attachment 155131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155131&action=review > > Thank you very much for the patch! > > > Source/WebCore/bindings/scripts/IDLAttributes.txt:28 > > +Clamp > > Please add the explanation to https://trac.webkit.org/wiki/WebKitIDL I have added explanation to http://trac.webkit.org/wiki/WebKitIDL#Clamp Could you please review those? Thanks..

(In reply to comment #20) > I have added explanation to http://trac.webkit.org/wiki/WebKitIDL#Clamp > Could you please review those? Thanks.. Great, thanks!