[webkit-reviews] review granted: [Bug 92358] [EFL][WK2] Add form client for Ewk_View : [Attachment 154617] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 92358: [EFL][WK2] Add form client for Ewk_View
https://bugs.webkit.org/show_bug.cgi?id=92358

Attachment 154617: Patch
https://bugs.webkit.org/attachment.cgi?id=154617&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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?