1071548 - BrowserWindow class misses close() method

Status: RESOLVED INVALID

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

Description

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

The BrowserWindow class as implemented in bug 1047235 is missing its own close() method. It should overwrite the BaseWindow close() method by allowing to specify the menu, shortcut etc. Itself it can then call the BaseWindow method with its own callback.

Daniel, would you mind getting this implemented. It will help us in tests where we have to handle multiple windows.

Comment 1

[Daniel Gherasim]

Attachment: fix_v1.patch

Up for review! :)

Comment 2

[Andrei Eftimie]

Comment on attachment 8494434 [details] [diff] [review]
fix_v1.patch

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

r=me with the nit fixed.

::: firefox/lib/ui/browser.js
@@ +58,5 @@
> +    case "menu":
> +      callback = () => { this._controller.mainMenu.click("#menu_closeWindow"); }
> +      break;
> +    case "shortcut":
> +       var closeWindowKey = utils.getEntity(this.dtds, "closeWindow.key");

nit: indentation is a bit off

Comment 3

[Daniel Gherasim]

Attachment: fix_v2.patch

Done!

Comment 4

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

Comment on attachment 8497416 [details] [diff] [review]
fix_v2.patch

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

::: firefox/lib/ui/browser.js
@@ +22,5 @@
>    this._controller = aController || mozmill.getBrowserController();
>    this._dtds = ["chrome://branding/locale/brand.dtd",
>                  "chrome://browser/locale/browser.dtd",
> +                "chrome://browser/locale/aboutPrivateBrowsing.dtd",
> +                "chrome://browser/locale/pageInfo.dtd"];

Mind helping me why we need this extra DTD file?

@@ +27,4 @@
>  }
>  
>  BrowserWindow.prototype = new baseWindow.BaseWindow(true);
>  BrowserWindow.prototype.constructor = BrowserWindow;

Do you mind changing this to Object.create() similar to the latest software update changes? Anything else about inheritance we will discuss on the other bug.

@@ +44,5 @@
> + *        Define new function that closes the window
> + * @param {boolean} [aSpec.force=false]
> + *        Force closing the window
> + */
> +BrowserWindow.prototype.close = function BrowserWindow_close(aSpec={}) {

nit: To reduce the line length we should use the prefix 'BW_' for the internal name.

@@ +50,5 @@
> +  var callback = null;
> +
> +  if (aSpec.force) {
> +    baseWindow.BaseWindow.prototype.close.call(this);
> +    return;

Why are you not waiting for the observer notifications here?

@@ +62,5 @@
> +      var closeWindowKey = utils.getEntity(this.dtds, "closeWindow.key");
> +      callback = () => {
> +        this._controller.keypress(null, closeWindowKey, {accelKey: true,
> +                                                         shiftKey: true});
> +      }

I like the definition of the individual callback methods. Can we do the same for the open() method too?

Comment 5

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

Mozmill tests have been superseded by Marionette tests.