Closed Bug 824240 Opened 7 years ago Closed 7 years ago

Make the Downloads View Seamonkey-friendly

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: mak, Assigned: mano)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → mano
Attached patch patch (obsolete) — Splinter Review
Apart that, we need to fix 822203 so we can avoid the special-casing for Downloads in the search box code.
Attachment #696001 - Flags: review?(mak77)
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?
Attachment #696001 - Flags: review?(mak77) → review+
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.
Attachment #696001 - Attachment description: Part 1 → patch
(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.
https://hg.mozilla.org/mozilla-central/rev/f3e277d2e0c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.