The structure of the objects serialized into "downloads.json" should be well-defined, for the best forward and backwards compatibility. Ideally, the createDownload function should be able to get a serialized object from each of the items in "download.json" directly, without any special logic or transformation applied before. In fact, if createDownload maintains this forward and backwards compatibility for the JSON file items, calls made by add-ons would remain compatible as well. It might also be a good idea not to serialize the array as a root object, so that additional global properties can be added to the file later if needed.

In line with the current Async recommendations, I think we should move from using nsIFile to using file path strings in the JavaScript API for downloads. Apart from efficiency reasons, and encouraging the use of OS.File, this solves the issue for which, if a roaming profile is used once on a different platform, download items are lost because the file path is invalid. This patch also does the same for source URIs. In the patch I've also streamlined serialization and deserialization, to make the code nearer to the object it involves. Now createDownload behaves as per comment 0, accepting a serialized representation from the JSON file (and simplifying the JSON loading code). I've moved the download list away from being the root, for extensibility, also as described in comment 0. The public documentation on createDownload is now more detailed, since it is the documentation that consumers see, while the documentation inside the DownloadCore.jsm module is internal.

Comment on attachment 771868 [details] [diff] [review] The patch Review of attachment 771868 [details] [diff] [review]: ----------------------------------------------------------------- At a glance, this looks good. Is there anything specific that you wish me to look at? ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +428,5 @@ > + * Returns a static representation of the current object state. > + * > + * @return A JavaScript object that can be serialized to JSON. > + */ > + serialize: function D_serialize() Nit: I wouldn't call this |serialize|, as it doesn't return a flat data structure (string or byte array). @@ +436,5 @@ > + target: this.target.serialize(), > + saver: this.saver.serialize(), > + }; > + > + // Simplify the representation for the most common saver type. Do I understand correctly that this is just an optimization to reduce the size of the serialized string?

(In reply to David Rajchenbach Teller [:Yoric] from comment #2) > At a glance, this looks good. > Is there anything specific that you wish me to look at? Just wanted an overall feedback, thanks for that! > Nit: I wouldn't call this |serialize|, as it doesn't return a flat data > structure (string or byte array). Good point. Naming suggestions, anyone? > Do I understand correctly that this is just an optimization to reduce the > size of the serialized string? Yes, the idea is that this both reduces the size and makes the representation easier to read manually.

Comment on attachment 771868 [details] [diff] [review] The patch Review of attachment 771868 [details] [diff] [review]: ----------------------------------------------------------------- I can figure some of the spec from the code, but may we actually get a separate proposal for downloads.json with a skeleton of all of the (current and short-future) supported key=>values and an example deserialized object, to get a better idea of its size and contents? Personally, that would help me making a better idea. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +75,5 @@ > +function isString(aValue) { > + // We cannot use the "instanceof" operator reliably across module boundaries. > + return (typeof aValue == "string") || > + (typeof aValue == "object" && "charAt" in aValue); > +} Are we exposing to consumers the need to distinguish between string primitives and string objects? that may be cause of subtle bugs, so I hope it's for internal use. If not, the javadocs should always use "String object" to describe them and not be confusing. @@ +428,5 @@ > + * Returns a static representation of the current object state. > + * > + * @return A JavaScript object that can be serialized to JSON. > + */ > + serialize: function D_serialize() Maybe toSerializable() @@ +448,5 @@ > + > +/** > + * Creates a new Download object from a partially or totally serialized > + * representation. This function is used by the createDownload method of > + * Downloads.jsm when a new Download object is requested. it's a bit unclear what "partially or totally" means, do you mean from a "serialized or serializable"? @@ +464,5 @@ > + * } > + * > + * @return The newly created Download object. > + */ > +Download.deserialize = function (aSerialized) { as well as here, deserialize, doesn't look like supporting getting a common object. why not just import, loadFrom, adopt, parse? @@ +495,5 @@ > function DownloadSource() { } > > DownloadSource.prototype = { > /** > + * String containing the URI for the download source. unclear if string primitive or String object, or any (nothing to do if it's just a primitive or doesn't matter) @@ +500,2 @@ > */ > uri: null, fwiw, I usually prefer to use url for string urls, rather than uri, mostly cause uri is reminiscent of nsIURI, we have some .uri properties in Places for example, and everytime I have to go check if it's an nsIURI or an urlstring :( and luckily they are interchangeable in this context. @@ +507,5 @@ > */ > isPrivate: false, > > /** > + * String containing the referrer URI of the download source, or null if no ditto about string @@ +517,5 @@ > * Returns a static representation of the current object state. > * > * @return A JavaScript object that can be serialized to JSON. > */ > serialize: function DS_serialize() ditto for the name (not going to repeat same things later, I think the concepts are clear enough :) )

(In reply to Marco Bonardo [:mak] from comment #4) > I can figure some of the spec from the code, but may we actually get a > separate proposal for downloads.json with a skeleton of all of the (current > and short-future) supported key=>values and an example deserialized object, > to get a better idea of its size and contents? Personally, that would help > me making a better idea. Here is what a valid "downloads.json" file could look like if artificially indented (this is taken from the regression test with the predefined string): { "list": [ { "source": "http://localhost:4444/source.txt", "target": "/tmp/firefox/xpcshellprofile/test-download-821732.txt" }, { "source": { "uri": "http://localhost:4444/empty.txt", "referrer": "http://localhost:4444/referrer.html" }, "target": "/tmp/firefox/xpcshellprofile/test-download-821732.txt" } ] } This would result in two Download objects being added to the download list, each with its appropriate DownloadSource and DownloadTarget objects. The objects from the JSON are compatible with the arguments of createDownload, and for example in the near future we might have: Downloads.createDownload({ source: { uri: "http://localhost:4444/page.html", referrer: "http://localhost:4444/referrer.html" }, target: { path: "/tmp/test-download.txt", partFilePath: "/tmp/test-download.txt.part" }, openWhenSucceeded: true, openWithApplication: "/usr/bin/firefox", }); > ::: toolkit/components/jsdownloads/src/DownloadCore.jsm > @@ +75,5 @@ > > +function isString(aValue) { > > + // We cannot use the "instanceof" operator reliably across module boundaries. > > + return (typeof aValue == "string") || > > + (typeof aValue == "object" && "charAt" in aValue); > > +} > > Are we exposing to consumers the need to distinguish between string > primitives and string objects? No, this is done exactly so that consumers don't need to care whether they are using a primitive string (common) or a String object (rare but possible). They behave similarly, except when checking their type, hence the helper function. > @@ +500,2 @@ > > */ > > uri: null, > > fwiw, I usually prefer to use url for string urls, rather than uri, mostly > cause uri is reminiscent of nsIURI, we have some .uri properties in Places > for example, and everytime I have to go check if it's an nsIURI or an > urlstring :( and luckily they are interchangeable in this context. Yeah, probably "url" is better (even if this can be any URI spec). We may also call this "source.spec" to be even clearer, or "source.location" to use a naming that is more similar to "source.referrer". What would you suggest? This must be a string, because I've not added code to accept an nsIURI here (only if used directly as the "source" argument). Do you think we should also accept an nsIURI for the location and the referrer?

(In reply to Paolo Amadini [:paolo] from comment #5) > Here is what a valid "downloads.json" file could look like if artificially > indented (this is taken from the regression test with the predefined string): what about the "saver", it just stores a type? (In reply to Paolo Amadini [:paolo] from comment #5) > Yeah, probably "url" is better (even if this can be any URI spec). We may > also > call this "source.spec" to be even clearer, or "source.location" to use a > naming that is more similar to "source.referrer". What would you suggest? well, I think I already made my suggestion :) location is confusing with nsIDOMLocation (one may think of location.href) spec would work, though it's a very unfriendly name. I don't think I'm willing to play the bikeshed game, I think url is pretty fine, if we don't want to change, then uri will still be ok. Could be I'm just so used to nsIURI that when I read .uri I try to assign an nsIURI to it or to extract a .spec from it.

(In reply to Marco Bonardo [:mak] from comment #6) > (In reply to Paolo Amadini [:paolo] from comment #5) > > Here is what a valid "downloads.json" file could look like if artificially > > indented (this is taken from the regression test with the predefined string): > > what about the "saver", it just stores a type? Good question, this in fact depends on the saver type. For example, the "copy" saver on a resumable source might store more state information if serialized, to attempt resuming after the saver is deserialized in another session: { "list": [ { "source": "http://localhost:4444/source.txt", "target": "/tmp/firefox/xpcshellprofile/test-download-821732.txt" "saver": { "type": "copy", "wasRunning": true, "entityId": "ID123456", } } ] }

Addressed Marco's comments (not setting the feedback flag again, but feel free to take a look at the updated version).

Comment on attachment 773964 [details] [diff] [review] Updated patch > Here is what a valid "downloads.json" file could look like if artificially > indented (this is taken from the regression test with the predefined string): > { > "list": [ > { > "source": "http://localhost:4444/source.txt", > "target": "/tmp/firefox/xpcshellprofile/test-download-821732.txt" > }, ... Add this or a similar example to the script for better documentation. >+ toSerializable: function () >+ { >+ let serializable = { >+ source: this.source.toSerializable(), >+ target: this.target.toSerializable(), >+ saver: this.saver.toSerializable(), >+ }; >+ >+ // Simplify the representation for the most common saver type. >+ if (serializable.saver === "copy") { >+ delete serializable.saver; >+ } This is a little strange. It would seem clearer to just write: let saver = this.saver.toSerializable(); if (saver !== "copy") serializable.saver = saver; Your example in comment 7 suggests that the saver might be an object with type = "copy" instead, and thus won't match the condition here. Maybe this should be handled here anyway to avoid missing this needed change later. >+ >+ return serializable; >+ }, >+}; >- target: { file: getTempFile(TEST_TARGET_FILE_NAME) }, >- saver: { type: "copy" }, >+ source: { url: TEST_EMPTY_URI.spec, >+ referrer: TEST_REFERRER_URI.spec }, Rather than the constant being an nsIURI and having to use TEST_EMPTY_URI.spec, the constant should just be the string. It looks like the string is used more often, if the uri is used at all. Similar for TEST_REFERRER_URI and any other similar constants.

(In reply to Neil Deakin from comment #9) > let saver = this.saver.toSerializable(); > if (saver !== "copy") > serializable.saver = saver; > > Your example in comment 7 suggests that the saver might be an object with > type = "copy" instead, and thus won't match the condition here. Maybe this > should be handled here anyway to avoid missing this needed change later. If the saver is an object instead of a simple string, we can't simplify it because we need to persist all its properties, not only "type". This may happen for savers of type "copy" as well as other types. > >- target: { file: getTempFile(TEST_TARGET_FILE_NAME) }, > >- saver: { type: "copy" }, > >+ source: { url: TEST_EMPTY_URI.spec, > >+ referrer: TEST_REFERRER_URI.spec }, > > Rather than the constant being an nsIURI and having to use > TEST_EMPTY_URI.spec, the constant should just be the string. It looks like > the string is used more often, if the uri is used at all. > > Similar for TEST_REFERRER_URI and any other similar constants. Actually, these will not be constants anymore once the patch is rebased on top of bug 888544. We could change this into something like: source: { url: httpUrl("empty.txt"), referrer: httpUrl("referrer.html") }

(In reply to Paolo Amadini [:paolo] from comment #10) > Actually, these will not be constants anymore once the patch is rebased on > top > of bug 888544. We could change this into something like: Can you make a new patch now that bug has been fixed?

Addressed previous comments and converted the constants.

Paolo, how can QA verify this fix? I can see several unit tests attached in the pushlogs. Is there anything else I can do here?

(In reply to Mihai Morar, QA (:MihaiMorar) [needinfo? me for any requests] from comment #15) > Paolo, how can QA verify this fix? I can see several unit tests attached in > the pushlogs. Is there anything else I can do here? This is more of an internal detail of the Downloads API, so I think there is no action required for further verification.

Marking as [qa-] based on Commen 16. Thanks Paolo!