[rfc][icedtea-web] rewert of changset 757 "Simplify IcedTeaScriptableJavaObject"

[Andrew Azores]

On 01/14/2014 10:48 AM, Jiri Vanek wrote:
> Unluckily the regressions caused by (otherwise good) 
> http://icedtea.classpath.org/hg/icedtea-web/rev/ee92f55c69a3
>
> +2013-06-21  Adam Domurad  <adomurad at redhat.com>
> +
> +    * plugin/icedteanp/IcedTeaScriptablePluginObject.cc: Simplify
> +    IcedTeaScriptableJavaObject
> +    * plugin/icedteanp/IcedTeaScriptablePluginObject.h: Same
>
> were never successfully fixed, and I gave up on it :(
>
> This is revert of patch in tip. Please note, that this is *manual* 
> revert of patch.
> When I did it , i was just 50% sure what I done. But the result of 
> selected set of reproducers have persuaded me that I did it correctly. 
> (three "r" attachments)
>
> So (any) reviwer, please (and sorry!), be double careful!-)
>
> J.

Well, the patch generally looks okay, and your results are convincing. 
There seems to be no affect for me running the tests in Firefox, but at 
least reverting the changes hasn't made anything worse as far as I can 
tell. There is this that I pointed out over IRC and will repeat here though:

      // If object is an array and requested "method" may be a number, 
check for it first
-    if ( scriptable_object->is_object_array  ||
+    if ( !((IcedTeaScriptableJavaObject*) npobj)->isArray()  ||
           (browser_functions.intfromidentifier(name_id) < 0))

it seems to me like the value of this condition has changed. But, you 
are restoring it back to how it used to be, so perhaps this is actually 
what the regression was in the first place. Like I said, it seems to 
make no difference in my testing, but perhaps it would be worth it if 
you checked the results of removing the negation in the condition as 
well. Most of the rest of the patch seems to be simple refactoring but 
this appears to actually change some behaviour.

Thanks,

-- 
Andrew A