Bug 176725 – [WPE][GTK] Deprecate and replace webkit_form_submission_request_get_text_fields

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

We should deprecate and replace webkit_form_submission_request_get_text_fields(). It's broken because it returns a GHashTable*, but the underlying C++ type is Vector<std::pair<String, String>>. The same "key" can appear multiple times in the form. This may not be very useful, but it is legal. Let's replace it with two different functions that each return an array of strings, webkit_form_submission_request_get_input_names() and webkit_form_submission_request_get_input_values().

[Michael Catanzaro]

Created attachment 320555 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

I'm not really sure we should be doing this. There is a flip side to this. The hash table API is probably just a better choice overall. It's not as if a form that has multiple elements with the same name is going to be able to do anything useful, so ensuring that we get every possible element for such forms is probably not very important.

[Michael Catanzaro]

I wound up deciding it's best to go ahead and expose the correct data types and deprecate the old version.

[Carlos Garcia Campos]

Comment on attachment 320555 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:82
>          request->priv->values = adoptGRef(g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free));

Maybe we could create the hash table on demand only when the deprecated method is called.

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:101
> + * function does not reliably return all text fields.

Use webkit_form_submission_request_get_text_field_names() and webkit_form_submission_request_get_text_field_values() instead.

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:122
> + * non-%NULL empty strings. This function is probably only useful if used

probably?

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:127
> + * Returns: (allow-none) (array zero-terminated=1) (element-type utf8) (transfer none):
> + *    A null-terminated list of text field names, or %NULL if the form
> + *    doesn't contain any text fields.

Why not returning GPtrArray for consistency with the web extensions API? That way we could include NULL values instead of allocating empty strings.

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:145
> + * Get a list with the values of the text fields contained in the form
> + * associated to @request. Text fields without values will be returned
> + * as non-%NULL empty strings. This function is probably only useful if
> + * used in combination with webkit_form_submission_request_get_text_field_names().

Maybe we can add a single function, returning a gboolean that indicates if the forms contains text fields or not, and two optional GPtrArray out parameters.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #5)
> Maybe we could create the hash table on demand only when the deprecated
> method is called.

OK.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:101
> > + * function does not reliably return all text fields.
> 
> Use webkit_form_submission_request_get_text_field_names() and
> webkit_form_submission_request_get_text_field_values() instead.

That's already in the Deprecated section... you want me to write it twice?

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:122
> > + * non-%NULL empty strings. This function is probably only useful if used
> 
> probably?

I'll reword it.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:127
> > + * Returns: (allow-none) (array zero-terminated=1) (element-type utf8) (transfer none):
> > + *    A null-terminated list of text field names, or %NULL if the form
> > + *    doesn't contain any text fields.
> 
> Why not returning GPtrArray for consistency with the web extensions API?
> That way we could include NULL values instead of allocating empty strings.

Good idea. I was trying to avoid GPtrArray just because I don't like it very much, but it's the right type to use here and using empty strings is awkward.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:145
> > + * Get a list with the values of the text fields contained in the form
> > + * associated to @request. Text fields without values will be returned
> > + * as non-%NULL empty strings. This function is probably only useful if
> > + * used in combination with webkit_form_submission_request_get_text_field_names().
> 
> Maybe we can add a single function, returning a gboolean that indicates if
> the forms contains text fields or not, and two optional GPtrArray out
> parameters.

OK.

[Michael Catanzaro]

Sorry for the delay on this.

[Michael Catanzaro]

Created attachment 329052 [details]
Patch

[Michael Catanzaro]

Comment on attachment 329052 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitFormSubmissionRequest.h:66
> +webkit_form_submission_request_list_text_fields (WebKitFormSubmissionRequest  *request,

Note: "list" here is used as a verb. Naming the function was difficult, since the perfect name, webkit_form_submission_request_get_text_fields(), was already taken. I also considered webkit_form_submission_request_get_text_field_lists(), which seemed awkward. My runner-up name is webkit_form_submission_request_get_text_fields_as_lists(), which is long.

[Michael Catanzaro]

Carlos, ping.

I'll update bug #173915 next.

[Carlos Garcia Campos]

Comment on attachment 329052 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:82
> +            g_ptr_array_add(request->priv->textFieldNames.get(), static_cast<gpointer>(g_strdup(values[i].first.utf8().data())));
> +            g_ptr_array_add(request->priv->textFieldValues.get(), static_cast<gpointer>(g_strdup(values[i].second.utf8().data())));

I don't think we need to cast here

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:98
> + * Returns: (allow-none) (transfer none): a #GHashTable with the form

Why did you add allow-none? We are always returning a valid GHashTable, no?

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:99
> + *    text fields, or %NULL if the form doesn't contain text fields.

We don't return NULL anymore, this could be considered an API change. Don't we have a unit tests for this particular case?

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:107
> +    if (!request->priv->values) {

To keep compatibility you should check here  && request->priv->textFieldNames->len

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:109
> +        ASSERT(request->priv->textFieldNames->len == request->priv->textFieldValues->len);

I don't think this is needed, the arrays are created internally with the same len value.

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:135
> + * Returns: %TRUE if the form contains text fields

, or %FALSE otherwise

> Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:148
> +    ASSERT(request->priv->textFieldNames->len == request->priv->textFieldValues->len);

I don't think this is needed, the arrays are created internally with the same len value.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:82
> > +            g_ptr_array_add(request->priv->textFieldNames.get(), static_cast<gpointer>(g_strdup(values[i].first.utf8().data())));
> > +            g_ptr_array_add(request->priv->textFieldValues.get(), static_cast<gpointer>(g_strdup(values[i].second.utf8().data())));
> 
> I don't think we need to cast here

You're right... I did not know this!

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:98
> > + * Returns: (allow-none) (transfer none): a #GHashTable with the form
> 
> Why did you add allow-none? We are always returning a valid GHashTable, no?

It was missing before. This is an API break, unfortunately, but the original annotation was plainly incorrect.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:99
> > + *    text fields, or %NULL if the form doesn't contain text fields.
> 
> We don't return NULL anymore, this could be considered an API change. Don't
> we have a unit tests for this particular case?

You're right, my code is broken. We must be missing a test for this, indeed.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:107
> > +    if (!request->priv->values) {
> 
> To keep compatibility you should check here  &&
> request->priv->textFieldNames->len

Yes.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:109
> > +        ASSERT(request->priv->textFieldNames->len == request->priv->textFieldValues->len);
> 
> I don't think this is needed, the arrays are created internally with the
> same len value.

As you prefer.

> > Source/WebKit/UIProcess/API/glib/WebKitFormSubmissionRequest.cpp:135
> > + * Returns: %TRUE if the form contains text fields
> 
> , or %FALSE otherwise

Meh, OK... I don't personally like this style, because it's obvious that the function returns FALSE when it does not return TRUE.

[Michael Catanzaro]

Committed r226264: <https://trac.webkit.org/changeset/226264>