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

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

Similarly to bug 1115971, we can remove the fallback to the Places icon, and use the generic icon for very old downloads.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[:Paolo Amadini]

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

Comment 4

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/bfc02c86998d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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