Keep only minimal state information in the DataItem

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

5 years ago
Okay, this refactoring is taking shape now. With only one download backend, we can avoid duplicating all the Download properties and methods in the DataItem.

The next step is representing history downloads using the same interface as this simplified DataItem, basically a DownloadsHistoryDataItem + fake HistoryDownload object.

When this is done, we can share some of the view and controller code between the Panel and the Library. We already use the same download.xml binding, but the controlling code is duplicated because it's driving different interfaces.

The DataItem still exists to keep the "state" property and track when it changes. But with the due care, the old state can be moved up to the view level (implementing onDataItemStateChanged inside onDataItemChanged), making the DataItem fully redundant in favor of pure Download/HistoryDownload objects.

Jared, this and other patches depend on bug 1115421 that I'd like Marco or Mano to review, but feel free to steal any other review, or do a first pass on bug 1115421.
Attachment #8541924 - Flags: review?(mak77)
Attachment #8541924 - Flags: review?(jaws)
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8541924 [details] [diff] [review]
Untested patch

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

::: browser/components/downloads/content/downloads.js
@@ +1428,5 @@
>      downloadsCmd_pauseResume() {
> +      if (this.dataItem.download.stopped) {
> +        this.dataItem.download.start();
> +      } else {
> +        this.dataItem.download.cancel();

This is now duplicated between this file (downloads.js) and allDownloadsViewOverlay.js. Are there plans to remove the duplication?
Attachment #8541924 - Flags: review?(jaws)
Assignee

Comment 2

5 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Comment on attachment 8541924 [details] [diff] [review]
> Untested patch
> 
> Review of attachment 8541924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/downloads/content/downloads.js
> @@ +1428,5 @@
> >      downloadsCmd_pauseResume() {
> > +      if (this.dataItem.download.stopped) {
> > +        this.dataItem.download.start();
> > +      } else {
> > +        this.dataItem.download.cancel();
> 
> This is now duplicated between this file (downloads.js) and
> allDownloadsViewOverlay.js. Are there plans to remove the duplication?

I think this can be done when the "download.xml" binding is unified, with the controller code implemented in the base DownloadElementShell.
Comment on attachment 8541924 [details] [diff] [review]
Untested patch

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

::: browser/components/downloads/DownloadsCommon.jsm
@@ +984,3 @@
>     */
>    get localFile() {
> +    // We should remove  should use this.download.target.partFilePath and check asyncrhonously.

broken comment?

@@ +984,4 @@
>     */
>    get localFile() {
> +    // We should remove  should use this.download.target.partFilePath and check asyncrhonously.
> +    return new FileUtils.File(this.download.target.path);

is there a bug filed already for this deprecation?
Code needs to change to support OS.File properly, even this same module needs localFile for showDownloadedFile...

@@ +993,5 @@
>     * @throws if the native path is not valid.  This can happen if the same
>     *         profile is used on different platforms, for example if a native
>     *         Windows path is stored and then the item is accessed on a Mac.
> +   *
> +   * @deprecated Callers should use OS.File and "download.target.partFilePath".

ditto, please ensure there's a bug filed for this.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +264,5 @@
>          };
>          if (this._dataItem.done) {
>            this._metaData.fileSize = this._dataItem.maxBytes;
>          }
> +        this._metaData.filePath = this._dataItem.download.target.path;

assign let targetPath = this._dataItem.download.target.path, use it for leafName and add filePath: targetPath to the object definition

@@ +490,5 @@
>      }
>      switch (aCommand) {
>        case "downloadsCmd_open":
> +        // We cannot open a session download file unless it's succeeded.
> +        // If it's succeeded, we need to make sure the file was not removed,

nit: just "it succeded"? (both lines)

@@ +519,5 @@
>          // temporarily assume that the file is in place.
>          return this.getDownloadMetaData().state == nsIDM.DOWNLOAD_FINISHED;
>        case "downloadsCmd_pauseResume":
>          return this._dataItem && this._dataItem.inProgress &&
> +               this._dataItem.download.hasPartialData;

I must admit that .resumable was far more readable then hasPartialData... I wonder if it may be worth to keep it

@@ +553,5 @@
>      switch (aCommand) {
>        case "downloadsCmd_open": {
> +        let file = new FileUtils.File(this._dataItem
> +                                      ? this._dataItem.download.target.path
> +                                      : this.getDownloadMetaData().filePath);

nit: I think I commented in the other bug about indentation of these, if you want to retain this structure please at least indent once more

@@ +561,5 @@
>        }
>        case "downloadsCmd_show": {
> +        let file = new FileUtils.File(this._dataItem
> +                                      ? this._dataItem.download.target.path
> +                                      : this.getDownloadMetaData().filePath);

ditto

@@ +580,5 @@
>          if (this._dataItem) {
> +          Downloads.getList(Downloads.ALL)
> +                   .then(list => list.remove(this._dataItem.download))
> +                   .then(() => this._dataItem.download.finalize(true))
> +                   .catch(Cu.reportError);

remove() is the other case where I feel like keeping the proxied helper would help... this is quite complex to not be centralized...

::: browser/components/downloads/content/downloads.js
@@ +1008,5 @@
>    // Set the URI that represents the correct icon for the target file.  As soon
>    // as bug 239948 comment 12 is handled, the "file" property will be always a
>    // file URL rather than a file name.  At that point we should remove the "//"
>    // (double slash) from the icon URI specification (see test_moz_icon_uri.js).
> +  this.image = "moz-icon://" + this.dataItem.download.target.path + "?size=32";

looks like you removed the previous comment about bug 239948 comment 12, since I expect the new store to be exepmt from that bug... at this point I guess this comment should be updated too.

@@ +1392,5 @@
>      cmd_delete() {
> +      Downloads.getList(Downloads.ALL)
> +               .then(list => list.remove(this.dataItem.download))
> +               .then(() => this.dataItem.download.finalize(true))
> +               .catch(Cu.reportError);

ditto, remove() this way is unreadable.
Attachment #8541924 - Flags: review?(mak77) → review+
Assignee

Comment 4

5 years ago
Assignee: nobody → paolo.mozmail
Assignee

Updated

4 years ago
Points: --- → 3
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Assignee

Updated

4 years ago
Blocks: 1129896
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Assignee

Updated

4 years ago
Attachment #8541924 - Flags: feedback?(smacleod)
Attachment #8541924 - Flags: feedback?(jaws)
https://hg.mozilla.org/mozilla-central/rev/8427ddc63056
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8541924 - Flags: feedback?(jaws) → feedback+
Attachment #8541924 - Flags: feedback?(smacleod) → feedback+
You need to log in before you can comment on or make changes to this bug.