920794 - Make Browser Console's default evaluation context the (last) browser window again

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[:Gijs (he/him)]

bug 860672 comment 25 notes that the browser console's input now gets evaluated against the console window, rather than against the browser window.

This is a bit inconvenient for browser developers like myself, and I imagine add-on authors will feel similarly. Is there any chance of changing the evaluation context back to the browser window, rather than the console, if and only if we're running in a browser?

Comment 1

[Mihai Sucan [:msucan]]

It would be confusing and inconsistent to have the browser console started from the Firefox browser window to target the browser window, but when you do -jsconsole to have it target the browser console window itself. I preferred consistency.

The new behavior seems to make sense - the Error Console evaluated JS in an iframe and made it less easy to break the browser chrome window by mistake. When we directly target the Firefox browser window anything you do could break the Firefox window.

If others believe we should target the Firefox browser window, when possible... we can try that.

Comment 2

[:Gijs (he/him)]

(In reply to Mihai Sucan [:msucan] from comment #1)
> It would be confusing and inconsistent to have the browser console started
> from the Firefox browser window to target the browser window, but when you
> do -jsconsole to have it target the browser console window itself. I
> preferred consistency.

I agree that could be confusing. When I tried the switch, it seems to also load a browser window, however. Can we just use a browser window if(/when it becomes) available? (and likewise, not break as badly as we do when that window goes away (bug 901885 - probably invalid at the current point in time))

We could use the window mediator to keep track of browser windows, and 'fix' on one as soon as one becomes available (you wouldn't want it to switch between several for no reason), and then find another one or revert to the web console window if the window we've picked goes away. We should probably show an info message in the console if it switches contexts.

I guess it boils down to what/whom we think the target is for the browser console. I've always thought of it as a tool for add-on/browser devs. We have the web console for web developers. Add-on/browser developers should know what they're doing when it comes to evaluating stuff, and not be surprised that if they, say, set gBrowser = null, the world starts breaking in weird places. Maybe my assumptions about the users of the browser console is wrong?

Comment 3

[Mihai Sucan [:msucan]]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Mihai Sucan [:msucan] from comment #1)
> > It would be confusing and inconsistent to have the browser console started
> > from the Firefox browser window to target the browser window, but when you
> > do -jsconsole to have it target the browser console window itself. I
> > preferred consistency.
> 
> I agree that could be confusing. When I tried the switch, it seems to also
> load a browser window, however. Can we just use a browser window if(/when it
> becomes) available? (and likewise, not break as badly as we do when that
> window goes away (bug 901885 - probably invalid at the current point in
> time))
> 
> We could use the window mediator to keep track of browser windows, and 'fix'
> on one as soon as one becomes available (you wouldn't want it to switch
> between several for no reason), and then find another one or revert to the
> web console window if the window we've picked goes away. We should probably
> show an info message in the console if it switches contexts.

This would be even more confusing/magical. The way the console actors work is that you pick the console actor that the client attaches to - the global console actor or some tab-specific console actor. Changing the window you evaluate into would be rather weird/confusing *after* you attach to the global console actor.


> I guess it boils down to what/whom we think the target is for the browser
> console. I've always thought of it as a tool for add-on/browser devs. We
> have the web console for web developers. Add-on/browser developers should
> know what they're doing when it comes to evaluating stuff, and not be
> surprised that if they, say, set gBrowser = null, the world starts breaking
> in weird places. Maybe my assumptions about the users of the browser console
> is wrong?

I see the browser console as a tool for (1) add-on/browser devs and (2) users to help with debugging browser/addon-related issues. The JS eval feature is meant to give you a chrome-privileged JS input - nothing specific about the target. Given that the browser console needs to work on its own, without any other window open, it made sense to make it use its own window. Since it's all chrome code, you can still acquire any browser window.

We plan to add support to pick different iframes in the web console, as the JS eval scope, see bug 609872. I believe a much nicer approach would be for the browser console to provide some sort of UI to allow the user to pick the JS eval scope - any of the browser windows, and addon contexts.

Comment 4

[:Gijs (he/him)]

(In reply to Mihai Sucan [:msucan] from comment #3)
> The way the console actors work
> is that you pick the console actor that the client attaches to - the global
> console actor or some tab-specific console actor. Changing the window you
> evaluate into would be rather weird/confusing *after* you attach to the
> global console actor.

I'm not familiar with the actor models used for the devtools, so I don't understand what this means (was the global console actor always using a browser window before, but not anymore, or something?). It sounds to me like you're saying the implementation would be tricky. Isn't that orthogonal to whether or not implementing it is a good idea from a user perspective?

Comment 5

[Mihai Sucan [:msucan]]

From the user perspective I believe that the JS execution scope should be consistent, irrespective of how/when I start the browser console, or how I decide to close/open other windows in Firefox.

However, I am willing to be convinced otherwise.

Comment 6

[:Gijs (he/him)]

(In reply to Mihai Sucan [:msucan] from comment #5)
> From the user perspective I believe that the JS execution scope should be
> consistent, irrespective of how/when I start the browser console, or how I
> decide to close/open other windows in Firefox.
> 
> However, I am willing to be convinced otherwise.

I think that the following is fairly intuitive as an end-user:

1) when the console is open and no browsers are open, it uses its own window
2) when the console is open and exactly one browser window is open, it uses that browser window
3) when the console is open and more than one browser window is open, it uses the window from which it was opened, or (if it wasn't opened from one of them) the browser window it was using when there was 1 window (that is, opening a second/nth window doesn't change the evaluation context).

In cases 2 or 3, if the browser window it uses to evaluate closes, we should
a) display a message to the user that their evaluation context has passed away, and that we picked a new one for them, namely:
b) the next most recent browser window, or the console if there are no other browser windows.

I think this will DWIM for users in 99% of cases. And yes, giving them the option of picking a particular window (and then sticking to that until it closes, in which case see the above paragraph) is a great idea, but I also think we should try to make life easy for users and not make them have to switch to the browser window manually themselves every time they start the browser.

I agree that consistency is important. Unless I missed something, the above rules are "stable" as long as no browser windows close. That is, the evaluation context will always be the same unless you close the window in which we're evaluating. As it would always be the user who closes them, I think it would be expected that stuff you did in/to the browser window is now gone (just like if you closed the tab with the web console open).

Comment 7

[:Gijs (he/him)]

Attachment: make browser console DWIM,

It is my understanding from earlier IRC conversations that the solution I outlined in comment 6 seemed to be acceptable. So I've tried to write a patch for it. This doesn't implement the notification when windows change, but otherwise should act as described. I'm not sure how best to go about that because I don't know my way around the devtools codebase very well, but I imagine that code lives somewhere else than in the server. I'm happy to write a patch there as well, if you could provide some minor pointers as to where to look in the codebase.

Comment 8

[:Gijs (he/him)]

Attachment: make browser console DWIM,

Oops, stray dump from me trying to figure out how this all worked...

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 817473 [details] [diff] [review]
make browser console DWIM,

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

Thank you for the patch!

::: toolkit/devtools/server/actors/webconsole.js
@@ +129,5 @@
> +      if (!window || window.closed) {
> +        this.lastChromeWindow = null;
> +        window = this.parentActor.window;
> +        if (window) {
> +          this.lastChromeWindow = Components.utils.getWeakReference(window);

Cu?

@@ +144,5 @@
> +                Services.obs.removeObserver(onChromeWindowOpened, "domwindowopened");
> +                this.lastChromeWindow = null;
> +              }
> +            }.bind(this);
> +            Services.obs.addObserver(onChromeWindowOpened, "domwindowopened", false);

Do we really need this observer? I think we can just return window here, the devtools:webconsole window, but don't set this.lastChromeWindow = window, so next time the global console actor is requested we check again if there's any suitable parentActor.window.

Comment 10

[:Gijs (he/him)]

Attachment: make browser console DWIM,

So this fixes the nit and adds a message. I left the observer after discussions on IRC about why it's there, but I had to remove the parentActor check because it turned out that there's no guarantee that getMostRecentWindow finds that window when we get notified...

The patch has two remaining issues that I found, which could be followups or in here if necessary:
1) autocomplete results are cached "cross windows". So if you type 'gBr' when the browser window is open, complete to gBrowser, close the browser window, and then type 'gBr' again, the same autocomplete results come up. I'm imagining this needs to be fixed in the autocomplete handler...
2) In order to stop the message showing up (twice - that's a separate bug, because logStringMessage was only being called once...) when the window is first opened, I had to add an ugly boolean tracking variable. This is beceause the observer explicitly sets lastChromeWindow to null, so checking if that value is falsy doesn't work. Of course, I could use a strict check in one place and set a non-null-but-still-falsy value in the observer, but that seemed very ugly...

Comment 11

[:Gijs (he/him)]

Attachment: make browser console DWIM,

And I should learn to qref more often.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 818907 [details] [diff] [review]
make browser console DWIM,

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

(will review the patch early next week just a quick comment)

Does console.properties belong to the error console? I am not sure if we should reuse it, but I wouldn't add another .properties file just for one string either. 

Also, trying to review/understand the getter for |window| makes me cringe at its complexity. We should think about simplifying that getter, split a part of it into a function, or some simpler logic...

(just a thought)

::: toolkit/locales/en-US/chrome/global/console.properties
@@ +12,5 @@
> +
> +# LOCALIZATION NOTE (evaluationContextChanged): The message displayed when the
> +# console's evaluation context (window against which input is evaluated)
> +# changes.
> +evaluationContextChanged=The console's evaluation context changed, probably because that window was closed or because you opened a main window from the browser console's window.

s/because that window/because the target window/

(what do you think?)

Also mention this is shown in the Browser Console, not in the Error Console.

Comment 13

[:Gijs (he/him)]

(In reply to Mihai Sucan [:msucan] from comment #12)
> Comment on attachment 818907 [details] [diff] [review]
> make browser console DWIM,
> 
> Review of attachment 818907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (will review the patch early next week just a quick comment)
> 
> Does console.properties belong to the error console? I am not sure if we
> should reuse it, but I wouldn't add another .properties file just for one
> string either. 

Yes. An alternative would be debugger.properties, which belongs to the browser debugger, it seems. I figured this would be the lesser of three evils.


> ::: toolkit/locales/en-US/chrome/global/console.properties
> @@ +12,5 @@
> > +
> > +# LOCALIZATION NOTE (evaluationContextChanged): The message displayed when the
> > +# console's evaluation context (window against which input is evaluated)
> > +# changes.
> > +evaluationContextChanged=The console's evaluation context changed, probably because that window was closed or because you opened a main window from the browser console's window.
> 
> s/because that window/because the target window/
> 
> (what do you think?)
> 
> Also mention this is shown in the Browser Console, not in the Error Console.

Good points. :-)

Comment 14

[Victor Porof [:vporof][:vp]]

(In reply to :Gijs Kruitbosch from comment #13)
> 
> Yes. An alternative would be debugger.properties, which belongs to the
> browser debugger, it seems. I figured this would be the lesser of three
> evils.

debugger.properties has strings for both the content *and* browser debugger.

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 818907 [details] [diff] [review]
make browser console DWIM,

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

Thank you for the patch, this is looking good, but I cannot give it r+ yet.

- Two tests fail now that the browser console changed its target.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_console.js | Timed out while waiting for: messages displayed
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_console.js | jsterm eval result is displayed - Didn't expect -1, but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_console_dead_objects.js | leaked window property: chromeWindow


- As discussed on IRC I still want to see the getter simplified. Here is my proposal:

https://pastebin.mozilla.org/3311820

I moved the |if (this.parentActor.isRootActor) { ... }| code block into a separate function, getBrowserConsoleWindow(). Then I noticed the hadChromeWindow and lastChromeWindow properties were being set multiple times, so I did a bit of code reorder / cleanup. I hope that makes it easier to follow.


- I am yet undecided about where we should put the new strings. Can you please ask Mossop if console.properties is an acceptable place?

Thanks!

::: toolkit/devtools/server/actors/webconsole.js
@@ +1632,5 @@
> +
> +/**
> + * Localization convenience methods.
> + */
> +let L10N = {

You do not need this stuff here. Just require the l10n object from webconsole/utils. See how this is done in browser/devtools/webconsole/webconsole.js (search for l10n).

Comment 16

[Mihai Sucan [:msucan]]

I forgot to mention I would prefer hadChromeWindow and lastChromeWindow properties to be private.

Comment 17

[:Gijs (he/him)]

Attachment: make browser console DWIM,

This should address all the comments, and fixes the tests.ents, and fixes the tests. I've run this through all the browser/devtools tests locally, and they pass. Note that I've put the l10n object on the actor because using a global conflicts with existing globals used by the debugger server, which includes this file, and so the JS engine starts yelling about redeclaring stuff.

Comment 18

[:Gijs (he/him)]

(In reply to Mihai Sucan [:msucan] from comment #15)
> - I am yet undecided about where we should put the new strings. Can you
> please ask Mossop if console.properties is an acceptable place?
> 
> Thanks!

Mossop, we want a string to tell the user that the evaluation object for the console has changed. The browser console doesn't have its own .properties file yet, so we were thinking of adding it to the console.properties file also used by the error console. Do you think that's OK? :-)

Comment 19

[:Gijs (he/him)]

Attachment: make browser console DWIM,

Forgot to update the string with the feedback from comment 12.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gijs Kruitbosch from comment #18)
> Mossop, we want a string to tell the user that the evaluation object for the
> console has changed. The browser console doesn't have its own .properties
> file yet, so we were thinking of adding it to the console.properties file
> also used by the error console. Do you think that's OK? :-)

That's fine!

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 820358 [details] [diff] [review]
make browser console DWIM,

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

Thanks for the updated patch. Please go for a try push and make sure this is green.

r+ with the comments below addressed, and a green try push. Thanks!

::: toolkit/devtools/server/actors/webconsole.js
@@ -34,5 @@
>      enumerable: true
>    });
>  }
>  
> -

Is this change needed?

@@ +64,5 @@
>    if (this.parentActor.isRootActor) {
>      Services.obs.addObserver(this._onObserverNotification,
>                               "last-pb-context-exited", false);
>    }
> +  this.l10n = new WebConsoleUtils.l10n("chrome://global/locale/console.properties");

Please put this directly on the WebConsoleActor object, as a static property.

@@ +127,5 @@
>      }
>      return this.parentActor.window;
>    },
>  
> +  _getWindowForBrowserConsole: function() {

Please add a jsdoc comment, and a name for the function, and put the { on a new line.

(follow the file coding style)

@@ +152,5 @@
> +
> +    return window;
> +  },
> +
> +  _handleNewWindow: function(window) {

Same as above.

@@ +165,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Whether we've been using a window before:

s/:/./

Please add @type info and @private. (coding style consistency)

@@ +172,5 @@
> +
> +  /**
> +   * A weak reference to the last chrome window we used to work with.
> +   */
> +  _lastChromeWindow: null,

Ditto.

::: toolkit/locales/en-US/chrome/global/console.properties
@@ +11,5 @@
>  errTime=Timestamp: %S
> +
> +# LOCALIZATION NOTE (evaluationContextChanged): The message displayed when the
> +# console's evaluation context (window against which input is evaluated)
> +# changes.

This localization note doesn't mention the browser console. It should.

Comment 22

[:Gijs (he/him)]

Attachment: make browser console DWIM,

Nits fixed in this patched, pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=ab5b0529d4ff

Comment 23

[:Gijs (he/him)]

Attachment: make browser console DWIM (for checkin)

rm spurious newline change

Comment 24

[:Gijs (he/him)]

Comment on attachment 821569 [details] [diff] [review]
make browser console DWIM (for checkin)

Carrying over r+

Comment 25

[:Gijs (he/him)]

Comment on attachment 821569 [details] [diff] [review]
make browser console DWIM (for checkin)

Try was green, landed as https://hg.mozilla.org/integration/fx-team/rev/fb8fabe1e2bc

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb8fabe1e2bc