[webkit-reviews] review requested: [Bug 77605] [Clamp] support in binding generator : [Attachment 154975] Updated Patch

[bugzilla-daemon at webkit.org]

Vineet Chaudhary (vineetc) <rgf748 at motorola.com> has asked  for review:
Bug 77605: [Clamp] support in binding generator
https://bugs.webkit.org/show_bug.cgi?id=77605

Attachment 154975: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=154975&action=review

------- Additional Comments from Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>
(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)