922298 - We should have something like OS.File.read to take the place of XHR

Status: NEW

(Toolkit :: Async Tooling, defect)

Description

[Joe Walker [:jwalker]]

Features:
* Returns a promise
* Doesn't have any ready state
* Doesn't parse XML, or mess with mime types

Comment 1

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

So, what exactly should it do? I gather from irc that you want it to read from any url?
Side-note: sounds like we should call this |wget|.

Comment 2

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

Paolo, could some part of your background downloader be used for such a task?

Comment 3

[Joe Walker [:jwalker]]

Specifically what I would like is something like:

return OS.wget('resource://gre/modules/devtools/thing.html').then((text) => {
  let node = document.createElement('div');
  node.innerHTML = text;
  return node;
});

The 3 lines in the middle are irrelevent (this is just an example) so really it's only the first line that's interesting.
My XHR version replaces that initial line with 14 lines of code, so it's quite a win. Personally I was only interested in firefox resources, but I can see this being useful for general internet stuff too.

Comment 4

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

Would a simple sane front-end for XHR be sufficient? That's something that could be developed in a matter of days.

Comment 5

[Joe Walker [:jwalker]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Would a simple sane front-end for XHR be sufficient? That's something that
> could be developed in a matter of days.

I didn't have any needs that couldn't be met by more code and XHR.

One thing that bothered me was the text/non-text reply variation. Node (with fs.readFile) returns you a Buffer unless you specify an encoding in which case it returns a string which feels a bit hacky to me. Especially when we are told the encoding by the stream (mostly) Maybe this should be called fetchText or something?

Comment 6

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

Mossop, do you think that there would be room in toolkit/ for a "sane" JS API on top of XHR (both for the main thread and for workers)?

Comment 7

[Dave Townsend [:mossop]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> Mossop, do you think that there would be room in toolkit/ for a "sane" JS
> API on top of XHR (both for the main thread and for workers)?

I guess my question would be how much is it going to get used? I don't recall having come across the need myself. Joe, what sorts of things are you using this for?

Comment 8

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Paolo, could some part of your background downloader be used for such a task?

There is some similarity with the Downloads API:

Components.utils.import("resource://gre/modules/Downloads.jsm");
Downloads.fetch("http://www.mozilla.org/", "/tmp/mozilla.html")
         .then(() => alert("Success!"), ex => alert(ex));

There is another technique that allows cancellation and progress reports. However, the Downloads API is specifically designed to save to a file in the background, and treats the source as an octet stream. Keeping the data in memory and decoding text based on the source specifications may have different requirements. Unless cancellation and progress are needed, there is probably no point in reusing the Downloads code.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Dave Townsend (:Mossop) from comment #7)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> > Mossop, do you think that there would be room in toolkit/ for a "sane" JS
> > API on top of XHR (both for the main thread and for workers)?
> 
> I guess my question would be how much is it going to get used? I don't
> recall having come across the need myself. Joe, what sorts of things are you
> using this for?

I'm using it to load a snippets of HTML and CSS on demand, but I'm irrelevant.

$ grep "@mozilla.org/xmlextras/xmlhttprequest;1" mozilla-central | wc -l
61

That's indiscriminate and misses other ways to do the same thing. But it pales into insignificance compared to uses of NetUtil, for example. So it could be a niche thing, depending on our definition of niche.

Comment 10

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

What about the following?
1. We modernize NetUtils.jsm by adding methods that return |Promise||;
2. Along the way, we add methods |fetchString| and |fetchBuffer|.

Comment 11

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

Attachment: Modernized NetUtils.jsm

So, what about this?

Comment 12

[Joe Walker [:jwalker]]

Comment on attachment 813042 [details] [diff] [review]
Modernized NetUtils.jsm

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

It looks as though fetchString would work just fine for my needs. Thanks.
I noted a couple of trivial things, but I'm not the person to rule on the question of whether this is a good idea in the wider context of NetUtils.

::: netwerk/base/src/NetUtil.jsm
@@ +116,5 @@
> +     * aSink (an output stream).  The copy will happen on some
> +     * background thread. Both streams will be closed when the copy
> +     * completes.
> +     *
> +     * NOTE: This method is not efficient for copying files for for

s/for for/or for/

@@ +134,5 @@
> +     *         The Promise itself resolves once the copy is complete or rejects
> +     *         with an Error in case of error.
> +     */
> +    copy: function (aSource, aSync) {
> +        let deferred = Promise.defer();

Do we care about a mix of 2 and 4 space tabs?

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 813042 [details] [diff] [review]
Modernized NetUtils.jsm

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

I don't have a problem with this, I just don't know how widely useful it will be. I skimmed over existing users of xmlhttprequest in the source, of the 20 or so that I looked at this function would only be able to replace one of them. The others do more custom stuff like loading json/xml or setting custom load flags.

::: netwerk/base/src/NetUtil.jsm
@@ +45,5 @@
> +     * OS.File.copy or Download.jsm.
> +     *
> +     * NOTE: This method is considered deprecated. You should
> +     * generally use the more modern NetUtil.copy instead of
> +     * NetUtil.asyncCopy.

NetUtil.copy just calls asyncCopy, so why is it better and why is this deprecated?

@@ +133,5 @@
> +     *         be launched.
> +     *         The Promise itself resolves once the copy is complete or rejects
> +     *         with an Error in case of error.
> +     */
> +    copy: function (aSource, aSync) {

s/aSync/aSink/ or this is just confusing

@@ +134,5 @@
> +     *         The Promise itself resolves once the copy is complete or rejects
> +     *         with an Error in case of error.
> +     */
> +    copy: function (aSource, aSync) {
> +        let deferred = Promise.defer();

Yes, for better or for worse

@@ +238,5 @@
>      /**
> +     * Asynchronously opens a source and fetches its contents.
> +     *
> +     * NOTE: This method will work with instances of nsIFile but you should
> +     * prefer OS.File.read, which is more efficient.

It doesn't seem smart to create a method that is instantly deprecated for one particular argument type. Why not just reject attempts to use nsIFile immediately?

Comment 14

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

(In reply to Dave Townsend (:Mossop) from comment #13)
> ::: netwerk/base/src/NetUtil.jsm
> @@ +45,5 @@
> > +     * OS.File.copy or Download.jsm.
> > +     *
> > +     * NOTE: This method is considered deprecated. You should
> > +     * generally use the more modern NetUtil.copy instead of
> > +     * NetUtil.asyncCopy.
> 
> NetUtil.copy just calls asyncCopy, so why is it better and why is this
> deprecated?

Well, we are trying to make async code use Promise instead of ad-hoc async-plus-don't-forget-to-check-the-status, which is more error-prone and leads to unreadable nests of code for even trivial stuff. We don't need to deprecate asyncCopy, but I believe that we should at least recommend the Promise-based version.

> 
> @@ +133,5 @@
> > +     *         be launched.
> > +     *         The Promise itself resolves once the copy is complete or rejects
> > +     *         with an Error in case of error.
> > +     */
> > +    copy: function (aSource, aSync) {
> 
> s/aSync/aSink/ or this is just confusing

Perhaps I should sleep more and code less.

> 
> @@ +134,5 @@
> > +     *         The Promise itself resolves once the copy is complete or rejects
> > +     *         with an Error in case of error.
> > +     */
> > +    copy: function (aSource, aSync) {
> > +        let deferred = Promise.defer();
> 
> Yes, for better or for worse

Sleep more, code less, gotcha.


> @@ +238,5 @@
> >      /**
> > +     * Asynchronously opens a source and fetches its contents.
> > +     *
> > +     * NOTE: This method will work with instances of nsIFile but you should
> > +     * prefer OS.File.read, which is more efficient.
> 
> It doesn't seem smart to create a method that is instantly deprecated for
> one particular argument type. Why not just reject attempts to use nsIFile
> immediately?

That's definitely a possibility. Or we could call OS.File.read. I don't know which is better.

Comment 15

[Dave Townsend [:mossop]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> (In reply to Dave Townsend (:Mossop) from comment #13)
> > @@ +238,5 @@
> > >      /**
> > > +     * Asynchronously opens a source and fetches its contents.
> > > +     *
> > > +     * NOTE: This method will work with instances of nsIFile but you should
> > > +     * prefer OS.File.read, which is more efficient.
> > 
> > It doesn't seem smart to create a method that is instantly deprecated for
> > one particular argument type. Why not just reject attempts to use nsIFile
> > immediately?
> 
> That's definitely a possibility. Or we could call OS.File.read. I don't know
> which is better.

If we allowed nsIFile then all NetUtil does is convert that to a URI and then open a channel to it. Does that actually cost main thread I/O?

Comment 16

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

If my memory serves correctly, the file is opened on the main thread.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Perhaps it's not quite addressing the same use case at this point, but see also http://mxr.mozilla.org/mozilla-central/source/services/common/rest.js