Bug 77520 – [WK2] Modify parameters of willSubmitForm callback

• Status: ASSIGNED
• Component: WebKit2

[Grzegorz Czajkowski]

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.

[Grzegorz Czajkowski]

Created attachment 124902 [details]
proposed patch

[Grzegorz Czajkowski]

CC'ing developers involved in HTML Forms and WebKit2.

[Adam Barth]

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?

[Adam Barth]

Sorry for the drive-by review.  You'll need someone with WebKit2 expertise to actually review your patch.

[Grzegorz Czajkowski]

Created attachment 125109 [details]
updated patch according to Adam's review

[Grzegorz Czajkowski]

(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?

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[WebKit Review Bot]

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

[Grzegorz Czajkowski]

Created attachment 125121 [details]
rebased patch to HEAD

[Early Warning System Bot]

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

[Grzegorz Czajkowski]

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

[Build Bot]

Comment on attachment 137062 [details]
updated patch

Attachment 137062 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12396630

[Sam Weinig]

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.

[Grzegorz Czajkowski]

(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.

[Grzegorz Czajkowski]

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.

[Build Bot]

Comment on attachment 142700 [details]
moved to WKBundle

Attachment 142700 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12719849

[Anders Carlsson]

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.

[Grzegorz Czajkowski]

Created attachment 143016 [details]
fix client interface size for given version

[Grzegorz Czajkowski]

(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.

[Build Bot]

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

[Grzegorz Czajkowski]

(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?

[Chris Dumez]

(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

[Grzegorz Czajkowski]

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.

[Build Bot]

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

[Thiago Marcos P. Santos]

(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

[Grzegorz Czajkowski]

Created attachment 158800 [details]
add missing symbols to WebCore.exp.in

[Kenneth Rohde Christiansen]

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?

[Grzegorz Czajkowski]

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

[Thiago Marcos P. Santos]

(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?

[Grzegorz Czajkowski]

(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!:)

[Grzegorz Czajkowski]

Hi,
Any feedback on this? Thanks.

[Andreas Kling]

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.