[webkit-reviews] review granted: [Bug 49704] Make binding code generation scripts support 'short' type : [Attachment 74315] Proposed Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Jian Li <jianli at chromium.org>'s
request for review:
Bug 49704: Make binding code generation scripts support 'short' type
https://bugs.webkit.org/show_bug.cgi?id=49704

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > All the changes in *.cpp and *.h are auto-generated by run-binding-tests.
The last person who made changes to code generation scripts did not run the
script to regenerate the binding test results. So this is the reason that you
see quite a few changes in *.cpp and *.h that are unrelated to adding "short"
support. Those style errors were introduced by some other people's changes to
code generation scripts, for which I rather not to fix.
> > 
> > I see. Looking at CodeGeneratorJS.pm, it would be trivial to at least
insert the missing line break after UNUSED_PARAM(exec) (line 2080). If that
clears up the style errors I think it is absolutely worth doing. Would you
please consider doing this in this patch?
> 
> Done.

Thanks. Unfortunately it doesn't look like that cleared up everything but it's
still an improvement.

> > 
> > > (In reply to comment #2)
> > > > (From update of attachment 74191 [details] [details] [details]
[details])
> > > > View in context:
https://bugs.webkit.org/attachment.cgi?id=74191&action=review
> > > >  
> > > > > WebCore/bindings/scripts/test/TestObj.idl:38
> > > > > +	   attribute short		      shortAttr;
> > > > 
> > > > I think you should test both short and unsigned short, in particular
including boundary conditions.
> > > 
> > > I am fixing this and will upload a new patch soo.
> > 
> > OK. I don't see exactly where (for example) it's tested that setting the
unsigned short attribute to 65535 and fetching it results in 65535. Are tests
like that handled by the harness?
> 
> Unfortunately, I think the current binding test is only for visual
verification purpose. The generated files are not compiled and ran.

Ah. Well, I suppose it will be tested with the DataView implementation shortly.