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

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.

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

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

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

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.

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.

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

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

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

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.

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

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

then add a comment about that please