We want to be able to show a progress bar when an app is being uploaded to a runtime. Is it possible to attach a progress listener to the uploader?

We could add a "progress" event to the StreamCopier[1], which is the main path that bulk data goes through to actually copy the data. It already stores state of how far along the transfer is, since it also needs to know when copying is done. Then, we'd want to bubble up this event to the debugger client so that's available to app-actor-front / WebIDE UI, etc. However, don't you really want progress for the non-bulk data path too? I would think it be cleanest to get a "progress" type event from both kinds of app installation. [1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/transport/stream-utils.js#61

> However, don't you really want progress for the non-bulk data path too? I would think it be cleanest to get a "progress" type event from both kinds of app installation. Works for me. Anything that would allow us to build a progress bar when pushing an app.

Paul, does the API here sound good to you? The app actor front will now emit "install-progress" events (for both bulk and non-bulk) during packaged app installs, with the bytes sent so far and the total bytes. I assume that's enough to build whatever UI you'd like here? Alex, I'm flagging you for review, since you're quite familiar with the app actor. There are also subtle transport changes too, but hopefully it's not too confusing. We were previously exposing an API that return a promise during copying. Instead, I now return an object that is a promise (has a |then| method), but also emits events. Try: https://tbpl.mozilla.org/?tree=Try&rev=116f8033bddc

Comment on attachment 8444894 [details] [diff] [review] Progress events for app install Works very well! I wrote a patch to add a progressmeter: bug 972069

Comment on attachment 8444894 [details] [diff] [review] Progress events for app install Review of attachment 8444894 [details] [diff] [review]: ----------------------------------------------------------------- You were right about how "subtle" the promise/event-emitter thing was, but looks good ;) I just have one concern about the install-progress event. ::: toolkit/devtools/apps/app-actor-front.js @@ +260,5 @@ > } > exports.installPackaged = installPackaged; > > +function emitInstallProgress(bytesSent, totalBytes) { > + AppActorFront.emit("install-progress", bytesSent, totalBytes); It is weird to emit such event on the app actor itself, as it is related to a given install process. It doesn't match perfectly the App client code I'm trying to introduce via bug 1025828. The App actor client/front will be instanciable, but that's something I can easily adapt once you land. I was excepting to listen this events through installPackage() somehow. What about also returning this weird promise/event emitter thing or passing a callback? ::: toolkit/devtools/client/dbg-client.jsm @@ +938,5 @@ > * @return Promise > * The promise is resolved when copying completes or rejected > * if any (unexpected) errors occur. > + * This object also emits "progress" events for each chunk > + * that is copied. See stream-utils.js. OMG, can't we write this doc once and refer to it instead of copy pasting such comments n-times in the same file?

(In reply to Alexandre Poirot (:ochameau) from comment #5) > It is weird to emit such event on the app actor itself, > as it is related to a given install process. > It doesn't match perfectly the App client code I'm trying to introduce via > bug 1025828. > The App actor client/front will be instanciable, but that's something I can > easily adapt once you land. > I was excepting to listen this events through installPackage() somehow. > What about also returning this weird promise/event emitter thing or passing > a callback? Yes, I agree it would be more natural for the events to come from an object returned by installPackage directly. It's a little harder to do today, because app-actor-front's install path (installPackaged) involves lots of little global functions calling each other. This would need to change into a single PackagedAppInstaller object (or something), so that we emit the progress events on the right instance. Such a change might also conflict with what you are doing in bug 1025828. Should I try to do this now? Or file a follow up as further cleanup of the actor front? If you look at Paul's patch to use the progress event, it does almost seem easier to just bind it on the entire actor (the way it is right now), since you just bind and you're done, but like I said I agree it's surprising. > ::: toolkit/devtools/client/dbg-client.jsm > @@ +938,5 @@ > > * @return Promise > > * The promise is resolved when copying completes or rejected > > * if any (unexpected) errors occur. > > + * This object also emits "progress" events for each chunk > > + * that is copied. See stream-utils.js. > > OMG, can't we write this doc once and refer to it instead of copy pasting > such comments n-times in the same file? But each giant block is slightly different, just to increase confusion! ;) I should probably write some kind of external doc about bulk data... Not sure, but yes, there is room for improvement for sure.

(In reply to J. Ryan Stinnett [:jryans] from comment #6) > (In reply to Alexandre Poirot (:ochameau) from comment #5) > > It is weird to emit such event on the app actor itself, > > as it is related to a given install process. > > It doesn't match perfectly the App client code I'm trying to introduce via > > bug 1025828. > > The App actor client/front will be instanciable, but that's something I can > > easily adapt once you land. > > I was excepting to listen this events through installPackage() somehow. > > What about also returning this weird promise/event emitter thing or passing > > a callback? > > Yes, I agree it would be more natural for the events to come from an object > returned by installPackage directly. > > It's a little harder to do today, because app-actor-front's install path > (installPackaged) involves lots of little global functions calling each > other. This would need to change into a single PackagedAppInstaller object > (or something), so that we emit the progress events on the right instance. > > Such a change might also conflict with what you are doing in bug 1025828. > > Should I try to do this now? Or file a follow up as further cleanup of the > actor front? We can do a followup and tweak things once we have a client. My only concern in landing as-is and later stick to emitting global events, we may face issues with events being mixed up if we upload multiple apps. But given RDP behavior, it is unlikely we are ever try to push multiple app simultaneously.

(In reply to Alexandre Poirot (:ochameau) from comment #7) > (In reply to J. Ryan Stinnett [:jryans] from comment #6) > > (In reply to Alexandre Poirot (:ochameau) from comment #5) > > > It is weird to emit such event on the app actor itself, > > > as it is related to a given install process. > > > It doesn't match perfectly the App client code I'm trying to introduce via > > > bug 1025828. > > > The App actor client/front will be instanciable, but that's something I can > > > easily adapt once you land. > > > I was excepting to listen this events through installPackage() somehow. > > > What about also returning this weird promise/event emitter thing or passing > > > a callback? > > > > Yes, I agree it would be more natural for the events to come from an object > > returned by installPackage directly. > > > > It's a little harder to do today, because app-actor-front's install path > > (installPackaged) involves lots of little global functions calling each > > other. This would need to change into a single PackagedAppInstaller object > > (or something), so that we emit the progress events on the right instance. > > > > Such a change might also conflict with what you are doing in bug 1025828. > > > > Should I try to do this now? Or file a follow up as further cleanup of the > > actor front? > > We can do a followup and tweak things once we have a client. > My only concern in landing as-is and later stick to emitting global events, > we may face issues with events being mixed up if we upload multiple apps. > But given RDP behavior, it is unlikely we are ever try to push multiple app > simultaneously. Okay, filed bug 1030977 for the follow up. I think it will be easier to make this change once your work in bug 1025828 is done.

Updated to emit an object instead of separate numbers. Try: https://tbpl.mozilla.org/?tree=Try&rev=891118db2cee