Bug 92358 – [EFL][WK2] Add form client for Ewk_View

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

We need to define a form client and attach it to the Ewk_View. This way, we can emit a signal on the view when a form is about to get submitted and provide information to the browser about the form and its data.
This can be used by the browser to store information (e.g. login information) that could be used to pre-fill the form later.

[Chris Dumez]

Created attachment 154611 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 154611 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.h:56
> + * When the reference count reaches 0, the intent is freed.

Intent??? Also shouldnt you say thta it will be submitted if not already.

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.h:99
> +EAPI Eina_Bool ewk_form_submission_request_submit(Ewk_Form_Submission_Request *request);

hmm here you could read it as a "request submit" instead of submit a submission_request. I dont have any better ideas though

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:948
> + * Emits signal: "form,request,new" with pointer to Ewk_Form_Submission_Request.

form,request,submit ? isn't that not more obvious? or form,submission,new?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:33
> + *   It is possible to handle the form submission request asynchronously, by simply calling

it is possible to intercept...?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:37
> + *   ewk_form_submission_request_submit() will be called.

called automatically?

[Chris Dumez]

Comment on attachment 154611 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:948
>> + * Emits signal: "form,request,new" with pointer to Ewk_Form_Submission_Request.
> 
> form,request,submit ? isn't that not more obvious? or form,submission,new?

Hmm, how about "form,submission,request" ?

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:33
>> + *   It is possible to handle the form submission request asynchronously, by simply calling
> 
> it is possible to intercept...?

No, there is no such method in the C API, we can only continue the submission, not cancel it.

[Chris Dumez]

Created attachment 154617 [details]
Patch

Take Kenneth feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 154617 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:49
> +    bool handledRequest;

bool wasHandled ? or maybe just bool submitted;

handledRequest sounds a bit weird, it is more like did handle request or request was handled

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:64
> +        // Make sure the request is always handled before destroying.
> +        if (!handledRequest)
> +            WKFormSubmissionListenerContinue(wkListener.get());

As this is more like a "already done, break" if sentence I would write it the other way around

if (handled)
   return

...

This is generally a good way because it allows for less errors when making refactorings

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
> + *   The Ewk_Form_Submission_Request passed contains information about the text fields of the form. This

What about other forms such as checkboxes, etc?

> Source/WebKit2/UIProcess/API/efl/ewk_view_form_client.cpp:46
> +    memset(&formClient, 0, sizeof(WKPageFormClient));

Any reason to null it if you are filling out all the values? Just to be safe for future extensions?

[Chris Dumez]

Comment on attachment 154617 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:49
>> +    bool handledRequest;
> 
> bool wasHandled ? or maybe just bool submitted;
> 
> handledRequest sounds a bit weird, it is more like did handle request or request was handled

Ok.

>> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:64
>> +            WKFormSubmissionListenerContinue(wkListener.get());
> 
> As this is more like a "already done, break" if sentence I would write it the other way around
> 
> if (handled)
>    return
> 
> ...
> 
> This is generally a good way because it allows for less errors when making refactorings

Ok.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:30
>> + *   The Ewk_Form_Submission_Request passed contains information about the text fields of the form. This
> 
> What about other forms such as checkboxes, etc?

No, only test fields values are passed.

>> Source/WebKit2/UIProcess/API/efl/ewk_view_form_client.cpp:46
>> +    memset(&formClient, 0, sizeof(WKPageFormClient));
> 
> Any reason to null it if you are filling out all the values? Just to be safe for future extensions?

Yes, for possible future extensions: this is safer.

[WebKit Review Bot]

Comment on attachment 154617 [details]
Patch

Clearing flags on attachment: 154617

Committed r123742: <http://trac.webkit.org/changeset/123742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.