208113 - Use urls instead of local paths when adding items to the list.

Status: RESOLVED DUPLICATE of bug 175881

(SeaMonkey :: Download & File Handling, defect)

Description

[Jan Varga [:janv]]

Currently, new items are added as local paths and then when the data source is
reserialized as uris, which causes problems like in bug 175881.
We should fix the download manafer to be consistent and use uris everywhere.

Comment 1

[Jan Varga [:janv]]

Attachment: alternative approach

Anyway, we might want to get rid of RDF completely, see bug 161783.
We'll see.

Comment 2

[Ian Neal]

Do we need to force a reserialization so that we can remove the !file code that
will be checked in with bug 175881 ?

Comment 3

[Jan Varga [:janv]]

that would be even *slower*
I think, let's stick with the patch in bug 175881 and reimplement download
storage in the future.

Comment 4

[Jan Varga [:janv]]

Comment on attachment 124825 [details] [diff] [review]
alternative approach

I forgot to mention that this patch needs a fresh downloads.rdf file to work
properly.
It doesn't work well witn an existing file.

Comment 5

[Ian Neal]

Presumably it would only have to be a one off reserialization and after that
this patch would work fine.

Comment 6

[Jan Varga [:janv]]

well, I mixed several things in this bug

1.
The patch I attached changes file property from resource to a literal. This
prevents RDF from converting it to file://
The problem is that it does only for new items. We would need to fix other items
too, which is quite ugly.
A reserialization doesn't help here much, because that doesn't automatically
change these properties to literals.

2.
My original approach was about fixing download manager to use uris (file://)
everywhere, but I'm starting to think that it isn't the best solution.

I think we should just reimplement the download manager to use mork instead of RDF.
We might want to get an inspiration from global history implementation.

Comment 7

[Jungshik Shin]

This is a off-topic here, but still might have some relevance. I found this bug
tracking down the cause of a curious problem I came across while trying to make
filenames with characters outside the repertoire of the default system locale
work correctly on Win2k/XP.  With my patch to 'Unicode-enabling' in local file
paths, |getFileFromURLSpec| appears to be invoked with a local file path (not
with a file url) by the download manager.  My patch induces 'NS_ERROR' at 
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsURLHelper.cpp#155
and I think that comes from
http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/resources/downloadmanager.js#370

What I don't understand is why it happens only with my patch. Before finding the
file, I was also baffled by that despite the assertion, I could still save the
file. Now that part is understood. |createLocalFile(aFilePath)| has a fallback. 

I'm wondering if it's nice to invoke getFileFromURLSpec only when |aFilePath| is
a URL. 

Comment 8

[Ian Neal]

That change in behaviour was introduced by the patch to bug 175881. Until the
patch on this bug makes it onto the trunk and users have only URLs in their RDF
file, |aFilePath| won't always be a URL. If you want to do some testing look at
bug 206441 which had an alternative approach and I'll attach a patch to it that
will let you patch downloadmanager.js so it hopefully only passes URLs to
|getFileFromURLSpec| to see if it resolves your problem.

Comment 9

[Jungshik Shin]

Thanks. I made an almost identical patch (see attachment 125219 [details] [diff] [review] to bug 162361.
I'm sorry I forgot to mention the bug number for my Unicode file IO patch for
Windows). BTW, in your patch (attachment 125179 [details] [diff] [review]), I think 'var file' has to be
outside if-else clause because |file| is returned outside the clause.

Comment 10

[Ian Neal]

I do |var file| in two places, the alternative is to declare it once outside the
if statement, which would probably make for a slightly smaller patch.

Comment 11

[Ian Neal]

Attached a revised patch to bug 206441 which is basically the same as jshin's
now as far as downloadmanager.js is concerned.

Comment 12

[Jungshik Shin]

Thanks. I saw 'var file', but I was mistaken because I didn't know that the
scoping rule of ECMAscript is different from that of C/C++ (if/while/for don't
create a new lexical scope)

Comment 13

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 14

[Ian Neal]

This was fixed by the fix to bug 175881 and then everything got replaced by the new download manager anyway.