For now, the webapps actor requires us to use adb in order to copy the webapp package to the device. That introduce complex scenario where we would need to find adb on file system, call it, ensure that it worked and then call the actor install method. That represent a bunch of OS specific code that requires to be strengthen. We shouldn't depend on adb as we may also in future communicate over wifi or any other communication channel other than adb.

Comment on attachment 794020 [details] [diff] [review] Add an actor to upload file from client to server Review of attachment 794020 [details] [diff] [review]: ----------------------------------------------------------------- See bug 908205 for usage of such actor. I've decided to implement a new actor for such feature, it allows to avoid having a huge webapps actor and this feature may be reused for other similar usage outside of webapps. Having said that, I'm fine doing otherwise as soon as we don't complexify the install method, that is already complex enough. (You can see in bug 908205, attachment 794039 [details] [diff] [review] that thanks to this actor, the addition of such upload file feature to the webapps actor is really simple)

Comment on attachment 794020 [details] [diff] [review] Add an actor to upload file from client to server Review of attachment 794020 [details] [diff] [review]: ----------------------------------------------------------------- The names "FileUploadActor" and "UploadActor" don't really suggest any meaningful distinction. Out of curiousity, how does the encoding of binary data work in the JSON stream? We've discussed before the need for a way to efficiently carry bulk data via the remote protocol. Here's an idea I had a while back for a protocol extension that allows "zero copy" data transfer: https://bugzilla.mozilla.org/show_bug.cgi?id=797639

(In reply to Jim Blandy :jimb from comment #3) > Comment on attachment 794020 [details] [diff] [review] > Add an actor to upload file from client to server > > The names "FileUploadActor" and "UploadActor" don't really suggest any > meaningful distinction. I was about to rename the actor to FileActor so that we may add another kind of request related to files. We can imagine `download` request for example. What do you think about naming the main actor: FileActor and keeping FileUploadActor for the other one. > > Out of curiousity, how does the encoding of binary data work in the JSON > stream? I don't know enough about strings, but both read and write uses strings to transfer data. And I think we end up with just a regular JS string in our JSON. > > We've discussed before the need for a way to efficiently carry bulk data via > the remote protocol. Here's an idea I had a while back for a protocol > extension that allows "zero copy" data transfer: > https://bugzilla.mozilla.org/show_bug.cgi?id=797639 That's interesting as the current implementation is really slow. I'd happily switch to use bulk data packets if it ends up being implemented. Is there a common agreement how what should be done. I see many very different proposal in this bug.

Comment on attachment 794020 [details] [diff] [review] Add an actor to upload file from client to server Review of attachment 794020 [details] [diff] [review]: ----------------------------------------------------------------- We discussed this in person last week, so I'll just post some of those points for posterity. One other thing that I notice in the webapp-related actors, is that they do not provide easy wrapper methods to the client that perform these packet requests. As it is now, the client must be familiar with the protocol details in order to install an app, which is suboptimal. This is not something that should be fixed in this bug, but I think it would be a worthwhile investment for a followup. ::: toolkit/devtools/server/actors/fileupload.js @@ +12,5 @@ > +/** > + * Creates a FileUploadActor. This actor allows to upload a file > + * from the client to the server. > + */ > +function FileUploadActor(aConnection) { As discussed, merging FileUploadActor into WebappsActor makes the most sense to me, so that the latter grows an additional "upload" request handler. @@ +76,5 @@ > + > +function UploadActor(aFile) { > + this._file = aFile; > + this.size = 0; > + this._ostream = FileUtils.openSafeFileOutputStream(aFile); OS.File is the preferred way now for doing off main thread I/O: https://developer.mozilla.org/docs/JavaScript_OS.File ::: toolkit/devtools/server/tests/unit/test_fileuploadactor.js @@ +5,5 @@ > +Components.utils.import("resource://gre/modules/FileUtils.jsm"); > + > +let {defer} = devtools.require("sdk/core/promise"); > + > +function TestFileReaderActor(aConnection) { Can you add a short comment about the purpose of this test on top of this file, please?

One other thing to consider is that the webapps actor uses an event to notify when the "install" request is actually finished, whereas other actors use promises in request handlers, so that a response is sent when the request is completed. Might make client-side code easier to write.

(In reply to Panos Astithas [:past] from comment #5) > ::: toolkit/devtools/server/actors/fileupload.js > @@ +12,5 @@ > > +/** > > + * Creates a FileUploadActor. This actor allows to upload a file > > + * from the client to the server. > > + */ > > +function FileUploadActor(aConnection) { > > As discussed, merging FileUploadActor into WebappsActor makes the most sense > to me, so that the latter grows an additional "upload" request handler. Ok, then I'll merge this patch with bug 908205.