GNOME Bugzilla – Bug 684439
Implement pre-filled forms in WebKit2
Last modified: 2013-03-14 19:49:37 UTC
We need to figure out a way to implement this, because DOM bindings are not available in WebKit2.
Created attachment 238460 [details] [review] Move find_username_and_password_elements to ephy-web-dom-utils
Created attachment 238461 [details] [review] Move auth data query/store methods from ephy-profile-utils to a new file
Created attachment 238462 [details] [review] Add EphyFormAuthDataCache to ephy-form-auth-data
I'm still working on this, these are some refactorings I've needed to implement pre-filled forms in WebKit2, that are ready to be reviewed.
Created attachment 238526 [details] [review] Patch
(In reply to comment #5) > Created an attachment (id=238526) [details] [review] > Patch Forgot to say that this patch requires new webkit API not landed yet (webkit_web_page_get_uri()), all other patches can be applied.
See https://bugs.webkit.org/show_bug.cgi?id=111938
Review of attachment 238460 [details] [review]: OK with the changes mentioned below. ::: lib/ephy-web-dom-utils.c @@ +360,3 @@ + guint i; + gboolean found_auth_elements = FALSE; + Probably should add a bunch of g_return_val_if_fail... @@ +363,3 @@ + elements = webkit_dom_html_form_element_get_elements (form); + + for (i = 0; i < webkit_dom_html_collection_get_length (elements); i++) { No need to ask for the length every time, it was already done right in the old code you are changing. ::: lib/ephy-web-dom-utils.h @@ +25,3 @@ #define EPHY_WEB_DOM_UTILS_H +#include <webkit/webkitdom.h> For WK1 I'd just include webkit.h as usual, since it already includes this. Not sure if it's allowed or not to include this header independently in WK1, but I think it should not be. For WK2 it's obviously different and this is fine.
Review of attachment 238461 [details] [review]: You need to change POTFILES.in too.
Review of attachment 238460 [details] [review]: ::: lib/ephy-web-dom-utils.c @@ +360,3 @@ + guint i; + gboolean found_auth_elements = FALSE; + OK, I was assuming this is actually private, maybe g_assert would be more appropriate. @@ +363,3 @@ + elements = webkit_dom_html_form_element_get_elements (form); + + for (i = 0; i < webkit_dom_html_collection_get_length (elements); i++) { You are right, I don't know why I changed it, sorry. ::: lib/ephy-web-dom-utils.h @@ +25,3 @@ #define EPHY_WEB_DOM_UTILS_H +#include <webkit/webkitdom.h> The header is common for both wk1 and wk2, since it's the same api, so I think it should work.
Review of attachment 238462 [details] [review]: Cool. ::: lib/ephy-form-auth-data.c @@ +23,3 @@ #include "ephy-form-auth-data.h" +#include "ephy-string.h" Needs an empty line after this, see HACKING.
(In reply to comment #10) > ::: lib/ephy-web-dom-utils.h > @@ +25,3 @@ > #define EPHY_WEB_DOM_UTILS_H > > +#include <webkit/webkitdom.h> > > The header is common for both wk1 and wk2, since it's the same api, so I think > it should work. Actually I was using the wrong header, webkit/webkitdom.h is the old dom header kept for backwards compatibility, this won't work if we eventually add an option to build wk without wk1. the right header that should work for both wk1 and wk2 is webkitdom/webkitdom.h
webkit_web_page_get_id() landed in the stable branch
Created attachment 238684 [details] [review] Updated patch Updated to apply on current git master
Review of attachment 238684 [details] [review]: This seems to work great in general. I pushed a couple of patches that could be pushed independently of this, but have a few questions left. ::: embed/ephy-embed-shell.c @@ +116,3 @@ + g_dbus_connection_signal_unsubscribe (g_dbus_proxy_get_connection (priv->web_extension), + priv->web_extension_form_auth_save_signal_id); + priv->web_extension_form_auth_save_signal_id = 0; Pushed this to master already. ::: embed/ephy-web-view.c @@ -585,3 @@ - /* We are no longer showing a store password infobar */ - web_view->priv->password_info_bar = NULL; Why is this not needed anymore? Was it wrong in the first place? ::: embed/web-extension/ephy-web-extension.c @@ +100,3 @@ + WebKitDOMNode *username_node; + WebKitDOMNode *password_node; +} EphyFormAuthNodesData; It really does like this should just be a separate GObject. Reimplementing ref counting is kinda weird. @@ +511,1 @@ web_page = get_webkit_web_page_or_return_dbus_error (invocation, web_extension, page_id); Pushed the style fixes to master already. @@ +573,3 @@ + } else { + dbus_connection = connection; + g_object_add_weak_pointer (G_OBJECT (connection), (gpointer *)&dbus_connection); What is this about? Seems unrelated to the patch? ::: lib/ephy-form-auth-data.c @@ +55,3 @@ soup_uri_set_scheme (uri, SOUP_URI_SCHEME_HTTP); + soup_uri_set_query (uri, NULL); I pushed this to master already.
(In reply to comment #15) > Review of attachment 238684 [details] [review]: > > This seems to work great in general. I pushed a couple of patches that could be > pushed independently of this, but have a few questions left. Thanks! > ::: embed/ephy-web-view.c > @@ -585,3 @@ > > - /* We are no longer showing a store password infobar */ > - web_view->priv->password_info_bar = NULL; > > Why is this not needed anymore? Was it wrong in the first place? This is automatically done by a weak pointer now, so that we don't need to keep the ephy_embed just for that. > ::: embed/web-extension/ephy-web-extension.c > @@ +100,3 @@ > + WebKitDOMNode *username_node; > + WebKitDOMNode *password_node; > +} EphyFormAuthNodesData; > > It really does like this should just be a separate GObject. Reimplementing ref > counting is kinda weird. This is just a small struct to pass around some async functions, the refcounting just makes it easier to use and avoids having to copy the struct at some point. It's pretty common to do this and the refcounting implementation is very straightforward. > > @@ +573,3 @@ > + } else { > + dbus_connection = connection; > + g_object_add_weak_pointer (G_OBJECT (connection), (gpointer > *)&dbus_connection); > > What is this about? Seems unrelated to the patch? No it's not, we need the connection to emit the FormAuthDataSaveConfirmationRequired signal.
Created attachment 238759 [details] [review] Updated patch Updated to apply on current git master. I guess it can be split a bit more, I can do it if you want.
(In reply to comment #16) > > It really does like this should just be a separate GObject. Reimplementing ref > > counting is kinda weird. > > This is just a small struct to pass around some async functions, the > refcounting just makes it easier to use and avoids having to copy the struct at > some point. It's pretty common to do this and the refcounting implementation is > very straightforward. It might straightforward but I honestly don't see the point at all. Moving this to a separate GObject in its own file should take 5 minutes and it will make the code cleaner in this file. I mean, ref counting + fields with data = GObject, IMHO. I can do it if you are busy, no problem.
(In reply to comment #18) > (In reply to comment #16) > > > It really does like this should just be a separate GObject. Reimplementing ref > > > counting is kinda weird. > > > > This is just a small struct to pass around some async functions, the > > refcounting just makes it easier to use and avoids having to copy the struct at > > some point. It's pretty common to do this and the refcounting implementation is > > very straightforward. > > It might straightforward but I honestly don't see the point at all. Moving this > to a separate GObject in its own file should take 5 minutes and it will make > the code cleaner in this file. I mean, ref counting + fields with data = > GObject, IMHO. I can do it if you are busy, no problem. No no, it's not a matter of being busy, it's just that we think differently, because I don't see the point of making it a gobject :-P, using a gobject just to have refcounting looks a bit overkill to me, but if you think it's better I'll turn it into a gobject. Just one question, why moving it its own file? The struct is private to that file and not supposed to be used by other files.
(In reply to comment #19) > No no, it's not a matter of being busy, it's just that we think differently, > because I don't see the point of making it a gobject :-P, using a gobject just > to have refcounting looks a bit overkill to me, but if you think it's better > I'll turn it into a gobject. Just one question, why moving it its own file? The > struct is private to that file and not supposed to be used by other files. I'd say it still makes sense to put a separate class into a separate file, as we usually do. And that file is growing way too big already, so I think this will help. Thanks!
Created attachment 238846 [details] [review] Do not use ephy-auth-data in ephy-embed-single for WebKit2
Created attachment 238847 [details] [review] Move the code to crate the remember passwords info bar widget to its own function
Created attachment 238848 [details] [review] Patch This new patch requires a change in wk that raised when moving the form auth data to its own file. I'll let you know when the fix lands in wk, the other two patches can be applied, though.
Review of attachment 238846 [details] [review]: OK.
Review of attachment 238847 [details] [review]: Looks good.
Review of attachment 238848 [details] [review]: This looks generally OK, and last time I tested it it worked great. I think the thing about strcmp0 is a real bug, but other than that looks good to go IMHO. ::: embed/web-extension/ephy-embed-form-auth.c @@ +66,3 @@ +{ + EphyEmbedFormAuth *form_auth = EPHY_EMBED_FORM_AUTH (g_object_new (EPHY_TYPE_EMBED_FORM_AUTH, NULL)); + Probably worth adding type checks here. DOM objects are the kind of thing where it's easy to pass the wrong data type. ::: embed/web-extension/ephy-web-extension.c @@ +105,3 @@ + g_direct_equal, + NULL, + (GDestroyNotify)g_object_unref); This hash table is never freed. I guess at least add a FIXME about it, since there does not seem to be any good place to do it. @@ +309,3 @@ + + retval = g_strcmp0 (username_field_name, form_data->form_username) + + g_strcmp0 (password_field_name, form_data->form_password); I think this will do the wrong thing if one comparison returns -1 and the other +1.
Review of attachment 238848 [details] [review]: ::: embed/web-extension/ephy-embed-form-auth.c @@ +66,3 @@ +{ + EphyEmbedFormAuth *form_auth = EPHY_EMBED_FORM_AUTH (g_object_new (EPHY_TYPE_EMBED_FORM_AUTH, NULL)); + I still consider this private, but Ok. ::: embed/web-extension/ephy-web-extension.c @@ +105,3 @@ + g_direct_equal, + NULL, + (GDestroyNotify)g_object_unref); Requests are removed from the hash table when finished, but the hash table itself is not freed. My plan is to add a singleton gobject that can be freed using atexit or whatever in the future. Same happens with the URI tester and the form auth data cache object. @@ +309,3 @@ + + retval = g_strcmp0 (username_field_name, form_data->form_username) + + g_strcmp0 (password_field_name, form_data->form_password); Ooops, good point :-P
Pushed a fixed version to git master, thanks!