Summary: | WebFrameLoaderClient::dispatchWillSendSubmitEvent() needs to be implemented for WebKit2 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Honeycutt <jhoneycutt> | ||||||||||||
Component: | WebKit2 | Assignee: | Jon Honeycutt <jhoneycutt> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, beidson, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Jon Honeycutt
2012-04-18 17:18:37 PDT
Created attachment 138223 [details]
Patch
Comment on attachment 138223 [details]
Patch
As with all new API additions, this should have an API test in TestWebKitAPI.
Created attachment 138699 [details]
Patch with test
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? (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? 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. 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) Created attachment 138889 [details]
Patch v3
(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. 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.
Created attachment 139899 [details]
Patch v4
Patch that should apply to ToT.
Ah, it's failing because this patch relies on another patch. Created attachment 139945 [details]
Patch v5
Nudge the EWS bots.
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.
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). (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>. |