Bug 77605 – [Clamp] support in binding generator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kent Tamura]

We don't support [Clamp] extended attribute in IDL files.  So we had to implement custom binding code for Bug 66925.

[Kentaro Hara]

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

[Takashi Toyoshima]

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

[Kent Tamura]

(In reply to comment #2)
> *** Bug 67463 has been marked as a duplicate of this bug. ***

Oh, I overlooked the existing bug.

[Takashi Toyoshima]

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

[Vineet Chaudhary (vineetc)]

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).

[Vineet Chaudhary (vineetc)]

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&);

[Kentaro Hara]

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.

[Kentaro Hara]

(> 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).

[Kentaro Hara]

(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?

[Vineet Chaudhary (vineetc)]

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)

[Kentaro Hara]

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.

[Vineet Chaudhary (vineetc)]

Created attachment 155127 [details]
patch

Updated Patch.

[Kentaro Hara]

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.

[Vineet Chaudhary (vineetc)]

Created attachment 155131 [details]
patch

[Vineet Chaudhary (vineetc)]

(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 ..

[WebKit Review Bot]

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.

[Kentaro Hara]

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

[WebKit Review Bot]

Comment on attachment 155131 [details]
patch

Clearing flags on attachment: 155131

Committed r123962: <http://trac.webkit.org/changeset/123962>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vineet Chaudhary (vineetc)]

(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..

[Kentaro Hara]

(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!