Use notifications instead of getViewItem for DownloadsView

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

Trunk
Firefox 38
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
Posted patch The patchSplinter Review
Based on how we use these notifications, this turns out to be less code, and no need to build objects on the fly anymore.
Attachment #8541256 - Flags: review?(jaws)
Assignee

Comment 1

5 years ago
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.
Assignee

Updated

5 years ago
Attachment #8541256 - Flags: review?(mak77)
Assignee: nobody → paolo.mozmail
Flags: qe-verify-
Flags: firefox-backlog+
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()
Attachment #8541256 - Flags: review?(mak77) → review+
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?
Attachment #8541256 - Flags: review?(jaws)
Assignee

Comment 4

5 years ago
(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;".
Assignee

Updated

5 years ago
Points: --- → 2
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
https://hg.mozilla.org/mozilla-central/rev/c10c7d4d04a2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.