564934 - (DownloadsPanel) Implement new Download Manager UI for browser

Status: RESOLVED WORKSFORME

(Firefox :: Downloads Panel, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Shawn Wilsher :sdwilsh]

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

Comment 2

[Jason Sonnenschein]

Attachment: Some canvas animations for a downloads badge

Comment 3

[Jason Sonnenschein]

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

Comment 4

[Jason Sonnenschein]

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.

Comment 5

[Jason Sonnenschein]

Attachment: screenshot

Comment 6

[Jason Sonnenschein]

Attachment: addon implementation of download badge UI

Comment 7

[Jason Sonnenschein]

Attachment: Addon implementation of download badge UI

Fixed some layout problems in MS Windows

Comment 8

[Jason Sonnenschein]

Attachment: Addon implementation of download badge UI

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

Comment 9

[Shawn Wilsher :sdwilsh]

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!

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: What I see.

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

Comment 11

[Jason Sonnenschein]

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

Comment 12

[Jason Sonnenschein]

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)

Comment 13

[Jason Sonnenschein]

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

Comment 14

[Shawn Wilsher :sdwilsh]

(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

Comment 15

[Jason Sonnenschein]

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

Comment 16

[Shawn Wilsher :sdwilsh]

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

Comment 17

[Jason Sonnenschein]

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

Comment 18

[Jason Sonnenschein]

Attachment: Visual comparison of my ETA strings vs Firefox

Comment 19

[Jason Sonnenschein]

Attachment: Addon implementation of download badge UI

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

Comment 20

[Jason Sonnenschein]

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

Comment 21

[Jason Sonnenschein]

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.

Comment 22

[Neil Deakin]

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

Comment 23

[Jason Sonnenschein]

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.

Comment 24

[Jason Sonnenschein]

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?

Comment 25

[Jason Sonnenschein]

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.

Comment 26

[Jason Sonnenschein]

Attachment: Patch v1

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?

Comment 27

[Jason Sonnenschein]

Attachment: patch v2

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

Comment 28

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

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?

Comment 29

[Jason Sonnenschein]

OK.  Changed reviewer

Comment 30

[Shawn Wilsher :sdwilsh]

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

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 32

[Jason Sonnenschein]

Attachment: For UI review by Limi

Comment 33

[Jason Sonnenschein]

Attachment: Patch v3

Fix for middle/right click issues reported by Wes

Comment 34

[Shawn Wilsher :sdwilsh]

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/

Comment 35

[Jason Sonnenschein]

Attachment: Patch v4

Comment 36

[Jason Sonnenschein]

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.

Comment 37

[Ed Lee :Mardak]

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?

Comment 38

[Jason Sonnenschein]

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

Comment 39

[Jason Sonnenschein]

Attachment: For UI Review by Limi

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

Comment 40

[Jason Sonnenschein]

Attachment: For UI Review by Limi

Comment 41

[Mehdi Mulani [:mmm] (I don't check this)]

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

Comment 42

[Jason Sonnenschein]

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?

Comment 43

[Jason Sonnenschein]

Attachment: Patch v5

Implements most requests of [e-mail] (cleaning up code readability, etc.) Also found a race condition and coded a fix.  Adds file icons to the downloads window.

Comment 44

[Jason Sonnenschein]

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.

Comment 45

[:Paolo Amadini]

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.

Comment 46

[:Paolo Amadini]

Attachment: Screenshot with addition of a button for "open"

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.

Comment 47

[:Paolo Amadini]

Attachment: Experiment for moving the DM to a panel

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.

Comment 48

[imradyurrad]

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.

Comment 49

[:Paolo Amadini]

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

Comment 50

[Paul [pwd]]

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

Comment 51

[Shawn Wilsher :sdwilsh]

(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

Comment 52

[:Paolo Amadini]

Attachment: Screenshot of all download types

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

Comment 53

[Michael Monreal [:micmon]]

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

Comment 54

[:Paolo Amadini]

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

Comment 55

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 56

[:Paolo Amadini]

Attachment: Mostly working (Windows theme only)

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.

Comment 57

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 58

[Alex Limi (:limi) — Firefox UX Team]

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.

Comment 59

[Ed Lee :Mardak]

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.

Comment 60

[Ed Lee :Mardak]

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.

Comment 61

[Shawn Wilsher :sdwilsh]

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

Comment 62

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 63

[Ed Lee :Mardak]

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

Comment 64

[Paul [pwd]]

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

Comment 65

[:Paolo Amadini]

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.

Comment 66

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 67

[:Paolo Amadini]

Attachment: [Comment 67] Old workflow for downloading and running a setup program

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.

Comment 68

[:Paolo Amadini]

Attachment: [Comment 68] New workflow for downloading and running a setup program

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!

Comment 69

[:Paolo Amadini]

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

Comment 70

[:Paolo Amadini]

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

Comment 71

[:Paolo Amadini]

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

Comment 72

[:Paolo Amadini]

Attachment: Far from finished, but ready for code review

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

Comment 73

[:Paolo Amadini]

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?

Comment 74

[Dietrich Ayala (:dietrich)]

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

Comment 75

[Johnathan Nightingale [:johnath]]

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

Comment 76

[Dave Garrett]

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.

Comment 77

[Johnathan Nightingale [:johnath]]

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.

Comment 78

[:Paolo Amadini]

Attachment: Feature-complete version for review (Windows theme only)

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

Comment 79

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 80

[Shawn Wilsher :sdwilsh]

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 <[e-mail]>

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.

Comment 81

[Ed Lee :Mardak]

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

Comment 82

[:Paolo Amadini]

(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 <[e-mail]>
> 
> 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.

Comment 83

[Shawn Wilsher :sdwilsh]

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

Comment 84

[Shawn Wilsher :sdwilsh]

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

Comment 85

[:Paolo Amadini]

Attachment: Updated feature-complete version (Windows theme only) [NOTE: has hg copies]

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.

Comment 86

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 87

[Shawn Wilsher :sdwilsh]

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

Comment 88

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 89

[Shawn Wilsher :sdwilsh]

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

Comment 90

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 91

[Dave Garrett]

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.

Comment 92

[Shawn Wilsher :sdwilsh]

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

Comment 93

[:Paolo Amadini]

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

Comment 94

[Shawn Wilsher :sdwilsh]

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.

Comment 95

[Leman Bennett [Omega]]

(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"?

Comment 96

[Shawn Wilsher :sdwilsh]

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

Comment 97

[Ivan Stetsenko]

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

Comment 98

[:Paolo Amadini]

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

Comment 99

[Shawn Wilsher :sdwilsh]

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

Comment 100

[:Paolo Amadini]

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.

Comment 101

[Peter Henkel [:Terepin]]

So this will not be in FX4?

Comment 102

[Dave Garrett]

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.

Comment 103

[Peter Henkel [:Terepin]]

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

Comment 104

[José Jeria]

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

Comment 105

[Axel Hecht [:Pike]]

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

Comment 106

[Shawn Wilsher :sdwilsh]

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

Comment 107

[Axel Hecht [:Pike]]

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.

Comment 108

[Ed Lee :Mardak]

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

Comment 109

[Axel Hecht [:Pike]]

Ah, now I get it, thanks.

Comment 110

[Peter Henkel [:Terepin]]

This isn't intended for 4.0?

Comment 111

[Dave Garrett]

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

Comment 112

[Peter Henkel [:Terepin]]

I'll prey for miracle then. :)

Comment 113

[Shawn Wilsher :sdwilsh]

This is not happening for Firefox 4.  Sorry folks.

Comment 114

[Nicolas Mandil (:NicolasWeb)]

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

Comment 115

[Po-chiang Chao [:bobchao]]

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

Comment 116

[Nicolas Mandil (:NicolasWeb)]

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

Comment 117

[Dave Garrett]

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.

Comment 118

[Axel Hecht [:Pike]]

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

Comment 119

[Dave Garrett]

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

Comment 120

[Dave Garrett]

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)

Comment 121

[Shawn Wilsher :sdwilsh]

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

Comment 122

[Nicolas Mandil (:NicolasWeb)]

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

Comment 123

[Dave Garrett]

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

Comment 124

[Peter Henkel [:Terepin]]

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.

Comment 125

[Josh Tumath]

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

Comment 126

[Peter Henkel [:Terepin]]

Bug 597145

Comment 127

[Nicolas Mandil (:NicolasWeb)]

(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

Comment 128

[DRC]

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!

Comment 129

[bogas04]

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

Comment 130

[Martin Arndt]

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

Comment 131

[bogas04]

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

Comment 132

[Martin Arndt]

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

Comment 133

[bogas04]

:)

Comment 134

[Shawn Wilsher :sdwilsh]

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

Comment 135

[:Paolo Amadini]

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

Comment 136

[Shawn Wilsher :sdwilsh]

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

Comment 137

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 138

[:Paolo Amadini]

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.

Comment 139

[Alex Limi (:limi) — Firefox UX Team]

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

Comment 142

[Peter Henkel [:Terepin]]

Any reason why was attachment deleted?

Comment 143

[Paul [pwd]]

It was the same as the patch with extra buttons.

Comment 144

[:Paolo Amadini]

Attachment: Updated feature-complete version [NOTE: has hg copies]

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/

Comment 145

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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

Comment 146

[Vova Olar]

Shouldn't it be an ArrowPanel??

Comment 147

[Siddhartha Dugar [:sdrocking]]

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.

Comment 148

[:Paolo Amadini]

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.

Comment 149

[:Paolo Amadini]

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

Comment 150

[bogas04]

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

Comment 151

[Paul [pwd]]

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

Comment 152

[Shawn Wilsher :sdwilsh]

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

Comment 153

[Paul [pwd]]

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

Comment 154

[Shawn Wilsher :sdwilsh]

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.

Comment 155

[:Paolo Amadini]

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

Comment 156

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

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

Comment 157

[:Paolo Amadini]

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

Comment 158

[:Paolo Amadini]

Attachment: Intermediate version [NOTE: has hg copies]

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.

Comment 159

[:Paolo Amadini]

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

Comment 160

[Shawn Wilsher :sdwilsh]

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

Comment 161

[Shawn Wilsher :sdwilsh]

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

Comment 162

[:Paolo Amadini]

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

Comment 163

[Ed Lee :Mardak]

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

Comment 164

[:Paolo Amadini]

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

Comment 165

[Shawn Wilsher :sdwilsh]

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

Comment 166

[Shawn Wilsher :sdwilsh]

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

Comment 167

[Paul [pwd]]

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

Comment 168

[:Paolo Amadini]

Attachment: Work in progress, with separation of model, view, and controller [NOTE: has hg copies]

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

Comment 169

[:Paolo Amadini]

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

Comment 170

[Shawn Wilsher :sdwilsh]

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.

Comment 171

[Marco Bonardo [:mak]]

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.

Comment 172

[:Paolo Amadini]

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

Comment 173

[:Paolo Amadini]

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

Comment 174

[Marco Bonardo [:mak]]

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

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

Comment 175

[Robert Kaiser]

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

Comment 176

[Shawn Wilsher :sdwilsh]

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

Comment 177

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 178

[Marco Bonardo [:mak]]

Attachment: feedback, round 1

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.

Comment 179

[(no longer active)]

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

Comment 181

[Robert Kaiser]

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

Comment 182

[:Paolo Amadini]

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

Comment 183

[:Paolo Amadini]

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

Comment 184

[(no longer active)]

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

Comment 185

[:Paolo Amadini]

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

Comment 186

[:Paolo Amadini]

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.

Comment 187

[Marco Bonardo [:mak]]

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

Comment 188

[:Paolo Amadini]

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

Comment 189

[Marco Bonardo [:mak]]

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

Comment 190

[:Paolo Amadini]

Attachment: Version with "components/downloads" ready for review

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!

Comment 191

[:Paolo Amadini]

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.

Comment 192

[Shawn Wilsher :sdwilsh]

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

Comment 193

[Shawn Wilsher :sdwilsh]

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

Comment 194

[:Paolo Amadini]

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

Comment 195

[Marco Bonardo [:mak]]

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

Comment 196

[Shawn Wilsher :sdwilsh]

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

Comment 197

[Marco Bonardo [:mak]]

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

Comment 198

[Shawn Wilsher :sdwilsh]

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 <[e-mail]> (v2.0) 
+#   Blake Ross <[e-mail]>
+#   Shawn Wilsher <[e-mail]> (v3.0)
+#   Edward Lee <[e-mail]>
+#   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...

Comment 199

[Shawn Wilsher :sdwilsh]

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

Comment 200

[:Paolo Amadini]

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.

Comment 201

[:Paolo Amadini]

Attachment: Intermediate version with "components/downloads/test" ready for review

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.

Comment 202

[Marco Zehe (:MarcoZ)]

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!

Comment 203

[:Paolo Amadini]

Attachment: Base version

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.

Comment 204

[:Paolo Amadini]

(In reply to comment #202)
> 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.

Thank you for your help Marco! I've started a tryserver build right now, based
on the latest patch, which includes an updated theme and a slightly different
interaction. Final builds should be available here in a few hours:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/paolo%2Emozmail%40amadzone%2Eorg-2eaa871313d1

Comment 205

[Frank Yan (:fryn)]

Paolo, thanks for writing the patch! You are awesome!

I've only found 3 issues:

1) I downloaded a file. After it was finished, I opened a new window and clicked on the Downloads toolbar button. It did not display the download, and the following was logged in the Error Console:
Error: DownloadUtils.getRecentDateTime is not a function
Source File: chrome://browser/content/downloads/downloads.js
Line: 869

2) At least on OS X, the dark text is very difficult to read, since its background is transparent on top of the dark panel.
Screenshot immediately upon clicking the Downloads toolbar button:
https://people.mozilla.com/~fyan/screenshots/downloads-ui-before-click.png
Screenshot after I clicked on the download:
https://people.mozilla.com/~fyan/screenshots/downloads-ui-after-click.png

3) I think the Downloads toolbar button would be better placed between the #search-container and #home-button. I don't think we want any more stuff between the back/forward button and the URL bar. Please get ui-review before landing.

Those issues aside, this patch provides a great base. Thanks again! :)

Comment 206

[Marco Zehe (:MarcoZ)]

Hm, so far I don't see *anything* on Windows using the NVDA screen reader. When I start a download, I'm thrown back to the page where I started the download from, and in all of the accessibility hierarchy, I don't find anything that looks like the downloads panel. Could this be of the JS error that was mentioned, that it simply doesn't work at all yet in the try build?

Comment 207

[Marco Bonardo [:mak]]

(In reply to comment #205)
> 3) I think the Downloads toolbar button would be better placed between the
> #search-container and #home-button. I don't think we want any more stuff
> between the back/forward button and the URL bar.

looks like a bug in the glue code, or you moved the bookmarks button, since this downloads button should be put right before the bookmarks button (whose default is being the last button on the right). Did you try in a clean profile?

Comment 208

[a4010956]

I tried your try-server build on win7 on a new profile - and it doesn't work. Downloads window doesn't get opened.
Seems like patch is not working for win-machines.

Comment 209

[Antoine Turmel [:GeekShadow]]

(In reply to comment #204)
> (In reply to comment #202)
> > 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.
> 
> Thank you for your help Marco! I've started a tryserver build right now, based
> on the latest patch, which includes an updated theme and a slightly different
> interaction. Final builds should be available here in a few hours:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/paolo%2Emozmail%40amadzone%2Eorg-2eaa871313d1

Download panel is not working for me :

Error: DownloadsPanel is undefined
Source File: chrome://browser/content/browser.xul
Line: 1

Comment 210

[Antoine Turmel [:GeekShadow]]

I forget to precise, i'm using linux build

Comment 211

[Axel Hecht [:Pike]]

I didn't get any love on the mac build either, on an existing Nightly profile, fwiw.

Comment 212

[Shawn Wilsher :sdwilsh]

Unfortunately, I'm traveling the so the soonest I'll be able to look at this is Monday, May 9th.

Comment 213

[HRipper]

(In reply to comment #211)
> I didn't get any love on the mac build either, on an existing Nightly profile,
> fwiw.

not working also on windows.

Comment 214

[:Paolo Amadini]

Thanks to everyone that tried out the tryserver builds! Unfortunately, it
looks like those builds don't work at all, while the same base patch
compiled locally on my machine and on Frank's one seems to work (Frank,
see below for the reason you see the JavaScript error).

There is certainly a difference in the build environment or configurations.
Although I can't look into this problem in detail now, I've started a new
tryserver job for the same patch set, this time building both debug and
optimized versions, configured to run automated tests. Let's see - I'll
probably be able to look at the results tomorrow or the day after.

(In reply to comment #205)
> 1) I downloaded a file. After it was finished, I opened a new window and
> clicked on the Downloads toolbar button. It did not display the download, and
> the following was logged in the Error Console:
> Error: DownloadUtils.getRecentDateTime is not a function
> Source File: chrome://browser/content/downloads/downloads.js
> Line: 869

This is probably due to the fact that this patch also requires the ones on
dependent bugs, at a minimum the one on bug 653261. Note that patches on
the other dependent bugs might not apply cleanly on latest trunk, I've
refreshed them in my Mercurial patch queue to make them apply.

> 2) At least on OS X, the dark text is very difficult to read, since its
> background is transparent on top of the dark panel.
> Screenshot immediately upon clicking the Downloads toolbar button:
> https://people.mozilla.com/~fyan/screenshots/downloads-ui-before-click.png
> Screenshot after I clicked on the download:
> https://people.mozilla.com/~fyan/screenshots/downloads-ui-after-click.png

Definitely a theming bug. I'll work around it, but the new Download Manager
will require a serious theming effort (from someone else) for all platforms.
Personally, I think even the current appearance details on Windows are far
from ideal, they're just there to give a rough idea and a starting point.

Comment 215

[:Paolo Amadini]

Note that there won't be a "downloads button" in the final version.

(In reply to comment #207)
> (In reply to comment #205)
> > 3) I think the Downloads toolbar button would be better placed between the
> > #search-container and #home-button. I don't think we want any more stuff
> > between the back/forward button and the URL bar.
> 
> looks like a bug in the glue code, or you moved the bookmarks button, since
> this downloads button should be put right before the bookmarks button (whose
> default is being the last button on the right). Did you try in a clean profile?

I've intentionally moved the temporary downloads button to this weird position
in this iteration, so that the panel can open with the same position and
orientation as the so-called "popup notifications" (the ones displayed for
add-on downloads, for example). Whether downloads will be integrated with
popup notifications or kept separate, is a user experience design decision
that should be evaluated in a greater context. In both cases, the place to
which the Download Manager panel arrow points will have a very different
look and feel from the button you see now.

Note that no serious user interaction design work has started yet. It will
probably start after the initial assessment of code stability. Despite that,
code review, accessibility review, and basic functionality testing on the
current patch are welcome, to establish a baseline we can build upon.

Comment 216

[Axel Hecht [:Pike]]

Comment on attachment 529878 [details] [diff] [review]
Base version

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

I only glanced at the locales/en-US files, so no review for non-localized UI strings or RTL issues or somesuch. But I got a few nits either way.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +11,5 @@
+<!ENTITY cmd.cancel.accesskey             "C">
+<!ENTITY cmd.show.label                   "Open Containing Folder">
+<!ENTITY cmd.show.accesskey               "F">
+<!ENTITY cmd.showMac.label                "Show in Finder">
+<!ENTITY cmd.showMac.accesskey            "F">

A localization note would be good here to emphasize that cmd.show and cmd.showMac are never shown in the same build, and thus it's perfectly fine to have the accesskeys of the two to conflict with each other in the source.

::: browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ +1,2 @@
+# LOCALIZATION NOTE (paused): â is the "em dash" (long dash)
+paused=Paused â  #1

Nit, the double whitespace after the em dash might be confusing.

@@ +20,5 @@
+stateCanceled=Canceled
+# LOCALIZATION NOTE (stateBlocked): 'Parental Controls' should be capitalized
+stateBlockedParentalControls=Blocked by Parental Controls
+# LOCALIZATION NOTE (stateBlockedPolicy): 'Security Zone Policy' should be capitalized 
+stateBlockedPolicy=Blocked by your Security Zone Policy

Are Parental Controls and Security Zone Policy elsewhere in our UI?

Also, I'm not sure if capitalized is right, it should probably be consistently named and spelled with the display of those features on Windows in that localization?

@@ +27,5 @@
+
+# LOCALIZATION NOTE (yesterday): Displayed time for files finished yesterday
+yesterday=Yesterday
+# LOCALIZATION NOTE (monthDate): #1 month name; #2 date number; e.g., January 22
+monthDate=#1 #2

I'm tempted to say that English is the odd case in terms of Month/Day, maybe make en-US be "#2 #1"?

Comment 217

[Shawn Wilsher :sdwilsh]

(In reply to comment #216)
> I'm tempted to say that English is the odd case in terms of Month/Day, maybe
> make en-US be "#2 #1"?
en-us should most certainly be "January 22" instead of "22 January".  Other English locales may differ, however.

Comment 218

[Ed Lee :Mardak]

I believe the suggestion was that make en-us #2 #1 and change the code to be flipped so that the common case for other locales is "#1 #2".

Comment 219

[Axel Hecht [:Pike]]

(In reply to comment #218)
> I believe the suggestion was that make en-us #2 #1 and change the code to be
> flipped so that the common case for other locales is "#1 #2".

Yes.

Comment 220

[Shawn Wilsher :sdwilsh]

(In reply to comment #219)
> (In reply to comment #218)
> > I believe the suggestion was that make en-us #2 #1 and change the code to be
> > flipped so that the common case for other locales is "#1 #2".
> Yes.
Ah, apologies.  That was unclear to me :)

Comment 221

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

The problem with the patch that caused the tryserver builds not to work is that "resource://app/modules/DownloadCommon.jsm" doesn't exist, because they are omnijar builds, and bug 620931 isn't fixed. You can use "resource:///modules/DownloadCommon.jsm" to refer to it in the mean time as a workaround.

Comment 222

[a4010956]

(In reply to comment #221)
> The problem with the patch that caused the tryserver builds not to work is that
> "resource://app/modules/DownloadCommon.jsm" doesn't exist, because they are
> omnijar builds, and bug 620931 isn't fixed. You can use
> "resource:///modules/DownloadCommon.jsm" to refer to it in the mean time as a
> workaround.

I have copied omni.jar/modules/DownloadCommon.jsm to omni.jar/app/modules/DownloadCommon.jsm and it didn't help.

Comment 223

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

You'd need to copy it to <appdir>/modules. But someone should just fix the patch and make new builds :)

Comment 224

[:Paolo Amadini]

(In reply to comment #221)
> The problem with the patch that caused the tryserver builds not to work is that
> "resource://app/modules/DownloadCommon.jsm" doesn't exist, because they are
> omnijar builds, and bug 620931 isn't fixed. You can use
> "resource:///modules/DownloadCommon.jsm" to refer to it in the mean time as a
> workaround.

Wow... thanks. I manually checked if all files were present in "omni.jar",
checked the "chrome:" URLs, but didn't think about "resource:" at the moment!
The fact that Cu.import failures didn't show up in the Error Console, or the
test logs, didn't help. Is the latter a known issue?

I started a new tryserver build with those lines changed. For the patch,
I'll batch this change together with localization improvements and review
comments after Monday.

Comment 225

[:Paolo Amadini]

New tryserver builds available here for accessibility tests and early feedback
(though comment 215 is a recommended reading if you're testing user experience):

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/paolo%2Emozmail%40amadzone%2Eorg-d28ebe022bea

There is another weird difference between my local build and this one, at least
on Windows: in my local version, the downloads panel opens automatically
whenever a new download starts, while in the tryserver build it does not
open, and no errors are reported in the Error Console. Clicking the temporary
download button after the download starts, instead, opens the panel as expected
in both builds.

Comment 226

[:Paolo Amadini]

(In reply to comment #216)
> ::: browser/locales/en-US/chrome/browser/downloads/downloads.properties
> @@ +1,2 @@
> +# LOCALIZATION NOTE (paused): â is the "em dash" (long dash)
> +paused=Paused â  #1
> 
> Nit, the double whitespace after the em dash might be confusing.

With regard to strings like these ones, I don't see why different locales
would want to change the "em dash" to something else, change the spacing,
or change the order of the placeholders. Seems like "zh-CN" and "ja" use
normal spacing. Strangely, "ko" gets rid of the entire " - #1". The latter
seems like a possible localization bug, but maybe I'm missing something?

http://mxr.mozilla.org/l10n-central/source/ko/toolkit/chrome/mozapps/downloads/downloads.properties

Wouldn't it make more sense to handle this formatting in the presentation,
and include only the pure strings (e.g. "Paused") in the localization? This
could also give more flexibility to the presentation, for example we might
want a bold label saying "Canceled" or "Paused", but a plain domain name.

Previously I tried to keep the strings similar to their counterpart in
Toolkit, but at this point everything else is basically brand new, so I
think we are free to change these too.

> +# LOCALIZATION NOTE (yesterday): Displayed time for files finished yesterday
> +yesterday=Yesterday
> +# LOCALIZATION NOTE (monthDate): #1 month name; #2 date number; e.g., January
> 22
> +monthDate=#1 #2
> 
> I'm tempted to say that English is the odd case in terms of Month/Day, maybe
> make en-US be "#2 #1"?

Sorry, these two lines are leftovers from previous incarnations - I've
removed them now. See bug 653261. I assume we don't want to change the
order of the elements in Toolkit as those have been already localized.

Comment 227

[Gabriela [:gaby2300]]

In my case, downloading with the try server build the download panel does not show automatically, in fact I know there is a download going on because the Nightly icon in the task bar glows green. The new download panel shows only if I click the temporary download button after the download starts.
I'm on Windows 7.

Comment 228

[Darren Kalck [:D-Kalck]]

Gabriela, please read comment 225.
The download panel should have scrollbars when there's too much downloads in the list. With current tryserver build, if there's too much downloads only the 17 first (on a 1920x1200 screen) are reachable.

Comment 229

[Gabriela [:gaby2300]]

(In reply to comment #228)
> Gabriela, please read comment 225.
> The download panel should have scrollbars when there's too much downloads in
> the list. With current tryserver build, if there's too much downloads only
> the 17 first (on a 1920x1200 screen) are reachable.

Darren, I didn't mention scrollbars in my comment so I'm afraid I don't understand yours.

Comment 230

[:Paolo Amadini]

(In reply to comment #227)
> In my case, downloading with the try server build the download panel does
> not show automatically, in fact I know there is a download going on because
> the Nightly icon in the task bar glows green. The new download panel shows
> only if I click the temporary download button after the download starts.
> I'm on Windows 7.

Thanks for confirming that this affects Windows 7 also, and not only my machine!

(In reply to comment #228)
> The download panel should have scrollbars when there's too much downloads in
> the list. With current tryserver build, if there's too much downloads only
> the 17 first (on a 1920x1200 screen) are reachable.

Thanks for the feedback: this looks like a regression in panels with
type="arrow". They are not limited by the height of the screen, while
normal panels are (though they overlap with the task bar on Windows).
These issues should be looked into by someone familiar with arrow panels.

Comment 231

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/paolo%2Emozmail%40amadzone%2Eorg-d28ebe022bea

After using this test build, I seem some ploblems at the latest design.

First, I cannot recognize the current status of download if DL icon remoed from toolbar.
Second, Download Manager is not opened from shortcut key.
Third, "Download" pane in Places library does not provide the download status.

Are these points fix in the future?

Comment 232

[Darren Kalck [:D-Kalck]]

(In reply to comment #229)
> (In reply to comment #228)
> > Gabriela, please read comment 225.
> > The download panel should have scrollbars when there's too much downloads in
> > the list. With current tryserver build, if there's too much downloads only
> > the 17 first (on a 1920x1200 screen) are reachable.
> 
> Darren, I didn't mention scrollbars in my comment so I'm afraid I don't
> understand yours.
The scrollbar part of my comment was adressed to Paolo.

Comment 233

[Gabriela [:gaby2300]]

(In reply to comment #232)
> (In reply to comment #229)
> > (In reply to comment #228)
> > > Gabriela, please read comment 225.
> > > The download panel should have scrollbars when there's too much downloads in
> > > the list. With current tryserver build, if there's too much downloads only
> > > the 17 first (on a 1920x1200 screen) are reachable.
> > 
> > Darren, I didn't mention scrollbars in my comment so I'm afraid I don't
> > understand yours.
> The scrollbar part of my comment was adressed to Paolo.

How could I know that part was addressed to Paolo when it starts addressed to me??? I'm sorry but you should have addressed that part to Paolo then!

Comment 234

[Darren Kalck [:D-Kalck]]

(In reply to comment #233)
> How could I know that part was addressed to Paolo when it starts addressed
> to me??? I'm sorry but you should have addressed that part to Paolo then!
Because it was nothing related to what you said ?

Comment 235

[Johnathan Nightingale [:johnath]]

(In reply to comment #234)
> (In reply to comment #233)
> > How could I know that part was addressed to Paolo when it starts addressed
> > to me??? I'm sorry but you should have addressed that part to Paolo then!
> Because it was nothing related to what you said ?

aaaand that's just about enough "using an active bugzilla bug to discuss conversational structure"

Let's keep it on topic, folks.

Comment 236

[Marco Zehe (:MarcoZ)]

I tested the try build with screen readers and have the following observations:

1. The list and context menu still work as in the old download manager UI. Great!
2. Problem: When tabbing from the list onwards, I hit the "Open" button twice. Once it's a sub menu button, the second a regular button. The next thing in the tab order is the "Show download history" button. Why these two tabstops on the Open button?
3. Problem: CTRL+J (on Windows) is not yet hooked up to open the new downloads UI. It currently doesn't do anything.
4. Problem: The panel does not announce itself as being the downloads panel any more. The old window had a title screen readers were able to read. How is it conveyed visually that this is the Downloads window? If through some sort of image, we'll have to apply some WAI-ARIA magic to also give the panel a label. It should also get a proper role, since it is currently non-existent in the a1y hierarchy and needs to be a container. This is done by applying the role attribute to the panel itself. The available roles can be found in the wAI-ARIA documentation: http://www.w3.org/TR/wai-aria/roles#role_definitions
For this case, a role of dialog is probably most appropriate, or group, we'd need to test those.
5. Question: It is not clear to me what actually closes the panel besides Esc. I pressed F6, and it also went away. I would expect F6 to move me to the next panel (document) or the address bar, but not to dismiss the panel automatically.

Comment 237

[Emil]

The try build isn't working on Windows XP SP3 for me. Even if I click on the download icon after starting a download it shows nothing.

Comment 238

[Antoine Turmel [:GeekShadow]]

Attachment: what it look like on Linux/Ubuntu

Posted a screenshot on what it look like on Ubuntu/Linux, it sure need some visual tweaks.

Comment 239

[Nguyen Bat Hung]

It work ok but should make like this mockup: http://www.stephenhorlander.com/images/blog-posts/theme-update-2010-04-20/panel-download-win7.png the UI isn't good much, hope it'll land on Nightly

Comment 240

[Ed Morley [:emorley]]

(In reply to comment #238)
> it sure need some visual tweaks

(In reply to comment #239)
> the UI isn't good much

Please can people read the comments above *before* posting; namely comment 215 that states:

> Note that no serious user interaction design work has started yet. It will
> probably start after the initial assessment of code stability. 

Paolo has done a great job with the work in this bug so far; people picking holes in the parts that are known incomplete not only adds unnecessary noise, but seems somewhat rude.

Keep up the great work Paolo :-)

Comment 241

[Paul [pwd]]

Just to reiterate what Ed said, this bug is nowhere near completion. Thus please leave the attempts to polish it until after it has landed on the Nightly channel.

Comment 242

[a4010956]

yeah, can we just at last have it landed to the nightly? I can write a CSS style to make it look like on the mockups.

Comment 243

[Paul [pwd]]

(In reply to comment #242)
> yeah, can we just at last have it landed to the nightly? I can write a CSS
> style to make it look like on the mockups.

It'll land on ready when it's finished. Currently it's still receiving technical feedback (critiquing), then will be the review(s) and then it will land on the Nightly channel. Please be patient.

Comment 244

[Brandon Cheng]

(In reply to comment #242)
> yeah, can we just at last have it landed to the nightly? I can write a CSS
> style to make it look like on the mockups.

Go ahead. I've already seen your comments on the Tab+NavBar curves. You claimed that you were "too lazy" to implement your own proposal. Why would it be any different for this bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=570279#c77

Comment 245

[Shawn Wilsher :sdwilsh]

Cut the cross chatter, folks.  Before you click submit, think long and hard about your post and if it follows the bugzilla etiquette.  If it doesn't, don't post it, or I will see to it that your account is disabled.  The cross chatter doesn't help move this bug forward; in fact, it only makes development move slower.
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 246

[a4010956]

(In reply to comment #244)
> (In reply to comment #242)
> > yeah, can we just at last have it landed to the nightly? I can write a CSS
> > style to make it look like on the mockups.
> 
> Go ahead. I've already seen your comments on the Tab+NavBar curves. You
> claimed that you were "too lazy" to implement your own proposal. Why would
> it be any different for this bug?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=570279#c77

I've described the way it is possible to do tab curves using pure css, but where we need to create 1 element in the DOM-tree. And I don't know how to create it. Someone said it's possible using DOMi - but I've tried and it didn't work for me.
Besides, I'm not interested in having tab curves, I like the current tabs (they are a bit too square too high and too wide by default, but it gets fixed easily with 5 lines of CSS code).
And I'm really interested in having Doorhanger-like downloads window.
Oh, and I'm not sure they I would be able to make a glassy border for that window myself, as -moz-win-glass is not working (and even if it would - it's not a cross-platform solution) due to existing (or was it resolved WONTFIX) bug created by me.
But I could fix it using SVG filter "Gaussian Blur" but once again - due to an unfixed/ignored bug - SVGs are not working in Firefox if they are inside CSS style. And I only know how to use SVGs using CSS, otherwise I would need to create some patch and Hg is a piece of sh1t for that purpose.

Comment 247

[Emil]

Hadn't worked to me because the German language pack was installed.
Tested it now and it is great! I really like the new Download manager.

Comment 248

[Shawn Wilsher :sdwilsh]

Comment on attachment 529878 [details] [diff] [review]
Base version

I'm actually going to let this go through one more feedback cycle from mak before I really dig into this.  He says he should get to this soon :D

Comment 249

[Marco Bonardo [:mak]]

Comment on attachment 529878 [details] [diff] [review]
Base version

Review of attachment 529878 [details] [diff] [review]:
-----------------------------------------------------------------

The button positioning needs serious planning.
I see there is discussion on whether it should be a button or an identity notification, but this has to be solved soon (Limi should get an updated trybuild asap).

Btw, this feedback includes everything but css files (styling is still to be defined) and downloads.js.
I plan to do downloads.js tomorrow morning, since it's the largest file and requires more attention.

::: browser/components/downloads/DownloadCommon.jsm
@@ +16,5 @@
> + * The Original Code is Download Manager Code.
> + *
> + * The Initial Developer of the Original Code is
> + * Paolo Amadini <http://www.amadzone.org/>.
> + * Portions created by the Initial Developer are Copyright (C) 2010-2011

just take one, we don't usually specify 2 years. This is usually the year you created the file first.

This seems to apply to all headers you created

@@ +71,5 @@
> +const DownloadCommon = {
> +  /**
> +   * Returns an object whose keys are the string names from the downloads string
> +   * bundle, and whose values are either the translated strings or functions
> +   * that can be used to obtain formatted strings.

nit: s/functions that can be used to obtain/getters for/

@@ +103,5 @@
> +                                   "nsIDownloadManager");
> +
> +XPCOMUtils.defineLazyServiceGetter(DownloadCommon, "ms",
> +                                   "@mozilla.org/mime;1",
> +                                   "nsIMIMEService");

I see that is short, but to me ms sounds not very describing (indeed I was guessing why you had a microsummaries service getter). Why not just "mime"?

::: browser/components/downloads/DownloadManagerUI.js
@@ +70,5 @@
> +
> +  show: function DMUI_show(aWindowContext, aID, aReason)
> +  {
> +    let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> +    if (browserWin && !browserWin.closed) {

For clarity, annotate bug 528706 in a comment above this.

@@ +79,5 @@
> +
> +  get visible()
> +  {
> +    let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> +    return browserWin ? browserWin.DownloadsPanel.isPanelShowing : false;

this may have same .closed bug, even if less important since the object should still be there. Btw maybe you could add a simple _getBrowserWindow helper and use it in both places.

::: browser/components/downloads/downloads.css
@@ +1,5 @@
> +/*** Download items ***/
> +
> +richlistitem[type="download"] {
> +  -moz-binding: url('chrome://browser/content/downloads/download.xml#download');
> +  -moz-box-orient: vertical;

out of curiosity, why is it needed to set content to horizontal in the binding and then orient to vertical in the style?

@@ +69,5 @@
> +                                           .downloadShow,
> +
> +.download-state[state="7"]                 .downloadCommandsSeparator
> +
> +{

nit: fwiw, this file, regardless all the indentation tricks, doesn't seem particularly readable to me. But this is a coding style thing I leave to the module owner :)

::: browser/components/downloads/downloadsOverlay.xul
@@ +102,5 @@
> +           align="center"
> +           onpopupshown="document.getElementById('downloadsListBox').focus();"
> +           onpopuphidden="DownloadsPanel.onPopupHidden();">
> +      <menupopup id="downloadsContextMenu"
> +                 class="download-state">

Did you have a chance to check if popups are still conflicting?
If so, you should ensure a bug on that exists and annotate it here with a comment on why this context menu is not defined in the popupset.
Otherwise, if that has been fixed, this should be moved.

@@ +155,5 @@
> +                  accesskey="&selectAllCmd.accesskey;"/>
> +      </menupopup>
> +
> +      <label id="downloadsNoDownloads"
> +             value="&nodownloads.label;"/>

Why is this a <label> rather than a <description>? doesn't seem to be related to any control

::: browser/components/preferences/tests/Makefile.in
@@ +45,5 @@
>  
>  _BROWSER_FILES = \
> +    browser_basic_functionality.js \
> +    browser_delete_key_removes.js \
> +    head.js \

just indent 2 spaces here

PS: Splinter seems to think this is browser/components/preferences/tests/Makefile.in, I was puzzled by that, but I see you hg copied that file, if possibile I'd suggest to rewrite this file since the copy blame is unrelated.

::: browser/components/downloads/test/browser_basic_functionality.js
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Sdwilsh: does downloads component style guide require the mode and vim comments? (btw, tab-width seems wrong)

@@ +29,5 @@
> +  ];
> +
> +  for (yy in gen_resetState()) yield;
> +  for (yy in gen_addDownloadRows(DownloadData)) yield;
> +  for (yy in gen_openPanel()) yield;

some comment on each one of these would be welcome, it's a bit cryptic otherwise

@@ +37,5 @@
> +  is(richlistbox.children.length, DownloadData.length,
> +     "There is the correct number of richlistitems");
> +  for (var i = 0; i < richlistbox.children.length; i++) {
> +    var element = richlistbox.children[i];
> +    var dataItem = new DownloadsViewItemController(element).dataItem;

nit: these seem good candidates for "let"

@@ +47,5 @@
> +
> +  // Clean up when the test finishes.
> +  for (yy in gen_resetState()) yield;
> +
> +  gTestTargetFile.remove(false);

you should do this in registerCleanupFunction, remember that browser-chrome tests don't run in separate sandboxes, so you have to ensure any change you make is correctly cleaned up both if the test succeeds and if it fails.

Unless this cleanup code will be specific to only some of the tests you could even move that registerCleanupFunction to head.js (it works, really)

::: browser/components/downloads/test/browser_delete_key_removes.js
@@ +19,5 @@
> +  ];
> +
> +  for (yy in gen_resetState()) yield;
> +  for (yy in gen_addDownloadRows(DownloadData)) yield;
> +  for (yy in gen_openPanel()) yield;

ditto for comments

@@ +34,5 @@
> +           richlistbox.children.length,
> +           "The database and the number of downloads display matches");
> +      },
> +      handleError: function(aError) { Cu.reportError(aError); },
> +      handleCompletion: function(aReason) { testRunner.continueTest(); },

for code style, don't oneline functions please.

@@ +42,5 @@
> +    // Check that the delete key removes each download.
> +    let len = DownloadData.length;
> +    for (let i = 0; i < len; i++) {
> +      // This removes the download synchronously.
> +      EventUtils.synthesizeKey("VK_DELETE", {});

I think it's good practice to focus things before synthesizing keys, the focus could go elsewhere for whatever reason on tinderboxes.

@@ +52,5 @@
> +             len - (i + 1),
> +             "The download was properly removed");
> +        },
> +        handleError: function(aError) { Cu.reportError(aError); },
> +        handleCompletion: function(aReason) { testRunner.continueTest(); },

ditto on oneline functions

@@ +63,5 @@
> +
> +  // Clean up when the test finishes.
> +  for (yy in gen_resetState()) yield;
> +
> +  gTestTargetFile.remove(false);

ditto for cleanup

::: browser/components/downloads/test/head.js
@@ +75,5 @@
> +                  "DELETE FROM moz_downloads");
> +  statement.executeAsync({
> +    handleResult: function(aResultSet) { },
> +    handleError: function(aError) { Cu.reportError(aError); },
> +    handleCompletion: function(aReason) { testRunner.continueTest(); },

ditto for oneline functions

@@ +86,5 @@
> +
> +function gen_addDownloadRows(aDataRows)
> +{
> +  let columnNames = [n for ([n] in Iterator(gDownloadRowTemplate))]
> +                    .join(", ");

do you really need an Iterator here? can't you do Object.keys(gDownloadRowTemplate).join(", "); ?

@@ +88,5 @@
> +{
> +  let columnNames = [n for ([n] in Iterator(gDownloadRowTemplate))]
> +                    .join(", ");
> +  let parameterNames = [":" + n for ([n] in Iterator(gDownloadRowTemplate))]
> +                       .join(", ");

Object.keys(gDownloadRowTemplate).map(function (v) ":" + v).join(", ");

@@ +96,5 @@
> +  try {
> +    // Execute the statement for each of the provided downloads.
> +    for ([, dataRow] in Iterator(aDataRows)) {
> +      // Populate insert parameters from the provided data.
> +      for (let [columnName] in Iterator(gDownloadRowTemplate)) {

just store columnNames as an array and join() it where needed?

@@ +109,5 @@
> +      // Run the statement asynchronously and wait.
> +      statement.executeAsync({
> +        handleResult: function(aResultSet) { },
> +        handleError: function(aError) { Cu.reportError(aError); },
> +        handleCompletion: function(aReason) { testRunner.continueTest(); },

ditto for oneline functions

::: browser/components/nsBrowserGlue.js
@@ +1165,5 @@
> +          currentset.indexOf("downloads-button") == -1) {
> +        // The element is added before the location bar, or as the first button.
> +        if (currentset.indexOf("urlbar-container") != -1) {
> +          currentset = currentset.replace(/(^|,)urlbar-container($|,)/,
> +                                          "$1downloads-button,urlbar-container$2")

clearly this is in couple with my other comments regarding getting early ux feedback on the button position (or existence)

@@ +1168,5 @@
> +          currentset = currentset.replace(/(^|,)urlbar-container($|,)/,
> +                                          "$1downloads-button,urlbar-container$2")
> +        }
> +        else {
> +          currentset = "downloads-button," + currentset;

won't this put the button even before back/forward buttons?

Comment 250

[Marco Bonardo [:mak]]

Comment on attachment 529878 [details] [diff] [review]
Base version

Review of attachment 529878 [details] [diff] [review]:
-----------------------------------------------------------------

So, following is a huge list of suggestions, but globally this is much much better than previous version, and pretty much clear.
Amazing work Paolo!
I didn't test the functionality (I'm sure we'll soon get a new trybuild for that), but looking at the code my feedback may only be positive.

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

I don't understand this, why having getters for constant values?

I'd rather add a
const STATE = {
  UNINITIALIZED: 0,
  HIDDEN: 1,
  SHOWING: 2,
  SHOWN: 3
}
and then use this._panelState != STATE.HIDDEN and such

@@ +120,5 @@
> +
> +  /**
> +   * Starts loading the download data in the background, without opening the
> +   * panel. You can call the showPanel method instead if you want to load the
> +   * data and open the panel at the same time.

s/in the background/in background/
"Use showPanel() instead to load the data and open the panel at the same time."

nit: per comments readability, double space after periods.

@@ +132,5 @@
> +
> +    window.addEventListener("unload", this.onWindowUnload, false);
> +
> +    DownloadsView.data = new DownloadsData(DownloadsView);
> +    DownloadsView.data.initialize();

is there a case where you have to create a DownloadsData without invoking .initialize()? if not you could just merge initialize in the constructor (luckily this is not cpp+xpcom)

@@ +133,5 @@
> +    window.addEventListener("unload", this.onWindowUnload, false);
> +
> +    DownloadsView.data = new DownloadsData(DownloadsView);
> +    DownloadsView.data.initialize();
> +    DownloadsViewController.initialize();

I think I'd rather provide a initialize() and terminate() in DownloadsView, and inside those init/terminate data and controller.
So the flow here on panel opening is just "we init a view for this panel" and on close "we terminate a view for this panel"

@@ +154,5 @@
> +    this.hidePanel();
> +
> +    DownloadsViewController.terminate();
> +    DownloadsView.data.terminate();
> +    DownloadsView.data = null;

ditto

@@ +167,5 @@
> +   * Approximate minimum number of items to get on initialization before showing
> +   * the panel. This avoids presenting to the user a panel that keeps vertically
> +   * growing for each loaded item.
> +   */
> +  get kMinItemCountForPopup() 20,

why a getter?

@@ +183,5 @@
> +    }
> +
> +    this.initialize();
> +    this._panelState = this.kPanelShowing;
> +    this.onViewItemCountChange();

comment on what this call is for (since the method says it has to be called when the list count changes)

@@ +194,5 @@
> +  hidePanel: function DO_hidePanel()
> +  {
> +    if (this._panelState == this.kPanelUninitialized) {
> +      return;
> +    }

return also if it's hidden? or better you could if (!isPanelShowing)

@@ +200,5 @@
> +    this.panel.hidePopup();
> +
> +    // Ensure that we allow the panel to be reopened, even if the popup was
> +    // already hidden and the onPopupHidden event was not invoked.
> +    this._panelState = this.kPanelHidden;

is not this already done by onPopupHidden?

@@ +209,5 @@
> +   */
> +  get isPanelShowing() {
> +    return this._panelState == this.kPanelShowing ||
> +           this._panelState == this.kPanelShown;
> +  },

I'd suggest to use the short form (no braces and no return) but I think Sdwilsh would be unhappy about that :p

@@ +257,5 @@
> +
> +  /**
> +   * Shows or focuses the user interface dedicated to downloads history.
> +   */
> +  showHistory: function DO_showHistory()

I'd rename this to showDownloadsHistory

@@ +274,5 @@
> +  /**
> +   * Internal method that actually opens the Download Manager panel interface
> +   * when enough data is ready to be displayed.
> +   */
> +  _openPopupIfReady: function DO_openPopupIfReady()

Read is a bit generic, I'd rename to openPopupIfDataReady

@@ +280,5 @@
> +    // We don't want to open the popup if we already displayed it, or if we are
> +    // still loading and there are not enough elements yet.
> +    if (this._panelState != this.kPanelShowing ||
> +        (DownloadsView.itemCount < this.kMinItemCountForPopup &&
> +         !DownloadsView.loadCompleted)) {

this should be more readable:

let isDataReady = DownloadsView.itemCount >= this.kMinItemCountForPopup ||
                  DownloadsView.loadCompleted;
if (this._panelState != this.kPanelShowing || !isDataReady)

@@ +292,5 @@
> +                                                      .ROLLUP_CONSUME);
> +
> +    // If the downloads button has been removed from the toolbar, anchor the
> +    // panel to the toolbar itself. The popup will open near the place where
> +    // the downloads button is present by default.

this comment could need update when there is a decision on button position

@@ +300,5 @@
> +  }
> +};
> +
> +XPCOMUtils.defineLazyGetter(DownloadsPanel, "panel", function()
> +                            document.getElementById("downloadsPanel"));

nit: move function () on the same line as getElementById if it doesn't go over 80 chars

@@ +308,5 @@
> +
> +/**
> + * Builds and updates the downloads list widget, responding to changes in the
> + * download state and real-time data. In addition, handles part of the user
> + * interaction events raised by the downloads list widget.

nit: "real-time data" is a bit generic.

@@ +331,5 @@
> +
> +  /**
> +   * First element in the download list widget whose state is in progress, or
> +   * null if no such element exists. If it exists, this element can be the same
> +   * as firstElementNotDone, or it can be one of the elements following it.

just "Can be the same as firstElementNotDone, or one of the next siblings."

@@ +337,5 @@
> +  firstElementInProgress: null,
> +
> +  /**
> +   * Places the XUL element associated with a download item in the correct
> +   * position in the downloads list widget, first in its category. If the

can probably be explained better, especially the ", first in its category"

@@ +346,5 @@
> +   *
> +   * @param aDataItem
> +   *        DownloadsDataItem that is being added or moved.
> +   */
> +  _addOrMoveElementToTop: function DV_addOrMoveElementToTop(aElement, aDataItem)

the method name is lying, this does not always puts to top (of the list) rather it top of the category. What about _insertAsFirstCategorizedElement or something like that?

@@ +350,5 @@
> +  _addOrMoveElementToTop: function DV_addOrMoveElementToTop(aElement, aDataItem)
> +  {
> +    // Items that are done are inserted before all other items.
> +    if (aDataItem.done) {
> +      this.richListBox.insertBefore(aElement, this.richListBox.firstChild);

should you unset this.firstElementInProgress and this.firstElementNotDone if they are same as aElement?

@@ +357,5 @@
> +
> +    // Items not in progress are inserted before items that are in progress.
> +    if (!aDataItem.inProgress) {
> +      this.richListBox.insertBefore(aElement, this.firstElementNotDone);
> +      this.firstElementNotDone = aElement;

should you unset this.firstElementInProgress if it's aElement?

@@ +383,5 @@
> +   * @param aDataItem
> +   *        DownloadsDataItem that is being added or moved.
> +   */
> +  _addOrMoveElementToBottom: function DV_addOrMoveElementToBottom(aElement,
> +                                                                  aDataItem)

same comments as above

@@ +430,5 @@
> +    // otherwise it means that no such element exists anymore.
> +    if (aElement != this.firstElementNotDone ||
> +        aElement != this.firstElementInProgress) {
> +      return;
> +    }

shouldn't this be an AND? if aElement is going away you should ensure it's not used in both.

@@ +453,5 @@
> +    }
> +    if (aElement == this.firstElementInProgress) {
> +      this.firstElementInProgress = nextDataItem.inProgress ? nextElement
> +                                                            : null;
> +    }

Unless DownloadsViewItemController() is much expensive I think inverting the ifs could be more readable:

if (aElement == this.firstElementNotDone) {
  if (!nextElement) {
    this.firstElementNotDone = null;
  }
  else {
    let nextDataItem = new DownloadsViewItemController(nextElement).dataItem;
    this.firstElementNotDone = !nextDataItem.done ? nextElement : null;
  }
}

if (aElement == this.firstElementInProgress) {
  ...
}

@@ +461,5 @@
> +   * Called when the number of download items in the list changes.
> +   */
> +  _itemCountChanged: function DV_itemCountChanged()
> +  {
> +    // Note that the "itemCount" property may not be available now.

clarify comment please (Add when and why, "now" is not enough)

@@ +462,5 @@
> +   */
> +  _itemCountChanged: function DV_itemCountChanged()
> +  {
> +    // Note that the "itemCount" property may not be available now.
> +    let newItemCount = this.richListBox.childNodes.length;

this.richListBox.childElementCount ?

@@ +486,5 @@
> +  {
> +    let previousSelectedIndex = this.richListBox.selectedIndex;
> +    this._beforeRemoveElement(aElement);
> +    this._addOrMoveElementToTop(aElement, aDataItem);
> +    this.richListBox.selectedIndex = previousSelectedIndex;

What if I selected this specific element, should not the selection follow it?

@@ +532,5 @@
> +  },
> +
> +  /**
> +   * Called when an event occurred for which the downloads database is not
> +   * available anymore and existing data should be discarded.

ehr, rephrase, something like:
"Called when the downloads database becomes unavailable.  Existing data should be discarded."
Bonus points if you also explain situations when it happens.

@@ +550,5 @@
> +   * @param aNewest
> +   *        When true, indicates that this item is the most recent and should be
> +   *        added in the topmost position of its category. This happens when a
> +   *        new download is started. When false, indicates that the item is the
> +   *        least recent with regard to the items that have been already added.

"least recent" or just "not the most recent"?

@@ +551,5 @@
> +   *        When true, indicates that this item is the most recent and should be
> +   *        added in the topmost position of its category. This happens when a
> +   *        new download is started. When false, indicates that the item is the
> +   *        least recent with regard to the items that have been already added.
> +   *        This generally happens during the initial data load, when we want

What is "this", that is set to true?

@@ +565,5 @@
> +      this._addOrMoveElementToTop(element, aDataItem);
> +    } else {
> +      this._addOrMoveElementToBottom(element, aDataItem);
> +    }
> +    this._itemCountChanged();

I'm under the impression that when you add an item selection changes, I suppose it should stay on the same item it was before

@@ +625,5 @@
> +      return;
> +    }
> +
> +    // Find the richlistitem for the download and execute the command.
> +    for (; target.nodeName != "richlistitem"; target = target.parentNode);

better avoiding empty-body for loops, they are confusing and error-prone. use a while instead:
while (target.nodeName != "richlistitem") {
  target = target.parentNode;
}

@@ +635,5 @@
> +    // Double clicks on buttons should not invoke the default action.
> +    if (aEvent.originalTarget.hasAttribute("command") ||
> +        aEvent.originalTarget.hasAttribute("oncommand")) {
> +      return;
> +    }

aEvent.preventDefault() ? or you're just trying to skip next code?

@@ +673,5 @@
> +
> +    var dt = aEvent.dataTransfer;
> +    dt.mozSetDataAt("application/x-moz-file", f, 0);
> +    dt.effectAllowed = "copyMove";
> +    dt.addElement(element);

you could want to stopPropagation since you handled the drag

@@ +683,5 @@
> +    if (types.contains("text/uri-list") ||
> +        types.contains("text/x-moz-url") ||
> +        types.contains("text/plain")) {
> +      aEvent.preventDefault();
> +    }

I think you may use
if (Services.droppedLinkHandler.canDropLink(aEvent, true)) {
  aEvent.preventDefault();
}

@@ +685,5 @@
> +        types.contains("text/plain")) {
> +      aEvent.preventDefault();
> +    }
> +    aEvent.stopPropagation();
> +  },

do you allow both copy and move everywhere? (you're not handling dropEffect in any special way so I suppose yes)

@@ +695,5 @@
> +    var name;
> +    if (!url) {
> +      url = dt.getData("text/x-moz-url") || dt.getData("text/plain");
> +      [url, name] = url.split("\n");
> +    }

you may use
var name = {};
var url = Services.droppedLinkHandler.dropLink(aEvent, name);
name = name.value;

@@ +699,5 @@
> +    }
> +    if (url) {
> +      saveURL(url, name ? name : url, null, true, true);
> +    }
> +  }

you're not checking which operation the user required (dropEffect == "copy" or "move"), but looks like you do the same for both.
you may also call aEvent.preventDefault to indicate you handled the drop.

@@ +710,5 @@
> +//// DownloadsViewItem
> +
> +/**
> + * Builds and updates a single item in the downloads list widget, responding to
> + * changes in the download state and real-time data.

define real-time data better.

@@ +724,5 @@
> +  this._element = aElement;
> +  this.dataItem = aDataItem;
> +
> +  this.previousDone = this.dataItem.done;
> +  this.previousInProgress = this.dataItem.inProgress;

previous sounds strange, maybe wasDone, wasInProgress?

@@ +725,5 @@
> +  this.dataItem = aDataItem;
> +
> +  this.previousDone = this.dataItem.done;
> +  this.previousInProgress = this.dataItem.inProgress;
> +  this.lastSeconds = Infinity;

I don't have any idea what lastSeconds represents, better name?

@@ +737,5 @@
> +    "state": this.dataItem.state,
> +    "progress": this.dataItem.inProgress ? this.dataItem.percentComplete : 100,
> +    "target": this.dataItem.target,
> +    "image": this.image
> +  };

const

@@ +777,5 @@
> +      try {
> +        // Refresh the icon, so that executable icons are shown. Tacking on
> +        // contentType bypasses cache.
> +        this.image += "&contentType=" + DownloadCommon.ms.getTypeFromFile(
> +                                                       this.dataItem.localFile);

rephrase comment and fix indentation a bit:
// Refresh the icon to ensure executable icons are shown.
// Add contentType to bypass the cache (is there a bug about this to annotate here?).
let mimeType = DownloadCommon.mime.getTypeFromFile(this.dataItem.localFile);
this.image += "&contentType=" + mimeType;

@@ +789,5 @@
> +    if (this.previousInProgress && !this.dataItem.inProgress) {
> +      this.endTime = Date.now();
> +    }
> +
> +    // Move the download to the appropriate place in the list if state changed.

"...to the appropriate list position if..."

@@ +851,5 @@
> +      let transfer = DownloadUtils.getTransferTotal(this.dataItem.currBytes,
> +                                                    this.dataItem.maxBytes);
> +      status = this._replaceInsert(DownloadCommon.strings.paused, 1, transfer);
> +    } else if (this.dataItem.state == nsIDM.DOWNLOAD_DOWNLOADING) {
> +      let newLast;

better var name

@@ +871,5 @@
> +          case nsIDM.DOWNLOAD_FAILED:           return s.stateFailed;
> +          case nsIDM.DOWNLOAD_CANCELED:         return s.stateCanceled;
> +          case nsIDM.DOWNLOAD_BLOCKED_PARENTAL: return s.stateBlockedParentalControls;
> +          case nsIDM.DOWNLOAD_BLOCKED_POLICY:   return s.stateBlockedPolicy;
> +          case nsIDM.DOWNLOAD_DIRTY:            return s.stateDirty;

I'm not sure I like this indentation, but I leave this to the module owner

@@ +882,5 @@
> +            let [size, unit] = DownloadUtils.convertByteUnits(fileSize);
> +            let sizeText = this._replaceInsert(s.doneSize, 1, size);
> +            return this._replaceInsert(sizeText, 2, unit);
> +        }
> +      }.apply(this);

just add another underscore helper and pass it this.dataItem, putting all of this here inflates the code without big value.

@@ +904,5 @@
> +   * Updates the time that gets shown for completed download items.
> +   */
> +  _updateTime: function DVI_updateTime()
> +  {
> +    // Don't bother updating for things that aren't finished.

"things"?

@@ +909,5 @@
> +    if (this.dataItem.inProgress) {
> +      return;
> +    }
> +
> +    let end = new Date(parseInt(this.dataItem.endTime));

why do you need to parseInt this here, shouldn't rather that be done on setting the property?

@@ +981,5 @@
> +      return false;
> +    }
> +    // Secondly, determine if focus is on a control in the downloads list.
> +    let e = document.commandDispatcher.focusedElement;
> +    for (; e && e != DownloadsView.richListBox; e = e.parentNode);

please avoid empty-body for loops, use a while instead.

@@ +1057,5 @@
> +  {
> +    // Update all the commands that are specific for the selected item.
> +    for ([cmd] in Iterator(DownloadsViewItemController.prototype.commands)) {
> +      goUpdateCommand(cmd);
> +    }

why not just for (cmd in DownloadsViewItemController.prototype.commands) ?

@@ +1088,5 @@
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// Constants
> +
> +  get kPrefBdmAlertOnExeOpen() "browser.download.manager.alertOnEXEOpen",
> +  get kPrefBdmScanWhenDone() "browser.download.manager.scanWhenDone",

why getters?

@@ +1132,5 @@
> +  doCommand: function DVIC_doCommand(aCommand)
> +  {
> +    // Invoke the command, if enabled, using this object for "this".
> +    if (this.isCommandEnabled(aCommand)) {
> +      this.commands[aCommand].apply(this);

I think you could actually use .bind(this) when defining the functions and avoid these apply.

Same for a bunch of apply() I see below

@@ +1313,5 @@
> +          case nsIDM.DOWNLOAD_SCANNING:         return "downloadsCmd_show";
> +          case nsIDM.DOWNLOAD_DIRTY:            return "downloadsCmd_openReferrer";
> +          case nsIDM.DOWNLOAD_BLOCKED_POLICY:   return "downloadsCmd_openReferrer";
> +        }
> +      }.apply(this);

as well as here you could just add a _helper

@@ +1321,5 @@
> +  },
> +
> +  commandsMultiple: {
> +    downloadsCmd_copyLocation: function
> +                               DVIC_CM_downloadsCmd_copyLocation(aControllers)

if it's too long just newline after ":" like
label:
function xyz()

@@ +1460,5 @@
> +  },
> +
> +  /**
> +   * Returns the data item associated with the provided Download Manager object
> +   * or download data row, following a real-time notification or initial load.

can probably be rephrased, it caused me an headache :)

@@ +1478,5 @@
> +   *
> +   * @return New or existing data item, or null if the item was deleted from the
> +   *         list of available downloads.
> +   */
> +  _getOrAddDataItem: function DD_getOrAddDataItem(aSource)

I'd just rename it to fetchDataItem, where it fetches it doesn't matter.

@@ +1601,5 @@
> +
> +    dataItem.state = aDownload.state;
> +    dataItem.referrer = aDownload.referrer && aDownload.referrer.spec;
> +    dataItem.resumable = aDownload.resumable;
> +    dataItem.startTime = Math.round(aDownload.startTime / 1000);

I think you may want to parseInt here, any reason to keep a double?

@@ +1632,5 @@
> +
> +  onStateChange: function(aWebProgress, aRequest, aState, aStatus,
> +                          aDownload) { },
> +
> +  onSecurityChange: function(aWebProgress, aRequest, aState, aDownload) { }

use empty args here in unused methods: "function () {},"

@@ +1677,5 @@
> +    this.target = aDownload.displayName;
> +    this.uri = aDownload.source.spec;
> +    this.referrer = aDownload.referrer && aDownload.referrer.spec;
> +    this.state = aDownload.state;
> +    this.startTime = Math.round(aDownload.startTime / 1000);

parseInt here?

@@ +1708,5 @@
> +    this.uri = aStorageRow.getResultByName("source");
> +    this.referrer = aStorageRow.getResultByName("referrer");
> +    this.state = aStorageRow.getResultByName("state");
> +    this.startTime = Math.round(aStorageRow.getResultByName("startTime") / 1000);
> +    this.endTime = Math.round(aStorageRow.getResultByName("endTime") / 1000);

parseInt here?

@@ +1713,5 @@
> +    this.currBytes = aStorageRow.getResultByName("currBytes");
> +    this.maxBytes = aStorageRow.getResultByName("maxBytes");
> +
> +    // Compute the other properties without accessing the download object.
> +    this.resumable = true;

is this always true?

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

nit: != -1 would be more correct

@@ +1742,5 @@
> +  },
> +
> +  /**
> +   * Indicates that a download that is proceeding normally is still in its
> +   * initial state, and the actual download of data bytes hasn't started yet.

rephrase

@@ +1748,5 @@
> +  get starting()
> +  {
> +    return this.state == nsIDM.DOWNLOAD_NOTSTARTED ||
> +           this.state == nsIDM.DOWNLOAD_QUEUED;
> +  },

I'd drop braces and return (if sdwilsh allows)

@@ +1766,5 @@
> +      nsIDM.DOWNLOAD_FINISHED,
> +      nsIDM.DOWNLOAD_BLOCKED_PARENTAL,
> +      nsIDM.DOWNLOAD_BLOCKED_POLICY,
> +      nsIDM.DOWNLOAD_DIRTY,
> +    ].indexOf(this.state) >= 0;

!= -1

@@ +1782,5 @@
> +  get canRetry()
> +  {
> +    return this.state == nsIDM.DOWNLOAD_CANCELED ||
> +           this.state == nsIDM.DOWNLOAD_FAILED;
> +  },

I'd drop braces and return (if sdwilsh allows)

@@ +1801,5 @@
> +    // We should be using real URLs all the time, but until bug 239948 is fully
> +    // fixed, this will do...
> +    if (this.file.substring(0, "file://".length) == "file://") {
> +      // If this is a URL, get the file from that.
> +      // XXX It's possible that using a null charset here is bad.

ni-nit:I like these old comments from old code, "//XXX it's possible today it will rain" :)

::: browser/components/downloads/downloadsOverlay.xul
@@ +100,5 @@
> +           type="arrow"
> +           orient="vertical"
> +           align="center"
> +           onpopupshown="document.getElementById('downloadsListBox').focus();"
> +           onpopuphidden="DownloadsPanel.onPopupHidden();">

I suspect in both you could want to check event.target == event.currentTarget (to avoid handling events for nested popups

Comment 251

[Marco Bonardo [:mak]]

Oh I see you used getters to simulate const, personally I don't think it's worth it though, unless you have some clear bad-scenario in mind.

Comment 252

[:Paolo Amadini]

Thanks _very much_ for the detailed review, Marco! I've read it all, now
I've started going through all the open issues in past comments, from the
oldest to the newest in order, to update the patch. Today I didn't get past
comment 216 through! I'll reply to yours and others' comments later.

Comment 253

[:Paolo Amadini]

(In reply to comment #216)
> I only glanced at the locales/en-US files, so no review for non-localized UI
> strings or RTL issues or somesuch. But I got a few nits either way.

Axel, thanks for the suggestions! I've implemented them, including the changes
outlined in comment 226. For me, there's only one open point:

> Are Parental Controls and Security Zone Policy elsewhere in our UI?

No.

> Also, I'm not sure if capitalized is right, it should probably be
> consistently named and spelled with the display of those features on Windows
> in that localization?

I think so, and actually that is already the case for most of the current
translations in the Download Manager:

http://mxr.mozilla.org/l10n-central/search?string=stateBlocked

The question is, do we want new translations of the same strings to be more
consistent with the operating system, or with our previous translations in
Firefox and other Toolkit-based browsers?

Assuming the former, I've provided some links to Windows reference material
in the translation notes. Being consistent with ourselves can however create
less confusion when searching the Internet for existing solutions (this is
relevant only to locales which translated the two strings inconsistently,
in English there is no problem as we already use the correct, official words).
For example:

http://www.google.it/search?q=Security+Zone+Policy

http://kb.mozillazine.org/Unable_to_save_or_download_files#Enable_downloads_blocked_by_Security_Zone_Policy_-_Windows

Here is the latest version of the locale file, if you want to take a look:

# LOCALIZATION NOTE (stateStarting):
# Indicates that the download is starting.
stateStarting=Starting…
# LOCALIZATION NOTE (stateScanning):
# Indicates that an external program is scanning the download for viruses.
stateScanning=Scanning for viruses…
# LOCALIZATION NOTE (stateFailed):
# Indicates that the download failed because of an error.
stateFailed=Failed
# LOCALIZATION NOTE (statePaused):
# Indicates that the download was paused by the user.
statePaused=Paused
# LOCALIZATION NOTE (stateCanceled):
# Indicates that the download was canceled by the user.
stateCanceled=Canceled
# LOCALIZATION NOTE (stateBlockedParentalControls):
# Indicates that the download was blocked by the Parental Controls feature of
# Windows. "Parental Controls" should be consistently named and capitalized with
# the display of this feature in Windows. The following article can provide a
# reference for the translation of "Parental Controls" in various languages:
# http://windows.microsoft.com/en-US/windows-vista/Set-up-Parental-Controls
stateBlockedParentalControls=Blocked by Parental Controls
# LOCALIZATION NOTE (stateBlockedPolicy):
# Indicates that the download was blocked on Windows because of the "Launching
# applications and unsafe files" setting of the "security zone" associated with
# the target site. "Security zone" should be consistently named and capitalized
# with the display of this feature in Windows. The following article can provide
# a reference for the translation of "security zone" in various languages:
# http://support.microsoft.com/kb/174360
stateBlockedPolicy=Blocked by your security zone policy
# LOCALIZATION NOTE (stateDirty):
# Indicates that the download was blocked after scanning.
stateDirty=Blocked: May contain a virus or spyware

# LOCALIZATION NOTE (doneSize):
# #1 size number; #2 size unit.
doneSize=#1 #2
doneSizeUnknown=Unknown size

fileExecutableSecurityWarning="%S" is an executable file. Executable files may contain viruses or other malicious code that could harm your computer. Use caution when opening this file. Are you sure you want to launch "%S"?
fileExecutableSecurityWarningTitle=Open Executable File?
fileExecutableSecurityWarningDontAsk=Don't ask me this again

Comment 254

[:Paolo Amadini]

I found out that the reason why the downloads panel didn't open automatically
and the keyboard shortcut didn't work in the tryserver build is that the XPCOM
component controlling this behavior wasn't included in the installers. This is
because new XPCOM components should be explicitly added to the file named
"package-manifest.in". The next patch revision will include the fix, and I
expect the panel to work correctly in the next tryserver build.

I wonder if package manifests, or portions of them, could be generated
automatically from makefiles. Meanwhile, I've updated the documentation here:

https://developer.mozilla.org/en/Adding_XPCOM_components_to_Mozilla_build_system

Comment 255

[Marco Bonardo [:mak]]

oops, this is totally my fault, I had in my todo list a check for package-manifest in this patch but I forgot to do it :(

Comment 256

[:Paolo Amadini]

(In reply to comment #231)
> "Download" pane in Places library does not provide the download status.

I don't know if we want the list of historical downloads to provide more
information and actions than now, for example seeing whether a past download
completed, and if yes allowing the user to open the target file or folder.

If so, either the patch on bug 564900 should be updated, or another patch on
top of that one should be developed.

Comment 257

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

For old files I think we should create sth like Chrome, simply a new tab when you click e.g.  "Show downloaded files" in new download manager.
I remember it was proposed somewhere like in bug #582594 or in meta bug #584942

Comment 258

[:Paolo Amadini]

(In reply to comment #236)
> 2. Problem: When tabbing from the list onwards, I hit the "Open" button
> twice. Once it's a sub menu button, the second a regular button. The next
> thing in the tab order is the "Show download history" button. Why these two
> tabstops on the Open button?

I verified that this is the same behavior as the menu-button in the popup
notifications (for example, the one that is opened when you click an add-on
download link in any website). Pressing ALT+Down (on Windows) opened the
drop-down menu only when focus was on the part of the button with the down
arrow, not on the main button. Maybe this is the expected behavior of XUL
menu-buttons? If not, I think a different bug should be filed.

> 4. Problem: The panel does not announce itself as being the downloads panel
> any more. The old window had a title screen readers were able to read. How
> is it conveyed visually that this is the Downloads window? If through some
> sort of image, we'll have to apply some WAI-ARIA magic to also give the
> panel a label.

I'd like to be initiated to this kind of magic :-) Which is the method
you have in mind for implementing this panel labeling? Which is the method
that is currently used for labeling the "popup notifications" panel, if any?

> It should also get a proper role, since it is currently
> non-existent in the a1y hierarchy and needs to be a container. This is done
> by applying the role attribute to the panel itself. The available roles can
> be found in the wAI-ARIA documentation:
> http://www.w3.org/TR/wai-aria/roles#role_definitions
> For this case, a role of dialog is probably most appropriate, or group, we'd
> need to test those.

Which tools can I use to do a basic verification that the panel is included
in the accessibility hierarchy, if I assign a role attribute to it?

> 5. Question: It is not clear to me what actually closes the panel besides
> Esc. I pressed F6, and it also went away. I would expect F6 to move me to
> the next panel (document) or the address bar, but not to dismiss the panel
> automatically.

For me, on Windows, pressing F6 doesn't dismiss the panel. This, however, is
confusing, as pressing F6 again never returns the keyboard focus to the panel.
The more serious problem is that, at this point, even pressing CTRL+J doesn't
return focus to the panel: it's open but inaccessible. This is the same
behavior as the popup notifications, but I think it's a bug in both cases.

I think both panels should be dismissed as soon as they lose keyboard focus.
In fact, I managed to have two panels open, the identity panel and an add-on
installation request, overlapping each other. This is not the best experience
visually. I don't know what could be the best behavior as far as screen
readers are concerned, however.

Comment 259

[:Paolo Amadini]

(In reply to comment #249)
> Review of attachment 529878 [details] [diff] [review] [review]:

Thanks for the review! It's funny to see how many comments at this point are
about things I was in doubt to do in one way or the other, and eventually I
did in "the other way" :-) I think I'll count them for my own curiosity ;-)

> ::: browser/components/downloads/DownloadCommon.jsm
> @@ +16,5 @@
> > + * The Original Code is Download Manager Code.
> > + *
> > + * The Initial Developer of the Original Code is
> > + * Paolo Amadini <http://www.amadzone.org/>.
> > + * Portions created by the Initial Developer are Copyright (C) 2010-2011
> 
> just take one, we don't usually specify 2 years. This is usually the year
> you created the file first.

(1) I wonder why I even bothered to do an MXR search to see if there were
files with two recent years specified, and since they existed, I assumed two
years were required when the work before check-in spanned multiple years.
I knew that this modification would stand out in the interdiff though, so
someone would have told me if things were different :-)

> > +const DownloadCommon = {
> > +  /**
> > +   * Returns an object whose keys are the string names from the downloads string
> > +   * bundle, and whose values are either the translated strings or functions
> > +   * that can be used to obtain formatted strings.
> 
> nit: s/functions that can be used to obtain/getters for/

nit on nit: They're not getters, technically. I've rephrased and shortened
as "functions returning formatted strings".

> > +XPCOMUtils.defineLazyServiceGetter(DownloadCommon, "ms",
> > +                                   "@mozilla.org/mime;1",
> > +                                   "nsIMIMEService");
> 
> I see that is short, but to me ms sounds not very describing (indeed I was
> guessing why you had a microsummaries service getter). Why not just "mime"?

(2) Sticking to the convention of "lowercase initials of the words in the
interface name" didn't convince me either, but I wanted a second opinion :-)

> > +  get visible()
> > +  {
> > +    let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> > +    return browserWin ? browserWin.DownloadsPanel.isPanelShowing : false;
> 
> this may have same .closed bug, even if less important since the object
> should still be there. Btw maybe you could add a simple _getBrowserWindow
> helper and use it in both places.

(3) Yeah, that was my reasoning. I opted for shorter code without the helper,
but since you recommend it too, I've now written the helper and referred to
the bug number in its comment.

> ::: browser/components/downloads/downloads.css
> @@ +1,5 @@
> > +/*** Download items ***/
> > +
> > +richlistitem[type="download"] {
> > +  -moz-binding: url('chrome://browser/content/downloads/download.xml#download');
> > +  -moz-box-orient: vertical;
> 
> out of curiosity, why is it needed to set content to horizontal in the
> binding and then orient to vertical in the style?

(4) I noticed this too, but didn't take the time to investigate if I could
remove it, as I've not started styling yet. Thanks for pointing it out
however, as I forgot to add this item to my local checklist and I could
have forgotten to verify it later.

> @@ +69,5 @@
> > +                                           .downloadShow,
> > +
> > +.download-state[state="7"]                 .downloadCommandsSeparator
> > +
> > +{
> 
> nit: fwiw, this file, regardless all the indentation tricks, doesn't seem
> particularly readable to me. But this is a coding style thing I leave to the
> module owner :)

(5) For some background regarding the choice of this CSS styling method,
see comment 157 and comment 160. If you have an idea to make the file
more readable using the same CSS styling method, it's welcome :-)

> ::: browser/components/preferences/tests/Makefile.in
> @@ +45,5 @@
> >  
> >  _BROWSER_FILES = \
> > +    browser_basic_functionality.js \
> > +    browser_delete_key_removes.js \
> > +    head.js \
> 
> just indent 2 spaces here
> 
> PS: Splinter seems to think this is
> browser/components/preferences/tests/Makefile.in, I was puzzled by that, but
> I see you hg copied that file, if possibile I'd suggest to rewrite this file
> since the copy blame is unrelated.

(6) You're not the first one, unfortunately, that gets confused by the fact
than review tools have problems with hg copies. Since writing these makefiles
from scratch is trivial, I'll just follow your suggestion.

> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=2 et sw=2 tw=80: */
> 
> Sdwilsh: does downloads component style guide require the mode and vim
> comments? (btw, tab-width seems wrong)

(7) I had the same doubt about tab-width and ts, the first time I saw these
lines in the style guide. Both are correct, we've already worked this out:
see bug 561472 comment 5. We also concluded that "Mode: C++" is valid for
the lack of a better, widely available Emacs mode.

> @@ +29,5 @@
> > +  for (yy in gen_resetState()) yield;
> > +  for (yy in gen_addDownloadRows(DownloadData)) yield;
> > +  for (yy in gen_openPanel()) yield;
> 
> some comment on each one of these would be welcome, it's a bit cryptic
> otherwise

(8) Ok, I also thought about adding comments but was afraid of over-commenting.

> @@ +37,5 @@
> > +  is(richlistbox.children.length, DownloadData.length,
> > +     "There is the correct number of richlistitems");
> > +  for (var i = 0; i < richlistbox.children.length; i++) {
> > +    var element = richlistbox.children[i];
> > +    var dataItem = new DownloadsViewItemController(element).dataItem;
> 
> nit: these seem good candidates for "let"

Yeah, I globally replaced "var" with "let" now.

> @@ +47,5 @@
> > +  // Clean up when the test finishes.
> > +  for (yy in gen_resetState()) yield;
> > +
> > +  gTestTargetFile.remove(false);
> 
> you should do this in registerCleanupFunction, remember that browser-chrome
> tests don't run in separate sandboxes, so you have to ensure any change you
> make is correctly cleaned up both if the test succeeds and if it fails.

Unfortunately, test infrastructure doesn't allow cleanup functions to be
asynchronous (regardless of the fact that we use generators or not):

http://mxr.mozilla.org/comm-central/source/mozilla/testing/mochitest/browser-test.js#169

My current thinking is that, for the first version, we should really cater
for the case where tests succeed, or fail without an exception or timing out.
The right, complete solution is to update the test infrastructure so that it
is possible to detect timeout, call the throw() function on the generator,
and allow the generator's stack to unwind so that all "finally" blocks are
called (with a second "unwind timeout", of course). I could outline the
required changes in a separate bug, but I don't think the implementation
should be done now or block the new Download Manager.

> Unless this cleanup code will be specific to only some of the tests you
> could even move that registerCleanupFunction to head.js (it works, really)

(9) We can group both initialization and cleanup code in a function in head.js.
I'll do that for sure if I have to add more tests with the same pattern.

> Object.keys(gDownloadRowTemplate).join(", "); ?
> Object.keys(gDownloadRowTemplate).map(function (v) ":" + v).join(", ");

Great suggestions, thanks!

> @@ +96,5 @@
> > +  try {
> > +    // Execute the statement for each of the provided downloads.
> > +    for ([, dataRow] in Iterator(aDataRows)) {
> > +      // Populate insert parameters from the provided data.
> > +      for (let [columnName] in Iterator(gDownloadRowTemplate)) {
> 
> just store columnNames as an array and join() it where needed?

The current way seems more readable to me. Enumerating an array or an object's
keys makes no big difference, and we have the object's keys already.

> @@ +1168,5 @@
> > +          currentset = currentset.replace(/(^|,)urlbar-container($|,)/,
> > +                                          "$1downloads-button,urlbar-container$2")
> > +        }
> > +        else {
> > +          currentset = "downloads-button," + currentset;
> 
> won't this put the button even before back/forward buttons?

(10) Yes. I hope that testers that removed the location bar from the
navigation toolbar won't get too upset by seeing the temporary downloads
button before the back/forward buttons instead of after them ;-)

Comment 260

[Paul [pwd]]

My apologies, I know this is mentioned somewhere in this huge bug, but why was it decided to put the downloads button smack-bang in the middle of the navigational elements (back, forward _AND_ url bar) rather than either before or (preferably) after?

Comment 261

[:Paolo Amadini]

(In reply to comment #260)
> My apologies, I know this is mentioned somewhere in this huge bug, but why
> was it decided to put the downloads button smack-bang in the middle of the
> navigational elements (back, forward _AND_ url bar) rather than either
> before or (preferably) after?

It's explained in comment 215, basically the downloads button you see now is a
temporary placeholder, and this is not the final position of the panel for sure.

Comment 262

[:Paolo Amadini]

(In reply to comment #250)
> nit: per comments readability, double space after periods.

This requires going through all the comments again and changing the style
globally. I'd rather keep French-style spacing at this point, which I've
used consistently until now, but if both you and Shawn require it, I can
find some time to update the style before the patch lands.

> @@ +132,5 @@
> > +    DownloadsView.data = new DownloadsData(DownloadsView);
> > +    DownloadsView.data.initialize();
> 
> is there a case where you have to create a DownloadsData without invoking
> .initialize()? if not you could just merge initialize in the constructor

Hehe, you fell for it :-) It's correct that I always have to call "initialize"
after construction, but if I called it inside the constructor, it would be too
early, as the assignment to the "data" property would not have been executed.

(11) In theory, as "initialize" starts receiving events, it should only be
called after all the objects are hooked up and ready to handle the events
themselves. I considered renaming the functions to "start" and "stop" to
reflect that, though my doubt is that "stop" might look like an optional
call, while "terminate" looks like a mandatory call. What do you think?

> @@ +133,5 @@
> > +    window.addEventListener("unload", this.onWindowUnload, false);
> > +
> > +    DownloadsView.data = new DownloadsData(DownloadsView);
> > +    DownloadsView.data.initialize();
> > +    DownloadsViewController.initialize();
> 
> I think I'd rather provide a initialize() and terminate() in DownloadsView,
> and inside those init/terminate data and controller.
> So the flow here on panel opening is just "we init a view for this panel"
> and on close "we terminate a view for this panel"

Well, as I understand it, from a pure MVC design point of view, the View
shouldn't reference the Controller or be responsible for its initialization.
It's true that I've implemented View and Controller separation very loosely,
to favor code simplicity where appropriate, but by the same principle we
might as well avoid to write two other methods that would be called once.

> @@ +200,5 @@
> > +    this.panel.hidePopup();
> > +
> > +    // Ensure that we allow the panel to be reopened, even if the popup was
> > +    // already hidden and the onPopupHidden event was not invoked.
> > +    this._panelState = this.kPanelHidden;
> 
> is not this already done by onPopupHidden?

Maybe you've just overlooked the comment here, but if the comment is not
clear then suggestions to improve it are welcome :-)

> > +  get isPanelShowing() {
> > +    return this._panelState == this.kPanelShowing ||
> > +           this._panelState == this.kPanelShown;
> > +  },
> 
> I'd suggest to use the short form (no braces and no return) but I think
> Sdwilsh would be unhappy about that :p

Yeah :-)

> > +XPCOMUtils.defineLazyGetter(DownloadsPanel, "panel", function()
> > +                            document.getElementById("downloadsPanel"));
> 
> nit: move function () on the same line as getElementById if it doesn't go
> over 80 chars

It does...

> > +/**
> > + * Builds and updates the downloads list widget, responding to changes in the
> > + * download state and real-time data. In addition, handles part of the user
> > + * interaction events raised by the downloads list widget.
> 
> nit: "real-time data" is a bit generic.

(12) Intentionally? I generally strive to write comments that can be robust
to future modifications in other areas of the code, while still providing an
overall idea of what they're describing, but without being pleonastic with
code written in their immediate surroundings. Of course this is not easy at
all :-) In this case, the solution I chose was to use "state" as a
definition-by-example of the more general group "real-time data". I
even thought that "real-time data" could look a bit generic, but couldn't
come up with a better term or formulation. If you have an idea, it's welcome!

And if you're smiling about how much thought I put in advance in every
secondary detail, you should know that I am smiling about that too :-) But
I see this frame of thought is quite encouraged here! (which I appreciate)

Given the number of items on which I could and would have asked a second
opinion beforehand, I'm also thinking about how being in the same room,
with over-the-shoulder review, makes things a lot faster with regard to
small things like these ones!

[Now I'm taking a break before working on all the following review items...]

Comment 263

[Marco Bonardo [:mak]]

> > is not this already done by onPopupHidden?
> 
> Maybe you've just overlooked the comment here, but if the comment is not
> clear then suggestions to improve it are welcome :-)

Well, the comment may say when that could happen, since looks strange that a popup closes without notifying onpopuphidden.

Regarding "real-time data", I mostly found puzzling to figure out which kind of data, if it was referring to real-time download statistics.

Regarding the comments nit, it's a nit, reviews don't block on those.

Comment 264

[:Paolo Amadini]

(In reply to comment #263)
> > Maybe you've just overlooked the comment here, but if the comment is not
> > clear then suggestions to improve it are welcome :-)
> 
> Well, the comment may say when that could happen, since looks strange that a
> popup closes without notifying onpopuphidden.

Updated, thanks!

> Regarding "real-time data", I mostly found puzzling to figure out which kind
> of data, if it was referring to real-time download statistics.

Not only statistics. I'll have to think if there can be a better term and
update the comment in case I find one.

> Regarding the comments nit, it's a nit, reviews don't block on those.

Yes, I imagine. I'm not trying to be serious in most of my replies to those
nits :-) I hope it's not a problem if I reply in a playful way!

(In reply to comment #250)
> @@ +350,5 @@
> > +  _addOrMoveElementToTop: function DV_addOrMoveElementToTop(aElement, aDataItem)
> > +  {
> > +    // Items that are done are inserted before all other items.
> > +    if (aDataItem.done) {
> > +      this.richListBox.insertBefore(aElement, this.richListBox.firstChild);
> 
> should you unset this.firstElementInProgress and this.firstElementNotDone if
> they are same as aElement?
> 
> > +    if (!aDataItem.inProgress) {
> > +      this.richListBox.insertBefore(aElement, this.firstElementNotDone);
> > +      this.firstElementNotDone = aElement;
> 
> should you unset this.firstElementInProgress if it's aElement?

This is done in _beforeRemoveElement, if I understand your comments correctly.

> @@ +430,5 @@
> > +    // otherwise it means that no such element exists anymore.
> > +    if (aElement != this.firstElementNotDone ||
> > +        aElement != this.firstElementInProgress) {
> > +      return;
> > +    }
> 
> shouldn't this be an AND? if aElement is going away you should ensure it's
> not used in both.

Good catch! I missed this while inverting the condition.

> @@ +453,5 @@
> > +    }
> > +    if (aElement == this.firstElementInProgress) {
> > +      this.firstElementInProgress = nextDataItem.inProgress ? nextElement
> > +                                                            : null;
> > +    }
> 
> Unless DownloadsViewItemController() is much expensive I think inverting the
> ifs could be more readable:

(13) It's a lookup in the dictionary of active downloads. I'm not sure how
much expensive it could become when there are many downloads. Even if I
don't think it's significant, in doubt I preferred to ensure that we don't
do two lookups unnecessarily (originally I wrote the ifs like you suggested).

> @@ +461,5 @@
> > +   * Called when the number of download items in the list changes.
> > +   */
> > +  _itemCountChanged: function DV_itemCountChanged()
> > +  {
> > +    // Note that the "itemCount" property may not be available now.
> 
> clarify comment please (Add when and why, "now" is not enough)

Ah, this time you caught me out! I didn't remember for sure in which cases the
property was not available anymore. But I've now verified that it happens
before the panel is shown for the first time, because XBL bindings haven't
been applied yet.

> @@ +486,5 @@
> > +  {
> > +    let previousSelectedIndex = this.richListBox.selectedIndex;
> > +    this._beforeRemoveElement(aElement);
> > +    this._addOrMoveElementToTop(aElement, aDataItem);
> > +    this.richListBox.selectedIndex = previousSelectedIndex;
> 
> What if I selected this specific element, should not the selection follow it?

(14) Well, maybe it should, if it's the only selected element, but maybe not
if there are multiple elements selected, even if it's the currently focused
element. In general, I think that the mere fact that elements change their
position in the list might be confusing for keyboard navigation. Probably
we should leave the exact selection mechanics for a later phase.

> @@ +550,5 @@
> > +   * @param aNewest
> > +   *        When true, indicates that this item is the most recent and should be
> > +   *        added in the topmost position of its category. This happens when a
> > +   *        new download is started. When false, indicates that the item is the
> > +   *        least recent with regard to the items that have been already added.
> 
> "least recent" or just "not the most recent"?

Least recent. Please don't ask to change aNewest to an enum for clarity :-)

> @@ +551,5 @@
> > +   *        When true, indicates that this item is the most recent and should be
> > +   *        added in the topmost position of its category. This happens when a
> > +   *        new download is started. When false, indicates that the item is the
> > +   *        least recent with regard to the items that have been already added.
> > +   *        This generally happens during the initial data load, when we want
> 
> What is "this", that is set to true?

That is set to false. I've now replaced "this" with "the latter".

> @@ +625,5 @@
> > +      return;
> > +    }
> > +
> > +    // Find the richlistitem for the download and execute the command.
> > +    for (; target.nodeName != "richlistitem"; target = target.parentNode);
> 
> better avoiding empty-body for loops, they are confusing and error-prone.
> use a while instead:
> while (target.nodeName != "richlistitem") {
>   target = target.parentNode;
> }

I prefer this way too. The fact that you suggested using such a loop in
attachment 526902 [details] made me think you preferred one-liners.

> @@ +635,5 @@
> > +    // Double clicks on buttons should not invoke the default action.
> > +    if (aEvent.originalTarget.hasAttribute("command") ||
> > +        aEvent.originalTarget.hasAttribute("oncommand")) {
> > +      return;
> > +    }
> 
> aEvent.preventDefault() ? or you're just trying to skip next code?

Yes, "default action" in the sense of "the default action associated with the
download item". I've clarified the comment, thanks!

Note: I'm skipping review comments on how to improve the drag-and-drop section
for now. There is no way to drop something on the panel after all :-) Drop
handling will move to the download progress indicator. The new functions
accessible from Services.droppedLinkHandler are cool however!

> @@ +725,5 @@
> > +  this.dataItem = aDataItem;
> > +
> > +  this.previousDone = this.dataItem.done;
> > +  this.previousInProgress = this.dataItem.inProgress;
> > +  this.lastSeconds = Infinity;
> 
> I don't have any idea what lastSeconds represents, better name?

My impression is that you have a score to settle with the current Download
Manager... not a single name or code line can escape and find their way in
the new code :-)

"lastSeconds", or "lastSec", is the name of an opaque parameter of the
"getDownloadStatus" function in the "DownloadUtils" module. It's used to
calculate the remaining download time more accurately. The comment says:
"Last time remaining in seconds or Infinity for unknown". Given this, what
do you think a better name could be?

> @@ +737,5 @@
> > +    "state": this.dataItem.state,
> > +    "progress": this.dataItem.inProgress ? this.dataItem.percentComplete : 100,
> > +    "target": this.dataItem.target,
> > +    "image": this.image
> > +  };
> 
> const

Can you explain the rationale of using "const" here? I often use "let" in a
lot of cases where the rest of the function doesn't change the variable's
value. Are "const" assignments re-evaluated when re-entering the same
function? (I couldn't find this documented.) If so, does it still make
sense to use "const"?

> @@ +777,5 @@
> > +      try {
> > +        // Refresh the icon, so that executable icons are shown. Tacking on
> > +        // contentType bypasses cache.
> > +        this.image += "&contentType=" + DownloadCommon.ms.getTypeFromFile(
> > +                                                       this.dataItem.localFile);
> 
> rephrase comment and fix indentation a bit

Another couple of lines that didn't escape :-)

> > +      let newLast;
> 
> better var name

It's the same declared name as the function's output. Suggestions?

> @@ +904,5 @@
> > +   * Updates the time that gets shown for completed download items.
> > +   */
> > +  _updateTime: function DVI_updateTime()
> > +  {
> > +    // Don't bother updating for things that aren't finished.
> 
> "things"?

Another line from the original Download Manager! You're a killer!

> @@ +1057,5 @@
> > +  {
> > +    // Update all the commands that are specific for the selected item.
> > +    for ([cmd] in Iterator(DownloadsViewItemController.prototype.commands)) {
> > +      goUpdateCommand(cmd);
> > +    }
> 
> why not just for (cmd in DownloadsViewItemController.prototype.commands) ?

For the reason outlined in comment 182: excluding enumerable properties
coming from the object's prototype chain.

Comment 265

[Marco Bonardo [:mak]]

Regarding selection and D&D, I guess you have a todo, would be fine to expose it somewhere and link it in the url field of this bug (either a feature page on the wiki, or an etherpad).

> "lastSeconds", or "lastSec", is the name of an opaque parameter of the
> "getDownloadStatus" function in the "DownloadUtils" module.

That rounding code is awesome :p
Maybe something like lastKnownSecondsLeft, lastCachedSecondsLeft, lastCachedRemainingTime, lastRemainingSeconds.
Actually my favorite (apart being verbose) is lastEstimatedSecondsLeft

> Can you explain the rationale of using "const" here?

Actually, mostly because it actually is const data, but I don't have that level of knowledge of js internals to have a strong opinion on it, consider it a nit.

> > > +      let newLast;

I see this is in couple with the above one, newEstimatedSecondsLeft?

Comment 266

[:Paolo Amadini]

(In reply to comment #265)
> Regarding selection and D&D, I guess you have a todo, would be fine to
> expose it somewhere and link it in the url field of this bug (either a
> feature page on the wiki, or an etherpad).

Excellent, doing this was already in my todo list ;-) I planned to start
cleaning up and publishing the todo list after code review, maybe I can
anticipate it to sometime near the end of this week.

> Actually my favorite (apart being verbose) is lastEstimatedSecondsLeft

Works for me too! We have the required horizontal space :-)

(In reply to comment #250)
> > +        // Refresh the icon, so that executable icons are shown. Tacking on
> > +        // contentType bypasses cache.
> > +        this.image += "&contentType=" + DownloadCommon.ms.getTypeFromFile(
> > +                                                       this.dataItem.localFile);

Digression on this one: I took some time to understand why we are adding the
contentType, and after reading the documentation and some experimentation, it
turns out that changing any parameter supported by nsIMozIconURI works as a
way of forcing the image to be reloaded (adding other parameters such as
"&the_usual_trick_to_force_reloading_urls=random_number" has no effect).

In the end, this line is enough:

this._element.setAttribute("image", this.image + "&state=normal");

We don't even need to call the MIME service for getting the content type, the
"moz-icon" channel already does that for us.

Documentation in the source code:

http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/nsIIconURI.idl#42

http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpr0n/test/unit/test_moz_icon_uri.js#9

History of changes / blame / annotation for that part of the code:

http://hg.mozilla.org/mozilla-central/diff/d4f5ebf5c55f/toolkit/mozapps/downloads/content/downloads.js#l1.135

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/toolkit/mozapps/downloads/content&command=DIFF&root=/cvsroot&file=downloads.js&rev1=1.20&rev2=1.21

https://bugzilla.mozilla.org/show_bug.cgi?id=236988

Comment 267

[:Paolo Amadini]

(In reply to comment #250)
> > +    let end = new Date(parseInt(this.dataItem.endTime));
> 
> why do you need to parseInt this here, shouldn't rather that be done on
> setting the property?

Correct, no need to parseInt as endTime is always of the number data type,
resulting from Date.now() or Math.round().

> @@ +1132,5 @@
> > +    // Invoke the command, if enabled, using this object for "this".
> > +    if (this.isCommandEnabled(aCommand)) {
> > +      this.commands[aCommand].apply(this);
> 
> I think you could actually use .bind(this) when defining the functions and
> avoid these apply.

Actually, using "apply" is simpler here, because these functions are defined
on the object's prototype, and using "bind" (as I've seen it used elsewhere)
would require enumerating all the commands in the object's constructor, to
use only one of them. And, of course, there is no point in just doing
".bind(this)()" instead of "apply(this)".

> > +  _getOrAddDataItem: function DD_getOrAddDataItem(aSource)
> 
> I'd just rename it to fetchDataItem, where it fetches it doesn't matter.

nit: I prefer getOrAdd since the function may invoke "onDataItemAdded"
(the "add" case) or may not do it (the "get" case).

> > +    dataItem.startTime = Math.round(aDownload.startTime / 1000);
> 
> I think you may want to parseInt here, any reason to keep a double?

The JavaScript documentation says that the result of Math.round is already an
integer. More exactly, it's an instance of the general number data type.

> @@ +1713,5 @@
> > +    // Compute the other properties without accessing the download object.
> > +    this.resumable = true;
> 
> is this always true?

No, you're right: this could actually result in a bug for which a running
download would look resumable if the panel is opened in a different window
from the one where the download started. Unfortunately, there is no way to
know the value of the property without accessing the underlying download
object, but I think that, for running downloads, we can access the object
without performance degradation. I've updated the code.

> @@ +1748,5 @@
> > +  get starting()
> > +  {
> > +    return this.state == nsIDM.DOWNLOAD_NOTSTARTED ||
> > +           this.state == nsIDM.DOWNLOAD_QUEUED;
> > +  },
> 
> I'd drop braces and return (if sdwilsh allows)

Doesn't ;-) By the way, at present here I have one-liners with short "get"
syntax mixed with with two-liners with normal "get" syntax. I'd rather be
consistent (for example, always use normal syntax). What do you think?

Comment 268

[Marco Bonardo [:mak]]

(In reply to comment #267)
I'm doing some experiment with bind(), will let you know if I figure out a better way.

> The JavaScript documentation says that the result of Math.round is already an
> integer. More exactly, it's an instance of the general number data type.

ehr, I misread that line as having a division out of the round, sorry.

> Doesn't ;-) By the way, at present here I have one-liners with short "get"
> syntax mixed with with two-liners with normal "get" syntax. I'd rather be
> consistent (for example, always use normal syntax). What do you think?

I can't tell, personally I'd go for the short syntax everytime there is only a short return condition, so it doesn't affect readability, but it's up to the module owner to decide the code style for the module. Both ways being consistent is a win.

Comment 269

[Shawn Wilsher :sdwilsh]

(In reply to comment #267)
> Doesn't ;-) By the way, at present here I have one-liners with short "get"
> syntax mixed with with two-liners with normal "get" syntax. I'd rather be
> consistent (for example, always use normal syntax). What do you think?
My rule of thumb is if it spans more than one line, use the long syntax (equivalent to brace all ifs), and if it doesn't, do whichever you want.

Comment 270

[:Paolo Amadini]

Attachment: Improved version with review comments addressed

Here is the updated patch. Thank you very much for the detailed review, Marco!
I'll let you check if I've interpreted your comments correctly before requesting
code review from Shawn and generating another tryserver build.

Comment 271

[Marco Bonardo [:mak]]

Comment on attachment 533137 [details] [diff] [review]
Improved version with review comments addressed

Review of attachment 533137 [details] [diff] [review]:
-----------------------------------------------------------------

notice I only looked at the patches diff, it looks fine based on my comments.  Thanks for so much patience on them :)

::: browser/components/downloads/Makefile.in
@@ +1,2 @@
> +# Any copyright is dedicated to the Public Domain.
> +# http://creativecommons.org/publicdomain/zero/1.0/

Ehr, I think you cannot use the pd license in Makefile, that license can only be used in test files.

::: browser/components/downloads/downloads.js
@@ +259,5 @@
> +  },
> +
> +  onPopupShown: function DO_onPopupShown(aEvent)
> +  {
> +    // Ignore events raised by children.

nit: by nested popups.

@@ +269,5 @@
> +  },
> +
> +  onPopupHidden: function DO_onPopupHidden(aEvent)
> +  {
> +    // Ignore events raised by children.

nit: by nested popups.

::: browser/installer/package-manifest.in
@@ +306,5 @@
>  #endif
>  @BINPATH@/components/nsHelperAppDlg.manifest
>  @BINPATH@/components/nsHelperAppDlg.js
> +@BINPATH@/components/DownloadManagerUI.manifest
> +@BINPATH@/components/DownloadManagerUI.js

I think you may have to update removed-files.in. Right now nsDownloadManagerUI.manifest and nsDownloadManagerUI.js are removed for omnijar, I've no idea if there are still non-omnijar consumers around though. If so those files should probably be moved out of the omnijar ifdef in removed-files.in

Comment 272

[:Paolo Amadini]

Comment on attachment 533137 [details] [diff] [review]
Improved version with review comments addressed

Thanks Marco! I think the patch is ready to be reviewed by Shawn (noting that
the few modifications in comment 271 will be addressed in the next version).
I'll provide an updated patch and try building it on the tryserver after that.

Regarding the licensing policy, I re-read it again and my interpretation is
that makefiles are not Product Code because they're merely part of a system
used to build the product, and no part of those files ends up in the binaries:

http://www.mozilla.org/MPL/license-policy.html

"Product Code means all files any part of which are included in nightly
binaries of the Mozilla family of Internet clients - Firefox, Thunderbird,
SeaMonkey and Camino. Note that this definition excludes the code of tools
used merely to build these products, such as system libraries."

While many parts of the build system are complex and worth of copyright
protection, the particular makefiles in my patch are really trivial, and
there's little creativity in them, because they cannot be written in a
significantly different way. As such, my interpretation is that these
particular makefiles could be covered by the Trivial Support Code Exception:

http://www.mozilla.org/MPL/license-policy-flowchart.png

This should give the option of including those files in the tree and retain
their public domain status. This avoids including a boilerplate text which
would be longer than the file itself.

Note that I've already dedicated those few lines to the public domain in my
patch. This is relevant in case someone thinks they are copyrightable at all,
which I doubt given that there's little or no creativity in them. Adding a
copyright notice now will have no practical effect, except if someone updates
the file in the future, that however, given the nature of the file, I expect
to be limited to mechanical work only.

Also note that I've dedicated to the public domain all the Mozilla test code
I wrote, despite it's not so trivial (in fact, we've already discussed many
little details about test implementation).

I hope I've made my point, but in doubt we can consult the licensing team.

Comment 273

[Marco Bonardo [:mak]]

(In reply to comment #272)
> While many parts of the build system are complex and worth of copyright
> protection

I think it's not much about strict copyright protection.  Btw, I mostly commented about that, because this is the first Makefile.in I see with that license, so it's worth figuring out if for future we can reduce the boilerplate in these files. I will ping Gerv and ask :)

> Also note that I've dedicated to the public domain all the Mozilla test code
> I wrote, despite it's not so trivial

Be clear, you don't have to, if you don't want.  We suggest doing it just for a shorter boilerplate (I think MPL2 will also have that benefit).

Comment 274

[Shawn Wilsher :sdwilsh]

(In reply to comment #273)
> Be clear, you don't have to, if you don't want.  We suggest doing it just for a
> shorter boilerplate (I think MPL2 will also have that benefit).
Also because it means that anybody can then use that code (so our test files become good documentation).

Comment 275

[:Paolo Amadini]

(In reply to comment #273)
> I think it's not much about strict copyright protection.  Btw, I mostly
> commented about that, because this is the first Makefile.in I see with that
> license, so it's worth figuring out if for future we can reduce the
> boilerplate in these files. I will ping Gerv and ask :)

This also was my personal motivation for writing comment 272 :-) Feel free
to point Gerv to it if he's the person to ask!

> > Also note that I've dedicated to the public domain all the Mozilla test code
> > I wrote, despite it's not so trivial
> 
> Be clear, you don't have to, if you don't want.  We suggest doing it just
> for a shorter boilerplate (I think MPL2 will also have that benefit).

Yes, by reading the policy I also understood it was not required, though
recommended (the flowchart is just a quick summary in this regard).

(In reply to comment #274)
> Also because it means that anybody can then use that code (so our test files
> become good documentation).

This is a good point.

Comment 276

[Marco Zehe (:MarcoZ)]

(In reply to comment #258)
> (In reply to comment #236)
> > 2. Problem: When tabbing from the list onwards, I hit the "Open" button
> > twice. Once it's a sub menu button, the second a regular button. The next
> > thing in the tab order is the "Show download history" button. Why these two
> > tabstops on the Open button?
> 
> I verified that this is the same behavior as the menu-button in the popup
> notifications (for example, the one that is opened when you click an add-on
> download link in any website). Pressing ALT+Down (on Windows) opened the
> drop-down menu only when focus was on the part of the button with the down
> arrow, not on the main button. Maybe this is the expected behavior of XUL
> menu-buttons? If not, I think a different bug should be filed.

All right, yes this is also already being discussed in bug 616136 which exhibits a similar issue.

> > 4. Problem: The panel does not announce itself as being the downloads panel
> > any more. The old window had a title screen readers were able to read. How
> > is it conveyed visually that this is the Downloads window? If through some
> > sort of image, we'll have to apply some WAI-ARIA magic to also give the
> > panel a label.
> I'd like to be initiated to this kind of magic :-) Which is the method
> you have in mind for implementing this panel labeling? Which is the method
> that is currently used for labeling the "popup notifications" panel, if any?

The notification popup does not have a label, but it has explanatory text (like "Do you want to save the password..."). The reason I asked how this panel is labelled visually is exactly to determine which method to use for labelling it for screen readers.

The simplest is to put an aria-label attribute pointing to a string that contains a human-readable label, for example the same that was used for the old "Downloads" window.
However, if there is an image labelling this panel, then that should receive alternative text (like a tool-tip text or something), and the attribute aria-labelledby should be used, with its value being the ID of the labelling element (in my example, the image).

> > It should also get a proper role, since it is currently
> > non-existent in the a1y hierarchy and needs to be a container. This is done
> > by applying the role attribute to the panel itself. The available roles can
> > be found in the wAI-ARIA documentation:
> > http://www.w3.org/TR/wai-aria/roles#role_definitions
> > For this case, a role of dialog is probably most appropriate, or group, we'd
> > need to test those.
> Which tools can I use to do a basic verification that the panel is included
> in the accessibility hierarchy, if I assign a role attribute to it?

If you assign the role attribute, you can be sure it is included, because *everything* that has the role attribute set also gets an accessible object created for it.
I am not certain about the current status of DOM Inspector, but it used to also have a working accessibility inspector. Note that this currently only works on Linux and Windows, not on mac, since accessibility isn't turned on for Mac OS X yet.

Regarding the dismissal of the panels, the most important rule is: Be consinstent, not confusing. ;)

Comment 277

[Shawn Wilsher :sdwilsh]

This review should happen on Monday.

Comment 278

[:Paolo Amadini]

*NEW PANEL-BASED DOWNLOAD MANAGER TRACKING PAGE*

I've polished and published my local list of open tasks, turning it into a
tracking page that I'll use to make sure that all issues raised on this
specific bug, and its dependencies, are addressed before the work is finished.

Formatting could be improved, but the status information I have is all there:

https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager



*NEW DOWNLOAD USER EXPERIENCE IMPROVEMENTS SUMMARY PAGE*

I've also reviewed and published a more general page that shows how this
feature fits in the overall download manager user experience improvements.
This is important because all the other features listed in that page are
"next steps", and explicit non-goals of this first part of the overall
improvements. The page is available here:

https://wiki.mozilla.org/User:P.A./Download_user_experience_improvements



*FEEDBACK*

Since there are now many places where possible download manager improvements
are described, and many different pieces of the puzzle are on the table, it's
important that feedback about each piece is provided using the right channels.

Comments directly relevant to the work that is being done in this bug
(downloads panel coding, development, implementation, as well as reviews
from the Accessibility, Development, Localization and User Experience teams)
should be posted here. I'll periodically update the tracking page according
to new comments and solved items.

For more general feedback or suggestions about the design, or about the
other features that are not being developed here, the usability forum is
probably the best place:

http://www.mozilla.org/about/forums/#dev-usability

Comment 279

[Shawn Wilsher :sdwilsh]

(In reply to comment #278)
> https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager
That page is simply awesome.  Thanks so much for doing that!

Comment 280

[Shawn Wilsher :sdwilsh]

Comment on attachment 533137 [details] [diff] [review]
Improved version with review comments addressed

Review of attachment 533137 [details] [diff] [review]:
-----------------------------------------------------------------

You should have fryn or dao look over your css and xbl changes as they are far better practiced in writing and reviewing that stuff than I am right now.

I didn't look at the tests either, but I trust mak to have caught any issues there at this point (I'm also all reviewed out at this point).

r=sdwilsh with comments addressed (yay!)

::: browser/components/downloads/DownloadCommon.jsm
@@ +85,5 @@
> +      if (stringName in kDownloadsStringsRequiringFormatting) {
> +        strings[stringName] = function() {
> +          // Convert "arguments" to a real array before calling into XPCOM
> +          return sb.formatStringFromName(stringName,
> +                                         Array.prototype.slice.call(arguments),

I'm 95% sure you don't need 'prototype' here.  Additionally, the first argument to slice is required and should be zero (you aren't passing in anything here, which amusingly seems to work but I'd rather not depend on that working).

@@ +99,5 @@
> +};
> +
> +XPCOMUtils.defineLazyServiceGetter(DownloadCommon, "dm",
> +                                   "@mozilla.org/download-manager;1",
> +                                   "nsIDownloadManager");

We should probably just add this to Services.jsm at this point and not include it here.  Everything can then just import that.

::: browser/components/downloads/DownloadManagerUI.js
@@ +91,5 @@
> +  /**
> +   * Returns the most recent browser window that is not closed, or null if no
> +   * such window exists. This helper function will be required at least until
> +   * bug 528706 (nsIWindowMediator::getMostRecentWindow should not return closed
> +   * windows) is fixed.

Can you file a bug about this explicitly, mark it dependent on that bug, and reference the bug number that you file here please?

::: browser/components/downloads/download.xml
@@ +83,5 @@
> +                             tooltiptext="&cmd.pausedisabled.tooltiptext;"
> +                             disabled="true"/>
> +          <xul:toolbarbutton class="downloadButton downloadResume"
> +                             tooltiptext="&cmd.resume.label;"
> +                             oncommand="DownloadsView.onItemCommand(event, 'downloadsCmd_resume');"/>

Why are these xul:toolbarbuttons as opposed to xul:button?  They aren't in a toolbar, so I'm fairly certain they should be xul:buttons instead (especially since you use xul:button further down).

@@ +95,5 @@
> +                             flex="1"
> +                             class="downloadProgressRunning"
> +                             min="0"
> +                             max="100"
> +                             xbl:inherits="mode=progressmode,value=progress"/>

Why did you opt for using three different progressmeters here instead of having one and changing its attributes (via xbl:inherits)?  I'm worried that the current approach makes this binding more expensive.

::: browser/components/downloads/downloads.css
@@ +13,5 @@
> +/*** Visibility of controls inside download items ***/
> +
> +.download-state:-moz-any(     [state="6"], /* Blocked     */
> +                              [state="8"], /* Blocked     */
> +                              [state="9"]) /* Blocked     */

Can you make the Blocked comment a little more detailed?  For example:
[state="8"], /* Bocked (DIRTY) */
This comment applies to the rest of the times you do this in this file.

::: browser/components/downloads/downloads.js
@@ +55,5 @@
> + *
> + * DownloadsView
> + * Builds and updates the downloads list widget, responding to changes in the
> + * download state and real-time data. In addition, handles part of the user
> + * interaction events raised by the downloads list widget.

I know mak commented on this, and this is a super nitty nit, but I'd really prefer to see two spaces after punctuation.  That is the dominant style throughout Mozilla code, as well.  With that being said, I'm not going to ask you to change it all.  That's your call.

@@ +89,5 @@
> +Cu.import("resource://gre/modules/DownloadUtils.jsm");
> +Cu.import("resource:///modules/DownloadCommon.jsm");
> +
> +const nsLocalFile = Components.Constructor("@mozilla.org/file/local;1",
> +                                           "nsILocalFile", "initWithPath");

Make this lazy please (with XPCOMUtils.defineLazyGetter)

@@ +218,5 @@
> +
> +  /**
> +   * Indicates whether the panel is shown or will be shown.
> +   */
> +  get isPanelShowing() {

nit: brace goes on a newline

@@ +388,5 @@
> +   *        XUL element associated with the download item. If the element is
> +   *        already in the list, it is moved to the appropriate position.
> +   *
> +   * @param aDataItem
> +   *        DownloadsDataItem that is being added or moved.

global-nit: no newlines between @param statements

@@ +910,5 @@
> +
> +      // We use the same XUL label to display both the state and the amount
> +      // transferred, separating them with the Unicode character U+2014 'EM
> +      // DASH'. For example, "Paused -  1.1 MB".
> +      status = DownloadCommon.strings.statePaused + " \u2014  " + transfer;

This doesn't feel very l10n friendly (namely rtl languages).

@@ +925,5 @@
> +    } else if (this.dataItem.state == nsIDM.DOWNLOAD_SCANNING) {
> +      status = DownloadCommon.strings.stateScanning;
> +    } else if (!this.dataItem.inProgress) {
> +      let stateLabel = function()
> +      {

nit: for inline functions like this, the brace should go on the same line as function()

@@ +943,5 @@
> +
> +      // We use the same XUL label to display both the state and the host name,
> +      // separating them with the Unicode character U+2014 'EM DASH'. For
> +      // example, "Canceled - 222.net" or "1.1 MB - website2.com".
> +      status = stateLabel + " \u2014 " + displayHost;

same here regarding l10n/rtl friendliness.

@@ +1238,5 @@
> +        try {
> +          dontAsk = !Services.prefs.getBoolPref(this.kPrefBdmAlertOnExeOpen);
> +        } catch (ex) { }
> +
> +#ifdef XP_WIN

If we can avoid prepocessing this file by some checks in JavaScript, I'd be a happier man.

@@ +1406,5 @@
> +  _openExternal: function DVIC_openExternal(aFile)
> +  {
> +    let protocolSvc = Cc["@mozilla.org/uriloader/external-protocol-service;1"]
> +                      .getService(Ci.nsIExternalProtocolService);
> +    protocolSvc.loadUrl(Services.io.newFileURI(aFile));

I'd prefer to see you use NetUtil.newURI here.

::: toolkit/components/downloads/Makefile.in
@@ +72,5 @@
>  # 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))

I think the right way to work around this problem is to just unregister the default component in final-ui-startup in browser land for the contract id.  I'm saying this without doing a whole lot of investigation to make sure that would work though...

Comment 281

[:Paolo Amadini]

(In reply to comment #280)
> r=sdwilsh with comments addressed (yay!)

Yay! :-)

> ::: browser/components/downloads/download.xml
> @@ +83,5 @@
> > +                             tooltiptext="&cmd.pausedisabled.tooltiptext;"
> > +                             disabled="true"/>
> > +          <xul:toolbarbutton class="downloadButton downloadResume"
> > +                             tooltiptext="&cmd.resume.label;"
> > +                             oncommand="DownloadsView.onItemCommand(event, 'downloadsCmd_resume');"/>
> 
> Why are these xul:toolbarbuttons as opposed to xul:button?  They aren't in a
> toolbar, so I'm fairly certain they should be xul:buttons instead
> (especially since you use xul:button further down).

The popup notifications used toolbarbuttons too, because this way you avoid
writing the additional CSS that is required to style the button as images,
I guess. Maybe ask Gavin?

> @@ +95,5 @@
> > +                             flex="1"
> > +                             class="downloadProgressRunning"
> > +                             min="0"
> > +                             max="100"
> > +                             xbl:inherits="mode=progressmode,value=progress"/>
> 
> Why did you opt for using three different progressmeters here instead of
> having one and changing its attributes (via xbl:inherits)?  I'm worried that
> the current approach makes this binding more expensive.

This just allowed to style the progress bars differently, without the
JavaScript needing additional code. Currently they all have the same style,
though, so having a single progress bar and a little bit more JavaScript
code might actually be better.

> ::: browser/components/downloads/downloads.js
> @@ +55,5 @@
> > + *
> > + * DownloadsView
> > + * Builds and updates the downloads list widget, responding to changes in the
> > + * download state and real-time data. In addition, handles part of the user
> > + * interaction events raised by the downloads list widget.
> 
> I know mak commented on this, and this is a super nitty nit, but I'd really
> prefer to see two spaces after punctuation.  That is the dominant style
> throughout Mozilla code, as well.  With that being said, I'm not going to
> ask you to change it all.  That's your call.

Since you both ask, I'll make a note to do that before the final checkin :-)

> @@ +910,5 @@
> > +
> > +      // We use the same XUL label to display both the state and the amount
> > +      // transferred, separating them with the Unicode character U+2014 'EM
> > +      // DASH'. For example, "Paused -  1.1 MB".
> > +      status = DownloadCommon.strings.statePaused + " \u2014  " + transfer;
> 
> This doesn't feel very l10n friendly (namely rtl languages).

This is actually an open item from comment 253. It seems that RTL languages
don't need to change the order of those strings. See for example:

http://mxr.mozilla.org/l10n-central/search?string=paused%3D

Note that for Arabic you might see text like "1#  - " because of the
directionality change in the text, but the string is still " -  #1".

> @@ +1238,5 @@
> > +        try {
> > +          dontAsk = !Services.prefs.getBoolPref(this.kPrefBdmAlertOnExeOpen);
> > +        } catch (ex) { }
> > +
> > +#ifdef XP_WIN
> 
> If we can avoid prepocessing this file by some checks in JavaScript, I'd be
> a happier man.

Nice! I'd also prefer that.

> ::: toolkit/components/downloads/Makefile.in
> @@ +72,5 @@
> >  # 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))
> 
> I think the right way to work around this problem is to just unregister the
> default component in final-ui-startup in browser land for the contract id. 
> I'm saying this without doing a whole lot of investigation to make sure that
> would work though...

Seems strange to me, because we know at build time that we don't need that
component. We shouldn't probably include neither the component nor the old
UI at all.

Comment 282

[Shawn Wilsher :sdwilsh]

(In reply to comment #281)
> The popup notifications used toolbarbuttons too, because this way you avoid
> writing the additional CSS that is required to style the button as images,
> I guess. Maybe ask Gavin?
Ah, I didn't think of checking the other things like that.

> This is actually an open item from comment 253. It seems that RTL languages
> don't need to change the order of those strings. See for example:
> 
> http://mxr.mozilla.org/l10n-central/search?string=paused%3D
> 
> Note that for Arabic you might see text like "1#  - " because of the
> directionality change in the text, but the string is still " -  #1".
Can you check with l10n please?  I'm still worried even if no locale currently does it differently.

> Seems strange to me, because we know at build time that we don't need that
> component. We shouldn't probably include neither the component nor the old
> UI at all.
Then we have no way to test it, which isn't acceptable (just because Firefox doesn't use it, it doesn't mean that it becomes unsupported in toolkit).  You can ask Dave Townsend (toolkit module owner) what he thinks is acceptable here.

Comment 283

[:Margaret Leibovic]

(In reply to comment #281)
> > ::: browser/components/downloads/download.xml
> > @@ +83,5 @@
> > > +                             tooltiptext="&cmd.pausedisabled.tooltiptext;"
> > > +                             disabled="true"/>
> > > +          <xul:toolbarbutton class="downloadButton downloadResume"
> > > +                             tooltiptext="&cmd.resume.label;"
> > > +                             oncommand="DownloadsView.onItemCommand(event, 'downloadsCmd_resume');"/>
> > 
> > Why are these xul:toolbarbuttons as opposed to xul:button?  They aren't in a
> > toolbar, so I'm fairly certain they should be xul:buttons instead
> > (especially since you use xul:button further down).
> 
> The popup notifications used toolbarbuttons too, because this way you avoid
> writing the additional CSS that is required to style the button as images,
> I guess. Maybe ask Gavin?

Popup notifications use a toolbarbutton for the close button because we're sharing styles with the notification bar messageCloseButton, which is a toolbarbutton. You should probably just use a normal button here.

Comment 284

[Axel Hecht [:Pike]]

(In reply to comment #282)
> (In reply to comment #281)
> > This is actually an open item from comment 253. It seems that RTL languages
> > don't need to change the order of those strings. See for example:
> > 
> > http://mxr.mozilla.org/l10n-central/search?string=paused%3D
> > 
> > Note that for Arabic you might see text like "1#  - " because of the
> > directionality change in the text, but the string is still " -  #1".
> Can you check with l10n please?  I'm still worried even if no locale
> currently does it differently.

Sorry for the lag here, I've had my head focused on a few other items.

Yes, please don't assume things to be equal across all languages, even if the ones we have make no other choice. They could be wrong, and we could encounter new languages that need the flexibility.

The localization notes are great stuff, nice job on those.

Comment 285

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

(In reply to comment #280)
> > +   * Returns the most recent browser window that is not closed
> 
> Can you file a bug about this explicitly, mark it dependent on that bug, and
> reference the bug number that you file here please?

You probably just want to use nsIBrowserGlue::getMostRecentBrowserWindow here instead.

Comment 286

[:Paolo Amadini]

Attachment: Addressed review nits

Updated version. You might want to take a look to the interdiff to verify the
few changes to the localized strings file, as well as the other minor changes.
See below for more comments.

(In reply to comment #280)
> ::: browser/components/downloads/DownloadManagerUI.js
> @@ +91,5 @@
> > +  /**
> > +   * Returns the most recent browser window that is not closed, or null if no
> > +   * such window exists. This helper function will be required at least until
> > +   * bug 528706 (nsIWindowMediator::getMostRecentWindow should not return closed
> > +   * windows) is fixed.
> 
> Can you file a bug about this explicitly, mark it dependent on that bug, and
> reference the bug number that you file here please?

If I understand correctly, this is required to track that an action should be
taken after both that bug and this one are marked as fixed, right? I ask
because this might not be necessary here, since this function is gone, but
maybe we should do the same for bug 492960, which is now just marked as a
dependency of this bug.

> I'd prefer to see you use NetUtil.newURI here.

About newURI in general, I'm now using the following code in another place:

let fileUrl = NetUtil.newURI(this.file).QueryInterface(Ci.nsIFileURL);

Last time I checked, the QueryInterface call (or an "instanceof" check) was
required in order to access the properties defined on nsIFileURL. If this
is still the case, shouldn't "NetUtil.newURI" do the QueryInterface for us?
Currently it doesn't, even if it would be a very simple change:

http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/NetUtil.jsm#228

> ::: toolkit/components/downloads/Makefile.in
> @@ +72,5 @@
> >  # 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))
> 
> I think the right way to work around this problem is to just unregister the
> default component in final-ui-startup in browser land for the contract id. 
> I'm saying this without doing a whole lot of investigation to make sure that
> would work though...

As far as I know, that wouldn't work because we are running all the Toolkit
chrome tests with a browser window open. I did my own investigation, and will
post my proposal in a separate comment later :-)

Comment 287

[Shawn Wilsher :sdwilsh]

(In reply to comment #286)
> If I understand correctly, this is required to track that an action should be
> taken after both that bug and this one are marked as fixed, right? I ask
> because this might not be necessary here, since this function is gone, but
> maybe we should do the same for bug 492960, which is now just marked as a
> dependency of this bug.
If it goes away, we don't need to worry about it, but in general, yes, it's so we can track that.

> Last time I checked, the QueryInterface call (or an "instanceof" check) was
> required in order to access the properties defined on nsIFileURL. If this
> is still the case, shouldn't "NetUtil.newURI" do the QueryInterface for us?
> Currently it doesn't, even if it would be a very simple change:
We should file a bug to automagically do that.

> As far as I know, that wouldn't work because we are running all the Toolkit
> chrome tests with a browser window open. I did my own investigation, and will
> post my proposal in a separate comment later :-)
You can get a component by uid still and not the contract, so the tests could do that.

I'm unlikely to be able to look at this before I go away for a while.

Comment 288

[:Paolo Amadini]

A new tryserver build is now ready, including a prototype of the new download
progress indicator in the main window, which is implemented in bug 663772.
Screenshots of the indicator can be found in attachment 538844 [details]
(<https://bugzilla.mozilla.org/attachment.cgi?id=538844