This repository has been archived by the owner. It is now read-only.

change runApp command to use mozApps API and app origin #304

Merged
merged 5 commits into from Feb 13, 2013

Conversation

Projects
None yet
2 participants
Member

rpl commented Feb 11, 2013

Change simulatorActor runApp command to use app origin to find and launch an installed app.
This change prevents the b2g-instance to exit if the application is not currently installed and doesn't launch the wrong app if you app share the same name.

NOTE: runApp doesn't always show the app when the simulator instance
is not already running, because the homescreen is not yet fully loaded and
the app will start in background (reachable by pressing the home button to reach
the gaia tasks list)

@mykmelez mykmelez commented on the diff Feb 12, 2013

addon/lib/simulator.js
@@ -124,13 +124,15 @@ let simulator = module.exports = {
}
console.log("Stored " + JSON.stringify(apps[webappFile]));
- this.updateApp(webappFile, function next(error, appId) {
+ this.updateApp(webappFile, function next(error, app) {
@mykmelez

mykmelez Feb 12, 2013

Owner

This and the other next functions have been all been made to take an app object as their second argument, which seems fine. But updateApp still passes those functions an app ID if there is an error. Granted, none of the next functions do anything with their second argument if there is an error. But we should still be consistent about what we pass, so all callers pass the same thing, and the next functions always know what to expect.

@mykmelez mykmelez commented on an outdated diff Feb 12, 2013

addon/lib/simulator.js
@@ -251,7 +253,7 @@ let simulator = module.exports = {
if (res.error) {
next(res.error + ": "+res.message, res.appId);
} else {
- next(null, res.appId);
+ next(null, config);
@mykmelez

mykmelez Feb 12, 2013

Owner

The change doesn't merge automatically, and I'm getting a strange conflict when I try to merge manually:

      archiveDir(archiveFile, sourceDir, function(error) {
        if (error) {
          if (next) {
<<<<<<< HEAD
            next(error);
=======
            // detect success/error and report to the "next" callback
            if (res.error) {
              next(res.error + ": "+res.message, res.appId);
            } else {
              next(null, config);
            }
>>>>>>> 514ebde8246e229a7d6f80ef4b7ec8f49aa31ca6

It isn't clear where the chunk from 514ebde8246e229a7d6f80ef4b7ec8f49aa31ca6 is coming from, but it does seem to be in the wrong place, as archiveDir()'s callback doesn't have a res parameter, it has an error parameter.

There is a code block like the one in the chunk a few lines down from there. But that one is indented twenty spaces rather than twelve. Nevertheless, it doesn't contain the necessary change, so I'm guessing that it is where the change should be made, and Git's merge algorithm somehow got it wrong (and changed the indent in the process?).

@mykmelez mykmelez commented on an outdated diff Feb 12, 2013

addon/lib/simulator.js
@@ -677,6 +684,13 @@ let simulator = module.exports = {
}
},
+ runApp: function(appOrigin, next) {
+ let cmd = (function () {
+ this.remoteSimulator.runApp(appOrigin, next);
+ }).bind(this);
+ this.run(cmd);
@mykmelez

mykmelez Feb 12, 2013

Owner

I don't mind binding functions. In fact, I rather like it. But we don't do it much in this module. Since simulator is a singleton, we instead typically just reference it directly, i.e.:

simulator.remoteSimulator.runApp(appOrigin, next);

So I wonder if this bind is strictly necessary.

@mykmelez mykmelez commented on an outdated diff Feb 12, 2013

prosthesis/content/simulator-actors.js
@@ -61,19 +61,72 @@ SimulatorActor.prototype = {
},
onRunApp: function(aRequest) {
- log("simulator actor received a 'runApp' command");
+ log("simulator actor received a 'runApp' command:"+aRequest.origin);
@mykmelez

mykmelez Feb 12, 2013

Owner

Nit: add whitespace around plus operator, i.e.:

log("simulator actor received a 'runApp' command:" + aRequest.origin);

rpl added some commits Feb 11, 2013

try to runApp after manual install/update/undo an app
NOTE: currently runApp doesn't always show the app when the simulator instance
is not already running, because the homescreen is not yet fully loaded and
the app will start in background (reachable by pressing the home button to reach
the gaia tasks list)
Member

rpl commented Feb 12, 2013

@mykmelez: how does this look with the suggested changes (and fix from commit 7f190d5)?

Owner

mykmelez commented Feb 13, 2013

Looks great, thanks @rpl!

mykmelez added a commit that referenced this pull request Feb 13, 2013

Merge pull request #304 from rpl/feature-mozApps-runApp
change runApp command to use mozApps API and app origin

@mykmelez mykmelez merged commit 9a13fc8 into mozilla:master Feb 13, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.