Bug 84304 – WebFrameLoaderClient::dispatchWillSendSubmitEvent() needs to be implemented for WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Jon Honeycutt]

WebFrameLoaderClient::dispatchWillSendSubmitEvent() needs to be implemented for WebKit2.

[Jon Honeycutt]

Created attachment 138223 [details]
Patch

[Sam Weinig]

Comment on attachment 138223 [details]
Patch

As with all new API additions, this should have an API test in TestWebKitAPI.

[Jon Honeycutt]

Created attachment 138699 [details]
Patch with test

[Darin Adler]

Comment on attachment 138699 [details]
Patch with test

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:743
> +    WebFrame* sourceFrame = static_cast<WebFrameLoaderClient*>(formState->sourceDocument()->frame()->loader()->client())->webFrame();

Can sourceDocument()->frame() be 0?

[Jon Honeycutt]

(In reply to comment #4)
> (From update of attachment 138699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138699&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:743
> > +    WebFrame* sourceFrame = static_cast<WebFrameLoaderClient*>(formState->sourceDocument()->frame()->loader()->client())->webFrame();
> 
> Can sourceDocument()->frame() be 0?

I don't know, but I copied this line from WebFrameLoaderClient::dispatchWillSubmitForm(). In what cases would a document's frame be 0?

[Alexey Proskuryakov]

A document created with document.implementation.createDocument, XMLHttpRequest.responseXML, or XSLTProcessor will have a null frame. I don't know if form.submit() will work in WebKit trunk in such document, but nothing obvious prevents it from working conceptually.

[Jessie Berlin]

Comment on attachment 138223 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:288
> +    WKBundlePageWillSendSubmitEventCallback                             willSendSubmitEvent;

This will break the nighties. You need to mark this as being in version 1 and set the kWKBundlePageFormClientCurrentVersion to 1 as well (see https://trac.webkit.org/changeset/90162)

[Jon Honeycutt]

Created attachment 138889 [details]
Patch v3

[Jon Honeycutt]

(In reply to comment #6)
> A document created with document.implementation.createDocument, XMLHttpRequest.responseXML, or XSLTProcessor will have a null frame. I don't know if form.submit() will work in WebKit trunk in such document, but nothing obvious prevents it from working conceptually.

HTMLFormElement null checks the frame and doesn't dispatch these events if the frame is null.

[Darin Adler]

Comment on attachment 138889 [details]
Patch v3

Your patch is not applying, and because of that, the EWS servers aren’t testing it. You should figure out why and fix that.

[Jon Honeycutt]

Created attachment 139899 [details]
Patch v4

Patch that should apply to ToT.

[Jon Honeycutt]

Ah, it's failing because this patch relies on another patch.

[Jon Honeycutt]

Created attachment 139945 [details]
Patch v5

Nudge the EWS bots.

[WebKit Review Bot]

Attachment 139945 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Tools/TestWebKitAPI/Tests/WebKit2/WillSendSubmitEvent_Bundle.cpp:38:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/WillSendSubmitEvent_Bundle.cpp:38:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jessie Berlin]

Comment on attachment 139945 [details]
Patch v5

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

r=me

> Tools/TestWebKitAPI/Tests/WebKit2/WillSendSubmitEvent_Bundle.cpp:45
> +    WKBundlePostMessage(InjectedBundleController::shared().bundle(), Util::toWK("DidReceiveWillSendSubmitEvent").get(), values);

You should probably check the values here as well.

> Tools/TestWebKitAPI/Tests/WebKit2/auto-submitting-form.html:7
> +                    return;

Why is this window.location.search check and early return necessary?

> Tools/TestWebKitAPI/Tests/WebKit2/auto-submitting-form.html:15
> +            <input type="hidden" name="hiddenField" value="hidden field">

It might be nice to add more fields + values here so that you can check they are being reported correctly through the API (see previous comment).

[Jon Honeycutt]

(In reply to comment #15)
> (From update of attachment 139945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139945&action=review
> 
> r=me
> 
> > Tools/TestWebKitAPI/Tests/WebKit2/WillSendSubmitEvent_Bundle.cpp:45
> > +    WKBundlePostMessage(InjectedBundleController::shared().bundle(), Util::toWK("DidReceiveWillSendSubmitEvent").get(), values);
> 
> You should probably check the values here as well.

The values are checked in the UI process.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/auto-submitting-form.html:7
> > +                    return;
> 
> Why is this window.location.search check and early return necessary?

The form is submitted back to itself, which I figured would trigger another onload event. I was trying to prevent an infinite loop of this, though I'm not sure it's necessary.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/auto-submitting-form.html:15
> > +            <input type="hidden" name="hiddenField" value="hidden field">
> 
> It might be nice to add more fields + values here so that you can check they are being reported correctly through the API (see previous comment).

OK, I added a password field. Only text field values are passed, so there's not a lot of value in other input types.

Thanks for the review. Landed in <http://trac.webkit.org/changeset/116016>.