801611 - In some cases, WebApps require synchronous startup file I/O

Status: RESOLVED DUPLICATE of bug 838210

(Firefox for Android Graveyard :: General, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

I can see this at least in the mobile version: http://hg.mozilla.org/mozilla-central/annotate/942ed5747b63/mobile/android/chrome/content/browser.js#l7440

It is my understanding that these code paths are not used yet, but should be used within a few months. We should get rid of this.

Comment 1

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

What's the replacement for readInputStreamToString ?

Comment 2

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

Note that Webapps.jsm also uses the method in some startup code:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> What's the replacement for readInputStreamToString ?

To read the contents of a file to a string, asynchronously, off main thread:

let decoder = new TextDecoder(); // Can be reused
OS.File.read(path).then(
  function onSuccess(array) {
    return decoder.decode(array);
  }
);

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I will try and fix this.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Looks like I won't have time to do this. Rebranding as a mentored bug.

Comment 6

[groodt]

If I understand this bug correctly, the idea is to replace the synchronous call to readInputStreamToString with the async version?

Comment 7

[[:fabrice] Fabrice Desré]

Yep. To use TextDecoder, you may need to use OS.File.read() instead of what we do currently.

Comment 8

[groodt]

Ok, I think I understand. How can I tell that I've got it working? Will the application fail to startup or is there something specific I need to test for?

Comment 9

[[:fabrice] Fabrice Desré]

Given that this is in the install path, this will make tests fail.

Comment 10

[groodt]

Attachment: First attempt at async loading

Comment 11

[groodt]

I've made an attempt at using the async loading mechanism. I'm not sure how to test that it is loading asynchronously, but running it on my phone does work. If this is the correct approach, I will do the same thing for Webapps.jsm

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 713596 [details] [diff] [review]
First attempt at async loading

Taking the r? for the time being.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 713596 [details] [diff] [review]
First attempt at async loading

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

Looks good to me.
Passing to mfinkle.

::: mobile/android/chrome/content/browser.js
@@ +8275,2 @@
>        try {
> +        let decoder = new TextDecoder(); // Can be reused

I guess that's a "could be reused", since you don't actually reuse it.

Comment 14

[groodt]

Yes, we can get rid of the comment.

Should I try the same approach in Webapps.jsm? Or leave that as a separate bug? I'm not sure how I would go about testing any changes to Webapps.jsm

Comment 15

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

Comment on attachment 713596 [details] [diff] [review]
First attempt at async loading

I think bug 838210 is overlapping this bug/patch. This section of code has changed a good bit too and will need to be rebased. Take a look at bug 838210 to see if we can consolidate.

r- until we are sure we want this patch alone.

Comment 16

[Marco Castelluccio [:marco]]

I think this is a duplicate of bug 838210.