1025828 - Factorize various webapps actor code into a webapps client

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Alexandre Poirot [:ochameau]]

Over time, we put various client side code related to webapps actor in various modules.
Sometime with important workarounds/implementation details.
In order to ease app actors creation for the monitor, we should factorize this code into what looks like a front, à la protocol.js, even if the webapps actor isn't using protocol.js.

Here is an example of this weak client code.
Here in app-manager.js, we listen for appOpen,appClose,... events:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/modules/app-manager.js#137
But watchApps isn't explicitely called. We rely on the fact that somewhere else, we are calling it:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/webapps-store.js#89
We can see subtle copy paste between these two files. And for the upcoming monitor panel, we would have been either hacking into AppManager internals or continue copy pasta.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: implement the front

This patch just duplicate the code a 3rd time.
I would like to provide a distinct patch after this one is r+,
to start factorizing app-manager.js and webapps-store.js.
Otherwise for this patch, I also would like to get rid of all these naive functions
(getAppActor, getAppInfo, listInstalledApps, listRunningApps)
that just map client.request to a promise.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: gaia tests

Last but not least, tests covering webapps actor!!
We can even start covering getAppActor \o/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Created attachment 8440591 [details] [diff] [review]
> gaia tests
> 
> Last but not least, tests covering webapps actor!!
> We can even start covering getAppActor \o/

Yay, I'm very happy to see this coverage! :D

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This refactoring seems to ignore the install paths, like |installPackaged|, at the moment.  Should those be moved onto |AppActorFront.prototype| too?

Comment 5

[Alexandre Poirot [:ochameau]]

Yes! In this patch I'm focusing on monitor actor needs, but that client should be augmented whenever we need something.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8440590 [details] [diff] [review]
implement the front

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

This patch needs to be rebased on top of bug 1025799 (which landed) because it introduced a `const AppActorFront` event emitter.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: implement a client

rebased.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: implement the client

This patch intentionaly implements just what the monitor needs,
I would like to add new features only when explicitely needed,
when we are going to refactor existing code to use the client.

I thought about renaming app-actor-front to app-actor-client,
and AppActorClient, as of today, this implementation is more like
a "Client" than a "Front", but there is still a chance we are going 
to switch to protocol.js so we may just end up renaming it again to Front...

https://tbpl.mozilla.org/?tree=Try&rev=ba6e5b837e49

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Test in gaia

And the gaia tests, that pass locally.
I would like to wait for the gecko patch to reach m-c before pushing this to gaia-try before landing this gaia patch.

In this patch I'm clearly working around integration test limitations...
for the sake of having clear test script and not use scary executeScriptAsync and marionetteFinish hacks over the whole test code.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8452340 [details] [diff] [review]
implement the client

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

Looks great, should clean up a lot of things! :D

Are you planning to clean up existing caller, like WebIDE, in a later patch?

::: toolkit/devtools/apps/app-actor-front.js
@@ +333,5 @@
>  }
>  exports.getTargetForApp = getTargetForApp;
>  
>  function reloadApp(client, webappsActor, manifestURL) {
> +  return getTargetForApp(client,

Realign the 2 args below this.

@@ +384,5 @@
> +    deferred.reject(error);
> +  });
> +  return deferred.promise;
> +}
> +

Nit: Remove extra line.

@@ +448,5 @@
> +
> +  this._listeners = [];
> +}
> +
> +AppActorFront.prototype = {

Does it make sense to move even more operations onto here, like the install methods?

@@ +629,5 @@
> +  }
> +}
> +
> +exports.AppActorFront = AppActorFront;
> +EventEmitter.decorate(AppActorFront);

Now that there's a real front object, it's probably more logical for the |install-progress| events to be emitted on the particular front instance that WebIDE is using, right?

But that would come later when existing callers are cleaned up to use this new style.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> @@ +448,5 @@
> > +
> > +  this._listeners = [];
> > +}
> > +
> > +AppActorFront.prototype = {
> 
> Does it make sense to move even more operations onto here, like the install
> methods?

Ah, I guess your above comment says "no, not for now".  Seems fine.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8452343 [details] [review]
Test in gaia

The DevTools parts of this seem fine to me, and I'm really happy to see this tested!  But you are definitely doing some strange things to the test framework, so someone else who knows more about the testing framework should review this to be sure it's okay.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: implement the client

Fixed EventEmitter interface, tweak indentation and spaces.

(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> Comment on attachment 8452340 [details] [diff] [review]
> @@ +629,5 @@
> > +  }
> > +}
> > +
> > +exports.AppActorFront = AppActorFront;
> > +EventEmitter.decorate(AppActorFront);
> 
> Now that there's a real front object, it's probably more logical for the
> |install-progress| events to be emitted on the particular front instance
> that WebIDE is using, right?
> 
> But that would come later when existing callers are cleaned up to use this
> new style.

You are right and I'm wrong here in this patch!
I either need to change these callsites or let it as-is here and emit on exports.
I tend to prefer keeping it as-is and refactor the callsites in a followup.

New try:
https://tbpl.mozilla.org/?tree=Try&rev=c425e62781bf

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> (In reply to J. Ryan Stinnett [:jryans] from comment #10)
> > @@ +448,5 @@
> > > +
> > > +  this._listeners = [];
> > > +}
> > > +
> > > +AppActorFront.prototype = {
> > 
> > Does it make sense to move even more operations onto here, like the install
> > methods?
> 
> Ah, I guess your above comment says "no, not for now".  Seems fine.

Not in this patch, but yes, in followups while refactoring existing code.


(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> Are you planning to clean up existing caller, like WebIDE, in a later patch?

Yes, if noone does that while I'm off ;)

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8452343 [details] [review]
Test in gaia

Gareth, would you be confident reviewing the "test framework" part of this patch?
I know it is kind of strange and looks like a test framework inside a test,
but that allows us to write decent tests against gecko codebase, that,
without having to entangle many executeAsyncScript/marionetteFinished in middle of the test script.
I'm not even sure it is even possible to write such test that listen to so various events at once by using only executeAsyncScript.

Ideally integration tests should be able to easily execute the test scripts on gecko side (instead of node). Marionette API isn't meant to write unit test, but more to write test frameworks and tools on top of it. Everything, not only this gecko test, would be easier if test scripts were running on gecko.
We wouldn't have this weird executeAsync/marionetteFinish thing, we wouldn't have to bind stuff on window to workaround. we wouldn't have this false async method that is sync. we wouldn't play with some proxy object but with real DOM objects. And last but not least it would be way easier to deal with anything being asynchronous: DOM events, callback, promises,...

Here I'm just working around all that. It is only meant to be used in this test.
I wish we could expose something similar to this and use it in any test easily.

Comment 16

[Gareth Aye [:gaye] (back from PTO)]

(In reply to Alexandre Poirot (:ochameau) from comment #15)
> Comment on attachment 8452343 [details] [review]
> Test in gaia
> 
> Gareth, would you be confident reviewing the "test framework" part of this
> patch?
> I know it is kind of strange and looks like a test framework inside a test,
> but that allows us to write decent tests against gecko codebase, that,
> without having to entangle many executeAsyncScript/marionetteFinished in
> middle of the test script.
> I'm not even sure it is even possible to write such test that listen to so
> various events at once by using only executeAsyncScript.
> 
> Ideally integration tests should be able to easily execute the test scripts
> on gecko side (instead of node). Marionette API isn't meant to write unit
> test, but more to write test frameworks and tools on top of it. Everything,
> not only this gecko test, would be easier if test scripts were running on
> gecko.
> We wouldn't have this weird executeAsync/marionetteFinish thing, we wouldn't
> have to bind stuff on window to workaround. we wouldn't have this false
> async method that is sync. we wouldn't play with some proxy object but with
> real DOM objects. And last but not least it would be way easier to deal with
> anything being asynchronous: DOM events, callback, promises,...
> 
> Here I'm just working around all that. It is only meant to be used in this
> test.
> I wish we could expose something similar to this and use it in any test
> easily.

Sure! Reading now

Comment 17

[James Lal [:lightsofapollo]]

My 2 cents (as the original author of most of the marionette js stuffs):

The use case you have makes perfect sense to me but the marionette-js-runner stuff in particular was designed around testing UI interactions (+ using some node stuff like http servers, etc... for those interactions). It does those things fairly well and most tests we have fall into those categories (generally falling into the executeAsyncScript pattern your describing above means something is missing in marionette or the test is trying to express something outside of the UI). 

Short term gluing these things together probably makes sense but long term we might be better off splitting these tests off into their own test suite (these look similar to me to mochitest chrome and webapi tests).

Comment 18

[Gareth Aye [:gaye] (back from PTO)]

What :lightsofapollo said :)

Comment 19

[Alexandre Poirot [:ochameau]]

checkin-needed for the gecko patch, attachment 8452392 [details] [diff] [review].

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to James Lal [:lightsofapollo] Unvailable until 6/30 from comment #17)
> The use case you have makes perfect sense to me but the marionette-js-runner
> stuff in particular was designed around testing UI interactions (+ using
> some node stuff like http servers, etc... for those interactions). It does
> those things fairly well ...

I totally agree on that, it is really handy when it comes to launch/stop b2g manually and have a fine control over its profile. And yes, when you have to run something else, outside of b2g, like http/imap server, it is made for this! That's sounds like very useful and important tests to have for some apps.

> ... and most tests we have fall into those categories

This is all but true! Take a look at all existing tests, really.

1/ More and more of them, if not the vast majority now, open *one* app, do many assertion against this sole app and close the app.
2/ Many of them do not require anything being made in node, no special server to run.
3/ For most of them, the value of creating a new empty profile and relaunching b2g is null. The vast majority just want some pref/settings to be set before running the test and unset after. We increase test running time for no reason.

> (generally falling into the executeAsyncScript pattern your describing above
> means something is missing in marionette or the test is trying to express
> something outside of the UI). 

It is kind of hiding dust under the carpet.
We are hiding execuseAsyncScript tricks in marionette-apps helper code :/
executeAsyncScript trick is a broken practice, really.
I'm convinced many intermittents are related to this.
See bug 1035928 comment 1.

> Short term gluing these things together probably makes sense but long term
> we might be better off splitting these tests off into their own test suite
> (these look similar to me to mochitest chrome and webapi tests).

That sounds fine. If you want to let gaia-integration tests as-is for the original tests purposes that's fine.
I just wish you would be more aware of the undercover issues lying around gaia-integratios tests.
The issues I'm highlighting here and in more detail in bug 1035928 are still going to exists.
My other concern is that I think we already have too many test suites, especially around gaia and I'm really scared to spawn yet another one.

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8452392 [details] [diff] [review]
implement the client

Carrying r+ from comment 12.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d8f5695199ef

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8f5695199ef