GNOME Bugzilla – Bug 742680
Port to WebKit 2
Last modified: 2017-08-08 10:45:28 UTC
We'll have to do this sooner or later, but now that we are getting crashes in the Foursquare mobile authentication page using WebKit 1, I think it becomes a more urgent task.
Created attachment 294190 [details] [review] Don't include webkit.h explicitly in providers
Created attachment 294191 [details] [review] provider: Deprecate preseed-data property Required for WebKit2 port, whose API doesn't allow to inject cookies manually so this property becomes useless.
Created attachment 294192 [details] [review] webview: Remove unused "status" private field
Created attachment 294193 [details] [review] [WIP] Port to WebKit 2 ---- WORK IN PROGRESS!! This is an early progress of the WebKit 2 port, we have to face how to enable DOM parsing again, which is currently disabled. There are also some minor nits for example that the network stack (soup) logger is no longer configured and the side effect of the preseed-data deprecation. Anyway, I think we can start a review of what we have.
(In reply to comment #2) > Created an attachment (id=294191) [details] [review] > provider: Deprecate preseed-data property > > Required for WebKit2 port, whose API doesn't allow to inject cookies > manually so this property becomes useless. I don't know what this feature is for, but if you are using persistent cookies, you could create a SoupCookieJar for the existing database file and add the cookies manually. I don't know if that might cause any concurrency problems, though.
Review of attachment 294193 [details] [review]: ::: src/goabackend/goaoauth2provider.c @@ +761,3 @@ +{ + GoaOAuth2Provider *provider = user_data; + provider->priv->load_failed = TRUE; You should return something here. If you return FALSE a default error page will be loaded, otherwise nothing happens. Where is this set to TRUE when a new load starts? @@ +766,2 @@ static void +on_web_view_load_chaged (WebKitWebView *web_view, WebKitLoadEvent *load_event, gpointer user_data) This is not correct, document-loaded is not equivalent to load-changed. This should be moved to a WebExtension where you can use WebKitWebPage::document-loaded and the DOM bindings API. @@ +779,3 @@ gulong i; + // Find a way to replace webkit_web_view_get_dom_document You don't need to replace anything, the exactly same API is available from the WebExtensions API. However, you might consider using other alternatives like user scripts, javascript and user script messages. @@ +1038,2 @@ webkit_web_view_load_uri (WEBKIT_WEB_VIEW (embed), url); + provider->priv->load_failed = FALSE; I see it's here. The proper place would probably be in load-changed when the event is load-started. ::: src/goabackend/goaoauth2provider.h @@ +27,3 @@ #include <goabackend/goaprovider-priv.h> +#include <webkit2/webkit2.h> +#include <webkitdom/webkitdom.h> The WebKitDOM API should never be included in the UI process. ::: src/goabackend/goawebview.c @@ -286,3 @@ - cookie_jar = soup_cookie_jar_new (); - soup_session_add_feature (session, SOUP_SESSION_FEATURE (cookie_jar)); - g_object_unref (cookie_jar); So, a session cookie jar is used? We could use a persistent cookie jar but temporary, created here and removed on dispose. That way we could keep the feature that requires to manually add cookies. @@ -291,3 @@ - soup_logger_set_printer (logger, web_view_log_printer, NULL, NULL); - soup_session_add_feature (session, SOUP_SESSION_FEATURE (logger)); - g_object_unref (logger); hmm, we don't have an alternative for this in the current API. What was this used for? Just for debugging? @@ +208,1 @@ scrolled_window = gtk_scrolled_window_new (NULL, NULL); The WebKitWebView in WebKit2 is scrollable, it shouldn't be added to a GtkScrolledWindow because it doesn't implement the GtkScrollable interface. @@ -305,3 @@ - settings = webkit_web_view_get_settings (WEBKIT_WEB_VIEW (priv->web_view)); - g_object_set (settings, "user-stylesheet-uri", "file://" PACKAGE_DATA_DIR "/goawebview.css", NULL); You can use WebKitUserContentManager to add custom stylesheets.
(In reply to comment #5) > (In reply to comment #2) > > Created an attachment (id=294191) [details] [review] [details] [review] > > provider: Deprecate preseed-data property > > > > Required for WebKit2 port, whose API doesn't allow to inject cookies > > manually so this property becomes useless. > > I don't know what this feature is for, but if you are using persistent cookies, > you could create a SoupCookieJar for the existing database file and add the > cookies manually. I don't know if that might cause any concurrency problems, > though. The idea behind this was to allow 'stealing' cookies from the user's browser. Imagine that you are already logged into Facebook and you want to add your FB account in GOA. In that case you could add the browser's cookies to GOA's web view and avoid having to log in again. This feature is currently unused. In the worst case we can drop it, but would be nice to keep it if possible.
Review of attachment 294193 [details] [review]: ::: configure.ac @@ +95,3 @@ +], [ + WEBKIT_GTK_PKGCONFIG=webkit2gtk-3.0 +]) Do we really need to do this? webkit2gtk-4.0 has been around for a couple of releases now, so we can simply require it.
Review of attachment 294191 [details] [review]: After some discussion in #webkitgtk+, we decided to retain this API by using a SQLite backed cookie jar with WebKitCookieManager.
Created attachment 297770 [details] [review] Port to WebKit 2 I updated the patch based on the earlier feedback.
Review of attachment 294193 [details] [review]: ::: src/goabackend/goawebview.c @@ -291,3 @@ - soup_logger_set_printer (logger, web_view_log_printer, NULL, NULL); - soup_session_add_feature (session, SOUP_SESSION_FEATURE (logger)); - soup_session_remove_feature_by_type (session, SOUP_TYPE_COOKIE_JAR); Yes, I think we don't do any particular use of the logger, I was trying to figure out how to setup the logger in WebKit2 but I couldn't get access to webkit_get_default_session (maybe the soup session is now part of the WebKitWebProcess) @@ -305,3 @@ - settings = webkit_web_view_get_settings (WEBKIT_WEB_VIEW (priv->web_view)); - g_object_set (settings, "user-stylesheet-uri", "file://" PACKAGE_DATA_DIR "/goawebview.css", NULL); The stylesheets was only used to hide WebKit 1 scrollbars and use Gtk+ ones instead, I think is no longer needed.
Created attachment 297981 [details] [review] Remove provider's DOM parsing related features This features need to be moved to a WebKit extension as part of the WebKit 2 port.
Created attachment 297982 [details] [review] Port to WebKit 2 --- - Delete cookies on goa_webview_init - Create directory for cookies jar - Set size request for web view again
Review of attachment 294192 [details] [review]: This should just be a part of the main "Port to WebKit2" patch.
Review of attachment 297982 [details] [review]: Thanks for filling in the little details. ::: src/goabackend/goawebview.c @@ +203,3 @@ + cookie_manager = webkit_web_context_get_cookie_manager (context); + jar_file = g_build_filename (g_get_user_cache_dir (), "goa-1.0", "cookies.sqlite", NULL); + g_mkdir_with_parents (g_path_get_dirname (jar_file), 0777); jar_file is the path to the SQLite file, not the directory. So we will have to do it in two steps. First create the path to the directory and the directory itself, then create the path to the file. Regarding the mode, I would prefer to use either 0755 or 0700. 0755 is what we already use to create ~/.config/goa-1.0, and 0700 is the most common mode for directories inside ~/.config, ~/.cache and ~/.local/share on my system. I am slightly leaning towards 0700 at the moment. :) @@ +206,3 @@ + priv->cookie_jar = soup_cookie_jar_db_new (jar_file, FALSE); + webkit_cookie_manager_set_persistent_storage (cookie_manager, jar_file, WEBKIT_COOKIE_PERSISTENT_STORAGE_SQLITE); + webkit_cookie_manager_delete_all_cookies (cookie_manager); Correct.
Damián, do you want me to create an initial skeleton for the web extensions? You can then fill it in. I would like to get this into 3.15.91, so let me know if you need a hand.
FYI: the latest WIP code is in the g-o-a:wip/wk2 branch.
Created attachment 298319 [details] [review] pocket: Add missing includes
Created attachment 298320 [details] [review] oauth, oauth2: Fix the WebKit includes
Created attachment 298321 [details] [review] live, pocket: Remove redundant includes
Created attachment 298322 [details] [review] build: Move things around to accommodate the new web extension
Created attachment 298323 [details] [review] Port to WebKit 2 This is still incomplete.
Review of attachment 298323 [details] [review]: I haven't looked at the patch in detail, but at a first glance I have some comments. ::: src/goabackend/goaoauth2provider.c @@ +804,3 @@ + return; + +#if 0 /* TODO: DOM Parsing */ Isn't this in the extension now? what's missing here? ::: src/goabackend/goawebextension.c @@ +22,3 @@ +#include "goaoauthprovider.h" +#include "goaoauth2provider.h" +#include "goaprovider.h" So, this provider objects are now shared between ui and web process? @@ +174,3 @@ + G_PARAM_WRITABLE | + G_PARAM_CONSTRUCT_ONLY | + G_PARAM_STATIC_STRINGS)); Don't you need to install the other two properties here? ::: src/goabackend/goawebextensionmain.c @@ +55,3 @@ + + the_extension = goa_web_extension_new (wk_extension, provider_type, existing_identity); + g_bus_own_name (G_BUS_TYPE_SESSION, Why do you need yo own a DBus name if you are not exposing any object? or do you plan to add some dbus API for the extension? ::: src/goabackend/goawebview.c @@ +216,3 @@ + g_free (jar_file); + + priv->web_view = webkit_web_view_new (); You need to use webkit_web_view_new_with_context to use the context you have just created. Wouldn't it make sense to make GoaWebView a derived class of WebKitWebView?
(In reply to Carlos Garcia Campos from comment #23) > Review of attachment 298323 [details] [review] [review]: > > I haven't looked at the patch in detail, but at a first glance I have some > comments. Thanks for stopping by, Carlos. TLDR: the patch is still a WIP and most of your comments are already fixed on my local machine and/or the goa:wip/wk2 branch. But it is still good to have someone sanity check it. :) > ::: src/goabackend/goaoauth2provider.c > @@ +804,3 @@ > + return; > + > +#if 0 /* TODO: DOM Parsing */ > > Isn't this in the extension now? what's missing here? Nothing. That function needs to be deleted. > ::: src/goabackend/goawebextension.c > @@ +22,3 @@ > +#include "goaoauthprovider.h" > +#include "goaoauth2provider.h" > +#include "goaprovider.h" > > So, this provider objects are now shared between ui and web process? Yes, because some bits of the DOM parsing logic is plugged in by the providers. > @@ +174,3 @@ > + G_PARAM_WRITABLE | > + > G_PARAM_CONSTRUCT_ONLY | > + > G_PARAM_STATIC_STRINGS)); > > Don't you need to install the other two properties here? Yes. > ::: src/goabackend/goawebextensionmain.c > @@ +55,3 @@ > + > + the_extension = goa_web_extension_new (wk_extension, provider_type, > existing_identity); > + g_bus_own_name (G_BUS_TYPE_SESSION, > > Why do you need yo own a DBus name if you are not exposing any object? or do > you plan to add some dbus API for the extension? We need some DBus API to pass the "click" and "submit" signals to the UI process. > ::: src/goabackend/goawebview.c > @@ +216,3 @@ > + g_free (jar_file); > + > + priv->web_view = webkit_web_view_new (); > > You need to use webkit_web_view_new_with_context to use the context you have > just created. Yes. It was a typo introduced by my attempts to see if I can avoid using a non-default context. > Wouldn't it make sense to make GoaWebView a derived class of > WebKitWebView? It is a sub-class of GtkOverlay. The WebView is added to it with the progress bar and floating URL bar overlaid on top.
Created attachment 298476 [details] Port to WebKit 2
(In reply to Debarshi Ray from comment #24) > (In reply to Carlos Garcia Campos from comment #23) > > Review of attachment 298323 [details] [review] [review] [review]: > > > > I haven't looked at the patch in detail, but at a first glance I have some > > comments. > > Thanks for stopping by, Carlos. > > TLDR: the patch is still a WIP and most of your comments are already fixed > on my local machine and/or the goa:wip/wk2 branch. But it is still good to > have someone sanity check it. :) > Ah, ok. > > ::: src/goabackend/goawebextensionmain.c > > @@ +55,3 @@ > > + > > + the_extension = goa_web_extension_new (wk_extension, provider_type, > > existing_identity); > > + g_bus_own_name (G_BUS_TYPE_SESSION, > > > > Why do you need yo own a DBus name if you are not exposing any object? or do > > you plan to add some dbus API for the extension? > > We need some DBus API to pass the "click" and "submit" signals to the UI > process. If the communication is always from the web extension to the UI process, you could use user script messages API, and you avoid all the DBus :-)
Created attachment 298561 [details] [review] Port to WebKit 2
(In reply to Carlos Garcia Campos from comment #26) > If the communication is always from the web extension to the UI process, you > could use user script messages API, and you avoid all the DBus :-) Yes, that was enough. Thanks for the suggestion.
Created attachment 298572 [details] [review] Port to WebKit 2 This is now as complete as it can be. I did see some CRITICALs when denying adding a Windows Live account. Something about the cookie manager not being valid. Doesn't happen for Google, for example. Could be a WebKitGTK+ bug, could be ours. Needs more investigation, but I don't want to block on that because we are running late for 3.15.91. WebKit2 suffers from the same bugs as WebKit1. We still can't latch on to the Flickr's 'deny node', and can not lock down Windows Live's username field. So, nothing to worry about for this port. I saw that Pocket's is_deny_node check doesn't work. A dialog with '... Forbidden' written on it gets thrown at the user. Something to fix, but not related to this port, either. Finally, please note that this depends on the following fixes: https://bugs.webkit.org/show_bug.cgi?id=142165 https://bugs.webkit.org/show_bug.cgi?id=142225 https://bugs.webkit.org/show_bug.cgi?id=142278 (if you have a new enough GCC)
Comment on attachment 298572 [details] [review] Port to WebKit 2 Damián, Carlos and Michael, thanks for all your help and effort on this. I am committing this and making the release. Let me know if there are regressions or other issues.
Review of attachment 298572 [details] [review]: For the sake of leaving a paper trail: ::: src/goabackend/goawebview.c @@ -282,3 @@ - - soup_session_add_feature_by_type (session, SOUP_TYPE_PROXY_RESOLVER_DEFAULT); - g_object_set (session, "accept-language-auto", TRUE, NULL); This should have been ported to webkit_web_context_set_preferred_languages. See bug 758716.