Closed Bug 947694 Opened 7 years ago Closed 7 years ago

Add a JSM with simple APIs for reading and writing JSON files

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [Async:ready])

Attachments

(1 file)

There are literally dozens of places in the tree where JSON files are written and loaded. This should be a one-liner. However, it isn't. I want to make it one.

While I'm here, I'm also going to throw in an API for reading and writing compressed JSON. AFAICT, nobody in tree is producing compressed JSON (probably because the XPCOM APIs for stream conversion are absurdly complex).
This should do it.

I'm not thrilled about the number of buffer allocations to convert to
gzipped data. I'm sure there is a better way, but someone with more
XPCOM string services knowledge will have to tell me about it.

The good news is this suboptimal implementation detail is abstracted
away. So if things are improved in the future, every consumer wins
traparently.
Attachment #8344305 - Flags: review?(dteller)
Assignee: nobody → gps
Status: NEW → ASSIGNED
Blocks: 875562
API design question: Should we be more intelligent and auto-detect encodings based on filename? e.g. if the filename ends with ".gz" we could automagically use gzip compression. This could be bolted on later, of course.
Comment on attachment 8344305 [details] [diff] [review]
Add JSON.jsm for common JSON operations

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

::: toolkit/modules/JSON.jsm
@@ +44,5 @@
> +   *
> +   * The written file contains the gzipped header.
> +   */
> +  writeGzippedFile: function (o, path, replacer, space, writeOptions) {
> +    let utf8 = JSON.stringify(o, replacer, space);

This is not utf-8.

@@ +51,5 @@
> +    // buffer allocations.
> +    let raw = this._utf8Converter.ConvertFromUnicode(utf8);
> +    raw += this._utf8Converter.Finish();
> +
> +    let gzipped = CommonUtils.convertString(raw, "uncompressed", "gzip");

This is a terrible API. You're not converting strings, you're converting byte streams, so your API should deal with Uint8Arrays or something similar, not strings.

@@ +80,5 @@
> +    OS.File.read(path).then((data) => {
> +      try {
> +        let decoder = new TextDecoder("utf-8");
> +        deferred.resolve(JSON.parse(decoder.decode(data), reviver));
> +      } catch (ex) {

Isn't this promises stuff supposed to catch your exceptions?

@@ +104,5 @@
> +          a.push(String.fromCharCode(dv.getUint8(i)));
> +        }
> +
> +        let gzipped = a.join("");
> +        let raw = CommonUtils.convertString(gzipped, "gzip", "uncompressed");

Again, shouldn't deal with strings
I'm not sure how to convert a Uint8Array (or similar) directly to a gzipped Uint8Array. That's why we do some funky conversion voodoo in the realm of JS strings. I would switch in a heartbeat if I knew how. The MDN docs for all this stuff are horribly inadequate, IMO.
Comment on attachment 8344305 [details] [diff] [review]
Add JSON.jsm for common JSON operations

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

I haven't really looked at the code. However, we should land bug 854169 quite soon. This will add lz4 compression "for free" (in terms of lines of code), off the main thread. I believe that any new module should build on top of this.

If you want me to look at the API design, feel free to f? me again.
Attachment #8344305 - Flags: review?(dteller)
Component: General → Async Tooling
Whiteboard: [Async]
Whiteboard: [Async] → [Async:meeting]
Now that bug 854169 has landed, reading/writing lz4-compressed data is a one-liner.
I suggest WONTFIXing this bug.
Whiteboard: [Async:meeting] → [Async:ready]
I agree.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.