1115369 - Use notifications instead of getViewItem for DownloadsView

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

Attachment: The patch

Based on how we use these notifications, this turns out to be less code, and no need to build objects on the fly anymore.

Comment 1

[:Paolo Amadini]

The new function names better reflect what these notifications have actually become.

Note how in the future we may get rid of onDataItemStateChanged entirely, in favor of checks for previous state inside onDataItemChanged in individual views that need them. This means each view would be able to detect only changes in the state it is interested in. This is the model used by the Downloads API.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8541256 [details] [diff] [review]
The patch

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

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +931,5 @@
>      // download is retired, onDataItemAdded is called again for the same
>      // data item. Thus, we also check that we make sure we don't have a view item
>      // already.
>      if (!shouldCreateShell &&
> +        aDataItem && !this._viewItemsForDataItems.get(aDataItem)) {

should use .has() instead of .get()

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8541256 [details] [diff] [review]
The patch

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

I'll let Marco's r+ here stand, but I'm just curious about the below change.

::: browser/components/downloads/DownloadsCommon.jsm
@@ -1366,5 @@
> -   * @return Object that can be used to notify item status events.
> -   */
> -  getViewItem(aDataItem) {
> -    let data = this._isPrivate ? PrivateDownloadsIndicatorData
> -                               : DownloadsIndicatorData;

Why don't we need to make a differentiation here for private versus non-private downloads anymore?

Comment 4

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> ::: browser/components/downloads/DownloadsCommon.jsm
> @@ -1366,5 @@
> > -   * @return Object that can be used to notify item status events.
> > -   */
> > -  getViewItem(aDataItem) {
> > -    let data = this._isPrivate ? PrivateDownloadsIndicatorData
> > -                               : DownloadsIndicatorData;
> 
> Why don't we need to make a differentiation here for private versus
> non-private downloads anymore?

It happens that "this" is already DownloadsIndicatorData or PrivateDownloadsIndicatorData depending on this._isPrivate. So it was just a longer way to say "let data = this;".

Comment 5

[:Paolo Amadini]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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