Closed Bug 913110 Opened 7 years ago Closed 7 years ago

Add a combined summary of public and private downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 2 obsolete files)

The download progress indicator on the task bar includes the state of both
public and private downloads. A summary object should be available to the
DownloadTaskbar module to register for updates to the overall progress.
Depends on: 913118
Attached patch Work in progress (obsolete) — Splinter Review
This implementation of the DownloadSummary object only provides the properties
required by the new DownloadTaskbar module. They are surprisingly few, for
example because, when the taskbar indicator was used in browser windows, the
old DownloadTaskbarProgress module only provided a generic progress indication
for in-progress downloads, without considering the paused state.

There also used to be the undetermined state, that we may want to figure out
how to handle now, and for which we may require another summary property. In
general, for the indicator I think we should feel free to implement what
makes more sense, without replicating the old behavior exactly.
Attachment #802278 - Flags: feedback?(enndeakin)
Some questions:

> +  onDownloadAdded: function (aDownload)
> +  {
> +    this._downloads.push(aDownload);
> +    if (this._list) {
> +      this._onListChanged();
> +    }
> +  },

Here you maintain a separate list of the downloads in this._downloads. Is this going to be different than the one provided by DownloadList.getAll, or could the same one be used?

Would it be better to only maintain a list of downloads that are active? Right now it looks as if _onListChanged iterates over every download to compute the summary.

> +    if (!(aType in this._summaryPromises)) {
> +      this._summaryPromises[aType] = Promise.resolve(new DownloadSummary());
> +    }
> +
> +    return this._summaryPromises[aType];

Why are these promises rather than the summaries directly?
(In reply to Neil Deakin from comment #2)
> Some questions:
> 
> > +  onDownloadAdded: function (aDownload)
> > +  {
> > +    this._downloads.push(aDownload);
> > +    if (this._list) {
> > +      this._onListChanged();
> > +    }
> > +  },
> 
> Here you maintain a separate list of the downloads in this._downloads. Is
> this going to be different than the one provided by DownloadList.getAll, or
> could the same one be used?

The getAll function returns a promise and provides a new copy of the internal list every time (so that callers can iterate the list to remove dowloads, and cannot add or remove downloads from the DownloadList without triggering notifications). To make the onDownloadChanged case (the most frequent one) more efficient, we keep our own static copy instead of asking a new list every time.

Maybe in the future we can implement alternatives to getAll to make this simpler, we can talk about this during the general API review tomorrow.

> Would it be better to only maintain a list of downloads that are active?
> Right now it looks as if _onListChanged iterates over every download to
> compute the summary.

This would require iterating over the list in onDownloadChanged to determine if the state of the provided download changed, so there is little point in doing it.

> > +    if (!(aType in this._summaryPromises)) {
> > +      this._summaryPromises[aType] = Promise.resolve(new DownloadSummary());
> > +    }
> > +
> > +    return this._summaryPromises[aType];
> 
> Why are these promises rather than the summaries directly?

The getSummary function returns a promise in case we need to trigger some kind of initialization in the future.

The reason why we store the promise in _summaryPromises is that in this way we don't need to re-create the resolved Promise object at every call. Actually, this function is not called very often, so we could in fact just store the DownloadSummary objects. I guess the remaining reason to store promises is just symmetry with getList().
Attachment #802278 - Flags: feedback?(enndeakin) → feedback+
Attached patch The patch (obsolete) — Splinter Review
It turns out that, even when calling only the getList(Downloads.PUBLIC) or the
getList(Downloads.PRIVATE) methods, we must create the combined list also, in
order to link it to the combined summary.

I had to change the initialization code to always create the three lists
together, so I also stored the objects instead of the promises as you
previously suggested.
Attachment #802278 - Attachment is obsolete: true
Attachment #803748 - Flags: review?(enndeakin)
Comment on attachment 803748 [details] [diff] [review]
The patch

> function promiseNewList(aIsPrivate)
> {
>-  let type = aIsPrivate ? Downloads.PRIVATE : Downloads.PUBLIC;
>+  Downloads._promiseListsInitialized = null;
>+  Downloads._lists = {};
>+  Downloads._summaries = {};

Keep the comments here as to why you are clearing these lists.


>-  let privateList = yield promiseNewList(true);
>+  let privateList = yield Downloads.getList(Downloads.PRIVATE);

It seems unfortunate that we can't use promiseNewList here. Maybe the lists should be reset with a separate method that is called at the beginning of the test.


>+  do_check_false(publicSummary.allStopped);
>+  do_check_eq(publicSummary.progressTotalBytes, TEST_DATA_SHORT.length * 2);
>+  do_check_eq(publicSummary.progressCurrentBytes, TEST_DATA_SHORT.length);
>+
>+  do_check_false(privateSummary.allStopped);
>+  do_check_eq(privateSummary.progressTotalBytes, TEST_DATA_SHORT.length * 2);
>+  do_check_eq(privateSummary.progressCurrentBytes, TEST_DATA_SHORT.length);
>+
>+  do_check_false(combinedSummary.allStopped);
>+  do_check_eq(combinedSummary.progressTotalBytes, TEST_DATA_SHORT.length * 4);
>+  do_check_eq(combinedSummary.progressCurrentBytes, TEST_DATA_SHORT.length * 2);

Add some comments here so that it is clear which of the downloads will be included in the count. (Just the in progress ones and not the cancelled one, right?)
Attachment #803748 - Flags: review?(enndeakin) → review+
Attached patch Updated patchSplinter Review
(In reply to Neil Deakin from comment #5)
> >-  let privateList = yield promiseNewList(true);
> >+  let privateList = yield Downloads.getList(Downloads.PRIVATE);
> 
> It seems unfortunate that we can't use promiseNewList here. Maybe the lists
> should be reset with a separate method that is called at the beginning of
> the test.

I thought about this, but we call the individual method quite often, so I opted for requiring less calls in the most common case.

I updated the boolean property name to allHaveStopped as discussed.
Attachment #803748 - Attachment is obsolete: true
(In reply to Wes Kocher (:KWierso) from comment #8)
> Backed out because either this or bug 906620 caused some leaks:
> https://hg.mozilla.org/integration/fx-team/rev/b1bfed7bc312

It was most probably bug 906620, re-landed this one:

https://hg.mozilla.org/integration/fx-team/rev/4d44bc8da75c
https://hg.mozilla.org/mozilla-central/rev/4d44bc8da75c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0f3d62cbb2 while chasing bug 916757.

Feel free to reland after a debug Windows try push with like 30 browser-chrome runs on WinXP and Win8, since we're still not especially sure what got rid of it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Re-landed after a local test run, as this is unrelated to browser-chrome tests:

https://hg.mozilla.org/integration/fx-team/rev/670f8219bddb
Blocks: 916430
Comment on attachment 804612 [details] [diff] [review]
Updated patch

This is another API change that should be in the first Aurora build together
with bug 916430.
Attachment #804612 - Flags: approval-mozilla-aurora?
Depends on: 916757
(In reply to :Paolo Amadini from comment #12)
> Re-landed after a local test run, as this is unrelated to browser-chrome
> tests:
> 
> https://hg.mozilla.org/integration/fx-team/rev/670f8219bddb

One local test run isn't sufficient IMO to determine that this had no effect (unless jsdownload isn't part of the build, this could still break things, given how convoluted an issue bug 916757 turned out to be). Bug 916757 caused the tree to be closed for near on 24 hrs during merge day, so has disrupted quite a few people - so just to be safe, please can you do as comment 11 suggested and perform at least ~30 retriggers of winXP browser-chrome on Try *before* relanding :-)

Backed out again until the Try run + retriggers are performed (plus a handful of other things that conflicted with the backout):
remote:   https://hg.mozilla.org/integration/fx-team/rev/5736f23d8a22
remote:   https://hg.mozilla.org/integration/fx-team/rev/fd257eef04b2
remote:   https://hg.mozilla.org/integration/fx-team/rev/00593fe07777
remote:   https://hg.mozilla.org/integration/fx-team/rev/ea0b23759685
remote:   https://hg.mozilla.org/integration/fx-team/rev/cbc7af0a20c4
Just noticed your try push (https://tbpl.mozilla.org/?tree=Try&rev=7db774d5af8f).

These assertions (and thus bug 916757) are only enabled in debug builds (comment 11), however your Try push only requested opt builds, so you'll need to push again with |-b d|.

If you need any help, just give a shout in #developers :-)
Using http://trychooser.pub.build.mozilla.org/ the try syntax best for your push (it also avoids doing unnecessary win7/win8 test runs), is:

try: -b d -p win32 -u mochitest-bc[Windows XP] -t none
> (In reply to :Paolo Amadini from comment #12)
> One local test run isn't sufficient IMO to determine that this had no effect

Ed, if you don't need my expertise in evaluating the impact of the bug based on my knowledge of the code and how it is used, I don't get the point of asking me to execute a purely mechanical task you could much more easily have done yourself.

The try build you noticed was for another issue, the browser-chrome debug build was this one:

https://tbpl.mozilla.org/?tree=Try&rev=0e6e531e344f

It looks green for bug 906620 also, which is the one that might have had an effect on browser-chrome tests.
(In reply to :Paolo Amadini from comment #17)
> > (In reply to :Paolo Amadini from comment #12)
> > One local test run isn't sufficient IMO to determine that this had no effect
> 
> Ed, if you don't need my expertise in evaluating the impact of the bug based
> on my knowledge of the code and how it is used, I don't get the point of
> asking me to execute a purely mechanical task you could much more easily
> have done yourself.

Local test runs are frequently not representative of the runs on our automation machines. Even if they were, it also wasn't clear what platform you tested on, or how many times you ran the entire suite (running a subset is likely to not reproduce). Extremely experienced Mozilla developers were stumped whilst investigating bug 916757, so I still don't think we can make any assumptions at this point (or at the least, I'm not comfortable risking the health of the tree when there is an easy way for us to find out first - Tryserver). My confidence was also slightly affected by the fact this bug (and a few related ones) have bounced *several times* already.

Please work with the sheriffs here, not against - we're trying to keep the trees open and unbroken for everyone, which will occasionally mean extra work for individual developers (sorry!), for the benefit of everyone else trying to land (or in the case of release management, trying to uplift a known good revision to aurora). 

If you have any questions, please ping me on IRC/via email and I'll be happy to explain how the development/landing process works for sheriffed trees :-)
Thanks for the response, I understand how your confidence was affected by seeing previous backouts of this bug.

The reason why I'm trying to land this bug and a few related ones more quickly than usual, also making some judgement about impact, is that we need to get those in the first Aurora build. We don't have a tracking flag for that (just the approval, that doesn't track which build), so it's my task to ensure they get tested in time. I appreciate any help in making those bugs stay in the tree.
https://hg.mozilla.org/mozilla-central/rev/47226f545fc3
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to :Paolo Amadini from comment #20)
> Thanks for the response, I understand how your confidence was affected by
> seeing previous backouts of this bug.
> 
> The reason why I'm trying to land this bug and a few related ones more
> quickly than usual, also making some judgement about impact, is that we need
> to get those in the first Aurora build. We don't have a tracking flag for
> that (just the approval, that doesn't track which build), so it's my task to
> ensure they get tested in time. I appreciate any help in making those bugs
> stay in the tree.

Paolo - we do have a flag for tracking (which I just set) - can you please fill out the nomination approval form which addresses the risk/reward of uplifting this?  I'll be putting the same tracking flag and question in but 916430.  Is this Downloads API work a feature that needs to be in Gecko 26  for b2g or is this a desktop feature?
Attachment #804612 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Sorry, mid-aired with Lukas' comment. This is a desktop feature. We've been working on it for a while and would like to ship as soon as possible. Aurora user feedback is important for that. We have high confidence in the feature, the automated testing coverage is good. There's also add-ons impact that we've been doing outreach for (working with Jorge), and keeping this on the 26 train will help with that.
Paolo what do you mean with public/private download? Dowloading from normal or private browsing?
(In reply to Mihai Morar, QA (:MihaiMorar) from comment #25)
> Paolo what do you mean with public/private download? Dowloading from normal
> or private browsing?

This bug contains back-end code for bug 906620, it doesn't need additional verification if you've tested that the progress of both public and private downloads appear in the taskbar indicator.
(In reply to :Paolo Amadini from comment #26)
 
> This bug contains back-end code for bug 906620, it doesn't need additional
> verification if you've tested that the progress of both public and private
> downloads appear in the taskbar indicator.

Marking as being Verified based on Comment 26 and on the verification I had done in Bug 906620 Comment 22
I confirm the progress is displayed in the taskbar for both public and private downloads.

Thanks Paolo!
Status: RESOLVED → VERIFIED
QA Contact: mihai.morar
You need to log in before you can comment on or make changes to this bug.