[rfc][icedtea-web] Remove wrongly 'undummied' JSObject->Java array code from MethodOverloadResolver

[Jiri Vanek]

On 05/01/2013 03:06 PM, Adam Domurad wrote:
> On 05/01/2013 08:19 AM, Jiri Vanek wrote:
>> On 04/29/2013 09:15 PM, Adam Domurad wrote:
>>> So part of what motivated me to rewrite the old MethodOverloadResolver was the fact that I noticed it had a dummy JSObject defined at the bottom of the code, and it was mistakenly using that. It puzzled me why this wasn't observable. Well, it seems it is because the only code path that relied on JSObject itself was incorrect.
>>>
>>> Thanks to Jana for the thorough tests that caught this!
>>> It looks like we do not support conversion of JSObject to a Java array in method signatures. This will bring the code back to its old functionality.
>>>
>>> There is a want for the real fix, but I think we should do a fix in a separate step.
>>>
>>
>> Looks mostly corect. Will the tests need some fixes?
>
> No -- unfortunately the JSObject related tests are commented don't have the necessary privileges to create a JSObject.

Not even in jana's tests?
>
>>> Happy hacking,
>>> -Adam
>>>
>>>
>>> remove-incorrect-method-overload-resolver-code.patch
>>>
>>>
>>> diff --git a/ChangeLog b/ChangeLog
>>> --- a/ChangeLog
>>> +++ b/ChangeLog
>>> @@ -1,3 +1,9 @@
>>> +2013-04-29 Adam Domurad<adomurad at redhat.com>
>>> +
>>> + * plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java
>>> + (getCostAndCastedObject): Remove code that had no effect before refactoring.
>>> + (getBestOverloadMatch): Move debug-only code to debug if-block.
>>> +
>>> 2013-04-29 Jiri Vanek<jvanek at redhat.com>
>>>
>>> More granular initialization of AwtHelper
>>> diff --git a/plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java b/plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java
>>> --- a/plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java
>>> +++ b/plugin/icedteanp/java/sun/applet/MethodOverloadResolver.java
>>> @@ -44,8 +44,6 @@ import java.util.ArrayList;
>>> import java.util.Arrays;
>>> import java.util.List;
>>>
>>> -import netscape.javascript.JSObject;
>>> -
>>> /*
>>> * This class resolved overloaded methods in Java objects using a cost
>>> * based-approach described here:
>>> @@ -65,7 +63,6 @@ public class MethodOverloadResolver {
>>> static final int CLASS_SUPERCLASS_COST = 6;
>>>
>>> static final int CLASS_STRING_COST = 7;
>>> - static final int JSOBJECT_TO_ARRAY_COST = CLASS_STRING_COST;
>>> static final int ARRAY_CAST_COST = 8;
>>>
>>> /* A method signature with its casted parameters
>>> @@ -198,10 +195,10 @@ public class MethodOverloadResolver {
>>>
>>> castedArgs[i] = castedObj;
>>>
>>> - Class<?> castedObjClass = castedObj == null ? null : castedObj.getClass();
>>> - boolean castedObjIsPrim = castedObj == null ? false : castedObj.getClass().isPrimitive();
>>> + if (PluginDebug.DEBUG) { /* avoid toString if not needed */
>>> + Class<?> castedObjClass = castedObj == null ? null : castedObj.getClass();
>>> + boolean castedObjIsPrim = castedObj == null ? false : castedObj.getClass().isPrimitive();
>>>
>>> - if (PluginDebug.DEBUG) { /* avoid toString if not needed */
>>> PluginDebug.debug("Param " + i + " of method " + candidate
>>> + " has cost " + weightedCast.getCost()
>>> + " original param type " + suppliedParamClass
>>> @@ -340,12 +337,6 @@ public class MethodOverloadResolver {
>>> return new WeightedCast(CLASS_STRING_COST, suppliedParam.toString());
>>> }
>>>
>>> - // JSObject to Java array
>>> - if (suppliedParam instanceof JSObject
>>> -&& paramTypeClass.isArray()) {
>>> - return new WeightedCast(JSOBJECT_TO_ARRAY_COST, suppliedParam);
>>
>> Just idea - what about throwing notyetImplementedException here?
>
> This would cause the same regression we're seeing. 'null' is the correct value in this case. We don't want to error out if this is an option, we want to pick a different method.
> I plan to add KnownToFail cases for this, which I think suffices.
>
>>> - }
>>> -
>>> return null;
>>> }
>>>
>>
>> J.
ok then.