860672 - command line option -jsconsole should open the Browser Console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Alice0775 White]

STR
set devtools.chrome.enabled to |true|
start Firefox.exe -jsconsole

Expected results:
Should open "new Browser Console"

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Open the console when -jsconsole is on the command-line

I had a play with this as I like the new console and regularly use the -jsconsole cmdline option.

Of note:

* toolkit/component/console now only registers itself as a command-line handler if we are not building the "browser" (ie, so thunderbird etc will still use it).  However, this might cause a problem if the patch is applied and an incremental build done - that component will still be there.  The simplest solution is to just delete jsconsole-clhandler* from the dist/bin/components directory before building with this patch (or, obviously, just do a clobber build)

* I named the new component devtools-clhandler in the expectation that in the future the devtools might support other command-line arguments - however, only -jsconsole is currently supported.

* for reasons I don't fully understand, it was necessary to wait for a window to open before attempting to open the console.  If there is a better way to know when it is a good time to initialize the devtools stuff, please let me know.

I randomly picked :msucan for feedback as they are a peer of the component and already on the bug - please feel free to reassign :)

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mark Hammond (:markh) from comment #1)
> * toolkit/component/console now only registers itself as a command-line
> handler if we are not building the "browser" (ie, so thunderbird etc will
> still use it).  However, this might cause a problem if the patch is applied
> and an incremental build done - that component will still be there.  The
> simplest solution is to just delete jsconsole-clhandler* from the
> dist/bin/components directory before building with this patch (or,
> obviously, just do a clobber build)

Will b2g desktop get the same treatment as desktop Firefox? Shouldn't it?

Also, you can make sure a clobber is required by modifying the CLOBBER file in the top-level directory.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Panos Astithas [:past] from comment #2)
> 
> Will b2g desktop get the same treatment as desktop Firefox? Shouldn't it?

Sure, if that is the consensus.  Or we can do that later I guess.

> Also, you can make sure a clobber is required by modifying the CLOBBER file
> in the top-level directory.

The lack of a clobber is likely to mean that the existing jsconsole might be used - so I'm not sure it's worth forcing a clobber for this.

Comment 4

[Mihai Sucan [:msucan]]

(In reply to Mark Hammond (:markh) from comment #1)
> Created attachment 756432 [details] [diff] [review]
> Open the console when -jsconsole is on the command-line
> 
> I had a play with this as I like the new console and regularly use the
> -jsconsole cmdline option.

I am glad you like the new console and thank you for the patch!

> Of note:
> 
> * toolkit/component/console now only registers itself as a command-line
> handler if we are not building the "browser" (ie, so thunderbird etc will
> still use it).  However, this might cause a problem if the patch is applied
> and an incremental build done - that component will still be there.  The
> simplest solution is to just delete jsconsole-clhandler* from the
> dist/bin/components directory before building with this patch (or,
> obviously, just do a clobber build)

Makes sense to only have the browser console take over -jsconsole when building Firefox. If we could also allow it with b2g-desktop it would be very nice.


> * for reasons I don't fully understand, it was necessary to wait for a
> window to open before attempting to open the console.  If there is a better
> way to know when it is a good time to initialize the devtools stuff, please
> let me know.

The reason why you needed to do this is because the browser console actor (in the debugger server) relies on having some window open to attach to - preferably a browser chrome window. Additionally, HUDService.jsm, which connects to the debugger server, also relies on having a browser window available, for UI-related stuff.

In both cases we can change the code to allow the browser console to open without requiring any browser window, except that of the console itself. This should not require too much work. I can explain what needs to be done, here or on IRC.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Mark Hammond (:markh) from comment #3)
> (In reply to Panos Astithas [:past] from comment #2)
> > 
> > Will b2g desktop get the same treatment as desktop Firefox? Shouldn't it?
> 
> Sure, if that is the consensus.  Or we can do that later I guess.

Sounds good.

> > Also, you can make sure a clobber is required by modifying the CLOBBER file
> > in the top-level directory.
> 
> The lack of a clobber is likely to mean that the existing jsconsole might be
> used - so I'm not sure it's worth forcing a clobber for this.

Agreed, we shouldn't force a clobber only for this purpose.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 756432 [details] [diff] [review]
Open the console when -jsconsole is on the command-line

This looks good, but for some weird reason it fails on my system (Linux here):

observe: domwindowopened
console.error: 
  Message: TypeError: devtools.TargetFactory is undefined
  Stack:
    getTarget@resource:///modules/HUDService.jsm:692
    ...

I see the console.error message only if I add in HUDService.jsm a line as follows:
  Promise._reportErrors = true;
(promises hide exceptions by default :( )

I am not sure why this happens. Maybe gDevTools.jsm is imported too early and TargetFactory is not yet available (or something weird). Did you see this error while working on this patch?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 756432 [details] [diff] [review]
> Open the console when -jsconsole is on the command-line
> 
> This looks good, but for some weird reason it fails on my system (Linux
> here):

I don't see that specific error, but my linux box does fail to create the console too (I suspect I simply can't see the error as once we fail to create the console, it continues to fail to operate even after startup is complete, so there's nowhere for me to see the error).

I could get it to work by creating the console in a setTimeout(..., 500) handler in the load event, which leads me to believe that there is simply a "race" between this code creating the console and the devtools starting up correctly.  However, after doing this the win.focus() call in the patch also failed to have any affect...

I think the best thing to do might be to arrange for the console to be able to be created without a window, as mentioned in comment 4, as this will create the console ASAP and also side-step the window focus issue completely.  My timezone is unfriendly at this time of year, so if you could outline what needs to be done here that would be great.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Mark Hammond (:markh) from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > Comment on attachment 756432 [details] [diff] [review]
> > Open the console when -jsconsole is on the command-line
> > 
> > This looks good, but for some weird reason it fails on my system (Linux
> > here):
> 
> I don't see that specific error, but my linux box does fail to create the
> console too (I suspect I simply can't see the error as once we fail to
> create the console, it continues to fail to operate even after startup is
> complete, so there's nowhere for me to see the error).

You need to add Promise._reportErrors = true. Otherwise the error doesn't show. The error will show in the terminal, not in the browser console.


> I could get it to work by creating the console in a setTimeout(..., 500)
> handler in the load event, which leads me to believe that there is simply a
> "race" between this code creating the console and the devtools starting up
> correctly.  However, after doing this the win.focus() call in the patch also
> failed to have any affect...
> 
> I think the best thing to do might be to arrange for the console to be able
> to be created without a window, as mentioned in comment 4, as this will
> create the console ASAP and also side-step the window focus issue
> completely.  My timezone is unfriendly at this time of year, so if you could
> outline what needs to be done here that would be great.

Sounds good.

The approach I have in mind goes as follows: change HUDService.jsm to open the browser console before it connects to the server. Then change the web console actor to find a devtools:browserconsole window (change windowtype such it matches) when it is used as a global console actor. Then, any server-side dependency on having a browser window *should* be gone and user's code will execute in the context of the browser console chrome window.

In HUDService.jsm you will also have to look at the code and see where a browser window is used, then make changes so we no longer need the browser window. Some examples:
- gViewSourceUtils - we can import the viewsource utils ourselves.
- mainPopupSet - we can add a popupset container to the webconsole.xul.

This is only the overview. There are some unanswered concerns: for example, why the devtools TargetFactory is undefined - that shouldn't wait for browser windows to initialize. I suggest you ping dcamp - he did the devtools jetpack-based loader. I am sure he can point you in the right direction.

Another problem: with the changes suggested here, we will make bug 878966 even worse.

If you have time to work on this bug, that's great, but please consider this is an experiment/exploration of how we can fix this bug. It might require more work.

Thank you!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: bug-860672-1-fx-team.diff

This patch is based on Mark's work (thank you Mark!). Did this patch in Paris, but I did not submit it because of some rebase needs in a couple of tests (wanted to avoid issues with bug 760876).

- the -jsconsole option only opens the browser console if the firefox browser is built.
- the browser console no longer targets the most recent browser window, it targets its own window. the Error Console also did something similar: it had a hidden iframe in which any js was evaluated.

Should I ask for review from anyone else as well? Panos and/or a toolkit peer for the console component change?

Thank you!


Try push: https://tbpl.mozilla.org/?tree=Try&rev=5825df04783c

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 805551 [details] [diff] [review]
bug-860672-1-fx-team.diff

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

::: toolkit/components/console/moz.build
@@ +7,5 @@
>  TEST_DIRS += ['tests']
>  
> +# The '-jsconsole' command-line argument is handled by the devtools
> +# if we are building the browser.
> +if CONFIG['MOZ_BUILD_APP'] != 'browser':

This won't work - Toolkit shouldn't have any dependency on browser, but more importantly this won't work for XULRunner builds.

The proper way to solve this is for devtools to override the JS Console's commandline handler component - mapping it's contract to the devtool's component (in addition to it's own contract).

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 805551 [details] [diff] [review]
bug-860672-1-fx-team.diff

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

What about creating a separate command flag (-console?) to open the browser console and leaving the error console as the default for toolkittish apps? we can't break those with this and moving all of the Browser Console into toolkit is not possible or even desirable.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #11)

> What about creating a separate command flag

That sounds good, but:

> (-console?) to open the browser console

-console is already used on Windows to open a "windows console" as the app starts.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #11)
> What about creating a separate command flag (-console?) to open the browser
> console and leaving the error console as the default for toolkittish apps?
> we can't break those with this and moving all of the Browser Console into
> toolkit is not possible or even desirable.

What I said in comment 10 would make it so -jsconsole continues to work normally for anything-not-Firefox.

Comment 14

[Mihai Sucan [:msucan]]

(In reply to Blair McBride [:Unfocused] from comment #10)
> Comment on attachment 805551 [details] [diff] [review]
> bug-860672-1-fx-team.diff
> 
> Review of attachment 805551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/console/moz.build
> @@ +7,5 @@
> >  TEST_DIRS += ['tests']
> >  
> > +# The '-jsconsole' command-line argument is handled by the devtools
> > +# if we are building the browser.
> > +if CONFIG['MOZ_BUILD_APP'] != 'browser':
> 
> This won't work - Toolkit shouldn't have any dependency on browser, but more
> importantly this won't work for XULRunner builds.

How does this change make toolkit dependent on browser?


> The proper way to solve this is for devtools to override the JS Console's
> commandline handler component - mapping it's contract to the devtool's
> component (in addition to it's own contract).

How can I do this properly? Shall I just add two sets of 'component/contract/category' lines to the devtools-clhandler.manifest? The second set should just copy the identifiers from the error console handler, but pointing to the devtools-clhandler.js file.


(In reply to Rob Campbell [:rc] (:robcee) from comment #11)
> Comment on attachment 805551 [details] [diff] [review]
> bug-860672-1-fx-team.diff
> 
> Review of attachment 805551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about creating a separate command flag (-console?) to open the browser
> console and leaving the error console as the default for toolkittish apps?
> we can't break those with this and moving all of the Browser Console into
> toolkit is not possible or even desirable.

Right. However, if overriding -jsconsole is possible we can do just that. What do you prefer? A new option or override? I think I prefer the latter - it's hard to discover new options.

Comment 15

[Rob Campbell [:rc] (:robcee)]

(In reply to Mihai Sucan [:msucan] from comment #14)
> Right. However, if overriding -jsconsole is possible we can do just that.
> What do you prefer? A new option or override? I think I prefer the latter -
> it's hard to discover new options.

ok, overriding -jsconsole for browser only.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mihai Sucan [:msucan] from comment #14)
> How does this change make toolkit dependent on browser?

I guess "dependency" is the wrong word. Generally we try to avoid having Toolkit contain any application-specific behaviours.


> > The proper way to solve this is for devtools to override the JS Console's
> > commandline handler component - mapping it's contract to the devtool's
> > component (in addition to it's own contract).
> 
> How can I do this properly? Shall I just add two sets of
> 'component/contract/category' lines to the devtools-clhandler.manifest? The
> second set should just copy the identifiers from the error console handler,
> but pointing to the devtools-clhandler.js file.

Just need to rename the contract ID for your new component to be the same as the Toolkit component's - ie, "@mozilla.org/toolkit/console-clh;1". (Ignore the part where I said "in addition to it's own contract".) Don't need a category for it, as that's already added in the Toolkit manifest.

So your new manifest would end up like:

component {9e9a9283-0ce9-4e4a-8f1c-ba129a032c32} devtools-clhandler.js
contract @mozilla.org/toolkit/console-clh;1 {9e9a9283-0ce9-4e4a-8f1c-ba129a032c32}

Comment 17

[Mihai Sucan [:msucan]]

Blair, thank you for your information!

Comment 18

[Mihai Sucan [:msucan]]

Attachment: bug-860672-2-fx-team.diff

Updated the patch as suggested by Blair.

Blair: please check this patch is as expected.

It seems to be working fine for me.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a4d678d74e1a

Comment 19

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 808511 [details] [diff] [review]
bug-860672-2-fx-team.diff

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

::: browser/devtools/webconsole/test/browser_console_dead_objects.js
@@ +26,5 @@
>      hud.jsterm.clearOutput();
> +    hud.jsterm.execute("Cu = Components.utils;" +
> +                       "Cu.import('resource://gre/modules/Services.jsm');" +
> +                       "chromeWindow = Services.wm.getMostRecentWindow('navigator:browser');" +
> +                       "foobarzTezt = chromeWindow.content.document", onAddVariable);

that's a funny variable name.

Comment 20

[Rob Campbell [:rc] (:robcee)]

lgtm!

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 808511 [details] [diff] [review]
bug-860672-2-fx-team.diff

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

Ship it!

Comment 22

[Mihai Sucan [:msucan]]

Thank you!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7d619d4cfd94

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d619d4cfd94

Comment 25

[Mihai Sucan [:msucan]]

(mid-air collision: I was adding the dev-doc-needed keyword this very moment)

Will: this patch changes the -jsconsole command line option to open the Browser Console instead of the Error Console. This patch also changes the JS evaluation target in the Browser Console: we no longer evaluate directly in the chrome browser window scope - I saw some MDN docs mention this. The new scope is the Browser Console window itself. If you eval |document.title| you will see "Browser Console".

Comment 26

[Mihai Sucan [:msucan]]

The Browser Console page on MDN mentions -jsconsole.