979991 - "Browser console" menu item toggles when it should just open/focus

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Open a fullscreen browser window.
2)  Tools > Web Developer > Browser Console
3)  Click on the browser window to raise it above the console window
4)  Tools > Web Developer > Browser Console

ACTUAL RESULTS: Nothing happens (or more precisely an invisible window is closed).

EXPECTED RESULTS: The browser console window is raised again.

The error console used to get this right.

Comment 1

[Abhishek Potnis [:avp]]

Hi Mihai,

I would like to work on this bug, can you please guide me on getting started with it?

Thanks.

Comment 2

[Mihai Sucan [:msucan]]

Abhishek, thanks for your interest to help!

For this bug you need to change browser-sets.inc. Find the Tools:BrowserConsole command. Change it to call a new HUDService method. Instead of toggleBrowserConsole() you can do openBrowserConsoleOrFocus().

Then also edit hudservice.js. Add the new method. This should look like:

let hud = this.getBrowserConsole();
if (hud) {
  hud.iframeWindow.focus();
  return promise.resolve(hud);
}
else {
  return this.toggleBrowserConsole();
}

Comment 3

[Abhishek Potnis [:avp]]

Attachment: bug-979991-fix.diff

Hi Mihai,

Thanks for the quick reply and detailed explanation !

I have made the changes, but how do I test my patch? I have built Firefox, now how must I proceed to test out my patch?

Thanks.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 8394717 [details] [diff] [review]
bug-979991-fix.diff

To test this patch: rebuild firefox, open the build, press ctrl-shift-j, check it opens the browser console, then go to the firefox window, press ctrl-shift-j again, check the browser console is focused (not closed).

To write a test: grep in the browser/devtools/webconsole/test folder for all tests that use toggleBrowserConsole(). Use those tests for inspiration. Write a new test that does what I explained in the previous paragraph.

Thank you!

Note: at the end of hudservice.js you will need to expose the new method. It's not available to the scripts that require() hudservice.

Comment 5

[Abhishek Potnis [:avp]]

Hi Mihai,

I exposed the new method at the end of hudservice.js and rebuilt firefox. The patch worked fine.

I am finding it difficult to write a test for the same. I went through <http://mxr.mozilla.org/mozilla-central/search?string=toggleBrowserConsole&find=browser/devtools/webconsole/test>, but I am not able to understand where to look. Could you please point me the file(s) which could help? Also is there any documentation, which I can refer, to understand the structure of a test?

Thanks.

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Abhishek Potnis [:avp] from comment #5)
> Hi Mihai,
> 
> I exposed the new method at the end of hudservice.js and rebuilt firefox.
> The patch worked fine.

Nice.

> I am finding it difficult to write a test for the same. I went through
> <http://mxr.mozilla.org/mozilla-central/
> search?string=toggleBrowserConsole&find=browser/devtools/webconsole/test>,
> but I am not able to understand where to look. Could you please point me the
> file(s) which could help? Also is there any documentation, which I can
> refer, to understand the structure of a test?

To add a test, please learn about browser chrome tests:

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Go to browser/devtools/webconsole/test and copy one of the "browser_" JS test files. Name the new file as you see fit, add the new file name to the list of tests in browser.ini. Update the test to test the bug you are fixing. Run mach/make to update the local build, then run the test as explained in the provided link.

I suggest you look into the tests you found with MXR. The browser_console.js test file opens the browser console using the keyboard shortcut, so you can reuse that.

Thanks!

Comment 7

[Abhishek Potnis [:avp]]

Attachment: WIP

Hi Mihai,

I have created a sample test file, added it in the browser.ini and built firefox. But now, even the already written tests fail. I have pastebined the Test Results here: http://pastebin.mozilla.org/4902849

When asked on IRC, I was told to this "touch browser/devtools/webconsole/moz.build", but to no use. Bugs related to this issue have been filed :
Bug 998849 and Bug 997744.

Please guide me on this.

Thanks.

Comment 8

[Mihai Sucan [:msucan]]

Abishek, apologies for this delayed reply. I was away for since late April until this week. Do you have time to finish this patch or should we do this? If you want to finish it, please update the patch and let me know how I can help.

Please use the needinfo? flag when you have questions for me, so I can reply quicker. Thanks!

Comment 9

[Abhishek Potnis [:avp]]

Hi Mihai,

The code has changed a lot since my last patch, I will take a look and upload the patch in a day.

Thanks.

Comment 10

[Abhishek Potnis [:avp]]

Hi Mihai,

Apologies, I am not able to find time to code the patch. Please feel free to fix it. I am unassigning myself.

Comment 11

[Nachiketh]

I am a beginner, I would like to contribute by fixing this bug. Can I take it up?

Comment 12

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

Hi Nachiketh, feel free to work on this bug. Abhishek has already made great progress in the attached patch. Read Mihai's comments above and try to rebase the existing patch first. Then you can work on finishing the test case.

Let me know if you need anything, here or in IRC.

Comment 13

[Mrinal Dhar]

Hi, is anybody still working on this? If not, I'd like to complete this one.

Comment 14

[Mrinal Dhar]

Attachment: Added function to open browser console or bring existing one into focus, instead of closing it

I've written a test file for this, but I need a little help. 

The tests run, but seem to fail in the end and gives the error: "Found an unexpected devtools:webconsole at the end of test run - expected PASS"

Also, how do I close the browser console? Since the destroy method is not exposed, I tried using the toggleBrowserConsole.

Comment 15

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

Comment on attachment 8554274 [details] [diff] [review]
Added function to open browser console or bring existing one into focus, instead of closing it

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

Thanks for the patch, but next time please remember to flag me for feedback!

::: browser/devtools/webconsole/test/browser_console_open_or_focus.js
@@ +4,5 @@
> + */
> +
> +"use strict";
> +
> +let test = asyncTest(function*() {

Please add a short comment about the purpose of this test, like we do elsewhere.

@@ +17,5 @@
> +  info("change focus back to window");
> +  window.focus();
> +
> +  EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }, window);
> +  hud = yield opened;

This is not useful, as you are yielding an already fulfilled promise. What needs to happen here is to check which window appears on top with Services.wm.getMostRecentWindow(null) and verify that it is the browser console window (e.g. check that the URL loaded in it is devtools.Tools.webConsole.url). 

See here for more details on that method:

https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWindowMediator#getMostRecentWindow%28%29

@@ +18,5 @@
> +  window.focus();
> +
> +  EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }, window);
> +  hud = yield opened;
> +  ok(hud, "browser console is still open");

The important test here is that the console window is on top.

@@ +20,5 @@
> +  EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }, window);
> +  hud = yield opened;
> +  ok(hud, "browser console is still open");
> +
> +  HUDService.toggleBrowserConsole();    // Code goes wrong here somewhere. Cant close the browser console.

toggleBrowserConsole() returns a promise, so you have to yield it for the test to wait until it is closed.

@@ +22,5 @@
> +  ok(hud, "browser console is still open");
> +
> +  HUDService.toggleBrowserConsole();    // Code goes wrong here somewhere. Cant close the browser console.
> +  hud = yield opened;
> +  ok(!hud, "browser console has been closed.");

Again, yielding the fulfilled promise is not useful, but you can use getBrowserConsole() to check if the console is open or not.

Comment 16

[Mrinal Dhar]

Attachment: Diff with the modified test

Changed the test function's workflow to check console focus instead of existence. 

The attached code brings up the console by directly calling the newly written HUDService.openBrowserConsoleOrFocus().

For some reason, this just doesn't work with EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }); 
The getBrowserConsole() is not able to fetch the console if I use the EventUtils to start the console.

I tried putting the getBrowserConsole in a setTimeout() and also in an executeSoon(), but that didn't do much good either.

Comment 17

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

Sorry for the review delay. I'm OK with using openBrowserConsoleOrFocus() directly instead of simulating the keystrokes, but the test has two remaining flaws:
- it doesn't switch the focus to the browser window between the two calls to openBrowserConsoleOrFocus(), therefore the checks are trivially true
- it doesn't wait long enough for the console to become operational, using something like waitForMessages, so the window never gets rendered at all on my fast laptop

That last bit might have also been the reason for your synthesizeKey troubles, but never mind that. Fix these two issues, then fold the two patches together and set the review flag on the patch to my name (not needinfo, unless you are not ready for review and just want to ask a question).

This is really close!

Comment 18

[Mrinal Dhar]

Attachment: Patch with fix and tests

Fixed the two issues you were talking about in the previous comment. Tests run fine.

Comment 19

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

Comment on attachment 8562668 [details] [diff] [review]
Patch with fix and tests

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

r=me with comments addressed.
Make sure you also edit the cset comment, to remove the 2 extra lines from folding the two patches and add r=past in the end.

Do a try run in Linux, Mac and Windows platforms, but only for mochitest-dt and mochitest-e10s-devtools-chrome test suites. If you don't have your L1 access by then, needinfo me and I'll push to try for you.

::: browser/devtools/webconsole/hudservice.js
@@ +776,5 @@
>  const HUDService = new HUD_SERVICE();
>  
>  (() => {
>    let methods = ["openWebConsole", "openBrowserConsole",
> +                 "toggleBrowserConsole", "openBrowserConsoleOrFocus", "getOpenWebConsole",

Nit: put the new item last, in a new line with the current last item. This way no line is beyond the code style limit and every line tidily contains the same number of items.

::: browser/devtools/webconsole/test/browser_console_open_or_focus.js
@@ +3,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +
> +//  Test that the "browser console" menu item opens or focuses (if already open) the console window 

Nit: trailing whitespace

@@ +29,5 @@
> +    }],
> +  });
> +
> +  currWindow = Services.wm.getMostRecentWindow(null);
> +  is(currWindow.document.documentURI, devtools.Tools.webConsole.url, "The Browser Console is open and has focus");

Nit: fold this overly long line to fit 80 columns. Aligning the third argument below the first should do it. Same for the other long line further down.

@@ +33,5 @@
> +  is(currWindow.document.documentURI, devtools.Tools.webConsole.url, "The Browser Console is open and has focus");
> +
> +  mainWindow.focus();
> +
> +  EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true });

Use "yield HUDService.openBrowserConsoleOrFocus();" here instead of synthesizeKey, so that the test waits for the console to come in front of the main window. Otherwise you need to tweak the rest of the test to wait on a promise that is fulfilled once the key event handler is done.

For future reference, you want to run these tests both with and without the --e10s flag, to make sure the test works in e10s (soon to become the default in Firefox). Running the test in e10s mode, without this change, fails.

@@ +34,5 @@
> +
> +  mainWindow.focus();
> +
> +  EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true });
> +  

Nit: trailing whitespace

Comment 20

[Mrinal Dhar]

Attachment: Final patch with nits fixed.

Fixed the nits.

I added it to the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=169d1efd119f

I hope that is how it was to be done.

Comment 21

[Mrinal Dhar]

Hi, Panos. 
All tests ran successfully. Is there anything else that needs to be done here?

Comment 22

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

You missed a few of my nits, but I took care of them before pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/dbed1e3c1a1e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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