GNOME Bugzilla – Bug 684448
Delete web application button doesn't work in WebKit2
Last modified: 2013-01-11 09:09:17 UTC
Because it requires DOM bindings that are not available in WebKit2. We could use a link, instead of a form, like chromium does for some prefs pages, and use the policy client API to catch the request, get the app id from the URL, delete the web app, ignore the request and start a new one for about:applications.
Created attachment 224942 [details] [review] Don't use DOM bindings to delete a web application from about:applications Use a different form for every web application with a hidden value containing the application id. Then use the policy client to ignore any form submission from about:applications and delete the application instead, reloading the about:applications page. This solution will work for WebKit2 too.
Created attachment 224943 [details] [review] Patch
Created attachment 229034 [details] [review] Update: Don't use DOM bindings to delete a web application from about:applications Include ephy-web-app-utils.h in ephy-window.c
Review of attachment 229034 [details] [review]: This looks good. The previous patch is obsolete, right? ::: embed/ephy-about-handler.c @@ +235,3 @@ } + g_string_append (data_str, "</table></body>"); This is because a single form would submit every Delete button, right?
Thank you, bugzilla.
Since we are going to have DOM bindings in WK2 I wonder if we shouldn't just fix the FIXME for WK2 and be done with this. I very much prefer the current approach to hidden forms and page reloads, fwiw.
(In reply to comment #8) > Since we are going to have DOM bindings in WK2 I wonder if we shouldn't just > fix the FIXME for WK2 and be done with this. I very much prefer the current > approach to hidden forms and page reloads, fwiw. Just for the sake of clarification after talking with Carlos: - The page already has a form. It's there just for the sake of forcing the page to load again, but no processing is done in the form itself. We hook into the buttons to know which app to delete using dom bindings, and we could easily just update the DOM there to remove that row. - So one option is to leave the code as it is and just implement the WK2 version using the WK2 flavor of the DOM bindings. - Another option is to do what this patch proposes, which moves processing of the app to delete to the form logic and gets rid of the DOM bindings usage. - And yet a third option is to even improve the code to not have forms at all (and no reloads), and update the page with the bindings. Then we'd do the same in WK2 too. My ideal scenario is the third one, then the first, and then the second, but I don't feel very strongly about this.
After another chat with Carlos we did agree on reworking the patch to save the extra load it introduces (so we'd be down to just the current load through the form), but we'll stop using the bindings to simplify things for the time being.
Created attachment 233076 [details] [review] Update: Don't use DOM bindings to delete a web application from about:applications Use policy use instead of ignore + load
Created attachment 233077 [details] [review] Patch Same here for WebKit2
Review of attachment 233076 [details] [review]: Looks OK, just those two comments. ::: src/ephy-window.c @@ +2476,3 @@ + + if (!uri->query) + return; This leaks the URI. @@ +2761,3 @@ + if (reason == WEBKIT_WEB_NAVIGATION_REASON_FORM_SUBMITTED && + g_str_has_prefix (webkit_web_view_get_uri (web_view), "ephy-about:applications")) My previous comment about this not being NULL safe still applies, I think.
Review of attachment 233077 [details] [review]: ::: src/ephy-window.c @@ +2635,2 @@ + if (navigation_type == WEBKIT_NAVIGATION_TYPE_FORM_SUBMITTED && + g_str_has_prefix (webkit_web_view_get_uri (web_view), "ephy-about:applications")) Just the same comment here.
(In reply to comment #13) > Review of attachment 233076 [details] [review]: > > Looks OK, just those two comments. > > ::: src/ephy-window.c > @@ +2476,3 @@ > + > + if (!uri->query) > + return; > > This leaks the URI. Good catch. I'll remove the early return. > @@ +2761,3 @@ > > + if (reason == WEBKIT_WEB_NAVIGATION_REASON_FORM_SUBMITTED && > + g_str_has_prefix (webkit_web_view_get_uri (web_view), > "ephy-about:applications")) > > My previous comment about this not being NULL safe still applies, I think. I don't know in WebKit1, but in WebKit2 you are guaranteed to have a URI when the policy signal is emitted. In any case, a NULL check doesn't hurt here.