Shawn Wilsher :sdwilsh Reporter
	Description • 14 years ago

Implement the panel UI instead of the window pop-up we currently have in the browser.  This involves overriding the toolkit nsIDownloadManagerUI component (implemented in nsDownloadManagerUI.js) and having it show a custom UI.  Might want to see how Fennec does this (http://mxr.mozilla.org/mobile-browser/source/components/DownloadManagerUI.js) as Firefox will need to do something similar.

	Shawn Wilsher :sdwilsh Reporter
	Comment 1 • 14 years ago

Mehdi is going to be working on other stuff, so if someone else wants to take this, feel free to do so!

	Jason Sonnenschein
	Comment 3 • 14 years ago

I attached javascript to the ticket for some canvas animations that seem to match what Limi was talking about for a new UI.  It draws a badge with number of current downloads in the center and the badge itself working as a round progress bar for total % downloaded.  It then pulses twice when all downloads are complete.  View a demo here: http://dl.dropbox.com/u/1357257/canvas_test.html

	Jason Sonnenschein
	Comment 4 • 14 years ago

OK.  I have a working addon that more or less implements the Limi UI.  There's a badge that's a circular progress bar of overall download percentage (all current downloading files.)  There's a number in the center for how many downloads are taking place.  

Hovering the mouse over the badge shows a simple progress bar and ETA for each downloading file.  Clicking on the badge brings up the full downloads window.

When a single download completes, the percentage and number of active downloads updates appropriately.  When the last download stops, the badge blinks twice if it was a successful download.  It just disappears immediately for failure or a pause/cancel by the user.  

If any of the downloads don't inform the browser of the file size before downloading, there's a different animation.  Three triangles spin around the circle, rather than a growing progress bar.  I'm having trouble finding a file server to test this with, though.

	Jason Sonnenschein
	Comment 7 • 14 years ago

Fixed some layout problems in MS Windows

	Jason Sonnenschein
	Comment 8 • 14 years ago

OK... last update for the night... more layout fixes.

	Shawn Wilsher :sdwilsh Reporter
	Comment 9 • 14 years ago

Nobody is planning on working on this Jason, so do you want to try and create the patch and get it landed for Firefox 4?  Awesome work so far!

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 14 years ago

(In reply to comment #8)
> Created attachment 459304 [details]
> Addon implementation of download badge UI
> 
> OK... last update for the night... more layout fixes.

All of the navigation bar's buttons are suddenly really tall with this add-on enabled for me in Windows 7 on Firefox 4 nightlies.
I haven't checked to see if it's a conflict with some other extension yet, but I thought I'd report this now before I forget it overnight.

	Jason Sonnenschein
	Comment 11 • 14 years ago

@Shawn, making this into a patch might not be too hard, but there's some bugs I don't know how to fix that are mainly related to what I used as a container for the xul code.

A XUL panel contains a row with the status of each download.  The only way I could get the panel to work was to put all the xul objects inside a button with type "panel."   There's some problems with that.  

First of all, it makes the ugly box you see.  There's no reason for that box... the badge itself could just hang out in the menubar if I could get rid of the button.  It might also fix the ugliness that Wes is seeing, because the button is larger than the canvas inside it.  

Also, the "click to bring up downloads window" code breaks in Firefox 4 (it requires a click for the button or window to get focus, then a second click to bring up the downloads window)... it's probably related to using a button and making it activate on hover, rather than click, and something stealing focus when the panel is showing.  Does anyone here have enough XUL experience to suggest an alternative to a button with type "panel" that triggers a XUL panel?

A second problem:  I'm not sure where to get internationalized strings for download time remaining.  Can anyone point me in the right direction?  I might be able to figure this one out on my own.

I _think_ I can figure out how to make this a patch instead of an addon, but I'll need the help I described above.

	Jason Sonnenschein
	Comment 12 • 14 years ago

Oh, and can anyone point me to a page where I can download something and the server doesn't report to me the file size at download time? (so the browser knows how many bytes of the file it has, but not percentage completed)

	Jason Sonnenschein
	Comment 13 • 14 years ago

I found the international strings.  I would argue that some new ones are needed.  There's no string for  "Click badge for Downloads window" and nothing anywhere near like that string... not even "Downloads window" as a phrase.

It would be nice to have abbreviated strings for hours and minutes in each language.  

The current code would require the strings for "about a minute" or "less than a minute" (though I admit I could change the code.)

	Shawn Wilsher :sdwilsh Reporter
	Comment 14 • 14 years ago

(In reply to comment #11)
> @Shawn, making this into a patch might not be too hard, but there's some bugs I
> don't know how to fix that are mainly related to what I used as a container for
> the xul code.
> 
> A XUL panel contains a row with the status of each download.  The only way I
> could get the panel to work was to put all the xul objects inside a button with
> type "panel."   There's some problems with that.
Neil is the one you want to ask xul:panel questions too.
  
(In reply to comment #12)
> Oh, and can anyone point me to a page where I can download something and the
> server doesn't report to me the file size at download time? (so the browser
> knows how many bytes of the file it has, but not percentage completed)
Most linux ISOs seem to do this in my experience.  Bonus is that it is long running.  If you make a patch, I think we have tests that you can copy to get the desired look as well.

(In reply to comment #13)
> I found the international strings.  I would argue that some new ones are
> needed.  There's no string for  "Click badge for Downloads window" and nothing
> anywhere near like that string... not even "Downloads window" as a phrase.
That's fine.  Strings are easy to add.

> It would be nice to have abbreviated strings for hours and minutes in each
> language.
hmm, do they need to be abbreviated?

> The current code would require the strings for "about a minute" or "less than a
> minute" (though I admit I could change the code.)
We already have code that handles that for us.  See http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm

	Jason Sonnenschein
	Comment 15 • 14 years ago

> > It would be nice to have abbreviated strings for hours and minutes in each
> > language.
> hmm, do they need to be abbreviated?

Abbreviations aren't strictly necessary, but making the panel smaller than the full downloads window seems logical to me, and the only ways I can think to do that are shorter ETA strings and abbreviating the download filename with an ellipsis (I haven't coded the ellipsis, but I was planning to.)  Would you rather a bigger panel, but full filename and no abbreviations in the ETA strings?  Also... on your comment later about already having code for "time remaining" strings... same arguments... I can reuse that code or create something new with more compact ETA strings.

> Most linux ISOs seem to do this in my experience. 

Thanks for the tip.

> Neil is the one you want to ask xul:panel questions to

How do I contact Neil, or is he Cc'ed on this bug?

	Shawn Wilsher :sdwilsh Reporter
	Comment 16 • 14 years ago

(In reply to comment #15)
> Abbreviations aren't strictly necessary, but making the panel smaller than the
> full downloads window seems logical to me, and the only ways I can think to do
> that are shorter ETA strings and abbreviating the download filename with an
> ellipsis (I haven't coded the ellipsis, but I was planning to.)  Would you
> rather a bigger panel, but full filename and no abbreviations in the ETA
> strings?  Also... on your comment later about already having code for "time
> remaining" strings... same arguments... I can reuse that code or create
> something new with more compact ETA strings.
I would suspect that we wouldn't want to abbreviate things, but we should get input from UX.

> How do I contact Neil, or is he Cc'ed on this bug?
I cc'd him to the bug.

	Jason Sonnenschein
	Comment 17 • 14 years ago

One other thought.  Instead of ellipsis, we could let a long filename wrap to a second line

	Jason Sonnenschein
	Comment 19 • 14 years ago

No longer displaying badge inside a button... this should fix the Windows 7/Ffx-4 visual glitches, fixes the problems with displaying panel on hover, and fixes problems clicking on the badge to get the downloads window in Ffx-4.

This version creates shorter ETA strings than the Firefox downloads window, but does so using il8n strings already contained in Firefox.

Other misc. formatting cleanup.

The string "Click badge to see Download Window" is still hard-coded.

At this point, it should be code-complete to be converted from an addon to a patch

	Jason Sonnenschein
	Comment 20 • 14 years ago

My mistake... problem with having to double-click on the badge in ffx4 persists.  Not sure why.

	Jason Sonnenschein
	Comment 21 • 14 years ago

Neil, can you offer suggestions?  

In firefox 4, after show panel is run, the parent element doesn't receive the next mouse click.  the user has to click the parent element twice for it to receive the click.  This doesn't happen in firefox 3.6.  Is there some javascript I can run after the show panel to fix this.  using focus() on the parent element doesn't help.  Nor does creating a mouse-click event via createEvent and dispatchEvent.

	Neil Deakin
	Comment 22 • 14 years ago

If you provide the code you are using (in plain text), and describe what you are doing, I can provide more help.

Are you trying to have something happen when an anchor button is pressed while the popup is open? The image seems to imply this. There is a way to do this, but the UI would made a lot more sense if it was just a button or link that was labeled 'Open Downloads' rather than giving instructions on some other thing to do. (which are unclear since most people don't know what a badge is in this context).

	Jason Sonnenschein
	Comment 23 • 14 years ago

Yes, I was trying to let them click on the badge when the panel is open.  I understand your point about "open badge" being less intuitive (wording could easily be changed if that's your concern,) but I also thought that there's no reason for them to move the mouse to the bottom of a list of downloads... they can click where their mouse is already hovering.  If we do things the way you suggest, there's no new il8n strings necessary, but the code about keeping the panel open gets more complex (right now there's only a panel when the user hovers over the badge.)  It has to stay open when the mouse is over the panel.  Any suggestions on how to write that part?  It must be somewhere in Firefox, already.

	Jason Sonnenschein
	Comment 24 • 14 years ago

Why not just do both?  Change the message to "click here for Downloads Window" at the bottom of the panel, and also allow the user to click on the progress graph itself.  Is there any reason to _not_ allow the user to click on the graph to bring up the downloads window?

	Jason Sonnenschein
	Comment 25 • 14 years ago

I had been developing on Firefox 3.6.  Playing with Firefox-4, I see your point.  I'm recoding to follow the lead of the web page identity and bookmarks panels.  I'm trying to copy their CSS, and it will be a click to bring up/hide the details of the individual downloads, rather than hovering to see it.  So I've also changed the last line in the panel to be the internationalized "click here to see downloads window" that is already in firefox.

I'm working now to re-implement as a patch.  We'll see how it goes.  I added preferences and font for the colors used in the progress bar and the number of downloads.  I really do believe that some people would want a compact popup panel, so I added a preference for this too... It makes a shortened ETA string (still internationalized) and sets width for the columns explicitly.  It adds very little to the size of the code base.

	Jason Sonnenschein
	Comment 26 • 14 years ago

Includes some CSS and minor visual improvements, fully internationalized, adds a visual warning to the user when a download fails.  Who should this be submitted to for review?

	Jason Sonnenschein
	Comment 27 • 14 years ago

Some syntax errors fixed and better use of objects over individual variables

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 28 • 14 years ago

I'm not sure I'll be able to get to this review request in the near future. Can you find someone else more familiar with the download manager UI code to do a first-pass review?

	Jason Sonnenschein
	Comment 29 • 14 years ago

OK.  Changed reviewer

	Shawn Wilsher :sdwilsh Reporter
	Comment 30 • 14 years ago

(In reply to comment #28)
> I'm not sure I'll be able to get to this review request in the near future. Can
> you find someone else more familiar with the download manager UI code to do a
> first-pass review?
That would likely be me, but my review queue is already large and it'd be at least two weeks before I could get to it.  Maybe Mardak?

Jason, you should also posts screenshots of various states and have limi give UI review on them.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 31 • 14 years ago

In at least the extension form of the button, right-clicking on the download button brings up the downloads window. I'd argue that it should bring up the nav bar's context menu, like every other button in the nav bar (except for the Feedback button...).

	Jason Sonnenschein
	Comment 33 • 14 years ago

Fix for middle/right click issues reported by Wes

	Shawn Wilsher :sdwilsh Reporter
	Comment 34 • 14 years ago

One thing this patch is missing is tests.  The current download manager has a number of tests that you'll likely want to duplicate for this UI:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/

	Jason Sonnenschein
	Comment 36 • 14 years ago

Sorry Shawn. I don't have the time to write all the tests you want.  Someone else will have to take over this bug if these are necessary for inclusion in Firefox 4.

	Ed Lee :Mardak
	Comment 37 • 14 years ago

How does this code relate to the existing download manager window? Did you take a look at how downloads.js (for the existing download manager window) works?

In particular, there was a comment in the patch re: unknown/undefined file sizes when updating the title/summary. The current window handles it this way:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.js#425

Is there a spec somewhere of how things should behave? For example, there was a bit about showing the infinity symbol:

>+        else if (activeDownloads>99) // too long to display, and unlikely to occur
>+            activeDownloads="\u221E";

General style comments that would make it easier to read in the context of other mozilla code:
- spaces around binary operators like =, +, ==, !=, ..
- spaces after commas
- {} instead of new Object()
- let instead of var
- setTimeout with function callbacks instead of javascript strings

I see that you're scoping things like DownloadBadge.canvas.{element,context,width,etc.}. It might make the code easier to follow if those were explicitly declared with canvas: { element: ... }.

Also, there's some prefs defined. Are those to simplify development or they're really for end users to tweak vs having the styles be platform specific?

	Jason Sonnenschein
	Comment 38 • 14 years ago

> How does this code relate to the existing download manager window? Did you take
> a look at how downloads.js (for the existing download manager window) works?

I started with the animations and added the data gathering via public API's from the downloadmanager as public API's are what I use when I'm writing an add-on for firefox. No, I'm afraid I didn't look at downloads.js. 

> In particular, there was a comment in the patch re: unknown/undefined file
> sizes when updating the title/summary.

Ah, I can do that in the next patch version.

> Is there a spec somewhere of how things should behave? For example, there was a
> bit about showing the infinity symbol:

I don't know.  I'm open to ideas of alternatives to the infinity symbol.   To increase the number of digits that could be displayed, you'd have to increase size of the canvas, which might then overflow the toolbar and force other elements to get their height stretched by XUL drawing routines (Wes already complained of this in an early version).  Or I suppose you could have the badge not be circular but oval... wider than tall... but I think that's an ugly looking solution.  How often will users have 99 downloads that are active?  Any alternatives you all can think of?

> General style comments

Points taken, I'm a hobby coder.  Sorry for the lack of knowledge, but what's the difference between let and var?  The only thing I've noticed so far is that if I use let inside an if/then statement, the variable fails one level up (which is a pain IMO).

> Also, there's some prefs defined. Are those to simplify development or they're
> really for end users to tweak vs having the styles be platform specific?

My thoughts were that they were for end users to tweak.  I haven't written any firefox skins, but I assumed that preferences for the colors used, perhaps the font as well... might be useful to people who like skinning the browser (or at least they could access them by hand.)  And the "compactView" setting makes the panel much smaller.  As the colors and fonts are used in drawing routines  for a canvas, css can't alter them... so to make them accessible by the user, I thought that preferences were the only method available.

I had one other thought about animation inconsistencies that I've re-coded for the next patch version, if no one objects.  Having the red warning pulse animation for a download failure at any time, but the green pulse for a successful download only when all downloads have finished seemed inconsistent.  So I thought it would be better for a pulsing animation to happen whenever a file completes... success or failure.  Opinions?

	Jason Sonnenschein
	Comment 39 • 14 years ago

Shows code updates (new patch coming) to insert file icon and adjust other data placement

	Mehdi Mulani [:mmm] (I don't check this)
	Comment 41 • 14 years ago

(In reply to comment #40)
> Created attachment 460413 [details]
> For UI Review by Limi

Hey Jason,

great job on your work so far, really awesome that someone is picking this up. :)

I did a quick scan through of your code yesterday and noticed that if there were more than 99 items, the panel would display the infinity symbol. Definitely a funny behaviour but I had a quick chat with Limi about it and he said that it would make sense to display something like "99+". He also said that it might be reasonable to make the threshold 10-20 items.

	Jason Sonnenschein
	Comment 42 • 14 years ago

Hi Mehdi.  Thanks for the comments.  Unfortunately, 99+ doesn't solve the problem.  99+ (or even 20+) puts us at three characters, which is the space issue I'm trying to avoid.  Other suggestions that might take less horizontal space?

	Jason Sonnenschein
	Comment 43 • 14 years ago

Implements most requests of edilee@mozilla.com (cleaning up code readability, etc.) Also found a race condition and coded a fix.  Adds file icons to the downloads window.

	Jason Sonnenschein
	Comment 44 • 14 years ago

I'm bowing out of owning the bug.  I don't have the time or the skills to implement Limi's far-reaching spec.  I just had time to work on the visual badge.  Downloads history and replacing the "what should I do?" dialog with a notification bar will have to fall to someone else.

	:Paolo Amadini Assignee
	Comment 45 • 14 years ago

Hello Jason,

thank you very much for having developed this prototype implementation!
I applied the patch on my computer and tried it out, and this was helpful
to get a sense of how things should work.

Then I agree that going from extension-style code to polished code that can
be included in core is a long way, even for good code like yours. There
are a lot of different details to care about, from interoperability to
accessibility to platform integration, just to name a few.

I think I can try and implement the new UI, but I think I'll approach the
problem form another angle and start by reusing existing core code from
"downloads.js" to create the panel. I'm not particularly skilled with
styling and canvas, thus I'll postpone the work on the actual badge and
progress indicator to the final phases.

I'm not sure if we'll be able to get this feature into Firefox 4.0, as only
a few weeks are available to us, but we can give it a try in any case.

	:Paolo Amadini Assignee
	Comment 46 • 14 years ago

This first experiment shows the Download Manager functionality moved to a
panel that can be displayed by clicking on a toolbar button.

The two differences with the Download Manager window are:

 1. There is no "Clear List" button, but every item has a remove icon
     (clicking the X button repeatedly will eventually remove all
     downloads; the same can be achieved using first the "Select All"
     and then the "Remove from List" context menu options).

 2. There is no search box. Searching completed downloads is expected to be
     implemented in the "Download History" window or tab, that can be opened
     using the button with the same name.

In the screenshot I also mocked up a "open" button whose purpose is to make
clear what is the next action expected on a completed download. Also an
"open parent folder" icon should probably be present.

Clicking the "open" button generally shifts the operating system's focus
to another window, and this closes the popup. I think this is OK. I'm not
sure on what should be done with the download item after "open" is clicked,
if we should remove the item, mark it as "used" by reducing its size, or
leave it alone waiting for the user to remove it manually.

When there are no downloads in the panel (the normal "rest" condition), only
the "Downloads History" button is visible, and the panel can be opened anyway.
I tried with a solution in which the panel couldn't be opened at all in that
case, but I found it counter-intuitive as I was unable to get to the downloads
history in the same way after having removed the last download.

	:Paolo Amadini Assignee
	Comment 47 • 14 years ago

This patch can be useful if you want to experiment with the concept
interactively. It is just a copy of the Download Manager code to the
browser content folder, with the minimum number of modifications
required to mock up the expected functionality.

Some notes:
 * When there are many downloads, the panel expands all the way to the
    bottom of the screen, and a scrollbar appears, correctly. However,
    on Windows XP, the panel overlaps with the taskbar and the taskbar
    is on top of it, effectively hiding the "Downloads History" button.
    Ideas on how to fix this?
 * The "Download History" button currently opens the standalone Download
    Manager instead of the downloads history window.
 * Do we want the download panel to be "detachable"? If, by clicking on a
    button or icon in the panel, you can open the standalone Download
    Manager, then you have a way to keep an eye on every single item of
    your current downloads. I'm not sure if we want to continue to support
    this use case.

	imradyurrad
	Comment 48 • 14 years ago

What milestone is this targeted for? FF 4.0 code freeze is coming up rather fast, and it'd be nice to get this out for testing purposes.

	:Paolo Amadini Assignee
	Comment 49 • 14 years ago

(In reply to comment #48)
> What milestone is this targeted for? FF 4.0 code freeze is coming up rather
> fast, and it'd be nice to get this out for testing purposes.

Since I should be able to work on this full time next week, I expect the new
panel UI to be ready in time for Beta 5, provided that I can get reviews for
code and user interface.

The exact mechanics of the feature depend on whether other changes, like the
download history changes in bug 564900, are also completed at the same time,
but those are not really blockers for moving the DM to a panel.

	Paul [pwd]
	Comment 50 • 14 years ago

In the mockups I saw, you could get full download history up in a tab, will this bug cover that also?

	Shawn Wilsher :sdwilsh Reporter
	Comment 51 • 14 years ago

(In reply to comment #50)
> In the mockups I saw, you could get full download history up in a tab, will
> this bug cover that also?
No, because the Library isn't moving in time for Firefox 4.0

	:Paolo Amadini Assignee
	Comment 52 • 14 years ago

Time for feedback! :-) This screenshot from an automated test case displays
one download item for every possible download state. The items are styled
after the new Add-ons Manager. The idea is that if the layout is more or
less OK, work for finalizing the visual style can continue in parallel with
the refinement of the exact dynamic behavior of each UI element (like whether
opening the file hides the download or not, or whether there is an "undo
remove from list" context menu item to resurrect the last download).

	Michael Monreal [:micmon]
	Comment 53 • 14 years ago

>> The items are styled after the new Add-ons Manager

I actually wondered if the style for downloads in the addons manager was final already. Personally, I really feel that those stop/resume/etc icons are WAY to small. I have trouble getting the meaning of the resume icon at my current display resolution (1280x1024) and I am sure noone will get it beyond 1600x1200. Also, remember that people are supposed to click those tiny 8x8 pixel (?) targets.

	:Paolo Amadini Assignee
	Comment 54 • 14 years ago

(In reply to comment #53)
> I actually wondered if the style for downloads in the addons manager was final
> already. Personally, I really feel that those stop/resume/etc icons are WAY to
> small. I have trouble getting the meaning of the resume icon at my current
> display resolution (1280x1024) and I am sure noone will get it beyond
> 1600x1200. Also, remember that people are supposed to click those tiny 8x8
> pixel (?) targets.

Good point, making the click targets larger and adjusting the icons
accordingly can be an improvement over the current Download Manager
"mini buttons".

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 55 • 14 years ago

(In reply to comment #53)
> >> The items are styled after the new Add-ons Manager
> 
> I actually wondered if the style for downloads in the addons manager was final
> already.

No, it's not final - the visual are being worked on now (for everything). We don't yet have any final mockups for the download progress, but I'm told the main difference will just be in the colors used.

	:Paolo Amadini Assignee
	Comment 56 • 14 years ago

At this point the code is more or less in its final form, except for the
second part of "downloads.js", that still requires to be namespaced. A few
important details need to be worked out, however.

The patch is based on the Download Manager files (revision 4a50f3c34d5a),
but since most of the code had to be namespaced, it is probably easier to
apply the patch and look at the resulting files rather than the diff.

	Alex Limi (:limi) — Firefox UX Team
	Comment 57 • 14 years ago

(In reply to comment #55)
> (In reply to comment #53)
> > >> The items are styled after the new Add-ons Manager
> > 
> > I actually wondered if the style for downloads in the addons manager was final
> > already.
> 
> No, it's not final - the visual are being worked on now (for everything). We
> don't yet have any final mockups for the download progress, but I'm told the
> main difference will just be in the colors used.

We'll work on the visual design soon, but this might bleed into the "polish" phase (ie. after beta5), we should focus on getting the interactions and functionality right.

There's a lot of polish missing on the (notification) panels, and that will improve the looks of this very much.

Here's a screenshot of the visual design of the panel:
http://www.stephenhorlander.com/images/blog-posts/theme-update-2010-04-20/panel-download-win7.png (note that the progress bar might change style-wise, and we aim to have the same styling in the add-ons manager and download manager, whether they technically use the same code or not).

	Alex Limi (:limi) — Firefox UX Team
	Comment 58 • 14 years ago

Comment on attachment 466688 [details]
Screenshot of all download types

The close buttons should be removed, and the list of downloads should be cleared on exit.

Looking good apart from that issue, with the understanding that we'll make the panels and content prettier in the polish phase.

	Ed Lee :Mardak
	Comment 59 • 14 years ago

Comment on attachment 467089 [details] [diff] [review]
Mostly working (Windows theme only)

I see that you've copied a bunch of files from toolkit's downloads and converted them into a browser.xul overlay. So the toolbar download icon will show the panel with a button showAllDownloads that opens up Tools:Downloads. There'll be quite a bit of duplicate code to drive similar ui.

Are we supposed to have both accessible? It seems kinda odd to have this new panel view that looks and behaves like the old one.

One issue code-structure-wise is that the toolkit implementation is used by other apps like SeaMonkey and Thunderbird, so the toolkit implementation needs to stay stand-alone functional without the new panel.

>+++ b/browser/base/content/browser.xul
>+             defaultset="unified-back-forward-button,reload-button,stop-button,home-button,urlbar-container,search-container,bookmarks-menu-button-container,downloads-button,navigator-throbber,fullscreenflex,window-controls"
There'll need to be some migration code for the new download button to show up for users that have changed from the default set.

>+++ b/browser/components/downloads/DownloadCommon.jsm
What is this common code supposed to be shared with? Right now it'll only be used for the Firefox download panel?

>+const kDownloadsStringBundleUrl =
>+      "chrome://browser/locale/downloads/downloads.properties";
You've copied the file from the toolkit version and removed some strings, so there'll be duplicate entries. Why does it need to be in a separate file vs reusing the existing toolkit ones?

I know that l10n prefers separate string entities for different uses, but I'm not sure if showing yesterday=Yesterday in the download window is different from showing it in a panel.

	Ed Lee :Mardak
	Comment 60 • 14 years ago

sdwilsh: Any suggestions for how the code should be structured to drive both the Firefox panel and toolkit download window?

I suppose the panel could land with its own copies of logic and strings, but it would be good to refactor things so that only the ui/view-specific code are "duplicated" and shared as a js module living in toolkit.

	Shawn Wilsher :sdwilsh Reporter
	Comment 61 • 14 years ago

(In reply to comment #60)
> sdwilsh: Any suggestions for how the code should be structured to drive both
> the Firefox panel and toolkit download window?
this sounds good to me:
> I suppose the panel could land with its own copies of logic and strings, but it
> would be good to refactor things so that only the ui/view-specific code are
> "duplicated" and shared as a js module living in toolkit.
although I agree with your comments about the strings in comment 59 and we probably shouldn't duplicate them.

	Alex Limi (:limi) — Firefox UX Team
	Comment 62 • 14 years ago

(In reply to comment #60)
> sdwilsh: Any suggestions for how the code should be structured to drive both
> the Firefox panel and toolkit download window?

If by "toolkit download window, you mean this:
http://people.mozilla.com/~madhava/blog/2008-05-26/download_manager.png

…then no, we're not supposed to have that one anymore.

If you're talking about the History view (which mmmulani has a patch that adds Downloads to the sidebar for), that's how it's supposed to work.

	Ed Lee :Mardak
	Comment 63 • 14 years ago

(In reply to comment #62)
> …then no, we're not supposed to have that one anymore.
Oh, then I suppose Tools -> Downloads will go to that History/Library view when it does land? Right now I believe the patch will trigger Tools -> Downloads when you click show Downloads History at the bottom of the panel.

sdwilsh: Even if we keep the two code paths separate (browser/components/downloads vs toolkit/mozapps/downloads) should prefs be shared? Also would we actively want to not package chrome://mozapps/content/downloads/ ?

Oh and what should the the shortcut for downloads trigger? Activate the panel or History view?

	Paul [pwd]
	Comment 64 • 14 years ago

(In reply to comment #62)
> If you're talking about the History view (which mmmulani has a patch that adds
> Downloads to the sidebar for), that's how it's supposed to work.

I thought I remembered seeing a mock-up of download history in a tab, but have been unable to locate it upon a quick check. Personally I don't use sidebars, I find them ugly and cumbersome, will there be a pref to open download history in a tab or will users be forced to use the sidebar?

	:Paolo Amadini Assignee
	Comment 65 • 14 years ago

Thank you for all the feedback! I'm thrilled to see this taking shape!

I'll try to work out the best solutions together with you, from the
perspectives of code management and interaction design separately.
For now I'll leave aside the visual design as I understand that this
will be finalized at a later time.

I'll answer each point as best I can, but firstly I took some time
to gather my ideas about why I was doing things in a certain way,
and share with you my reasoning. In my experience I found out
that it's much easier to talk about features and details when
the motivations for their existence is clear, forming a common
reference. I think this summarizing work is useful for this
reason, whether the features will be deemed relevant or not,
although I hope there is still room for a contribution in the
area of interaction design.

I considered a specific use case, that I believe is common, from
start to finish: downloading and running a setup program. There
are probably other use cases for which the same considerations
may apply.

For this use case, I compared the current workflow with what I think
that a new workflow, based on Limi's design, could look like. This
is scoped to what is feasible in the limited time until Beta 5.

	Alex Limi (:limi) — Firefox UX Team
	Comment 66 • 14 years ago

(In reply to comment #64)
> I thought I remembered seeing a mock-up of download history in a tab, but have
> been unable to locate it upon a quick check. Personally I don't use sidebars, I
> find them ugly and cumbersome, will there be a pref to open download history in
> a tab or will users be forced to use the sidebar?

The patch for showing downloads in History is bug 564900 (it's listed as a dependency of this bug).

You should be able to have History in a tab eventually, but for 4.0 it'll probably still be in a window. Post 4.0, we'll invest some time in improving and rethinking history/bookmarks and related UI.

	:Paolo Amadini Assignee
	Comment 67 • 14 years ago

Here is the current workflow, with default preferences:

 1. Click on a download link on a website.
 2. Confirm download.
 3. The download window appears on top.
 4. If the download takes time, return to the browser window and
    continue browsing while the file is being downloaded.
 5. A download completed notification appears briefly. Generally
    it disappears before you can click on the hyperlink.
 6. Find the downloads window in the operating system's taskbar.
 7. ?

This is where I think there is no obvious hint on what to do next.
You can double-click the file name to continue.

 7. Double-click the file name to continue.
 8. Confirm execution, and (not shown) run setup wizard.
 9. Continue browsing, but before that, remove download.

Although it can be argued that there is no point in removing a
download from the list, and I might as well leave it as it is,
I feel much more comfortable if I can hide the "used up"
downloads. This is because, for me, the downloads list view is
a transient view that is primarily used for current tasks.

Not removing the download is like leaving a tab open when I'm
finished with it - in fact, why close the tab at all? ;-)

This is why I regard point 9 as an integral part of this use case.

	:Paolo Amadini Assignee
	Comment 68 • 14 years ago

Here are two proposals for a new workflow:

 1. Click on a download link on a website. (same as before)
 2. Confirm download. (same as before - there is a plan to remove
    this confirmation, but it's outside of the scope of this bug)
 3. Download indicator flashes, and download panel opens. (the panel
    might open only the first time, or this behavior might be
    controlled by a preference)

I can continue browsing just by dismissing the panel.

 4. Download indicator becomes "highlighted". This indicates that
    there are new downloads for which an action can be performed.
    Click the indicator at any time to open the panel. (the panel
    might also open automatically, or an "icon style" notification
    may appear, or this behavior might be controlled by a preference)
 5. Click "Open" to continue. (a dropdown menu is available if you
    want to perform different non-default actions, like in the new
    "notifications" interface).
 6. Confirm execution and run setup wizard. (same as before - the
    confirmation is asked for by the operating system in some cases)

At this point I see two alternatives.

In solution (A), I'm done! The download is truly "notification
style", that is, the notification disappears on exit. In this way,
when I return to the browser after running the setup wizard, the
downloads indicator is not "highlighted" anymore, and I can
continue browsing without worrying about the download.

If something went wrong with the setup there is an "undo" button
(might look differently from the mockup) to display the last
download again. This is analogous to the "undo close tab" use case.

In solution (B), I can reopen the panel and rest assured that
I will find it in the same state as when it closed itself. If I
want, I can remove the download using a natural interaction (the
left mouse button on the X). If I change my mind, I can resurrect
the item with the undo button (I think this operation should be
first-class, like for tabs, without requiring the history
window to be opened).

Another interesting property of the "close" button is that it can
easily be used to hide multiple downloads by repeated clicking.
For the close buttons of tabs, an effort was recently made to
achieve this property. It is already there in this case.

Comments on these ideas are very welcome!

	:Paolo Amadini Assignee
	Comment 69 • 14 years ago

(In reply to comment #59)
> Comment on attachment 467089 [details] [diff] [review]
> 
> >+++ b/browser/base/content/browser.xul
> >+             defaultset="unified-back-forward-button,reload-button,stop-button,home-button,urlbar-container,search-container,bookmarks-menu-button-container,downloads-button,navigator-throbber,fullscreenflex,window-controls"
> There'll need to be some migration code for the new download button to show up
> for users that have changed from the default set.

Ah, I hoped this was handled by the XUL toolbar. Is there already some
code that does that for other cases?

> >+++ b/browser/components/downloads/DownloadCommon.jsm
> What is this common code supposed to be shared with? Right now it'll only be
> used for the Firefox download panel?

Since it is a JS module, its data (localized strings for now) is shared
between all the browser window instances, while the other objects have
an instance for each window.

As only one panel is visible at any one time, I had a half idea of
having a single populated panel and move it "in block" to another window,
if at all possible, instead of having each window re-query the same
downloads. When doing this, I would move the data access object to the
JS module. But this is only an optimization, and maybe not even feasible,
so I'm not considering it as a priority at this point.

> >+const kDownloadsStringBundleUrl =
> >+      "chrome://browser/locale/downloads/downloads.properties";
> You've copied the file from the toolkit version and removed some strings, so
> there'll be duplicate entries. Why does it need to be in a separate file vs
> reusing the existing toolkit ones?
> 
> I know that l10n prefers separate string entities for different uses, but I'm
> not sure if showing yesterday=Yesterday in the download window is different
> from showing it in a panel.

Localized strings are one of the points that I was unsure about. It's
exactly as you say, some strings have to be different because the interface
is in a different place (for example, I've already reworded a few English
strings for space reasons). But for other strings, like the "open
executable warning", it might make sense to share them.

But in this case, I think it would not be so difficult to move the entire
interactive confirmation function to a shared place in Toolkit, like
DownloadUtils.jsm. Shawn, what do you think?

	:Paolo Amadini Assignee
	Comment 70 • 14 years ago

(In reply to comment #61)
> (In reply to comment #60)
> > sdwilsh: Any suggestions for how the code should be structured to drive both
> > the Firefox panel and toolkit download window?
> this sounds good to me:

I'm not sure about what you're suggesting exactly, but what I'm trying to do
now is to create separate "blocks" (the singleton objects you can already see)
for various parts of the code, like data access, to make it easier to move
them around.

What I'm trying to achieve is to make it easy (and fast) for this bug to
land independently from Toolkit, and then when the panel is polished and
the interactions are finalized, it should be very easy to replace most of
Toolkit's code with the new one. At that point we could also decide which
blocks to migrate, that is if we want to keep the old interactions for
Toolkit or if we want the DM to simply become a window that hosts the
new panel UI overlay, moved to Toolkit in its entirety.

	:Paolo Amadini Assignee
	Comment 71 • 14 years ago

(In reply to comment #63)
> Oh and what should the the shortcut for downloads trigger? Activate the panel
> or History view?

I'm not sure about this one. I'd say the panel ("manage" current downloads),
with maybe a separate command in the History menu?

I think this was the last unanswered point from the above comments, I've
finished spamming for now ;-) Let me know if you have any other concern!

	:Paolo Amadini Assignee
	Comment 72 • 14 years ago

This patch appears to work correctly (Windows theme only), and most of the
code should be in its final form, except for the global functions in the
second part of "downloads.js". The finished portions are ready for review;
the idea is that after review those parts won't change very much and the
subsequent enhancements can be in turn reviewed using interdiff.

Still missing from this version:
 * Always opening the panel instead of the Download Manager
 * Correct behavior of the Downloads History button
 * Migration code for having the download icon or badge visible by default
 * Share the "open download" function, or at least its strings, with Toolkit
 * Copy the Windows theme to the other theme folders

	:Paolo Amadini Assignee
	Comment 73 • 14 years ago

Edward, I have another patch that implements the correct behavior for
displaying the Downloads panel and history, as well as fixing a few other
issues. This means basically a feature-complete version. Do you prefer
to receive it as a patch on top of the existing one, or as a full patch
including the previous one?

	Dietrich Ayala (:dietrich)
	Comment 74 • 14 years ago

This blocks the status bar replacement in bug 574688, so is also blocking+.

	Johnathan Nightingale [:johnath]
	Comment 75 • 14 years ago

(In reply to comment #74)
> This blocks the status bar replacement in bug 574688, so is also blocking+.

Talked this through with Dietrich and Limi - we do not believe that this is necessary for the status bar work.

	Dave Garrett
	Comment 76 • 14 years ago

It may not block due to status bar work, but shouldn't it block a beta for feature freeze in its own right? The current download manager feels old and out of place in Firefox 4 and lots of people would really like to see this get in.

	Johnathan Nightingale [:johnath]
	Comment 77 • 14 years ago

I would love to have it, don't get me wrong, but no, we can't hold the release on it. We are too late in the cycle to contain the risk. But I would love to have it.

	:Paolo Amadini Assignee
	Comment 78 • 14 years ago

Whether we'll be able to get the new Download Manager in Firefox 4 actually
comes down to how much review effort is required to finalize it. Shawn kindly
offered to do a first-pass review that may help us in understanding it.

The good news is that this version of the patch is feature-complete, only
the following trivial "maintenance" tasks are still open:
 * Move global functions in "downloads.js" to a dedicated namespace.
 * Ensure the toolbar icon is visible when upgrading from an old profile.
 * Share the "open download" function, or at least its strings, with Toolkit.
 * Copy the Windows theme to the other theme folders.

In case someone wants to try it live, consider that this patch is based on
changeset 9fd11a17eb1a. Also, the dependencies 564900 and 591289 are
required for enjoying the full experience. :-)

	Alex Limi (:limi) — Firefox UX Team
	Comment 79 • 14 years ago

Excellent work, Paolo!

Yes, we'd love to have this, but the definition of a blocker is that we'd hold the release for it — which we won't.

That doesn't mean we shouldn't try to get it landed, however! :)

	Shawn Wilsher :sdwilsh Reporter
	Comment 80 • 14 years ago

Comment on attachment 470869 [details] [diff] [review]
Feature-complete version for review (Windows theme only)

So, it seems to me that we are completely breaking the toolkit UI with this patch.  Is that right?  We cannot do that since other applications (Thunderbird, Songbird, others probably) do depend on this UI.

For review comments with expandable file context, please see http://reviews.visophyte.org/r/470869/.

on file: browser/base/content/browser.xul line 1004
>       <toolbarbutton id="downloads-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      oncommand="DownloadsOverlay.showPopup();"
>                      ondrop="DownloadsButtonDNDObserver.onDrop(event)"
>                      ondragover="DownloadsButtonDNDObserver.onDragOver(event)"
>                      ondragenter="DownloadsButtonDNDObserver.onDragOver(event)"
>                      ondragleave="DownloadsButtonDNDObserver.onDragLeave(event)"
>                      label="&downloads.label;" removable="true"
>                      tooltiptext="&downloads.tooltip;"/>

nit: one property per line please.  It's easy to miss some of them because
there are randomly more than one on a line.


on file: browser/components/downloads/DownloadCommon.jsm line 81
>   /**
>    * True as soon as initialization begins.
>    */
>   _initStarted: false,

This is private state, so it shouldn't be on an exported symbol.


on file: browser/components/downloads/DownloadCommon.jsm line 95
>     // convert strings to those in the string bundle
>     let (sb = Services.strings.createBundle(kDownloadsStringBundleUrl)) {
>       for (let [name, value] in Iterator(DownloadCommonStrings)) {
>         if (typeof value == "string") {
>           DownloadCommonStrings[name] = sb.GetStringFromName(value);
>         } else {
>           // Convert "arguments" to a real array before calling into XPCOM
>           DownloadCommonStrings[name] = function() sb.formatStringFromName(
>             value[0], Array.prototype.slice.call(arguments), arguments.length);
>         }
>       }
>     }

We should be doing this lazily, as needed (see how DownloadUtils.jsm does
this).


on file: browser/components/downloads/DownloadManagerUI.js line 59
>     var browser = Services.wm.getMostRecentWindow("navigator:browser");
>     if (browser) {
>       browser.DownloadsOverlay.showPopup();
>     }

feels wrong to not show anything if there are now browser windows open.


on file: browser/components/downloads/tests/browser_basic_functionality.js line 180
>     gDownloadsView.focus();

hmm, why do we need to focus?


on file: browser/themes/winstripe/browser/jar.mn line 51
>         skin/classic/browser/downloads/downloadButtons.png           (downloads/downloadButtons.png)
>         skin/classic/browser/downloads/downloadIcon.png              (downloads/downloadIcon.png)
>         skin/classic/browser/downloads/downloads.css                 (downloads/downloads.css)
>         skin/classic/browser/downloads/pause.png                     (downloads/pause.png)
>         skin/classic/browser/downloads/stop.png                      (downloads/stop.png)

note: we'll need to have something, even if not great, for linux and os x
before this can land.


on file: toolkit/components/downloads/src/Makefile.in line 69
> # TODO: Disable this also when building the "browser" application.
> #EXTRA_COMPONENTS = \
> #  nsDownloadManagerUI.js \
> #  nsDownloadManagerUI.manifest \

needs to be fixed before moving forward.  You might get help from khuey on how
to do this properly.

We also still need to test this UI, which means we'll need to build it
somehow.


on file: browser/locales/en-US/chrome/browser/downloads/downloads.properties line 18
> stateBlockedPolicy=Blocked by your Security Zone Policy

if you change a string, you also need to change it's name, otherwise
localizers will not know it's meaning changed.


on file: browser/locales/en-US/chrome/browser/downloads/downloads.properties line 19
> stateDirty=Blocked: May contain a virus or spyware

needs a localization note


on file: browser/components/downloads/download.xml line 29
> #   Blair McBride <bmcbride@mozilla.com>

er?


on file: browser/components/downloads/downloads.js line 57
> Cu.import("resource://gre/modules/DownloadCommon.jsm");

this file you reference is built in browser/ so toolkit/ cannot depend on it.

	Ed Lee :Mardak
	Comment 81 • 14 years ago

(In reply to comment #80)
> So, it seems to me that we are completely breaking the toolkit UI with this
I thought that at first as well, but notice that all the files are being copied then modified. It just shows up strange in the diffs.

	:Paolo Amadini Assignee
	Comment 82 • 14 years ago

(In reply to comment #80)
> on file: browser/components/downloads/DownloadManagerUI.js line 59
> >     var browser = Services.wm.getMostRecentWindow("navigator:browser");
> >     if (browser) {
> >       browser.DownloadsOverlay.showPopup();
> >     }
> 
> feels wrong to not show anything if there are now browser windows open.

That's inherent in the panel design I think... but maybe we can ensure that
a new browser window is opened if the call is the result of an explicit user
request, while for new downloads initiated while there is no browser window
open (rare case I think) I don't think we should open a new window, there
will be a screen-level notification when the download is completed in any
case.

I'm also interested in Limi's opinion on this.

> on file: browser/themes/winstripe/browser/jar.mn line 51
> >         skin/classic/browser/downloads/downloadButtons.png
> note: we'll need to have something, even if not great, for linux and os x
> before this can land.

I think copying the folder as is will suffice for now, it's based on the
new Add-ons Manager design which is not platform-specific, and refining
the visual style is explicitly a non-goal for the next beta. I've not done
the copy yet in case I spot any bugs or any change is needed, but I'll do
the copy before the final version.

> on file: toolkit/components/downloads/src/Makefile.in line 69
> > # TODO: Disable this also when building the "browser" application.
> > #EXTRA_COMPONENTS = \
> > #  nsDownloadManagerUI.js \
> > #  nsDownloadManagerUI.manifest \
> 
> needs to be fixed before moving forward.  You might get help from khuey on how
> to do this properly.

Thanks, I'll ask.

> on file: browser/locales/en-US/chrome/browser/downloads/downloads.properties
> line 19
> > stateDirty=Blocked: May contain a virus or spyware
> 
> needs a localization note

Why?

> on file: browser/components/downloads/download.xml line 29
> > #   Blair McBride <bmcbride@mozilla.com>
> 
> er?

This reflects the fact that I originally included work from the
Add-ons Manager download progress XBL binding. It's heavily modified
however, so I'm not sure if the credit is still required.

I'll work on the other nits immediately.

	Shawn Wilsher :sdwilsh Reporter
	Comment 83 • 14 years ago

(In reply to comment #81)
> I thought that at first as well, but notice that all the files are being copied
> then modified. It just shows up strange in the diffs.
Ugh, review board appears to have mislead me :(

	Shawn Wilsher :sdwilsh Reporter
	Comment 84 • 14 years ago

(In reply to comment #82)
> That's inherent in the panel design I think... but maybe we can ensure that
> a new browser window is opened if the call is the result of an explicit user
> request, while for new downloads initiated while there is no browser window
> open (rare case I think) I don't think we should open a new window, there
> will be a screen-level notification when the download is completed in any
> case.
Hmm, we should clearly get input from UX on this.

> I think copying the folder as is will suffice for now, it's based on the
> new Add-ons Manager design which is not platform-specific, and refining
> the visual style is explicitly a non-goal for the next beta. I've not done
> the copy yet in case I spot any bugs or any change is needed, but I'll do
> the copy before the final version.
As long as UX is OK with this, and we actually have resources to get the theming right, I'm OK with this.  If either of those conditions is a problem, this should probably slip to Firefox.Next

> Why?
So localizers know what it's for.  Also note how the other states have notes.

> This reflects the fact that I originally included work from the
> Add-ons Manager download progress XBL binding. It's heavily modified
> however, so I'm not sure if the credit is still required.
Probably not worthwhile (but make sure you are on there if you care).

	:Paolo Amadini Assignee
	Comment 85 • 14 years ago

All nits addressed. Here is the current list of open tasks:
 * Understand what to do when user requests to open the download manager
    and there is no browser window open.
 * Move global functions in "downloads.js" to a dedicated namespace.
 * Share the "open download" function, or at least its strings, with Toolkit.
 * Copy the Windows theme to the other theme folders.

I also implemented the migration code for the toolbar button. As there is no
automated test suite for testing profile migration from older versions, I did
the following manual tests:
 - Created four new profiles in Firefox 3.6 (A, B, C and D)
 - Started FF 3.6 with profile B and closed immediatley
 - Started FF 3.6 with profile C, customized the toolbar without adding a
    Downloads button, then closed the browser
 - Started FF 3.6 with profile D, added the Downloads button and closed
 - Started Minefield once per profile, and verified that the Download
    button always appeared and there were no related errors in the Console.

	Alex Limi (:limi) — Firefox UX Team
	Comment 86 • 14 years ago

(In reply to comment #84)
> (In reply to comment #82)
> > That's inherent in the panel design I think... but maybe we can ensure that
> > a new browser window is opened if the call is the result of an explicit user
> > request, while for new downloads initiated while there is no browser window
> > open (rare case I think) I don't think we should open a new window, there
> > will be a screen-level notification when the download is completed in any
> > case.
> Hmm, we should clearly get input from UX on this.

Wait, how can you get a download without any windows open? Or is that not what this means? :)

To try to cover some edges cases up front in the name of efficiency:
- If you're clicking a link from an external application, you get a browser window.
- If you shut down the last window on Windows, you are quitting the application. If any downloads are in progress, we warn about quitting.
- If you shut down the last window on the Mac, there's no download progress indicator in any windows. The downloads folder will still bounce when the download is done, and you can always open a window to get to the panel, so this is OK too.

Anything else that we need to handle?


> > I think copying the folder as is will suffice for now, it's based on the
> > new Add-ons Manager design which is not platform-specific, and refining
> > the visual style is explicitly a non-goal for the next beta. I've not done
> > the copy yet in case I spot any bugs or any change is needed, but I'll do
> > the copy before the final version.
> As long as UX is OK with this, and we actually have resources to get the
> theming right, I'm OK with this.  If either of those conditions is a problem,
> this should probably slip to Firefox.Next

We are OK with this. Ideally, they should be the same thing eventually, but for now it seems like this is the easiest way, even if it results in some duplication.

	Shawn Wilsher :sdwilsh Reporter
	Comment 87 • 14 years ago

(In reply to comment #86)
> Wait, how can you get a download without any windows open? Or is that not what
> this means? :)
An add-on could very easily do this via APIs.

> To try to cover some edges cases up front in the name of efficiency:
> - If you're clicking a link from an external application, you get a browser
> window.
> - If you shut down the last window on Windows, you are quitting the
> application. If any downloads are in progress, we warn about quitting.
> - If you shut down the last window on the Mac, there's no download progress
> indicator in any windows. The downloads folder will still bounce when the
> download is done, and you can always open a window to get to the panel, so this
> is OK too.
- Library or other non-browser window (even add-on provided) is the only thing open.

That's all I got right now, but my gut says there is more.

	Alex Limi (:limi) — Firefox UX Team
	Comment 88 • 14 years ago

(In reply to comment #87)
> (In reply to comment #86)
> > Wait, how can you get a download without any windows open? Or is that not what
> > this means? :)
> An add-on could very easily do this via APIs.

OK. I still think that's sufficiently edge-case-y that we're OK with not showing it, given that all major OSes have bounce/glow indicators on download completion.

> - Library or other non-browser window (even add-on provided) is the only thing
> open.

Right. So don't show any panel, but bounce/glow on the OS level still works?

	Shawn Wilsher :sdwilsh Reporter
	Comment 89 • 14 years ago

(In reply to comment #88)
> OK. I still think that's sufficiently edge-case-y that we're OK with not
> showing it, given that all major OSes have bounce/glow indicators on download
> completion.
Er, we don't do that at all on windows (except for the browser window, which would be closed in this situation) AFAIK.

	Alex Limi (:limi) — Firefox UX Team
	Comment 90 • 14 years ago

(In reply to comment #89)
> (In reply to comment #88)
> > OK. I still think that's sufficiently edge-case-y that we're OK with not
> > showing it, given that all major OSes have bounce/glow indicators on download
> > completion.
> Er, we don't do that at all on windows (except for the browser window, which
> would be closed in this situation) AFAIK.

I'm confused. How can the app be open on Windows with no windows open? (what an awkward sentence :P )

	Dave Garrett
	Comment 91 • 14 years ago

Why not just open a new about:blank window as a fallback when needed? Probably not ideal, but in such an odd edge case it may be fine, at least for now.

	Shawn Wilsher :sdwilsh Reporter
	Comment 92 • 14 years ago

(In reply to comment #90)
> I'm confused. How can the app be open on Windows with no windows open? (what an
> awkward sentence :P )
Having a non-browser window open (like the Library) will keep the application running.  However, we don't, AFAIK, do any download indication on those windows.  We only do that for actual browser windows.

We will still do the toaster pop-up though.

	:Paolo Amadini Assignee
	Comment 93 • 14 years ago

(In reply to comment #92)
> We will still do the toaster pop-up though.

Exactly; see attachment 467455 [details], step 5. Clicking the hyperlink named "All
files have finished downloading" used to open the download manager window (by
the way, I never managed to click on it before the notification went away).

To reproduce the condition we're talking about, you'd have to start a
download, open the Library window, close the last browser window, wait for
the screen-level notification, and click the link. In this case, I don't
know if it makes sense to open a new browser window and show the downloads
panel. We might just ignore the click; you'll see the download in the panel
the next time you open a browser window.

	Shawn Wilsher :sdwilsh Reporter
	Comment 94 • 14 years ago

Note that my concern here is that we are no longer following the API contract here which says it *will* show the UI if it isn't already visible.  Just because the browser might be OK with this change, other consumers may not be.

	Leman Bennett [Omega]
	Comment 95 • 14 years ago

(In reply to comment #92)
> (In reply to comment #90)
> > I'm confused. How can the app be open on Windows with no windows open? (what an
> > awkward sentence :P )
> Having a non-browser window open (like the Library) will keep the application
> running.  However, we don't, AFAIK, do any download indication on those
> windows.  We only do that for actual browser windows.
> 
> We will still do the toaster pop-up though.

Sorry if this is off topic.

Will we get a faster, less cpu intensive "Toaster-Popup"?

	Shawn Wilsher :sdwilsh Reporter
	Comment 96 • 14 years ago

(In reply to comment #95)
> Will we get a faster, less cpu intensive "Toaster-Popup"?
File a different bug if you have issues with it please.

	Ivan Stetsenko
	Comment 97 • 14 years ago

Just a question about the appearance of the "downloads" button on the toolbar... Are there any plans about making an indicator of the download progress (like in the "download badge" at the beginning of the discussion)?

	:Paolo Amadini Assignee
	Comment 98 • 14 years ago

(In reply to comment #97)
> Just a question about the appearance of the "downloads" button on the
> toolbar... Are there any plans about making an indicator of the download
> progress (like in the "download badge" at the beginning of the discussion)?

Yes, actually my idea was to do that indicator right after beta 6, since that
is not a new feature but a visual improvement requiring no localization changes.

That said, at this point it doesn't seem the panel UI will be able to be
included in Firefox 4, but will have to wait for the next version, since
currently all the people that could review it are assigned to other tasks.

Shawn has been very kind to help with giving an assessment of how much
review effort was required to finish the task, both for this bug and the
dependencies, but, since it was a side-trip from his assigned duties, the
assessment was conducted in a hurry and probably even resulted in the
current code situation being perceived as worse than it actually is,
which would not have happened in a relaxed environment.

I'm happy to have had the opportunity of finishing the task in time, but
I too prefer to have my code reviewed and evaluated without hurry! Then,
the feature will have plenty of time to be polished and even optimized
for performance before it is released for its first public beta.

Moreover, it is also possible that before Firefox 4.1 someone else will
also work on the broader download experience improvements outlined by
Alexander Limi and we'll be able to release them all at once.

	Shawn Wilsher :sdwilsh Reporter
	Comment 99 • 14 years ago

Sorry, but I don't think I can get to this before Friday anymore :(

	:Paolo Amadini Assignee
	Comment 100 • 14 years ago

Comment on attachment 471241 [details] [diff] [review]
Updated feature-complete version (Windows theme only) [NOTE: has hg copies]

(In reply to comment #99)
> Sorry, but I don't think I can get to this before Friday anymore :(

That's fine, I wouldn't have been able to get a final review in time in any
case. Thank you for having looked at this in the first place! Since now
there's a lot of time before this feature is released, I think I'll swap
the order in which I thought to do the job, and work first on features of
Toolkit's download manager that will make a shared implementation possible.

I'm currently thinking about bug 438342 and bug 410538, and will file new
bugs as necessary in the following months.

	Peter Henkel [:Terepin]
	Comment 101 • 14 years ago

So this will not be in FX4?

	Dave Garrett
	Comment 102 • 14 years ago

It sounds like the plan is to not aim for FF4, but I would still hold out hope for a later than usual backport from future Trunk to the FF4 branch in time for 4.0. This is very important and everyone wants it. Screw the procedure if need be for this one, I say. If not, I do hope 4.1 isn't a fantasy number this time.

	Peter Henkel [:Terepin]
	Comment 103 • 14 years ago

(In reply to comment #102)
> If not, I do hope 4.1 isn't a fantasy number this time.

Exactly, I hope 4.1 won't morph into 4.5 and with one year waiting (3.5, anyone?).

	José Jeria
	Comment 104 • 14 years ago

(In reply to comment #103)
> Exactly, I hope 4.1 won't morph into 4.5 and with one year waiting (3.5,
> anyone?).

Please read the bugzilla guidelines before commenting: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Interesting for you here would be 1.1 and 2.

	Axel Hecht [:Pike]
	Comment 105 • 14 years ago

I'm confused by a bunch of removed strings in attachment 471241 [details] [diff] [review], at least for one I didn't find a corresponding code change (timeDouble).

	Shawn Wilsher :sdwilsh Reporter
	Comment 106 • 14 years ago

(In reply to comment #105)
> I'm confused by a bunch of removed strings in attachment 471241 [details] [diff] [review], at least for
> one I didn't find a corresponding code change (timeDouble).
The patch is misleading how it's displayed in bugzilla and other tools.  Read the raw diff.

	Axel Hecht [:Pike]
	Comment 107 • 14 years ago

There is quite some content removed from downloads/downloads.properties, which is used in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#76, AFAICT, and not removed by this patch.

	Ed Lee :Mardak
	Comment 108 • 14 years ago

The strings being removed are from the browser copy of downloads.properties not toolkit's downloads.properties:

copy from toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties
copy to browser/locales/en-US/chrome/browser/downloads/downloads.properties
--- a/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties
+++ b/browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ -1,120 +1,31 @@
-# LOCALIZATION NOTE (seconds, minutes, hours, days): Semi-colon list of plural

	Axel Hecht [:Pike]
	Comment 109 • 14 years ago

Ah, now I get it, thanks.

	Peter Henkel [:Terepin]
	Comment 110 • 14 years ago

This isn't intended for 4.0?

	Dave Garrett
	Comment 111 • 14 years ago

It might require a miracle, but I believe it's still possible.

	Peter Henkel [:Terepin]
	Comment 112 • 14 years ago

I'll prey for miracle then. :)

	Shawn Wilsher :sdwilsh Reporter
	Comment 113 • 14 years ago

This is not happening for Firefox 4.  Sorry folks.

	Nicolas Mandil (:NicolasWeb)
	Comment 114 • 14 years ago

(In reply to comment #113)
> This is not happening for Firefox 4.  Sorry folks.

I didn't follow everything here (sorry if it's a dup :-(), but is implementing the download manger that was in the status bar in the hint of the new download button in the primary UI a "good" solution as this bug won't land in time for Fx4 ?

	Po-chiang Chao [:bobchao]
	Comment 115 • 14 years ago

I believe that we are trying to knock the status bar out, at least hidden by default. (see Bug 574688)

	Nicolas Mandil (:NicolasWeb)
	Comment 116 • 14 years ago

(In reply to comment #115)
> I believe that we are trying to knock the status bar out, at least hidden by
> default. (see Bug 574688)

I know that, but I was refering to download label that *was* in ths status bar that could be shown *now* into the new download button hint (on mouse over) to not break too much this functionality with the removal of the status bar (that I don't discuss here) as this info will stay in the primary UI.

Is that a good plan with the situation of this bug now ?

	Dave Garrett
	Comment 117 • 14 years ago

My apologies for bending etiquette here, but I just have to fight for this bug.

How close are we here to being able to have a fully workable patch, within a reasonable minimum criteria? The feature and string freeze beta keeps getting delayed (i.e. the first "beta" that's really not a high-quality alpha) so there's more time than it might look.

Is it possible to do the apparently standard cheat of committing a strings only patch and then getting the rest in at a later date?

The Addon Manager got its nice fancy new tab, but the Download Manager is still futzing with an old popup window. The Download Manager is the last bastion of the old style of GUI and not getting it updated feels to me like Firefox 4 wouldn't be fully complete.

	Axel Hecht [:Pike]
	Comment 118 • 14 years ago

We've had feature and string freeze yesterday, and it missed feature freeze.

	Dave Garrett
	Comment 119 • 14 years ago

Can we cheat somehow then, please? This one is particularly important, I think. Though, I feel I may be waging a loosing battle here... :/

	Dave Garrett
	Comment 120 • 14 years ago

The application of the definition of "feature" here is debatable. It's a GUI update primarily, not really redoing the entire concept of a download manager.

(sorry for the bugspam)

	Shawn Wilsher :sdwilsh Reporter
	Comment 121 • 14 years ago

(In reply to comment #119)
> Can we cheat somehow then, please? This one is particularly important, I think.
> Though, I feel I may be waging a loosing battle here... :/
No.  This is explicitly not blocking 2.0, and it's not making Firefox 4.  That is the final answer.

	Nicolas Mandil (:NicolasWeb)
	Comment 122 • 14 years ago

(In reply to comment #116)
> I know that, but I was refering to the download label that *was* in ths status > bar that could be shown *now* into the new download button hint (on mouse over) > to not break too much this functionality with the removal of the status bar
> (that I don't discuss here) as this info will stay in the primary UI.
> 
> Is that a good plan with the situation of this bug now ?

I've filled bug 597067 for this late solution to not break too much the primary UI.

	Dave Garrett
	Comment 123 • 14 years ago

(In reply to comment #121)
> (In reply to comment #119)
> > Can we cheat somehow then, please? This one is particularly important, I think.
> > Though, I feel I may be waging a loosing battle here... :/
> No.  This is explicitly not blocking 2.0, and it's not making Firefox 4.  That
> is the final answer.

:sigh:  Ruling accepted.

However, *something* is going to have to be done, because as pointed out by others even the old GUI is getting broken by the status bar removal.

	Peter Henkel [:Terepin]
	Comment 124 • 14 years ago

Agreed.
Here is an idea: As the original mockup from Stephen (http://www.stephenhorlander.com/images/blog-posts/theme-update-2010-04-20/panel-download-win7.png) has showed, we can use pie chart inside button's icon. This is basicaly only transfer the progress meter to a different place, not a new feature, so it shouldn't be affected by feature-freeze, just like showing link's adress in location bar.

	Josh Tumath
	Comment 125 • 14 years ago

Maybe we should file a bug to investigate an alturnative place to put download notifications?

	Peter Henkel [:Terepin]
	Comment 126 • 14 years ago

Bug 597145

	Nicolas Mandil (:NicolasWeb)
	Comment 127 • 14 years ago

(In reply to comment #125)
> Maybe we should file a bug to investigate an alturnative place to put download
> notifications?

(In reply to comment #126)
> Bug 597145

I've filled a generic bug 597155 for this

	DRC
	Comment 128 • 14 years ago

Having this default in new firefox would be awesome, also because in b7 I don't see any downloads information (need to have downloads windows open)

good work!

	bogas04
	Comment 129 • 14 years ago

well i feel pathetic right now , but i kind of made a quick mockup for other method of implementation 

http://img517.imageshack.us/img517/214/20177387.png

	Martin Arndt
	Comment 130 • 14 years ago

This neither looks good nor is it a useful attempt - the previous mockup makes current download's status accessible within 1 click on the downloads-button.
Your attempt needs 1 click and several mouse-moves through submenus to get to needed information..

	bogas04
	Comment 131 • 14 years ago

True but it takes no new button , but yeah , it should be accessible at the earliest hmm

	Martin Arndt
	Comment 132 • 14 years ago

I'd prefer a little, but informative, button over a complicated submenu-structure.. ;)

	bogas04
	Comment 133 • 14 years ago

:)

	Shawn Wilsher :sdwilsh Reporter
	Comment 134 • 13 years ago

If we want this for Firefox 5, we should start moving on it soon.  Paolo, do you have time to work on this soon?

	:Paolo Amadini Assignee
	Comment 135 • 13 years ago

(In reply to comment #134)
> If we want this for Firefox 5, we should start moving on it soon.  Paolo, do
> you have time to work on this soon?

Probably, but it depends on what you mean by "soon" :-)

My understanding was that everybody that could review the patch was assigned
to other tasks related to the Firefox 4 release. If this is still the case,
then it means we can't start to work on this again until Firefox 4 is
released, and that would actually match with my current planning :-)

In any case, this bug is not waiting on myself at present. Anybody should
feel free to do code review on the current patch as soon as they wish!

	Shawn Wilsher :sdwilsh Reporter
	Comment 136 • 13 years ago

(In reply to comment #135)
> In any case, this bug is not waiting on myself at present. Anybody should
> feel free to do code review on the current patch as soon as they wish!
Ah, in that case, I think it'd be good for Mardak to do a first pass, and limi needs to provide UI feedback.

	Alex Limi (:limi) — Firefox UX Team
	Comment 137 • 13 years ago

If someone can get me a tryserver build, I'd be happy to do a first round of UI feedback!

	:Paolo Amadini Assignee
	Comment 138 • 13 years ago

I can request first-pass code review as soon as Mardak is free, but I guess
the tryserver build would require unbitrotting the patch first, because it's
several months old now, and would also require copying the styles to the
non-Windows themes to allow people to test on any platform.

	Alex Limi (:limi) — Firefox UX Team
	Comment 139 • 13 years ago

(In reply to comment #138)
> and would also require copying the styles to the
> non-Windows themes to allow people to test on any platform.

I'm fine with limiting the initial UI review to a single platform (Mac, Linux) if that makes it easier to get this started again.

	Peter Henkel [:Terepin]
	Comment 142 • 13 years ago

Any reason why was attachment deleted?

	Paul [pwd]
	Comment 143 • 13 years ago

It was the same as the patch with extra buttons.

	:Paolo Amadini Assignee
	Comment 144 • 13 years ago

I created an updated patch that can be used for testing on all platforms. Builds
that include this patch will be available here for some time:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/paolo%2Emozmail%40amadzone%2Eorg-278e7fd33e27/

	Tetsuharu OHZEKI [:tetsuharu] (UTC+9)
	Comment 145 • 13 years ago

(In reply to comment #144)
I can not open about download window with shortcut key at its revision.

	Vova Olar
	Comment 146 • 13 years ago

Shouldn't it be an ArrowPanel??

	Siddhartha Dugar [:sdrocking]
	Comment 147 • 13 years ago

1. Ongoing download should be at the top of the list.
2. Drop down menu for "Open" should close on clicking arrow button again.
3. The overall appearance also needs to be improved.

	:Paolo Amadini Assignee
	Comment 148 • 13 years ago

Thanks for the feedback! The appearance is intentionally raw, it will be
refined as a next step whan all the functionality is there, also because
it will be different for different operating systems. It will definitely
be an arrow panel!

For now, we're concentrating on functionality and usability.

	:Paolo Amadini Assignee
	Comment 149 • 13 years ago

(In reply to comment #147)
> 1. Ongoing download should be at the top of the list.

I'm really interested in this piece of feedback. Did you use the new build
while browsing normally, or only for testing? In the former case, did you
find a real usability problem with completed downloads being listed first?

The point is that the item you're most likely to interact with is the download
that just finished. This is why it is at the top of the list, to avoid it being
moved out of sight by ongoing downloads. Probably you want to open the file, or
just know that that particular download has finished and dismiss it by clicking
the X (like for tabs).

There will be a separate indicator to see the overall status of ongoing
downloads, that will be visible even without clicking on the button.

	bogas04
	Comment 150 • 13 years ago

1) Clear Download history option is unavailable
2) For two or above downloads , this scheme would not be feasible enough for the case of netbooks as it takes hell lot of space. Instead a theme with 4~5 downloads visible under 600px would do great
3) This requires us to have another download button (i know its discussed) , i think we should use the same idea of downloading of an addon , and place a download button just prior to the url and its icon (whatever its called) and make it global (not the case for addons) . The advantage is that , the user already knowns that his addons download progress can be seen from that point , and now if we put downloads there too , it would be easily noticed

	Paul [pwd]
	Comment 151 • 13 years ago

Is download history being handled in a separate bug or will this bug encompass that?

	Shawn Wilsher :sdwilsh Reporter
	Comment 152 • 13 years ago

I know many of you care a great deal about this, but this bug is not the right place to discuss various designs and their pros and cons.  Doing so will make it substantially more difficult to track the work, which will make it take longer (which I don't think any of you want).

Please take the discussion to a newsgroup thread, where it belongs:
https://www.mozilla.org/about/forums/#dev-usability

	Paul [pwd]
	Comment 153 • 13 years ago

(In reply to comment #149)
I think it's just an issue of how much you download. If you download a lot, then it's easy to have current download progress lost and ultimately that's what a user wants to look for at a glance.

(In reply to comment #150)
> 2) For two or above downloads , this scheme would not be feasible enough for
> the case of netbooks as it takes hell lot of space. Instead a theme with 4~5
> downloads visible under 600px would do great
I think that in the past, we've had issues with long download lists slowing Firefox right down, so in that regard, we'd want this list cleared at the end of each session anyway, thus negating the argument about netbooks and screensize as the average download per session on average, particularly on a netbook will not be anywhere near 4-5 downloads.

(In reply to comment #152)
I wasn't being rude and ignoring you. I was just giving feedback requested by the dev assigned to the bug. Apologies if it caused any offence.

	Shawn Wilsher :sdwilsh Reporter
	Comment 154 • 13 years ago

Comment on attachment 511848 [details] [diff] [review]
Updated feature-complete version [NOTE: has hg copies]

This is a first pass.  As I look at this more and more, I'm likely to see things clearer, so don't be surprised if I catch something in later review iterations that I didn't mention at all before.

>+++ b/browser/base/content/browser.xul
> #ifdef WINCE
>              iconsize="small" defaulticonsize="small"
>-             defaultset="unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,home-button,bookmarks-menu-button-container,navigator-throbber,fullscreenflex,window-controls"
>+             defaultset="unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,home-button,bookmarks-menu-button-container,downloads-button,navigator-throbber,fullscreenflex,window-controls"
> #else
>              iconsize="large"
>-             defaultset="unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,home-button,bookmarks-menu-button-container,fullscreenflex,window-controls"
>+             defaultset="unified-back-forward-button,urlbar-container,reload-button,stop-button,search-container,home-button,bookmarks-menu-button-container,downloads-button,fullscreenflex,window-controls"
> #endif
Nuke the WINCE stuff.  We won't be supporting it, so it can just disappear.

>+++ b/browser/components/downloads/DownloadCommon.jsm
>+const kDownloadsStringBundleUrl =
>+      "chrome://browser/locale/downloads/downloads.properties";
nit: only indent two spaces here

>+/**
>+ * True as soon as initialization begins.
>+ */
>+gInitStarted = false;
var or let here (please declare it)
However, the method this protects could only ever be entered (and running) at once.  As a result, I'd prefer it if this was called gInitialized.

>+  initialize: function DC_initialize() {
global-nit (new files): brace on newline

>+      function DC_setStringFunction(name, value)
dump this into a variable or pull it out globally.  Right now, it's made into a global anyway.

>+++ b/browser/components/downloads/DownloadManagerUI.js
nit: I'd prefer to have this file split into sections like https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js

>+  show: function DMUI_show(aWindowContext, aID, aReason) {
>+    var browser = Services.wm.getMostRecentWindow("navigator:browser");
>+    if (browser) {
>+      browser.DownloadsOverlay.showPopup();
>+    }
I think we'd also want to bring that browser window forward if it isn't the most forward one.

>--- a/toolkit/mozapps/downloads/content/download.xml
>+++ b/browser/components/downloads/download.xml
I think that doing the hg copy here might not be terribly useful and that it might be better to just do an hg add with the new file.  We've made enough changes here that I think preserving history isn't worthwhile.  (Additionally, it will likely make it easier to review.)

>+          <xul:image class="downloadTypeIcon NOT_BLOCKED"
>+                     validate="always"
>                      xbl:inherits="src=image"/>
>+          <xul:image class="downloadTypeIcon blockedIcon BLOCKED"/>
I think it'd be better to have the CSS match on the state attribute of the download, and then hide/unhide these based on that.

>+        <xul:vbox pack="start"
>+                  flex="1">
>+          <xul:hbox align="center"
>+                    flex="1">
>+            <xul:label xbl:inherits="value=target,tooltiptext=target"
>+                       crop="center"
>+                       flex="1"
>+                       class="downloadTarget"/>
>+            <xul:label xbl:inherits="value=dateTime,tooltiptext=dateTimeTip"
>+                       class="downloadDateTime FINISHED FAILED CANCELED BLOCKED"/>
Likewise, this seems rather odd.  We should be able to style all of these based on download[state="#"] and be OK.

>--- a/toolkit/mozapps/downloads/content/downloads.js
>+++ b/browser/components/downloads/downloads.js
Same here; not sure the hg copy is worthwhile at this point.

>+  initialize: function DO_initialize() {
>+    if (this._initStarted) {
>+      return;
>+    }
>+    this._initStarted = true;
>+
>+    if (!gDownloadManager) {
>+      gDownloadManager = Cc["@mozilla.org/download-manager;1"]
>+                         .getService(nsIDM);
We can just use defineLazyServiceGetter globally for this

>+      gDownloadsView = document.getElementById("downloadsView");
defineLazyGetter globally here

>+  /**
>+   * Initialize the statement without actually opening the database connection.
>+   */
>+  initialize: function DDA_initialize() {
>+    if (!this._statement) {
>+      this._statement = gDownloadManager.DBConnection.createStatement(
>+        "SELECT id, target, name, source, state, startTime, endTime, " +
>+               "referrer, currBytes, maxBytes, " +
>+               "state IN (?1, ?2, ?3, ?4, ?5) isActive, " +
>+               "state IN (?6, ?7, ?8, ?9) isDone " +
>+        "FROM moz_downloads " +
>+        "ORDER BY isDone DESC, isActive ASC, endTime DESC, startTime DESC");
>+    }
We need to use async statements for this (will have to change our call to close too).  This does mean that a bunch of this stuff is going to have to change, sadly.

>--- a/toolkit/mozapps/downloads/content/downloads.xul
>+++ b/browser/components/downloads/downloadsOverlay.xul
likewise, pretty sure there is no value in using hg copy here

>+++ b/browser/components/downloads/tests/browser_basic_functionality.js
global-nit: tests should use http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

This is a good start.  Will look closer in the next iteration.

	:Paolo Amadini Assignee
	Comment 155 • 13 years ago

(In reply to comment #154)

Thank you Shawn for your review! Probably I can spend some time tomorrow to
create a revised patch. Meanwhile, I have a few questions.

> Nuke the WINCE stuff.  We won't be supporting it, so it can just disappear.

Do you mean that "WINCE" will never be defined in supported configurations,
or just that the Download Manager UI isn't applicable on that platform?
Apart from Browser, will Toolkit support Windows CE? I'm asking since I
don't understand if I should remove everything inside any #ifdef WINCE
in "browser.xul" or just do that near the areas I'm touching, and if I
should do that in a separate patch.

> >+++ b/browser/components/downloads/DownloadCommon.jsm

I'm now thinking of defining a "DownloadCommon.strings" lazy getter instead
of using the "DownloadCommonStrings" exported symbol (that, as such, can't
be made lazily-loaded if I understand correctly). This way I could get rid
of the "initialize" method, at least until it's needed again.

"downloads.js" would do something like this:

XPCOMUtils.defineLazyGetter(this, "gStr", function() DownloadCommon.strings);

Another consideration: there aren't many strings in the bundle, we'll probably
need more than one string at a time, and we're already iterating over every
available string name when we need the first one anyway. Does it make sense
to create a lazy getter for each static string, instead of just storing its
value immediately? Is calling into XPCOM the expensive part? When we need the
first string, the "downloads.properties" file has probably already been fully
read and parsed.

> >--- a/toolkit/mozapps/downloads/content/download.xml
> >+++ b/browser/components/downloads/download.xml
> I think that doing the hg copy here might not be terribly useful and that it
> might be better to just do an hg add with the new file.  We've made enough
> changes here that I think preserving history isn't worthwhile.  (Additionally,
> it will likely make it easier to review.)
> >--- a/toolkit/mozapps/downloads/content/downloads.js
> >+++ b/browser/components/downloads/downloads.js
> Same here; not sure the hg copy is worthwhile at this point.

If it's easier for you to review this file as if it was a new file, I think
I can also go one step forward and move many global functions into objects.
Maybe, if not in the next iteration, soon after that.

> We need to use async statements for this (will have to change our call to
> close too).  This does mean that a bunch of this stuff is going to have to
> change, sadly.

Like in the patch you wrote some time ago for bug 438342?

Since we have some more time now to do things in the right way, and you're
reviewing this as if it's new code, I'm also thinking of moving all data
access to a shared JSM module while I rewrite it, so that once the list
of downloads is in memory, it isn't reloaded when the panel is displayed
in other browser windows. Concurrency issues should then be taken into
account, of course.

Another optimization I was thinking about is to allow at most one instance
of the panel to open at a time, and if the user requests to open a panel in
a browser window and we already loaded the downloads list in another one,
just move (import) the XUL nodes across the chrome documents instead of
rebuilding the list. Actually, although faster, that would be a synchronous
operation, thus it might not be appropriate if the list is very large.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 156 • 13 years ago

We're mostly getting rid of WINCE code across the platform - see bug 614720.

	:Paolo Amadini Assignee
	Comment 157 • 13 years ago

(In reply to comment #154)
> >+            <xul:label xbl:inherits="value=dateTime,tooltiptext=dateTimeTip"
> >+                       class="downloadDateTime FINISHED FAILED CANCELED BLOCKED"/>
> Likewise, this seems rather odd.  We should be able to style all of these
> based on download[state="#"] and be OK.

I started to follow this route but actually the combinations rapidly
exploded. I basically had to add a rule for every element whose visibility
should be controlled by the download state, and I had to apply a new specific
class to those elements that didn't have one already. See for example
".downloadProgress..." below. I stopped when I realized I would have had
to do the same thing for each item in the context menu defined in
"downloadsOverlay.xul".

Is there any specific problem why the current solution, apart oddness? ;-)
The alternative I tried seems more complex, but maybe there's a simpler
alternative I'm missing...

Example of the new, verbose CSS:

.download-state:not(:-moz-any([state="1"], /* Finished */
                              [state="2"], /* Failed   */
                              [state="3"], /* Canceled */
                              [state="6"], /* Blocked  */
                              [state="8"], /* Blocked  */
                              [state="9"]) /* Blocked  */)
               .downloadDateTime,

.download-state:not(:-moz-any([state="-1"],/* Starting */
                              [state="5"]) /* Starting */)
               .downloadProgressNotStarted,

.download-state:not(:-moz-any([state="2"], /* Failed   */
                              [state="3"]) /* Canceled */)
               .downloadProgressStopped,

.download-state:not(:-moz-any([state="0"], /* Downloading */
                              [state="4"]) /* Paused      */)
               .downloadProgressRunning,

.download-state:not(:-moz-any([state="2"], /* Failed   */
                              [state="3"]) /* Canceled */)
               .downloadProgressStopped
{
  display: none;
}

	:Paolo Amadini Assignee
	Comment 158 • 13 years ago

Meanwhile, here is an intermediate version of the patch without the hg copies
on some of the files, that should be easier to review. I'm going to make
relevant changes to it, probably during the week-end, but if you want you
can now take a look at it without the hg copies getting in the way.

In particular I'd like to separate the various parts and modules in such
a way that they can be easily moved to Toolkit to improve the "classic"
Download Manager, for example using asynchronous statements for the
database queries. If this works out well, we might be able to replace
Toolkit's Download Manager with this improved code, while keeping it in
a detached window for applications that want to use it this way, like it
was done for the Add-ons Manager. This would mean more work now, but
fewer code to maintain later.

	:Paolo Amadini Assignee
	Comment 159 • 13 years ago

(In reply to comment #154)
> >+++ b/browser/components/downloads/tests/browser_basic_functionality.js
> global-nit: tests should use http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

nit: in this case, these are tests I copied from existing files, thus
I couldn't just change the license. Maybe you know if all test code whose
copyright is owned by the Mozilla Corporation is being dedicated to the
Public Domain using CC0?

	Shawn Wilsher :sdwilsh Reporter
	Comment 160 • 13 years ago

(In reply to comment #155)
> Do you mean that "WINCE" will never be defined in supported configurations,
> or just that the Download Manager UI isn't applicable on that platform?
> Apart from Browser, will Toolkit support Windows CE? I'm asking since I
> don't understand if I should remove everything inside any #ifdef WINCE
> in "browser.xul" or just do that near the areas I'm touching, and if I
> should do that in a separate patch.
I mean just remove it when you are touching it.  I suppose someone will come along and remove it all at some point, but there is no need for us to maintain that code anymore.

> Another consideration: there aren't many strings in the bundle, we'll probably
> need more than one string at a time, and we're already iterating over every
> available string name when we need the first one anyway. Does it make sense
> to create a lazy getter for each static string, instead of just storing its
> value immediately? Is calling into XPCOM the expensive part? When we need the
> first string, the "downloads.properties" file has probably already been fully
> read and parsed.
In this module, we probably don't need to make a lazy getter for each string.  The code you grabbed this from had a bunch more.  I'm fine with either approach in this case.

> If it's easier for you to review this file as if it was a new file, I think
> I can also go one step forward and move many global functions into objects.
> Maybe, if not in the next iteration, soon after that.
Works for me!

> Like in the patch you wrote some time ago for bug 438342?
Correct, although we may need to do some changes to that.

> Since we have some more time now to do things in the right way, and you're
> reviewing this as if it's new code, I'm also thinking of moving all data
> access to a shared JSM module while I rewrite it, so that once the list
> of downloads is in memory, it isn't reloaded when the panel is displayed
> in other browser windows. Concurrency issues should then be taken into
> account, of course.
This is a good idea.

> Another optimization I was thinking about is to allow at most one instance
> of the panel to open at a time, and if the user requests to open a panel in
> a browser window and we already loaded the downloads list in another one,
> just move (import) the XUL nodes across the chrome documents instead of
> rebuilding the list. Actually, although faster, that would be a synchronous
> operation, thus it might not be appropriate if the list is very large.
Not sure if importing the nodes is needed, but the panel would close when you click elsewhere anyway, right?

(In reply to comment #157)
> Is there any specific problem why the current solution, apart oddness? ;-)
I guess I can't wrap my head around it (even if it works).  It's as if we are saying the download is FINISHED, FAILED, CANCELED, and BLOCKED all at the same time.

> Example of the new, verbose CSS:
That doesn't seem too terribly verbose, and isn't hard to follow.

(In reply to comment #159)
> nit: in this case, these are tests I copied from existing files, thus
> I couldn't just change the license. Maybe you know if all test code whose
> copyright is owned by the Mozilla Corporation is being dedicated to the
> Public Domain using CC0?
Yes, anything that is owned by Mozilla is OK to change.

	Shawn Wilsher :sdwilsh Reporter
	Comment 161 • 13 years ago

I'll try to get this feedback request done by Monday.  It might happen today even.  Sorry for the delay.

	:Paolo Amadini Assignee
	Comment 162 • 13 years ago

(In reply to comment #154)
> We need to use async statements for this (will have to change our call to close
> too).  This does mean that a bunch of this stuff is going to have to change,
> sadly.

The Download Manager used to build the list of downloads with the synchronous
storage API, but it read the results asynchronously, some rows at a time,
waiting some time between each set of rows. This effectively limited the
rate at which results were processed. The time interval between reads was
smaller for the first few items in the list, so that building the list was
initially faster, and then slowed down.

Using the asynchronous API, instead, we can't control the rate at which
results come in. Do you think we should generate the download list items
as soon as we get the results, or that we should buffer the results so
that we can still throttle at least the creation of the XUL nodes?

	Ed Lee :Mardak
	Comment 163 • 13 years ago

(In reply to comment #162)
> Using the asynchronous API, instead, we can't control the rate at which
> results come in.
The rate can still be slowed down by querying for a certain number of items then waiting then continuing with a new query.

	:Paolo Amadini Assignee
	Comment 164 • 13 years ago

(In reply to comment #163)
> The rate can still be slowed down by querying for a certain number of items
> then waiting then continuing with a new query.

Good idea, I can implement it if slowing down the query is actually useful.

With this method, I'm still afraid of getting the same download item twice
if I execute multiple LIMITed queries and a download is removed in the
meantime, even if I sort the results by start time. But, probably, this
is an edge case and I can just refresh the entire list if I'm notified
of such an event while I'm building the list.

	Shawn Wilsher :sdwilsh Reporter
	Comment 165 • 13 years ago

(In reply to comment #162)
> The Download Manager used to build the list of downloads with the synchronous
> storage API, but it read the results asynchronously, some rows at a time,
> waiting some time between each set of rows. This effectively limited the
> rate at which results were processed. The time interval between reads was
> smaller for the first few items in the list, so that building the list was
> initially faster, and then slowed down.
It was still reading the results synchronously, just in chunks.

> Using the asynchronous API, instead, we can't control the rate at which
> results come in. Do you think we should generate the download list items
> as soon as we get the results, or that we should buffer the results so
> that we can still throttle at least the creation of the XUL nodes?
We might have to try a few approaches here, actually.  Probably the first option is to dump the results in an array when we get them, and then build the DOM.

	Shawn Wilsher :sdwilsh Reporter
	Comment 166 • 13 years ago

This gets looked at Friday by me (either the current patch, or something new).

	Paul [pwd]
	Comment 167 • 13 years ago

With all this talk of switching from synchronous reads to asynchronous reads, will this fix bug 610775 and bug 610776?

	:Paolo Amadini Assignee
	Comment 168 • 13 years ago

(In reply to comment #166)
> This gets looked at Friday by me (either the current patch, or something new).

And now for something completely different :-)

	:Paolo Amadini Assignee
	Comment 169 • 13 years ago

(In reply to comment #167)
> With all this talk of switching from synchronous reads to asynchronous reads,
> will this fix bug 610775 and bug 610776?

No, sorry, download performance is different from user interface performance
and responsiveness, that these changes will improve.

	Shawn Wilsher :sdwilsh Reporter
	Comment 170 • 13 years ago

Comment on attachment 526282 [details] [diff] [review]
Work in progress, with separation of model, view, and controller [NOTE: has hg copies]

>+++ b/browser/components/downloads/Makefile.in
> ifdef ENABLE_TESTS
>-DIRS += test
>+DIRS += tests
> endif
You actually want to call the directory test and not tests.  I know we are inconsistent in the tree, but "test" is the right one.

>+++ b/browser/components/downloads/downloads.js
>+/**
>+ * Handles the Download Manager user interface and data access.
>+ *
>+ * This module includes the following classes and global objects:
This whole big comment is awesome.  My only comment on it so far is that "class" isn't really what you want.  You say they are global objects, and that's what they really are; just objects.  I wouldn't call them anything, and just say "DownloadsOverlay\n".

>+ * class DownloadsOverlay
>+ *       Handles the panel interface defined in "downloadsOverlay.xul".
I think this is better called DownloadsUI or DownloadsPanel.  Thoughts?

>+  /**
>+   * Updates the time that gets shown for completed download items.
>+   */
>+  updateTime: function DVI_updateTime()
So, this is basically the same as downloads.js for the toolkit UI.  Can you move this to DownloadUtils.jsm (in a patch that would be applied before this one to minimize patch sizes)?

>+++ b/toolkit/components/downloads/src/Makefile.in
> ifndef MOZ_SUITE
> # XXX - Until Suite builds off XULRunner we can't guarantee our implementation
> # of nsIDownloadManagerUI overrides toolkit's.
>+# This is also disabled when building the "browser" application since it also
>+# has its own implementation of nsIDownloadManagerUI.
>+ifneq (browser,$(MOZ_BUILD_APP))
> EXTRA_COMPONENTS = \
>   nsDownloadManagerUI.js \
>   nsDownloadManagerUI.manifest \
>   $(NULL)
> endif
>+endif
This really sucks and means that we run no tests on the toolkit UI, yeah?  We need to figure out how to resolve this in a better way...

This is probably going to take me the better part of a day to review when it's all ready.

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 171 • 13 years ago

May I ask you to put your button _before_ bookmarks-menu-button-container for now? Since that crazy bookmarks button magically moves to the toolbar, it's easier for the user to notice the movement if it's the last button on the right.
This is planned to change, but since I have no schedule I think for now it's better to avoid problems.

Also, you don't need that utils.js file imported in all browser-chrome tests, just rename it to head.js and it will automagically imported in each test.

And the change in nsBrowserGlue seems bogus, you should not hijack another migration step, just add a new one, otherwise I feel like that will cause you some headaches in future.

I just took a quick look at the patch for personal interest, I'm not a downloads peer, but if you want more eyes on this feel free to f? me.

	:Paolo Amadini Assignee
	Comment 172 • 13 years ago

(In reply to comment #170)
> >+ * class DownloadsOverlay
> >+ *       Handles the panel interface defined in "downloadsOverlay.xul".
> I think this is better called DownloadsUI or DownloadsPanel.  Thoughts?

It's just my habit to call the object that contains the "entry point"
functions (called by code outside the overlay) with the same name of
the JavaScript file, but DownloadsPanel is fine for me since that part
of the code is focused on opening and closing the panel itself.

> >+  /**
> >+   * Updates the time that gets shown for completed download items.
> >+   */
> >+  updateTime: function DVI_updateTime()
> So, this is basically the same as downloads.js for the toolkit UI.  Can you
> move this to DownloadUtils.jsm (in a patch that would be applied before this
> one to minimize patch sizes)?

Now that you say it, it fits there perfectly :-)

> >+++ b/toolkit/components/downloads/src/Makefile.in
> > ifndef MOZ_SUITE
> > # XXX - Until Suite builds off XULRunner we can't guarantee our implementation
> > # of nsIDownloadManagerUI overrides toolkit's.
> >+# This is also disabled when building the "browser" application since it also
> >+# has its own implementation of nsIDownloadManagerUI.
> >+ifneq (browser,$(MOZ_BUILD_APP))
> > EXTRA_COMPONENTS = \
> >   nsDownloadManagerUI.js \
> >   nsDownloadManagerUI.manifest \
> >   $(NULL)
> > endif
> >+endif
> This really sucks and means that we run no tests on the toolkit UI, yeah?  We
> need to figure out how to resolve this in a better way...

Ah, I didn't realize that this killed test coverage for the "classic" UI
on mozilla-central automated builds... but now that I think about it, if
mozilla-central automation only builds Firefox and the classic UI isn't
included in Firefox anymore, what can we do? Can we run tests without
including the unused "content" files in the final product?

This is something we should think about even if we are brave enough to
replace part of the Toolkit's UI implementation with the new one :-)
I've seen that the Add-ons Manager runs tests on mozilla-central for
a windowed version that isn't actually used in Firefox, right?

By the way, isn't "mozapps/downloads" the right place for the
"nsDownloadManagerUI.js" module, rather than "components/downloads"?

> This is probably going to take me the better part of a day to review when it's
> all ready.

I think so! And I'll need at least one more day of work to make things
shine, meaning that new patches might be ready sometime during next week.

	:Paolo Amadini Assignee
	Comment 173 • 13 years ago

(In reply to comment #171)
> May I ask you to put your button _before_ bookmarks-menu-button-container for
> now? Since that crazy bookmarks button magically moves to the toolbar, it's
> easier for the user to notice the movement if it's the last button on the
> right.

Of course, I can change this for now. In the final version, I'm not sure if
the panel's anchor will end up as toolbar button, rather than a notification
icon like the one that is shown for add-on downloads, to make the experience
consistent. Input from the UX team is welcome here!

> Also, you don't need that utils.js file imported in all browser-chrome tests,
> just rename it to head.js and it will automagically imported in each test.

Excellent :-)

> And the change in nsBrowserGlue seems bogus, you should not hijack another
> migration step, just add a new one, otherwise I feel like that will cause you
> some headaches in future.

Good catch, actually the change made sense in the version from one year ago.
Then I merged my patch on top of the latest trunk, messed up that part, and
then forgot about it :-) Thanks for pointing it out!

> I just took a quick look at the patch for personal interest, I'm not a
> downloads peer, but if you want more eyes on this feel free to f? me.

Thank you very much Marco! You needn't ask a second time :-)

The model - view - controller implementation is inspired by the Places
front-end code, and since I understand you worked in that area, I think
you may have valuable suggestions in this regard.

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 174 • 13 years ago

(In reply to comment #173)
> The model - view - controller implementation is inspired by the Places
> front-end code

NOW I'm scared! (just kidding...)

	Robert Kaiser
	Comment 175 • 13 years ago

(In reply to comment #170)
> This really sucks and means that we run no tests on the toolkit UI, yeah?  We
> need to figure out how to resolve this in a better way...

SeaMonkey's nsIDownloadManagerUI implementation has a pref to switch between the toolkit and suite UI, maybe Firefox wants to do that as well so at least the UI itself can be tested by setting that pref?

	Shawn Wilsher :sdwilsh Reporter
	Comment 176 • 13 years ago

(In reply to comment #175)
> SeaMonkey's nsIDownloadManagerUI implementation has a pref to switch between
> the toolkit and suite UI, maybe Firefox wants to do that as well so at least
> the UI itself can be tested by setting that pref?
We still end up not testing the component that we ship in toolkit for displaying the UI.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 177 • 13 years ago

(In reply to comment #172)
> I've seen that the Add-ons Manager runs tests on mozilla-central for
> a windowed version that isn't actually used in Firefox, right?

Yes, but I don't think that will help you here. The addons manager tests are just copied to a second directory (so they're in tests/browser/ and tests/browser-window/). The only difference is that the tests in /browser-window/ are run in a window, which is already supported by the code shipped in Firefox (it's just not exposed in Firefox's menus).

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 178 • 13 years ago

Feel free to skip answering each single line of feedback, since it would take double the size of this txt to do that :)
It's possible I missed some reasoning behind some of the choices, in such a case just ignore me, and I'll check the next version. I moved quickly through the View since it still looks a WIP with lots of TODOs, will check it better at the next iteration.

	(no longer active)
	Comment 179 • 13 years ago

Is there a build with this patch available?  I'd like to test how the changes to the interface look like in RTL mode.

	Robert Kaiser
	Comment 181 • 13 years ago

(In reply to comment #176)
> (In reply to comment #175)
> > SeaMonkey's nsIDownloadManagerUI implementation has a pref to switch between
> > the toolkit and suite UI, maybe Firefox wants to do that as well so at least
> > the UI itself can be tested by setting that pref?
> We still end up not testing the component that we ship in toolkit for
> displaying the UI.

Right, that's why I said "at least the UI".

	:Paolo Amadini Assignee
	Comment 182 • 13 years ago

(In reply to comment #178)
> Feel free to skip answering each single line of feedback, since it would take
> double the size of this txt to do that :)

Thanks - you know I would be tempted :-)

You'll find an answer to many comments in the next patch, except of course
for those that refer to code that has already disappeared (refactoring like
unifying DownloadsData was already underway). I've to say that I find this
feedback very useful, especially the JavaScript tips, even if on code that
will not be present in the next iteration.

There are still a few minor items, mostly related to style, for which I'm
not completely sure how to proceed. It would be better if we can sort out
these points early while I'm still writing the code:

* The main JavaScript file is included from an overlay of the browser window,
  and in the future some objects might even be moved elsewhere, thus I'd like
  to avoid global namespace pollution, leaving only the "Downloads..."
  objects (with only a few necessary exceptions). This is why
  "minItemCountForPopup" is a property, for example, which I actually
  considered calling "kMinItemCountForPopup". It can become a "const",
  but at the cost of being in the global namespace. Personally I'd prefer
  to keep it as a property, and even change other "const" to properties,
  but if there is a different recommendation I'm fine either way.

* While reviewing the initialization and termination, I realized that the
  "initialize" and "terminate" methods are not usually called from the
  outside (only test code does that), but they have a clearly defined
  purpose and interface: "initialize" allows preloading the download list
  in the background without opening the panel, while "terminate" frees
  the associated resources until they're needed again. Regardless of
  the name of the methods, which might be improved, should we mark them
  as "private" on the rationale that they're called only by test code?

* I guess methods and properties that are called or accessed by other
  objects, even in the same file, shouldn't be marked as "private".

* I'm accustomed to use "Iterator()" in for loops because it filters out
  properties added to "Object.prototype", for example by extensions that
  don't behave in the right way. Do we care making at least newly written
  front-end code robust to this eventuality?

On a final note, everything related to user interface styling is not only
not final, but also very different from the wanted appearance. I'd like to
focus on one thing at a time, thus I'll not work on improving CSS files for
some more time. However, I'll remember to take your suggestions into account
for the new style sheets at the right time.

Test code is mostly a copy from Toolkit's (it's not an "hg copy" anymore
because it confused things) and I'm not sure if I should spend as much time
to refactor it as I'm spending on production code.

	:Paolo Amadini Assignee
	Comment 183 • 13 years ago

(In reply to comment #179)
> Is there a build with this patch available?  I'd like to test how the changes
> to the interface look like in RTL mode.

User interface styling is far from final, but if you want to provide early
feedback you're welcome to download this intermediate tryserver build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/old/paolo%2Emozmail%40amadzone%2Eorg-ca777069fd10/

Note that this build might contain a few minor bugs (I've seen that the panel
doesn't always open automatically when it should, for example).

Also note that this build will expire and disappear very shortly.

	(no longer active)
	Comment 184 • 13 years ago

(In reply to comment #183)
> (In reply to comment #179)
> > Is there a build with this patch available?  I'd like to test how the changes
> > to the interface look like in RTL mode.
> 
> User interface styling is far from final, but if you want to provide early
> feedback you're welcome to download this intermediate tryserver build:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/old/paolo%2Emozmail%40amadzone%2Eorg-ca777069fd10/
> 
> Note that this build might contain a few minor bugs (I've seen that the panel
> doesn't always open automatically when it should, for example).
> 
> Also note that this build will expire and disappear very shortly.

Paolo, I tried this build in RTL mode, and it was really, really, *really* good!  :-)

I'm honestly surprised, I expected there to be RTL problems somewhere but I couldn't find any.  Keep up the good work!  :-)

	:Paolo Amadini Assignee
	Comment 185 • 13 years ago

(In reply to comment #184)
> Paolo, I tried this build in RTL mode, and it was really, really, *really*
> good!  :-)
> 
> I'm honestly surprised, I expected there to be RTL problems somewhere but I
> couldn't find any.  Keep up the good work!  :-)

Merit goes to those who wrote the original Download Manager and former Add-ons
Manager styles, which I mostly just copied. Thanks for the encouragement in
any case, I appreciate it very much!

And this reminds me to test the right-to-left layout with the Force RTL add-on
when the time comes to update the styles to match those now in Firefox 4 :-)

	:Paolo Amadini Assignee
	Comment 186 • 13 years ago

Comment on attachment 526902 [details]
feedback, round 1

I've addressed most of the feedback comments, thank you Marco for your help!

Some minor tweaks are still pending, thus I won't be able to attach an
updated patch before Thursday. Meanwhile, I've distilled those comments
that are not handled in the code yet:

> >+Cu.import("resource://gre/modules/DownloadCommon.jsm");
> 
> DownloadCommon is a browser module, so this should be resource://app/modules/...

Do I have to change something in "Makefile.in" if I make this modification?

> >+  onDrop: function DV_onDrop(aEvent)
> 
> Does this handle the uri-list case correctly?

I've not checked, but I guess not, if the current Download Manager doesn't.
This might be a follow-up if it's deemed useful. And, in any case, I think
the drop action should be handled by the download indicator in the final
version.

> >+    if (multiCommand) {
> >+      multiCommand.after.call(null, context); 
> >+    }
> 
> couldn't .before and .after handle an internal _context? and could not
> doCommand detect if it's inside before and after to figure out if this
> is a multiple command?

Not sure what you're asking...

> Any reason this menupopup is defined inside the panel rather than
> directly in the popupset?

I remember there were interaction problems when having two popups open if one
wasn't a child of the other. I've not re-tested it for almost a year, though.

> This is interesting, does the panel allow resizing? The problem is that
> so far tries to make the panel resizeable failed due to some platform
> resize bug. So I'm mostly asking if resizeability was taken into account
> (it could absolutely be a follow-up, though)

The styles used for the content should work correctly for any panel size,
if a minimum width is enforced.

> This is losing the attention flashing compared to the toolkit
> implementation, I guess this could be wanted for the first
> implementation but some sort of attention is planned for the
> button itself?

Yes, the button will handle attention and the progress indicator.

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 187 • 13 years ago

(In reply to comment #186)
> > >+Cu.import("resource://gre/modules/DownloadCommon.jsm");
> > 
> > DownloadCommon is a browser module, so this should be resource://app/modules/...
> 
> Do I have to change something in "Makefile.in" if I make this modification?

no there is no change required, as far as the Makefile.in is in browser/ this is a app module.

> > >+  onDrop: function DV_onDrop(aEvent)
> > 
> > Does this handle the uri-list case correctly?
> 
> I've not checked, but I guess not, if the current Download Manager doesn't.
> This might be a follow-up if it's deemed useful. And, in any case, I think
> the drop action should be handled by the download indicator in the final
> version.

Sure, if it's unsupported by the current manager, it's a follow-up (maybe also already filed)

> > >+    if (multiCommand) {
> > >+      multiCommand.after.call(null, context); 
> > >+    }
> > 
> > couldn't .before and .after handle an internal _context? and could not
> > doCommand detect if it's inside before and after to figure out if this
> > is a multiple command?
> 
> Not sure what you're asking...

mostly if this really needs .before and .after methods and wrapping variables (see context = {}) and could not be done with a callback, like .multipleCommandHandler(function () {do_stuff_here} );

> > This is interesting, does the panel allow resizing? The problem is that
> > so far tries to make the panel resizeable failed due to some platform
> > resize bug. So I'm mostly asking if resizeability was taken into account
> > (it could absolutely be a follow-up, though)
> 
> The styles used for the content should work correctly for any panel size,
> if a minimum width is enforced.

yes, I'm guessing if it works at all in reality though (see bug 652115)

	:Paolo Amadini Assignee
	Comment 188 • 13 years ago

(In reply to comment #187)
> > > >+    if (multiCommand) {
> > > >+      multiCommand.after.call(null, context); 
> > > >+    }
> > > 
> > > couldn't .before and .after handle an internal _context? and could not
> > > doCommand detect if it's inside before and after to figure out if this
> > > is a multiple command?
> > 
> > Not sure what you're asking...
> 
> mostly if this really needs .before and .after methods and wrapping variables
> (see context = {}) and could not be done with a callback, like
> .multipleCommandHandler(function () {do_stuff_here} );

Yes, we could pass an iterator as an argument to the "multiple" version of
the command, then iterate on the items with "for (let item in aItems)", or
use more advanced array comprehensions. The function will just access
"item" instead of using "this". We still need to know in advance which
commands want to handle multiple items in this way.

> > > This is interesting, does the panel allow resizing? The problem is that
> > > so far tries to make the panel resizeable failed due to some platform
> > > resize bug. So I'm mostly asking if resizeability was taken into account
> > > (it could absolutely be a follow-up, though)
> > 
> > The styles used for the content should work correctly for any panel size,
> > if a minimum width is enforced.
> 
> yes, I'm guessing if it works at all in reality though (see bug 652115)

Right, what I'm saying is that once bug 652115 is fixed there should be no
showstoppers due to the way the styles in the panel are written.

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 189 • 13 years ago

(In reply to comment #188)
> Right, what I'm saying is that once bug 652115 is fixed there should be no
> showstoppers due to the way the styles in the panel are written.

bug 652115 is a showstopper by itself if the first implementation wants to be resizeable, that's why I'm asking. Indeed in such a case that bug should be reprioritized and get someone assigned to it.

	:Paolo Amadini Assignee
	Comment 190 • 13 years ago

Here is an updated patch with all comments addressed. Files located directly
under "browser/components/downloads" are ready for a full coding style review,
though they will still be subject to incremental changes to reflect future
user experience iterations. Files in other folders, including "browser/themes"
and "browser/components/downloads/tests", will probably undergo major changes.

I've used the "feedback" flag since I'm requesting a full review from Shawn
only for a portion of the work-in-progress. Marco, if you want to see if
I've interpreted correctly your previous feedback, you're really welcome!

	:Paolo Amadini Assignee
	Comment 191 • 13 years ago

I went through the bug history to see if I could find comments that still
apply to the current version. Some might be more relevant, some are just
food for thought or may become follow-ups. This list is not sorted.

* Figure out how automated tests in mozilla-central builds could be executed
  for a Toolkit component that we don't use or include in Firefox.

* The openURL global function in Firefox 4 doesn't open a new tab and
  doesn't give any visual feedback if the target page is not available,
  for example because there is no network connection. This happens when
  the "open download page" link is clicked.

* The logic of _addOrMoveElementToTop and _addOrMoveElementToBottom might
  not be clear enough, this might be addressed by the code review.

* The "open" button should be disabled when the file doesn't exist.

* Manual tests for toolbar migration code are listed in comment 85.

* When there are many downloads, the panel expands all the way to the
  bottom of the screen, and a scrollbar appears, correctly. However,
  on Windows XP, the panel overlaps with the taskbar and the taskbar
  is on top of it, effectively hiding the "Downloads History" button.

* Do we want the download panel to be "detachable"? If, by clicking on a
  button or icon in the panel, you can open the standalone Download
  Manager, then you have a way to keep an eye on every single item of
  your current downloads. I'm not sure if we want to continue to support
  this use case.

* The Downloads view in the Library window does not allow to locate the
  target file, even if the file was downloaded successfully.

* Share the "open download" function, or at least its strings, with Toolkit.

* Behavior when a download starts and no browser window is open (comment 87).

* Behavior when a download finishes and no browser window is open. Steps
  to reproduce are written in comment 93.

* Share the new data access and user interface code with Toolkit.

* Isn't "mozapps/downloads" the right place for the "nsDownloadManagerUI.js"
  module, rather than "components/downloads"?

* Minor coding style questions (comment 182).

* Proceed with updating tests, rename "tests" to "test", migrate more tests.

	Shawn Wilsher :sdwilsh Reporter
	Comment 192 • 13 years ago

(In reply to comment #191)
> * Do we want the download panel to be "detachable"? If, by clicking on a
>   button or icon in the panel, you can open the standalone Download
>   Manager, then you have a way to keep an eye on every single item of
>   your current downloads. I'm not sure if we want to continue to support
>   this use case.
I suspect we do not want it to be detachable.

> * The Downloads view in the Library window does not allow to locate the
>   target file, even if the file was downloaded successfully.
We can patch that.  I even think mmmulani did a patch for that, so you might want to poke him (or at least adds it to the API so we can store it as an annotation).

> * Behavior when a download starts and no browser window is open (comment 87).
This only applies on OS X FWIW.

> * Behavior when a download finishes and no browser window is open. Steps
>   to reproduce are written in comment 93.
Same here.

> * Isn't "mozapps/downloads" the right place for the "nsDownloadManagerUI.js"
>   module, rather than "components/downloads"?
Yes it is.  Feel free to move it there (but file a different bug to do so please!).

I will try to look at this tomorrow, but if not Friday for sure!

	Shawn Wilsher :sdwilsh Reporter
	Comment 193 • 13 years ago

OK, so this is going to happen Friday and not today.  If you want to me give review on parts of it, I encourage you to split those parts out into a different patch and request review on it.  This makes things easier for bookkeeping, and then I don't need to look at it again (even accidentally) in future iterations of the mega patch, which results in a faster review turn around time for you! :D

	:Paolo Amadini Assignee
	Comment 194 • 13 years ago

(In reply to comment #193)
> OK, so this is going to happen Friday and not today.  If you want to me give
> review on parts of it, I encourage you to split those parts out into a
> different patch and request review on it.  This makes things easier for
> bookkeeping, and then I don't need to look at it again (even accidentally) in
> future iterations of the mega patch, which results in a faster review turn
> around time for you! :D

The point is that even those parts on which I'm requesting a more thorough
review (for example, main XUL files and non-skin CSS) are probably going to
change slightly in the next iteration, to reflect changes in the skin, and
those changes would need to be examined together in any case...

Since, if this feedback round on the JavaScript is OK, I'm going to begin skin
work and request full review on everything in the next iteration, which is
going to happen in days and not months from now, maybe keeping one patch is
actually simpler? You can do a thorough review on the main JavaScript in any
case tomorrow, since next week you'll still remember what you looked at :-)

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 195 • 13 years ago

Comment on attachment 528720 [details] [diff] [review]
Version with "components/downloads" ready for review

Review of attachment 528720 [details] [diff] [review]:

Some comments on tests.

You have not yet renamed utils.js to head.js in tests, and removed the useless import code from them.

::: browser/components/downloads/tests/browser_basic_functionality.js
@@ +1,3 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+

see the comments on the other test, most apply here as well, so I won't repeat them :)

::: browser/components/downloads/tests/browser_delete_key_removes.js
@@ +14,5 @@
+{
+  var dmFile = Services.dirsvc.get("TmpD", Ci.nsIFile);
+  dmFile.append("dm-ui-test.file");
+  dmFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0666);
+  var testPath = Services.io.newFileURI(dmFile).spec;

NetUtil.newURI manages also fileURI transparently iirc.

@@ +67,5 @@
+    var richlistbox = document.getElementById("downloadsView");
+    let stmt = dm.DBConnection
+                 .createStatement("SELECT COUNT(*) FROM moz_downloads");
+    try {
+      stmt.executeStep();

check with ok() this call?

@@ +98,5 @@
+
+  function runTests() {
+    // Now we can run our tests
+    for each (let t in testFuncs)
+      t();

nit: For code sanity, don't use for each on arrays. In this case it doesn't matter since this array is not going into the wild, but better not showing bad habits :) use testFuncs.forEach instead.
Also, maybe you could want to start managing these tests as async, even if they are currently synchronous in future they could change (like deleting an entry could become async) and making them fake async from the beginning would speed up fixing them. So this could rather be a runNextTest.

@@ +104,5 @@
+    dmFile.remove(false);
+    DownloadsPanel.onViewLoadCompleted = originalOnViewLoadCompleted;
+
+    // Unload the panel, discarding the current test data.
+    DownloadsPanel.terminate();

why not setCleanState? I suspect you want to cleanup after the test to avoid next tests to see leftover downloads, ideally you should do that in a function registered through registerCleanupFunction.

@@ +110,5 @@
+  }
+
+  // Hook to know when the test data has been loaded
+  var originalOnViewLoadCompleted = DownloadsPanel.onViewLoadCompleted;
+  DownloadsPanel.onViewLoadCompleted = function() {

nit: this is one of the reasons in future you could want an addObserver and removeObserver in the view and support for multiple observers.

@@ +112,5 @@
+  // Hook to know when the test data has been loaded
+  var originalOnViewLoadCompleted = DownloadsPanel.onViewLoadCompleted;
+  DownloadsPanel.onViewLoadCompleted = function() {
+    originalOnViewLoadCompleted.apply(this);
+    waitForFocus(function() setTimeout(runTests, 0));

waitForFocus already setTimeout(0), so this call seems useless.

@@ +117,5 @@
+  };
+
+  // Show the Download Manager UI
+  DownloadsPanel.showPanel();
+  waitForExplicitFinish();

nit: I suggest specifying this as first command in test() so that it's immediately clear the test is async.

::: browser/components/downloads/tests/utils.js
@@ +57,5 @@
+      let ios = Cc["@mozilla.org/network/io-service;1"].
+                getService(Ci.nsIIOService);
+      return (aObj instanceof Ci.nsIFile) ? ios.newFileURI(aObj) :
+                                            ios.newURI(aObj, null, null);
+    }

NetUtil.newURI does all of this this for you IIRC.

@@ +68,5 @@
+        let rnum = Math.floor(Math.random() * chars.length);
+        randomstring += chars.substring(rnum, rnum+1);
+      }
+      return randomstring;
+    }

A neat trick:
function randomString()
  Math.random().toString(36).replace(/^.*\./, '');

notice random is random, so if you need this to also be unique, add an external counter, increment and append it.

@@ +77,5 @@
+    persist.persistFlags = nsIWBP.PERSIST_FLAGS_REPLACE_EXISTING_FILES |
+                           nsIWBP.PERSIST_FLAGS_BYPASS_CACHE |
+                           nsIWBP.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;
+    let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
+                 getService(Ci.nsIProperties);

Services.dirsvc

@@ +110,5 @@
+    "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " +
+      "state, currBytes, maxBytes, preferredAction, autoResume) " +
+    "VALUES (:name, :source, :target, :startTime, :endTime, :state, " +
+      ":currBytes, :maxBytes, :preferredAction, :autoResume)");
+  for each (let dl in DownloadData) {

for code evangelization, don't use for each on arrays, use .forEach

@@ +115,5 @@
+    for (let prop in dl) {
+      stmt.params[prop] = dl[prop];
+    }
+
+    stmt.execute();

just for sanity, .reset() after executing

@@ +117,5 @@
+    }
+
+    stmt.execute();
+  }
+  stmt.finalize();

put into a finally

	Shawn Wilsher :sdwilsh Reporter
	Comment 196 • 13 years ago

(In reply to comment #195)
> +    stmt.execute();
> 
> just for sanity, .reset() after executing
execute calls reset (API guarantee), so calling reset is not needed.

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 197 • 13 years ago

(In reply to comment #196)
> execute calls reset (API guarantee), so calling reset is not needed.

my fault, I thought it was only for asyncExecute!

	Shawn Wilsher :sdwilsh Reporter
	Comment 198 • 13 years ago

Comment on attachment 528720 [details] [diff] [review]
Version with "components/downloads" ready for review

Review of attachment 528720 [details] [diff] [review]:

Plus all of mak's comments, this looks good.

::: browser/components/downloads/download.xml
@@ +25,5 @@
+#   Ben Goodger <ben@bengoodger.com> (v2.0) 
+#   Blake Ross <blakeross@telocity.com>
+#   Shawn Wilsher <me@shawnwilsher.com> (v3.0)
+#   Edward Lee <edward.lee@engineering.uiuc.edu>
+#   Paolo Amadini <http://www.amadzone.org/>

(v4.0) for you, yeah?

::: browser/components/downloads/downloads.js
@@ +115,5 @@
+  kPanelHidden: 1,
+  /** The panel will be shown as soon as possible. */
+  kPanelShowing: 2,
+  /** The panel is open, though download data might still be loading. */
+  kPanelShown: 3,

if you actually want this to be constants, you should implement them as getters.

@@ +207,5 @@
+  /**
+   * Indicates whether the panel is shown or will be shown.
+   */
+  get isPanelShowing() this._panelState == this.kPanelShowing ||
+                       this._panelState == this.kPanelShown,

Because this spans two lines here, I'd prefer it if you didn't use the magic here and used braces and all the normal JS stuff :)

@@ +339,5 @@
+   * element was already in the list, it is moved to the appropriate position.
+   *
+   * @param aElement
+   *        XUL element associated with the download item.
+   *

global-nit: no newline here

@@ +352,5 @@
+    this.richListBox.insertBefore(aElement, aDataItem.done
+                                            ? this.richListBox.firstChild
+                                            : !aDataItem.inProgress
+                                            ? this.firstElementNotDone
+                                            : this.firstElementInProgress);

This is all very complicated and hard to follow.  I suspect you can improve this by using a temporary variable or two.  You do this in a few places, so this comment applies there as well.

@@ +1702,5 @@
+                   , nsIDM.DOWNLOAD_QUEUED
+                   , nsIDM.DOWNLOAD_DOWNLOADING
+                   , nsIDM.DOWNLOAD_PAUSED
+                   , nsIDM.DOWNLOAD_SCANNING
+                   ].indexOf(this.state) >= 0,

Trailing commas are legal in arrays, and I'd rather see that here.  Also, since this spans multiple lines, use braces and a return statement please.

@@ +1717,5 @@
+  get done() [ nsIDM.DOWNLOAD_FINISHED
+             , nsIDM.DOWNLOAD_BLOCKED_PARENTAL
+             , nsIDM.DOWNLOAD_BLOCKED_POLICY
+             , nsIDM.DOWNLOAD_DIRTY
+             ].indexOf(this.state) >= 0,

same here

@@ +1729,5 @@
+   * Indicates whether the download stopped because of an error, and can be
+   * resumed manually.
+   */
+  get canRetry() this.state == nsIDM.DOWNLOAD_CANCELED ||
+                 this.state == nsIDM.DOWNLOAD_FAILED,

and here (going to stop commenting on this, but it applies everywhere)

::: browser/components/downloads/tests/browser_basic_functionality.js
@@ +13,5 @@
+ */
+function test()
+{
+  var dmFile = Services.dirsvc.get("TmpD", Ci.nsIFile);
+  dmFile.append("dm-ui-test.file");

I can one-up mak's suggestion: FileUtils.getFile("TmpD", ["dm-ui-test.file"]);

::: browser/components/downloads/tests/utils.js
@@ +57,5 @@
+      let ios = Cc["@mozilla.org/network/io-service;1"].
+                getService(Ci.nsIIOService);
+      return (aObj instanceof Ci.nsIFile) ? ios.newFileURI(aObj) :
+                                            ios.newURI(aObj, null, null);
+    }

As mak suspected, this whole method can be nuked and you can just use NetUtil.newURI everywhere.  NetUtil is already pulled into browser.js too I think, so you don't even have to import it.

::: browser/components/nsBrowserGlue.js
@@ +1039,5 @@
     box.persistence = -1; // Until user closes it
   },
 
   _migrateUI: function BG__migrateUI() {
+    const UI_VERSION = 6;

Amusing that this might coincide with the browser version...

	Shawn Wilsher :sdwilsh Reporter
	Comment 199 • 13 years ago

(In reply to comment #197)
> my fault, I thought it was only for asyncExecute!
Nope, just executeStep needs reset called afterwards.

	:Paolo Amadini Assignee
	Comment 200 • 13 years ago

Marco, Shawn, many thanks for the detailed reviews!

Actually, right now I was rewriting the entire test suite using the
generator-based asynchronous browser-chrome test runner of Toolkit.
Everything is much shorter and neater now. Marco, sorry if it seemed
I forgot to address your comments. In fact, I've already implemented
most of your old and new suggestions during my rewrite!

I'll implement some tips like using NetUtils now, and I'll wrap up the
new patch with the test suite for you to see.

	:Paolo Amadini Assignee
	Comment 201 • 13 years ago

Shawn, Marco, if you want to take a look, in this version the tests in
the "browser/components/downloads/test" folder are reimplemented. Note
that this patch is also regenerated upon a recent trunk revision.

The only thing I'd want to draw your attention to, before you look at the
patch, is that in the generator-iterator world I have to use an iteration
whenever I want some tasks to be delegated to an inner generator. Inner
functions cannot be used directly, by the way this is why you'll never
see "myArray.forEach(...)" but only "for ... in Iterator(myArray)".

Iteration calls for subroutines look like this:

  for (yy in gen_example("Parameter")) yield;

Apart for that, control flow is similar to a normal synchronous piece of
code (including iterations and flow of finally statements), and test
functions can be read and followed more or less in the same way as normal
functions. The "yield" causes the test runner to wait for the next event.

(In reply to comment #198)
> NetUtil is already pulled into browser.js too I think, so you don't even
> have to import it.

Now, I have to import it since I'm using it in the global scope.

	Marco Zehe (:MarcoZ)
	Comment 202 • 13 years ago

Paolo, have you kicked off a try-server build with the latest patch? I'd like to run this through some tests to see if everything is still properly exposed to assistive technologies for people with disabilities.
Thanks!

	:Paolo Amadini Assignee
	Comment 203 • 13 years ago

Finally, this is the first version of the patch where I checked every single
line of code, and is thus ready for final code review. This does not mean that
the feature is complete, but only that subsequent changes can be laid out
incrementally and reviewed separately.

The draft theming is based on the popup notifications of Firefox 4. The
temporary toolbar button is near the notifications anchor by default.

The required changes include adding more automated test cases, designing the
best user interaction, and refining visual styles for all platforms.

Dependent bugs should also be resolved before the whole feature can be
completed. They include replacing the downloads button with an appropriate
download progress and status indicator. This activity is not started yet.