Bug 113139 - Call Netscape Plugin's toString() and valueOf() instead of providing default implementation
Summary: Call Netscape Plugin's toString() and valueOf() instead of providing default ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-23 09:55 PDT by Arunprasad
Modified: 2013-04-08 15:03 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.51 KB, patch)
2013-03-23 12:35 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (15.36 KB, patch)
2013-03-24 04:07 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64 (932.55 KB, application/zip)
2013-03-24 05:13 PDT, WebKit Review Bot
no flags Details
Patch (14.61 KB, patch)
2013-03-24 05:21 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (14.33 KB, patch)
2013-03-24 07:12 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (14.23 KB, patch)
2013-03-24 08:43 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-future (574.68 KB, application/zip)
2013-03-24 12:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 (656.90 KB, application/zip)
2013-03-24 12:25 PDT, WebKit Review Bot
no flags Details
Patch (13.69 KB, patch)
2013-03-25 13:00 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-future (899.74 KB, application/zip)
2013-03-25 16:39 PDT, Build Bot
no flags Details
Patch (14.91 KB, patch)
2013-04-02 11:10 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (15.73 KB, patch)
2013-04-08 11:46 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (22.09 KB, patch)
2013-04-08 13:16 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (21.62 KB, patch)
2013-04-08 13:51 PDT, Arunprasad Rajkumar
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (720.00 KB, application/zip)
2013-04-08 14:45 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arunprasad 2013-03-23 09:55:25 PDT
Calling a toString()/valueOf() method on NPObject returns some weird value like "NPObject 0x800000, NPClass 0x810000".

For example,

var obj = document.getElementById("pluginElement");
var npObject = obj.getApplication(); //getApplication is a native method implemented in a plugin and return NPObject
console.log("name of the npObject"+npObject); // which prints something like "NPObject 0x800000, NPClass 0x810000"


Can we delegate this call to npapi plugin, instead of providing default value? I saw that this have been done in c_instance.cpp,

JSValue CInstance::stringValue(ExecState* exec) const
{
    char buf[1024];
    snprintf(buf, sizeof(buf), "NPObject %p, NPClass %p", _object, _object->_class);
    return jsString(exec, buf);
}

So instead of providing default implementation, let this to call "toString()"/"valueOf()" method of plugin, if it doesn't exists the let it to fallback to WebCore's default implementation.

JSValue CInstance::stringValue(ExecState* exec)
{
    NPIdentifier ident = NPN_GetStringIdentifier("toString");
    if (!_object->_class->hasMethod(_object, ident))
        return jsUndefined();

    // Invoke the 'C' method.
    bool retval = true;
    NPVariant resultVariant;
    VOID_TO_NPVARIANT(resultVariant);

    {
        JSLock::DropAllLocks dropAllLocks(exec);
        ASSERT(globalExceptionString().isNull());
        retval = _object->_class->invoke(_object, ident, 0, 0, &resultVariant);
        moveGlobalExceptionToExecState(exec);
    }
    
    if (!retval)
        throwError(exec, createError(exec, "Error calling method on NPObject."));

    JSValue resultValue = convertNPVariantToValue(exec, &resultVariant, m_rootObject.get());
    _NPN_ReleaseVariantValue(&resultVariant);
    return resultValue;
}

I would like to provide a patch which looks something similar like above to solve this issue. Is it acceptable?
Comment 1 Eric Seidel (no email) 2013-03-23 11:48:05 PDT
I'm not actively workign on plugins.  I'm not sure who is.
Comment 2 Arunprasad 2013-03-23 12:03:03 PDT
(In reply to comment #1)
> I'm not actively workign on plugins.  I'm not sure who is.

Thanks Eric, Will try in IRC :)
Comment 3 Arunprasad Rajkumar 2013-03-23 12:35:22 PDT
Created attachment 194714 [details]
Patch
Comment 4 Anders Carlsson 2013-03-23 12:42:00 PDT
Comment on attachment 194714 [details]
Patch

Patch looks good but needs a layout test.
Comment 5 Arunprasad 2013-03-23 12:53:01 PDT
(In reply to comment #4)
> (From update of attachment 194714 [details])
> Patch looks good but needs a layout test.

Thanks for your valuable feedback Anders :) I will look into webkit/LayoutTests/plugins/npruntime inorder to understand how the existing test was done.
Comment 6 Arunprasad Rajkumar 2013-03-24 04:07:21 PDT
Created attachment 194742 [details]
Patch
Comment 7 WebKit Review Bot 2013-03-24 04:09:38 PDT
Attachment 194742 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/plugins/npruntime/tostring-expected.txt', u'LayoutTests/plugins/npruntime/tostring.html', u'LayoutTests/plugins/npruntime/valueof-expected.txt', u'LayoutTests/plugins/npruntime/valueof.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bridge/c/c_instance.cpp', u'Source/WebCore/bridge/c/c_instance.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp']" exit_code: 1
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Arunprasad 2013-03-24 04:14:53 PDT
(In reply to comment #7)
> Attachment 194742 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/plugins/npruntime/tostring-expected.txt', u'LayoutTests/plugins/npruntime/tostring.html', u'LayoutTests/plugins/npruntime/valueof-expected.txt', u'LayoutTests/plugins/npruntime/valueof.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bridge/c/c_instance.cpp', u'Source/WebCore/bridge/c/c_instance.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp']" exit_code: 1
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Tools/DumpRenderTree/TestNetscapePlugIn/TestObject.cpp:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 4 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Oops :(

I just followed the existing style which was used for Plugin Properties, 

enum {
    ID_PROPERTY_FOO = 0,
    ID_PROPERTY_BAR,
    ID_PROPERTY_OBJECT_POINTER,
    ID_PROPERTY_TEST_OBJECT,
    ID_PROPERTY_REF_COUNT,
    NUM_TEST_IDENTIFIERS,
};

Is it a false-positive ? (or) Properties follows very old guidelines? Shall I change all properties & method as indicated by style-checker( enum members should use InterCaps with an initial capital letter.)?
Comment 9 Build Bot 2013-03-24 04:40:45 PDT
Comment on attachment 194742 [details]
Patch

Attachment 194742 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17310044
Comment 10 WebKit Review Bot 2013-03-24 05:13:23 PDT
Comment on attachment 194742 [details]
Patch

Attachment 194742 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17188842

New failing tests:
fast/css/font-family-pictograph.html
Comment 11 WebKit Review Bot 2013-03-24 05:13:27 PDT
Created attachment 194743 [details]
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 12 Arunprasad Rajkumar 2013-03-24 05:21:45 PDT
Created attachment 194744 [details]
Patch
Comment 13 Build Bot 2013-03-24 06:04:55 PDT
Comment on attachment 194744 [details]
Patch

Attachment 194744 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17293262
Comment 14 Build Bot 2013-03-24 06:14:15 PDT
Comment on attachment 194744 [details]
Patch

Attachment 194744 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17120830
Comment 15 Arunprasad Rajkumar 2013-03-24 07:12:16 PDT
Created attachment 194750 [details]
Patch
Comment 16 Arunprasad Rajkumar 2013-03-24 08:43:26 PDT
Created attachment 194753 [details]
Patch
Comment 17 Build Bot 2013-03-24 12:19:41 PDT
Comment on attachment 194753 [details]
Patch

Attachment 194753 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17228529

New failing tests:
plugins/npruntime/tostring.html
plugins/npruntime/valueof.html
Comment 18 Build Bot 2013-03-24 12:19:43 PDT
Created attachment 194761 [details]
Archive of layout-test-results from webkit-ews-05 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 19 WebKit Review Bot 2013-03-24 12:25:41 PDT
Comment on attachment 194753 [details]
Patch

Attachment 194753 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17317025

New failing tests:
fast/css/font-family-pictograph.html
Comment 20 WebKit Review Bot 2013-03-24 12:25:45 PDT
Created attachment 194762 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 21 Arunprasad 2013-03-24 20:53:37 PDT
(In reply to comment #17)
> (From update of attachment 194753 [details])
> Attachment 194753 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17228529
> 
> New failing tests:
> plugins/npruntime/tostring.html
> plugins/npruntime/valueof.html

I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?
Comment 22 Arunprasad 2013-03-24 21:25:54 PDT
(In reply to comment #17)
> (From update of attachment 194753 [details])
> Attachment 194753 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17228529
> 
> New failing tests:
> plugins/npruntime/tostring.html
> plugins/npruntime/valueof.html

(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 194753 [details] [details])
> > Attachment 194753 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17228529
> > 
> > New failing tests:
> > plugins/npruntime/tostring.html
> > plugins/npruntime/valueof.html
> 
> I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?

I guess Chromium port never use NPAPI plugin, so they completely avoided LayoutTests/plugins/*, but they are running any new tests if added to LayoutTests. Can anybody from Chromium Team help me to understand this ? Thanks in advance :)
Comment 23 Vivek Galatage 2013-03-25 02:18:53 PDT
(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 194753 [details] [details])
> > Attachment 194753 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17228529
> > 
> > New failing tests:
> > plugins/npruntime/tostring.html
> > plugins/npruntime/valueof.html
> 
> I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?

Because only cr-linux-release runs the tests while cr-linux-debug doen't. Having said that its interesting to find out how the tests are passing on mac-wk2 (I suppose this bot also runs the tests). Also check the test expectation file for the mac to check whether NPAPI tests are being skipped there.
Comment 24 Arunprasad Rajkumar 2013-03-25 13:00:54 PDT
Created attachment 194908 [details]
Patch
Comment 25 Arunprasad 2013-03-25 13:04:26 PDT
(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #17)
> > > (From update of attachment 194753 [details] [details] [details])
> > > Attachment 194753 [details] [details] [details] did not pass mac-ews (mac):
> > > Output: http://webkit-commit-queue.appspot.com/results/17228529
> > > 
> > > New failing tests:
> > > plugins/npruntime/tostring.html
> > > plugins/npruntime/valueof.html
> > 
> > I don't know how it is failing only for cr-linux(release). But why not cr-linux-debug?
> 
> Because only cr-linux-release runs the tests while cr-linux-debug doen't. Having said that its interesting to find out how the tests are passing on mac-wk2 (I suppose this bot also runs the tests). Also check the test expectation file for the mac to check whether NPAPI tests are being skipped there.

Thanks for your clue Vivek :) only qt-mac completely skips the npapi related tests. Other ports omits very few which are not relavent to this bug's testcases. I'm trying out to run LayoutTests for chromium linux build.
Comment 26 Build Bot 2013-03-25 16:39:20 PDT
Comment on attachment 194908 [details]
Patch

Attachment 194908 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17144832

New failing tests:
plugins/npruntime/tostring.html
plugins/npruntime/valueof.html
Comment 27 Build Bot 2013-03-25 16:39:22 PDT
Created attachment 194945 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 28 Arunprasad Rajkumar 2013-04-02 11:10:02 PDT
Created attachment 196188 [details]
Patch
Comment 29 Benjamin Poulain 2013-04-07 23:37:56 PDT
Comment on attachment 196188 [details]
Patch

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.
Comment 30 Arunprasad Rajkumar 2013-04-08 00:31:27 PDT
Comment on attachment 196188 [details]
Patch

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 :)
Comment 31 Arunprasad Rajkumar 2013-04-08 11:46:25 PDT
Created attachment 196879 [details]
Patch
Comment 32 Arunprasad Rajkumar 2013-04-08 11:49:25 PDT
(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 :)
Comment 33 Anders Carlsson 2013-04-08 11:53:19 PDT
Comment on attachment 196879 [details]
Patch

The code change looks good, but for the test please make a specific plug-in test (See the Tests/ subdirectory in TestNetscapePlugIn).
Comment 34 Arunprasad Rajkumar 2013-04-08 13:16:13 PDT
Created attachment 196946 [details]
Patch
Comment 35 Arunprasad Rajkumar 2013-04-08 13:51:17 PDT
Created attachment 196950 [details]
Patch
Comment 36 Anders Carlsson 2013-04-08 14:24:08 PDT
Comment on attachment 196950 [details]
Patch

r=me. Iā€™m going to land this manually so I can add the test file to the Xcode project.
Comment 37 Build Bot 2013-04-08 14:45:40 PDT
Comment on attachment 196950 [details]
Patch

Attachment 196950 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17502380

New failing tests:
plugins/npruntime/tostring.html
plugins/npruntime/valueof.html
Comment 38 Build Bot 2013-04-08 14:45:42 PDT
Created attachment 196959 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 39 Anders Carlsson 2013-04-08 15:03:54 PDT
Committed r147956: <http://trac.webkit.org/changeset/147956>