824240 - Make the Downloads View Seamonkey-friendly

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Seamonkey is likely not going to port the new downloads view and will keep using the usual Library window. Though they want to keep staying up-to-date with our code.

There are some problems to evalute here:
- places.xul imports chrome://browser/content/downloads/allDownloadsViewOverlay.xul that won't exist there
- ContentArea takes decision to use the new view only based on the temporary pref we set, but we want to get rid of that pref, so it should take decision based on availability of the view

So maybe we should load the overlay manually and if it doesn't exist don't activate the view.

Blocking on this just in case we'd need to move some files, that is something we should do before release.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Apart that, we need to fix 822203 so we can avoid the special-casing for Downloads in the search box code.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 696001 [details] [diff] [review]
patch

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

I like this, I'm assuming you tested it working :)

::: browser/components/places/content/downloadsViewOverlay.xul
@@ +19,5 @@
> +        Components.interfaces.nsINavHistoryService.TRANSITION_DOWNLOAD +
> +        "&sort=" +
> +        Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;
> +
> +      ContentArea.setContentViewForQueryString(DOWNLOADS_QUERY, 

trailing space

::: browser/components/places/content/places.js
@@ +1252,5 @@
>    function CA_getContentViewForQueryString(aQueryString) {
> +    try {
> +      if (this._specialViews.has(aQueryString)) {
> +        let view = this._specialViews.get(aQueryString);
> +        if (typeof(view) == "function") {

I think we prefer the typeof something form in the codebase (no parenthesis)

@@ +1273,5 @@
> +   *        a query string
> +   * @param aView
> +   *        Either the custom view or a function that will return the view
> +   *        the first (and only) time it's called.
> +   */

please also add a javadoc to getContentViewForQueryString

@@ +1278,3 @@
>    setContentViewForQueryString:
>    function CA_setContentViewForQueryString(aQueryString, aView) {
>      this._specialViews.set(aQueryString, aView);

I wonder if here we should check input: aQueryString && typeof aView == "object" || typeof aView == "function", or throw

::: browser/components/places/jar.mn
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  browser.jar:
> +% overlay chrome://browser/content/places/places.xul chrome://browser/content/places/downloadsViewOverlay.xul

So basically seamonkey can just avoid this line and everything will keep working, right?

Comment 3

[Asaf Romano (gone)]

In turns out "Part 1" will be the only "part" here. As for search, we'll just need to keep the following code in even after the preference for disabling thew view is remove is removed 

      case "downloads":
        if (currentView == ContentTree.view) {
          let query = PlacesUtils.history.getNewQuery();
          query.searchTerms = filterString;
          query.setTransitions([Ci.nsINavHistoryService.TRANSITION_DOWNLOAD], 1);
          let options = currentOptions.clone();
          // Make sure we're getting uri results.
          options.resultType = currentOptions.RESULT_TYPE_URI;
          options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY;
          options.includeHidden = true;
          currentView.load([query], options);
        }
        else {
          // The new downloads view doesn't use places for searching downloads.
          currentView.searchTerm = filterString;
        }
        break;

Well, until bug 822203 is fixed.

Comment 4

[Asaf Romano (gone)]

Attachment: as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/f3e277d2e0c1

Comment 5

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #2)

> So basically seamonkey can just avoid this line and everything will keep
> working, right?

Yes.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3e277d2e0c1