[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 #30 from Arunprasad <ArunPrasadR at nds.com>  2013-04-08 00:31:27 PST ---
(From update of attachment 196188)
View in context: https://bugs.webkit.org/attachment.cgi?id=196188&action=review

>> Source/WebCore/bridge/c/c_instance.cpp:284
>> +    // Fallback to default implementation
> 
> Missing period at the end of the sentence.

It is my mistake, anyhow I will raise a bug against check-webkit-style, also will try to add this constraint to trap in EWS itself :)

>> 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?

Sure :) Will find one !!

>> Source/WebCore/bridge/c/c_instance.cpp:318
>> +    // Invoke the 'C' method.
> 
> 'C'? :)

NPAPI method is called as 'C' method

>> Source/WebCore/bridge/c/c_instance.cpp:331
>> +        throwError(exec, createError(exec, "Error calling method on NPObject."));
> 
> ->ASCIILiteral("Error calling method on NPObject.")

Sorry :( I was not aware of that class. Will change that :), AFAIK ASCIILiteral is to reduce the memory usage in case of const string, isn't it?

>> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:158
>> +        return true;
> 
> No need to release the memory?

Memory will be release by the JSValue CInstance::invokeMethod after converting NPAPI's NPVariant type to JSC's JSValue using _NPN_ReleaseVariantValue(&resultVariant);

>> LayoutTests/plugins/npruntime/tostring.html:20
>> +        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.

I just blindly followed the existing test scripts like "LayoutTests/plugins/npruntime/construct.html". Your comment is really valid, will change accordingly :)

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