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.

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; }

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.

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

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?

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

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!)

(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 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.

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 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.

(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).

(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.

(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.

(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...

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.

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.

Bug 506987 filed!

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 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.

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 on attachment 393290 [details] [diff] [review] Refactoring of getTargetFile [Checkin: Comment 21] http://hg.mozilla.org/mozilla-central/rev/f264d3771fe8