Closed Bug 1007218 Opened 10 years ago Closed 10 years ago

Ability to execute App Manager V2 commands from shell

Categories

(DevTools Graveyard :: WebIDE, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: tofumatt, Assigned: paul)

References

Details

Attachments

(1 file, 10 obsolete files)

38.46 KB, patch
paul
: review+
Details | Diff | Splinter Review
It would be excellent if we could execute things like `appmanager simulator` from the command line on various platforms. I am not aware of how Firefox responds to CLI things cross-platform, but would this be something we could offer to developers? It would make writing, say, a Sublime Text plugin that allowed a user to run their app in the App Manager/Simulator very trivial.
Firefox has a command-line handling API:

  https://developer.mozilla.org/en-US/docs/Chrome/Command_Line

But I'm unsure how well the Firefox executable routes invocations to existing processes.  In theory, it does (unless you specify -no-remote or -new-instance), but I haven't been able to make that work on Mac for a while now, even with the ancient (but theoretically still-available) -remote flag <http://www-archive.mozilla.org/unix/remote.html>.

I suppose the App Manager could instead grow an interface similar to the Simulator's Remote Debugger Protocol (RDP), as described in related bug 900230, so you could communicate with it via a port.
I looked at the command line. Also considered using a protocol handler. Didn't get any interesting results yet.
Depends on: 999417
Depends on: 1010387
How it works:

  $ firefox --webide 'COMMAND'
  
  Where COMMAND is a set of "key=value" pairs, separated by '&'.

  --webide 'actions=action1,action2,action3&param1=value1&param2=value2'

  'actions': executed in order. actionN will be executed only if actionN-1 doesn't fail

  actions:
    'addPackagedApp' (requires 'location' parameter). Select the directory (location);
    'connectToRuntime' (requires 'runtimeType' parameter). Connect to runtime (start simulator if needed);
    'play'. Install selected directory. Start or reload app on connected runtime;

  parameters:
    'location' packaged app directory or hosted app manifest URL;
    'runtimeType' "usb" or "simulator";
    'runtimeID' which runtime to use. By default, the most recent USB device or most recent simulator;

  examples:

      $ firefox --webide "actions=addPackagedApp,connectToRuntime,play&location=/home/bob/Downloads/foobar/&runtimeType=usb"
      Select app located in /home/bob/Downloads/foobar, then
      Connect to USB device
      Install app


I will add more commands later.
This only works if the app manager is running. Next step is to automatically start Firefox and/or the app manager.
Is that enough to support phonegap?
Flags: needinfo?(zaloon)
Flags: needinfo?(zaloon)
Thanks!
I think it's indeed all we need to play with Cordova CLI
Attachment #8423170 - Attachment is obsolete: true
Attachment #8424504 - Flags: feedback?(janx)
Comment on attachment 8424504 [details] [diff] [review]
Ability to execute App Manager V2 commands from shell

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

This is awesome! Thank you Paul, f+.

A few questions and nits below, but in general I'd be very interested to see real use cases of this, especially with phonegap.

- This probably needs tests.
- I think in time we'll need more actions (e.g. addHostedApp, stop, debug, rebootRuntime?).
- For now, there seems to be only one flow: Add an app, connect to the runtime, install/refresh. Is it really useful to separate these 3 actions? When would they be used independently?
- The CLI params disallow going back-and-forth between build scripts and the App Manager, because this would call `firefox` several times. Maybe that's not useful, but what if you want to build and push several apps? Or change some local storage files through ADB after pushing an app but before running it?

::: browser/devtools/webide/components/webideCli.js
@@ +21,5 @@
> +    if (!param) {
> +      return;
> +    }
> +
> +    // If remote command, we don't Firefox to open a new tab

Nit: I don't even Firefox.

@@ +40,5 @@
> +      win.handleCommandline(param);
> +    }
> +
> +    Services.obs.addObserver(onWebIDEAvailable, "devtools-webide-ready", false);
> +    let ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(Ci.nsIWindowWatcher);

(Nit: Not sure we care about line length here but this is way over 80 chars.)

::: browser/devtools/webide/content/cli.js
@@ +4,5 @@
> +
> +/**
> +
> +  $ firefox --webide 'COMMAND'
> +  

Nit: Trailing spaces.

@@ +14,5 @@
> +
> +  actions:
> +    'addPackagedApp' (requires 'location' parameter). Select the directory (location);
> +    'connectToRuntime' (requires 'runtimeType' parameter). Connect to runtime (start simulator if needed);
> +      Note: doesn't work if webide was not started initially

Does that mean "if webide is not already running"?

@@ +15,5 @@
> +  actions:
> +    'addPackagedApp' (requires 'location' parameter). Select the directory (location);
> +    'connectToRuntime' (requires 'runtimeType' parameter). Connect to runtime (start simulator if needed);
> +      Note: doesn't work if webide was not started initially
> +    'play'. Install selected directory. Start or reload app on connected runtime;

Nit: s/reload/refresh/ ?

@@ +18,5 @@
> +      Note: doesn't work if webide was not started initially
> +    'play'. Install selected directory. Start or reload app on connected runtime;
> +
> +  parameters:
> +    'location' packaged app directory or hosted app manifest URL;

There is no 'addHostedApp' action listed above, maybe there should be one?
Attachment #8424504 - Flags: feedback?(janx) → feedback+
(In reply to Jan Keromnes [:janx] from comment #9)
> A few questions and nits below, but in general I'd be very interested to see
> real use cases of this, especially with phonegap.

s/use cases/uses/

> > +      Note: doesn't work if webide was not started initially
> 
> Does that mean "if webide is not already running"?

... or "if webide has never been started on this computer"?
(In reply to Jan Keromnes [:janx] from comment #9)
> - This probably needs tests.

Yes.

> - I think in time we'll need more actions (e.g. addHostedApp, stop, debug,
> rebootRuntime?).

Yes. My plan is to land that first, and then add more.

> - For now, there seems to be only one flow: Add an app, connect to the
> runtime, install/refresh. Is it really useful to separate these 3 actions?
> When would they be used independently?

Granularity is better I guess.

> - The CLI params disallow going back-and-forth between build scripts and the
> App Manager, because this would call `firefox` several times. Maybe that's
> not useful, but what if you want to build and push several apps? Or change
> some local storage files through ADB after pushing an app but before running
> it?

Sadly, it's just impossible to hear back from Firefox after running a webide command.
I guess here we're covering a very basic use case, and if you want to do more, you'll
need to attach scripts to the app manager (run complex scripts app-manager side).

> (Nit: Not sure we care about line length here but this is way over 80 chars.)

I don't.

> > +  actions:
> > +    'addPackagedApp' (requires 'location' parameter). Select the directory (location);
> > +    'connectToRuntime' (requires 'runtimeType' parameter). Connect to runtime (start simulator if needed);
> > +      Note: doesn't work if webide was not started initially
> 
> Does that mean "if webide is not already running"?

Yes.

> > +      Note: doesn't work if webide was not started initially
> > +    'play'. Install selected directory. Start or reload app on connected runtime;
> > +
> > +  parameters:
> > +    'location' packaged app directory or hosted app manifest URL;
> 
> There is no 'addHostedApp' action listed above, maybe there should be one?

Yes.
(In reply to Jan Keromnes [:janx] from comment #10)
> (In reply to Jan Keromnes [:janx] from comment #9)
> > A few questions and nits below, but in general I'd be very interested to see
> > real use cases of this, especially with phonegap.
> 
> s/use cases/uses/
> 
> > > +      Note: doesn't work if webide was not started initially
> > 
> > Does that mean "if webide is not already running"?
> 
> ... or "if webide has never been started on this computer"?

"if webide is not already running"
Blocks: build-am2
No longer blocks: enable-webide
Attachment #8424504 - Attachment is obsolete: true
Attached patch part2: bind console calls (obsolete) — Splinter Review
Attachment #8429102 - Attachment description: xpcom component for command line handler → part1: xpcom component for command line handler
Attached patch part5: process command line (obsolete) — Splinter Review
Attached patch part6: tests (obsolete) — Splinter Review
Attachment #8429102 - Flags: review?(poirot.alex)
Attachment #8429103 - Flags: review?(poirot.alex)
Attachment #8429104 - Flags: review?(poirot.alex)
Attachment #8429105 - Flags: review?(poirot.alex)
Attachment #8429107 - Flags: review?(poirot.alex)
Attachment #8429108 - Flags: review?(poirot.alex)
Comment on attachment 8429106 [details] [diff] [review]
part5: process command line

(feedback as there's still a FIXME in this patch)
Attachment #8429106 - Flags: feedback?(poirot.alex)
Alex -

I split this patch in few patches to make the review easier. I had to wrap some of the operations we do in Task.spawn to do a better job at chaining async operations. It solves some races when this code is not triggered by user actions.

I need your help for an issue I'm running into with AppActorFront.getTargetForApp. It's not new that it fails if the app is not loaded. So we used to loop until we manage to get the target. But for some reason, after the very first time it fails to connect to the app (noSuchActor exception), it's just impossible to reclaim the target again. We keep getting the same exception. Even if done manually.
To reproduce the noSuchActor exception:

- import a package app to app manager (to at least have one app)
- close app manager & firefox
- run app manager like that:

./obj.firefox/dist/bin/firefox -no-remote -P dev -jsconsole -webide  "actions=connectToRuntime,play,debug&runtimeType=usb"                                                                                    ~/mozilla/src

In the console, see the 10 failed attempts to connect to the app.
It's important to make sure that the app is not running on the device first.
Attached patch part5: process command line (obsolete) — Splinter Review
Rebased, and now I don't have this problem anymore.
Attachment #8429106 - Attachment is obsolete: true
Attachment #8429106 - Flags: feedback?(poirot.alex)
Attachment #8429269 - Flags: review?(poirot.alex)
Comment on attachment 8429106 [details] [diff] [review]
part5: process command line

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

::: browser/devtools/webide/content/cli.js
@@ +58,5 @@
> +    );
> +  }
> +});
> +
> +window.handleCommandline = function(cmdline) {

Shouldn't you call UI.busyUntil from handleCommandLine, in order to be busy when handleCommandLine is called from the command line xpcom?

@@ +145,5 @@
> +      if (UI.toolboxIframe) {
> +        yield Cmds.toggleToolbox();
> +      }
> +
> +      yield Cmds.toggleToolbox();

What? Double conditional toggle?
Wouldn't it be easier to follow by exposing Cmds.closeToolbox or call UI.closeToolbox()?

@@ +156,5 @@
> +      if (type != "usb" && type != "simulator") {
> +        return promise.reject("Unkown runtime type");
> +      }
> +
> +      yield Cmds.disconnectRuntime();

I'm not an expert of Task, but aren't we going to stop here when disconnectRuntime will reject if we weren't already connected?

@@ +162,5 @@
> +      if (AppManager.runtimeList[type].length == 0) {
> +        // Wait 3 seconds, to give a chance to USB
> +        // devices to register.
> +        let deferred = promise.defer();
> +        setTimeout(deferred.resolve, 3000);

We can listen for new runtimes instead of an arbirary, significant timeout:
  AppManager.on("app-manager-update", (evt) => {evt == "runtimelist" && deferred.resolve()});
(We may keep the timeout in case no runtime gets connected)

@@ +186,5 @@
> +        return promise.reject("Can't find any runtime to connect to");
> +      }
> +
> +      let deferred = promise.defer();
> +      AppManager.webAppsStore.once("store-ready", deferred.resolve);

A comment about this listener can be helpful. Why we are listening for more than connectToRuntime resolution.
Comment on attachment 8429102 [details] [diff] [review]
part1: xpcom component for command line handler

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

::: browser/devtools/webide/components/webideCli.js
@@ +55,5 @@
> +                                 "chrome,centerscreen,resizable,dialog=no",
> +                                 null);
> +
> +    if (param) {
> +      win.arguments = [param];

What about always calling win.handleCommandLine (and wait for load event), intead of having two different codepaths?
Attachment #8429102 - Flags: review?(poirot.alex) → review+
Attachment #8429103 - Flags: review?(poirot.alex) → review+
Attachment #8429104 - Flags: review?(poirot.alex) → review+
Attachment #8429105 - Flags: review?(poirot.alex) → review+
Comment on attachment 8429107 [details] [diff] [review]
part6: tests

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

Do you have a try run with this new test?
Attachment #8429107 - Flags: review?(poirot.alex) → review+
Attachment #8429108 - Flags: review?(poirot.alex) → review+
Comment on attachment 8429269 [details] [diff] [review]
part5: process command line

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

r+ with the other comments from the previous version, attachment 8429106 [details] [diff] [review].

::: browser/devtools/webide/modules/app-manager.js
@@ +338,5 @@
>        let installPromise;
>  
> +      if (project.type != "packaged" && project.type != "hosted") {
> +        return promise.reject("Don't know how to install project");
> +      }

nit: I would have made the following code like that:
if (project.type == "packaged") {
...
} else if (project.type == "hosted") {
...
} else {
  return promise.reject("Don't know how to install this project");
}

@@ +380,5 @@
>        }
>  
> +      let manifest = self.getProjectManifestURL(project);
> +      if (!self._runningApps.has(manifest)) {
> +        console.log("Launching app: " + project.name);

Do we still need these logs?

@@ +387,3 @@
>  
> +      } else {
> +        console.log("Reloading app: " + project.name);

Do we still need these logs?
Attachment #8429269 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #26)
> @@ +156,5 @@
> > +      if (type != "usb" && type != "simulator") {
> > +        return promise.reject("Unkown runtime type");
> > +      }
> > +
> > +      yield Cmds.disconnectRuntime();
> 
> I'm not an expert of Task, but aren't we going to stop here when
> disconnectRuntime will reject if we weren't already connected?

disconnectRuntime doesn't reject.
Addressed comments.
Assignee: nobody → paul
Attachment #8429102 - Attachment is obsolete: true
Attachment #8429103 - Attachment is obsolete: true
Attachment #8429104 - Attachment is obsolete: true
Attachment #8429105 - Attachment is obsolete: true
Attachment #8429107 - Attachment is obsolete: true
Attachment #8429108 - Attachment is obsolete: true
Attachment #8429269 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8429360 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7e6e3238f348
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: