587757 - (global-console) Implement Browser Console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Kevin Dangoor]

The Error Console is a toolkit component, but for Firefox at least we should consider replacing the Error Console with the new Web Console. This bug is here to track the issues relating to do so.

Comment 1

[Philip Chee]

Hmm. Does the web console show chrome errors? And can it execute javascript in the chrome (privileged) scope?

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> Hmm. Does the web console show chrome errors? And can it execute javascript in
> the chrome (privileged) scope?

Right now it knows about Chrome Errors/XPConnect errors, but ignores them. Adding in a hook for surfacing them should be trivial. 

The Console's JSTerm can use any window in it's sandbox execution scope, so yes. Again, another patch with some consideration of UI/UX.

Comment 3

[David Dahl :ddahl]

This seems like an INVALID bug until we have a plan for modernizing the Error Console

Comment 4

[Dão Gottwald [:dao]]

Wouldn't a plan for modernizing the Error Console invalidate this even more?

Comment 5

[David Dahl :ddahl]

(In reply to comment #4)
> Wouldn't a plan for modernizing the Error Console invalidate this even more?

Indeed. we need a plan.

Comment 6

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

(In reply to comment #4)
> Wouldn't a plan for modernizing the Error Console invalidate this even more?

is there a plan for this? I thought that's what we were talking about here (and have been talking about for nearly a year now).

We've been talking about a windowed version of the Web Console that behaves more as a Global Console for awhile. It should have the same jsterm input line as the web console and let people enter JS in a Chrome context.

We should probably communicate that more. Whether it'll live in Toolkit or not is up for debate. It would certainly be nice to modernize the Error Console with something better for all toolkit apps.

Comment 7

[Dão Gottwald [:dao]]

You seem to be overloading the term Error Console... In comment 3 it's the tool that should be modernized, in the bug summary it's seemingly the back-end that should be replaced. That's kind of confusing. Maybe the summary should be "Re-implement the Error Console on top of the Web Console" or something.

Comment 8

[David Dahl :ddahl]

(In reply to comment #7)
> You seem to be overloading the term Error Console... In comment 3 it's the tool
> that should be modernized, in the bug summary it's seemingly the back-end that
> should be replaced. That's kind of confusing. Maybe the summary should be
> "Re-implement the Error Console on top of the Web Console" or something.

We want a new, additional console that traps all errors and has a better JS execution 'terminal' like the web console. this should be a completely new entity in its own window. We will steal code and ideas from the web console but not implement on top of it.

Comment 10

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

Bug triage, filter on PEGASUS.

Comment 11

[Zhenshuo Fang (:fang) - Firefox UX Team]

Not sure if ui-wanted is needed for now.
Removed it but can be added later.

Comment 12

[Mihai Sucan [:msucan]]

Resetting assignee to reflect reality. However, we are close to having the Global Console ready.

Comment 13

[Mihai Sucan [:msucan]]

In bug 827083 I've started removing code that relies on the presence of a xul:tab.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: first cut

This adds the new Browser Console to the dev tools menu, hidden behind the devtools.chrome.enabled pref by default.

All existing tests pass and it's really usable.

Concerns:

- do we want to hide this by default? I'd rather not, seeing the Error Console is enabled by default.

- but we cannot have both the Error Console and the Browser Console enabled by default. What shall we do?

- keyboard shortcut - take over the Error Console?

- need to write some new tests.

- in the future we need to prioritize work on improving message filtering (per addons, and any other ideas we might have). However, this is not something we want to do in this patch.

- this is disconnected from the toolbox UI - rather on purpose, at this point. We haven't figured out for a long time already what we do with the Browser Debugger and the Browser Console. We do not want to spawn a new process for the console. Based on talks with Rob he seems to agree with the approach I'm taking in this patch. Again, this is not a problem I'd like to solve in this patch. We can do that in a dependency bug (and we can discuss if that's a blocker for this feature to land, or not).

Thoughts? Thank you!

Comment 15

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #14)
> Concerns:
> 
> - do we want to hide this by default? I'd rather not, seeing the Error
> Console is enabled by default.

Error console is also hidden by default and is hidden behind two prefs, not only one.

> - but we cannot have both the Error Console and the Browser Console enabled
> by default. What shall we do?

Since this does exactly what error console do, aren't we close to disabling error console all together ?

> - keyboard shortcut - take over the Error Console?

Sounds good.

Comment 16

[Paul Rouget [:paul]]

Comment on attachment 724816 [details] [diff] [review]
first cut

Sorry, I won't have time to look at this before I live. Joe might be able to help.

Comment 17

[Joe Walker [:jwalker]]

Comment on attachment 724816 [details] [diff] [review]
first cut

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

Looks good to me.

::: browser/base/content/browser.js
@@ +1557,2 @@
>                    gPrefService.getBoolPref("devtools.debugger.chrome-enabled") &&
>                    gPrefService.getBoolPref("devtools.debugger.remote-enabled");

Please could we change enabled to remoteChromeEnabled or something?

::: browser/devtools/webconsole/HUDService.jsm
@@ +162,5 @@
> + *        - target: the target that the web console will connect to.
> + *        - window: the window where the web console UI is already loaded.
> + *        - chromeWindow: the window of the web console owner.
> + *        - browserConsole: boolean that tells if this is a browser
> + *        console or not.

I like the use of an options object. I think it's worth asking the question - 'how many of these are optional'. I wouldn't want to have an "All options must be optional" rule, but if most are and 1/2 are not, perhaps they should be params instead.

I guess that they're mandatory in the case of a BrowserConsole, but not otherwise. Perhaps this is a case where we should have a subclass? They'd basically be the same except for the  toggleBrowserConsole function.

::: browser/devtools/webconsole/WebConsolePanel.jsm
@@ +69,3 @@
>      }
>  
> +    return deferredIframe.promise.then(function() promiseTarget)

I wonder if we shouldn't start fat-arrowing things?

::: browser/devtools/webconsole/webconsole.xul
@@ +41,2 @@
>    </commandset>
> +  <keyset id="consoleKeys">

Do we need the id at all?

Comment 18

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

Comment on attachment 724816 [details] [diff] [review]
first cut

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

I think we should stick to the original plan to have the Global Console replace the Error Console (regardless if we change its name or not). If we can't get it to a state that is good enough for that, then I'd say let's kill-switch it (or back it out), but I believe there is no point in keeping both tools, especially since one of them is no longer actively maintained. Let's get feedback from firefox-dev when we are ready to do so.

::: browser/devtools/webconsole/HUDService.jsm
@@ +31,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleUtils",
>      "resource://gre/modules/devtools/WebConsoleUtils.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "webConsoleDefinition",
> +    "resource:///modules/devtools/ToolDefinitions.jsm");

I wish we could use a common toolbox for both global console and browser debugger to avoid this sort of layering violations, but I agree that this is the right way to do it for now.

@@ +541,5 @@
> +        chrome: true,
> +      };
> +
> +      target = TargetFactory.forTab(options);
> +      return target.makeRemote(options);

I think you meant to whine to me about that in the work week, but we forgot about it. I don't remember any good reason for passing options twice in this case, I believe it's just an artifact of the TabTarget refactoring that I didn't finish. I'll file a bug to do just that.

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #15)
> (In reply to Mihai Sucan [:msucan] from comment #14)
> > Concerns:
> > 
> > - do we want to hide this by default? I'd rather not, seeing the Error
> > Console is enabled by default.
> 
> Error console is also hidden by default and is hidden behind two prefs, not
> only one.

The error console is enabled by default, temporarily. See bug 798925.


> > - but we cannot have both the Error Console and the Browser Console enabled
> > by default. What shall we do?
> 
> Since this does exactly what error console do, aren't we close to disabling
> error console all together ?

That's the plan, but we can't do that overnight (when we land this patch). An email to d-a-f is in order.

Comment 20

[Mihai Sucan [:msucan]]

(In reply to Joe Walker [:jwalker] from comment #17)
> Comment on attachment 724816 [details] [diff] [review]
> first cut
> 
> Review of attachment 724816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> ::: browser/base/content/browser.js
> @@ +1557,2 @@
> >                    gPrefService.getBoolPref("devtools.debugger.chrome-enabled") &&
> >                    gPrefService.getBoolPref("devtools.debugger.remote-enabled");
> 
> Please could we change enabled to remoteChromeEnabled or something?

Rename remote-enabled to remoteChromeEnabled? Do you want this change in this bug? I would favor a separate bug.


> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +162,5 @@
> > + *        - target: the target that the web console will connect to.
> > + *        - window: the window where the web console UI is already loaded.
> > + *        - chromeWindow: the window of the web console owner.
> > + *        - browserConsole: boolean that tells if this is a browser
> > + *        console or not.
> 
> I like the use of an options object. I think it's worth asking the question
> - 'how many of these are optional'. I wouldn't want to have an "All options
> must be optional" rule, but if most are and 1/2 are not, perhaps they should
> be params instead.

I see your point. I'm using |aOptions| as a way to provide arguments by name, to avoid changing the arguments every time when I add/remove some option (optional or not).

I'll change the constructor to use arguments for the required args, and aOptions for optional arguments.
 
> I guess that they're mandatory in the case of a BrowserConsole, but not
> otherwise. Perhaps this is a case where we should have a subclass? They'd
> basically be the same except for the  toggleBrowserConsole function.

This makes sense and I'll look into this possibility.


> ::: browser/devtools/webconsole/WebConsolePanel.jsm
> @@ +69,3 @@
> >      }
> >  
> > +    return deferredIframe.promise.then(function() promiseTarget)
> 
> I wonder if we shouldn't start fat-arrowing things?

Yes!


> ::: browser/devtools/webconsole/webconsole.xul
> @@ +41,2 @@
> >    </commandset>
> > +  <keyset id="consoleKeys">
> 
> Do we need the id at all?

I was thinking it makes it easier for xul extensions to change webconsole.xul with overlays. Or am I mistaken?

Comment 21

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #18)
> Comment on attachment 724816 [details] [diff] [review]
> first cut
> 
> Review of attachment 724816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should stick to the original plan to have the Global Console
> replace the Error Console (regardless if we change its name or not). If we
> can't get it to a state that is good enough for that, then I'd say let's
> kill-switch it (or back it out), but I believe there is no point in keeping
> both tools, especially since one of them is no longer actively maintained.
> Let's get feedback from firefox-dev when we are ready to do so.

Sounds good to me. However, having read comments in bug 798925 I believe there's some confusion within our teams related to the purpose of the error console, web console and soon the new browser console. I believe I should land this patch with the browser console off by default, no keyboard shortcut, and no touching of the error console. Then I can email firefox-dev (or d-a-f?) with an explanation of each tool's purpose and people can then test the browser console and see if they'd like it to replace the error console, maybe we get feedback on what we need to fix before we can do the replacement, etc. Thoughts?


> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +31,5 @@
> >  XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleUtils",
> >      "resource://gre/modules/devtools/WebConsoleUtils.jsm");
> >  
> > +XPCOMUtils.defineLazyModuleGetter(this, "webConsoleDefinition",
> > +    "resource:///modules/devtools/ToolDefinitions.jsm");
> 
> I wish we could use a common toolbox for both global console and browser
> debugger to avoid this sort of layering violations, but I agree that this is
> the right way to do it for now.

Agreed.

> @@ +541,5 @@
> > +        chrome: true,
> > +      };
> > +
> > +      target = TargetFactory.forTab(options);
> > +      return target.makeRemote(options);
> 
> I think you meant to whine to me about that in the work week, but we forgot
> about it. I don't remember any good reason for passing options twice in this
> case, I believe it's just an artifact of the TabTarget refactoring that I
> didn't finish. I'll file a bug to do just that.

Yes, this was the problem. Did you open the bug report by any chance?


Thanks to you and to Joe for the feedback.

Comment 22

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

(In reply to Mihai Sucan [:msucan] from comment #21)
> (In reply to Panos Astithas [:past] from comment #18)
> > @@ +541,5 @@
> > > +        chrome: true,
> > > +      };
> > > +
> > > +      target = TargetFactory.forTab(options);
> > > +      return target.makeRemote(options);
> > 
> > I think you meant to whine to me about that in the work week, but we forgot
> > about it. I don't remember any good reason for passing options twice in this
> > case, I believe it's just an artifact of the TabTarget refactoring that I
> > didn't finish. I'll file a bug to do just that.
> 
> Yes, this was the problem. Did you open the bug report by any chance?

I just did! Bug 857082.

Comment 23

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch, which includes a test for the Browser Console. I also tried to address Joe's and Panos's feedback.

Please review, test and let me know of any concerns you have.

Dave: I am asking you for review as well, since I am touching browser.js.

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

Do note the patch queue (in order): bugs 837723, 783499, 808370 and 587757.


Thank you!

Comment 24

[Mihai Sucan [:msucan]]

Updating the dependency to reflect the actual situation.

Comment 25

[Dave Townsend [:mossop]]

Comment on attachment 732962 [details] [diff] [review]
proposed patch

The non-devtools pieces look good to me

Comment 26

[Joe Walker [:jwalker]]

Comment on attachment 732962 [details] [diff] [review]
proposed patch

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

::: browser/base/content/browser.js
@@ +1576,3 @@
>                    gPrefService.getBoolPref("devtools.debugger.chrome-enabled") &&
>                    gPrefService.getBoolPref("devtools.debugger.remote-enabled");
>      if (enabled) {

This is a nit, and feel free to ignore, but the comment that I was making previously was that the 'enabled' variable is confusing, and perhaps 'remoteEnabled' would be a better name.

Comment 27

[Mihai Sucan [:msucan]]

(In reply to Joe Walker [:jwalker] from comment #26)
> Comment on attachment 732962 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 732962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +1576,3 @@
> >                    gPrefService.getBoolPref("devtools.debugger.chrome-enabled") &&
> >                    gPrefService.getBoolPref("devtools.debugger.remote-enabled");
> >      if (enabled) {
> 
> This is a nit, and feel free to ignore, but the comment that I was making
> previously was that the 'enabled' variable is confusing, and perhaps
> 'remoteEnabled' would be a better name.

Sorry, my English parser failed in your previous review comment. I understand the request now, and I'm going to update the patch accordingly.

Thank you for the r+!

Comment 28

[Mihai Sucan [:msucan]]

Attachment: fixed test

Tests pass now on the try servers. I had some oranges. Green try push https://tbpl.mozilla.org/?tree=Try&rev=2608048b7776

I also addressed Joe's last review comment.

Comment 29

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

Comment on attachment 734254 [details] [diff] [review]
fixed test

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

Looks good to me.

::: browser/devtools/webconsole/HUDService.jsm
@@ +679,5 @@
> +        chrome: true,
> +      };
> +
> +      target = TargetFactory.forTab(options);
> +      return target.makeRemote(options);

Now that bug 857082 landed, you can just do:

return TargetFactory.forRemoteTab(options);

Comment 30

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #29)
> Comment on attachment 734254 [details] [diff] [review]
> fixed test
> 
> Review of attachment 734254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +679,5 @@
> > +        chrome: true,
> > +      };
> > +
> > +      target = TargetFactory.forTab(options);
> > +      return target.makeRemote(options);
> 
> Now that bug 857082 landed, you can just do:
> 
> return TargetFactory.forRemoteTab(options);

Thank you, will update accordingly.

Comment 31

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/26fb3bd67f5f

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26fb3bd67f5f

Comment 33

[Mihai Sucan [:msucan]]

The documentation needed here is about the new Browser Console menu item that shows in the Web Developer menu. This new menu item is only visible if the user turns devtools.chrome.enabled to true in about:config.

The Browser Console can be described as a possible replacement for the Error Console: it shows all of the errors and warnings that happen throughout the browser, which is what the Error Console does. In addition to that, users get the same features as in the web console: window.console API support that works with any content window or chrome window, network logging, JS evaluation with chrome privileges and object inspection. All of this should work with extensions as well.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 35

[Mihai Sucan [:msucan]]

Thank you!

Comment 36

[Will Bamberg [:wbamberg]]

this got documented: https://developer.mozilla.org/en-US/docs/Tools/Browser_Console, so marking dev-doc-complete