838210 - get rid of some main-thread IO (mobile)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marco Castelluccio [:marco]]

I'm also going to port something to OS.File.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch v2

I've found other code that could be ported to use OS.File.
Should I use a lazy getter to import osfile.jsm and Task.jsm?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 710990 [details] [diff] [review]
Patch v2

In general this looks OK. I would like to see more use of lazy getters though.

I also don't like adding back the "self = this" pattern. We have been using function.bind(this) in new code and removing the old pattern. Can you use function/bind() as well?


Some specifics:

>diff --git a/mobile/android/chrome/content/aboutDownloads.js b/mobile/android/chrome/content/aboutDownloads.js

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      let promise = OS.File.writeAtomic(aFile.path, array, {tmpPath: aFile.path + ".tmp"}).then(null, function onError(reason) {

nit: use a space inside the {}

{ tmpPath: aFile.path + ".tmp" }

>+        OS.File.writeAtomic(this._file.path, array, {tmpPath: this._file.path + ".tmp"});

Same

>+    Task.spawn(function() {
>+      let bytes = yield OS.File.read(aFile.path);
>+      let raw = new TextDecoder("UTF-8").decode(bytes) || "";
> 
>       try {
>         aCallback(JSON.parse(raw));
>       } catch (e) {
>         Cu.reportError("Distribution: Could not parse JSON: " + e);
>       }
>+    }).then(null, function onError(reason) {
>+      if (!(reason instanceof OS.File.Error && reason.becauseNoSuchFile)) {
>+        Cu.reportError("Distribution: Could not read from " + aFile.leafName + " file");
>+      }

Task is a new pattern for me. Why are we using it instead of the promise returned by OS.File.read ?
Are there docs on Task somewhere?

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Task is a new pattern for me. Why are we using it instead of the promise
> returned by OS.File.read ?
> Are there docs on Task somewhere?

It just improves readability, because it makes asynchronous operations look like synchronous ones.
There's some documentation here (http://dxr.mozilla.org/mozilla-central/toolkit/content/Task.jsm.html) and here (http://taskjs.org/).

I'll fix the other things, I've just a doubt about the lazy getters. I don't know when I should use them and when I shouldn't.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

One rule of thumb for lazy getters is this: If the code is not used during startup, it's nice to use a lazy getter so we can defer the cost of loading and parsing the JS.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 712158 [details] [diff] [review]
Patch v3

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+XPCOMUtils.defineLazyGetter(this, "Task", function() {
>+  Cu.import("resource://gre/modules/Task.jsm");
>+  return DebuggerServer;

    return Task;

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

>diff --git a/mobile/android/chrome/content/downloads.js b/mobile/android/chrome/content/downloads.js

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

>diff --git a/mobile/android/components/AddonUpdateService.js b/mobile/android/components/AddonUpdateService.js

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>+XPCOMUtils.defineLazyGetter(this, "Task", function() {
>+  Cu.import("resource://gre/modules/Task.jsm");
>+  return DebuggerServer;

    return Task;

>+XPCOMUtils.defineLazyGetter(this, "OS.File", function() {
>+  Cu.import("resource://gre/modules/osfile.jsm");
>+  return DebuggerServer;

    return OS.File;

Let's get a Try Server run for the next patch. Also, request review from :bnicholson as well. He has worked in many of these files.

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Patch v4

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5558c370aa6b

There are contacts api tests failing, but I guess they're unrelated.

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 784476 [details] [diff] [review]
Patch v4

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

::: mobile/android/components/HelperAppDialog.js
@@ +132,5 @@
>        try {
>          // Remove the file so that it's not there when we ensure non-existence later;
>          // this is safe because for the file to exist, the user would have had to
>          // confirm that he wanted the file overwritten.
> +        file.remove(false);

I assume you're not changing this to OS.File.remove() because of a potential race with the non-existence check described in the comment -- is that correct?

::: mobile/android/components/SessionStore.js
@@ +1002,1 @@
>              notifyObservers("fail");

Move this line outside of the if/else block; notifyObservers() must be called unconditionally after attempting to restore.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch v5

(In reply to Brian Nicholson (:bnicholson) (gone through August 11) from comment #13)
> I assume you're not changing this to OS.File.remove() because of a potential
> race with the non-existence check described in the comment -- is that
> correct?

Exactly.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 785448 [details] [diff] [review]
Patch v5

Sorry for the delay in the review. Looks good.

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Rebased patch

https://tbpl.mozilla.org/?tree=Try&rev=f459d11974e5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b742229dc66c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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