Closed Bug 1115972 Opened 8 years ago Closed 8 years ago

Don't fall back to the Places icon for downloads without the target file name annotation

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Similarly to bug 1115971, we can remove the fallback to the Places icon, and use the generic icon for very old downloads.
Attached patch The patchSplinter Review
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8541915 - Flags: review?(mak77)
Comment on attachment 8541915 [details] [diff] [review]
The patch

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

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +52,5 @@
>   * valid. That's the caller (a DownloadsPlacesView object) responsibility.
>   *
> + * The caller is also responsible for "passing over" notifications. The
> + * DownloadsPlacesView object implements onDataItemStateChanged and
> + * onDataItemChanged of the DownloadsView pseudo interface.

"and registers as a Places result observer." (we still observe nodeAnnotationChanged, nodeInserted and nodeRemoved)

@@ +166,2 @@
>      }
>      if (this._dataItem) {

please add a newline before this and a comment
// Assert unreachable.

cause basically the next exceptions should never happen.

@@ +444,5 @@
>      this._targetFileInfoFetched = false;
>  
> +    let metaData = this.getDownloadMetaData();
> +    this._element.setAttribute("displayName", metaData.displayName);
> +    this._element.setAttribute("image", this._getIcon());

For now I'd leave _updateDisplayNameAndIcon and land this bug and bug 1115971 before bug 1115421 that is 
far more complicated. then remove _updateDisplayNameAndIcon in bug 1115421 eventually.

These 2 bugs are simple and can be well contained, so the dependency should be reversed.

ditto for _forEachDownloadElementShellForURI, please move these changes to bug 1115421, so that it's the last piece of the puzzle, not the first one.

@@ +1236,5 @@
>    nodeRemoved(aParent, aPlacesNode, aOldIndex) {
>      this._removeHistoryDownloadFromView(aPlacesNode);
>    },
>  
> +  nodeIconChanged(aNode) {},

s/aNode//
Attachment #8541915 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> ditto for _forEachDownloadElementShellForURI, please move these changes to
> bug 1115421, so that it's the last piece of the puzzle, not the first one.

Thanks for the review! Will do this patch reordering as soon as the other dependencies can land, and possibly when other reviews later in the chain are also completed. All of these annotation bugs are dependent on bug 1115369 and bug 1115379, you might want to review those as well, since Jared might be away.

Basically there is a queue of dependencies in bug 1068656, they're all applied in bug number order. I've not marked the dependency chain explicitly in each bug, but I can do that if it helps.
Flags: qe-verify-
Flags: firefox-backlog+
Points: --- → 1
Iteration: --- → 38.1 - 26 Jan
https://hg.mozilla.org/mozilla-central/rev/bfc02c86998d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.