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.

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

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

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.

Fixed some layout problems in MS Windows

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

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!

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

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

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)

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

(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

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

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

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

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

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

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.

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

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.

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?

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.

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?

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

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?

OK. Changed reviewer

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

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

Fix for middle/right click issues reported by Wes

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/

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.

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?

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

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

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

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?

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

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.

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.

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.

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.

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.

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

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

(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

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

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

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

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

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.

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

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.

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

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

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

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

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.

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

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.

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!

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

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

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

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

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?

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

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

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.

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.

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

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 on attachment 470869 [details] [diff] [review] Feature-complete version for review (Windows theme only) So, it seems to me that we are completely breaking the toolkit UI with this patch. Is that right? We cannot do that since other applications (Thunderbird, Songbird, others probably) do depend on this UI. For review comments with expandable file context, please see http://reviews.visophyte.org/r/470869/. on file: browser/base/content/browser.xul line 1004 > <toolbarbutton id="downloads-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > oncommand="DownloadsOverlay.showPopup();" > ondrop="DownloadsButtonDNDObserver.onDrop(event)" > ondragover="DownloadsButtonDNDObserver.onDragOver(event)" > ondragenter="DownloadsButtonDNDObserver.onDragOver(event)" > ondragleave="DownloadsButtonDNDObserver.onDragLeave(event)" > label="&downloads.label;" removable="true" > tooltiptext="&downloads.tooltip;"/> nit: one property per line please. It's easy to miss some of them because there are randomly more than one on a line. on file: browser/components/downloads/DownloadCommon.jsm line 81 > /** > * True as soon as initialization begins. > */ > _initStarted: false, This is private state, so it shouldn't be on an exported symbol. on file: browser/components/downloads/DownloadCommon.jsm line 95 > // convert strings to those in the string bundle > let (sb = Services.strings.createBundle(kDownloadsStringBundleUrl)) { > for (let [name, value] in Iterator(DownloadCommonStrings)) { > if (typeof value == "string") { > DownloadCommonStrings[name] = sb.GetStringFromName(value); > } else { > // Convert "arguments" to a real array before calling into XPCOM > DownloadCommonStrings[name] = function() sb.formatStringFromName( > value[0], Array.prototype.slice.call(arguments), arguments.length); > } > } > } We should be doing this lazily, as needed (see how DownloadUtils.jsm does this). on file: browser/components/downloads/DownloadManagerUI.js line 59 > var browser = Services.wm.getMostRecentWindow("navigator:browser"); > if (browser) { > browser.DownloadsOverlay.showPopup(); > } feels wrong to not show anything if there are now browser windows open. on file: browser/components/downloads/tests/browser_basic_functionality.js line 180 > gDownloadsView.focus(); hmm, why do we need to focus? on file: browser/themes/winstripe/browser/jar.mn line 51 > skin/classic/browser/downloads/downloadButtons.png (downloads/downloadButtons.png) > skin/classic/browser/downloads/downloadIcon.png (downloads/downloadIcon.png) > skin/classic/browser/downloads/downloads.css (downloads/downloads.css) > skin/classic/browser/downloads/pause.png (downloads/pause.png) > skin/classic/browser/downloads/stop.png (downloads/stop.png) note: we'll need to have something, even if not great, for linux and os x before this can land. on file: toolkit/components/downloads/src/Makefile.in line 69 > # TODO: Disable this also when building the "browser" application. > #EXTRA_COMPONENTS = \ > # nsDownloadManagerUI.js \ > # nsDownloadManagerUI.manifest \ needs to be fixed before moving forward. You might get help from khuey on how to do this properly. We also still need to test this UI, which means we'll need to build it somehow. on file: browser/locales/en-US/chrome/browser/downloads/downloads.properties line 18 > stateBlockedPolicy=Blocked by your Security Zone Policy if you change a string, you also need to change it's name, otherwise localizers will not know it's meaning changed. on file: browser/locales/en-US/chrome/browser/downloads/downloads.properties line 19 > stateDirty=Blocked: May contain a virus or spyware needs a localization note on file: browser/components/downloads/download.xml line 29 > # Blair McBride <bmcbride@mozilla.com> er? on file: browser/components/downloads/downloads.js line 57 > Cu.import("resource://gre/modules/DownloadCommon.jsm"); this file you reference is built in browser/ so toolkit/ cannot depend on it.

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

(In reply to comment #80) > on file: browser/components/downloads/DownloadManagerUI.js line 59 > > var browser = Services.wm.getMostRecentWindow("navigator:browser"); > > if (browser) { > > browser.DownloadsOverlay.showPopup(); > > } > > feels wrong to not show anything if there are now browser windows open. That's inherent in the panel design I think... but maybe we can ensure that a new browser window is opened if the call is the result of an explicit user request, while for new downloads initiated while there is no browser window open (rare case I think) I don't think we should open a new window, there will be a screen-level notification when the download is completed in any case. I'm also interested in Limi's opinion on this. > on file: browser/themes/winstripe/browser/jar.mn line 51 > > skin/classic/browser/downloads/downloadButtons.png > note: we'll need to have something, even if not great, for linux and os x > before this can land. I think copying the folder as is will suffice for now, it's based on the new Add-ons Manager design which is not platform-specific, and refining the visual style is explicitly a non-goal for the next beta. I've not done the copy yet in case I spot any bugs or any change is needed, but I'll do the copy before the final version. > on file: toolkit/components/downloads/src/Makefile.in line 69 > > # TODO: Disable this also when building the "browser" application. > > #EXTRA_COMPONENTS = \ > > # nsDownloadManagerUI.js \ > > # nsDownloadManagerUI.manifest \ > > needs to be fixed before moving forward. You might get help from khuey on how > to do this properly. Thanks, I'll ask. > on file: browser/locales/en-US/chrome/browser/downloads/downloads.properties > line 19 > > stateDirty=Blocked: May contain a virus or spyware > > needs a localization note Why? > on file: browser/components/downloads/download.xml line 29 > > # Blair McBride <bmcbride@mozilla.com> > > er? This reflects the fact that I originally included work from the Add-ons Manager download progress XBL binding. It's heavily modified however, so I'm not sure if the credit is still required. I'll work on the other nits immediately.

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

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

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.

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

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

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

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

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

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.

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

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

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.

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

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

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

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

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

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.

So this will not be in FX4?

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.

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

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

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

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

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.

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

Ah, now I get it, thanks.

This isn't intended for 4.0?

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

I'll prey for miracle then. :)

This is not happening for Firefox 4. Sorry folks.

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

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

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

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.

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

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

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)

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

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

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

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.

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

(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

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!

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

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

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

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

:)

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

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

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

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

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.

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

Any reason why was attachment deleted?

It was the same as the patch with extra buttons.

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/

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

Shouldn't it be an ArrowPanel??

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.

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.

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

(In reply to comment #173) > The model - view - controller implementation is inspired by the Places > front-end code NOW I'm scared! (just kidding...)

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

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

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

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.

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

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

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

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

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

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

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

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

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

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!

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.

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

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

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

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

(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 on attachment 528720 [details] [diff] [review] Version with "components/downloads" ready for review Review of attachment 528720 [details] [diff] [review]: Plus all of mak's comments, this looks good. ::: browser/components/downloads/download.xml @@ +25,5 @@ +# Ben Goodger <ben@bengoodger.com> (v2.0) +# Blake Ross <blakeross@telocity.com> +# Shawn Wilsher <me@shawnwilsher.com> (v3.0) +# Edward Lee <edward.lee@engineering.uiuc.edu> +# Paolo Amadini <http://www.amadzone.org/> (v4.0) for you, yeah? ::: browser/components/downloads/downloads.js @@ +115,5 @@ + kPanelHidden: 1, + /** The panel will be shown as soon as possible. */ + kPanelShowing: 2, + /** The panel is open, though download data might still be loading. */ + kPanelShown: 3, if you actually want this to be constants, you should implement them as getters. @@ +207,5 @@ + /** + * Indicates whether the panel is shown or will be shown. + */ + get isPanelShowing() this._panelState == this.kPanelShowing || + this._panelState == this.kPanelShown, Because this spans two lines here, I'd prefer it if you didn't use the magic here and used braces and all the normal JS stuff :) @@ +339,5 @@ + * element was already in the list, it is moved to the appropriate position. + * + * @param aElement + * XUL element associated with the download item. + * global-nit: no newline here @@ +352,5 @@ + this.richListBox.insertBefore(aElement, aDataItem.done + ? this.richListBox.firstChild + : !aDataItem.inProgress + ? this.firstElementNotDone + : this.firstElementInProgress); This is all very complicated and hard to follow. I suspect you can improve this by using a temporary variable or two. You do this in a few places, so this comment applies there as well. @@ +1702,5 @@ + , nsIDM.DOWNLOAD_QUEUED + , nsIDM.DOWNLOAD_DOWNLOADING + , nsIDM.DOWNLOAD_PAUSED + , nsIDM.DOWNLOAD_SCANNING + ].indexOf(this.state) >= 0, Trailing commas are legal in arrays, and I'd rather see that here. Also, since this spans multiple lines, use braces and a return statement please. @@ +1717,5 @@ + get done() [ nsIDM.DOWNLOAD_FINISHED + , nsIDM.DOWNLOAD_BLOCKED_PARENTAL + , nsIDM.DOWNLOAD_BLOCKED_POLICY + , nsIDM.DOWNLOAD_DIRTY + ].indexOf(this.state) >= 0, same here @@ +1729,5 @@ + * Indicates whether the download stopped because of an error, and can be + * resumed manually. + */ + get canRetry() this.state == nsIDM.DOWNLOAD_CANCELED || + this.state == nsIDM.DOWNLOAD_FAILED, and here (going to stop commenting on this, but it applies everywhere) ::: browser/components/downloads/tests/browser_basic_functionality.js @@ +13,5 @@ + */ +function test() +{ + var dmFile = Services.dirsvc.get("TmpD", Ci.nsIFile); + dmFile.append("dm-ui-test.file"); I can one-up mak's suggestion: FileUtils.getFile("TmpD", ["dm-ui-test.file"]); ::: browser/components/downloads/tests/utils.js @@ +57,5 @@ + let ios = Cc["@mozilla.org/network/io-service;1"]. + getService(Ci.nsIIOService); + return (aObj instanceof Ci.nsIFile) ? ios.newFileURI(aObj) : + ios.newURI(aObj, null, null); + } As mak suspected, this whole method can be nuked and you can just use NetUtil.newURI everywhere. NetUtil is already pulled into browser.js too I think, so you don't even have to import it. ::: browser/components/nsBrowserGlue.js @@ +1039,5 @@ box.persistence = -1; // Until user closes it }, _migrateUI: function BG__migrateUI() { + const UI_VERSION = 6; Amusing that this might coincide with the browser version...

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

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.