GNOME Bugzilla – Bug 600987
Save link as / Save image need EphyEmbedPersist to work
Last modified: 2010-02-25 23:02:19 UTC
In Epiphany 2.29.1, right clicking images, or links, and using the different options to save or download the objects no longer seems to work.
Confirmed regression.
*** Bug 606101 has been marked as a duplicate of this bug. ***
Should investigate about naming in bug #572819 after fixing this one.
Created attachment 151369 [details] [review] Implement EphyEmbedPersist using WebKitDownload EphyEmbedPersist is the object in charge of most context menu options. It handles "Save target as", "Open Image", "Save image as", among other actions. This reimplements it using WebKitDownload. Bug #600987
Created attachment 151370 [details] [review] tests: add a test for EphyEmbedPersist Bug #600987
Review of attachment 151369 [details] [review]: ::: embed/ephy-embed-persist.c @@ +7,1 @@ * Can't be both ;) @@ +480,1 @@ This is not needed, private data is zeroed already. @@ +496,3 @@ + + g_object_unref (priv->download); + if (priv->download) This should happen in dispose, not finalize. @@ +643,3 @@ + else + webkit_download_cancel (persist->priv->download); + if (persist->priv->download) I don't quite get why the signal is only emitted if there's no download going on, care to explain? Also, the G_OBJECT cast for the signal emission is not needed. @@ +691,3 @@ + { + g_signal_emit_by_name (G_OBJECT (persist), "cancelled"); + } I guess you should unref the persist at about this point... @@ +717,1 @@ g_object_ref (persist); I think a comment saying where this is balanced by another unref would be good. @@ +719,3 @@ + if (priv->source == NULL) return FALSE; + + priv = persist->priv; Will the persist ever be unreffed in this case? @@ +730,3 @@ + if (priv->source == NULL) return FALSE; + + priv = persist->priv; Why do you connect to progress if you only check the status? @@ +779,1 @@ } Just return FALSE, not need for an } else {
Created attachment 151481 [details] [review] Implement EphyEmbedPersist using WebKitDownload EphyEmbedPersist is the object in charge of most context menu options. It handles "Save target as", "Open Image", "Save image as", among other actions. This reimplements it using WebKitDownload. Bug #600987
Created attachment 151482 [details] [review] tests: add a test for EphyEmbedPersist Bug #600987
Created attachment 151494 [details] [review] Implement EphyEmbedPersist using WebKitDownload EphyEmbedPersist is the object in charge of most context menu options. It handles "Save target as", "Open Image", "Save image as", among other actions. This reimplements it using WebKitDownload. Bug #600987
Created attachment 151495 [details] [review] tests: add a test for EphyEmbedPersist Bug #600987
Created attachment 151496 [details] [review] ephy-embed-persist: remove unused flags Bug #600987
Created attachment 151500 [details] [review] tests: add a test for EphyEmbedPersist Bug #600987 This addresses Xan's comments regarding style in main()
Created attachment 151502 [details] [review] tests: apply style fixes suggested by Xan Bug #600987 Trivial style fix based on the same suggestions as the embed-persist test.
Created attachment 151970 [details] [review] Implement EphyEmbedPersist using WebKitDownload EphyEmbedPersist is the object in charge of most context menu options. It handles "Save target as", "Open Image", "Save image as", among other actions. This reimplements it using WebKitDownload. Bug #600987
Created attachment 151971 [details] [review] tests: add test for EphyEmbedPersist Bug #600987 Cleaner version of the previous tests patch. I applied the misc simplification bits from the previous one to master and the style fixes. The test itself for EphyEmbedPersist is still here.
Comment on attachment 151970 [details] [review] Implement EphyEmbedPersist using WebKitDownload Argh. git bz exploded.
Comment on attachment 151970 [details] [review] Implement EphyEmbedPersist using WebKitDownload >diff --git a/embed/downloader-view.c b/embed/downloader-view.c >index 59653c5..e0dac07 100644 >--- a/embed/downloader-view.c >+++ b/embed/downloader-view.c >@@ -493,7 +493,6 @@ update_download_row (DownloaderView *dv, WebKitDownload *download) > open_downloaded_file_location (dv, download); > break; > default: >- g_assert_not_reached (); What. > break; > } > About the rest of the patch: the fact that it automatically adds stuff to the downloader view is completely wrong. This is supposed to be a low-level class to download things, it should not mess with any UI other than the filechooser when the flag for that is set. It was even used to download things like favicons before, we certaintly don't want the download view to show up for that! Please re-do it without that.
Comment on attachment 151971 [details] [review] tests: add test for EphyEmbedPersist Can we use SoupServer to make the tests work without hitting the net? You can check how Gustavo did recently the same thing to the WebKit tests. Also, when you remove the DV from EphyEmbedPersist you can stop including/using libnotify I guess.
Created attachment 152288 [details] [review] Implement EphyEmbedPersist using WebKitDownload EphyEmbedPersist is the object in charge of most context menu options. It handles "Save target as", "Open Image", "Save image as", among other actions. This reimplements it using WebKitDownload. Bug #600987
Created attachment 152289 [details] [review] tests: add test for EphyEmbedPersist Bug #600987
Sorry for the status spam, git bz keeps going boom. This new version of the patch doesn't include the downloader-view part as discussed on jabber, it also adds two small fixes, WEBKIT_DOWNLOAD_STATUS_ERROR should fire the cancelled signal and public functions should check if the persist argument is a EphyEmbedPersist with a g_return_if_fail. The test uses SoupServer now, and is a bit better.
Comment on attachment 152288 [details] [review] Implement EphyEmbedPersist using WebKitDownload > static void >+ephy_embed_persist_dispose (GObject *object) >+{ >+ EphyEmbedPersist *persist = EPHY_EMBED_PERSIST (object); >+ EphyEmbedPersistPrivate *priv = persist->priv; >+ >+ if (priv->download) >+ g_object_unref (priv->download); dispose can be called more than once, so set the object to NULL after the unref. >+ >+ LOG ("EphyEmbedPersist disposed %p", object); >+ >+ G_OBJECT_CLASS (ephy_embed_persist_parent_class)->dispose (object); >+} >+ > void > ephy_embed_persist_cancel (EphyEmbedPersist *persist) > { >- g_object_unref (persist); >+ g_return_if_fail (EPHY_IS_EMBED_PERSIST (persist)); >+ >+ /* webkit_download_cancel() triggers download_status_changed_cb() with >+ * status = WEBKIT_DOWNLOAD_STATUS_CANCELLED so we don't need to emit >+ * the signal. >+ */ >+ if (persist->priv->download) >+ { >+ webkit_download_cancel (persist->priv->download); >+ } >+ else >+ { >+ g_signal_emit_by_name (persist, "cancelled"); >+ g_object_unref (persist); OK, so... isn't it an error to call cancel if there's no download going on? What's the use of it? >+ } >+} >+ >+static void >+response_cb (GtkDialog *dialog, >+ int response_id, >+ WebKitDownload *download) >+{ >+ if (response_id == GTK_RESPONSE_ACCEPT) >+ { >+ char *uri; >+ >+ uri = gtk_file_chooser_get_uri (GTK_FILE_CHOOSER(dialog)); >+ >+ webkit_download_set_destination_uri (download, uri); >+ webkit_download_start (download); >+ >+ g_free (uri); >+ } >+ else >+ { >+ webkit_download_cancel (download); Is it ok to cancel a download that never started? Is it needed? Do you do it only for the side effects? >+ } >+ >+ gtk_widget_destroy (GTK_WIDGET (dialog)); >+} >+ >+static void >+download_status_changed_cb (GObject *object, >+ GParamSpec *pspec, >+ EphyEmbedPersist *persist) >+{ >+ EphyEmbedPersistPrivate *priv; >+ WebKitDownload *download; >+ WebKitDownloadStatus status; >+ >+ priv = persist->priv; >+ download = WEBKIT_DOWNLOAD (object); >+ status = webkit_download_get_status (download); >+ >+ if (status == WEBKIT_DOWNLOAD_STATUS_FINISHED) >+ { >+ g_signal_emit_by_name (persist, "completed"); >+ g_object_unref (persist); >+ } >+ else if (status == WEBKIT_DOWNLOAD_STATUS_CANCELLED || >+ status == WEBKIT_DOWNLOAD_STATUS_ERROR) >+ { >+ g_signal_emit_by_name (persist, "cancelled"); >+ g_object_unref (persist); >+ } You can unref at the end of the function. > } > > /** >@@ -639,7 +723,10 @@ ephy_embed_persist_cancel (EphyEmbedPersist *persist) > * Begins saving the file specified in @persist. > * > * If @persist's #EphyEmbedPersistFlags include %EPHY_EMBED_PERSIST_ASK_DESTINATION, a >- * filechooser dialog will be shown first. >+ * filechooser dialog will be shown first. If this flag is not set and no >+ * destination has been set, the target will be saved to the default download >+ * directory using the suggested name, if no suggested name can be get the >+ * download will fail. > * > * The file will continue to download in the background until either the > * ::completed or the ::cancelled signals are emitted by @persist. >@@ -649,8 +736,110 @@ ephy_embed_persist_cancel (EphyEmbedPersist *persist) > gboolean > ephy_embed_persist_save (EphyEmbedPersist *persist) > { >+ EphyEmbedPersistPrivate *priv; >+ WebKitNetworkRequest *request; >+ >+ g_return_val_if_fail (EPHY_IS_EMBED_PERSIST (persist), FALSE); >+ >+ /* Balanced when priv->download is not needed anymore: here, in >+ * ephy_embed_persist_cancel() and in download_status_changed_cb(). >+ */ > g_object_ref (persist); > >+ priv = persist->priv; >+ >+ if (priv->source == NULL) >+ { >+ g_object_unref (persist); >+ return FALSE; >+ } Nitpick, but you can do this check before the ref ... and it should probably emit a warning? >+ >+ request = webkit_network_request_new (priv->source); >+ priv->download = webkit_download_new (request); >+ g_object_unref (request); >+ >+ g_signal_connect (G_OBJECT (priv->download), "notify::status", No need for the G_OBJECT cast. >+ G_CALLBACK (download_status_changed_cb), >+ persist); >+ >+ if (priv->flags & EPHY_EMBED_PERSIST_ASK_DESTINATION) >+ { >+ EphyFileChooser *dialog; >+ GtkWidget *window; >+ const char *suggested_filename; >+ >+ suggested_filename = webkit_download_get_suggested_filename >+ (priv->download); This indentation is a 7.8 in the "Meh" scale :p >+ window = GTK_WIDGET (priv->fc_parent); >+ >+ if (!gtk_widget_is_toplevel (GTK_WIDGET (priv->fc_parent))) >+ window = NULL; This seems weird. Shouldn't it be an error to set a non toplevel widget? Also, it could be NULL, which would print a bunch of warnings being a valid value. >+ >+ dialog = ephy_file_chooser_new (priv->fc_title, fc_title could be not set too. >+ window, >+ GTK_FILE_CHOOSER_ACTION_SAVE, >+ priv->persist_key, Same? >+ EPHY_FILE_FILTER_ALL_SUPPORTED); >+ >+ gtk_file_chooser_set_do_overwrite_confirmation >+ (GTK_FILE_CHOOSER (dialog), TRUE); >+ gtk_file_chooser_set_current_name >+ (GTK_FILE_CHOOSER (dialog), suggested_filename); >+ >+ g_signal_connect (dialog, "response", >+ G_CALLBACK (response_cb), priv->download); >+ >+ gtk_widget_show (GTK_WIDGET (dialog)); >+ >+ return TRUE; >+ } >+ else if (priv->dest) >+ { >+ char *dest_uri; >+ >+ dest_uri = g_filename_to_uri (priv->dest, NULL, NULL); >+ >+ webkit_download_set_destination_uri (priv->download, dest_uri); >+ webkit_download_start (priv->download); >+ >+ g_free (dest_uri); >+ >+ return TRUE; >+ } >+ else >+ { >+ const char *suggested_filename = NULL; >+ char *downloads_dir; >+ char *dest_filename; >+ char *dest_uri; >+ >+ suggested_filename = webkit_download_get_suggested_filename >+ (priv->download); >+ >+ if (suggested_filename == NULL) >+ { >+ g_object_unref (persist); >+ return FALSE; >+ } >+ >+ downloads_dir = ephy_file_get_downloads_dir (); >+ dest_filename = g_build_filename (downloads_dir, >+ suggested_filename, >+ NULL); >+ >+ priv->dest = dest_filename; >+ dest_uri = g_filename_to_uri (dest_filename, NULL, NULL); >+ >+ webkit_download_set_destination_uri (priv->download, dest_uri); >+ webkit_download_start (priv->download); >+ >+ g_free (downloads_dir); >+ g_free (dest_uri); You have the last 5/6 lines duplicated, refactoring opportunity! >+ >+ return TRUE; >+ } >+ >+ g_object_unref (persist); > return FALSE; > } >
Created attachment 152349 [details] [review] popup-commands: missing unref Bug #600987
Created attachment 152350 [details] [review] Implement EphyEmbedPersist using WebKitDownload EphyEmbedPersist is the object in charge of most context menu options. It handles "Save target as", "Open Image", "Save image as", among other actions. This reimplements it using WebKitDownload. Bug #600987
Created attachment 152352 [details] [review] tests: add test for EphyEmbedPersist Bug #600987
persist_key defaults to NULL and ephy_file_chooser_new accepts NULL as persist_key argument.
Comment on attachment 152350 [details] [review] Implement EphyEmbedPersist using WebKitDownload This looks good to me.
Comment on attachment 152352 [details] [review] tests: add test for EphyEmbedPersist >+++ b/tests/testephyembedpersist.c >@@ -0,0 +1,314 @@ >+/* vim: set sw=2 ts=2 sts=2 et: */ Emacs modeline must be added! >+static void >+server_callback (SoupServer *server, >+ SoupMessage *msg, >+ const char *path, >+ GHashTable *query, >+ SoupClientContext *context, >+ gpointer data) >+{ >+ soup_message_set_status (msg, SOUP_STATUS_OK); >+ >+ if (g_str_equal (path, "/cancelled")) >+ soup_message_set_status (msg, SOUP_STATUS_CANT_CONNECT); >+ >+ soup_message_body_append (msg->response_body, SOUP_MEMORY_STATIC, >+ HTML_STRING, strlen (HTML_STRING)); >+ >+ soup_server_unpause_message (server, msg); Mmm, who is pausing this? >+ >+ soup_message_body_complete (msg->response_body); >+} >+ >+typedef struct { >+ GMainLoop *loop; >+ EphyEmbedPersist *embed; >+ char *destination; >+} PersistFixture; >+ >+static void >+static void >+persist_fixture_teardown (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ if (EPHY_IS_EMBED_PERSIST (fixture->embed)) >+ g_object_unref (fixture->embed); >+ >+ g_assert (EPHY_IS_EMBED_PERSIST (fixture->embed) == FALSE); I'm not sure checking the type of now-dead objects is a very healthy thing to do. What do you want to know exactly? Couldn't this go nuts if you just unrefed the last persist? (Since the class would go away I think). >+ >+ g_unlink (fixture->destination); >+ g_free (fixture->destination); >+ >+ g_main_loop_unref (fixture->loop); >+} >+ >+static void >+test_embed_persist_new (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ g_assert (EPHY_IS_EMBED_PERSIST (fixture->embed)); >+} >+ >+static void >+test_embed_persist_set_embed (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ EphyEmbed *orig_value; >+ EphyEmbed *fail_value; >+ EphyEmbed *read_value; >+ >+ orig_value = EPHY_EMBED (g_object_new (EPHY_TYPE_EMBED, NULL)); >+ fail_value = EPHY_EMBED (g_object_new (EPHY_TYPE_EMBED, NULL)); >+ >+ ephy_embed_persist_set_embed (fixture->embed, orig_value); >+ g_object_get (G_OBJECT (fixture->embed), "embed", &read_value, NULL); I think this adds a ref to the object, so you are probably leaking it... >+ >+ g_assert (read_value == orig_value); >+ g_assert (read_value != fail_value); >+} >+ >+static void >+test_embed_persist_save (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ /* Source and dest set, should return TRUE */ >+ g_assert (ephy_embed_persist_save (fixture->embed) == TRUE); >+ >+ /* Otherwise the reference from ephy_embed_persist_save () is never unref'd */ >+ ephy_embed_persist_cancel (fixture->embed); >+} >+ >+static void >+test_embed_persist_cancel (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ ephy_embed_persist_cancel (fixture->embed); >+ >+ g_assert (EPHY_IS_EMBED_PERSIST (fixture->embed) == FALSE); >+} >+ Mmm, this again.
Comment on attachment 152349 [details] [review] popup-commands: missing unref OK.
Attachment 152349 [details] pushed as 306c647 - popup-commands: missing unref Attachment 152350 [details] pushed as be7e6f1 - Implement EphyEmbedPersist using WebKitDownload
Created attachment 152605 [details] [review] tests: add test for EphyEmbedPersist Bug #600987 -- The unpause was an oversight of previous code I was using to kick errors. The checks for EPHY_IS_EMBED_PERSIST though were only to check if the persist was properly dying, I don't think it's critical for the test if there's no safe way to check this.
Comment on attachment 152605 [details] [review] tests: add test for EphyEmbedPersist >From 95bf85c3cffc36ddedcb185858664319d8247aa9 Mon Sep 17 00:00:00 2001 >+static void >+persist_fixture_teardown (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ g_unlink (fixture->destination); >+ g_free (fixture->destination); >+ You are not freeing the persist, and I don't think all tests do end up freeing the object rigth? Like test_new... so it's probably leaking in some cases. >+ g_main_loop_unref (fixture->loop); >+static void >+test_embed_persist_set_embed (PersistFixture *fixture, >+ gconstpointer data) >+{ >+ EphyEmbed *orig_value; >+ EphyEmbed *fail_value; >+ EphyEmbed *read_value; >+ >+ orig_value = EPHY_EMBED (g_object_new (EPHY_TYPE_EMBED, NULL)); >+ fail_value = EPHY_EMBED (g_object_new (EPHY_TYPE_EMBED, NULL)); >+ >+ ephy_embed_persist_set_embed (fixture->embed, orig_value); >+ g_object_get (G_OBJECT (fixture->embed), "embed", &read_value, NULL); >+ >+ g_assert (read_value == orig_value); >+ g_assert (read_value != fail_value); >+ >+ g_object_unref (read_value); fail_value is leaked AFAICT. You can commit when you re-check those two issues.
(In reply to comment #32) > (From update of attachment 152605 [details] [review]) > >From 95bf85c3cffc36ddedcb185858664319d8247aa9 Mon Sep 17 00:00:00 2001 > >+static void > >+persist_fixture_teardown (PersistFixture *fixture, > >+ gconstpointer data) > >+{ > >+ g_unlink (fixture->destination); > >+ g_free (fixture->destination); > >+ > > You are not freeing the persist, and I don't think all tests do end up freeing > the object rigth? Like test_new... so it's probably leaking in some cases. > The only case where this would fail is in test_embed_persist_cancel, because when you cancel a persist, it dies. So I added a fixture->embed = NULL and the corresponding if (!NULL) unref. Sounds reasonable? > >+ > >+ g_assert (read_value == orig_value); > >+ g_assert (read_value != fail_value); > >+ > >+ g_object_unref (read_value); > > fail_value is leaked AFAICT. > Freeing it says I'm trying to unref a floating reference. So I guess it dies by itself?
(In reply to comment #33) > (In reply to comment #32) > > (From update of attachment 152605 [details] [review] [details]) > > >From 95bf85c3cffc36ddedcb185858664319d8247aa9 Mon Sep 17 00:00:00 2001 > > >+static void > > >+persist_fixture_teardown (PersistFixture *fixture, > > >+ gconstpointer data) > > >+{ > > >+ g_unlink (fixture->destination); > > >+ g_free (fixture->destination); > > >+ > > > > You are not freeing the persist, and I don't think all tests do end up freeing > > the object rigth? Like test_new... so it's probably leaking in some cases. > > > > The only case where this would fail is in test_embed_persist_cancel, because > when you cancel a persist, it dies. So I added a fixture->embed = NULL and the > corresponding if (!NULL) unref. Sounds reasonable? You mean that the only case where you don't need to unref is in cancel or...? Anyway, I can look at the updated patch. > > > >+ > > >+ g_assert (read_value == orig_value); > > >+ g_assert (read_value != fail_value); > > >+ > > >+ g_object_unref (read_value); > > > > fail_value is leaked AFAICT. > > > > Freeing it says I'm trying to unref a floating reference. So I guess it dies by > itself? Well, everything dies by itself when the process dies, but the object is being leaked. You need to sink it and then unref for the code to be correct.
(In reply to comment #34) > > > > Freeing it says I'm trying to unref a floating reference. So I guess it dies by > > itself? > > Well, everything dies by itself when the process dies, but the object is being > leaked. You need to sink it and then unref for the code to be correct. But Persist is a GObject, not a GInitiallyUnowned. I don't get how it can be floating.
Created attachment 153816 [details] [review] tests: add test for EphyEmbedPersist Bug #600987
Comment on attachment 152605 [details] [review] tests: add test for EphyEmbedPersist Here's an update, sorry I didn't quite get what we were talking about :-). I confused the persist with the embed, indeed the embed is floating and the persist is not. Duh. Regarding the unref of the persist in teardown: in test_embed_persist_cancel() the call to ephy_embed_persist_cancel() unrefs the persist, hence it is already dead. I'm setting ->embed to NULL so I don't unref twice (since this is the only case where it happens).
Comment on attachment 153816 [details] [review] tests: add test for EphyEmbedPersist woot.
Attachment 153816 [details] pushed as 57b887e - tests: add test for EphyEmbedPersist