Bug 77520 - [WK2] Modify parameters of willSubmitForm callback
Summary: [WK2] Modify parameters of willSubmitForm callback
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-01 01:09 PST by Grzegorz Czajkowski
Modified: 2014-02-05 10:52 PST (History)
22 users (show)

See Also:


Attachments
proposed patch (15.75 KB, patch)
2012-02-01 01:10 PST, Grzegorz Czajkowski
abarth: review-
Details | Formatted Diff | Diff
updated patch according to Adam's review (15.43 KB, patch)
2012-02-02 04:16 PST, Grzegorz Czajkowski
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
rebased patch to HEAD (15.40 KB, patch)
2012-02-02 05:23 PST, Grzegorz Czajkowski
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
updated patch (15.55 KB, patch)
2012-04-13 02:17 PDT, Grzegorz Czajkowski
sam: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
moved to WKBundle (31.02 KB, patch)
2012-05-18 06:31 PDT, Grzegorz Czajkowski
andersca: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
fix client interface size for given version (31.07 KB, patch)
2012-05-21 06:13 PDT, Grzegorz Czajkowski
buildbot: commit-queue-
Details | Formatted Diff | Diff
add missing symbols to WebCore.order (32.50 KB, patch)
2012-07-19 06:43 PDT, Grzegorz Czajkowski
buildbot: commit-queue-
Details | Formatted Diff | Diff
add missing symbols to WebCore.exp.in (32.14 KB, patch)
2012-08-16 06:36 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2012-02-01 01:09:22 PST
There is useful callback WKPageWillSubmitFormCallback to notify an application when submit button was clicked.
This callback provides through its parameters values of input fields by WKDictionaryRef type where key is a name of input field and value is a typed text to input field.

I see three disadvantages of it:
1.  Input fields information is keep in hashmap, so they  are not keep in order as they occur on page. Application can’t determine order of input fields based on passed hashmap.
2. Type of input fields are not passed (like password, text), we have only its name.
3. HashMap for given key returns typed text to an input field not a default one. Input field's value attribute is reserved for default value of it. It may confuse a developer.

I wouldn't limit input fields information to theirs names and typed text only.
What do you think about passing WKArrayRef<WKDictionary> instead of WKDictionaryRef where each item of array will contain all attributes of input fields as key-value?
Additionally we can add form attributes as WKDictionary to the callback to identify form from submit button was clicked.

I am aware that this patch changes API of WK2 but anyway I'd be glad to hear your opinion about it.
If anyone still wants to use previous values of input fields there is possibility to receive them from injectedBundleFormClient.
Comment 1 Grzegorz Czajkowski 2012-02-01 01:10:35 PST
Created attachment 124902 [details]
proposed patch
Comment 2 Grzegorz Czajkowski 2012-02-01 02:04:12 PST
CC'ing developers involved in HTML Forms and WebKit2.
Comment 3 Adam Barth 2012-02-01 02:14:42 PST
Comment on attachment 124902 [details]
proposed patch

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

This patch doesn't have any tests.

> Source/WebCore/loader/FormState.cpp:40
> +static String currentValueKey = "currentValue";

This creates a static initializer and will break the AppleMac build.

> Source/WebCore/loader/FormState.cpp:55
> +const Vector<HashMap<String, String> > FormState::textFieldsPropertis() const

Why are you returning const here?  textFieldsPropertis is misspelled.

> Source/WebCore/loader/FormState.cpp:58
> +    for (size_t i = 0; i < m_form->associatedElements().size(); ++i) {

Is calling associatedElements() expensive?
Comment 4 Adam Barth 2012-02-01 02:15:13 PST
Sorry for the drive-by review.  You'll need someone with WebKit2 expertise to actually review your patch.
Comment 5 Grzegorz Czajkowski 2012-02-02 04:16:40 PST
Created attachment 125109 [details]
updated patch according to Adam's review
Comment 6 Grzegorz Czajkowski 2012-02-02 04:35:12 PST
(In reply to comment #3)
> (From update of attachment 124902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124902&action=review
> 
> This patch doesn't have any tests.

I suppose that you meant unit tests. Is there any mechanism for test WK2 API?

> 
> > Source/WebCore/loader/FormState.cpp:40
> > +static String currentValueKey = "currentValue";
> 
> This creates a static initializer and will break the AppleMac build.

Fixed - removed static and moved initialization to function.

> 
> > Source/WebCore/loader/FormState.cpp:55
> > +const Vector<HashMap<String, String> > FormState::textFieldsPropertis() const
> 
> Why are you returning const here?  textFieldsPropertis is misspelled.

Yes, you're right it is useless here. I removed it. 
Fixed typo.

> 
> > Source/WebCore/loader/FormState.cpp:58
> > +    for (size_t i = 0; i < m_form->associatedElements().size(); ++i) {
> 
> Is calling associatedElements() expensive?

It depends on size of vector/elements number on page. A lot of WebCore methods obtain this object directly (without assignee to auxiliary variable). Can we leave as it is?
Comment 7 Early Warning System Bot 2012-02-02 04:37:10 PST
Comment on attachment 125109 [details]
updated patch according to Adam's review

Attachment 125109 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11396789
Comment 8 Gyuyoung Kim 2012-02-02 04:41:36 PST
Comment on attachment 125109 [details]
updated patch according to Adam's review

Attachment 125109 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11394777
Comment 9 WebKit Review Bot 2012-02-02 05:06:14 PST
Comment on attachment 125109 [details]
updated patch according to Adam's review

Attachment 125109 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11394789
Comment 10 Grzegorz Czajkowski 2012-02-02 05:23:52 PST
Created attachment 125121 [details]
rebased patch to HEAD
Comment 11 Early Warning System Bot 2012-03-07 02:10:59 PST
Comment on attachment 125121 [details]
rebased patch to HEAD

Attachment 125121 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11836857
Comment 12 Grzegorz Czajkowski 2012-04-13 02:17:30 PDT
Created attachment 137062 [details]
updated patch

Fix build break by applying the latest changes from NamedNodeMap class https://bugs.webkit.org/show_bug.cgi?id=75069
Comment 13 Build Bot 2012-04-13 03:02:33 PDT
Comment on attachment 137062 [details]
updated patch

Attachment 137062 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12396630
Comment 14 Sam Weinig 2012-04-15 16:19:11 PDT
Comment on attachment 137062 [details]
updated patch

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

This seems like a bad idea, and too expensive for the general case.  The bundle can be used to annotate the callback with this data if it necessary.

> Source/WebKit2/UIProcess/API/C/WKPage.h:140
> +typedef void (*WKPageWillSubmitFormCallback)(WKPageRef page, WKFrameRef frame, WKFrameRef sourceFrame, WKArrayRef textFieldsProperties, WKDictionaryRef formAttributes, WKTypeRef userData, WKFormSubmissionListenerRef listener, const void* clientInfo);

This will break existing clients.  You can;t just change delegates like this, you need to version it.
Comment 15 Grzegorz Czajkowski 2012-04-16 06:11:39 PDT
(In reply to comment #14)
> (From update of attachment 137062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137062&action=review
> 
> This seems like a bad idea, and too expensive for the general case.  The bundle can be used to annotate the callback with this data if it necessary.

First of all thanks for your review.
Do you mean that expanded version of willSubmitForm callback should be moved to WKBundlePage.h and current API of WKPage.h shouldn't be changed?

Isn't it a misunderstanding to keep in the fact two different callbacks?
I agree that this callback doesn't require any sync between UIProcess and WebProcess. Is it the main reason of moving it to WKBundlePage.h ?

> 
> > Source/WebKit2/UIProcess/API/C/WKPage.h:140
> > +typedef void (*WKPageWillSubmitFormCallback)(WKPageRef page, WKFrameRef frame, WKFrameRef sourceFrame, WKArrayRef textFieldsProperties, WKDictionaryRef formAttributes, WKTypeRef userData, WKFormSubmissionListenerRef listener, const void* clientInfo);
> 
> This will break existing clients.  You can;t just change delegates like this, you need to version it.

Make sense, thanks. It will probably fix build break on mac port.
Comment 16 Grzegorz Czajkowski 2012-05-18 06:31:55 PDT
Created attachment 142700 [details]
moved to WKBundle

This patch moves extended version of willSubmitForm from WKPage to WKBundle.
A new API version has been introduced.
Added unit test to webkit-gtk.
Comment 17 Build Bot 2012-05-18 06:50:55 PDT
Comment on attachment 142700 [details]
moved to WKBundle

Attachment 142700 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12719849
Comment 18 Anders Carlsson 2012-05-18 11:18:04 PDT
Comment on attachment 142700 [details]
moved to WKBundle

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

> Source/WebKit2/Shared/APIClientTraits.cpp:68
> -    offsetof(WKBundlePageFormClient, willSendSubmitEvent),
> +    offsetof(WKBundlePageFormClient, willSubmitForm),
>      sizeof(WKBundlePageFormClient)
>  };

This part is wrong. You can't remove the version 0 offset and replace it. Instead, you need to add a new version with the right offset.
Comment 19 Grzegorz Czajkowski 2012-05-21 06:13:09 PDT
Created attachment 143016 [details]
fix client interface size for given version
Comment 20 Grzegorz Czajkowski 2012-05-21 06:20:04 PDT
(In reply to comment #18)
> (From update of attachment 142700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142700&action=review
> 
> > Source/WebKit2/Shared/APIClientTraits.cpp:68
> > -    offsetof(WKBundlePageFormClient, willSendSubmitEvent),
> > +    offsetof(WKBundlePageFormClient, willSubmitForm),
> >      sizeof(WKBundlePageFormClient)
> >  };
> 
> This part is wrong. You can't remove the version 0 offset and replace it. Instead, you need to add a new version with the right offset.

Thanks, fixed.
Comment 21 Build Bot 2012-05-21 06:29:23 PDT
Comment on attachment 143016 [details]
fix client interface size for given version 

Attachment 143016 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12731598
Comment 22 Grzegorz Czajkowski 2012-05-21 07:47:35 PDT
(In reply to comment #21)
> (From update of attachment 143016 [details])
> Attachment 143016 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12731598

Undefined symbols for architecture x86_64:
  "__ZNK7WebCore9FormState20textFieldsPropertiesEv",
  "__ZNK7WebCore9FormState14formAttributesEv", 
ld: symbol(s) not found for architecture x86_64

It looks like symbols of newly added methods to FormState class are missed. Where should I add them to do not break mac build?
Comment 23 Chris Dumez 2012-06-07 05:07:46 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 143016 [details] [details])
> > Attachment 143016 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/12731598
> 
> Undefined symbols for architecture x86_64:
>   "__ZNK7WebCore9FormState20textFieldsPropertiesEv",
>   "__ZNK7WebCore9FormState14formAttributesEv", 
> ld: symbol(s) not found for architecture x86_64
> 
> It looks like symbols of newly added methods to FormState class are missed. Where should I add them to do not break mac build?

I believe you should add those to Source/WebCore/WebCore.order
Comment 24 Grzegorz Czajkowski 2012-07-19 06:43:30 PDT
Created attachment 153247 [details]
add missing symbols to WebCore.order

1) Add missing symbols to WebCore.order
2) Add WillSubmitFormCallback unit test to WebKit-Efl and WebKit-Gtk builds.
Comment 25 Build Bot 2012-07-19 06:57:26 PDT
Comment on attachment 153247 [details]
add missing symbols to WebCore.order

Attachment 153247 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13277864
Comment 26 Thiago Marcos P. Santos 2012-08-07 08:12:21 PDT
(In reply to comment #25)
> (From update of attachment 153247 [details])
> Attachment 153247 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13277864

This mail thread here might help you:

http://lists.macosforge.org/pipermail/webkit-dev/2012-March/019938.html
Comment 27 Grzegorz Czajkowski 2012-08-16 06:36:56 PDT
Created attachment 158800 [details]
add missing symbols to WebCore.exp.in
Comment 28 Kenneth Rohde Christiansen 2012-08-16 06:53:35 PDT
Comment on attachment 158800 [details]
add missing symbols to WebCore.exp.in

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

> Source/WebKit2/ChangeLog:15
> +        Modifies parameters of willSubmitForm of WKBundle API by changing WKDictionaryRef
> +        type to WKArrayRef<WKDictionary> to allow passing all input fields' properties,
> +        not only list of pairs: input field's name and typed text.
> +        They can be obtained in the sam order as they occurr on page.
> +        Each item of array contains all attributes of input fields for example,
> +        type=text or type=password as key-value hash map.
> +        Additionally form's atrributes are added to willSubmitForm callback to
> +        associate input fields with form.

You are not really making it clear why you need access to these?
Comment 29 Grzegorz Czajkowski 2012-08-16 07:04:20 PDT
Comment on attachment 158800 [details]
add missing symbols to WebCore.exp.in

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

>> Source/WebKit2/ChangeLog:15
>> +        associate input fields with form.
> 
> You are not really making it clear why you need access to these?

I mentioned about it in the description of the bug by enumerating disadvantages of current API:
1. Application can’t determine order of input fields based on passed hashmap.
2. Type of input fields are not passed (like password, text), we have only its name.
3. HashMap for the key 'value' returns typed text to an input field not a default one. It may confuse a developer.

Do you think that those details should be added to ChangeLog too? Thanks
Comment 30 Thiago Marcos P. Santos 2012-08-16 07:34:45 PDT
(In reply to comment #29)
> (From update of attachment 158800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158800&action=review
> 
> >> Source/WebKit2/ChangeLog:15
> >> +        associate input fields with form.
> > 
> > You are not really making it clear why you need access to these?
> 
> I mentioned about it in the description of the bug by enumerating disadvantages of current API:
> 1. Application can’t determine order of input fields based on passed hashmap.
> 2. Type of input fields are not passed (like password, text), we have only its name.
> 3. HashMap for the key 'value' returns typed text to an input field not a default one. It may confuse a developer.
> 
> Do you think that those details should be added to ChangeLog too? Thanks

The use case would be for example the browser offer to save password, right?
Comment 31 Grzegorz Czajkowski 2012-08-16 07:55:20 PDT
(In reply to comment #30)
> The use case would be for example the browser offer to save password, right?

Yes, it might be. With this patch application is able to grab all data from the form on which the submit button was clicked.

Anyway thanks for the hint how to fix Mac build - as you can see it works!:)
Comment 32 Grzegorz Czajkowski 2012-11-19 07:16:12 PST
Hi,
Any feedback on this? Thanks.
Comment 33 Andreas Kling 2014-02-05 10:52:31 PST
Comment on attachment 158800 [details]
add missing symbols to WebCore.exp.in

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.