Closed Bug 839862 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

Attachments

(1 file, 7 obsolete files)

i.e. using the Firefox remote debug architecture.
Attached patch v1 (obsolete) — Splinter Review
See https://github.com/mozilla/gcli/pull/66 for the changes in smaller chunks.
Assignee: nobody → jwalker
Attachment #742464 - Flags: review?(mratcliffe)
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 on attachment 742464 [details] [diff] [review]
v1

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

Looks good to me, r+
Attachment #742464 - Flags: review?(mratcliffe) → review+
Whiteboard: [land-in-fx-team]
Not ready to land. Needs rebase, push to try, probably test fixes.
Whiteboard: [land-in-fx-team]
Attached patch v2 (obsolete) — Splinter Review
Fixes tests. Should be ready to land given green on try.
Attachment #742464 - Attachment is obsolete: true
Attachment #746918 - Flags: review?(mratcliffe)
(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 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
Attachment #746918 - Flags: review?(mratcliffe) → review+
Attached patch v3 (obsolete) — Splinter Review
Fixes review comments, and a try failure. Thanks Mike.
Attachment #746918 - Attachment is obsolete: true
Attached patch v4 (obsolete) — Splinter Review
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
Attachment #746985 - Attachment is obsolete: true
Attachment #747372 - Flags: review?(mratcliffe)
Comment on attachment 747372 [details] [diff] [review]
v4

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

Looks good to me, r+
Attachment #747372 - Flags: review?(mratcliffe) → review+
Attached patch v5 (obsolete) — Splinter Review
Forgot to `hg add browser/devtools/commandline/test/browser_cmd_cookie.html`
Attachment #747372 - Attachment is obsolete: true
Attachment #747390 - Flags: review?(mratcliffe)
Comment on attachment 747390 [details] [diff] [review]
v5

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

zap=zep zip=zop r+
Attachment #747390 - Flags: review?(mratcliffe) → review+
Backout for xpcshell bustage
https://hg.mozilla.org/integration/fx-team/rev/dd5e3232a63c
Whiteboard: [fixed-in-fx-team]
Attached patch v6 (obsolete) — Splinter Review
xpcshell tests were borked because gcli.jsm isn't in toolkit.

https://tbpl.mozilla.org/?tree=Try&rev=4a96d7048bb4
Attachment #747390 - Attachment is obsolete: true
Attachment #747883 - Flags: review?(mratcliffe)
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 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.
Attachment #747883 - Flags: review?(mratcliffe) → review+
Attached patch v7 (obsolete) — Splinter Review
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)
Attachment #747883 - Attachment is obsolete: true
Attachment #747963 - Flags: review?(mratcliffe)
Attachment #747963 - Flags: review?(mratcliffe) → review+
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.
Attached patch v8Splinter Review
Minor tweak to fix a header

This is green on try: https://tbpl.mozilla.org/?tree=Try&rev=5e3de43b8ee1
Attachment #747963 - Attachment is obsolete: true
Blocks: 871066
https://hg.mozilla.org/mozilla-central/rev/511d15a517bd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
The description of the 'connect' and 'disconnect' commands are identical. Is that correct?
Blocks: 871245
(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
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...
Also, it adds both a tab actor and a global actor, but they seem identical - is that intentional?
Depends on: 871697
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.