[rfc][icedtea-web] Rewrite of MethodOverloadResolver
jfabriko at redhat.com
Mon Apr 22 03:29:09 PDT 2013
Hi Adam and Jiri,
I am sorry if this patch was delayed by my forgetfulness. I ran the
liveconnect interactive tests on icedtea-web with the patch and without
the patch - there are three tests that improved in Firefox by applying
JS->J type cast Boolean to java Object - "error" to "passed"
J->JS get boolean - improvement from "error" to "passed"
J->JS get 2d array - improvement from "error" to "passed"
It is a good change and should be pushed,
On Fri, 2013-04-19 at 11:43 +0200, Jiri Vanek wrote:
> On 01/15/2013 05:14 PM, Adam Domurad wrote:
> > On 01/15/2013 08:46 AM, Jiri Vanek wrote:
> >> On 01/09/2013 09:05 PM, Adam Domurad wrote:
> >>> On 12/14/2012 03:43 PM, Adam Domurad wrote:
> >>>> Thought I'd do a simple patch to turn the test code embedded in MethodOverloadResolver into a a
> >>>> proper unit test ... unfortunately, my small look into this class lead me to find many subtle bugs
> >>>> -- and coincidentally decide to do it properly. Lots of corner cases covered, and it could
> >>>> probably use unit tests even still, but its a major improvement and I'm a bit tired of looking at
> >>>> it so I'm posting it.
> >>>> Some broken things I uncovered included improper overloading order, method/argument situations
> >>>> that would error-out if they ever occurred (but were valid), and the fact that the JSObject used
> >>>> throughout the class was actually a dummy value used at the bottom of the class. The code was also
> >>>> made a lot cleaner in the process, in the interest of 'doing it right' while fixing it.
> >>>> ChangeLog:
> >>>> 2012-12-14 Adam Domurad <adomurad at redhat.com>
> >>>> Rewrite of MethodOverloadResolver with detailed unittests.
> >>>> * plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java:
> >>>> Rewritten to reduce duplicated code, fix very subtle bugs in
> >>>> never-tested codepaths, obey spec properly. Introduced new helper types
> >>>> where Object arrays with special-meaning positions were passed
> >>>> around.
> >>>> * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
> >>>> Updated to work with newly introduced types / refactored overload
> >>>> resolver.
> >>>> * tests/netx/unit/sun/applet/MethodOverloadResolverTest.java: In-depth
> >>>> unit tests of hairy details of method overloading in JS<->Java.
> >>> Ping?
> >>> -Adam
> >> Although I really like this patch as it is making MethodOverloadResolver much more readable and better, there is no significant *visible* improvement (according to Jana's tests). Also I was unable to review it properly - as it was de facto replacement.
> >> Unless you+jana will find some improvement in (at least) liveconect test or similar, then I must revoke this patch.
> >> If no "proof" will be found by you and jana, then I would like to get this into head by small (reviewable) steps accompanied by unit/reproducer tests.
> >> Sorry for troubles
> >> J.
> > Just to be clear - I believe many fixed cases are visible in the unit tests. However these are difficult to determine because they were written against the new MethodOverloadResolver. Especially like I pointed out, JSObject was resolving to the completely wrong class. There were certain cases where exceptions would be thrown merely if a certain overload was present. If this is the issue, we can get more of these cases into the reproducers, but I do think there were a number of them. I'll work with Jana on this one.
> > -Adam
> This get lost athe end?
More information about the distro-pkg-dev