206441 - Show File Location fails, Status bar includes file:// for downloads, getFileForItem should return filepath not URL

Status: RESOLVED DUPLICATE of bug 175881

(SeaMonkey :: Download & File Handling, defect)

Description

[Ian Neal]

Using BuildID 2003042508 on linux (RH8.0)

Steps to reproduce
1. Download a file (e.g. foo.tar.gz to /tmp)
2. Select entry in download manager
3. Look at status bar

Expected results
1. Status bar shows /tmp/foo.tar.gz

Actual results:
1. Status bar shows file:///tmp/foo.tar.gz

The status bar is set from the selected item's id so we probably need to either
change what the status bar uses or change what is used for the id.

Comment 1

[[e-mail]]

I've seen this but not for downloads in the current session.

I've also seen something similar in Windows, but only for UNC paths.

Comment 2

[Ian Neal]

When a new download shows up in the manager it doesn't have the file:// it's
only after you exit mozilla and go back in again the file:// is added. It also
breaks the testing of file existance.

Comment 3

[Ian Neal]

Attachment: Patch v1.0 - first pass

This patch just deals with the status bar display and fixes the file existance
testing resulting from how the file resource is stored in the download manager
RDF file. It doesn't deal with making download manager store the file resource
in a consistent manner - when a new download is created its id isn't a URL
whereas downloads from previous sessions are.

Comment 4

[Ian Neal]

Also happens on Mac OS X so setting to All/All
From zach's testing looks like it was present in BuildID 20030501xx in the form
of not being able to launch files because getFileForItem was being passed a URL
instead of a filepath.
Still need to look at what is causing the File resource to be changed on mozilla
restart.

Comment 5

[Ian Neal]

*** Bug 157647 has been marked as a duplicate of this bug. ***

Comment 6

[Ian Neal]

Taking

Comment 7

[Ian Neal]

Accepting

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Is there no better way to fix this? What does your patch do to UNC Paths, will
they still work? Why not let necko convert the url to a file?

Comment 9

[Ian Neal]

Attachment: Patch v1.1

This patch goes back to my first idea for a solution which I originally
couldn't get to work but now does thanks to biesi (I was missing a .path).

Comment 10

[[e-mail]]

Comment on attachment 124389 [details] [diff] [review]
Patch v1.1

Hmm... sometimes you want a nsILocalFile and sometimes you want a path. It's
just that converting a file to a  path and back to a file somehow seems
wasteful.

Comment 11

[Ian Neal]

Are you refering to the showinshell part of downloadmanager.js?

Comment 12

[Ian Neal]

Attachment: Patch v1.2a

Hopefully avoids converting things from one form to another and back again and
no longer introduces a new function. Status bar now makes use of
getFileFromItem too.

Comment 13

[Ian Neal]

Attachment: Patch v1.2b

Moved check from getFileForItem to createLocalFile as suggested by Neil on IRC

Comment 14

[jag (Peter Annema)]

Comment on attachment 124730 [details] [diff] [review]
Patch v1.2b

sr=jag

Seems like we're doing more work here than we should have to do. Oh well.

Comment 15

[jag (Peter Annema)]

Which patch is better... This one, or the one in bug 175881?

Comment 16

[Ian Neal]

Both do similar things with createLocalFile just the check for a URL is done
differently and mine's missing some explanatory comments.

Comment 17

[Jan Varga [:janv]]

That url check is unnecessary once we fix the download manager to be consistent.
My patch also caches the io service :)

Btw, your patch misses a QI to nsILocalFile which is required on windows and mac
for launch() method be accessible.

Comment 18

[Ian Neal]

Hmmm are you sure win32 needs QI? As I've been testing this patch (or a previous
revision) for 3-4 days and I've been able to launch files without a problem. I
don't have access to mac for testing purposes.

Now caching sounds like a good idea :-)

Comment 19

[Jan Varga [:janv]]

It's needed on Mac (I had to add it while I was testing on this platform) and I
guess it's also needed on Windows.
However, your patch should work with new entries in the download manager, but I
haven't tried.

Comment 20

[Ian Neal]

I'm happy to either incorporate your patch into mine using your caching, the
added QI, whichever is the least costly check and the navigator.platform bit or
for you to do similar and include my gStatusBar.label bit

Comment 21

[[e-mail]]

*** This bug has been marked as a duplicate of 175881 ***

Comment 22

[Ian Neal]

Removing blocking request and blocking bug number

Comment 23

[Ian Neal]

Attachment: Patch to help Jungshik Shin

This patches downloadmanager.js so that it only passes URLs to
|getFileFromURLSpec|

Comment 24

[[e-mail]]

Comment on attachment 125179 [details] [diff] [review]
Patch to help Jungshik Shin

>-  var file;
Don't remove this. Only one declaration per function, please.

Comment 25

[Ian Neal]

Attachment: Revised patch to help Jungshik Shin

Using only one |var file;|

Comment 26

[Jungshik Shin]

Just FYI, my patch refered to is  attachment 125219 [details] [diff] [review] to bug 162361.
'downloadmanager.js' part in the patch is virtually identical to attachment
125260 [details] [diff] [review] as Ian wrote.

Anyway, shouldn't we land this patch? It seems like a waste to invoke
getFileFromURLSpec only to find that it fails (quite annoying in a debug build
on Windows because NS_ERROR(...)).