Closed Bug 912475 Opened 6 years ago Closed 6 years ago

Use promise instead of custom event for install request from webapp actor

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 4 obsolete files)

When install request has been implemented, there was no promise support.
So that you had to send back a custom event, here `webappsEvent` in order communicate the asynchronous result for this given installation.
We should switch to promise now that the gecko18 branch is frozen.
This patch is on top of bug 914604 that adds various tests for the actor.
Attachment #799467 - Attachment is obsolete: true
Attachment #802313 - Flags: review?(past)
Comment on attachment 802313 [details] [diff] [review]
Use promise for async install request instead of sending events. r=past

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

Looks good!

::: toolkit/devtools/server/actors/webapps.js
@@ +192,5 @@
>  
>          delete aApp.manifest;
> +        aDefer.resolve({ appId: aId, path: aDir.path });
> +
> +        // Keep sending this event for compatility with old clients

I don't believe we care about supporting old clients. The only scenario we want to support is a recent devtools client targeting an old server (probably embedded in a phone).

@@ +265,5 @@
>    installHostedApp: function wa_actorInstallHosted(aDir, aId, aReceipts,
>                                                     aManifest, aMetadata) {
>      debug("installHostedApp");
>      let self = this;
> +    let defer = promise.defer();

Nit: the object created by defer() is commonly called deferred, as we routinely use nouns for objects and verbs for functions. In this case you would also use aDeferred for the formal argument.

@@ +512,5 @@
> +          appDir.remove(true);
> +        } catch(e) {}
> +        return { error: "badParameterType",
> +                 message: "hosted app file and manifest/metadata fields " +
> +                          "are missing" 

Trailing whitespace.
Attachment #802313 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #3)
> ::: toolkit/devtools/server/actors/webapps.js
> @@ +192,5 @@
> >  
> >          delete aApp.manifest;
> > +        aDefer.resolve({ appId: aId, path: aDir.path });
> > +
> > +        // Keep sending this event for compatility with old clients
> 
> I don't believe we care about supporting old clients. The only scenario we
> want to support is a recent devtools client targeting an old server
> (probably embedded in a phone).

CCing Dave, in case I'm misrepresenting our current policy.
I think should assume that developers are using the latest browser on the same channel as the thing they're debugging.

So a nightly b2g build can assume an up-to-date nightly.  Once something hits release, the latest release desktop client should work on it.

So since this is only on nightly and will move to aurora on desktop concurrent with/before it gets to aurora on b2g, I don't think we need extra compat here.
(In reply to Panos Astithas [:past] from comment #3)
> Comment on attachment 802313 [details] [diff] [review]
> I don't believe we care about supporting old clients. The only scenario we
> want to support is a recent devtools client targeting an old server
> (probably embedded in a phone).

Actually I agree with that with most other features being exposed by the remote protocol, except this one, the install method is already being used by external dependencies. Like b2g remote addon from fabrice that has been forked since by some external contributors.
Like this tool:
  https://github.com/arcturus/node-firefoxos-cli

But having look at its source, it wasn't listening for the webappsEvent...

So if you think we shouldn't care I can drop this compatibility layer and try to message projects I see using install request.
Attachment #802313 - Attachment is obsolete: true
Comment on attachment 803008 [details] [diff] [review]
Removed compatibility code and renamed all defer to deferred

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

I modified some other pieces of webapps actor that was using defer instead of deferred.
Also I had to modify some new piece of code, tests, that were listening for webappsEvent.
Attachment #803008 - Flags: review?(past)
Comment on attachment 803008 [details] [diff] [review]
Removed compatibility code and renamed all defer to deferred

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

::: toolkit/devtools/server/actors/webapps.js
@@ +143,5 @@
>      this._actorPool = null;
>      this.conn = null;
>    },
>  
> +  _registerApp: function wa_actorRegisterApp(aDefer, aApp, aId, aDir) {

aDefer -> aDeferred

@@ +203,5 @@
>          aDir.remove(true);
>      });
>    },
>  
> +  _sendError: function wa_actorSendError(aDefer, aMsg, aId) {

Same here.
Attachment #803008 - Flags: review?(past) → review+
Attached patch Renamed aDefer to aDeferred (obsolete) — Splinter Review
Attachment #803008 - Attachment is obsolete: true
Attachment #803060 - Flags: review+
Depends on: 914604
Attachment #805061 - Flags: review+
Note for checkin, it needs to be landed after bug 914604, which is also ready to land.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/09b50d116090
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/09b50d116090
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.