506987 - Refactoring: in contentAreaUtils.js, clean up the getTargetFile function thoroughly

Status: RESOLVED INACTIVE

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

+++ This bug was initially created as a clone of Bug #498695 +++

------- Comment #1 From Paolo Amadini -------

Possible improvements:

>+  // Default to lastDir if we must prompt, and lastDir
>+  // is configured and valid. Otherwise, keep the default of
>+  // the user's default downloads directory; since it doesn't
>+  // exist, it will be changed to the user's desktop later.
>+  if (!useDownloadDir) try {

Removing this "if" condition would cause the last used directory to be
proposed in the file picker, instead of the user's desktop, when an
attempt to automatically save in the user's default download directory
without a prompt failed because the directory doesn't exist anymore.

This would make the file picker's default directory logic more consistent;
I don't believe the current behavior was intended by design.

>   var inPrivateBrowsing = false;
>   try {
>     var pbs = Components.classes["@mozilla.org/privatebrowsing;1"]
>                         .getService(Components.interfaces.nsIPrivateBrowsingService);
>     inPrivateBrowsing = pbs.privateBrowsingEnabled;
>   }
>   catch (e) {
>   }

Replacing the try block with a component check would fix a strict
JavaScript warning and avoid that other exceptions get eaten:

if ("@mozilla.org/privatebrowsing;1" in Components.classes) {
  var pbs = Components.classes["@mozilla.org/privatebrowsing;1"]
            .getService(Components.interfaces.nsIPrivateBrowsingService);
  inPrivateBrowsing = pbs.privateBrowsingEnabled;
}

------- Comment #5 From Boris Zbarsky -------

>+  var prefs = getPrefsBrowserDownload("browser.download.");

That function's name is sort of redundant with the argument, and the
documentation doesn't match the implementation.  If you want a generic
getPrefBranch function here, that might be fine; just check with a toolkit
peer?

------- Comment #7 From Paolo Amadini -------

>+ * @param aSkipPrompt

Probably aSkipPrompt is not the best name for the parameter, see
bug 484616 comment 8, 9 and 10 for an enumeration of alternatives ;-)

I propose to leave the name as it is for now; we can always change it later
in a follow-up patch together with the behavior changes.

------- Comment #10 From Shawn Wilsher -------

http://reviews.visophyte.org/r/383508/
on file: toolkit/content/contentAreaUtils.js line 529
>  * Given the Filepicker Parameters (aFpP), show the file picker dialog,

I think file picker should be two words here

on file: toolkit/content/contentAreaUtils.js line 531
>  * @param aFpP a structure (see definition in internalSave(...) method)
>  *        containing all the data used within this method.

nit: newline after aFpP.  Also, can we use an actual phrase here please?

on file: toolkit/content/contentAreaUtils.js line 577
>   if (!useDownloadDir) try {

I'm surprised that's valid.  Add braces for the if and have the try on a
newline please.

on file: toolkit/content/contentAreaUtils.js line 587
>   } catch(e) {}

}
catch (e) {
}
with a comment in the catch as to why we are doing this

on file: toolkit/content/contentAreaUtils.js line 638
> // Since we're automatically downloading, we don't get the file picker's
> // logic to check for existing files, so we need to do that here.
> //
> // Note - this code is identical to that in
> //   mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
> // If you are updating this code, update that code too! We can't share code
> // here since that code is called in a js component.

Can you please file a follow-up to move this stuff to DownloadUtils.jsm and
then it can be shared.

on file: toolkit/content/contentAreaUtils.js line 645
> function uniqueFile(aLocalFile)

javadoc comment please?

on file: toolkit/content/contentAreaUtils.js line 771
> // Get the preferences branch ("browser.download." for normal 'save' mode)...
> function getPrefsBrowserDownload(branch)

Ditto on bz's comment.  Also, this should have a javadoc style comment.

on file: toolkit/content/contentAreaUtils.js line 776
>   return Components.classes[prefSvcContractID].getService(prefSvcIID).getBranch(branch);

Don't we have Cc and Ci defined in this file?  Please use them.

Comment 1

[:Paolo Amadini]

> on file: toolkit/content/contentAreaUtils.js line 638
> > // Since we're automatically downloading, we don't get the file picker's
> > // logic to check for existing files, so we need to do that here.
> > //
> > // Note - this code is identical to that in
> > //   mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
> > // If you are updating this code, update that code too! We can't share code
> > // here since that code is called in a js component.
> 
> Can you please file a follow-up to move this stuff to DownloadUtils.jsm and
> then it can be shared.

Now that bug 484616 is closed, I've spent some time in studying the existing
code related to the selection of the file name to be used for downloads. I'm
impressed by the fact that, besides the uniqueFile/makeFileUnique functions,
most of the logic in nsHelperAppDlg.js and contentAreaUtils.js is identical,
just implemented with totally different code, but the results are the same
even in some corner cases I tested.

I'm considering whether it can be appropriate to move most of the common
logic to a new JS module, say DownloadPaths.jsm (whereas DownloadUtils.jsm
is currently dedicated to calculating download times). This refactoring
should be done carefully, keeping the various corner cases in mind, but
might help in sorting out the desired behavior on various platforms, and
maybe ease the task of adding new features consistently (like the ability
of saving a page using its title as the file name).

Another reasons why this refactoring would be useful for me is that I'd
write the functions in such a way that customizing their behavior would
be easier; for example, I need to add file types for web archives to the
"Save As" dialog, but today I have to override entire functions in
contentAreaUtils.js. There are also extensions that want to customize
the automatic download folder based on the file type, for example.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> I'm considering whether it can be appropriate to move most of the common
> logic to a new JS module, say DownloadPaths.jsm (whereas DownloadUtils.jsm
> is currently dedicated to calculating download times). This refactoring
> should be done carefully, keeping the various corner cases in mind, but
> might help in sorting out the desired behavior on various platforms, and
> maybe ease the task of adding new features consistently (like the ability
> of saving a page using its title as the file name).
This would probably be a great idea!

Comment 3

[:Paolo Amadini]

Attachment: Work in progress - DownloadPaths module

Here is the DownloadPaths module with a few functions already defined.
Shawn, can you have a look and see if this structure makes sense?

The next step would consist in adding a constructor or factory function
in DownloadPaths to create DownloadTarget JavaScript objects. These objects
would have properties like sourceUri, contentType, contentDisposition and
mimeInfo that can be set to have the object find out both the suggested file
name and target download directory by itself. Overriding the factory function
or constructor function to return derived objects (the "__proto__" technique)
would allow extensions to customize this behavior.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> Shawn, can you have a look and see if this structure makes sense?
It will be at least a week until I can take a look I am sorry to say.

Comment 5

[:Paolo Amadini]

(In reply to comment #4)
> It will be at least a week until I can take a look I am sorry to say.

No problem, thanks for telling me :-) I won't have much time to work on
this bug this week too, but meanwhile I'll try to do some experiments with
the DownloadTarget object.

Comment 6

[:Paolo Amadini]

If you could take a look to the work in progress and provide some feedback
on the idea in comment 3, it would be helpful. I'm not asking for a review,
just feedback on the approach. Or would you like to see some other code first?

Comment 7

[Shawn Wilsher :sdwilsh]

There is a feedback flag on patches just for this ;)

Comment 8

[:Paolo Amadini]

Comment on attachment 432526 [details] [diff] [review]
Work in progress - DownloadPaths module

(In reply to comment #7)
> There is a feedback flag on patches just for this ;)

Ah, interesting...

Comment 9

[Shawn Wilsher :sdwilsh]

I was really hoping to get to the feedback request today, but it doesn't look like it is going to happen.  I'll prioritize it once I get back in a week.  Sorry for the delay!

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 432526 [details] [diff] [review]
Work in progress - DownloadPaths module

createNiceUniqueFile should not touch aLocalFile if it throws an exception.  Just clone it.  In fact, I'd rather it return the new file and never touch the one passed in.

Of course, you'll have tests when I do a final review :)

Comment 11

[:Paolo Amadini]

Attachment: Work in progress - DownloadPaths module

(In reply to comment #10)
> (From update of attachment 432526 [details] [diff] [review])
> Of course, you'll have tests when I do a final review :)

Whoops... where did the tests go?

Ah, just forgot to "hg add" the file... mind to take a look? In my
intentions, the test were an integral part of the feedback request!

Comment 12

[:Paolo Amadini]

I'd also like to know your opinion on the approach described in comment 3.
I can also write some code to show what I mean, if required.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> I'd also like to know your opinion on the approach described in comment 3.
> I can also write some code to show what I mean, if required.
I'd appreciate some elaboration because it is not completely clear to me.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 439487 [details] [diff] [review]
Work in progress - DownloadPaths module

Yeah, this looks good.  Also, if you are OK with the licensing, you should have the test be under the creative commons public domain per http://www.mozilla.org/MPL/license-policy.html

Comment 15

[:Paolo Amadini]

Comment on attachment 439487 [details] [diff] [review]
Work in progress - DownloadPaths module

I moved the part of the patch that's not related to contentAreaUtils.js to
bug 561472, so that it can land independently. The part in contentAreaUtils.js
still requires attention since the new createNiceUniqueFile function creates
an empty file as a placeholder while getTargetFile doesn't.

Comment 16

[ISHIKAWA, Chiaki]

Hi,
I am coming from
https://bugzilla.mozilla.org/show_bug.cgi?id=429827

It seems that proposed changes to this file are finally settling down (in 561472).
Now, I am interested in incorporating the changes into
nsHelperAppDlg.js
But I need to figure out how to create the .jsm file
for it. Or can I share it with this change? (I am a newbie to javascript and
am unfamiliar with how javascript modules are handled inside mozilla code
at runtime although I did read basic xpcom documents.)

Anyway, do let me know when I can finally work on the 429827.
Quick reading made me think that my proposed changes in 429827 and changes
proposed here don't step on each other (of course we need to split
and introduce a new file [or not]), so the merging the changes should
be OK.

The only thing I need to figure out is whether I need to introduce 
a separate .jsm file or not.

TIA

step on e

Comment 17

[Shawn Wilsher :sdwilsh]

I'm pretty sure you don't need to get a new jsm; you should be able to import the one that this bug creates (see the tests for how!)

Comment 18

[ISHIKAWA, Chiaki]

(In reply to comment #17)
> I'm pretty sure you don't need to get a new jsm; you should be able to import
> the one that this bug creates (see the tests for how!)

Thanks.

I can finally test and build my latest clean up patch that is based on the checked-in patch from Paolo Amadini.

I wonder if it is all right to post the "PROPERLY INDENTED" nsHelperAppDlg.js first ( see bug 559833 ) first, and then
post the patch to fix the problem mentioned in bug 429827 .

Well, anyway, to show how the patch looks like.
I will first post the indentation patch to bug 559833.

Then later, I will post the patch to fix the bug 429827, hopefully
the indention is applied for bug 559833.

TIA.

Comment 19

[ISHIKAWA, Chiaki]

> I can finally test and build my latest clean up patch that is based on the
> checked-in patch from Paolo Amadini.

I meant to say that "I could finally test", and I DID find the newly modified patch for 429827 works with this factoring already applied.

Thanks for the clean up, Paolo.

I agree that the less code, the better. Usually, the less code means less bugs.
So removing duplicate like this is a way to go where applicable.

TIA.

Comment 20

[:Paolo Amadini]

I went through the list of improvements in comment 0 and noticed that most of them have already been implemented, while others don't apply anymore since the code has changed significantly in the meantime.