1210482 - Write tests for window.[location|menu|personal|status|tool]bar.visible

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Bug 1194897 turned out to be the result of us having broken window.[location|menu|personal|status|tool]bar.visible for e10s.

We should probably have some regression tests for those.

Comment 1

[Antonio de Luna [:A-deLuna]]

Hey Mike, love your streams, I'd like to work on this. Where should the tests live?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey A-deLuna!

Glad you like the streams!

I think the tests would probably make the most sense under dom/tests/browser, as I think we might want to open a few windows with chrome privileges as well to make sure we can set the toolbar bits, etc.

So, let's try the following:

1) Content tests, which will test the two kinds of popup windows:
  a) Full-browser popups, like those created with window.open('http://foo.com', '_blank');. These should have all toolbars report as visible.
  b) "Compact" popups, when we pass non-default features, like disabling the menubar, toolbar, and location (though we should test that setting location to false should NOT change window.locationbar.visible, as the location feature is ignored by default).
2) Chrome-privileged tests, which should open popup windows with the location bar, personal bar, menubar, etc disabled.

That's kind of high-level and hand-wavey, but that's how I'm thinking. Do you have any questions on how to proceed?

Comment 3

[Antonio de Luna [:A-deLuna]]

Just a couple of questions,

for 1-a and 1-b I should do browser chrome tests https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

for 2 I should use https://developer.mozilla.org/en/docs/Chrome_tests

Also maybe I should place the tests in a different folder as they are disabled for e10s https://dxr.mozilla.org/mozilla-central/source/dom/tests/browser/browser.ini#2

Comment 4

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1210482-part1

Hello Mike, this is what I have so far for the first type of tests, it's mostly working as expected except for this test: 

>is(compactWindow.locationbar.visible, true,
>+        "locationbar should be visible even with location=no");

it's returing true but as you said, it should be returning false. Weird thing is that if run the same steps of the test on my local build's console it works as expected, what am I missing here?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682323 [details] [diff] [review]
bug-1210482-part1

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

Hey A-deLuna,

See below for some ideas on how to approach this. There are probably ways for reducing the amount of duplication between each test, but I suppose we can address that in a later iteration.

Let me know if you have any questions!

::: dom/tests/browser/browser_test_toolbars_visibility.js
@@ +1,1 @@
> +function test() {

I think doing this as browser mochitests is fine. We should, however, take advantage of some utilities that exist to make doing async things easier to write and read.

For one thing, we should probably use add_task, instead of function test.

Example:

const PAGE = "http://www.example.com/browser/dom/tests/browser/test_new_window_from_content_child.html";

add_task(function() {
  // This test can now yield DOM Promises, which makes it easier
  // to write async tests!

  // Then we need to load content that will open windows. Thankfully,
  // there's already an HTML file in dom/tests/browser that you can use -
  // test_new_window_from_content_child.html

  // You can then load that page using this:
  yield BrowserTestUtils.withNewTab({
    gBrowser,
    url: PAGE,
  }, function*(browser) {
    // At this point, the browser is loaded at the page, and we're ready to click on
    // items.
    let newWinPromise = BrowserTestUtils.waitForNewWindow();
    yield BrowserTestUtils.synthesizeMouseAtCenter("#winOpenDefault", {}, browser);
    let win = yield newWinPromise;

    // Now win is the new window we just opened by clicking on the
    // winOpenDefault item in test_new_window_from_content_child.html.

    // Now we can run some code in there to test that the toolbars are visible
    // as we'd expect.
    let toolbars = yield ContentTask.spawn(browser, null, function*() {
      // This code is run in the content!
      return {
        toolbar: content.toolbar.visible,
        personalbar: content.personalbar.visible,
        // etc
      };
    });

    ok(toolbars.toolbar, "Toolbar should be visible");
    ok(toolbars.personalbar, "Personal Bar should be visible");

    yield BrowserTestUtils.closeWin(win);

    // Next we open the window with #winOpenNonDefault, and make sure the right
    // toolbar visibility values are sent back up from ContentTask. Note that
    // locationbar should _always_ be visible.

    // ...

    // Then, test windows that are opened from this testing scope, meaning
    // that they're opened from chrome privilege. We should make sure the
    // right toolbar visibility settings get sent down there too.
  });

});

Comment 6

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1210482.patch

Hello Mike, took me a while to learn about promises, yield, generator functions and Task.jsm but I think this might be a step in the right direction.

I found that statusbar.visible will return true even when it's hidden as per bug#55820, what should I do about this?

Also I tried to remove repetition but couldn't manage to find a way to make a getToolbars function that would work for both a tab and a window, what do you suggest me to do?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8684078 [details] [diff] [review]
bug-1210482.patch

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

Hey Antonio,

This looks great - it's awesome how quickly you picked up the Promise / Task stuff! :D

I think you're doing a good job here detecting the content case, but the chrome case isn't exactly right (and probably will mean having it's own special testing add_task function, perhaps).

With chrome, what you'll want to do is call window.open directly from the test, since the test has chrome privileges. You'll then want to capture the visibility of the opened popup window. Note, however, that the chrome-privileged window can have all of its toolbars shut off. Example, opening the Browser Console (Ctrl-Shift-J), and putting in

window.open("https://www.mikeconley.ca", "_blank", "toolbar=yes,personalbar=no,location=no,resizable=yes")

Should successfully open a window that has no location bar. This is okay because Browser Console has chrome privileges. And you can duplicate that sort of thing in your test by using window.open directly in your test file. You can then access the visibility of the toolbars from the opened window using something like your getToolbarsFromWindow.

Great work! Let me know if you have any questions!

::: dom/tests/browser/browser_test_toolbars_visibility.js
@@ +1,1 @@
> +

Nit - extra newline.

@@ +1,2 @@
> +
> +// Tests that toolbars have proper visibility when a new window is opened

Should mention that you test both the chrome and content contexts.

@@ +1,4 @@
> +
> +// Tests that toolbars have proper visibility when a new window is opened
> +
> +const contentPage = "http://www.example.com/browser/dom/tests/browser/test_new_window_from_content_child.html";

consts should be differentiated from other variables - we usually put them in ALL_CAPS.

@@ +4,5 @@
> +const contentPage = "http://www.example.com/browser/dom/tests/browser/test_new_window_from_content_child.html";
> +const chrome = "context chrome";
> +const content = "context content";
> +
> +function getToolbarsFromBrowser(context, aBrowser){

Each helper function should have documentation. I like this format:

/**
 * Description of what the function does.
 *
 * @param context (CHROME or CONTENT)
 *        The context being tested.
 * @param aBrowser (<xul:browser>)
 *        The browser to query for toolbar visibility states
 * @returns Promise
 *        A Promise that resolves when x y z with such and
 *        such a value.
 */

@@ +20,5 @@
> +      break;
> +    case chrome:
> +      return Task.spawn(function*() {
> +        return {
> +          toolbar: toolbar.visible,

I don't think this works as advertised. This is going to always get the toolbar values for the window that the test is running in, and not necessarily for the browser that's being passed in.

I don't actually think we need getToolbarsFromBrowser for the chrome case, so this can probably go away.

@@ +45,5 @@
> +        };
> +      });
> +      break;
> +    case chrome:
> +      return Task.spawn(function*() {

Because this Task is not actually yielding anything, you can return a resolving Promise directly, like:

return Promise.resolve({
  toolbar: win.toolbar.visible,
  // etc
});

@@ +60,5 @@
> +}
> +
> +function testDefaultToolbars(toolbars){
> +  ok(toolbars.locationbar,
> +      'locationbar should be visible on default window.open()');

I'm seeing inconsistency with single and double-quotes. Might as well stick with double.

@@ +72,5 @@
> +      'toolbar should be visible on default window.open()');
> +}
> +
> +function testPopupToolbars(toolbars){
> +  // Locationbar should always be visible

Good documentation in here.

@@ +73,5 @@
> +}
> +
> +function testPopupToolbars(toolbars){
> +  // Locationbar should always be visible
> +  is(toolbars.locationbar, true,

Should probably just use ok, like:

ok(toolbars.locationbar,
   "locationbar should be visible even with location=no");
ok(!toolbars.menubar,
   "menubar shouldn't be visible when menubar=no");

// etc

@@ +86,5 @@
> +  is(toolbars.toolbar, false,
> +      "toolbar shouldn't be visible when toolbar=no");
> +}
> +
> +function testToolbarVisibilityFromContext(context){

Space before {. That goes for every function in here.

@@ +91,5 @@
> +  return BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: contentPage,
> +  }, function*(browser) {
> +

Nit: Extra newline.

@@ +108,5 @@
> +    // Now let's open a window with toolbars hidden
> +    let winPromise = BrowserTestUtils.waitForNewWindow();
> +    yield BrowserTestUtils.synthesizeMouseAtCenter("#winOpenNonDefault", {}, browser);
> +    let popupWindow = yield winPromise;
> +

Nit - extra newline

Comment 8

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1210482.patch

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8684605 [details] [diff] [review]
bug-1210482.patch

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

This looks fantastic! Just some final fixes, and I think we're done. Great work!

::: dom/tests/browser/browser_test_toolbars_visibility.js
@@ +52,5 @@
> + *        the visibility state of the toolbar elements
> + */
> +function testDefaultToolbars(toolbars) {
> +  ok(toolbars.locationbar,
> +      "locationbar should be visible on default window.open()");

The strings should be aligned to the first character in the first argument - example:

```Javascript

ok(toolbars.locationbar,
   "locationbar should be visible on default window.open()");

```

Same goes for testNonDefaultContentToolbars and testNonDefaultChromeToolbars.

@@ +131,5 @@
> +    // Check that all toolbars are visible
> +    let toolbars = yield getToolbarsFromBrowserContent(gBrowser);
> +    testDefaultToolbars(toolbars);
> +
> +    //cleanup

Space after //

@@ +168,5 @@
> +  let hiddenToolbars = getToolbarsFromWindowChrome(popupWindow);
> +  testNonDefaultChromeToolbars(hiddenToolbars);
> +
> +  // cleanup
> +  defaultWindow.close();

Closing windows is async, so try:

yield BrowserTestUtils.closeWindow(defaultWindow);
yield BrowserTestUtils.closeWindow(popupWindow);

(when you move this into the add_task)

@@ +175,5 @@
> +  popupWindow = null;
> +}
> +
> +add_task(function*() {
> +  yield testToolbarVisibilityFromContent();

Since testToolbarVisibilityFromContent and testToolbarVisibilityFromChrome are only called once each, I don't think they need to be functions defined outside these tasks. Might as well just drop their code into these add_tasks.

Comment 10

[Antonio de Luna [:A-deLuna]]

Hi Mike, thanks for the reviews!.
A quick question, if I'm going to be doing this for the chrome tests:
>yield BrowserTestUtils.closeWindow(defaultWindow);
>yield BrowserTestUtils.closeWindow(popupWindow);

Should I also use BrowserTestUtils.waitForNewWindow() when opening the windows with window.open()?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Antonio de Luna [:A-deLuna] from comment #10)
> Hi Mike, thanks for the reviews!.
> A quick question, if I'm going to be doing this for the chrome tests:
> >yield BrowserTestUtils.closeWindow(defaultWindow);
> >yield BrowserTestUtils.closeWindow(popupWindow);
> 
> Should I also use BrowserTestUtils.waitForNewWindow() when opening the
> windows with window.open()?

Yes, that's a good idea!

Comment 12

[Antonio de Luna [:A-deLuna]]

Final question Mike I promise,

should I be doing this:
>let defaultWindow = window.open("about:robots", "_blank");
>yield BrowserTestUtils.waitForNewWindow();

or this:
>let windowPromise =  BrowserTestUtils.waitForNewWindow(); 
>window.open("about:robots", "_blank");
>let defaultWindow = yield windowPromise;

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Antonio de Luna [:A-deLuna] from comment #12)
> Final question Mike I promise,
> 
> should I be doing this:
> >let defaultWindow = window.open("about:robots", "_blank");
> >yield BrowserTestUtils.waitForNewWindow();
> 
> or this:
> >let windowPromise =  BrowserTestUtils.waitForNewWindow(); 
> >window.open("about:robots", "_blank");
> >let defaultWindow = yield windowPromise;

I'd prefer the latter. I'm pretty sure they're equivalent (the event listeners and observers will get hooked up before the window gets a chance to start opening), but I'm always just a bit antsy about it. So let's go with Option 2.

Comment 14

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1210482.patch

Should I do a push to try?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8685158 [details] [diff] [review]
bug-1210482.patch

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

This looks good to me! Yes, let's push to try. Assuming a green try run, I think this is good to go. Great work Antonio!

Are you interested in helping us fix up some other tests to work properly with e10s?

Comment 16

[Antonio de Luna [:A-deLuna]]

Attachment: bug-1210482.patch

I'd be glad to help Mike, If you have any suggestions of which bug to tackle next please let me know.

I had an issue when running the test with e10s enabled.
whenever I was trying to access popupWindow's toolbars with ContentTask.spawn in the content context, the test timed out.

It got fixed by adding:
>let popupBrowser = popupWindow.gBrowser.selectedBrowser;
>yield BrowserTestUtils.browserLoaded(popupBrowser);

But I don't really understand why it got fixed... or why did it ran fine with e10s disabled.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Antonio de Luna [:A-deLuna] from comment #16)
> Created attachment 8685307 [details] [diff] [review]
> bug-1210482.patch
> 
> I'd be glad to help Mike, If you have any suggestions of which bug to tackle
> next please let me know.
> 
> I had an issue when running the test with e10s enabled.
> whenever I was trying to access popupWindow's toolbars with
> ContentTask.spawn in the content context, the test timed out.
> 
> It got fixed by adding:
> >let popupBrowser = popupWindow.gBrowser.selectedBrowser;
> >yield BrowserTestUtils.browserLoaded(popupBrowser);
> 
> But I don't really understand why it got fixed... or why did it ran fine
> with e10s disabled.

Yeah, there's some tricky stuff going on there.

When a popup window is opened in Firefox with e10s enabled, we do quite a bit of juggling. For one thing, the initial browser is made to be remote so that we can pass its "nsITabParent" back down to the child so that the remote opener (in this case, the example.com test_new_window_from_content_child.html page) can get a reference to it to manipulate.

The popup, however, is attempting to open about:robots, which is currently only supported in non-remote browsers. The remote browser notices this, and messages to the parent requesting a "remoteness flip", which will put the popup window's browser into the non-remote state.

Flipping from remote to non-remote or vice-versa results in a new message manager getting created. Any message listeners you set up before a remoteness switch will no longer receive messages. And that's what's happening here - your ContentTask sets up a message listener on the remote browser in the popup before it has figured out it needs to do a remoteness flip. Then it does the flip, and then your message listener never hears about any updates in the content because the message manager was "disconnected".

So that's why waiting for the window browser to load is the right choice - at that point, we know that the browser has finished loading about:robots, and that the remoteness switch has taken place.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aafa5f3b02d7

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8685307 [details] [diff] [review]
bug-1210482.patch

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

This looks good! I've pushed it to try for you.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Try looks good! Great job A-deLuna.

Comment 21

[Antonio de Luna [:A-deLuna]]

Thanks Mike!

You've been a great mentor and I appreciate your explanations, I've learned a lot doing this.

I was about to also do a try push but was unsure which platforms we needed to test with, is there any specific reason we're only testing with Linux?

I'll look forward to work on other bugs mentored by you.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

(In reply to Antonio de Luna [:A-deLuna] from comment #21)
> Thanks Mike!
> 
> You've been a great mentor and I appreciate your explanations, I've learned
> a lot doing this.
> 

Glad you had a good experience!

> I was about to also do a try push but was unsure which platforms we needed
> to test with, is there any specific reason we're only testing with Linux?
> 
> I'll look forward to work on other bugs mentored by you.

At least for me, it's experience based. I know that there isn't too much OS-specific stuff that goes into opening windows (at least above the widget level). I also know that your test will run on all platforms. I also know that our Windows and OS X test machines are usually backlogged because we can't provision them from Amazon. That means that we get results for Linux tests faster.

So that's why I went with Linux-only.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93e3a308d826

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/93e3a308d826