Bug 45222

Summary: Add ability to send UserData to willSubmitForm
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch mitz: review+

Description Sam Weinig 2010-09-03 18:37:01 PDT
Add ability to send UserData to willSubmitForm
Comment 1 Sam Weinig 2010-09-03 19:39:10 PDT
Created attachment 66581 [details]
Patch
Comment 2 WebKit Review Bot 2010-09-03 19:45:15 PDT
Attachment 66581 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:150:  Extra space between WKBundlePageWillSubmitFormCallback and willSubmitForm  [whitespace/declaration] [3]
WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 mitz 2010-09-03 19:48:02 PDT
Comment on attachment 66581 [details]
Patch

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

> WebKit2/UIProcess/WebPageProxy.h:205
> +    void willSubmitForm(WebFrameProxy* frame, WebFrameProxy* frameSource, Vector<std::pair<WTF::String, WTF::String> >& textFieldValues, APIObject*, uint64_t listenerID);
I think it would be helpful to name the argument here.

> WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFormClient.h:45
> +class APIObject;
>  class WebFrame;
>  class WebPage;
> +class ImmutableDictionary;
Please sort.

r=me
Comment 4 Adam Barth 2010-09-03 19:49:06 PDT
Comment on attachment 66581 [details]
Patch

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

Mostly just curious what this code looked like.  Very pretty.  Some (trivial) comments below.

> WebKit2/UIProcess/WebPageProxy.cpp:581
> -            willSubmitForm(process()->webFrame(frameID), process()->webFrame(sourceFrameID), textFieldValues, listenerID);
> +            willSubmitForm(process()->webFrame(frameID), process()->webFrame(sourceFrameID), textFieldValues, 0/*userData*/, listenerID);
Slightly more beautiful would be a noUserData constant that was just a null pointer.

> WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:513
> +    RefPtr<FormState> formState = prpFormState;
I've never really like this pattern, but I don't see a way around it.

> WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:520
> +    RefPtr<APIObject> userData;
> +    webPage->injectedBundleFormClient().willSubmitForm(webPage, form, m_frame, sourceFrame, values, userData);
I'm confused.  Don't we need to actually allocate a userData object here?  Does willSubmitForm take a RefPtr by reference as an out param?
Comment 5 Sam Weinig 2010-09-03 19:58:16 PDT
(In reply to comment #4)
> (From update of attachment 66581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66581&action=prettypatch
> 
> Mostly just curious what this code looked like.  Very pretty.  Some (trivial) comments below.
> 
> > WebKit2/UIProcess/WebPageProxy.cpp:581
> > -            willSubmitForm(process()->webFrame(frameID), process()->webFrame(sourceFrameID), textFieldValues, listenerID);
> > +            willSubmitForm(process()->webFrame(frameID), process()->webFrame(sourceFrameID), textFieldValues, 0/*userData*/, listenerID);
> Slightly more beautiful would be a noUserData constant that was just a null pointer.

Ok.

> 
> > WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:520
> > +    RefPtr<APIObject> userData;
> > +    webPage->injectedBundleFormClient().willSubmitForm(webPage, form, m_frame, sourceFrame, values, userData);
> I'm confused.  Don't we need to actually allocate a userData object here?  Does willSubmitForm take a RefPtr by reference as an out param?

Yes it does and very intentionally.

Thanks for the review.
Comment 6 Eric Seidel (no email) 2010-10-13 12:27:14 PDT
Attachment 66581 [details] was posted by a committer and has review+, assigning to Sam Weinig for commit.
Comment 7 Sam Weinig 2010-10-28 13:17:59 PDT
Fixed in http://trac.webkit.org/changeset/66789.