1067424 - Cover webapps actor 'getappActor' request with a test

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Alexandre Poirot [:ochameau]]

I think I finally found a way to provide a somewhat easy and "landable" way of testing the rest of the webapps actor requests we are desperatly missing in test coverage!
We should be able to catch most of our silent regression by writting a mochitest that would override the "AppFrames" variable from webapps actor with a Mock.
That wouldn't cover gaia codebase, so that we wouldn't detect brekages due to gaia regressions, but that would cover 90% of gecko regressions.
I think we were hit only once by a gaia regression, during the window manager refactoring.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

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

Comment 2

[Paul Rouget [:paul]]

Does this fail when bug 1037235 is applied?

Comment 3

[Alexandre Poirot [:ochameau]]

Not this patch v1, but I'm working on another version, using remote frame that does reproduce it.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

So this patch reproduces the XULAppInfo exception,
and also adds a try/catch in child.js in order to see the exception.
(No idea why the exception is trapped by loadFrameScript...)

This patch only covers getAppActor,
but we could continue and cover more.
For ex, try to send a naive request to console actor,
or check appOpen/appClose events.

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

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Green test - https://tbpl.mozilla.org/?tree=Try&rev=876841a707c2

Comment 6

[Alexandre Poirot [:ochameau]]

Try on b2g desktop and emulator:
https://tbpl.mozilla.org/?tree=Try&rev=c8119fd48e76

Comment 7

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

Comment on attachment 8489993 [details] [diff] [review]
patch v3

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

I am so excited to finally get some coverage here! :D

Let's try to think of a better way to set the mock, but it seems great overall.

::: toolkit/devtools/apps/tests/debugger-protocol-helper.js
@@ +126,5 @@
> +addMessageListener("addFrame", function (aMessage) {
> +  let win = Services.wm.getMostRecentWindow("navigator:browser");
> +  let doc = win.document;
> +  let frame = doc.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
> +  frame.setAttribute("mozbrowser", "true");

Why is this here if you can also set via the message a few lines later?  (Also, are there actually other values than "true" that make sense...?)

@@ +150,5 @@
>      gClient.close();
>    });
>  });
>  
> +

Nit: Just one blank line seems fine.

@@ +151,5 @@
>    });
>  });
>  
> +
> +DebuggerServer.AppFramesMock = {

DebuggerServer feels pretty random to me here, since it has nothing to do with loading AppFrames in the non-mock case...  But I guess I don't have any wonderful suggestions.

Maybe if the lazy load of AppFrames moved out of webapps.js to DebuggerServer for real, and then here you're replacing the real one?  That would seem less strange to me.

Or maybe there is a different way to pass the mock to the webapps actor?

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> Comment on attachment 8489993 [details] [diff] [review]
> ::: toolkit/devtools/apps/tests/debugger-protocol-helper.js
> @@ +126,5 @@
> > +addMessageListener("addFrame", function (aMessage) {
> > +  let win = Services.wm.getMostRecentWindow("navigator:browser");
> > +  let doc = win.document;
> > +  let frame = doc.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
> > +  frame.setAttribute("mozbrowser", "true");
> 
> Why is this here if you can also set via the message a few lines later? 
> (Also, are there actually other values than "true" that make sense...?)

Oops, we don't want to make it optional as we will only play with mozbrowser frames.
non-mozbrowser won't have any message manager and won't be debuggable.

There is no particular value for mozbrowser, it is just an attribute that has to be defined:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-mozbrowser

> @@ +151,5 @@
> >    });
> >  });
> >  
> > +
> > +DebuggerServer.AppFramesMock = {
> 
> DebuggerServer feels pretty random to me here, since it has nothing to do
> with loading AppFrames in the non-mock case...  But I guess I don't have any
> wonderful suggestions.
> 
> Maybe if the lazy load of AppFrames moved out of webapps.js to
> DebuggerServer for real, and then here you're replacing the real one?  That
> would seem less strange to me.

I'm not sure it is any better to put some random App related stuff in main.js

> 
> Or maybe there is a different way to pass the mock to the webapps actor?

A more explicit way would be to load the debugger server in a custom loader with a magic module/global.
It would mean hacking Loader.jsm with some random app related stuff...

Or expose an API on webapps.js itself to mock this object. I'll submit another patch with this approach.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Convert webapps actor to SDK module

So in order to ease calling webapps actor method,
I first convert it to SDK module...
That won't be harmful! Instead it should improve lazy loading of actors \o/

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

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

I think the Mock story is much better...

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8490866 [details] [diff] [review]
Convert webapps actor to SDK module

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

Note that this patch depends on the just-landed bug 988237 (might still be only in fx-team)

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Convert webapps actor to SDK module

I missed a require(Services) in webapps.js.
It is frightening that is only failed on mochitests only on b2g and none of other tests :/

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

Comment 13

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

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Created attachment 8491010 [details] [diff] [review]
> Convert webapps actor to SDK module
> 
> I missed a require(Services) in webapps.js.
> It is frightening that is only failed on mochitests only on b2g and none of
> other tests :/

Hmm, yeah, how could the XPCShell test (test_webappsActor.js) have passed?  It's only run on desktop at the moment, but still seems odd.

Comment 14

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

Comment on attachment 8491010 [details] [diff] [review]
Convert webapps actor to SDK module

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

Yay, more laziness!

::: toolkit/devtools/server/actors/webapps.js
@@ +17,5 @@
> +let { ActorPool } = require("devtools/server/actors/common");
> +let { DebuggerServer } = require("devtools/server/main");
> +let Services = require("Services");
> +
> +

Nit: remove extra blank line

@@ +521,5 @@
>            // frame script. That will flush the jar cache for this app and allow
>            // loading fresh updated resources if we reload its document.
>            let FlushFrameScript = function (path) {
> +            let jar = Cc["@mozilla.org/file/local;1"]
> +                                .createInstance(Ci.nsILocalFile);

Nit: re-indent

@@ +526,3 @@
>              jar.initWithPath(path);
> +            let obs = Cc["@mozilla.org/observer-service;1"]
> +                                .getService(Ci.nsIObserverService);

Nit: re-indent

Comment 15

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

Comment on attachment 8490867 [details] [diff] [review]
patch v4

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

Thanks, looks better now!

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Convert webapps actor to SDK module

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Convert webapps actor to SDK module

Rebased.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Comment 20

[Alexandre Poirot [:ochameau]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9e88f96e430f
https://hg.mozilla.org/integration/fx-team/rev/62bec3669030

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e88f96e430f
https://hg.mozilla.org/mozilla-central/rev/62bec3669030