Bug 176725

Created attachment 320555 [details] Patch

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

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.

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

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.

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

Sorry for the delay on this.

Created attachment 329052 [details] Patch

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.

Carlos, ping. I'll update bug #173915 next.

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.

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

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