On first feature activation the panel should limit reported downloads

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mak, Assigned: mconley)

Tracking

unspecified
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

7 years ago
I got reported that someone on first use of the panel got a list with all the downloads in the last months, that shouldn't happen, should likely limit to the last X days or so.
The list in all conditions should fit in 4-5 downloads and have scrollbars for seeing the remaining.
Component: General → Downloads Panel
QA Contact: general → downloads.panel

Comment 2

7 years ago
This might be as simple as calling the cleanUp method of nsIDownloadManager on
migration in nsBrowserGlue.js, at the cost of initializing the download manager
synchronously on startup, or might involve introducing a mechanism that prevents
loading the downloads history on first run, in DowloadsStartup.js, by tuning the
flag we pass to ensurePersistentDataLoaded.

Comment 3

7 years ago
To be more precise, what we want here is to avoid old completed downloads from
the Downloads Window to leak in the session-specific completed downloads list
of the panel, when the feature is activated for the first time.
Assignee

Updated

7 years ago
Assignee: nobody → mconley
I think I'm going to try the DownloadsStartup.js route, rather than the nsBrowserGlue.js migration, since we'll likely only want to trigger this if the toolkit UI hasn't been selected...and migrations seem to roll the UI version forward regardless of outcome. Feels slippery.
Posted patch Patch v1 (obsolete) — Splinter Review
I've added another pref, recording whether or not this is the first time the Downloads Manager has been initted with the panel enabled. If so, we run the clean up.

Let me know if I've missed any corner-cases.
Attachment #672385 - Flags: feedback?(paolo.mozmail)
The patch I've put up is based on the patch I put up in bug 766263, so I'm marking the dependency in case anybody wants to try this patch out.

If this gets r+'d before 766263 lands, I'll just rebase.
Depends on: 766263

Comment 7

7 years ago
Comment on attachment 672385 [details] [diff] [review]
Patch v1

I _think_ we should be able to achieve the same effect by passing true to
ensurePersistentDataLoaded. In this case, maybe we should reset the preference
after the usual cleanUp on shutdown, to avoid old downloads actually reppearing
in case of crashes before the browser shuts down. Can you verify if this works?
Attachment #672385 - Flags: feedback?(paolo.mozmail)
Posted patch Patch v2 (obsolete) — Splinter Review
Paolo:

Is this what you had in mind?  The behaviour is more or less equivalent, except that if the user opens up the old downloads window, they can still see all of the old inactive downloads on the first run.

But other than that, it seems much the same.

-Mike
Attachment #672385 - Attachment is obsolete: true
Attachment #673268 - Flags: feedback?(paolo.mozmail)

Comment 9

7 years ago
Comment on attachment 673268 [details] [diff] [review]
Patch v2

(In reply to Mike Conley (:mconley) from comment #8)
> The behaviour is more or less equivalent,
> except that if the user opens up the old downloads window, they can still
> see all of the old inactive downloads on the first run.

That's right. Since this is the same behavior as the current Nightly, and won't
influence the Library window, I think this solution is better, for the startup
performance improvements it will give once every piece is in place.

+        if (this._panelFirstRun) {
+          this._panelFirstRun = false;
+        }

nit: is this actually faster than just setting the value, without the if?
Attachment #673268 - Flags: feedback?(paolo.mozmail) → feedback+
Posted patch Patch v3 (obsolete) — Splinter Review
(In reply to Paolo Amadini [:paolo] from comment #9)
> Comment on attachment 673268 [details] [diff] [review]
> Patch v2
> 
> (In reply to Mike Conley (:mconley) from comment #8)
> > The behaviour is more or less equivalent,
> > except that if the user opens up the old downloads window, they can still
> > see all of the old inactive downloads on the first run.
> 
> That's right. Since this is the same behavior as the current Nightly, and
> won't
> influence the Library window, I think this solution is better, for the
> startup
> performance improvements it will give once every piece is in place.
> 
> +        if (this._panelFirstRun) {
> +          this._panelFirstRun = false;
> +        }
> 
> nit: is this actually faster than just setting the value, without the if?

I don't actually know. The performance hit of writing a pref isn't something I've studied.

I'll trust your expertise in the matter. I've removed the conditional.
Attachment #673268 - Attachment is obsolete: true
Attachment #673304 - Flags: review?(mak77)

Comment 11

7 years ago
(In reply to Mike Conley (:mconley) from comment #10)
> I'll trust your expertise in the matter. I've removed the conditional.

I've no expertise, mine was a real question :-) But without a reason to do
otherwise, I agree the code is just simpler without the conditional.
Reporter

Comment 12

7 years ago
Comment on attachment 673304 [details] [diff] [review]
Patch v3

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

With bug 766263, we end up having 2 prefs that sound like are doing the same thing, even if they are not.
We should either:
- unify the 2
- clearly rename this to not make it sound like it refers to some first use

To unify we may store a cached session variable to avoid reopening in the session, while on next restart just go and read this firstRun pref.
Reporter

Comment 13

7 years ago
Comment on attachment 673304 [details] [diff] [review]
Patch v3

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

::: browser/components/downloads/src/DownloadsStartup.js
@@ +234,5 @@
>          gPrivateBrowsingService.privateBrowsingEnabled) {
>        return;
>      }
>  
> +    if (!DownloadsCommon.useToolkitUI && this._panelFirstRun) {

if we are using toolkitUI the panel should not have had a first run.
I know this is correct code-wise but the name is misleading, call it dataFirstLoaded or whatever else, but don't use panel if this may happen regarless it being used.
Attachment #673304 - Flags: review?(mak77) → review-
Posted patch Patch v4 (obsolete) — Splinter Review
Is this any clearer?
Attachment #673304 - Attachment is obsolete: true
Attachment #674242 - Flags: feedback?(mak77)
Reporter

Comment 15

7 years ago
Comment on attachment 674242 [details] [diff] [review]
Patch v4

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

yeah better. I still don't like this session restore code as a whole, but I guess it's just session restore itself that is confusing.

::: browser/components/downloads/src/DownloadsStartup.js
@@ +207,5 @@
>    /**
>     * Indicates whether we should load all downloads from the previous session,
>     * including completed items as well as active downloads.
>     */
> +  _restoringSession: false,

The comment needs to be updated, anything you can do to make clear what it mens and causes will be welcome...

@@ +241,5 @@
> +
> +  /**
> +   * True if we've never completed a session with the Downloads Panel enabled.
> +   */
> +  get _firstPanelSession() {

So... why not keeping the pref name that is clear enough as _firstSessionCompleted
you invert the pref here, then you invert again in _recoverallDownloads... so we can just never invert the meaning.
Attachment #674242 - Flags: feedback?(mak77) → feedback+
Posted patch Patch v5Splinter Review
(In reply to Marco Bonardo [:mak] from comment #15)
> Comment on attachment 674242 [details] [diff] [review]
> Patch v4
> 
> Review of attachment 674242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> ::: browser/components/downloads/src/DownloadsStartup.js
> @@ +207,5 @@
> >    /**
> >     * Indicates whether we should load all downloads from the previous session,
> >     * including completed items as well as active downloads.
> >     */
> > +  _restoringSession: false,
> 
> The comment needs to be updated, anything you can do to make clear what it
> mens and causes will be welcome...

Ok, done.

> 
> @@ +241,5 @@
> > +
> > +  /**
> > +   * True if we've never completed a session with the Downloads Panel enabled.
> > +   */
> > +  get _firstPanelSession() {
> 
> So... why not keeping the pref name that is clear enough as
> _firstSessionCompleted
> you invert the pref here, then you invert again in _recoverallDownloads...
> so we can just never invert the meaning.

Good idea. Done.
Attachment #674242 - Attachment is obsolete: true
Attachment #674291 - Flags: review?(mak77)
Reporter

Comment 17

7 years ago
Comment on attachment 674291 [details] [diff] [review]
Patch v5

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

It looks ok, thanks.
Attachment #674291 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/6c68ce017824
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.