Closed
Bug 839862
Opened 8 years ago
Closed 8 years ago
The developer toolbar / GCLI should be able to execute commands remotely
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: jwalker, Assigned: jwalker)
References
Details
Attachments
(1 file, 7 obsolete files)
280.81 KB,
patch
|
Details | Diff | Splinter Review |
i.e. using the Firefox remote debug architecture.
Assignee | ||
Comment 1•8 years ago
|
||
See https://github.com/mozilla/gcli/pull/66 for the changes in smaller chunks.
Assignee: nobody → jwalker
Attachment #742464 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 2•8 years ago
|
||
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]
Assignee | ||
Comment 4•8 years ago
|
||
Not ready to land. Needs rebase, push to try, probably test fixes.
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
Fixes review comments, and a try failure. Thanks Mike.
Attachment #746918 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
Forgot to `hg add browser/devtools/commandline/test/browser_cmd_cookie.html`
Attachment #747372 -
Attachment is obsolete: true
Attachment #747390 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 12•8 years ago
|
||
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=7ac21da57bbe
Assignee | ||
Comment 13•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=9ae9cc383d0f https://hg.mozilla.org/integration/fx-team/rev/d6249744132e
Whiteboard: [fixed-in-fx-team]
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+
Assignee | ||
Comment 15•8 years ago
|
||
Backout for xpcshell bustage https://hg.mozilla.org/integration/fx-team/rev/dd5e3232a63c
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
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+
Assignee | ||
Comment 20•8 years ago
|
||
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.
Assignee | ||
Comment 21•8 years ago
|
||
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
Assignee | ||
Comment 22•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=511d15a517bd https://hg.mozilla.org/integration/fx-team/rev/511d15a517bd
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/511d15a517bd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Comment 24•8 years ago
|
||
The description of the 'connect' and 'disconnect' commands are identical. Is that correct?
Assignee | ||
Comment 25•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
Also, it adds both a tab actor and a global actor, but they seem identical - is that intentional?
Updated•3 years ago
|
Product: Firefox → DevTools
Updated•3 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•