839862 - The developer toolbar / GCLI should be able to execute commands remotely

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

i.e. using the Firefox remote debug architecture.

Comment 1

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v1

See https://github.com/mozilla/gcli/pull/66 for the changes in smaller chunks.

Comment 2

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Added extra tests and moved PR to my repo to avoid spamming other members of the 'mozilla' github organization

https://github.com/joewalker/gcli/pull/9

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 742464 [details] [diff] [review]
v1

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

Looks good to me, r+

Comment 4

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Not ready to land. Needs rebase, push to try, probably test fixes.

Comment 5

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v2

Fixes tests. Should be ready to land given green on try.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] from comment #5)
> Created attachment 746918 [details] [diff] [review]
> v2
> 
> Fixes tests. Should be ready to land given green on try.

Do you have a link to a try run?

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 746918 [details] [diff] [review]
v2

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

r+ with the nits addressed.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +1920,5 @@
> +
> +/* Remoting ----------------------------------------------------------- */
> +
> +// Is this defined somewhere else that I can import?
> +const DEFAULT_DEBUG_PORT = 4242;

Is this not devtools.debugger.chrome-debugging-port?

::: browser/devtools/commandline/gcli.jsm
@@ +92,5 @@
>  
>    'use strict';
>  
>    // Internal startup process. Not exported
> +  // The first group are depended on by others so they must be registered first

Other groups depend on the first group so it must be registered first.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +1265,5 @@
> +# LOCALIZATION NOTE (listenDesc) A very short string used to describe the
> +# function of the 'listen' command.
> +listenDesc=Open a remote debug port
> +
> +# LOCALIZATION NOTE (profilerManual) A longer description of the 'listen'

listenManual

Comment 8

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v3

Fixes review comments, and a try failure. Thanks Mike.

Comment 9

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v4

Update with 2 changes
* add/fixed tests for the cookie command:
  Interdiff - http://pastebin.mozilla.org/2387338
* clean-up to GcliActor:
  Interdiff - http://pastebin.mozilla.org/2387201

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

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 747372 [details] [diff] [review]
v4

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

Looks good to me, r+

Comment 11

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v5

Forgot to `hg add browser/devtools/commandline/test/browser_cmd_cookie.html`

Comment 12

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=7ac21da57bbe

Comment 13

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=9ae9cc383d0f
https://hg.mozilla.org/integration/fx-team/rev/d6249744132e

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 747390 [details] [diff] [review]
v5

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

zap=zep zip=zop r+

Comment 15

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Backout for xpcshell bustage
https://hg.mozilla.org/integration/fx-team/rev/dd5e3232a63c

Comment 16

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v6

xpcshell tests were borked because gcli.jsm isn't in toolkit.

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

Comment 17

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

So this works because gcli.jsm is loaded by xpcshell tests, but it isn't actually used from there, so there are a number of lazy-loaded browser resources that are ok. The question is can we accept 'lazy-loading' as a solution to the dependency problem?

Here are the resources in question:
* resource:///modules/devtools/BuiltinCommands.jsm
  Removing the dependency on this is fairly easy - I might do it anyway
* l10n files:
  chrome://browser/locale/devtools/gcli.properties
  chrome://browser/locale/devtools/gclicommands.properties
  I'm guessing that moving these is going to make l10n people be cross with us,
  but we can do it
* Preferences: devtools.debugger.chrome-debugging-port and others. I can think of
  several versions of the lazy-load solution, but they're all hacks
* resource:///modules/highlighter.jsm
  Not used yet, but we're going to want to at some stage.
  I include it here to make the point that we're going to need to accept that
  lazy-loaded, unused-in-xpcshell-test resources have to be OK or we're just going
  to move everything into toolkit

There is an orange in the try above, but it's due to a test that I didn't update properly. It does prove that the lazy-load solution is probably technically viable.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 747883 [details] [diff] [review]
v6

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

(In reply to Joe Walker [:jwalker] from comment #17)
> So this works because gcli.jsm is loaded by xpcshell tests, but it isn't
> actually used from there, so there are a number of lazy-loaded browser
> resources that are ok. The question is can we accept 'lazy-loading' as a
> solution to the dependency problem?
> 

It is a reasonable workaround but feels wrong to me. I don't see any other options though.

> Here are the resources in question:
> * resource:///modules/devtools/BuiltinCommands.jsm
>   Removing the dependency on this is fairly easy - I might do it anyway
> * l10n files:
>   chrome://browser/locale/devtools/gcli.properties
>   chrome://browser/locale/devtools/gclicommands.properties
>   I'm guessing that moving these is going to make l10n people be cross with
> us,
>   but we can do it
> * Preferences: devtools.debugger.chrome-debugging-port and others. I can
> think of
>   several versions of the lazy-load solution, but they're all hacks
> * resource:///modules/highlighter.jsm
>   Not used yet, but we're going to want to at some stage.
>   I include it here to make the point that we're going to need to accept that
>   lazy-loaded, unused-in-xpcshell-test resources have to be OK or we're just
> going
>   to move everything into toolkit
> 

Is that really such a bad thing? That would make it possible to use devtools in other products. Probably a fair amount of work though.

> There is an orange in the try above, but it's due to a test that I didn't
> update properly. It does prove that the lazy-load solution is probably
> technically viable.

r+ if you fix your test.

Comment 19

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v7

This update:
* Move connect function from BuiltinCommands.jsm to gcli.jsm to remove a dependency
  from toolkit to browser
* Remove the browser_browser.basic.js test (no longer needed, cause of orange)

Comment 20

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Discussed at team meeting: Summary - we don't like dependencies from toolkit to browser, but basically we're going to have to move everything into toolkit to avoid it. That feels like overkill right now. GCLI didn't work from non-firefox apps before, and it doesn't work from non-firefox apps after, but it should be usable from b2g.

We'll work on reducing the dependencies.
Please shout if you disagree.

Comment 21

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Attachment: v8

Minor tweak to fix a header

This is green on try: https://tbpl.mozilla.org/?tree=Try&rev=5e3de43b8ee1

Comment 22

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=511d15a517bd
https://hg.mozilla.org/integration/fx-team/rev/511d15a517bd

Comment 23

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

https://hg.mozilla.org/mozilla-central/rev/511d15a517bd

Comment 24

[u60234]

The description of the 'connect' and 'disconnect' commands are identical. Is that correct?

Comment 25

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

(In reply to Hasse from comment #24)
> The description of the 'connect' and 'disconnect' commands are identical. Is
> that correct?

It's not, thanks
Bug 871697

Comment 26

[Dave Camp (:dcamp)]

This doesn't really work at all.

'connect foo localhost' and 'connect foo localhost 6030' both complain about too many arguments.

If I use the default args, 'connect foo' fails with "Promise not defined"

When I fixed that missing declaration I was able to connect, but 'foo help' didn't get a reply packet.  I gave up at that point.

Are there any automated tests for this?  It shouldn't have gotten r+ without them...

Comment 27

[Dave Camp (:dcamp)]

Also, it adds both a tab actor and a global actor, but they seem identical - is that intentional?