Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement the Downloads Panel

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Downloads Panel
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 6 bugs, {addon-compat})

Trunk
Firefox 20
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [=== See the STATUS PAGE before posting - linked in the bug's URL field ===], URL)

Attachments

(3 attachments, 13 obsolete attachments)

5.82 KB, text/plain
Details
100 bytes, text/plain
Details
253.89 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Initial version of the Downloads Panel, to be included in Nightly builds for
wider testing, enabled by default. The feature is ready to be disabled in the
Aurora channel until all the release criteria has been met.

This does not include the changes to the Downloads folder in the Library window.
(Assignee)

Comment 1

6 years ago
Created attachment 596494 [details] [diff] [review]
Coalesced patch

This initial patch includes all the changes from bug 564934, bug 663772,
bug 685357, bug 708566, bug 697686, bug 697688, bug 697683, bug 697680,
bug 697678, and Javi Rueda's patch in bug 697679.
(Assignee)

Updated

6 years ago
Blocks: 726445
(Assignee)

Updated

6 years ago
Blocks: 726447
(Assignee)

Updated

6 years ago
Blocks: 726450
(Assignee)

Updated

6 years ago
Blocks: 726451
(Assignee)

Updated

6 years ago
Blocks: 726453
(Assignee)

Updated

6 years ago
Blocks: 726454
(Assignee)

Updated

6 years ago
Blocks: 726456
(Assignee)

Updated

6 years ago
Duplicate of this bug: 663772
(Assignee)

Updated

6 years ago
Duplicate of this bug: 685357
(Assignee)

Updated

6 years ago
Duplicate of this bug: 708566
(Assignee)

Updated

6 years ago
Duplicate of this bug: 697686
(Assignee)

Updated

6 years ago
Duplicate of this bug: 697688
(Assignee)

Updated

6 years ago
Duplicate of this bug: 697683
(Assignee)

Updated

6 years ago
Duplicate of this bug: 697680
(Assignee)

Updated

6 years ago
Duplicate of this bug: 697678
(Assignee)

Updated

6 years ago
Duplicate of this bug: 697679
(Assignee)

Comment 11

6 years ago
Created attachment 596501 [details]
Current Downloads Panel design

This is the current design from the Downloads Panel feature page. Can you check
if it matches what we want to do in the end when the panel is released?
Attachment #596501 - Flags: ui-review?(ux-review)
(Assignee)

Comment 12

6 years ago
Created attachment 596503 [details]
Current tryserver build

Different question :-) Here you can find the latest tryserver build, including
the latest developments and bug fixes:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/paolo.mozmail%40amadzone.org-76ac14c73097/

Our goal is to land this on Nightly _enabled_, for wider testing, and disable
the feature on Aurora until it is ready for release. Do you think that this
trybuild is ready to land enabled? If not, what is missing exactly?

Note that we don't need a list of issues we should fix for release, just for
making the feature available to Nightly users. Lists of known issues are
already filed as follow-ups of this bug. See in particular bug 726447 for
issues related to theming and visual design. We know that many (maybe all)
of them do not need to be fixed for Nightly users.
Attachment #596503 - Flags: ui-review?(ux-review)
Comment on attachment 596494 [details] [diff] [review]
Coalesced patch

>+      <toolbarbutton id="downloads-button"
>+                     class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                     label="&downloads.label;"
>+                     tooltiptext="&downloads.tooltip;"
>+                     removable="true"/>

There's no feedback upon starting a download with the button removed. What should happen instead seems pretty to me: The button should appear transiently and disappear again when starting up without active downloads.
(In reply to Dão Gottwald [:dao] from comment #13)
> There's no feedback upon starting a download with the button removed. What
> should happen instead seems pretty to me: The button should appear
> transiently and disappear again when starting up without active downloads.

While it's an important case to handle, I don't think should block Nightly testing, rather it should block Aurora enable-by-default.  We should file a tracking bug for what blocks enable-by-default on Aurora and add dependencies on it.
Given my seemingly unheard preaching on related issues*, I'd rather see it addressed upfront.

*: I haven't yet seen an explanation why the behavior described in comment 13 isn't the /default/ behavior.
(Assignee)

Comment 16

6 years ago
This bug is about landing enabled in Nightly only, we shouldn't block on other
improvements here, even if they're very important for release (like accessibility).

There is a pending UX review for determining the Nightly landing criteria for
the feature behavior. This is explicitly a responsibility of the UX team. If
there are any suggestions for them, they should probably be discussed in the
dev.usability forum.
It's a responsibility from peers as well. I can make this explicit if you want: you're adding a removable button without reasonably handling the case if the button being removed. This is r- worthy and therefore belongs in this bug, not newsgroups. "Nightly only" doesn't matter too much. There's only one mozilla-central and stuff wanting to land there needs to meet certain criteria.

By the way, what exactly is the plan for disabling on Aurora? Flipping a pref won't be enough, given the permanent modifications to the navigation toolbar.
"the case if the button being removed" => "the case of the button being removed"
Comment on attachment 596501 [details]
Current Downloads Panel design

I'm OK with landing the current version on Nightly. And yes, it has visual bugs. :)
Attachment #596501 - Flags: ui-review?(ux-review) → ui-review+
Comment on attachment 596503 [details]
Current tryserver build

I'm OK with landing the current version on Nightly. And yes, it has visual bugs. :)
Attachment #596503 - Flags: ui-review?(ux-review) → ui-review+
(In reply to Dão Gottwald [:dao] from comment #17)
> you're adding a removable button without reasonably handling the case
> if the button being removed. 

Limi: What is the expected behavior for when a user customizes-away the downloads button?
(Assignee)

Comment 22

6 years ago
(In reply to Dão Gottwald [:dao] from comment #17)
> By the way, what exactly is the plan for disabling on Aurora? Flipping a
> pref won't be enough, given the permanent modifications to the navigation
> toolbar.

That's a good point. In fact I think that, for testing in Nightly, we will go for
a solution that does not require permanent modifications to the navigation bar,
with no migration code involved. Just flipping a preference should revert Nightly
users to the previous state, as if the feature never existed.

This is a bit more work now, but gives us maximum flexibility when figuring out
the final behavior, position, etc. for the button without limiting our choices
or creating more work for reverting later.
(Assignee)

Comment 23

6 years ago
Created attachment 597771 [details] [diff] [review]
No migration code and other minor changes

This patch implements the "no migration code" approach.

- The button is shown in the navigation bar by default.
- For now you cannot customize the button away from the navigation bar, and the
  only way to remove it is to disable the entire feature. For this reason, the
  button is currently hidden when there are no downloads.
- If you placed the downloads button on a toolbar, we show the indicator there,
  always visible. Note that if you test this build in the same profile where
  you tested the previous ones, you already have the button there, so you have
  to remove the placeholder from the toolbar again for the button to disappear
  when there are no downloads (or test on a clean profile).

We are free to improve on this design without affecting existing profiles.

To disable feature: flip "browser.download.manager.useWindowUI" and restart.

Tryserver build: https://tbpl.mozilla.org/?tree=Try&rev=fed844f653d2
Attachment #596494 - Attachment is obsolete: true
Attachment #597771 - Flags: review?(mak77)
(Assignee)

Comment 24

6 years ago
Limi, are you fine with Nightly users testing the design in comment 23, thus
with the behavior (on a clean profile) of the tryserver build that will soon
be available here?

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/paolo%2Emozmail%40amadzone%2Eorg-fed844f653d2
Comment on attachment 597771 [details] [diff] [review]
No migration code and other minor changes

>--- /dev/null
>+++ b/browser/themes/winstripe/downloads/downloads-aero.css

some of the following comments apply to other files as well

>+#downloadsPanel .panel-inner-arrowcontent {

use the child selector

>+@media all and (-moz-windows-default-theme) {

remove "all and"

>+  richlistitem[type="download"][state="1"] .downloadInfo {
>+    border: 1px solid transparent;
>+    -moz-padding-end: 8px;
>+  }
>+
>+  richlistitem[type="download"][state="1"] .downloadInfo:hover {
>+    border: 1px solid hsl(213,45%,65%);
>+    box-shadow: 0 0 0 1px hsla(0,0%,100%,.5) inset,
>+                0 1px 0 hsla(0,0%,100%,.3) inset;
>+    background-image: -moz-linear-gradient(hsl(212,86%,92%), hsl(212,91%,86%));
>+    color: black;
>+  }

child selector

>+@media not all and (-moz-windows-classic) {
>+  richlistitem[type="download"][state="1"]:hover .downloadButton {

child selector

>+  @media all and (-moz-windows-classic) {

this doesn't seem to make sense. you're still inside "@media not all and (-moz-windows-classic)".

>+    richlistitem[type="download"][state="1"] .downloadInfo:hover .downloadDetails {

child selector

>+@media all and (-moz-windows-compositor) {

remove "all and"

>+++ b/browser/themes/winstripe/downloads/downloads.css

>+@media not all and (-moz-windows-classic) {
>+  #downloadsHistory {
>+    background-color: hsla(216,45%,88%,.98);
>+    box-shadow: 0px 1px 2px rgb(204,214,234) inset;  
>+  }
>+}

I'm doubtful that this is the right color for all Windows versions and themes except classic.
While some css comments can be fixed now (thanks for any feedback on any simple fixes we can do immediately), I think we should be fine with fixing theme glitches live in Nightly.  Obviously we woulnd't ship it in Aurora unless it reaches our quality levels.
As you know, there may be many small details in themes and would be an infinite work to fix all of them in a 260KB patch.  Would you be fine with handling theme details in follow-ups so we can work on small cleanup patches?
(In reply to Marco Bonardo [:mak] from comment #26)
> While some css comments can be fixed now (thanks for any feedback on any
> simple fixes we can do immediately), I think we should be fine with fixing
> theme glitches live in Nightly.  Obviously we woulnd't ship it in Aurora
> unless it reaches our quality levels.

What ensures this? What's the downside of addressing review comments before landing stuff? It happens too often that followup bugs are filed but never taken care of.
We should address comments before landing, we should not hang trying to reach perfection in every single detail though, like getting the right color or deciding a font should be 1px larger.
Once we have followups we can prioritize them as blocking or not, and blocking ones MUST be fixed before the feature can be enabled on Aurora. What we gain is:
1. more testing
2. more eyes on the code
3. more developers able to contribute fixes, rather than a single one
4. small incremental patches, easier to review and not bitrotting every other day
I wasn't lamenting over color shades or font sizes. I pointed out code that looks bogus to me. Code identified as being bogus should simply be fixed before it lands unless there's a good reason why that's not practical in a particular case. "I want to land this as soon as possible" isn't such a reason.

Prioritizing known bugs post-landing is what my previous question aimed at: How does it ensure things get addressed? Experience shows that bugs classified as non-blocking likely just won't be taken care of. This is a recipe for eroding code quality and one central reason why we require code review.
(In reply to Dão Gottwald [:dao] from comment #29)
> Experience shows that bugs
> classified as non-blocking likely just won't be taken care of.

Bugs classified as non blocking are non blocking, otherwise would be classified differently. If the classification was wrong that's another story.
And I say review comments need to be addressed regardless of some blocking classification.

Comment 32

6 years ago
I see this over and over in bug reports, "It would help if you provided a patch". Going with that philosophy Dao, you are, "... doubtful that this is the right color for all Windows versions and themes except classic." 

Are you doubtful or are you certain? If you are certain, suggest the right color for all Windows versions and themes except classic so that the change can be made as per your request.

We've been waiting for the new download manager since Limi first proposed the design in April 2011. Let's figure out what needs to be done to remove obstacles in the path of landing the feature that has been mimicked by Safari for about 8 months now.
I can't suggest the right color, because the part that looks wrong is "@media not all and (-moz-windows-classic)". There's very little that the themes targeted there have in common.

Comment 34

6 years ago
There are a number of theme developers on the mozillazine forum commenting on daily builds. Without this feature landing in Nightly, there is no way to solicit their feedback on the impact of the changes to their theming efforts.

If getting theme impact is material to fixing this bug, I suggest posting a try build to the forum and gauging the response. The community is eager to test and comment on this new feature.

Comment 35

6 years ago
Its now in latest UX builds : http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ux-win32/

I made a video preview of it http://www.youtube.com/watch?v=eon_jVq2GOs
(Assignee)

Comment 36

6 years ago
(In reply to bogas04 from comment #35)
> I made a video preview of it http://www.youtube.com/watch?v=eon_jVq2GOs

Thank you! This is a very effective overview.

I noticed you had the downloads button placed on the toolbar, since you probably
tested on the same profile where you also tried out a previous iteration.

In the current version, the button is not placed on the navigation bar
automatically, but still appears there if there are any recent downloads.
It also disappears when you remove all the download items from the list.
To see this new default behavior, you can just remove the button from any
toolbar during the customization.

We don't know yet what the exact behavior will be in the final version.

With regard to availability, it's very probable that the feature will be present
but still *disabled* by default in the Aurora channel for Firefox 13. Might be
worth mentioning in the video's description :-)
If possible please keep the discussions in bug 564934, this is an implementation bug.
Whiteboard: [please discuss the feature in bug 564934, not here]
Created attachment 601123 [details]
review of attachment 597771 [details] [diff] [review]

globally it's still good enough to land.
After comments are addressed, there is still a strong requirement before landing, that is to do a couple try runs without and with the patch and compare-talos them to check for eventual regressions.
If you have concerns on the comments feel free to ping me on IRC, or drop me a mail.
Comment on attachment 597771 [details] [diff] [review]
No migration code and other minor changes

review attached, will flag when requirements from previous comment are satisfied
Attachment #597771 - Flags: review?(mak77)
As a side note, if you want to meet up in person, to do final checks and fixes, we can adjust that.
(Assignee)

Updated

6 years ago
Blocks: 732924
(Assignee)

Comment 41

6 years ago
Created attachment 602855 [details] [diff] [review]
The patch

Tryserver builds for compare-talos just started:

https://tbpl.mozilla.org/?tree=Try&rev=ffb696a80bc7
https://tbpl.mozilla.org/?tree=Try&rev=a988a8a0e290
Attachment #597771 - Attachment is obsolete: true
Attachment #601123 - Attachment is obsolete: true
Attachment #602855 - Flags: review?(mak77)
I have retriggered all Talos, so we can account for noise.
Looks like there is a new failure on Mac in browser_delete_key_removes.js.
(Assignee)

Comment 43

5 years ago
Created attachment 603207 [details] [diff] [review]
Reverted one change in the panel opening sequence

(In reply to Marco Bonardo [:mak] from comment #42)
> I have retriggered all Talos, so we can account for noise.

I'm still not sure how to interpret the values I see in compare-talos.

> Looks like there is a new failure on Mac in browser_delete_key_removes.js.

I've already run a separate tryserver build to pinpoint the problem. It's
confirmed to be caused by one of the changes made to the panel focus functions,
though I can't directly test on Mac so I can't understand the issue exactly.
Probably, on Mac, selectedIndex should be set before moving focus to the list.
Since I don't have enough information to file a bug or add a specific comment,
I've simply reverted the change. The regression test will ensure that the code
continues to work as expected.

This change doesn't affect Talos, so there's no need to do another tryserver
build for all platforms.
Attachment #602855 - Attachment is obsolete: true
Attachment #602855 - Flags: review?(mak77)
Attachment #603207 - Flags: review?(mak77)
(In reply to Paolo Amadini from comment #43)
> I'm still not sure how to interpret the values I see in compare-talos.

Show details here, I removed the tests with uninteresting results.
http://tinyurl.com/73ar4ql
Looks like there is an about 10ms hit on Win and Mac (Lion excluded) in Ts paint and Tpaint. We should recheck all the init and see what we can do there.
Comment on attachment 603207 [details] [diff] [review]
Reverted one change in the panel opening sequence

>--- /dev/null
>+++ b/browser/components/downloads/content/downloads.js

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");

Those are already in utilityOverlay.js.

>+XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>+                                  "resource://gre/modules/NetUtil.jsm");

This also seems to be in the global browser window scope already, although I don't know where it's coming from. Anyway, general-purpose modules shouldn't be loaded in overlays.

>+const nsIDM = Ci.nsIDownloadManager;

Please don't put it internal stuff like this in the global scope that you share with other scripts.
(Assignee)

Comment 46

5 years ago
Created attachment 603390 [details] [diff] [review]
Removed constants and general-purpose modules from the global scope
Attachment #603207 - Attachment is obsolete: true
Attachment #603207 - Flags: review?(mak77)
(Assignee)

Comment 47

5 years ago
To help in understanding the cause of the startup performance reduction, I've
started a new series of Talos tests in these conditions:

https://tbpl.mozilla.org/?tree=Try&rev=ea16e55764b7
-- Removed downloadsOverlay.xul entirely.

https://tbpl.mozilla.org/?tree=Try&rev=f4f897f9157e
-- Kept the overlay but removed the two calls on startup.

https://tbpl.mozilla.org/?tree=Try&rev=0c6c69f31fa4
-- Removed the synchronous startup call, kept the delayed call.

https://tbpl.mozilla.org/?tree=Try&rev=23df0304d7be
-- Kept the synchronous startup call, removed the delayed call.

https://tbpl.mozilla.org/?tree=Try&rev=fb7f94c85413
-- Kept everything as in the Talos run from a few days ago. The only change is
   that we don't re-import two general-purpose JavaScript modules.
(In reply to Paolo Amadini from comment #47)
> https://tbpl.mozilla.org/?tree=Try&rev=ea16e55764b7
> -- Removed downloadsOverlay.xul entirely.

this is the only one that shows no regression, not so surprisingly.

The modules import removal seems to have helped a little bit, 1 or 2ms. Though it still shows a 4-6ms hit.
(Assignee)

Comment 49

5 years ago
We've started two more tests:

https://tbpl.mozilla.org/?tree=Try&rev=7f3a403d0381
-- Kept everything except the calls in the global scope of the scripts.

https://tbpl.mozilla.org/?tree=Try&rev=7f017ea4d29b
-- Kept the scripts but removed the overlay and the two calls on startup.
(Assignee)

Comment 50

5 years ago
Created attachment 604357 [details] [diff] [review]
Removed a few calls in the global scope and fixed use of system colors
Attachment #603390 - Attachment is obsolete: true
Attachment #604357 - Flags: review?(mak77)
(Assignee)

Comment 51

5 years ago
Created attachment 604358 [details] [diff] [review]
Lazily load the panel overlay.

This should give a measurable startup performance gain.
Attachment #604358 - Flags: review?(mak77)
Comment on attachment 604358 [details] [diff] [review]
Lazily load the panel overlay.

This will trigger bug 640158.

(Using overlays was probably the wrong design choice, since you don't want to plug this into anything but browser.xul. If you're doing it for code separation, the parts could be put together at build time rather than run time.)
Attachment #604358 - Flags: review?(mak77) → review-
(Assignee)

Comment 53

5 years ago
(In reply to Dão Gottwald [:dao] from comment #52)
> This will trigger bug 640158.

Thanks! I see that there is a workaround for this problem, if the performance
gain of delaying the creation of the panel element is significant.

> (Using overlays was probably the wrong design choice, since you don't want
> to plug this into anything but browser.xul. If you're doing it for code
> separation, the parts could be put together at build time rather than run
> time.)

This is something I was also considering. I will do a tryserver build using an
"#include" for the elements directly in browser.xul. If the performance gain is
comparable to lazy loading, it is definitely a better solution.
(Assignee)

Comment 54

5 years ago
Created attachment 604361 [details] [diff] [review]
Don't use overlays.
So, along the last hours we examined a bunch of Talos results and we identified two main issues:

1. There is some minor work at shutdown that we can avoid in most cases. Paolo will update the main patch for that tomorrow.

2. There is a small regression on window opening.  We fixed most of the low hanging fruits for that (all global functions and modules have been removed).  There is still one clear cause that is the overlay.  As suggested by Dão the main overlay will be included at build time, though after measuring this, we found there is still a small regression in Ts and Tpaint (http://tinyurl.com/83396wl). So the plan is still to do lazy loading of just the panel overlay, obviously workarounding bug 640158, as we did for the star panel. The measurement shows this is indeed effective (http://tinyurl.com/73425q3).

Any doubts or concerns before we do this?
(Assignee)

Comment 56

5 years ago
Created attachment 604615 [details] [diff] [review]
Improved startup and shutdown performance

A new tryserver build with Talos tests enabled is running here:
https://tbpl.mozilla.org/?tree=Try&rev=04495213dc7e

The build above is based on the same base changeset as the previous tests:
https://tbpl.mozilla.org/?tree=Try&rev=f1278af7662f

A unit-test only build on a more recent changeset is running here:
https://tbpl.mozilla.org/?tree=Try&rev=c0f2c2e99f9f
Attachment #604357 - Attachment is obsolete: true
Attachment #604358 - Attachment is obsolete: true
Attachment #604361 - Attachment is obsolete: true
Attachment #604357 - Flags: review?(mak77)
Attachment #604615 - Flags: review?(mak77)
Comment on attachment 604615 [details] [diff] [review]
Improved startup and shutdown performance

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

::: browser/components/downloads/content/downloads.js
@@ +115,5 @@
> +  initialize: function DP_initialize(aCallback)
> +  {
> +    if (this._panelState != this.kPanelUninitialized) {
> +      this._ensureOverlayLoaded(aCallback);
> +      return;

kPanelUninitialized is the initial value, in such a case we know the overlay has to be loaded, and indeed we proceed, in all the other cases couldn't we just aCallback() since we know the overlay loading has already been started?
Latest patch talos results: http://tinyurl.com/7db9334
Comment on attachment 604615 [details] [diff] [review]
Improved startup and shutdown performance

Discusses on irc.  The numbers look good, there is still some minor unclear result, but considered the 16ms resolution of Windows timers, we may end figthing ghosts for days or weeks.  Not sure there's anything more we could do right now.
So I suggest proceeding with the landing and see real numbers in the tree, a backout in any case is easy and won't leave any traces.
Attachment #604615 - Flags: review?(mak77) → review+
(Assignee)

Comment 60

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7da6256b23

Note: this feature will be disabled by default in Aurora.
Target Milestone: --- → Firefox 13
(Assignee)

Comment 61

5 years ago
As said, the plan is to disable this feature using a preference in Firefox 13,
however it has also been suggested to keep it enabled for one week, like it was
done for the New Tab page.
tracking-firefox13: --- → ?

Comment 62

5 years ago
w00t! \o/
tracking-firefox13: ? → ---
Depends on: 734701
Depends on: 734702
Was the tracking-firefox13 flag intentionally cleared? The post that set it and the post that cleared it were only a minute apart.
tracking-firefox13: --- → ?
No longer depends on: 734702
There are numerous regression reports on dev.tree-management for this. Mak, happy to let you decide best course of action, but they are across multiple tests and platforms, so seem a lot more real than suspected in comment 59.

Regression :( Tp5r MozAfterPaint (Private Bytes) increase 3.98% on Linux Mozilla-Inbound-Non-PGO
------------------------------------------------------------------------------------------------
    Previous: avg 377455366.667 stddev 2673427.601 of 30 runs up to revision b74bbbfafde3
    New     : avg 392469200.000 stddev 837328.908 of 5 runs since revision ba7da6256b23
    Change  : +15013833.333 (3.98% / z=5.616)
    Graph   : http://mzl.la/A8uYbx 

Regression :( Trace Malloc MaxHeap increase 1.18% on CentOS release 5 (Final) Mozilla-Inbound
---------------------------------------------------------------------------------------------
    Previous: avg 26491933.333 stddev 79171.526 of 30 runs up to revision b74bbbfafde3
    New     : avg 26804620.000 stddev 21195.448 of 5 runs since revision ba7da6256b23
    Change  : +312686.667 (1.18% / z=3.949)
    Graph   : http://mzl.la/wlp1ZK 

Regression :( Paint increase 9.68% on Win7 Mozilla-Inbound-Non-PGO
------------------------------------------------------------------
    Previous: avg 184.530 stddev 3.704 of 30 runs up to revision b74bbbfafde3
    New     : avg 202.400 stddev 3.270 of 5 runs since revision ba7da6256b23
    Change  : +17.870 (9.68% / z=4.825)
    Graph   : http://mzl.la/yX22u3 

Regression :( Trace Malloc Allocs increase 1.91% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
-----------------------------------------------------------------------------------------------------
    Previous: avg 449646.600 stddev 749.481 of 30 runs up to revision b74bbbfafde3
    New     : avg 458254.800 stddev 308.326 of 5 runs since revision ba7da6256b23
    Change  : +8608.200 (1.91% / z=11.486)
    Graph   : http://mzl.la/yIYdKc 

Regression :( Trace Malloc MaxHeap increase 1.66% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
------------------------------------------------------------------------------------------------------
    Previous: avg 36321716.667 stddev 5774.085 of 30 runs up to revision b74bbbfafde3
    New     : avg 36924800.000 stddev 9138.928 of 5 runs since revision ba7da6256b23
    Change  : +603083.333 (1.66% / z=104.447)
    Graph   : http://mzl.la/ynu1ix  

Regression :( Trace Malloc MaxHeap increase 1.73% on WINNT 5.2 Mozilla-Inbound
------------------------------------------------------------------------------
    Previous: avg 25351820.000 stddev 64804.211 of 30 runs up to revision b74bbbfafde3
    New     : avg 25790620.000 stddev 3439.041 of 5 runs since revision ba7da6256b23
    Change  : +438800.000 (1.73% / z=6.771)
    Graph   : http://mzl.la/ysjjdf 

Regression :( Tp5r MozAfterPaint (Private Bytes) increase 3.63% on Linux x64 Mozilla-Inbound-Non-PGO
----------------------------------------------------------------------------------------------------
    Previous: avg 422754200.000 stddev 2998802.490 of 30 runs up to revision b74bbbfafde3
    New     : avg 438108800.000 stddev 2696260.781 of 5 runs since revision ba7da6256b23
    Change  : +15354600.000 (3.63% / z=5.120)
    Graph   : http://mzl.la/A7ysNU 

Regression :( Paint increase 5.82% on Win7 Mozilla-Inbound
----------------------------------------------------------
    Previous: avg 155.514 stddev 1.319 of 30 runs up to revision b74bbbfafde3
    New     : avg 164.569 stddev 1.817 of 5 runs since revision c29f828d1ce4
    Change  : +9.054 (5.82% / z=6.864)
    Graph   : http://mzl.la/x4PUTX 

Regression :( Tp5r MozAfterPaint (Private Bytes) increase 3.48% on Linux x64 Mozilla-Inbound
--------------------------------------------------------------------------------------------
    Previous: avg 424051766.667 stddev 2622374.795 of 30 runs up to revision ad71757b3229
    New     : avg 438804000.000 stddev 1400962.883 of 5 runs since revision ba7da6256b23
    Change  : +14752233.333 (3.48% / z=5.626)
    Graph   : http://mzl.la/wHkZr6
no way out from a plain backout for now. If anyone could do that would be appreciated, since I will be out tomorrow in the morning.

Actually the only real worrying thing is Paint increase 9.68% on Win7 Mozilla-Inbound-Non-PGO.  A bit more memory may be expected, and a small regression in Paint could exist (after all we are adding something to all the browser windows), but we should understand why Win7 is affected so much.
(In reply to Marco Bonardo [:mak] from comment #65)
> no way out from a plain backout for now. If anyone could do that would be
> appreciated, since I will be out tomorrow in the morning.

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a006794b94

Thanks mak :-)
Just for the record, here are the m-c csets for the landing & backout (both merged to m-c):
  https://hg.mozilla.org/mozilla-central/rev/ba7da6256b23
  https://hg.mozilla.org/mozilla-central/rev/f0a006794b94
(Assignee)

Comment 68

5 years ago
We worked in the past few days to track the performance issues, and we tried a
few more tests, but we couldn't get convincing results. In general we noticed
differences and anomalies that we couldn't explain clearly, that were often
platform-specific, and accounting for measurement noise on try builds was also
difficult.

So we've decided to change approach and we've booked a dedicated branch (cedar)
with continuous builds and tests, on which we'll re-create the feature
incrementally and see more clearly how the Talos tests evolve over time.
tracking-firefox13: ? → ---

Updated

5 years ago
Depends on: 736172
In the latest UX builds the download button shows nothing on hover.
In the latest UX builds, there is an error in the css applied to the selected richlist item . In hover state, the hovered item has a kind of blue gradient background and black text. But the selected item has white text and no gradient background (i.e. white background) Thus nothing is visible until we hover it.
This is confirmed that the selected item has this problem by pressing the arrow keys and watching the text of the corresponding item getting white.

One more suggestion, on hovering/selecting the gradient is not covering the whole horizontal space, but leaving the search kinda icon with white background. This looks little inconsistent and ugly. IMHO, the gradient should cover the whole horizontal space.
Even the height of each item can be reduced by 10px. Its too wide.
(Assignee)

Comment 71

5 years ago
(In reply to Girish Sharma from comment #70)
> In the latest UX builds, there is an error in the css applied to the
> selected richlist item . In hover state, the hovered item has a kind of blue
> gradient background and black text. But the selected item has white text and
> no gradient background (i.e. white background) Thus nothing is visible until
> we hover it.
> This is confirmed that the selected item has this problem by pressing the
> arrow keys and watching the text of the corresponding item getting white.

We might have fixed this while working on recent improvements, I'll check out
the status of the UX build in the next few days.

> One more suggestion, on hovering/selecting the gradient is not covering the
> whole horizontal space, but leaving the search kinda icon with white
> background. This looks little inconsistent and ugly. IMHO, the gradient
> should cover the whole horizontal space.

This is part of the UX design we'll address once the first version is available.

> Even the height of each item can be reduced by 10px. Its too wide.

This also probably changed already.
(Assignee)

Comment 72

5 years ago
Created attachment 608980 [details] [diff] [review]
Other performance and styling changes
Attachment #604615 - Attachment is obsolete: true
I've got two big bugs on latest UX build :
-The first download in the list only shows icon and progress bar.
-Arrow is not considered as a button.

Comment 74

5 years ago
third
-If a download is in progress , on closing firefox , there is no notification indicating about them.
(In reply to bogas04 from comment #74)
> third
> -If a download is in progress , on closing firefox , there is no
> notification indicating about them.

Not a bug. But it's a very good thing to implement.
In your reply [1] , you said that the height of each download item has been reduced (as there was too much of padding) and also when hovering the item, the full width gets highlighted . But none of those change are visible in the latest build.

Also, the downloads panel's toolbar button (the down arrow button) should be of white color on glass, like all other buttons are.

Comment 77

5 years ago
(In reply to Guillaume C. [:GE3K0S] from comment #75)
> (In reply to bogas04 from comment #74)
> > third
> > -If a download is in progress , on closing firefox , there is no
> > notification indicating about them.
> 
> Not a bug. But it's a very good thing to implement.

If you or someone else files a new bug# for that enhancement please post it's bug# here, Thanks.
(In reply to rob64rock from comment #77)
> (In reply to Guillaume C. [:GE3K0S] from comment #75)
> > (In reply to bogas04 from comment #74)
> > > third
> > > -If a download is in progress , on closing firefox , there is no
> > > notification indicating about them.
> > 
> > Not a bug. But it's a very good thing to implement.
> 
> If you or someone else files a new bug# for that enhancement please post
> it's bug# here, Thanks.

I think UX team and Paolo should seriously take this into consideration. The dialog should also have the option "don't show this dialog anymore".

I've also checked download arrow is the only button that doesn't act like one. Maybe it's because of the recently resolved bug about borderless button.

Comment 79

5 years ago
(In reply to rob64rock from comment #77)
> (In reply to Guillaume C. [:GE3K0S] from comment #75)
> > (In reply to bogas04 from comment #74)
> > > third
> > > -If a download is in progress , on closing firefox , there is no
> > > notification indicating about them.
> > 
> > Not a bug. But it's a very good thing to implement.
> 
> If you or someone else files a new bug# for that enhancement please post
> it's bug# here, Thanks.

I have filed Bug 741312 for it.
In the current UX build, if a download fails, the context menu contains an entry to cancel the download instead of one for removing it. The only way to remove it is to clear all the list.
(Assignee)

Comment 81

5 years ago
Thank you to everyone that provided feedback and reported bugs! I will look into
them next week, now that the work on startup performance is almost finished.

Hopefully, we will be able to land the feature in Nightly in a short time. Even
if not all of the current issues will be necessarily fixed at that time, once the
feature lands we will be able to fix the issues incrementally, and more rapidly.

Similarly to what we planned for Firefox 13, even if the feature will land in
Nightly builds for Firefox 14, it be disabled by default in the Aurora channel.

When the feature lands, I will provide a more thorough status update, detailing
what to expect from the current state of the feature.
(Assignee)

Comment 82

5 years ago
Created attachment 614439 [details] [diff] [review]
Nearly optimal startup and shutdown performance

This version should provide near-zero startup and shutdown performance impact in
case there are no downloads in the panel, which is the most common case now, and
at all times when the feature is disabled.

A very little impact is expected for the code we cannot load later, like
setting up startup observers (that however don't do anything unless required).

Ideally we should share the delay load code with the bookmarks panel, but we
can do that in a follow-up.
Attachment #608980 - Attachment is obsolete: true
Attachment #614439 - Flags: review?(mak77)
Good news. However I think theming and visual design issues (bug 726447 ) are the most important things to fix before landing.
(Assignee)

Comment 84

5 years ago
I've verified the issue with the download indicator styling in the toolbars on
Windows. The indicator does not get any "hover" or "open" state when the toolbar
is in large icons mode, but works correctly in small icons mode.

This is probably due to the recent Australis design changes tracked in bug 734373
(maybe the borderless buttons implemented in bug 735691). It could be related to
bug 738304, affecting non-default buttons, though this should be investigated.

It's possible that when those bugs are fixed the indicator will be fixed as well.

To limit the possible regressions when the feature is disabled, I've made sure
that, in that case, we show the current downloads button (which is styled
correctly) instead of the downloads indicator in neutral state. This also fixes
an issue for which we would have needed an update of all the custom themes to
implement the stylesheets for the indicator, even with the feature disabled.

As long as the feature in disabled state doesn't regress, we are fine with making
the feature available in Nightly, even if there are known cosmetic issues when it
is enabled.
(Assignee)

Comment 85

5 years ago
Created attachment 615371 [details] [diff] [review]
Use the current Downloads button when the feature is disabled

Another update of the patch. I forgot to mention that in the last patch I also
removed the test requiring focusing the panel, because on the latest trunk it
failed intermittently on my machine (even with the changes inherited from the
UX branch). We can add it back in a follow-up when we figure out more precisely
how the panel and list box focus work.
Attachment #614439 - Attachment is obsolete: true
Attachment #614439 - Flags: review?(mak77)
Attachment #615371 - Flags: review?(mak77)
(In reply to Paolo Amadini [:paolo] from comment #84)
> It could be related to bug 738304, affecting non-default buttons,

Not really, no. The styling is broken because your button's internal structure is unlike the structure of standard toolbar buttons.
(Assignee)

Comment 87

5 years ago
(In reply to Dão Gottwald [:dao] from comment #86)
> (In reply to Paolo Amadini [:paolo] from comment #84)
> > It could be related to bug 738304, affecting non-default buttons,
> 
> Not really, no. The styling is broken because your button's internal
> structure is unlike the structure of standard toolbar buttons.

Thank you very much for the indication! So, I verified that the styling issue can
be solved nicely by assigning the "toolbarbutton-icon" class to the dynamic
element representing the button's contents. No need to change the browser's CSS.
Comment on attachment 615371 [details] [diff] [review]
Use the current Downloads button when the feature is disabled

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

the load overlay object is a bit confusing with that re-enqueing, but not a blocking thing.

::: browser/base/content/browser.js
@@ +1641,5 @@
>      }
>  #endif
>    }, 10000);
>  
> +  // The object hanlding the downloads indicator is also initialized here in the

typo: hanlding

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

actually, in a follow-up to avoid creating possible regressions, you may just track uninitialized and use panel.state to track other states (probably can figure out an alternative way to track uninitialized with some other internal tracker)

@@ +85,5 @@
> +  initialize: function DP_initialize(aCallback)
> +  {
> +    if (this._panelState != this.kPanelUninitialized) {
> +      DownloadsOverlayLoader.ensureOverlayLoaded(
> +                             this.kDownloadsOverlay, aCallback);

nit: reindent (you can just indent aCallback)
add a comment on why we still need to go through ensureOverlayLoaded

@@ +100,5 @@
> +
> +    // Now that data loading has eventually started, load the required XUL
> +    // elements and initialize our views.
> +    DownloadsOverlayLoader.ensureOverlayLoaded(
> +                           this.kDownloadsOverlay, function DP_I_callback() {

nit: reindent (just function on new line)
Attachment #615371 - Flags: review?(mak77) → review+
(Assignee)

Comment 89

5 years ago
Created attachment 615665 [details] [diff] [review]
Finally!

We're ready to land this version on the mozilla-incoming branch.
Attachment #615371 - Attachment is obsolete: true
(Assignee)

Comment 90

5 years ago
Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/913f7811c068
(Assignee)

Comment 91

5 years ago
Again, the plan is to disable this feature for Firefox 14 in the Aurora channel,
by enabling the preference named "browser.download.useToolkitUI".

We might also keep it enabled for one week on the Aurora channel to get more
early feedback, like it was done for the New Tab page.
tracking-firefox14: --- → ?
(Assignee)

Updated

5 years ago
Target Milestone: Firefox 13 → Firefox 14
This regressed Talos tests such as Tp5 MozAfterPaint and a bunch of others by 3-11%.  I think we should back this out for now and investigate the performance regressions.
(In reply to Ehsan Akhgari [:ehsan] from comment #92)
> This regressed Talos tests such as Tp5 MozAfterPaint and a bunch of others
> by 3-11%.  I think we should back this out for now and investigate the
> performance regressions.

Where do you see a 11% tp5 regression?  I only see a couple private bytes regressions (max 3%, and only Linux, Windows is happy) and a couple 1% MaxHeap.
We can't expect to duplicate functionality (cause this is actually duping all the DM code) and add a whole new feature without a tiny memory hit, if we do, this is 99% wontfix, like a bunch of other features.  That said, we will investigate further (We already did, and found no reason in the code) using Cedar numbers we have, but don't think a backout will bring anything useful.
(In reply to Marco Bonardo [:mak] from comment #93)
> (In reply to Ehsan Akhgari [:ehsan] from comment #92)
> > This regressed Talos tests such as Tp5 MozAfterPaint and a bunch of others
> > by 3-11%.  I think we should back this out for now and investigate the
> > performance regressions.
> 
> Where do you see a 11% tp5 regression?  I only see a couple private bytes
> regressions (max 3%, and only Linux, Windows is happy) and a couple 1%
> MaxHeap.
> We can't expect to duplicate functionality (cause this is actually duping
> all the DM code) and add a whole new feature without a tiny memory hit, if
> we do, this is 99% wontfix, like a bunch of other features.  That said, we
> will investigate further (We already did, and found no reason in the code)
> using Cedar numbers we have, but don't think a backout will bring anything
> useful.

Appologies, this was probably my mistake (I seem to have read the graphs wrong or something.)
no worries, rather I should file a bug cause looks like Cedar is not properly running and reporting the new Tp5 Row MozAfterPaint SuperExtraMegaCool measures...
https://hg.mozilla.org/mozilla-central/rev/913f7811c068

Please note:
- There are known bugs, see the blocked bugs and the dependencies in bug 564934 (we will probably unify all the deps in an "enable the new downaloads panel by default" bug.
- This will be disabled in Aurora until we consider the quality high enough, will stay enabled in Nightly to help figuring and fixing issues, can be disabled by setting browser.download.useToolkitUI to true.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 746766
A couple of Litmus tests will have to be updated now with this feature has been landed.
Flags: in-litmus?(simona.marcu)
(In reply to Henrik Skupin (:whimboo) from comment #97)
> A couple of Litmus tests will have to be updated now with this feature has
> been landed.

note it will stay enabled only in Nightly, for now.

Updated

5 years ago
Depends on: 746787

Updated

5 years ago
Depends on: 746853

Comment 99

5 years ago
Not sure if already filled but here are few things NewDM should be fixed at and i do realize this is not the place to tell but can't file new bugs too :

1) NewDM should notify about on-going downloads when Firefox is closed.
2) The panel when with an empty list , looks weird , IMO it should mention "There are no downloads for the session" 
3) The text for time left seems very thin , is it using default font? or is it rendered in some other way?

Other than that it feels very modern and IMO the best DM i have ever seen.
One regression (and an important one) caused by the new DM :

You cannot visit the directory for a downloaded file from a previous session, as the Downloads panel gets empty on each session end. You cannot know the directory of any file from previous session since the Library does not have a column for that or any button to open the containing folder.

Is there a bug or should I file one ?
(In reply to Girish Sharma [:Optimizer] from comment #100)
> One regression (and an important one) caused by the new DM :
> 
> You cannot visit the directory for a downloaded file from a previous
> session, as the Downloads panel gets empty on each session end. You cannot
> know the directory of any file from previous session since the Library does
> not have a column for that or any button to open the containing folder.
> 
> Is there a bug or should I file one ?

If the Downloads Arrow is showing, clicking on it will show your 'last download', clicking on the teeny-tiny magnifying glass will open the 'Containing Folder'

In the Library use the 'View' in the Menu - Add columns - check 'Visit Date', then you will see in the Library a date for all your downloads.  

Nothing really missing, just rearranged.

Updated

5 years ago
Depends on: 747018
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #101)
> In the Library use the 'View' in the Menu - Add columns - check 'Visit
> Date', then you will see in the Library a date for all your downloads.  
> 
> Nothing really missing, just rearranged.

Seriously ??
I was talking about the fact that in the Library, there is no way to know the downloads directory of any particular download. Where did I even intend to know the visit date of a download ?

Comment 103

5 years ago
Icon doesn't show up in different Firefox themes -- for example, fxchrome https://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&ved=0CC4QFjAA&url=https%3A%2F%2Faddons.mozilla.org%2Fen-US%2Ffirefox%2Faddon%2Ffxchrome%2F&ei=wGKQT6r8Maq8iwKk-pG-Aw&usg=AFQjCNFCgA1JCoPObUYXuBiOLoELPrPcBw&sig2=zYFMAhrzeciBV8bWUwc-Pw&cad=rja --  despite the button still being there and there being downloads in it, effectively resulting in what looks at first glance to be wasted space. At this point it won't let me change the icon or background image with CSS (Stylish addon) either. Using the default theme works fine, and in these themes the icon is there and completely visible in the customize toolbars mode.

Updated

5 years ago
Depends on: 747310
(Assignee)

Comment 104

5 years ago
There is a new feature status page detailing some of the known issues:

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

Note that I'm listing only the most noticeable issues there, while all the other
known issues are in bugs blocking bug 564934. There is a query linked from the
page to see the list of all those bugs.
Keywords: addon-compat
Whiteboard: [please discuss the feature in bug 564934, not here] → [=== See the STATUS PAGE before posting - linked in the bug's URL field ===]
(Assignee)

Updated

5 years ago
No longer depends on: 746766

Comment 105

5 years ago
Where is the option to disable the new Downloads Panel and revert to the Original Downloads Window for all the users who don't like the new panel and prefer the extra versatility of the original window?

Perhaps I've missed it.  If not, then will you please make the new panel vs the original window a selectable option.  This is in keeping with the trend in browsers to provide additional new options to users while keeping the original options available.  It has proven to be much preferred over replacing one option with a different one.

Providing users selectable options keeps everybody happy, rather than making many users unhappy while pleasing some of the remaining users (there are always others who could care less, as we all know). ;)

After all, customizing (or personalizing) your experience -is- the big thing these days. :)

p.s. You can default whichever option you feel the most users will prefer.

Thanks!
(In reply to mlngr1960 from comment #105)
> Where is the option to disable the new Downloads Panel and revert to the
> Original Downloads Window for all the users who don't like the new panel and
> prefer the extra versatility of the original window?

go to about:config and toggle browser.download.useToolkitUI
Component: General → Downloads Panel
QA Contact: general → downloads.panel
(Assignee)

Comment 107

5 years ago
Disabling the feature in Aurora is now tracked in bug 748541.
tracking-firefox14: ? → ---
Depends on: 781455
Target Milestone: Firefox 14 → Firefox 20
(In reply to Henrik Skupin (:whimboo) from comment #97)
> A couple of Litmus tests will have to be updated now with this feature has
> been landed.

Tests that verifies the feature's functionality have been added in Moztrap starting Firefox 20. Removing the "in-litmus" flag.
Flags: in-litmus?(simona.marcu)
(In reply to Simona B [QA] from comment #108)
> Tests that verifies the feature's functionality have been added in Moztrap
> starting Firefox 20. Removing the "in-litmus" flag.

Please ensure to set the in-moztrap+ flag in those cases. As best even provide a link to the test. That would be very helpful for later found regressions.
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.