Closed Bug 381157 Opened 17 years ago Closed 15 years ago

Make SeaMonkey download manager use toolkit backend

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.0b1

People

(Reporter: kairo, Assigned: Callek)

References

(Blocks 2 open bugs)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(4 files, 16 obsolete files)

513 bytes, patch
neil
: review+
Details | Diff | Splinter Review
594 bytes, patch
Details | Diff | Splinter Review
543 bytes, patch
kairo
: review+
standard8
: superreview+
Details | Diff | Splinter Review
61.18 KB, patch
neil
: review+
Details | Diff | Splinter Review
We have decided to temporarily use xpfe's download manager for suiterunner in bug 348386 - that leaves us with a bunch of suite-ifdefs in toolkit, excluding their download manager stuff.
Once the toolkit backend has been reworked in bug 380250, we should look into building our own UI on that, driving a <tree> by JS in a similar way to how their UI is driven after that bug is fixed.
This should be done without the need of ifdef'ing anything in toolkit though, we can use chrome-level overrides for the UI where/if needed.
Blocks: 336874
Depends on: suiterunner
Blocks: 161783
Depends on: 389982
Depends on: 394502
Blocks: 394502
No longer depends on: 394502
Blocks: 366673
(It seems bug 380250 and the other dependencies were fixed in the meantime,
so the current bug could be worked on now.)
1) Depending on the way of implementation, bug 321172 might be needed for this.
2) We are a volunteer project, so someone needs to volunteer to do this.

Oh, BTW, I've set out a bug bounty for this, see http://www.kairo.at/bugbounty/2007/bmo381157.html
http://developer.mozilla.org/en/docs/nsIDownloadManagerUI is probably a good doc to help whoever wants to work on this.
The UI work needing to be done here blocks L10n builds from fully working, therefore this bug blocks Alpha per http://home.kairo.at/blog/2008-01/seamonkey_2_alpha_criteria
Flags: blocking-seamonkey2.0a1+
Attached patch WIP (obsolete) — Splinter Review
This patch will allow anyone to use the toolkit download manager.

Does not yet enable anything dependant on the toolkit download manager, nor does it update the installer to use it.

May need a clobber build to fully remove the old implementation from being loaded.
Assignee: download-manager → bugspam.Callek
Status: NEW → ASSIGNED
Comment on attachment 308482 [details] [diff] [review]
WIP

>-DIRS += tests
>+DIRS = src \
>+       tests \
>+       $(NULL)
I would prefer to have a \ on the DIRS line, and have a two space indentation and new line for each directory.
Comment on attachment 308482 [details] [diff] [review]
WIP

>Index: xpfe/components/Makefile.in
>===================================================================
> ifneq ($(MOZ_BUILD_APP),camino)
>+ifndef MOZ_SUITE
> DIRS += download-manager
> endif
>+endif

According to http://mxr.mozilla.org/mozilla/source/xpfe/components/Makefile.in#79 this DIRS line nowadays is in a block of
ifdef MOZ_HAVE_BROWSER
ifndef MOZ_PHOENIX
ifndef MOZ_XULRUNNER
ifneq ($(MOZ_BUILD_APP),camino)

I think we have no browser in our build system that is not "phoenix", xulrunner or Camino other than suite, so please just kill download-manager completely - at least from the Makefile (if you don't want to do the removal of the actual code here, file a followup).
Attached patch WIP 2 (obsolete) — Splinter Review
This should address nits from earlier WIP, implements a "nsSuiteDownloadManagerUI", and has installer changes among the sanitize and migration fixes.

Installer and migration has not been tested, and I probably am missing some polish in areas here (such as dropping migration/use of old DLMGR prefs)

Patch as is should enable a working toolkit-style DLMGR with appropriate hooks in SeaMonkey (No pref panel for it though)
Attachment #308482 - Attachment is obsolete: true
(In reply to comment #10)
> Created an attachment (id=310947) [details]
> WIP 2
> 
> This should address nits from earlier WIP, implements a
> "nsSuiteDownloadManagerUI", and has installer changes among the sanitize and
> migration fixes.

The patch seems to work fine here from a first look, including use of the toolkit help app dialog - of course the currently used download manager UI looks very un-SeaMonkey-ish, but even cross-session pause and resume works there :)
Comment on attachment 310947 [details] [diff] [review]
WIP 2

>+pref("browser.download.folderList", 0);
I think we might want this to default to 2, to match our current behaviour.

>+pref("browser.download.useDownloadDir", false);
This appears to be the helper app dialog's equivalent of autoDownload, but I can't work out what it does in toolkit's contentAreaUtils.js :-(
Comment on attachment 310947 [details] [diff] [review]
WIP 2

>-// XXX Bug 381157 When suite uses Toolkit's DM backend, we need to
>-// activate this code.
>-#ifdef SUITE_USING_TOOLKIT_DM
> nsresult
> nsNetscapeProfileMigratorBase::SetDownloadManager(PrefTransform* aTransform,
>                                                   nsIPrefBranch* aBranch)
You might need to tweak this if you manage to implement progress dialogs properly.

>-  // XXX Bug 381157 When suite uses Toolkit's DM backend, we need to
>-  // activate this code.
>-#ifdef SUITE_USING_TOOLKIT_DM
>   MAKESAMETYPEPREFTRANSFORM("browser.download.autoDownload",           Bool),
I think you need to transform this into useDefaultDir?
>   MAKESAMETYPEPREFTRANSFORM("browser.download.lastLocation",           Bool),
I don't think we've ever used this pref. On the other hand we might want to migrate browser.download.dir

>-  // XXX Bug 381157 When suite uses Toolkit's DM backend, we need to
>-  // activate this code.
>-#ifndef SUITE_USING_TOOLKIT_DM
>     "browser.download.",
>-#endif
This is actually ifndef, so it needs to be removed.

>+  Components.classes["@mozilla.org/download-manager-ui;1"].
>+  getService(Components.interfaces.nsIDownloadManagerUI).show(window);
Please write this in suite style.

>Index: suite/common/downloads/nsSuiteDownloadManagerUI.js
Not commenting on this because I expect it to change anyway.
Blocks: 132112
(In reply to comment #12)
> (From update of attachment 310947 [details] [diff] [review])
> >+pref("browser.download.folderList", 0);
> I think we might want this to default to 2, to match our current behaviour.

If we set this to 2 we need a legitimate download location to use, (if the useDownloadDir is set to true). we set that location with .dir; I think this pref is correct.

> >+pref("browser.download.useDownloadDir", false);
> This appears to be the helper app dialog's equivalent of autoDownload, but I
> can't work out what it does in toolkit's contentAreaUtils.js :-(

That does seem to be what it does, but toolkits contentAreaUtils use may take some code-searching to understand, but I don't see any reason to use something different based on a potential mis-use in contentAreaUtils anyway.
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 310947 [details] [diff] [review] [details])
> > >+pref("browser.download.folderList", 0);
> > I think we might want this to default to 2, to match our current behaviour.
> 
> If we set this to 2 we need a legitimate download location to use, (if the
> useDownloadDir is set to true). we set that location with .dir; I think this
> pref is correct.
> 

Actually it "works" if we have it set as '2' but in the absence of the .dir (and .dir existing) it will use the platforms default downloads directory, (and in Win XPs case, it doesn't exist, so it creates a "Downloads" folder on the desktop) -- if this behavior is desired I guess I can flip it, but I think using 0 here and importing the directory-related prefs in a migration of profiles is probably best.
Attached patch WIP 3 (obsolete) — Splinter Review
This is still a WIP but I would appreciate a review from at least Neil.

This patch does not yet implement progress dialoges, but it should get migration right and fix-up some pref uses in the suite itself.

no need for a review on nsSuiteDownloadManagerUI.js yet.
Attachment #310947 - Attachment is obsolete: true
Attachment #318204 - Flags: review?(neil)
Comment on attachment 318204 [details] [diff] [review]
WIP 3

This looks vaguely sane, but I didn't try it out.
Attachment #318204 - Flags: review?(neil)
Blocks: 18004
Blocks: 171142
Blocks: 290117
Justin, (Neil,)
any progress since "WIP 3" patch ?
Serge: Please don't touch bugs with such questions, when their last activity is newer than 6 months or so. Esp. for bugs that have a blocking+ lfag, you can be sure that the Council does track their progress.
Depends on: 440932
Still wanted for Alpha, but not blocking it any more. This blocks final though, probably even beta as it blocks fully localized builds currently.
Flags: blocking-seamonkey2.0a1-
Flags: blocking-seamonkey2.0a1+
Flags: blocking-seamonkey2+
No longer blocks: 366673
Blocks: 389254
WIP 3 long dead...

This is a supplement patch to finish off Bug 440932 I just noticed... (patch is to m-c)
Attachment #318204 - Attachment is obsolete: true
Attachment #342712 - Flags: superreview?(neil)
Attachment #342712 - Flags: review?(neil)
This is the hg-port of the last WIP patch here, to disable building of the toolkit DM UI component.
Attached patch WIP 3.1.2 -- c-c (obsolete) — Splinter Review
This is the barely updated version of WIP 3, hg-based for c-c.

Untested so far, but wanted to get it up here.
Attached patch WIP 3.1.3 -- c-c (obsolete) — Splinter Review
forgot one minor change, and one important change in last posted patch
Attachment #342718 - Attachment is obsolete: true
c:/Programming/Sources/comm-central/suite/profile/migration/src/nsSeamonkeyProfileMigrator.cpp(284) : fatal error C1021: invalid preprocessor command 'ifnfdef'

Fixing locally...
...looks like app-config.mk changes (such as removing this define) do not rebuild "the world", so clobber is needed currently for this patch...

I hope to look into that fact before landing this.
Blocks: 461618
Blocks: 461617
Comment on attachment 342717 [details] [diff] [review]
m-c, don't use nsDownloadManagerUI [checkin c#32]

sdwilsh since Toolkit is tightening, can I get your review here...

Neil, sr?-ing you even though your not a toolkit peer, just to sign off on this conceptual use.
Attachment #342717 - Flags: superreview?(neil)
Attachment #342717 - Flags: review?(sdwilsh)
Comment on attachment 342717 [details] [diff] [review]
m-c, don't use nsDownloadManagerUI [checkin c#32]

rs=me
Attachment #342717 - Flags: review?(sdwilsh)
Attachment #342717 - Flags: approval1.9.1b2?
Comment on attachment 342717 [details] [diff] [review]
m-c, don't use nsDownloadManagerUI [checkin c#32]

Has rs+, Neil's review is a formality.
Attachment #342717 - Flags: superreview?(neil) → superreview+
Comment on attachment 342712 [details] [diff] [review]
m-c followup patch [checkin c#32]

With ifdef as discussed on IRC.
Attachment #342712 - Flags: superreview?(neil)
Attachment #342712 - Flags: superreview+
Attachment #342712 - Flags: review?(neil)
Attachment #342712 - Flags: review+
Comment on attachment 342717 [details] [diff] [review]
m-c, don't use nsDownloadManagerUI [checkin c#32]

cancelling approval request; as sdwilsh asserts this is NPOTDB
Attachment #342717 - Flags: approval1.9.1b2?
Comment on attachment 342712 [details] [diff] [review]
m-c followup patch [checkin c#32]

$ hg outgoing
comparing with ssh://hg.mozilla.org/mozilla-central/
searching for changes
changeset:   21430:0850e0b68e14
user:        Justin Wood <Callek@gmail.com>
date:        Fri Nov 07 19:32:25 2008 -0500
summary:     Bug 381157, Make SeaMonkey download manager use toolkit backend

changeset:   21431:6f6f50b46005
tag:         tip
user:        Justin Wood <Callek@gmail.com>
date:        Fri Nov 07 19:32:41 2008 -0500
summary:     Bug 381157, Make SeaMonkey download manager use toolkit backend

$ hg push
pushing to ssh://hg.mozilla.org/mozilla-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 2 changes to 2 files
Attachment #342712 - Attachment description: m-c followup patch → m-c followup patch [checkin c#32]
Attachment #342717 - Attachment description: WIP 3.1.1 -- m-c → m-c, don't use nsDownloadManagerUI [checkin c#32]
Depends on: 463824
No longer blocks: 389254
Blocks: 472001
QA Contact: download
Blocks: 426742
Flags: blocking-seamonkey2.0b1+
Attached patch WIP 4 (obsolete) — Splinter Review
This works, has the dummy progress dialog. No migration testing done (even though is part of this patch, is not yet complete).

When coupled with KaiRo's UI patch (and appropriate url added here) will [should] load suite-like UI. Note must clobber-build this to work (as it changes a define we need almost everywhere, just |touch autoconf.mk| for us and mozilla/ if your testing as well as code-review.
Attachment #342719 - Attachment is obsolete: true
Attachment #366743 - Flags: superreview?(neil)
Attachment #366743 - Flags: review?(neil)
Attached patch WIP 4.0.1 (correct patch) (obsolete) — Splinter Review
correct patch...
Attachment #366743 - Attachment is obsolete: true
Attachment #366968 - Flags: superreview?(neil)
Attachment #366968 - Flags: review?(neil)
Attachment #366743 - Flags: superreview?(neil)
Attachment #366743 - Flags: review?(neil)
Just a quick first pass. I probably missed a lot of nits.

+pref("browser.download.useDownloadDir", false);
I think we are missing:
  pref("browser.download.manager.resumeOnWakeDelay", 10000);

-pref("browser.downloadmanager.behavior", 0);
+pref("browser.download.manager.behavior", 0);
Presumably you are doing this for consistency?

-#ifdef SUITE_USING_TOOLKIT_DM
-  MAKEPREFTRANSFORM("browser.downloadmanager.behavior", 0, Int,
-                    DownloadManager),
+#ifndef SUITE_USING_XPFE_DM
I don't understand the profile migration code at all but don't we still need to migrate |browser.downloadmanager.behavior| from 1.1 profiles?

+    // 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 we use a js module here to share code?

+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cr = Components.results;
No way Jose!

+    var prefs = Cc["@mozilla.org/preferences-service;1"].
+                getService(Ci.nsIPrefBranch);
Suite reviewers do not like the trailing dots style.
(Several instances)

+    try {
+      flashCount = prefs.getIntPref(PREF_FLASH_COUNT);
+    } catch (e) { }
We control the vertical and the horizontal, so the try block isn't needed.

+    var win = this.recentWindow.QueryInterface(Ci.nsIDOMChromeWindow);
+    win.getAttentionWithCycleCount(flashCount);
+  },
I don't think |win| is reused anywhere in this scope, so perhaps:

this.recentWindow.QueryInterface(Components.iinterfaces.nsIDOMChromeWindow)
    .getAttentionWithCycleCount(flashCount);

+    } catch (e) { /* it's OK to not have a parent window */ }
Put the comment on a separate line.

+let components = [nsDownloadManagerUI];
Several instances of useless use of let. Use vars instead. Top level lets (including the top level inside functions are equivalent to vars and are internally converted to vars by spidermonkey.

+   - The Initial Developer of the Original Code is
+   - Netscape Communications Corp.
Really?

+   - Portions created by the Initial Developer are Copyright (C) 2002
+   - the Initial Developer. All Rights Reserved.
+   -
+   - Contributor(s):
+   -   Scott MacGregor <mscott@netscape.com>
+   -   Bill Law        <law@netscape.com>
+   -   Aaron Kaluszka  <ask@swva.net>
Does any of their code survive in the current stub?

+++ b/suite/installer/windows/packages
+#ifdef SUITE_USING_XPFE_DM
Err, not too sure I understand why this ifdef is introduced?

+<!ENTITY progressTitle        "Download in Progress...">
Please use the UTF-8 ellipse character.
Comment on attachment 366968 [details] [diff] [review]
WIP 4.0.1 (correct patch)

>+  var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>+                          .getService(Components.interfaces.nsIDownloadManager);

nit: I prefer dlmgr or dlMgr as var name, it's reasonably clear, don't you think?
"dnld" is reasonably hard to decipher for me that I don't see how it's better than full "download" or just "dl".
The "aFpP" identifier used in here is already bad enough to decipher, no need to add another brain-twister ;-)

>+EXTRA_COMPONENTS = \
>+  nsSuiteDownloadManagerUI.js \
>+  $(NULL)

So this does peacefully coexist with the toolkit component, or do we still need a twist to keep that one off limits until we go XULRunner?

>+++ b/suite/common/downloads/progress/nsProgressDialog.xul

Any special reason for the "ns" in this? IMHO, we shouldn't use that prefix unless it's an interface implementation or maybe component.

>+  //Ported extensions may only implement the Basic toolkit Interface
>+  //and not our progress dialoges.
>+  try {
>+     Components.classes["@mozilla.org/download-manager-ui;1"]
>+               .getService(Components.interfaces.nsISuiteDownloadManagerUI)
>+               .show(window);

Shouldn't this showManager() actually?
And is there a better way to detect if the interface is implemented by that component than try/catch?

>+     return;
>+  } catch(e) {}
>+  Components.classes["@mozilla.org/download-manager-ui;1"]
>+          .getService(Components.interfaces.nsIDownloadManagerUI)
>+          .show(window);
>+  return;

Shouldn't this be in the catch (and the . correctly lined up)?

>+#ifndef SUITE_USING_XPFE_DM

Do we still need this ifdef? Why not go all the way and drop the old stuff right away?
>--- a/suite/profile/migration/src/nsNetscapeProfileMigratorBase.cpp
>+++ b/suite/profile/migration/src/nsNetscapeProfileMigratorBase.cpp

Of course, we still need fixes here to actually do migration of 1.x prefs, and maybe the UI component or nsSuiteGlue needs to do some migration of already existing 2.x profile data.


Else, it works fine with the toolkit UI from what I see so far (didn't care about progress dialogs for now). Will take it one step further and test with the bug 472001 UI now ;-)
Comment on attachment 366968 [details] [diff] [review]
WIP 4.0.1 (correct patch)

>+const DOWNLOAD_MANAGER_URL = "chrome://mozapps/content/downloads/downloads.xul";

const DOWNLOAD_MANAGER_URL = "chrome://communicator/content/downloads/downloadmanager.xul";

For the bug 472001 UI, just as a note (I think we should land them together anyway).

>+    ww.openWindow(parent,
>+                  DOWNLOAD_MANAGER_URL,
>+                  "Download:Manager",
>+                  "chrome,dialog=no,resizable",

Please make this:
                  "chrome,menubar,toolbar,status,dialog=no,resizable",
(so that the tree-based UI shows everything it needs)



I just realized we might want to go and just disable opening the progress dialogs and try to get this landed ASAP (with migration done and nits fixed), and then do the progress dialogs as a separate step (we should really try and get them done for beta though). Callek, Neil, what do you think about that?
Comment on attachment 366968 [details] [diff] [review]
WIP 4.0.1 (correct patch)

>+++ b/suite/common/downloads/Makefile.in
>+
>+MODULE		= suitecommon
>+XPIDL_MODULE	= suitecommon

https://developer.mozilla.org/En/Installing_headers_using_EXPORTS says:
"For XPIDL-generated headers, you may also set XPIDL_MODULE to determine which typelib file is produced from the IDL files. The distinction between XPIDL_MODULE and MODULE is that each directory with IDL files must have a unique typelib file, but several directories may export headers to the same location."

If we're going with keeping the files in this dir, you need to use a different XPIDL_MODULE.
Depends on: 483054
(In reply to comment #35)
> Just a quick first pass. I probably missed a lot of nits.
> 
> +pref("browser.download.useDownloadDir", false);
> I think we are missing:
>   pref("browser.download.manager.resumeOnWakeDelay", 10000);

Good catch, added.

> -pref("browser.downloadmanager.behavior", 0);
> +pref("browser.download.manager.behavior", 0);
> Presumably you are doing this for consistency?

Exactly.

> -#ifdef SUITE_USING_TOOLKIT_DM
> -  MAKEPREFTRANSFORM("browser.downloadmanager.behavior", 0, Int,
> -                    DownloadManager),
> +#ifndef SUITE_USING_XPFE_DM
> I don't understand the profile migration code at all but don't we still need to
> migrate |browser.downloadmanager.behavior| from 1.1 profiles?

Yea, we need .behavior migrated. (Migration not complete yet, so I'll skip answering those nits).

> +    // 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 we use a js module here to share code?

Sure, but given the places its used, perhaps a 1.9.2 task, for me, sdwilsh or someone else.

> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> No way Jose!

As mentioned elsewhere, I explicitly chose to make this file use the toolkit styling (for now) as I want to be able to copy/paste the toolkit changes in here. But Neil will have final word, but I do strongly feel that one single file (with consistent formatted in that one file) should be ok.

> +    var prefs = Cc["@mozilla.org/preferences-service;1"].
> +                getService(Ci.nsIPrefBranch);
> Suite reviewers do not like the trailing dots style.
> (Several instances)

Unless I missed somewhere, that style should only be in nsSuiteDownloadManagerUI.js and nowhere else.

> +    try {
> +      flashCount = prefs.getIntPref(PREF_FLASH_COUNT);
> +    } catch (e) { }
> We control the vertical and the horizontal, so the try block isn't needed.

I'm not certain what you mean by that comment. prefs.getIntPref throws if it fails. (missing pref)

Most other nits you cited from this file are direct copies from toolkits file, and I prefer to keep it in sync if possible, as it makes ease of future feature enhancements/bug fix porting easier. So I did not address them

> 
> +   - The Initial Developer of the Original Code is
> +   - Netscape Communications Corp.
> Really?
> 
> +   - Portions created by the Initial Developer are Copyright (C) 2002
> +   - the Initial Developer. All Rights Reserved.
> +   -
> +   - Contributor(s):
> +   -   Scott MacGregor <mscott@netscape.com>
> +   -   Bill Law        <law@netscape.com>
> +   -   Aaron Kaluszka  <ask@swva.net>
> Does any of their code survive in the current stub?

Good point, on the contrib's, any pref on "Original Code is"...? (I kept it in tact initially because I had a straight pull of the orig progress xul)

> +++ b/suite/installer/windows/packages
> +#ifdef SUITE_USING_XPFE_DM
> Err, not too sure I understand why this ifdef is introduced?

I suppose no real reason, but if possible, I'd like to leave them for now, and clean them ALL up in a followup bug.

> +<!ENTITY progressTitle        "Download in Progress...">
> Please use the UTF-8 ellipse character.

Sure... (didn't know we were using the utf-8 version in our dtd's now for suite)

(In reply to comment #36)
> (From update of attachment 366968 [details] [diff] [review])
> >+  var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
> >+                          .getService(Components.interfaces.nsIDownloadManager);
> 
> nit: I prefer dlmgr or dlMgr as var name, it's reasonably clear, don't you
> think?

Was a straight copy from toolkit I believe, but no strong pref, fixed locally.

> >+EXTRA_COMPONENTS = \
> >+  nsSuiteDownloadManagerUI.js \
> >+  $(NULL)
> 
> So this does peacefully coexist with the toolkit component, or do we still need
> a twist to keep that one off limits until we go XULRunner?

Answered on IRC, but we don't have a guarantee that it would coexist with the toolkit component, so leaving the toolkit one off-limits until we go XULRunner (m-c patch is still in place to keep it off-limits)

> >+++ b/suite/common/downloads/progress/nsProgressDialog.xul
> 
> Any special reason for the "ns" in this? IMHO, we shouldn't use that prefix
> unless it's an interface implementation or maybe component.

No special reason, just habitual. Deferring to Neil.

> >+  //Ported extensions may only implement the Basic toolkit Interface
> >+  //and not our progress dialoges.
> >+  try {
> >+     Components.classes["@mozilla.org/download-manager-ui;1"]
> >+               .getService(Components.interfaces.nsISuiteDownloadManagerUI)
> >+               .show(window);
> 
> Shouldn't this showManager() actually?

err -- yea. (which is why I had the nsISuite* portion here at all anyway.

> And is there a better way to detect if the interface is implemented by that
> component than try/catch?

This is the best I came up with, have a better idea?

> >+     return;
> >+  } catch(e) {}
> >+  Components.classes["@mozilla.org/download-manager-ui;1"]
> >+          .getService(Components.interfaces.nsIDownloadManagerUI)
> >+          .show(window);
> >+  return;
> 
> Shouldn't this be in the catch (and the . correctly lined up)?

I didn't see a reason to keep the try{}catch{} scoped longer for this. and yes the .'s should be lined up though

> >+#ifndef SUITE_USING_XPFE_DM
> 
> Do we still need this ifdef? Why not go all the way and drop the old stuff
> right away?

Likely not, might as well not add any in this patch, but I'll hold off for final cleanup of those ifdefs for a later ver of patch or followup. [not yet fixed locally]

(In reply to comment #37)
> (From update of attachment 366968 [details] [diff] [review])
> >+const DOWNLOAD_MANAGER_URL = "chrome://mozapps/content/downloads/downloads.xul";
> 
> const DOWNLOAD_MANAGER_URL =
> "chrome://communicator/content/downloads/downloadmanager.xul";
> 
> For the bug 472001 UI, just as a note (I think we should land them together
> anyway).

Thanks

> >+    ww.openWindow(parent,
> >+                  DOWNLOAD_MANAGER_URL,
> >+                  "Download:Manager",
> >+                  "chrome,dialog=no,resizable",
> 
> Please make this:
>                   "chrome,menubar,toolbar,status,dialog=no,resizable",
> (so that the tree-based UI shows everything it needs)

All in all, I was thinking of keeping toolkit UI working for a hidden pref (mostly due to toolkit tests and the fact that itd be easy), I can provide a patch to your bug that makes your stuff work here (and/or an alt patch that keeps toolkit stuff working) -- not fixed locally.

> I just realized we might want to go and just disable opening the progress
> dialogs and try to get this landed ASAP (with migration done and nits fixed),
> and then do the progress dialogs as a separate step (we should really try and
> get them done for beta though). Callek, Neil, what do you think about that?

No big preference, but Neil did say he wanted "something" to pop up, stub or not, until this landed. and needed some sort of suite-based UI (which you satisfied with your bug).

(In reply to comment #38)
Thanks for looking into this, not yet fixed locally (suddenly too tired), but will take care of it asap, as this fix is actually fairly easy.
Comment on attachment 366968 [details] [diff] [review]
WIP 4.0.1 (correct patch)

>+  var prefs = getPrefsBrowserDownload("browser.download.");
>-  var branch = getPrefsBrowserDownload("browser.download.");
Why this change?

>+  var useDownloadDir = prefs.getBoolPref("useDownloadDir");
This always seems to be combined with aSkipPrompt, and not obviously correctly either; it seems to me that this code requires both to be set to skip the prompt, is this the intent? I think what you should do is to update aSkipPrompt from the pref if the caller didn't require it to be skipped.

>+  var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>+                          .getService(Components.interfaces.nsIDownloadManager);
dm/dlmgr/dlMgr

>+  try {                          
>+    var lastDir = prefs.getComplexValue("lastDir", nsILocalFile);
>+    if ((!aSkipPrompt || !useDownloadDir) && lastDir.exists())
>+      dir = lastDir;
>+    else
>+      dir = dnldMgr.userDownloadsDirectory;
>+  } catch(ex) {
>+    dir = dnldMgr.userDownloadsDirectory;
>   }
var dir = dm.userDownloadsDirectory;
if (!aSkipPrompt) try {
  var lastDir = prefs.getComplexValue("lastDir", nsILocalFile);
  if (lastDir.exists())
    dir = lastDir;
} catch (e) {}

>+  if (!aSkipPrompt || !useDownloadDir || !dir || (dir && !dir.exists())) {
>+    if (!dir || (dir && !dir.exists())) {
Don't need to test dir four times.

>+    // 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.
Please move this code "back" into uniqueFile(), and use an early return when we're automatically downloading, as per the old code.

>+XPIDLSRCS = \
>+	nsISuiteDownloadManagerUI.idl \
>+	$(NULL)
>+
>+EXTRA_COMPONENTS = \
>+  nsSuiteDownloadManagerUI.js \
>+  $(NULL)
Put these into suite/common/public and suite/common/src

>+    if (!aReason)
>+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
But REASON_USER_INTERACTED is zero...

>+    var prefs = Cc["@mozilla.org/preferences-service;1"].
>+                getService(Ci.nsIPrefBranch);
...

>+    try {
>+      behavior = prefs.getIntPref(PREF_DM_BEHAVIOR);
>+    } catch (e) { }
Don't bother getting the behaviour for an interactive show.

>+    this.showProgress(aWindowContext, aID, aReason);
Only do this for a behaviour of 1. It might be a nice idea to add behaviour constants to your new .idl file, although at the moment I'm not convinced you need one, see below.

>+    return (null != this.recentWindow);
Don't need the ()s.
Also, I prefer this.recentWindow != null or !!this.recentWindow.

>+    var win = this.recentWindow.QueryInterface(Ci.nsIDOMChromeWindow);
>+    win.getAttentionWithCycleCount(flashCount);
I really hope you don't need the QI. Also, what happens if win is null?

>+    if (!aReason)
>+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
...

>+                  "chrome,dialog=no,resizable",
all,dialog=no

>+  //Ported extensions may only implement the Basic toolkit Interface
>+  //and not our progress dialoges.
Typo: dialogs (also somewhere else)

>+  try {
>+     Components.classes["@mozilla.org/download-manager-ui;1"]
>+               .getService(Components.interfaces.nsISuiteDownloadManagerUI)
>+               .show(window);
>+     return;
>+  } catch(e) {}
>+  Components.classes["@mozilla.org/download-manager-ui;1"]
>+          .getService(Components.interfaces.nsIDownloadManagerUI)
>+          .show(window);
It's the same show method, so we don't actually care if it supports our interface or not.

>+  locale/@AB_CD@/communicator/downloads/progress/nsProgressDialog.dtd       (%chrome/common/downloads/progress/nsProgressDialog.dtd)
I don't think a subdir is warranted for one file.

>+  nsresult rv = NS_OK;
>+
>+  // Seamonkey's download manager used a single pref for last
>+  // prompt directory, and permanent "save here" directory.
>+  aBranch->SetCharPref("browser.download.dir", aTransform->stringValue);
>+  aBranch->SetCharPref("browser.download.lastDir", aTransform->stringValue);
>+  return rv;
Why bother with rv?
I just tried the test suites affected by toolkit/components/downloads/test with this patch and the tree-based UI: all TUnit tests pass, the browser-chrome tests don't fully pass browser_nsIDownloadManagerUI.js (probably as expected):

chrome://mochikit/content/browser/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js
TEST-PASS | chrome://mochikit/content/browser/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js | nsIDownloadManagerUI indicates that the UI is visible
TEST-PASS | chrome://mochikit/content/browser/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js | nsIDownloadManagerUI indicates that the UI is visible
TEST-PASS | chrome://mochikit/content/browser/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js | nsIDownloadManagerUI indicates that the UI is not visible
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js | Exception was caught, as expected - Got false,expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js | Timed out
Blocks: 483241
Depends on: 483781
Blocks: 484397
No longer blocks: 483959
Hmm, you don't seem to migrate the finished_download_alert preference...
(In reply to comment #41)
> I just tried the test suites affected by toolkit/components/downloads/test with
> this patch and the tree-based UI: all TUnit tests pass, the browser-chrome
> tests don't fully pass browser_nsIDownloadManagerUI.js (probably as expected):

Without tree-based UI, all tests pass. TUnit, and Mochi-suite! So good news there at least...
Blocks: 484616
Comment on attachment 366968 [details] [diff] [review]
WIP 4.0.1 (correct patch)

>+++ b/suite/common/downloads/Makefile.in
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2000
>+# the Initial Developer. All Rights Reserved.

Really? I don't think there's anything substantial enough in that Makefile to tell it's based on anything so old.

>+++ b/suite/common/downloads/nsISuiteDownloadManagerUI.idl
>+ * The Original Code is the Firefox Browser Glue Service.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.

Wow, really? What the matching .js has sounds more believable.

>+++ b/suite/common/downloads/progress/nsProgressDialog.xul
>+   - The Original Code is Mozilla Progress Dialog.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Netscape Communications Corp.
>+   - Portions created by the Initial Developer are Copyright (C) 2002
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -   Scott MacGregor <mscott@netscape.com>
>+   -   Bill Law        <law@netscape.com>
>+   -   Aaron Kaluszka  <ask@swva.net>

Is there anything left from the old progress dialog that warrants this?

>+++ b/suite/locales/en-US/chrome/common/downloads/progress/nsProgressDialog.dtd
>+   - The Original Code is the print preview toolbar.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Netscape Communications Corporation.
>+   - Portions created by the Initial Developer are Copyright (C) 2009
>+   - the Initial Developer. All Rights Reserved.

This rather small DTD can reasonably be based on the print preview toolbar, be initially developed by Netscape in 2009 and have you as the original author? Wow.

It's all just nits, but we may want to go with sane license notices for the code we introduce ;-)
Attached patch WIP 5 (obsolete) — Splinter Review
Responses in no particular order...

> >+    this.showProgress(aWindowContext, aID, aReason);
> Only do this for a behaviour of 1. It might be a nice idea to add behaviour
> constants to your new .idl file, although at the moment I'm not convinced you
> need one, see below.

You want what done then if behavior != 0 and != 1? DLMGR instead of progress?

I left the constant definitions out of this patch for now (I think it might be best to do in a followup, after all will be fairly easy to pull off)

> >+    var win = this.recentWindow.QueryInterface(Ci.nsIDOMChromeWindow);
> >+    win.getAttentionWithCycleCount(flashCount);
> I really hope you don't need the QI. Also, what happens if win is null?
this.visible handles the null. You are right though in that we don't seem to need the QI.

> > >+++ b/suite/common/downloads/progress/nsProgressDialog.xul
> > 
> > Any special reason for the "ns" in this? IMHO, we shouldn't use that prefix
> > unless it's an interface implementation or maybe component.

moved this to |suite/common/downloads/progressDialog.*| (moved the l10n one too)

> (From update of attachment 366968 [details] [diff] [review])
> >+  var prefs = getPrefsBrowserDownload("browser.download.");
> >-  var branch = getPrefsBrowserDownload("browser.download.");
> Why this change?

much more understandable than "branch" to me (and consistency with toolkit/)

> >+  var useDownloadDir = prefs.getBoolPref("useDownloadDir");
> This always seems to be combined with aSkipPrompt, and not obviously correctly
> either; it seems to me that this code requires both to be set to skip the
> prompt, is this the intent? I think what you should do is to update aSkipPrompt
> from the pref if the caller didn't require it to be skipped.

Good catch, I went with |var skipPrompt| as I hate modifing [in] values to functions as a general rule. and it is quite clear.

> var dir = dm.userDownloadsDirectory;
> if (!aSkipPrompt) try {
>   var lastDir = prefs.getComplexValue("lastDir", nsILocalFile);
>   if (lastDir.exists())
>     dir = lastDir;
> } catch (e) {}

fixed.

> >+  if (!aSkipPrompt || !useDownloadDir || !dir || (dir && !dir.exists())) {
> >+    if (!dir || (dir && !dir.exists())) {
> Don't need to test dir four times.

We need both sets of ifs here, I fixed this in my own way, verify for me that it is correct please.

> Please move this code "back" into uniqueFile(), and use an early return when
> we're automatically downloading, as per the old code.

Done, after re-reading it I agree it is cleaner.

> >nsSuiteDownloadManagerUI.{js,idl}
> Put these into suite/common/public and suite/common/src

> >+    if (!aReason)
> >+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
> But REASON_USER_INTERACTED is zero...

Toolkit also does this, left-as-is.

> Don't bother getting the behaviour for an interactive show.

fixed.

> >+    if (!aReason)
> >+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
because we also call this method directly... we have to do the same as show()

> >+                  "chrome,dialog=no,resizable",
> all,dialog=no

sure. (should it be done for the progress-stub as well?)

> >+  try {
> >+     Components.classes["@mozilla.org/download-manager-ui;1"]
> >+               .getService(Components.interfaces.nsISuiteDownloadManagerUI)
> >+               .show(window);
> >+     return;
> >+  } catch(e) {}
> >+  Components.classes["@mozilla.org/download-manager-ui;1"]
> >+          .getService(Components.interfaces.nsIDownloadManagerUI)
> >+          .show(window);
> It's the same show method, so we don't actually care if it supports our
> interface or not.
Actually we do care, that was a mistake on my part.

> > And is there a better way to detect if the interface is implemented by that
> > component than try/catch?
> 
> This is the best I came up with, have a better idea?

I could not (easily) come up with a better idea, though there probably is... if you (or someone else) has one, I'll gladly implement it.

----

Migration *still* not done, but this *should* be ready for final review on the rest (if I didn't miss/forget anything)....
Attachment #366968 - Attachment is obsolete: true
Attachment #372455 - Flags: superreview?(neil)
Attachment #372455 - Flags: review?(neil)
Attachment #366968 - Flags: superreview?(neil)
Attachment #366968 - Flags: review?(neil)
(In reply to comment #45)
> > >+    this.showProgress(aWindowContext, aID, aReason);
> > Only do this for a behaviour of 1. It might be a nice idea to add behaviour
> > constants to your new .idl file, although at the moment I'm not convinced you
> > need one, see below.
> You want what done then if behavior != 0 and != 1? DLMGR instead of progress?
Previous behavior was nothing opens (just finished download alert or whatever).

> > >+    if (!aReason)
> > >+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
> > But REASON_USER_INTERACTED is zero...
> Toolkit also does this, left-as-is.
Well I'd like zero-operation code to be removed, please!

> sure. (should it be done for the progress-stub as well?)
No, progress dialogs are different (sorry I don't know the right flags offhand).

> > > And is there a better way to detect if the interface is implemented by that
> > > component than try/catch?
> > This is the best I came up with, have a better idea?
> I could not (easily) come up with a better idea, though there probably is... if
> you (or someone else) has one, I'll gladly implement it.
instanceof
Comment on attachment 372455 [details] [diff] [review]
WIP 5

>WIP 3.1
>NOT FOR REVIEW
;-)

> // 2 and other values, no download manager, no progress dialog. 
;-)

>+  var skipPrompt = (aSkipPrompt && prefs.getBoolPref("useDownloadDir"));
Is this supposed to be || ? I'm assuming here that aSkipPrompt == true means that you do want the prompt to be skipped, and == false means that you want to use the user's preference for whether or not to prompt.

>+  if (!skipPrompt) try {
>+    var lastDir = prefs.getComplexValue("lastDir", nsILocalFile);
>+    if (lastDir.exists()) {
>+      dir = lastDir;
>+      dirExists = true;
>+    }
>+  } catch(e) {}
>+  
>+  if (skipPrompt && dirExists) {
The block above doesn't change skipPrompt so it could come after this one. The old code was different because it only used the one dir.

>+    dir.append(getNormalizedLeafName(aFpP.fileInfo.fileName,
>+                                     aFpP.fileInfo.fileExt));
>+    var file = uniqueFile(dir);
Should use the old code here, no?
By the way, what's the correct behaviour when none of the dirs exist but we want to skip the prompt?

>   fp.defaultExtension = aFpP.fileInfo.fileExt;
>   fp.defaultString = getNormalizedLeafName(aFpP.fileInfo.fileName,
>                                            aFpP.fileInfo.fileExt);
>   appendFiltersForContentType(fp, aFpP.contentType, aFpP.fileInfo.fileExt,
>                               aFpP.saveMode);
> 
>+  if (dir)
>+    fp.displayDirectory = dir;
Isn't dir guaranteed to exist by now? If so, move it back up above.

>-  if (aFpP.isDocument) 
>-    branch.setIntPref("save_converter_index", fp.filterIndex);
What was wrong with s/branch/prefs/ ?

>+  var directory = fp.file.parent.QueryInterface(nsILocalFile);
>+  prefs.setComplexValue("lastDir", nsILocalFile, directory);
There's a bug for updating the pref pane, right?

>+      aLocalFile.leafName = aLocalFile.leafName.replace(/^(.*\()\d+\)/, "$1" + (collisionCount+1) + ")");
Nit: spaces in (collisionCount + 1)

>-   content/communicator/pref/pref-applications-edit.xul             (pref/pref-applications-edit.xul)
Remember to hg remove these on commit.

> 	nsISuiteGlue.idl \
> 	nsISessionStartup.idl \
> 	nsISessionStore.idl \
>+    nsISuiteDownloadManagerUI.idl \
Nit: indentation.
(git-apply also complained about trailing whitespace on 18 lines)

> 	nsAboutSessionRestore.js \
> 	nsSessionStartup.js \
> 	nsSuiteGlue.js \
>+    nsSuiteDownloadManagerUI.js \
Nit: indentation, and D < G.

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;
You don't even use Cr :-P

>+        var prefs = Cc["@mozilla.org/preferences-service;1"].
>+              getService(Ci.nsIPrefBranch);
And then you get these ugly wrapped lines of course.

>+    let flashCount = 2;
var

>+  //Ported extensions may only implement the Basic toolkit Interface
>+  //and not our progress dialogs.
>+  try {
>+     Components.classes["@mozilla.org/download-manager-ui;1"]
>+               .getService(Components.interfaces.nsISuiteDownloadManagerUI)
>+               .showManager(window);
>+     return;
>+  } catch(e) {}
>+  Components.classes["@mozilla.org/download-manager-ui;1"]
>+            .getService(Components.interfaces.nsIDownloadManagerUI)
>+            .show(window);
I still don't see the difference between calling show and showManager when the reason is defaulting to USER_INTERACED.

Not looked at profile migration.
No longer blocks: 474152
Depends on: 474152
Depends on: 485187
Blocks: 377688
Attachment #374902 - Flags: superreview?(bugzilla)
Attachment #374902 - Flags: review?
Attachment #374902 - Flags: review? → review?(kairo)
Attached file Planning Document for pref migration (obsolete) —
This document is a rough outline that I used as my planning doc for pref migration, for migration _process_ concerns please use this, for code formatting concerns please use the DLMGR patch coming.
Attachment #374903 - Flags: superreview?(neil)
Attachment #374903 - Flags: review?(kairo)
Attached patch v1.0 (obsolete) — Splinter Review
this should be good for review!
Attachment #372455 - Attachment is obsolete: true
Attachment #374904 - Flags: superreview?(neil)
Attachment #374904 - Flags: review?(neil)
Attachment #372455 - Flags: superreview?(neil)
Attachment #372455 - Flags: review?(neil)
Blocks: 490464
Blocks: 490467
Comment on attachment 374902 [details] [diff] [review]
make app-config generate dependancy (checked in)

Looks good for now, but I think we should move to making app-config be a wildcarded dep globally in the long term.
Attachment #374902 - Flags: review?(kairo) → review+
Comment on attachment 374903 [details]
Planning Document for pref migration

I agree with all your assessments in that pref migration lists, even though some of the toolkit prefs acting on the dlmgr UI probably are not implemented in my UI patch so far, we probably should care to implement them in followups.
Attachment #374903 - Flags: review?(kairo) → review+
(In reply to comment #50)
> Created an attachment (id=374904) [details]
> v1.0
> 
> this should be good for review!

Doesn't this patch remove all the 1.x to 2 migration code? Are you now looking at doing it all in one place as per pref migration list?
(In reply to comment #53)
> (In reply to comment #50)
> > Created an attachment (id=374904) [details] [details]
> > v1.0
> > 
> > this should be good for review!
> 
> Doesn't this patch remove all the 1.x to 2 migration code? Are you now looking
> at doing it all in one place as per pref migration list?

I hope to do it initially in the Cpp stuff, I still migrate the entire browser.download.* branch (and .downloadmanager) I just handle it all well in the .js for now (for existing nightly users/alpha users).

What I have here makes the migration code itself non-blocking.
Comment on attachment 374903 [details]
Planning Document for pref migration

>* browser.download.folderList
>    Indicates the location users wish to save downloaded files too.  
>      Values: 
>        0 - The desktop is the default download location. 
>        1 - The system's downloads folder is the default download location. 
>        2 - The default download location is elsewhere as specified in browser.download.dir. If invalid, userDownloadsDirectory will fallback on defaultDownloadsDirectory.
>    -- Migration: If (browser.download.dir).exists() && (browser.download.autoDownload==true) set to 2; otherwise leave as default.
Why can't we always set this to 2? If there's no dir it will just fall back.

>* browser.download.lastDir:
>    When prompting uses this as default display DIR.
>    -- migrate from browser.download.dir if (browser.download.lastLocation==true)
>       otherwise set to "".
No, this should always be migrated; lastLocation just specifies whether a new download should update the pref.

>* browser.download.manager.alertOnEXEOpen
>    Should we warn the user when launching an exe (potentially harmful)
>    file. [?Controls Adding metadata to file for security reasons]
>    -- identical to !browser.download.progressDnlgDialog.dontAskForLaunch; No plans to migrate this due to its added feature[s], and easy for consumer to deactivate.
Whoa, it can deactivate the alert on Vista? Sounds dangerous. Agreed!

>* browser.download.manager.closeWhenDone
>    When the DownloadManager is auto-opened, should it be auto-closed.
>    Firefox migrates this based on our .behavior of 1 (progress dialog
>    similar behavior).
>    -- Migrate when browser.download.manager.behavior==1 and base it off
>       (!browser.download.progressDnldDialog.keepAlive)
I don't think this should be migrated at all. The only reason Firefox migrates it is because it doesn't have progress dialogs.

>* browser.download.manager.showAlertOnComplete
>    Should we show an alert (notification) when a download completes.
>    Theoretically identical to browser.download.finished_download_alert
>     but less invasive than what that pref controls.
So how exactly does it differ?

>* browser.download.manager.showWhenStarting
>    Show the DLMGR (or progress) when starting a download.
>    -- Migrate based on (bool)(browser.downloadmanager.behavior < 2)
Unnecessary, because if the behaviour is > 1 then we won't open anything.
I didn't notice much of comment #47 being addressed...

>+    if (aReason === undefined)
Worse than if (!aReason)

>+    if (behavior == 0) {
switch (behavior) { ?

The latest patch also forgot to remove pref-applications-edit.xul
Attachment #374902 - Flags: superreview?(bugzilla) → superreview+
Comment on attachment 374902 [details] [diff] [review]
make app-config generate dependancy (checked in)

Pushed the app-config patch as http://hg.mozilla.org/comm-central/rev/3109eb20dad1
Attachment #374902 - Attachment description: make app-config generate dependancy → make app-config generate dependancy (checked in)
Comment on attachment 374904 [details] [diff] [review]
v1.0

These comments supercede all of my previous comments (especially the incorrect ones ;-) ) except those for the migration planning document.

>-pref("browser.downloadmanager.behavior", 1);
>+//XXXCallek uncomment following line for when we implement progress dialoges
>+//pref("browser.download.manager.behavior", 1);
Use some other way of commenting out that doesn't change this line.

>-
>-  try {
>-    if (dir.exists())
>-      fp.displayDirectory = dir;
>-  } catch (e) {
>-  }
> 
>   fp.defaultExtension = aFpP.fileInfo.fileExt;
>   fp.defaultString = getNormalizedLeafName(aFpP.fileInfo.fileName,
>@@ -574,9 +591,12 @@ function poseFilePicker(aFpP)
>   appendFiltersForContentType(fp, aFpP.contentType, aFpP.fileInfo.fileExt,
>                               aFpP.saveMode);
> 
>+  if (dir)
>+    fp.displayDirectory = dir;
Nit: don't need to move this, also probably don't need to check dir

>-  if (aFpP.isDocument) 
>-    branch.setIntPref("save_converter_index", fp.filterIndex);
>+  var directory = fp.file.parent.QueryInterface(nsILocalFile);
>+  prefs.setComplexValue("lastDir", nsILocalFile, directory);
> 
>-  // Now that the user has had a chance to change the directory and/or filename,
>-  // re-read those values...
>-  if (branch.getBoolPref("lastLocation") || autoDownload) {
>-    var directory = fp.file.parent.QueryInterface(nsILocalFile);
>-    branch.setComplexValue(kDownloadDirPref, nsILocalFile, directory);
>-  }
>   fp.file.leafName = validateFileName(fp.file.leafName);
>-
>   aFpP.saveAsType = fp.filterIndex;
>   aFpP.file = fp.file;
>   aFpP.fileURL = fp.fileURL;
>+
>+  if (aFpP.isDocument)
>+    prefs.setIntPref("save_converter_index", aFpP.saveAsType);
Nit: don't need to move this

>deleted file mode 100644
My bad, it must have moved to a different place in the patch or something

>+        var prefs = Cc["@mozilla.org/preferences-service;1"].
>+              getService(Ci.nsIPrefBranch);
Nit: wrong indentation

>+    if (behavior == 0) {
>+      this.showManager(aWindowContext, aID, aReason);
>+      return;
>+    }
>+
>+    if (behavior == 1) {
>+      this.showProgress(aWindowContext, aID, aReason);
>+      return;
>+    }
Nit: use a switch?

>+      throw Cr.NS_ERROR_UNEXPECTED;
Oh, my bad again, you did use Cr.

>+    if (aReason === undefined)
>+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
This is even worse than your previous version...

>+    var params = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
Nit: different style to the rest of the file.
Also, might be worth having a separate function to populate the params.

>+                  "Download:Manager",
This is wrong.

>+    //aId, aWindowContext
???

>+    this._updatePrefs();
I haven't looked at this code, instead I commented on the planning document.

>+  } else {
>+    //instanceof Components.interfaces.nsIDownloadManagerUI
You can't assume this, you've only got an nsISupports... but it would work to getService(Components.interfaces.nsIDownloadManagerUI) since all download managers would implement that.
Attached patch v1.01 (obsolete) — Splinter Review
I took the liberty and addressed the review comments from Comment 58:
- Changed the way the pref is commented out
- Moved fp.displayDirectory = dir; back, removed if(dir) check
- Moved prefs.setIntPref("save_converter_index" ... back
- Fixed indentation
- Uses switch
- Removed if (aReason === undefined) code
- Fixed style in the "var params" line
- Replaced "Download:Manager" with "null" in the nsIWindowWatcher.openWindow call (chrome windows usually have no id)
- Removed useless(?) comment
- Changed getService() call to to getService(Components.interfaces.nsIDownloadManagerUI)
Attachment #374904 - Attachment is obsolete: true
Attachment #378104 - Flags: superreview?(neil)
Attachment #378104 - Flags: review?(neil)
Attachment #374904 - Flags: superreview?(neil)
Attachment #374904 - Flags: review?(neil)
Comment on attachment 378104 [details] [diff] [review]
v1.01

>+//XXXCallek progressDialogs and sound is not implemented in new DLMGR yet
Nit: s/is/are/

>+/* XXXCallek uncomment following line for when we implement progress dialoges
Nit: dialogs

>+  var skipPrompt = (aSkipPrompt && prefs.getBoolPref("useDownloadDir"));
I still don't think this is right.

>+  var dirExists = (dir && dir.exists());
Nit: don't need quite so many ()s (also somewhere else)

>-
>   aFpP.saveAsType = fp.filterIndex;
>   aFpP.file = fp.file;
>   aFpP.fileURL = fp.fileURL;
>+
Nit: blank line moved unnecessarily

>+    if (aReason === undefined)
>+      aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
Why is this still here?

>+        this.showProgress(aWindowContext, aID, aReason); 
Nit: trailing whitespace

>+  var dlUI = Components.classes["@mozilla.org/download-manager-ui;1"]
>+                       .getService(Components.interfaces.nsIDownloadManagerUI);
>+  if (dlUI instanceof Components.interfaces.nsISuiteDownloadManagerUI) {
>+    dlUI.showManager(window);
>+  } else {
>+    //instanceof Components.interfaces.nsIDownloadManagerUI
Don't need this comment now.
(In reply to comment #47)
> >+  var skipPrompt = (aSkipPrompt && prefs.getBoolPref("useDownloadDir"));
> Is this supposed to be || ? I'm assuming here that aSkipPrompt == true means
> that you do want the prompt to be skipped, and == false means that you want to
> use the user's preference for whether or not to prompt.

No, this is correct, we only care about the value of "useDownloadDir" if our code paths dictate we have a chance of skipping the prompt, and should only skip if the pref is true && we WANT to [try] skipping.

> 
> >+    dir.append(getNormalizedLeafName(aFpP.fileInfo.fileName,
> >+                                     aFpP.fileInfo.fileExt));
> >+    var file = uniqueFile(dir);
> Should use the old code here, no?
Top of my head not sure :(
> By the way, what's the correct behaviour when none of the dirs exist but we
> want to skip the prompt?

We prompt, the code is smart enough to set a default prompting dir to go from. If the place we want to save to doesn't exist its better to save than create


> >   fp.defaultExtension = aFpP.fileInfo.fileExt;
> >   fp.defaultString = getNormalizedLeafName(aFpP.fileInfo.fileName,
> >                                            aFpP.fileInfo.fileExt);
> >   appendFiltersForContentType(fp, aFpP.contentType, aFpP.fileInfo.fileExt,
> >                               aFpP.saveMode);
> > 
> >+  if (dir)
> >+    fp.displayDirectory = dir;
> Isn't dir guaranteed to exist by now? If so, move it back up above.

I don't think it was guaranteed... but if you need a why I'll have to answer another day.

> >-  if (aFpP.isDocument) 
> >-    branch.setIntPref("save_converter_index", fp.filterIndex);
> What was wrong with s/branch/prefs/ ?
Nothing I guess.
(In reply to comment #61)
> (In reply to comment #47)
> > >+  var skipPrompt = (aSkipPrompt && prefs.getBoolPref("useDownloadDir"));
> > Is this supposed to be || ? I'm assuming here that aSkipPrompt == true means
> > that you do want the prompt to be skipped, and == false means that you want to
> > use the user's preference for whether or not to prompt.
> No, this is correct, we only care about the value of "useDownloadDir" if our
> code paths dictate we have a chance of skipping the prompt, and should only
> skip if the pref is true && we WANT to [try] skipping.
aSkipPrompt is new, and you haven't fixed the callers, which means that we never check the pref, which is a regression.
OK, so I did some Firefox CVS archaeology and it turns out that aSkipPrompt doesn't really mean what it says, it means what Callek said it means, which is best described as aAllowSkipPrompt.

Unlike the Firefox code, our code works on the basis that the normal thing to do is to allow the prompt to be skipped; Firefox only allows it to be skipped in some cases, e.g. dragging a link to the download manager.

So, an acceptable patch would either have to not implement aAllowSkipPrompt, or fix up all of our existing callers that expect that user's pref to be honoured.
Attached patch v1.02 (obsolete) — Splinter Review
ok, so:
- Fixed the nits
- I basically deleted the aSkipPrompt var from the code, so I took the way of not implementing this
- Moved empty line back
- Deleted the aReason line
- Deleted trailing whitespace
- Always set browser.download.folderList to 2
- Always migrate browser.download.lastDir
- Left the code for migrating "browser.download.manager.showWhenStarting" as is because if browser.downloadmanager.behavior is undefined, xpconnect will convert this to false as far as I know?
- Removed the migration of the browser.download.manager.closeWhenDone pref
Attachment #379772 - Flags: superreview?(neil)
Attachment #379772 - Flags: review?(neil)
Attached patch v1.02 (obsolete) — Splinter Review
Correct one.
Attachment #378104 - Attachment is obsolete: true
Attachment #379772 - Attachment is obsolete: true
Attachment #378104 - Flags: superreview?(neil)
Attachment #378104 - Flags: review?(neil)
Attachment #379772 - Flags: superreview?(neil)
Attachment #379772 - Flags: review?(neil)
Attachment #379773 - Flags: superreview?(neil)
Attachment #379773 - Flags: review?(neil)
Comment on attachment 379773 [details] [diff] [review]
v1.02

> function internalSave(aURL, aDocument, aDefaultFileName, aContentDisposition,
>                       aContentType, aShouldBypassCache, aFilePickerTitleKey,
>-                      aChosenData, aReferrer)
>+                      aChosenData, aReferrer, aSkipPrompt)
Forgot to remove this change ;-)

>-    if (!poseFilePicker(fpParams))
>+    if (!getTargetFile(fpParams)
Missing )
Comment on attachment 379773 [details] [diff] [review]
v1.02

>+//XXXCallek progressDialogs and sound are not implemented in new DLMGR yet

leave out progress dialogs here, as we'll check them in right away with this patch, now that they have reviews.

>-pref("browser.downloadmanager.behavior", 1);
>+/* XXXCallek uncomment following line for when we implement progress dialogs
>+pref("browser.download.manager.behavior", 1);
>+*/

Same here, just leave out the comment and make it live, progress dialogs/windows will land right away bundled with this patch.
btw: I noticed in Bug 483241 the patch uses the browser.download.manager.closeWhenDone pref to determinate if the progress dialog should be closed after the download is done. So maybe we should migrate the browser.download.progressDnldDialog.keepAlive pref (or use another pref name in Bug 483241)?
Comment on attachment 379773 [details] [diff] [review]
v1.02

>+    // Ensure that even if we don't end up migrating to a lastDir that we don't attempt another
>+    // update. This will throw when QI'ed to nsILocalFile, but it does fallback gracefully.
>+    prefs.setCharPref("browser.download.lastDir", "");
[Could just do this in the null-check case below perhaps?]

>+    var autoDownload = this._tryGetPref(prefs, 'bool',
>+                                        'browser.download.autoDownload', false);
Not used?

>+    prefs.setIntPref("browser.download.folderList", 2);
Move this to browser-prefs.js

>+    var lastLocation = this._tryGetPref(prefs, 'bool',
>+                                        'browser.download.lastLocation', true);
Not used?

>+    prefs.setComplexValue("browser.download.lastDir",
>+                          Components.interfaces.nsILocalFile,
>+                          dir);
Don't you have to null-check dir?

>+    }
???

>+    var showWhenStarting = (behavior === undefined) || (behavior < 2);
>+    prefs.setBoolPref("browser.download.manager.showWhenStarting", showWhenStarting);
I don't see that we need to set this, since the behaviour already covers it.

>+  _tryGetPref: function(prefBranch, type, pref, fallback)
I'm not sure that this isn't overkill...
(In reply to comment #68)
> btw: I noticed in Bug 483241 the patch uses the
> browser.download.manager.closeWhenDone pref to determinate if the progress
> dialog should be closed after the download is done. So maybe we should migrate
> the browser.download.progressDnldDialog.keepAlive pref (or use another pref
> name in Bug 483241)?

.closeWhenDone will probably affect either progress dialog or download manager (or both), depending on what's open (usually what .behavior says), but I haven't implemented it for the dlmgr UI yet. Because that's a change, we might not want to migrate it, but I'll leave that up to Neil.

I have filed bug 495048 though on inspection of your patch to remove the progress dialogs prefs from browser-prefs.js (I want to avoid any possible delay in landing those dlmgr patches, better clean up stuff in followups).
Comment on attachment 379773 [details] [diff] [review]
v1.02

>new file mode 100644
>--- /dev/null
>+++ b/suite/common/public/nsISuiteDownloadManagerUI.idl

I just did a local package-compare and realized that we need to care to add this to the packages files, please include that in this patch.
Comment on attachment 379773 [details] [diff] [review]
v1.02

>--- a/suite/common/src/nsSuiteGlue.js
>+++ b/suite/common/src/nsSuiteGlue.js
[...]
>+    }

Now I understand why Neil choked on this one - see what my build says:

Error: missing } after property list
Source File: file:///opt/seamonkey-dbg/components/nsSuiteGlue.js
Line: 294, Column: 4
Source Code:
    prefs.setBoolPref("browser.download.useDownloadDir", autoDownload);
Attached patch v1.03 (obsolete) — Splinter Review
Ok, this patch should be of better quality than the last one:
- Fixed the } and the )
- Removed the aSkipPrompt param
- Removed comment around pref("browser.download.manager.behavior", 1);
- Removed unused autoDownload and lastLocation vars
- Moved prefs.setIntPref("browser.download.folderList", 2); to browser-prefs.js
- Added null check for dir again
- Removed prefs.setBoolPref("browser.download.manager.showWhenStarting", showWhenStarting);
- Left _tryGetPref as is, I think it's ok
Attachment #379773 - Attachment is obsolete: true
Attachment #379964 - Flags: superreview?(neil)
Attachment #379964 - Flags: review?(neil)
Attachment #379773 - Flags: superreview?(neil)
Attachment #379773 - Flags: review?(neil)
(In reply to comment #69)
> (From update of attachment 379773 [details] [diff] [review])
> >+    var autoDownload = this._tryGetPref(prefs, 'bool',
> >+                                        'browser.download.autoDownload', false);
> Not used?
Oops, used. But not here...

(In reply to comment #70)
> (In reply to comment #68)
> > btw: I noticed in Bug 483241 the patch uses the
> > browser.download.manager.closeWhenDone pref to determinate if the progress
> > dialog should be closed after the download is done. So maybe we should migrate
> > the browser.download.progressDnldDialog.keepAlive pref (or use another pref
> > name in Bug 483241)?
> .closeWhenDone will probably affect either progress dialog or download manager
> (or both), depending on what's open (usually what .behavior says), but I
> haven't implemented it for the dlmgr UI yet. Because that's a change, we might
> not want to migrate it, but I'll leave that up to Neil.
It depends on whether you intend to make closeWhenDone apply to the download manager, in which case we want the progress dialog to use the old pref, otherwise you can choose to use the "new" pref for the progress dialog but then you would have to migrate it now since we only have one chance to migrate.
Comment on attachment 379964 [details] [diff] [review]
v1.03

>+    var PREF_INVALID = Components.interfaces.nsIPrefBranch.PREF_INVALID;
Nit: Just noticed this should be const ;-)

>+    prefs.setBoolPref("browser.download.useDownloadDir", autoDownload);
Oops, we use this after all...

>+    var behavior = this._tryGetPref(prefs, 'int', 'browser.downloadmanager.behavior');
>+    if (behavior !== undefined)
>+      prefs.setIntPref("browser.download.manager.behavior", behavior);
My point is that you could easily write this as
try {
  prefs.setIntPref("browser.download.manager.behavior",
      prefs.getIntPref("browser.downloadmanager.behavior"));
} catch (e) {}
Attached patch v1.04 (obsolete) — Splinter Review
Fixed the issues mentioned.
Re Comment 55:
">* browser.download.manager.showAlertOnComplete
>    Should we show an alert (notification) when a download completes.
>    Theoretically identical to browser.download.finished_download_alert
>     but less invasive than what that pref controls.
So how exactly does it differ?"

As far as I see browser.download.manager.showAlertOnComplete only displays one download complete alert every n seconds as defined in browser.download.manager.showAlertInterval. Other than that I do not see any differences that matter.
Attachment #379964 - Attachment is obsolete: true
Attachment #380187 - Flags: superreview?(neil)
Attachment #380187 - Flags: review?(neil)
Attachment #379964 - Flags: superreview?(neil)
Attachment #379964 - Flags: review?(neil)
(In reply to comment #76)
> Created an attachment (id=380187) [details]
> v1.04
> 
> Fixed the issues mentioned.
> Re Comment 55:
> ">* browser.download.manager.showAlertOnComplete
> >    Should we show an alert (notification) when a download completes.
> >    Theoretically identical to browser.download.finished_download_alert
> >     but less invasive than what that pref controls.
> So how exactly does it differ?"
> 
> As far as I see browser.download.manager.showAlertOnComplete only displays one
> download complete alert every n seconds as defined in
> browser.download.manager.showAlertInterval. Other than that I do not see any
> differences that matter.

XPFE shows an alert dialog per download. toolkit will show an alert every n seconds yes, but uses the growl/windows "balloon" if possible instead, which is much less invasive.
Attached patch v1.05 (obsolete) — Splinter Review
Remove _tryGetPref
Attachment #380187 - Attachment is obsolete: true
Attachment #380237 - Flags: superreview?(neil)
Attachment #380237 - Flags: review?(neil)
Attachment #380187 - Flags: superreview?(neil)
Attachment #380187 - Flags: review?(neil)
Attached patch v1.06Splinter Review
Changed the way browser.download.lastDir is set
Attachment #380237 - Attachment is obsolete: true
Attachment #380254 - Flags: superreview?(neil)
Attachment #380254 - Flags: review?(neil)
Attachment #380237 - Flags: superreview?(neil)
Attachment #380237 - Flags: review?(neil)
Comment on attachment 380254 [details] [diff] [review]
v1.06

>+      prefs.setCharPref("browser.download.lastDir", "");
I wasn't expecting you to remove the comment...
Attachment #380254 - Flags: superreview?(neil)
Attachment #380254 - Flags: superreview+
Attachment #380254 - Flags: review?(neil)
Attachment #380254 - Flags: review+
Attachment #374903 - Attachment is obsolete: true
Attachment #374903 - Flags: superreview?(neil)
Comment on attachment 380254 [details] [diff] [review]
v1.06

Will check in this patch and the whole stack of already reviewed patches depending on this in about 12 hours when I'm fully awake and able to watch all trees go through full cycles (that hopefully end up green or passing as expected).
Pushed as http://hg.mozilla.org/comm-central/rev/178d054e3163 - that makes this fixed, please file followups on any issues that arise.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1
Comment on attachment 380254 [details] [diff] [review]
v1.06

>diff --git a/suite/common/src/nsSuiteGlue.js b/suite/common/src/nsSuiteGlue.js
>--- a/suite/common/src/nsSuiteGlue.js
>+++ b/suite/common/src/nsSuiteGlue.js
>@@ -126,6 +126,8 @@
>+    try {
>+      prefs.setBoolPref("browser.download.useDownloadDir",
>+                        prefs.getBoolPref("browser.download.autoDownload");

Missing )
(In reply to comment #85)
> Missing )

Oops. Pushed as http://hg.mozilla.org/comm-central/rev/b50fee7ba253 (I consider that a bustage fix that doesn't need explicit review).
I just started the first download with toolkit download manager landed thereby it seems SeaMonkey tried to download all files again that where listed in my downloads.rdf. But no file was really downloaded. Maybe a side-effect of transferring the data from downloads.rdf to downloads.sqlite?
(In reply to comment #87)
> I just started the first download with toolkit download manager landed thereby
> it seems SeaMonkey tried to download all files again that where listed in my
> downloads.rdf. But no file was really downloaded. Maybe a side-effect of
> transferring the data from downloads.rdf to downloads.sqlite?

What do you mean with "it seems SeaMonkey tried to download all files again"? Could you tell us what you actually saw instead of what I think it could have done behind the scenes? It's surely true that the data is imported and the downloads.rdf didn't have all the fields so it's defining a number of fields to some convenient defaults.
Sorry for being so imprecise. 
I could trigger this once again by deleting downloads.sqlite. Starting the first download with no downloads.sqlite I got for each download in downloads.rdf an error message:
"x:\...\file.zip could not be saved, because an unknown error occurred.
Try saving to a different location."
The location exists and there is enough space to save the files but no file is saved. After clicking through all the error boxes with the error message finally the download manager opens and I see the list with all my downloads that were stored in downloads.rdf marked as finished/failed or canceled.
In the error console there is this error listed "Error: too much recursion" for probably each download.
(In reply to comment #89)
> Sorry for being so imprecise. 
> I could trigger this once again by deleting downloads.sqlite. Starting the
> first download with no downloads.sqlite I got for each download in
> downloads.rdf an error message:
> "x:\...\file.zip could not be saved, because an unknown error occurred.
> Try saving to a different location."

Strange, I haven't seen that before. Could you please file a new bug, dependent on this one? This might be a toolkit problem. Please copy the full entry from the error console (one of them) to that new report.
(In reply to comment #58)
> >+      throw Cr.NS_ERROR_UNEXPECTED;
> Oh, my bad again, you did use Cr.

Someone didn't see this when addressing all the comments. I re-added Cr per this comment in this push: http://hg.mozilla.org/comm-central/rev/68bd3360bcef
Blocks: 495604
No longer blocks: 495629
Depends on: 495629
No longer blocks: 495604
Depends on: 495604
No longer blocks: 290117
No longer blocks: 461618
Blocks: 495680
(In reply to comment #90)

> Strange, I haven't seen that before. Could you please file a new bug, dependent
> on this one? This might be a toolkit problem. Please copy the full entry from
> the error console (one of them) to that new report.

Bug 495680
Blocks: 495692
Comment on attachment 380254 [details] [diff] [review]
v1.06

>-# XXX: remove line below once we have a helper app dialog that doesn't need it (comes with toolkit download manager)
>-  locale/@AB_CD@/communicator/pref/pref-applications-edit.dtd               (%chrome/common/pref/pref-applications-edit.dtd)

Is there a reason, why this file hasn't been removed from the locales/... folder?
(In reply to comment #93)
> (From update of attachment 380254 [details] [diff] [review])
> >-# XXX: remove line below once we have a helper app dialog that doesn't need it (comes with toolkit download manager)
> >-  locale/@AB_CD@/communicator/pref/pref-applications-edit.dtd               (%chrome/common/pref/pref-applications-edit.dtd)
> 
> Is there a reason, why this file hasn't been removed from the locales/...
> folder?

Sounds like an oversight, can you file a followup for it?
Blocks: 496343
Blocks: 502363
(In reply to comment #94)
> (In reply to comment #93)
> > Is there a reason, why this file hasn't been removed from the locales/...
> > folder?
> 
> Sounds like an oversight, can you file a followup for it?

I filed bug 496343.
Add fixed-seamonkey2.0 to make it go away from "unfixed blockers" query
VERIFIED to make it go away from "Most Frequently Reported Bugs" query
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.