Open Bug 922298 Opened 11 years ago Updated 2 years ago

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

Categories

(Toolkit :: Async Tooling, defect)

defect

Tracking

()

People

(Reporter: jwalker, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Async:ready][Async Tooling])

Attachments

(1 file)

Features: * Returns a promise * Doesn't have any ready state * Doesn't parse XML, or mess with mime types
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|.
Paolo, could some part of your background downloader be used for such a task?
Flags: needinfo?(paolo.mozmail)
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.
Would a simple sane front-end for XHR be sufficient? That's something that could be developed in a matter of days.
Flags: needinfo?(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?
Flags: needinfo?(jwalker)
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)?
Flags: needinfo?(dtownsend+bugmail)
(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?
Flags: needinfo?(dtownsend+bugmail) → needinfo?(jwalker)
(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.
Flags: needinfo?(paolo.mozmail)
(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.
Flags: needinfo?(jwalker)
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|.
Attached patch Modernized NetUtils.jsm (deleted) — Splinter Review
So, what about this?
Attachment #813042 - Flags: feedback?(jwalker)
Attachment #813042 - Flags: feedback?(dtownsend+bugmail)
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?
Attachment #813042 - Flags: feedback?(jwalker) → feedback+
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?
Attachment #813042 - Flags: feedback?(dtownsend+bugmail) → feedback+
(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.
(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?
If my memory serves correctly, the file is opened on the main thread.
Whiteboard: [Async:P2][Async Tooling]
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
Whiteboard: [Async:P2][Async Tooling] → [Async:ready][Async Tooling]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: