Bug 64434 – [Chromium] Implement PluginViewBase::getFormValue

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kenichi Ishibashi]

Chromium now can retrieve form value from NPAPI plugins.

[Kenichi Ishibashi]

Created attachment 100630 [details]
Patch V0

[WebKit Review Bot]

Comment on attachment 100630 [details]
Patch V0

Attachment 100630 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9022427

[Kenichi Ishibashi]

Created attachment 100635 [details]
Patch V1 (attemps to fix build failure)

[WebKit Review Bot]

Comment on attachment 100635 [details]
Patch V1 (attemps to fix build failure)

Attachment 100635 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9018468

New failing tests:
plugins/form-value.html

[WebKit Review Bot]

Created attachment 100637 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

(In reply to comment #4)
> (From update of attachment 100635 [details])
> Attachment 100635 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9018468
> 
> New failing tests:
> plugins/form-value.html

You need to roll chromium_rev in WebKit/chromium/DEPS to at least 92315.

[Kent Tamura]

Cc fishd for Chromium WebKit API change.

[Darin Fisher (:fishd, Google)]

Comment on attachment 100635 [details]
Patch V1 (attemps to fix build failure)

View in context: https://bugs.webkit.org/attachment.cgi?id=100635&action=review

> Source/WebKit/chromium/public/WebPlugin.h:60
> +    virtual bool getFormValue(WebString*) { return false; }

I think this method is not very self-explanatory.  What is a form value in the context of a plugin?  When I think of HTML forms, I expect there to be a key name associated with a value, yet this function does not take a key name parameter.

Other question:  Do you really need the bool return?  Can you perhaps use WebString::isNull() to determine if the plugin has no form value?

[Kenichi Ishibashi]

Comment on attachment 100635 [details]
Patch V1 (attemps to fix build failure)

View in context: https://bugs.webkit.org/attachment.cgi?id=100635&action=review

Darin,

Thank you for taking a look for this.

>> Source/WebKit/chromium/public/WebPlugin.h:60
>> +    virtual bool getFormValue(WebString*) { return false; }
> 
> I think this method is not very self-explanatory.  What is a form value in the context of a plugin?  When I think of HTML forms, I expect there to be a key name associated with a value, yet this function does not take a key name parameter.
> 
> Other question:  Do you really need the bool return?  Can you perhaps use WebString::isNull() to determine if the plugin has no form value?

The meaning of the form value of a plugin is specified in the spec (http://www.w3.org/TR/html5/association-of-controls-and-forms.html#constructing-form-data-set) and the key name to be associated with is the value of the 'name' attribute of the corresponding object element. This is why this function does not take a key name parameter.  I didn't come up with an appropriate function name other than it..

The spec says "try to obtain a form submission value from the plugin, and if that is successful, append an entry to the form data set with name as the name".  By returning boolean value, I just want to recognize whether obtaining form value is failed or succeeded but the value is empty.  The current implementation doesn't append the entry for the plugin when the function returns false, while it appends the entry even if the value is empty when the function returns true.

[Kenichi Ishibashi]

Created attachment 100753 [details]
Patch V2

[Kent Tamura]

I think the patch is reasonable.  fishd, do you have additional comments?

[Kenichi Ishibashi]

Created attachment 118937 [details]
Patch V3

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Kenichi Ishibashi]

(In reply to comment #12)
> Created an attachment (id=118937) [details]
> Patch V3

Revised the patch. It seems that I also need to add the feature to PPAPI plugins so removed the change in test_expectations.txt for now.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118937 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=118937&action=review

> Source/WebKit/chromium/public/WebPlugin.h:60
> +    // Returns true if the form submission value is successfully obtained

Thanks for adding this comment!  It helps a lot.

nit: please add a new line above this comment since getFormValue and scriptableObject
really aren't all that related.

> Source/WebKit/chromium/public/WebPlugin.h:63
> +    virtual bool getFormValue(WebString*) { return false; }

how about passing WebString& here instead?  the WebKit API tries to follow WebKit
coding conventions, which call for a reference here instead of a pointer.

[Kenichi Ishibashi]

Comment on attachment 118937 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=118937&action=review

Hi Darin,

Thank you for review!

>> Source/WebKit/chromium/public/WebPlugin.h:60
>> +    // Returns true if the form submission value is successfully obtained
> 
> Thanks for adding this comment!  It helps a lot.
> 
> nit: please add a new line above this comment since getFormValue and scriptableObject
> really aren't all that related.

Will do.

>> Source/WebKit/chromium/public/WebPlugin.h:63
>> +    virtual bool getFormValue(WebString*) { return false; }
> 
> how about passing WebString& here instead?  the WebKit API tries to follow WebKit
> coding conventions, which call for a reference here instead of a pointer.

I already added Chromium side implementation using a pointer:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/webkit/plugins/npapi/webplugin_impl.h&exact_package=chromium&q=webkit/plugins/npapi/webplugin_impl.h&type=cs&l=72

I'll change Chromium side and then revise the patch.

[Kenichi Ishibashi]

Created attachment 119130 [details]
Patch V4

[Kenichi Ishibashi]

(In reply to comment #17)
> Created an attachment (id=119130) [details]
> Patch V4

Changes in Chromium side is landed.

http://src.chromium.org/viewvc/chrome?view=rev&revision=114338

[WebKit Review Bot]

Comment on attachment 119130 [details]
Patch V4

Clearing flags on attachment: 119130

Committed r102873: <http://trac.webkit.org/changeset/102873>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

Reverted r102873 for reason:

Break the build

Committed r102881: <http://trac.webkit.org/changeset/102881>

[Kenichi Ishibashi]

Created attachment 119723 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119723 [details]
Patch for landing

Clearing flags on attachment: 119723

Committed r103147: <http://trac.webkit.org/changeset/103147>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.