498695 - Refactoring: in contentAreaUtils.js, clean up the getTargetFile function, aligning it to the SeaMonkey version

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Attachment: Refactoring of getTargetFile

This, like bug 471875, is another refactoring that doesn't change
the functionality of "contentAreaUtils.js". Its purpose is to align the
getTargetFile function with the copy of the code in use by SeaMonkey,
since the logic of the function is more explicit there:

http://mxr.mozilla.org/comm-central/source/suite/common/contentAreaUtils.js#530

The new function is very similar to the SeaMonkey version; the main
difference is that the code required for private browsing mode isn't
present there.

This work is related to bug 484616.

Comment 1

[: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 2

[:Gavin Sharp [email: [e-mail]]]

sdwilsh might be a better choice for (first-pass?) review here, I'm not sure I'll be able to get to this in the near future.

Comment 3

[Shawn Wilsher :sdwilsh]

sadly, that is probably true (my review queue is getting long :( )

Comment 4

[:Paolo Amadini]

Comment on attachment 383508 [details] [diff] [review]
Refactoring of getTargetFile

Boris, maybe you can review this refactoring of contentAreaUtils.js, since Gavin and Shawn are busy and you already reviewed the one in bug 471875?

Comment 5

[Boris Zbarsky [:bzbarsky]]

>+++ b/toolkit/content/contentAreaUtils.js
>+ * @param aSkipPrompt
>+ *        If false, don't save the file automatically to the user's
>+ *        default download directory, even if the associated preference
>+ *        is set, but ask for the target explicitly.

Is this a behavior change?

> function getTargetFile(aFpP, /* optional */ aSkipPrompt)

>+  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?

In fact, I think that this really needs a toolkit peer's review.  I don't know this code all that well, or how it's supposed to interact with toolkit's download manager (which I don't know at all)....  So my review wouldn't be worth much.

Comment 6

[Shawn Wilsher :sdwilsh]

I can likely take a look at this this week, but I'm not a toolkit peer (but I do own the download manager code!)

Comment 7

[:Paolo Amadini]

(In reply to comment #5)
> >+++ b/toolkit/content/contentAreaUtils.js
> >+ * @param aSkipPrompt
> >+ *        If false, don't save the file automatically to the user's
> >+ *        default download directory, even if the associated preference
> >+ *        is set, but ask for the target explicitly.
> 
> Is this a behavior change?

No, that's exactly how the function worked before. I proposed two minor
unrelated behavior changes in comment 1.

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. The goal of this
first step is to make the two "contentAreaUtils.js" files in SeaMonkey and
in Toolkit look as similar as possible, before unforking.

> > function getTargetFile(aFpP, /* optional */ aSkipPrompt)
> 
> >+  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?

I agree. This, however, is how the function is implemented in SeaMonkey.

I think that in this case we might also change this immediately even if this
results in a little difference between the files.

Comment 8

[:Paolo Amadini]

Comment on attachment 383508 [details] [diff] [review]
Refactoring of getTargetFile

(In reply to comment #6)
> I can likely take a look at this this week, but I'm not a toolkit peer (but I
> do own the download manager code!)

I'd be happy if Shawn could review this, if Gavin's fine with it.

If there is any doubt left we may also ask Neil Rashbrook, who previously commented on bug 484616.

Comment 9

[Shawn Wilsher :sdwilsh]

I'm looking at this now, but I have some issues with trying to make this line up with SeaMonkey.  I don't agree with some of their method names, etc, and I think we should fix it right on mozilla-central.  If that means you want to split your work up into two patches, the first to get it lined up with SeaMonkey, and the second to make the changed per review comments, I'm fine with that.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 383508 [details] [diff] [review]
Refactoring of getTargetFile

Sorry this took so long.  Future reviews shouldn't take as long on this bug.

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 533
>  * @param aSkipPrompt
>  *        If false, don't save the file automatically to the user's
>  *        default download directory, even if the associated preference
>  *        is set, but ask for the target explicitly.

Do you really mean false here?  If so, we should change the name to
aDoNotSkipPrompt, no?


on file: toolkit/content/contentAreaUtils.js line 571
>   // Must Prompt
> 
>   // 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.

Instead of saying Must Prompt, and then the next comment, how about:
Default to lastDir since we must prompt.  Check that lastDir is valid, and if
it's not, use the user's default downloads directory.

The rest of the comment should be continued further down...


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 620
>   if (aFpP.saveMode != SAVEMODE_FILEONLY)
>     prefs.setIntPref("save_converter_index", fp.filterIndex);

What is this about?  I see you've moved it, but a comment here would be
awesome.


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. (I'd actually be really happy if you'd do that in this
bug, but I won't make that a requirement)


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

javadoc comment please?


on file: toolkit/content/contentAreaUtils.js line 651
>       // Append "(2)" before the last dot in (or at the end of) the filename
>       // special case .ext.gz etc files so we don't wind up with .tar(2).gz

I'm sorta surprised this doesn't use nsIFile's createUnique.  This is how we
get a unique name in storage...
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageService.cpp#361


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 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> on file: toolkit/content/contentAreaUtils.js line 651
> >       // Append "(2)" before the last dot in (or at the end of) the filename
> >       // special case .ext.gz etc files so we don't wind up with .tar(2).gz
> 
> I'm sorta surprised this doesn't use nsIFile's createUnique.  This is how we
> get a unique name in storage...

CreateUnique-generated filenames were considered uglier than the ()-suffix, I think. It's possible that the behavior also changes with regard to how it deals with multiple-extension filenames?

Asking for a bunch of style/nit cleanup in a bug about syncing with SeaMonkey kind of defeats the point of the syncing (since those changes will then need to be made over in the seamonkey version...) Seems like we should defer those until the files are actually in sync (or mostly so).

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> CreateUnique-generated filenames were considered uglier than the ()-suffix, I
> think. It's possible that the behavior also changes with regard to how it deals
> with multiple-extension filenames?
Ah, yeah, probably how it looks. :/

> Asking for a bunch of style/nit cleanup in a bug about syncing with SeaMonkey
> kind of defeats the point of the syncing (since those changes will then need to
> be made over in the seamonkey version...) Seems like we should defer those
> until the files are actually in sync (or mostly so).
That's why I asked if you could do it in two patches.  We can land the first patch to synch them, remove comm-central's, and then apply the new one on mozilla-central.  My review comments reflect how I'd like to see this as the final result.  How we get there, I'm not as concerned about.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> > Asking for a bunch of style/nit cleanup in a bug about syncing with SeaMonkey
> > kind of defeats the point of the syncing (since those changes will then need to
> > be made over in the seamonkey version...) Seems like we should defer those
> > until the files are actually in sync (or mostly so).
> That's why I asked if you could do it in two patches.

I think you're confusing Paolo and I? But anyways, I hadn't seen comment 9 before making that comment - sorry about that.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> I think you're confusing Paolo and I? But anyways, I hadn't seen comment 9
> before making that comment - sorry about that.
I don't think I read who it was from.  My bad...

Comment 15

[:Paolo Amadini]

I agree with most of the observations on the SeaMonkey code, however I think
that most of them apply to the existing Toolkit code on trunk as well, and in
the end the SeaMonkey code is easier to read as it disentangles some of the
obscure logic behind the Toolkit function (unsurprisingly, now a lot of people
began asking about this aSkipPrompt parameter :-) ).

Can't we just land the synchronization patch and address the cleanup later,
maybe together with the behavioral changes I proposed in comment 1 (and that
no-one seemed to notice at first ;-) )?

I think the new situation on trunk would still be an improvement over the
current one. I could have done the synchronization the other way around,
asking the SeaMonkey owners to modify their code and return to the Toolkit
version, but I think that would be a regression on the understanding of the
function itself and I don't think they'd agree, for this reason.

Comment 16

[:Paolo Amadini]

Attachment: Refactoring of getTargetFile

This addresses some of the comment-related concerns from Shawn's review, that
actually don't affect the diff with the SeaMonkey code very much. I'll file a
followup bug to cover all the other issues that have been raised, in order to
fix them right after the unforking.

Comment 17

[:Paolo Amadini]

Bug 506987 filed!

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 391153 [details] [diff] [review]
Refactoring of getTargetFile

r=sdwilsh

You'll need a toolkit peer to look at this too, so I'm gonna have mconnor do it (who should get to it today or next week friday).

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 391153 [details] [diff] [review]
Refactoring of getTargetFile

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

> function getTargetFile(aFpP, /* optional */ aSkipPrompt)
                 
>+  // Default to the user's default downloads directory configured
>+  // through download prefs.
>+  var dlMgr = Components.classes["@mozilla.org/download-manager;1"]
>+                        .getService(Components.interfaces.nsIDownloadManager);

I can't believe we haven't switched to Cc/Ci here... oh well.


>+  // We must prompt for the file name explicitly. If we must prompt because we
>+  // were asked to (not because the default download directory doesn't exist)...

so... we're hit this codepath if the directory does exist but useDownloadDirectory is false..  so this comment is confusing.  Even if it exists, we'll use the last downlaod dir, not the default download directory, so take out the parens bit.


>+// Get the preferences branch ("browser.download." for normal 'save' mode)...
>+function getPrefsBrowserDownload(branch)
>+{
>+  const prefSvcContractID = "@mozilla.org/preferences-service;1";
>+  const prefSvcIID = Components.interfaces.nsIPrefService;                              
>+  return Components.classes[prefSvcContractID].getService(prefSvcIID).getBranch(branch);

Why is this a function?  It's used once.  And it's named very specifically, but takes an argument to get any branch.  Just leave this inline.

Comment 20

[:Paolo Amadini]

Attachment: Refactoring of getTargetFile [Checkin: Comment 21]

Thanks Mike for having a look at this patch. In this new version, I updated
the comment you think is confusing as you suggested. As for the strange
getPrefsBrowserDownload function, fixing this is already filed as bug
506987, as the purpose of this bug is only to align the Toolkit code to
the SeaMonkey code (except for small differences in comments) in order to
facilitate the removal of their copy of "contentAreaUtils.js" eventually.
The cleanup will follow right after the unforking.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 393290 [details] [diff] [review]
Refactoring of getTargetFile
[Checkin: Comment 21]


http://hg.mozilla.org/mozilla-central/rev/f264d3771fe8