[Webkit-unassigned] [Bug 113139] Call Netscape Plugin's toString() and valueOf() instead of providing default implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=113139





--- Comment #32 from Arunprasad <ArunPrasadR at nds.com>  2013-04-08 11:49:25 PST ---
(In reply to comment #29)
> (From update of attachment 196188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196188&action=review
> 
> r- because of the tests.
> 
> They are not bad, but they could be a lot better.
> 
> Please get Anders for a full review.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:284
> > +    // Fallback to default implementation
> 
> Missing period at the end of the sentence.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:298
> > +    // ECMA 9.2
> 
> This comment is too terse.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:308
> > +    // Fallback to default implementation
> 
> Missing period.
> 
> > Source/WebCore/bridge/c/c_instance.cpp:312
> > +bool CInstance::convertToPrimitive(ExecState* exec, const char* name, JSValue& resultValue) const
> 
> Maybe it makes sense in NPAPI, but for me, convertToPrimitive() is a mysterious name to me.
> Could this be named better?
> 
> > Source/WebCore/bridge/c/c_instance.cpp:318
> > +    // Invoke the 'C' method.
> 
> 'C'? :)
> 
> > Source/WebCore/bridge/c/c_instance.cpp:331
> > +        throwError(exec, createError(exec, "Error calling method on NPObject."));
> 
> ->ASCIILiteral("Error calling method on NPObject.")
> 
> > Source/WebCore/bridge/c/c_instance.h:75
> >      JSValue booleanValue() const;
> > +    // Helper method to call NPObject's invoke on behalf of toString and valueOf.
> > +    bool convertToPrimitive(ExecState*, const char*, JSValue&) const;
> 
> You should look for the best name possible instead of having a comment.
> 
> This should be private.
> 
> If the function is incorrect if not called from toString() or valueOf(), it should likely be a static function instead of an object method. If the call site does not matter, you should not list call sites in a comment because the comment will get out of date.
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:154
> > +        // Return classname as string
> 
> Missing period.
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:158
> > +        char* className = static_cast<char*>(browser->memalloc(11));
> > +        strcpy(className, "TestObject");
> > +        STRINGZ_TO_NPVARIANT(className, *result);
> > +        return true;
> 
> No need to release the memory?
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:161
> > +        // Return some number
> 
> Period.
> 
> > LayoutTests/plugins/npruntime/tostring.html:12
> > +        successCount ++;
> 
> Remove the extra space.
> 
> > LayoutTests/plugins/npruntime/tostring.html:15
> > +    // Normal plugin.testObject + "" will call valueOf,
> > +    // Do some tricks to make a call to implicit toString.
> 
> Do -> do.
> 
> > LayoutTests/plugins/npruntime/tostring.html:17
> > +        successCount ++;
> 
> Remote the extra space.
> 
> > LayoutTests/plugins/npruntime/tostring.html:20
> > +    if (successCount == 2)
> > +        document.getElementById('result').innerHTML = 'SUCCESS';
> 
> Instead of that, the whole test should use the JavaScript test scripts.
> 
> The comparison should be done with shouldBeEqualToString(foo, bar);
> 
> The reason is on fail, one would know exactly which part failed. Currently, any failure would just mean there is no "SUCCESS" in the results. This is okay when it succeed, but it does not provide enough readability when it fails.
> 
> > LayoutTests/plugins/npruntime/tostring.html:25
> > +This tests that toString() works correctly on NPObject.
> 
> Then with JavaScript test, you should use description("This tests that toString() works correctly on NPObject.").
> 
> > LayoutTests/plugins/npruntime/valueof.html:18
> > +
> > +    var successCount = 0;
> > +    var plugin = document.getElementById("testPlugin");
> > +
> > +    if (plugin.testObject.valueOf() === 123456789)
> > +        successCount ++;
> > +
> > +    if (plugin.testObject == 123456789)
> > +        successCount ++;
> > +
> > +    if (successCount == 2)
> > +        document.getElementById('result').innerHTML = 'SUCCESS';
> 
> Ditto.

Updated a new patch which adheres to Benjamin's suggestions :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.