[rfc][icedtea-web] Rewrite of MethodOverloadResolver

[Jiri Vanek]

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?