After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 674291 - Save As suggests long complicated filename
Save As suggests long complicated filename
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Downloads
git master
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
polish
: 678658 720881 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-04-17 23:20 UTC by Adam Dingle
Modified: 2014-01-13 23:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Rework how downloads are managed (6.88 KB, patch)
2013-07-03 21:21 UTC, Garrett Regier
reviewed Details | Review
Bug 674291 - Use the suggested name for Save As (4.10 KB, patch)
2013-07-03 21:22 UTC, Garrett Regier
needs-work Details | Review
Add filename-suggested signal to EphyDownload to propagate WebKit's decide-destination signal. (1.69 KB, patch)
2013-11-23 00:26 UTC, Daniel Wyatt
none Details | Review
Add ephy_file_move_uri to ephy-file-helpers. (2.07 KB, patch)
2013-11-23 00:27 UTC, Daniel Wyatt
none Details | Review
Fix Save As to use suggested filename (4.36 KB, patch)
2013-11-23 00:28 UTC, Daniel Wyatt
none Details | Review
Add filename-suggested signal to EphyDownload (2.03 KB, patch)
2013-11-30 17:50 UTC, Daniel Wyatt
reviewed Details | Review
Add ephy_file_move_uri to ephy-file-helpers. (2.44 KB, patch)
2013-11-30 17:51 UTC, Daniel Wyatt
reviewed Details | Review
Fix Save As to use suggested filename (4.97 KB, patch)
2013-11-30 17:51 UTC, Daniel Wyatt
reviewed Details | Review
Add filename-suggested signal to EphyDownload (2.08 KB, patch)
2013-12-21 17:58 UTC, William Jon McCann
committed Details | Review
Add ephy_file_move_uri to ephy-file-helpers (2.48 KB, patch)
2013-12-21 17:59 UTC, William Jon McCann
committed Details | Review
Fix Save As to use suggested filename (6.50 KB, patch)
2013-12-21 17:59 UTC, William Jon McCann
none Details | Review
Make temporary downloads hidden files (3.40 KB, patch)
2013-12-21 17:59 UTC, William Jon McCann
reviewed Details | Review
Do not duplicate the destination in EphyDownload (6.14 KB, patch)
2014-01-09 12:33 UTC, Carlos Garcia Campos
committed Details | Review
Simplify save_property_url (3.24 KB, patch)
2014-01-09 12:33 UTC, Carlos Garcia Campos
committed Details | Review
Do not show downloads in the UI until a destination has been created (1.85 KB, patch)
2014-01-09 12:34 UTC, Carlos Garcia Campos
none Details | Review
Use the filename suggested by the server when asking the user (4.57 KB, patch)
2014-01-09 12:35 UTC, Carlos Garcia Campos
none Details | Review
Do not add automatically the downloads to the UI when they are created (14.60 KB, patch)
2014-01-09 14:44 UTC, Carlos Garcia Campos
committed Details | Review
Use the filename suggested by the server when asking the user (5.09 KB, patch)
2014-01-09 14:45 UTC, Carlos Garcia Campos
committed Details | Review

Description Adam Dingle 2012-04-17 23:20:56 UTC
To see the problem:

1. Send yourself a message in Gmail and attach a file foo.txt.
2. In Epiphany, read the message containing the attachment.  It will have links "View" and "Download".
3. Click the Download link. Epiphany will download the file and will name it "foo.txt".
4. Now right click the Download link and choose Save Link As.  Epiphany will open a file chooser dialog.  In that dialog, the proposed filename will look something like this:

?ui=2&ik=a01b8d774a&view=att&th=136c280a34d3496b&attid=0.1&disp=safe&realattid=f_h15jqkke0&zw

Instead, Epiphany should propose the same name "foo.txt" that it used when you downloaded directly in step 3 above.
Comment 1 Adam Dingle 2012-04-18 18:22:04 UTC
This also occurs in Epiphany 3.0.4, so this is not new.
Comment 2 Zan Dobersek 2012-04-21 13:34:22 UTC
Even when querying WebKitDownload for the suggested filename, the name is still incorrect. This being a WebKit problem, it seems WebKitDownload needs to receive a network response to properly set up the suggested filename.
Comment 3 Xan Lopez 2012-08-01 15:56:26 UTC
(In reply to comment #2)
> Even when querying WebKitDownload for the suggested filename, the name is still
> incorrect. This being a WebKit problem, it seems WebKitDownload needs to
> receive a network response to properly set up the suggested filename.

So here's what I think is going on

For 'Save as' the Download API is not used, we are just using the base name for the resource. We need to add a get_suggested_filename API in WebKitNetworkResponse that uses the Content-Disposition data of the already received data. That would fix doing 'Save as' in the menu for an already loaded page (think; attachments in bugzilla). This is in window-commands.c.

For right-click 'Save Link As' we crate a new download and then use the basename of that download as the suggested filename. This is broken. Using the network response API would not help, because there's been none already. So I'm not sure what to do here, my impression is that the initial suggestion is just going to suck in some cases? Maybe we can start downloading it in the background for a second before showing the suggested filename? The code for this is in popup-commands.c
Comment 4 Claudio Saavedra 2012-08-01 17:47:23 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Even when querying WebKitDownload for the suggested filename, the name is still
> > incorrect. This being a WebKit problem, it seems WebKitDownload needs to
> > receive a network response to properly set up the suggested filename.
> 
> So here's what I think is going on
> 
> For 'Save as' the Download API is not used, we are just using the base name for
> the resource. We need to add a get_suggested_filename API in
> WebKitNetworkResponse that uses the Content-Disposition data of the already
> received data. That would fix doing 'Save as' in the menu for an already loaded
> page (think; attachments in bugzilla). This is in window-commands.c.

The patch for this is already in wk bugzilla, waiting to land:

https://bugs.webkit.org/show_bug.cgi?id=92878

I'll cook a patch once it lands.

> 
> For right-click 'Save Link As' we crate a new download and then use the
> basename of that download as the suggested filename. This is broken. Using the
> network response API would not help, because there's been none already. So I'm
> not sure what to do here, my impression is that the initial suggestion is just
> going to suck in some cases? Maybe we can start downloading it in the
> background for a second before showing the suggested filename? The code for
> this is in popup-commands.c

I think this is what other browsers do..
Comment 5 Xan Lopez 2012-08-02 10:09:34 UTC
I pushed http://git.gnome.org/browse/epiphany/commit/?id=8a38c7a9ba0238092edc4e398ca7635bb4392c5f for the 'Save As' case. Leaving this open for the 'Save Link As' bug.
Comment 6 Claudio Saavedra 2012-08-02 10:12:45 UTC
This will break the build with WK2. The API for WK2 has not landed yet and it's slightly different, see https://bugs.webkit.org/show_bug.cgi?id=92967 .

Also, these methods can return NULL, so I'm not sure your patch will handle that case ok.
Comment 7 Xan Lopez 2012-08-02 10:30:12 UTC
(In reply to comment #6)
> This will break the build with WK2. The API for WK2 has not landed yet and it's
> slightly different, see https://bugs.webkit.org/show_bug.cgi?id=92967 .

Ah right, just need to branch for that.

> 
> Also, these methods can return NULL, so I'm not sure your patch will handle
> that case ok.

I suppose we should fallback to the base path in that case. I can do that now.
Comment 8 Adam Dingle 2013-04-21 09:21:13 UTC
This is still not working even in the Save As case.  For example, visit bug 697812 and scroll down to the attachments section near the end of the page.  Right click on the attachment "first log file" and choose Save As.  Epiphany will suggest the filename "attachment.cgi?id=241264".  In this situation Firefox will propose the filename "Xorg.blank.log", which was the attachment's original filename and which is correct in this case.
Comment 9 Garrett Regier 2013-07-03 21:21:15 UTC
Created attachment 248352 [details] [review]
Rework how downloads are managed

In WebKit2 the "download-started" signal is emitted before the
EphyDownload is created and as such the "ephy-download-set"
object data will never be set so a new EphyDownload will be created.
This results in two EphyDownloads being created per WebKitDownload.

Instead set the EphyDownload as object data on the WebKitDownload
and return it if it is set. Also connect to the EphyEmbedShell's
"download-removed" signals and remove the download from the UI
in the callback.
Comment 10 Garrett Regier 2013-07-03 21:22:36 UTC
Created attachment 248353 [details] [review]
Bug 674291 - Use the suggested name for Save As
Comment 11 Adam Dingle 2013-07-10 17:31:46 UTC
Garrett, thanks for the fix!  This bug has been a major annoyance for a long time so I'd love to see this committed - can one of the Epiphany developers review?
Comment 12 Claudio Saavedra 2013-07-12 10:11:29 UTC
Review of attachment 248352 [details] [review]:

I think it would be best for the review if you could split your rework in a series of minor patches, whenever possible, and explain each change. That would make review much easier.
Comment 13 Claudio Saavedra 2013-07-12 10:12:31 UTC
Review of attachment 248353 [details] [review]:

Is this independent from the previous patch?
Comment 14 Garrett Regier 2013-07-12 12:36:32 UTC
(In reply to comment #12)
> Review of attachment 248352 [details] [review]:
> 
> I think it would be best for the review if you could split your rework in a
> series of minor patches, whenever possible, and explain each change. That would
> make review much easier.

How should I split this? I had to completely rework how downloads were managed because it was broken, which is all this patch fixes as explained in the commit message. The second patch was split because it was for something else.

(In reply to comment #13)
> Review of attachment 248353 [details] [review]:
> 
> Is this independent from the previous patch?

No, without the other the file is downloaded twice.
Comment 15 Adam Dingle 2013-07-17 11:15:17 UTC
Ping?  This bug is a constant annoyance, and I'd love to see a fix committed for 3.10.
Comment 16 Claudio Saavedra 2013-07-23 08:01:47 UTC
Sorry, I'll try to review this before 3.10.
Comment 17 Claudio Saavedra 2013-08-03 10:33:24 UTC
Review of attachment 248353 [details] [review]:

Looks good in general, except the comments below. Nonetheless, you need to write a more descriptive comment in the log.

::: src/popup-commands.c
@@ +236,3 @@
+	ephy_download = ephy_download_new_for_download (download, GTK_WINDOW (window));
+
+	switch (gtk_dialog_run (GTK_DIALOG (dialog)))

Please don't use gtk_dialog_run().

@@ +283,3 @@
 		char *base;
 
+		base = ephy_sanitize_filename (g_path_get_basename (location));

This change should definitely go in a separate patch (even a separate bug), explaining why it is necessary, even if it is obvious and a small change, as it is unrelated to the bug you're fixing with this patch.
Comment 18 Claudio Saavedra 2013-08-03 10:40:30 UTC
Review of attachment 248352 [details] [review]:

I have a hard time understanding what you're trying to fix with this patch, and therefore I can't get the solution you're proposing. Could you elaborate in the exact problem first so that we can discuss what would be the best way to fix whatever is broken?

Also, please note that we've removed WebKit1 support from epiphany, so if it makes your patches cleaner, feel free to remoe the left-over bits from WebKit1 in a zero patch.

::: embed/ephy-download.c
@@ +959,3 @@
+    }
+
+    return ephy_download;

There is something wrong in our direction if a method that is called ephy_download_new_for_download() already has a download and returns the existing one. We need to make sure this doesn't happen instead of working around the case when the method is called a second time.

::: src/ephy-window.c
@@ +3379,3 @@
+		children = gtk_container_get_children (GTK_CONTAINER (window->priv->downloads_box));
+		if (g_list_length (children) == 1)
+			ephy_window_set_downloads_box_visibility (window, FALSE);

I don't understand why we need to do UI related changes in this patch. Could you please elaborate?

::: src/window-commands.c
@@ +630,3 @@
+	/* We do not want this download to show up in the UI */
+	ephy_download = ephy_download_new_for_download (download, NULL);
+	ephy_embed_shell_remove_download (ephy_embed_shell_get_default (), ephy_download);

I think we need to rethink the way this is done. I don't think creating  a download and immediately removing it is good. If the API is forcing us to do this, then we need to rework the API.
Comment 19 Adam Dingle 2013-11-10 12:39:18 UTC
This is still not working in git master.  I believe that Garrett is no longer working on this.  I'd love to see someone pick this back up.
Comment 20 Daniel Wyatt 2013-11-23 00:21:02 UTC
I had a go at this.
I'll submit it as a series of three small patches.
Comment 21 Daniel Wyatt 2013-11-23 00:26:31 UTC
Created attachment 261277 [details] [review]
Add filename-suggested signal to EphyDownload to propagate WebKit's decide-destination signal.
Comment 22 Daniel Wyatt 2013-11-23 00:27:42 UTC
Created attachment 261278 [details] [review]
Add ephy_file_move_uri to ephy-file-helpers.
Comment 23 Daniel Wyatt 2013-11-23 00:28:18 UTC
Created attachment 261279 [details] [review]
Fix Save As to use suggested filename
Comment 24 Adam Dingle 2013-11-30 15:57:27 UTC
Daniel, thanks for your work on this.  To try it out, I just built Epiphany from git master with your patches applied.

First, I think you should generate patches using 'git format-patch' rather than 'git apply' - that way your name/email and a description of each change can be included in the git history.

WIth your changes, if I right click a link (e.g. one of the "Created an attachment" links above) and choose Save As, the Save Link As dialog shows the correct filename ("filename-suggested-signal.diff").   That's great.  However,  I also see at the bottom of the window that the file is being downloaded to a filename such as ephy-download-MHU36W, which appears in my Downloads directory.  That file apparently gets copied to filename-suggested-signal.diff at the location I choose.

It seems a bit ugly that this temporary filename is exposed to the user.  Is that necessary?  Could the file be downloaded using its actual name ("filename-suggested-signal.diff")? Could it be moved rather than copied to the destination so there won't be an extra copy lying around afterward?
Comment 25 Daniel Wyatt 2013-11-30 16:09:38 UTC
Adam, you bring up some good points.
As for moving vs copying, it _should_ already be a move operation, not a copy. It uses g_file_move.
Anyways, I'll see what I can do to address the other points.
Comment 26 Daniel Wyatt 2013-11-30 17:50:30 UTC
Created attachment 263211 [details] [review]
Add filename-suggested signal to EphyDownload
Comment 27 Daniel Wyatt 2013-11-30 17:51:04 UTC
Created attachment 263212 [details] [review]
Add ephy_file_move_uri to ephy-file-helpers.
Comment 28 Daniel Wyatt 2013-11-30 17:51:23 UTC
Created attachment 263213 [details] [review]
Fix Save As to use suggested filename
Comment 29 Daniel Wyatt 2013-11-30 17:56:54 UTC
The new patches should provide a better user experience.
I made two changes:
1) Hide the download widget until the user decides on a destination so they won't see the ugly temporary filename.
2) Delete temporary files if a download is canceled.
Comment 30 Adam Dingle 2013-11-30 18:19:44 UTC
I built with these latest patches.  This certainly seems much better from a user standpoint - thanks.

I'm curious about one more thing.  If I click on a large download without choosing "Save As", then the file is downloaded directly to my Downloads directory with its final name.  If I open a Nautilus window on that directory, I can see the file increasing in size as the download progresses.  That seems nice.

But even with this patch, if I choose "Save As" and choose a directory and a filename, Epiphany first downloads to the Downloads directory with a temporary filename, and only afterward moves the file to its destination.  Why is that necessary?  Could we not just download directly to the destination?  It seems like that would be simpler and more efficient (no file move necessary).  I can only surmise that the goal is to begin the download even while the user is in the Save As dialog; since we don't yet know the destination filename, we have to download to Downloads instead.  (If that is the intention, I'm a little skeptical that saving a few seconds of download time is worth potentially having to do a large copy across filesystems later on, though I don't feel strongly about this.)

Can one of the Epiphany developers comment on this last point, and review these patches?  It would be nice to land this if possible.
Comment 31 Daniel Wyatt 2013-11-30 19:18:10 UTC
>I can
only surmise that the goal is to begin the download even while the user is in
the Save As dialog

This is essentially correct. The download must be started to receive the suggested filename from the server.
So by the time the Save As dialog is shown, the connection is already open.
We can't really rely on a TCP connection staying open forever.

So, it downloads to a temporary filename because the user may take any amount of time to specify a destination via the Save As dialog.
(The way Chrome seems to handle this is it starts downloading to a temporary after the Save As dialog is open for about 2 seconds.)

It would be nice to move and continue the download as soon as the location is decided by the user.
Unfortunately, looking at the WebKit docs, I don't see any way to relocate a download while it is in progress.
Chrome (and I'll assume Chromium) does manage to do this but I believe it does not use WebKit for downloads.

Anyways, I guess we'll see what the devs have to say.
Comment 32 Adam Dingle 2013-12-07 16:22:55 UTC
Epiphany developers, could you possibly look at this and land it if appropriate?  It would be really nice to have this fix for 3.12.
Comment 33 Claudio Saavedra 2013-12-08 14:54:10 UTC
Review of attachment 263213 [details] [review]:

I have a few comments, but I think Carlos should review this too.

::: src/popup-commands.c
@@ +241,3 @@
+	if (completed && dest_uri) {
+		const char *source = ephy_download_get_destination_uri (download);
+		ephy_file_move_uri (source, dest_uri);

This can happen much latter than the user selects the destination. A file could be created in the location of "save-as-uri", and this move here will then overwrite it. This is not safe.

@@ +337,3 @@
+
+		download = ephy_download_new_for_uri (location,
+					GTK_WINDOW (window));

Indentation.

@@ +339,3 @@
+					GTK_WINDOW (window));
+
+		gtk_widget_hide (ephy_download_get_widget (download));

Is the widget visible before this at all? If it is, then this will look odd.

@@ +342,3 @@
+
+		dest_dir = ephy_file_get_downloads_dir ();
+		dest_name = ephy_file_tmp_filename ("ephy-download-XXXXXX", NULL);

How about also making the file hidden?

@@ +358,3 @@
+			G_CALLBACK (save_as_completed_cb), NULL);
+		g_signal_connect (download, "error",
+			G_CALLBACK (save_as_error_cb), NULL);

Indentation in these four calls.
Comment 34 Claudio Saavedra 2013-12-08 14:54:32 UTC
Review of attachment 263211 [details] [review]:

OK
Comment 35 Claudio Saavedra 2013-12-08 14:54:59 UTC
Review of attachment 263212 [details] [review]:

OK
Comment 36 William Jon McCann 2013-12-21 17:58:38 UTC
Created attachment 264721 [details] [review]
Add filename-suggested signal to EphyDownload
Comment 37 William Jon McCann 2013-12-21 17:59:00 UTC
Created attachment 264722 [details] [review]
Add ephy_file_move_uri to ephy-file-helpers
Comment 38 William Jon McCann 2013-12-21 17:59:06 UTC
Created attachment 264723 [details] [review]
Fix Save As to use suggested filename
Comment 39 William Jon McCann 2013-12-21 17:59:14 UTC
Created attachment 264724 [details] [review]
Make temporary downloads hidden files
Comment 40 William Jon McCann 2013-12-21 18:02:59 UTC
*** Bug 720881 has been marked as a duplicate of this bug. ***
Comment 41 Daniel Wyatt 2013-12-23 15:23:43 UTC
Forgot about this for a bit. So, where do we stand on this now?
Comment 42 Daniel Wyatt 2013-12-24 07:15:16 UTC
Ping. Server was malfunctioning when I commented last.
Comment 43 William Jon McCann 2014-01-03 18:12:30 UTC
Comment on attachment 264722 [details] [review]
Add ephy_file_move_uri to ephy-file-helpers

transferring status of last patch to this one
Comment 44 William Jon McCann 2014-01-03 18:13:14 UTC
Comment on attachment 264721 [details] [review]
Add filename-suggested signal to EphyDownload

adding last review status to new patch
Comment 45 William Jon McCann 2014-01-07 21:52:43 UTC
Attachment 264721 [details] pushed as 4897f86 - Add filename-suggested signal to EphyDownload
Attachment 264722 [details] pushed as 5409a66 - Add ephy_file_move_uri to ephy-file-helpers
Comment 46 Adam Dingle 2014-01-08 12:32:03 UTC
It's nice to see some activity on this bug.  Jon, it looks like you've pushed two of the patches that Daniel posted above but not the third, which would be required to fix this.

As Daniel asked above, where do we now stand?  In comment 33 above Claudio posted some feedback on the patch "Fix Save As to use suggested filename", but said "Carlos should review this too".  So are we now waiting for Carlos's review comments?
Comment 47 Bastien Nocera 2014-01-08 13:06:45 UTC
Review of attachment 264724 [details] [review]:

Instead of using a hidden file, I would have appended .part to the filename (as bittorrent clients usually do) or even added a new extension (Chrome uses .crdownload for example).
This makes it easier to identify unfinished downloads in the Downloads directory, and avoids large amounts of space being used and hidden from the user.
Comment 48 Michael Catanzaro 2014-01-08 14:47:25 UTC
(In reply to comment #47)
> Review of attachment 264724 [details] [review]:
> 
> Instead of using a hidden file, I would have appended .part to the filename (as
> bittorrent clients usually do) or even added a new extension (Chrome uses
> .crdownload for example).
> This makes it easier to identify unfinished downloads in the Downloads
> directory, and avoids large amounts of space being used and hidden from the
> user.

But see Bug #721726
Comment 49 Bastien Nocera 2014-01-08 15:36:05 UTC
(In reply to comment #48)
> (In reply to comment #47)
> > Review of attachment 264724 [details] [review] [details]:
> > 
> > Instead of using a hidden file, I would have appended .part to the filename (as
> > bittorrent clients usually do) or even added a new extension (Chrome uses
> > .crdownload for example).
> > This makes it easier to identify unfinished downloads in the Downloads
> > directory, and avoids large amounts of space being used and hidden from the
> > user.
> 
> But see Bug #721726

I don't see how this bug matters in our case. If epiphany crashes, or the download is still on-going, I'd rather see the file with a .part suffix than nothing at all.
Comment 50 Carlos Garcia Campos 2014-01-08 19:34:54 UTC
(In reply to comment #47)
> Review of attachment 264724 [details] [review]:
> 
> Instead of using a hidden file, I would have appended .part to the filename (as
> bittorrent clients usually do) or even added a new extension (Chrome uses
> .crdownload for example).
> This makes it easier to identify unfinished downloads in the Downloads
> directory, and avoids large amounts of space being used and hidden from the
> user.

Exactly, but that should be done by webkit, I'm currently working on a patch for webkit to do it, using .wkdownload suffix.
Comment 51 Carlos Garcia Campos 2014-01-08 19:37:00 UTC
(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > > Review of attachment 264724 [details] [review] [details] [details]:
> > > 
> > > Instead of using a hidden file, I would have appended .part to the filename (as
> > > bittorrent clients usually do) or even added a new extension (Chrome uses
> > > .crdownload for example).
> > > This makes it easier to identify unfinished downloads in the Downloads
> > > directory, and avoids large amounts of space being used and hidden from the
> > > user.
> > 
> > But see Bug #721726
> 
> I don't see how this bug matters in our case. If epiphany crashes, or the
> download is still on-going, I'd rather see the file with a .part suffix than
> nothing at all.

Bug #721726 is about cancelled downloads, not ongoing or partial file left after a crash. The partial file should be removed when the download is cancelled, but again it's not something ephy should care about. I'm fixing it in webkit too as part of the intermediate file patch.
Comment 52 Carlos Garcia Campos 2014-01-09 12:33:11 UTC
Created attachment 265833 [details] [review]
Do not duplicate the destination in EphyDownload
Comment 53 Carlos Garcia Campos 2014-01-09 12:33:44 UTC
Created attachment 265834 [details] [review]
Simplify save_property_url
Comment 54 Carlos Garcia Campos 2014-01-09 12:34:28 UTC
Created attachment 265835 [details] [review]
Do not show downloads in the UI until a destination has been created
Comment 55 Carlos Garcia Campos 2014-01-09 12:35:14 UTC
Created attachment 265837 [details] [review]
Use the filename suggested by the server when asking the user
Comment 56 Carlos Garcia Campos 2014-01-09 13:26:04 UTC
(In reply to comment #54)
> Created an attachment (id=265835) [details] [review]
> Do not show downloads in the UI until a destination has been created

This is wrong, and leaks the downloads cancelled before being shown, because for some reason is the download widget the one taking ownership of the download. Working on a new patch.
Comment 57 Carlos Garcia Campos 2014-01-09 14:44:52 UTC
Created attachment 265846 [details] [review]
Do not add automatically the downloads to the UI when they are created
Comment 58 Carlos Garcia Campos 2014-01-09 14:45:23 UTC
Created attachment 265847 [details] [review]
Use the filename suggested by the server when asking the user
Comment 59 Claudio Saavedra 2014-01-13 15:00:29 UTC
Review of attachment 265833 [details] [review]:

Looks good.
Comment 60 Claudio Saavedra 2014-01-13 15:10:19 UTC
Review of attachment 265834 [details] [review]:

Looks good. Were we leaking a string here?
Comment 61 Claudio Saavedra 2014-01-13 15:44:00 UTC
Review of attachment 265846 [details] [review]:

Looks good.
Comment 62 Claudio Saavedra 2014-01-13 15:48:24 UTC
Review of attachment 265847 [details] [review]:

Looks good.

::: src/popup-commands.c
@@ +155,2 @@
 {
+	ephy_download_cancel (download);

Why does this need to go to an idle?
Comment 63 Carlos Garcia Campos 2014-01-13 16:24:11 UTC
(In reply to comment #60)
> Review of attachment 265834 [details] [review]:
> 
> Looks good. Were we leaking a string here?

Yes, the GValue was leaked.
Comment 64 Carlos Garcia Campos 2014-01-13 16:31:52 UTC
(In reply to comment #62)
> Review of attachment 265847 [details] [review]:
> 
> Looks good.
> 
> ::: src/popup-commands.c
> @@ +155,2 @@
>  {
> +    ephy_download_cancel (download);
> 
> Why does this need to go to an idle?

Because cancel finishes the webkit download, and the finished callbacks destroys the ephy download and we are in the decide-filename callback of the webkit download. When we return from filename-suggested callback, the ephy downloads is expected to be alive, but it can be destroyed, so it's better to cancel the download after the decide destination has finished.
Comment 65 William Jon McCann 2014-01-13 23:33:29 UTC
*** Bug 678658 has been marked as a duplicate of this bug. ***