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.

Does this fail when bug 1037235 is applied?

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

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

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

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

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?

(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.

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

I think the Mock story is much better...

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)

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

(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 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 on attachment 8490867 [details] [diff] [review] patch v4 Review of attachment 8490867 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks better now!

Rebased.

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

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