697678 - [Downloads Panel] Investigate how Download Manager preferences affect the panel

Status: RESOLVED DUPLICATE of bug 726444

(Firefox :: General, defect)

Description

[:Paolo Amadini]

| This bug describes changes to the version of the downloads panel and
| indicator that is currently present in the UX branch, more specifically:
| * Bug 564934, attachment 558500 [details] [diff] [review]
| * Bug 663772, attachment 558504 [details] [diff] [review]
| 
| Patches for this bug should apply on top of the two mentioned patches.

"about:config" preferences in the "browser.download.manager." namespace might
affect the operation of the panel, which is based on Toolkit's Download
Manager for its operation. Investigate this aspect, considering that the
specifications on the panel's feature page should apply by default to most
configurations.

In particular the first download in the session should cause the panel to
open, for discoverability, regardless of the previous preferences (at least
for a few times after the new panel is available).

Comment 1

[:Paolo Amadini]

So, the majority of the preferences listed here apply consistently to the
standalone Download Manager as well as the new Downloads Panel:

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

For example, "browser.download.manager.addToRecentDocs" determines if the
download should be added to the recent documents, regardless of the user
interface model. There are only a few preferences that may apply differently
to the panel and the window interface. In particular:

== browser.download.manager.showWhenStarting ==
== browser.download.manager.focusWhenStarting ==

Both these preferences affect whether, when a new download is added, the method
that opens the panel ("nsIDownloadManagerUI::Show") is called at all. See:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadProxy.h#81

We want the panel to open with a different logic, for which we need to know when
a new download is added. Maybe we can bypass the "nsIDownloadManagerUI::Show"
method and use other notifications; if not, we need to ensure that these
preferences are always set to a known value. Note that "showWhenStarting" can
be synced across multiple profiles, apparently.

== browser.download.manager.retention ==

The only potentially problematic value is 0, that causes downloads to be removed
from view as soon as they are successfully completed. I'm not sure if we want to
allow that with the new panel. At present this preference can be set from the
options window.

We would need changes to Toolkit's Download Manager if we want this preference
to be ignored in new versions (maybe adding a hook to have the Download Manager
ignore the preference value). Alternatively we could just reset the preference
on version migration to its default value of 2 (never remove automatically),
though using the same profile with a different version would allow the
preference to be switched back to the old behavior.

This preference can be synced across between profiles also.

Comment 2

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini from comment #1)
> == browser.download.manager.showWhenStarting ==
> == browser.download.manager.focusWhenStarting ==
> 
> Both these preferences affect whether, when a new download is added, the
> method
> that opens the panel ("nsIDownloadManagerUI::Show") is called at all. See:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> nsDownloadProxy.h#81
> 
> We want the panel to open with a different logic, for which we need to know
> when
> a new download is added.

Could you also post the difference between what happens as it is now and what UX rather wants? That would drive the discussion on how to handle that.

> == browser.download.manager.retention ==
> 
> The only potentially problematic value is 0, that causes downloads to be
> removed
> from view as soon as they are successfully completed. I'm not sure if we
> want to
> allow that with the new panel.

This should be clarified with UX. We should respect user's will, but at the same time we should ask ourselves what the user is trying to achieve through this preference. He doesn't want to preserve visible downloads history for privacy reasons (And currently we are not really respecting this, indeed the Library will keep it all, do we have a bug filed on that?), if the panel would keep information, it would be a well exposed primary UI access to what he is trying to hide.
So, I don't think we could just ignore it or reset it, since that way we'd hurt the user's privacy needs.

Comment 3

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #2)
> > == browser.download.manager.showWhenStarting ==
> > == browser.download.manager.focusWhenStarting ==
> 
> Could you also post the difference between what happens as it is now and
> what UX rather wants? That would drive the discussion on how to handle that.

This corresponds to the following design items in the feature page:

"New downloads are notified with a brief animation near the indicator."
"The panel should be shown automatically for the first download of the
 browsing session."

Currently, if showWhenStarting is false, we do neither of those.

> > == browser.download.manager.retention ==
> 
> This should be clarified with UX. We should respect user's will, but at the
> same time we should ask ourselves what the user is trying to achieve through
> this preference. He doesn't want to preserve visible downloads history for
> privacy reasons (And currently we are not really respecting this, indeed the
> Library will keep it all, do we have a bug filed on that?), if the panel
> would keep information, it would be a well exposed primary UI access to what
> he is trying to hide.

Interesting, so you're saying that the purpose of this preference is to make the
item disappear from the primary UI as soon as possible (like closing a tab),
though not necessarily removing the item from the browsing history. Makes sense.

So we don't need to implement anything special for the new panel; we should keep
the preference in the UI, we just need UX input to understand where to place it
(it's actually orthogonal to Private Browsing Mode). We should change it from
"Remember download history" to something like "Hide downloads as soon as they
are finished" (looking forward to UX providing a better name).

Downloads in the Library window probably fall under the more general "Remember
my browsing history" preference, as they're not presented or looked up in a
different way from page visits. Maybe phrase it as "Remember my browsing and
download history", since we have two separate views in the Library?

Comment 4

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini from comment #3)
> Interesting, so you're saying that the purpose of this preference is to make
> the
> item disappear from the primary UI as soon as possible (like closing a tab),
> though not necessarily removing the item from the browsing history. Makes
> sense.

The problem is that the pref itself is confusing, it has always been but we made it more confusing by adding Downloads to the Library, before they were already there, but they were somehow "hidden" in the middle of other visits.
So I'd expect some users thinking that the pref just controls whether downloads history is saved, and some thinking it controls what the DM window is showing.

For the former case we should relabel the "Remember browsing history" as I said in bug 697679 and as you suggested here.

The latter case needs solution.

> We should change it from
> "Remember download history" to something like "Hide downloads as soon as they
> are finished" (looking forward to UX providing a better name).

this may work, need to figure out if the use-case is important enough to be kept.

Comment 5

[:Paolo Amadini]

Attachment: The patch

This changes how legacy preferences (see comment 1) affect the panel.

== browser.download.manager.showWhenStarting ==
== browser.download.manager.focusWhenStarting ==

These have no effect anymore on the panel, but still control the legacy window
if the panel is disabled. We don't change the value of these preferences. They
are invisible in the preferences window if the panel is enabled (bug 697679).

== browser.download.manager.retention ==

The preference to hide downloads as soon as they are finished has disappeared
from the preferences window according to the UX review in bug 708595.

For the panel, we want the behavior to be consistent for all users and, by
default, not delete the downloads as soon as they are finished (they are
deleted when closing the last browser window). In fact, previously the
preference to hide downloads when finished was set as a side-effect of
telling the browser never to remember history, generally not with that
specific intention in mind.

Thus, we reset the preference to default on migration. However, if a user
really wants to hide downloads immediately for the reasons explained in
comment 2, it can go to "about:config" to intentionally set the preference.

Comment 6

[:Paolo Amadini]

By the way, this is the last step towards proper profile migration, and together
with bug 697680 should allow us to make a first version of the panel for broader
feedback, incremental visual design and interface bug fixes.

Comment 7

[:Paolo Amadini]

The patch on this bug is now included as part of the patch in bug 726444. Any
pending issue here should have already been filed as one of the bug's follow-ups.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 596014 [details] [diff] [review]
The patch

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

::: browser/components/downloads/content/downloads.js
@@ +183,5 @@
> +    // Delay displaying the panel because this function will sometimes be called
> +    // while another window is closing (like the window for selecting whether to
> +    // save or open the file), and that would cause the panel to close
> +    // immediately.
> +    setTimeout(function() DownloadsPanel._openPopupIfDataReady(), 0);

nit space before () for anonymous functions. though I wonder if you could just pass DownloadsPanel._openPopupIfDataReady (eventually you could bind() it).

::: browser/components/nsBrowserGlue.js
@@ +1235,5 @@
> +      // With the new Downloads Panel, the preference to hide downloads as soon
> +      // as they are finished disappeared from the preferences window.  Now we
> +      // want downloads to consistently stay in the panel by default, though
> +      // this preference can still be changed in "about:config" after migration.
> +      Services.prefs.clearUserPref("browser.download.manager.retention");

this is still problematic when keeping the possibility to disable the feature and go back to the old window, we explicitly clear a pref the user set :(
Then we disable the feature in Aurora and we are hosed. We might either have a completely different code path for the panel, or mirror the pref value to a mirror pref and restore it on switching off the panel.

Comment 9

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #8)
> Then we disable the feature in Aurora and we are hosed. We might either have
> a completely different code path for the panel, or mirror the pref value to
> a mirror pref and restore it on switching off the panel.

We'll probably need to remove all the migration code in nsBrowserGlue from Aurora,
including the addition of the button and the switching of the preference, and
put it back with the right "version number" when we want to enable on Aurora.

Not sure if it's worth mirroring the preference if we do that, considering that
people probably use a dedicated profile for the Nightly update channel.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini from comment #9)
> We'll probably need to remove all the migration code in nsBrowserGlue from
> Aurora,
> including the addition of the button and the switching of the preference, and
> put it back with the right "version number" when we want to enable on Aurora.

I'm not sure it's going to work that easily, we may end up switching later, people may move across channels and so on. There's a lot of cases where this won't cover us from losing a user pref :(

> people probably use a dedicated profile for the Nightly update channel.

I strongly doubt this, even if it would be clever to do.

Comment 11

[Marco Bonardo [:mak]]

without taking into account issues we may have with removing and adding back migration here, someone else may then push a different migration with the same migration version or a bigger one, and our step would not run even adding it back.

Comment 12

[:Paolo Amadini]

Attachment: Improve support for same profile on different channels

Well, personally I think we're on the verge of over-engineering, however with
this patch the panel provides the best support for running the same profile
on different channels and:
 - Make sure that, with the panel interface, the default behavior is not to
    hide downloads as soon as they are finished.
 - Allow the new behavior to be changed using an about:config preference.
 - Keep the value of the old retention preference for the window interface,
    if the profile is used with Firefox 10, then with Firefox 13, and back
    to Firefox 10, or if the panel is disabled in Firefox 13.

By the way, note that in Firefox 13 the retention behavior for the window
interface cannot be changed from the GUI anymore, so we may have cases where
users who don't know about "about:config" have different behavior with
apparently the same preferences. But this is another story.

For the Downloads button migration, instead, I think we can proceed like this:
 - Put the button in the toolbar, but only when migrating to Firefox 13 with
   the feature enabled (meaning that, by default, it will be done only when
   using the profile at least once in the Firefox 13 Nightly).
 - Nightly users can easily customize the button away if they don't want it
   visible, for example if they want to go back to using the profile in
   Firefox 10 only. It will not be put back.
 - Having different button positions and visibility depending on whether you
   use the same profile on Nightly or Release is not supported.

When we enable the feature for everyone, we do the button migration again
(i.e. with a higher "UI version number") and with no preference check.

In this patch the UseWindowUI check for migration is still missing, though
I added a few more checks to get fully backwards compatible behavior for
the button. Will be there in the final version.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 596955 [details] [diff] [review]
Improve support for same profile on different channels

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

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +929,5 @@
> +  retentionBehavior->SetData(val);
> +  (void)mObserverService->NotifyObservers(retentionBehavior,
> +                                          "download-manager-change-retention",
> +                                          nsnull);
> +  retentionBehavior->GetData(&val);

please use aData argument to pass over data

Comment 14

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #13)
> please use aData argument to pass over data

I thought it would be possible, and I was looking into that now, but as soon as
I tried I noticed that aData is an immutable string, so I'm stuck with using
aSubject, like other notifications in the codebase do, even if it's not as
intuitive.

Comment 15

[Marco Bonardo [:mak]]

then add a comment about that please