835885 - Add serialization and deserialization

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the jsdownloads API, the DownloadStore object should be able to create
serializable JSON data from a DownloadList object, and re-populate a
DownloadList based on the deserialized data.

Comment 1

[:Paolo Amadini]

Attachment: The patch

While the exact format of the "downloads.json" file is still subject to change
(bug 851454), this patch adds a first version the code required to serialize
and deserialize the file, including regression tests. Tests where source JSON
strings are defined will be changed in bug 851454 to reflect the final format.

This patch also establishes "DownloadIntegration" as a very compact module
that provides the functions that depend on having a user profile defined, and
in general those functions that regression tests might want to override with
mock implementations to provide more encapsulated unit testing.

Comment 2

[Neil Deakin]

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

>+  /**
>+   * Performs initialization of the list of persistent downloads, before its
>+   * first use by the host application.  This function may be called only once
>+   * during the entire lifetime of the application.

If it should only be called once, this function should have a check to ensure this.

>+  load: function DS_load()
>+  {
>+    return Task.spawn(function task_DS_load() {

Do you want to clear this.list first before loading?


>+          // If an item is unrecognized, don't prevent others to be loaded.

'others to be loaded' -> 'others from being loaded'


>+
>+  // Disable integration with the host application requiring profile access.
>+  DownloadIntegration.loadPersistent = function() Promise.resolve();
> });

This seems quite hacky. Should you even be allowed to modify the objects like this?
Do you plan on adding tests that check the interaction with the real download list?


>+/**
>+ * Checks that loading from a missing file results in an empty list.
>+ */
>+add_task(function test_load_empty()
>+{

You might want to check that the file does not exist first.



>+  let string = "[{\"source\":{\"uri\":\"http://localhost:4444/source.txt\"}," +
>+                "\"target\":{\"file\":" + filePathLiteral + "}," +
>+                "\"saver\":{\"type\":\"copy\"}}," +
>+                "{\"source\":{\"uri\":\"http://localhost:4444/empty.txt\"}," +
>+                "\"target\":{\"file\":" + filePathLiteral + "}," +
>+                "\"saver\":{\"type\":\"copy\"}}]";

You should use the defined constants for the hostname/filename.

>+  let string = "[{\"source\":null," +
>+                "\"target\":null}," +
>+                "{\"source\":{\"uri\":\"http://localhost:4444/source.txt\"}," +
>+                "\"target\":{\"file\":" + filePathLiteral + "}," +
>+                "\"saver\":{\"type\":\"copy\"}}]";

Same here.

>+
>+  let string = "[{\"source\":null,\"target\":null}," +
>+                "{\"source\":{\"uri\":\"http://localhost:4444/empty.txt\"}}";

And here.

>+
>+  yield OS.File.writeAtomic(store.path, new TextEncoder().encode(string),
>+                            { tmpPath: store.path + ".tmp" });
>+ 
>+  try {
>+    yield store.load();
>+    do_throw("Exception expected when JSON data is malformed.");
>+  } catch (ex if ex.name == "SyntaxError") { }

Add a check inside the catch block just to be clear and complete.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

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

::: toolkit/components/jsdownloads/src/DownloadStore.jsm
@@ +59,3 @@
>   */
> +function DownloadStore(aList, aPath)
> +{

some checks on the inputs may be welcome

@@ +99,5 @@
> +        try {
> +          let download = yield Downloads.createDownload({
> +            source: { uri: NetUtil.newURI(downloadData.source.uri) },
> +            target: { file: new LocalFile(downloadData.target.file) },
> +            saver: downloadData.saver,

This is not a saver object, is more like a descriptor of the saver, thus the name is imo quite misleading.

::: toolkit/components/jsdownloads/test/unit/test_DownloadStore.js
@@ +181,5 @@
> +                "{\"source\":{\"uri\":\"http://localhost:4444/empty.txt\"}}";
> +
> +  yield OS.File.writeAtomic(store.path, new TextEncoder().encode(string),
> +                            { tmpPath: store.path + ".tmp" });
> + 

nit: trailing space

Comment 4

[:Paolo Amadini]

Attachment: Updated patch

Addressed review comments, more details below.

(In reply to Neil Deakin from comment #2)
> >+  load: function DS_load()
> 
> Do you want to clear this.list first before loading?

No, I don't think that's necessary.

> >+  // Disable integration with the host application requiring profile access.
> >+  DownloadIntegration.loadPersistent = function() Promise.resolve();
> > });
> 
> This seems quite hacky. Should you even be allowed to modify the objects
> like this?

We've gone for using test mode boolean variables in the meantime, this is also
not optimal but might be sufficient for now.

> Do you plan on adding tests that check the interaction with the real
> download list?

Loading and saving across real browser restarts would be out of scope for this
test unit. We may add those tests at application integration time, probably not
as xpcshell tests, and they may even be simple manual tests because most of
the unit tests in this module already check the correctness of the operation.

(In reply to Marco Bonardo [:mak] from comment #3)
> @@ +99,5 @@
> > +        try {
> > +          let download = yield Downloads.createDownload({
> > +            source: { uri: NetUtil.newURI(downloadData.source.uri) },
> > +            target: { file: new LocalFile(downloadData.target.file) },
> > +            saver: downloadData.saver,
> 
> This is not a saver object, is more like a descriptor of the saver, thus the
> name is imo quite misleading.

I think we can work in bug 851454 to define what we want to do with these names.

Comment 5

[Neil Deakin]

Comment on attachment 765390 [details] [diff] [review]
Updated patch

>+
>+  /**
>+   * This promise is resolved with a reference to a DownloadList object that
>+   * represents persistent downloads.  This property is null before the list of
>+   * downloads is requested for the first time.
>+   */
>+  _promisePublicDownloadList: null,
> 

Did you intend the fields '_promisePublicDownloadList' and '_privateDownloadList' to be inconsistently named?

Comment 6

[:Paolo Amadini]

(In reply to Neil Deakin from comment #5)
> Did you intend the fields '_promisePublicDownloadList' and
> '_privateDownloadList' to be inconsistently named?

One of them contains a promise and the other contains the list itself, because the
public downloads list has code to handle persistence, while the private one doesn't.

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2adac489a51

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 765390 [details] [diff] [review]
Updated patch

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

not sure how I missed this feedback, I don't have anything to add.