Summary: | [WK2] Modify parameters of willSubmitForm callback | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||||
Status: | ASSIGNED --- | ||||||||||||||||||||
Severity: | Enhancement | CC: | adele, andersca, ap, cdumez, cgarcia, darin, dglazkov, gyuyoung.kim, gyuyoung.kim, hepaajan, japhet, jhoneycutt, jochen, jonlee, js45.yang, kling, laszlo.gombos, rakuco, sam, sullivan, tmpsantos, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Grzegorz Czajkowski
2012-02-01 01:09:22 PST
Created attachment 124902 [details]
proposed patch
CC'ing developers involved in HTML Forms and WebKit2. 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? Sorry for the drive-by review. You'll need someone with WebKit2 expertise to actually review your patch. Created attachment 125109 [details]
updated patch according to Adam's review
(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 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 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 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 Created attachment 125121 [details]
rebased patch to HEAD
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 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 on attachment 137062 [details] updated patch Attachment 137062 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12396630 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. (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. 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 on attachment 142700 [details] moved to WKBundle Attachment 142700 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12719849 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. Created attachment 143016 [details]
fix client interface size for given version
(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 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 (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? (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 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 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 (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 Created attachment 158800 [details]
add missing symbols to WebCore.exp.in
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 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 (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? (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!:) Hi, Any feedback on this? Thanks. 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.
|