1047235 - Add browser.js UI library with the BrowserWindow class

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Daniel Gherasim]

We need to add a new library that basically covers the functionality of a basic browser window.

This class will be inherited from the BaseWindow implemented in bug 1006996 & will contain some basic operations we need at the moment:
- isPrivate() // will file a new bug to replace private-browsing.js with this.
- openPreferences() // for bug 1005811
- openPlacesOrganizerWindow() // for bug 996530
- downloadFileOfUnknownType() // for bug 908649

The file will also contain a method for opening & returning a new instance of the BrowserWindow: open() (type: default/private window through all methods).

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

Oh, openPreferences & openPlacesOrganizer will have to return instances of the classes so I would add them once with the class.

Here is the patch for the BrowserWindow class, inherited from BaseWindow made in bug 1006996. Needs the patch from there applied first.

Comment 2

[Andrei Eftimie]

Comment on attachment 8467718 [details] [diff] [review]
v1.patch

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

You missed to add the `windows.js` lib file.

::: firefox/lib/tests/testBrowserWindow.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please use strict mode.
> "use strict";

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + Cu.import("resource://gre/modules/Downloads.jsm");

Missing "use strict";.
nit: extra whitespace

@@ +17,5 @@
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +}

If there's nothing to clean up we can omit this function completely.

@@ +23,5 @@
> +function testBrowserWindow() {
> +  browserWindow.downloadFileOfUnknownType(TEST_DATA);
> +
> +  // Check that the file was downloaded
> +  var downloadsList = null;

Since List implies a plural, lets name this "downloadList"

@@ +36,5 @@
> +
> +  expect.equal(downloadsList.length, 1, "One download file is present");
> +
> +  // Shouldn't take more then 2 seconds to download the local file
> +  expect.waitFor(() => downloadsList[0].succeeded, 2000 ,"Download finished");

nit: spacings

Also the default timeout value is 5s, not sure if limiting this to 2s for this particular case improves anything.

::: firefox/lib/ui/browser.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I can't review this file in lieu of the missing windows lib since it inherits the base prototype object from there.

::: firefox/lib/ui/save-file-dialog.js
@@ +62,5 @@
> +   * @parma {string} aSpec.type
> +   *        Identifier of the elements
> +   * @param {string} [aSpec.subtype]
> +   *        Attribute of the elements to filter
> +   * @param {string} [aSpec.value]

We don't make use of either spec.subtype or spec.value

@@ +85,5 @@
> +        nodeCollector.queryNodes("[dlgtype=accept]");
> +        elems = nodeCollector.elements;
> +        break;
> +      case "save":
> +        elems = [new findElement.ID(root, "save")];

findElement doesn't need `new`

Comment 3

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #2)
> You missed to add the `windows.js` lib file.

We need patch for 1006996 which implements windows.js lib file.

Comment 4

[Daniel Gherasim]

Attachment: v1.1.patch

Thanks! 
Changed all you've asked to.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8468425 [details] [diff] [review]
v1.1.patch

Given that we are blocked here, removing the review request for now.

Comment 6

[Daniel Gherasim]

Attachment: v2.patch

Updated according the latest updates in the dependency bug.

Comment 7

[Daniel Gherasim]

Attachment: v3.patch

The window library is up now & we need this for the blocked P1 bugs here.

Comment 8

[Andrei Eftimie]

Comment on attachment 8489946 [details] [diff] [review]
v3.patch

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

I'm guessing we'll update the rest of the code to open windows with the new 'class' in a separate issue. (And to closeAllWindows
We should file a bug for that if we don't already have one.

I would give this a tenatative r+, but I'd like a short discussion on Object.create() vs new in the below mentioned place.
I'd also like Henrik's opinion here, flagging for feedback.

::: firefox/lib/ui/browser.js
@@ +27,5 @@
> +
> +  baseWindow.BaseWindow.call(this, this._controller);
> +}
> +
> +BrowserWindow.prototype = Object.create(baseWindow.BaseWindow.prototype, {

I haven't had the pleasure to use Object.create in this way yet.
So because of this we have to call the constructor manually above...

Do we have any clear advantages over instantiating BaseWindow with new and passing our controller, then just extending the prototype as we usually do without the need for descriptors?
(I don't have any clear preference here)

@@ +33,5 @@
> +    /**
> +     * Gets all the needed external DTD urls as an array
> +     *
> +     * @returns Array of external DTD urls
> +     * @type [string]

nit: I think:
@returns {[string]} description

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8489946 [details] [diff] [review]
v3.patch

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

::: firefox/lib/ui/browser.js
@@ +27,5 @@
> +
> +  baseWindow.BaseWindow.call(this, this._controller);
> +}
> +
> +BrowserWindow.prototype = Object.create(baseWindow.BaseWindow.prototype, {

I'm not fond in changing the way of how we handle sub-classing. Please stick with the prototype method as Andrei said. We could evaluate Object.create() later, but this is out of scope at the moment.

Comment 10

[Daniel Gherasim]

Attachment: inheritance-example.js

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8489946 [details] [diff] [review]
> v3.patch
> 
> Review of attachment 8489946 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/ui/browser.js
> @@ +27,5 @@
> > +
> > +  baseWindow.BaseWindow.call(this, this._controller);
> > +}
> > +
> > +BrowserWindow.prototype = Object.create(baseWindow.BaseWindow.prototype, {
> 
> I'm not fond in changing the way of how we handle sub-classing. Please stick
> with the prototype method as Andrei said. We could evaluate Object.create()
> later, but this is out of scope at the moment.

I don't thing we do have inherited classes in our code so far (my bad if we have and I missed it), but I would start using them, and Object.create offers the best implementation and flexibility in this scope (we can declare proprieties which can be writable, configurable or enumerable):
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

See the attachment for a different implementation (using new).

Otherwise, given that there are numerous ways of implementing this, please give specific details of how you want it.

Comment 11

[Daniel Gherasim]

Comment on attachment 8490618 [details]
inheritance-example.js

Actually a needinfo is needed.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so I re-checked our test repository, and you may be right in terms of inheritance. As it looks like we have inheritance in the assertion module only. And this is going to be removed. In Mozmill itself the errors module is using Object.create().

Anyway, I also would like to see a good way of inheritance but this will need a bit of investigation and discussion first. There are so many variants as you have already said. So given that this blocks our current work, please use the known and working solution for inheritance via the old-style prototype.

We could work on an Object.create() replacement on another bug.

Comment 13

[Daniel Gherasim]

Attachment: v4.patch

Updated as requested.

Comment 14

[Daniel Gherasim]

Attachment: v4.1.patch

Removed the assert/expect include line from the library.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8490711 [details] [diff] [review]
v4.1.patch

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

See my comments inline. Also next time please ask Andrei or Andreea for review first, as what we usually do.

::: firefox/lib/tests/testBrowserWindow.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

nit: extra space at the beginning.

@@ +9,5 @@
> +var windows = require("../../../lib/windows");
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserWindow = new browser.BrowserWindow(aModule.controller);

No need to define the controller here. It's an optional argument for BrowserWindow().

@@ +13,5 @@
> +  aModule.browserWindow = new browser.BrowserWindow(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +  windows.closeAllWindows(aModule.controller);

I would propose we write the tests to always use the new ui classes. So use the browser window instance as parameter here.

@@ +19,5 @@
> +
> +function testBrowserWindow() {
> +  // Open default windows
> +  var defaultWindow1 = browserWindow.open({type: "default", method: "shortcut"});
> +  var defaultWindow2 = browserWindow.open({type: "default", method: "menu"});

How does that work on OS X if no other browser window is open? Maybe another little test for that might be good to have.

Also I would remove one type definition to check that we correctly use the default.

@@ +23,5 @@
> +  var defaultWindow2 = browserWindow.open({type: "default", method: "menu"});
> +
> +  // Check that they are not private
> +  expect.ok(!defaultWindow1.isPrivate(), "Window is not private");
> +  expect.ok(!defaultWindow2.isPrivate(), "Window is not private");

nit: "Browser window..."

@@ +35,5 @@
> +  var privateWindow2 = browserWindow.open({type: "private", method: "menu"});
> +
> +  // Check they are private
> +  expect.ok(privateWindow1.isPrivate(), "Window is private");
> +  expect.ok(privateWindow2.isPrivate(), "Window is private");

Same as above.

::: firefox/lib/ui/browser.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// Import PrivateBrowsingUtils to gain acces to its API

I don't see what this comment gives us for additional information. Just remove it.

@@ +12,5 @@
> +var utils = require("../../../lib/utils");
> +var windows = require("../../../lib/windows");
> +
> +/**
> + * Browser UI class

"Browser Window class"

@@ +15,5 @@
> +/**
> + * Browser UI class
> + *
> + * @constructor
> + * @param {MozMillController} [aController]

We have a default value, which is used. Please mark this in the jsdoc section.

@@ +22,5 @@
> +function BrowserWindow(aController) {
> +  this._controller = aController || mozmill.getBrowserController();
> +}
> +
> +BrowserWindow.prototype = new baseWindow.BaseWindow(true);

Bad workaround we need here to make the prototype chain working. So lets investigate the Object.create() option as soon as possible. Maybe also have a look at the inheritance module we used in the mozmill-tests-refactor project: https://github.com/whimboo/mozmill-test-refactor. Please lets follow-up on this in a new bug.

@@ +28,5 @@
> +
> +/**
> + * Gets all the needed external DTD urls as an array
> + *
> + * @returns {[string]} Array of external DTD urls

nit: string[]

@@ +34,5 @@
> +BrowserWindow.prototype.getDtds = function BrowserWindow_getDtds() {
> +  return ["chrome://branding/locale/brand.dtd",
> +          "chrome://browser/locale/browser.dtd",
> +          "chrome://browser/locale/aboutPrivateBrowsing.dtd"];
> +};

Lets get the dtd's defined as a variable on the class, and the getDtds() method just return it. That way you can implement this method in the BaseWindow itself only.

@@ +44,5 @@
> + */
> +BrowserWindow.prototype.isPrivate = function BrowserWindow_isPrivate() {
> +  return PrivateBrowsingUtils.isWindowPrivate(this._controller.window);
> +
> +};

nit: extra empty line

@@ +54,5 @@
> + *        Information about opening the new window
> + * @param {string} [aSpec.method=menu]
> + *        Method to use
> + * @param {string} [aSpec.type="default"]
> + *        Type of the new Browser Window to open

I would call this private, and let it default to false. Not sure if we will ever have other modes. Lets make it simple.

@@ +109,5 @@
> +    // We need to wait for the about:privatebrowsing page to load,
> +    // otherwise it might load _after_ we try loading another page
> +    newWindow.controller.waitForPageLoad();
> +
> +    assert.ok(newWindow.isPrivate(), "A new private window has been opened");

I would do this check first, so we know that we really have a private browsing window.

Comment 16

[Daniel Gherasim]

Attachment: v5.patch

Comment 17

[Andrei Eftimie]

Comment on attachment 8491284 [details] [diff] [review]
v5.patch

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

With this 2 nits fixed, this gets an r+ from me.
Carry the review flag back to Henrik please.

::: firefox/lib/ui/browser.js
@@ +37,5 @@
> +  return this._controller.isLoaded(this._controller.window);
> +};
> +
> +/**
> + * Check it the current window is a private type of window

"[..] is private"
should suffice

::: lib/ui/base-window.js
@@ +46,5 @@
> +  },
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {string[]]} Array of external DTD urls

nit: there's an extra ]

Comment 18

[Daniel Gherasim]

Attachment: v5.1.patch

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not blocked anymore on bug 1006996, given that the patch already landed on default and is sticky.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8491422 [details] [diff] [review]
v5.1.patch

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

::: firefox/lib/tests/testBrowserWindow.js
@@ +8,5 @@
> +var browser = require("../ui/browser");
> +var windows = require("../../../lib/windows");
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();

As already said in my last review, there is no need to define the controller. It's not used anywhere in this test. Also check the other updated tests for the necessity of the bare controller.

::: firefox/lib/ui/browser.js
@@ +34,5 @@
> + * @returns {boolean} True if open, false otherwise
> + */
> +BrowserWindow.prototype.isOpen = function BrowserWindow_isOpen() {
> +  return this._controller.isLoaded(this._controller.window);
> +};

Why not a property? Maybe we should redefine and use .open and .closed for ui elements in the future? It's closer to the DOMWindow with the .closed property. Also I think we should determine the status based on DOMWindow.closed and not on the loaded status. That would mean as long as a window has not been finished loaded it is closed for us, which is not true and cause race conditions.

@@ +43,5 @@
> + * @returns {boolean} True is private, false otherwise
> + */
> +BrowserWindow.prototype.isPrivate = function BrowserWindow_isPrivate() {
> +  return PrivateBrowsingUtils.isWindowPrivate(this._controller.window);
> +};

Same here regarding a property.

@@ +51,5 @@
> + *
> + * @param {string} aSpec
> + *        Information about opening the new window
> + * @param {string} [aSpec.method=menu]
> + *        Method to use

Maybe you tell what other options exist?

@@ +68,5 @@
> +  var shiftKey = false;
> +
> +  if (spec.private) {
> +    menuItem = "#menu_newPrivateWindow";
> +    cmdKey = utils.getEntity(this._dtds, "privateBrowsingCmd.commandkey");

Please use the `getDTDs()` method here, or convert this method to a property named `dtds`, which might be even better.

::: firefox/tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js
@@ +5,5 @@
>  "use strict";
>  
>  // Include the required modules
>  var { assert, expect } = require("../../../../lib/assertions");
> +var browser = require("../../../lib/ui/browser");

I would suggest that we separate the ui libraries from the backend modules. So putting them in an extra block afterward.

::: lib/tests/testBaseWindow.js
@@ +16,4 @@
>  }
>  
>  function teardownModule(aModule) {
> +  windows.closeAllWindows(aModule.browserWindow);

BaseWindow should not use the BrowserWindow class. You could have left it as it was before. I haven't complained about it. By using the controller we can ensure that those methods also work with it.

@@ +21,5 @@
>  
>  function testBaseWindow() {
>    var length = mozmill.utils.getWindows().length;
>  
> +  var win1 = browserWindow.open();

This does not test the BaseWindow.

::: lib/ui/base-window.js
@@ +44,5 @@
>      windows.waitForWindowState(callback, {state: windows.WINDOW_STATES.closed,
>                                            window: this._controller.window});
> +  },
> +  /**
> +   * Gets all the needed external DTD urls as an array

I would say: "Get list of required DTD locations".

@@ +48,5 @@
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {string[]} Array of external DTD urls
> +   */
> +  getDtds: function Window_getDtds() {

The class prefix should be 'BaseWindow' and not 'Window'. Same for the other methods here.

Comment 21

[Daniel Gherasim]

Attachment: v6.patch

Updated base on review and discussion I had with Henrik on IRC.

Comment 22

[Daniel Gherasim]

If I remove the check for 
>  27   url: "http://ssl-ov.mozqa.com/",
>  28   identity: "",
>  29   type: "unknownIdentity",
>  30   callback: checkSecurityTab_OV
>  31 }

Test passes, I will continue the investigation on Monday.

Comment 23

[Daniel Gherasim]

Wrong bug. Sorry!

Comment 24

[Andrei Eftimie]

Comment on attachment 8492118 [details] [diff] [review]
v6.patch

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

With the description added r+ from me.

::: lib/ui/base-window.js
@@ +29,5 @@
>     */
>    get controller() {
>      return this._controller;
>    },
>  

These 2 methods are pretty self-explanatory. Still we should add a short description.

Comment 25

[Daniel Gherasim]

Attachment: v6.1.patch

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8492883 [details] [diff] [review]
v6.1.patch

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

We are close. Lets quickly re-iterate over the remaining listed items, so we have a clean implementation. I would like to re-review it once done.

::: firefox/lib/tests/testBrowserWindow.js
@@ +16,5 @@
> +  windows.closeAllWindows(aModule.browserWindow);
> +}
> +
> +function testBrowserWindow() {
> +  assert.ok(browserWindow.open, "Default browser window is open");

Sorry to touch this again. But I'm not sure if that was a good idea, that I proposed the usage of 'open'. It's a verb, which could be confusing by any action to open a new window. Shall we better only make use of 'closed' as how Firefox handled it for the different window classes?

@@ +21,5 @@
> +
> +  // Open default windows
> +  var defaultWindow1 = browserWindow.openNewWindow({private: false,
> +                                                    method: "shortcut"});
> +  var defaultWindow2 = browserWindow.openNewWindow({method: "menu"});

Beside the above comment I think we should keep that. It's clear enough.

@@ +29,5 @@
> +  expect.ok(!defaultWindow2.private, "Browser window is not private");
> +
> +  // Close the recently opened windows
> +  defaultWindow1.close();
> +  defaultWindow2.close();

I would add a check for `.closed` so we can ensure that we handle it correctly.

::: firefox/lib/ui/browser.js
@@ +37,5 @@
> + *
> + * @param {string} aSpec
> + *        Information about opening the new window
> + * @param {string} [aSpec.method=menu]
> + *        Method to use (menu, shortcut or callback)

nit: Please put quotes around each listed method.

::: lib/tests/testBaseWindow.js
@@ +64,2 @@
>    return new baseWindow.BaseWindow(newController);
>  }

I think that nothing in here would prevent us know from getting this method added to the BaseWindow class, or?

Comment 27

[Daniel Gherasim]

Attachment: v6.2.patch

Thanks! So I updated, tested & now re-asking for a final review.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8493038 [details] [diff] [review]
v6.2.patch

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

Except the one comment as mentioned inline I don't see anything which should keep us blocking from getting this patch landed. Andreea, please just have a sanity check over it. Thanks.

::: firefox/lib/tests/testBrowserWindow.js
@@ +21,5 @@
> +
> +  // Open default windows
> +  var defaultWindow1 = browserWindow.open({private: false,
> +                                           method: "shortcut"});
> +  var defaultWindow2 = browserWindow.open({method: "menu"});

As mentioned in my last review it would have been good to leave as is with openNewWindow(), but since you reverted also this lets leave it now.

Comment 29

[Daniel Gherasim]

This will be landed on nightly and aurora only. (As we did with the dependency bug too).

Comment 30

[Daniel Gherasim]

Ran endurance/functional/remote on Mac/Linux/Windows with Nightly and I got no failure.
Also ran the lib tests with nightly multiple times on across all platforms, I think we're safe to land this.

Comment 31

[Andrei Eftimie]

Comment on attachment 8493038 [details] [diff] [review]
v6.2.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/4fbf72be5c32 (default)

Comment 32

[Andrei Eftimie]

All works fine on Aurora (ran functional, remote and endurance).

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/171ca54a863d (mozilla-aurora)