Closed
Bug 1007218
Opened 11 years ago
Closed 10 years ago
Ability to execute App Manager V2 commands from shell
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: tofumatt, Assigned: paul)
References
Details
Attachments
(1 file, 10 obsolete files)
(deleted),
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.
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
I looked at the command line. Also considered using a protocol handler. Didn't get any interesting results yet.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
How it works:
$ firefox --webide 'COMMAND'
Where COMMAND is a set of "key=value" pairs, separated by '&'.
--webide 'actions=action1,action2,action3¶m1=value1¶m2=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.
Assignee | ||
Comment 5•10 years ago
|
||
This only works if the app manager is running. Next step is to automatically start Firefox and/or the app manager.
Updated•10 years ago
|
Flags: needinfo?(zaloon)
Comment 7•10 years ago
|
||
Thanks!
I think it's indeed all we need to play with Cordova CLI
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8423170 -
Attachment is obsolete: true
Attachment #8424504 -
Flags: feedback?(janx)
Comment 9•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
(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"?
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
(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"
Assignee | ||
Updated•10 years ago
|
Blocks: enable-webide
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Attachment #8424504 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8429102 -
Attachment description: xpcom component for command line handler → part1: xpcom component for command line handler
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8429102 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Attachment #8429103 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Attachment #8429104 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Attachment #8429105 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Attachment #8429107 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Attachment #8429108 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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.
Assignee | ||
Comment 23•10 years ago
|
||
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.
Assignee | ||
Comment 24•10 years ago
|
||
It's important to make sure that the app is not running on the device first.
Assignee | ||
Comment 25•10 years ago
|
||
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 26•10 years ago
|
||
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 27•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8429103 -
Flags: review?(poirot.alex) → review+
Updated•10 years ago
|
Attachment #8429104 -
Flags: review?(poirot.alex) → review+
Updated•10 years ago
|
Attachment #8429105 -
Flags: review?(poirot.alex) → review+
Comment 28•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8429108 -
Flags: review?(poirot.alex) → review+
Comment 29•10 years ago
|
||
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+
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Assignee | ||
Comment 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 34•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•