Summary: | Call Netscape Plugin's toString() and valueOf() instead of providing default implementation | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Arunprasad <ararunprasad> | ||||||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Major | CC: | andersca, arurajku, benjamin, buildbot, dglazkov, eric, rniwa, vivekg, webkit.review.bot | ||||||||||||||||||||||||||||||||
Priority: | P1 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Arunprasad
2013-03-23 09:55:25 PDT
I'm not actively workign on plugins. I'm not sure who is. (In reply to comment #1) > I'm not actively workign on plugins. I'm not sure who is. Thanks Eric, Will try in IRC :) Created attachment 194714 [details]
Patch
Comment on attachment 194714 [details]
Patch
Patch looks good but needs a layout test.
(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. Created attachment 194742 [details]
Patch
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.
(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 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 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 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
Created attachment 194744 [details]
Patch
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 on attachment 194744 [details] Patch Attachment 194744 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17120830 Created attachment 194750 [details]
Patch
Created attachment 194753 [details]
Patch
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 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 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 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
(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? (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 :) (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. Created attachment 194908 [details]
Patch
(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 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 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
Created attachment 196188 [details]
Patch
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 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 :) Created attachment 196879 [details]
Patch
(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 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).
Created attachment 196946 [details]
Patch
Created attachment 196950 [details]
Patch
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 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 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
Committed r147956: <http://trac.webkit.org/changeset/147956> |