962065 - mozmill.newBrowserController is not supported in SeaMonkey

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

[Tooru Fujisawa [:arai]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140116125114

Steps to reproduce:

1. Run Mozmill test with SeaMonkey 2.23 on Mac OS X, with following command:
  mozmill -t someTest.js -b /Applications/SeaMonkey.app
2. call mozmill.newBrowserController() in someTest.js



Actual results:

Assertion fails in mozmill/extension/resource/driver/controller.js:247:
>  assert.waitFor(function () {
>    return window != null && self.isLoaded();
>  }, "controller(): Window has been initialized.");

because OpenBrowserWindow function does not return window in SeaMonkey
(defined in chrome/comm/content/communicator/tasksOverlay.js:106)

and undefined is passed as first argument for controller.MozMillController, in mozmill/extension/resource/driver/mozmill.js:155:
> function newBrowserController () {
>  return new controller.MozMillController(utils.getMethodInWindows('OpenBrowserWindow')());
> }



Expected results:

There is no simple way of knowing which window is opened by last OpenBrowserWindow function call,
so exception should be thrown (such like 'new Error("mozmill.newBrowserController is not supported.")') .

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: throw exception in newBrowserController on SeaMonkey

Comment 2

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

Comment on attachment 8362953 [details] [diff] [review]
throw exception in newBrowserController on SeaMonkey

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

(In reply to Tooru Fujisawa [:arai] from comment #0)
> There is no simple way of knowing which window is opened by last
> OpenBrowserWindow function call,

In that case I would suggest to open a bug for SeaMonkey so that this can be changed. It looks like a trivial fix.

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +156,5 @@
> +  if (Application == 'SeaMonkey') {
> +    throw new Error("mozmill.newBrowserController is not supported.");
> +  } else {
> +    return new controller.MozMillController(utils.getMethodInWindows('OpenBrowserWindow')());
> +  }

I think that we should change the code of this method instead. So it will be similar to getAddonsController(). Means we call OpenBrowserWindow first, and do the controller creation in a second line by querying for the most recent browser window. We might need an extra check so that we really wait for the new window and don't return the current one, just because it has been taken longer to open the new window.

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> In that case I would suggest to open a bug for SeaMonkey so that this can be
> changed. It looks like a trivial fix.

Ah, you are right. It's a better solution.

> I think that we should change the code of this method instead. So it will be
> similar to getAddonsController(). Means we call OpenBrowserWindow first, and
> do the controller creation in a second line by querying for the most recent
> browser window. We might need an extra check so that we really wait for the
> new window and don't return the current one, just because it has been taken
> longer to open the new window.

Sorry I didn't notice that there already exists similar code and it works correctly.
So we don't have to modify SeaMonkey itself, right?
I'll post new patch soon.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Implement newBrowserController on SeaMonkey

getMostRecentWindow returns old browser window when I call it just after calling OpenBrowserWindow.
So, to prevent increasing the amount of time for testing,  
use return value of OpenBrowserWindow on Firefox,
and use getMethodInWindows only on SeaMonkey.

Comment 5

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

(In reply to Tooru Fujisawa [:arai] from comment #3)
> So we don't have to modify SeaMonkey itself, right?

You should file a bug for SeaMonkey to get this fixed. The code here should only be a fallback method for applications which really do not return a valid window instance.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Implement fallback method of newBrowserController

(In reply to Henrik Skupin (:whimboo) from comment #5)
> You should file a bug for SeaMonkey to get this fixed. The code here should
> only be a fallback method for applications which really do not return a
> valid window instance.
Okay, I see,
I'll file a bug.

Then, if this is fallback method, the code should not check browser name, but check return value itself.
So I attach updated patch.

Comment 7

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

Comment on attachment 8365866 [details] [diff] [review]
Implement fallback method of newBrowserController

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

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +153,5 @@
>  
>  
>  function newBrowserController () {
> +  var oldBrowserWindow = wm.getMostRecentWindow('navigator:browser');
> +  var browserWindow = utils.getMethodInWindows('OpenBrowserWindow')();

Can we call those variables 'currentWindow' and 'newWindow' instead?

@@ +157,5 @@
> +  var browserWindow = utils.getMethodInWindows('OpenBrowserWindow')();
> +  if (!browserWindow) {
> +    for (;;) {
> +      browserWindow = wm.getMostRecentWindow('navigator:browser');
> +      if (browserWindow && browserWindow != oldBrowserWindow) {

I would prefer if we actually could  compare the window ids here.

@@ +160,5 @@
> +      browserWindow = wm.getMostRecentWindow('navigator:browser');
> +      if (browserWindow && browserWindow != oldBrowserWindow) {
> +        break;
> +      }
> +      utils.sleep(100);

Please make use of assert.waitFor() which you will get by importing 'resource://mozmill/modules/assertions.js'. See the top of utils.js how to do this. Then we can also define a timeout so we do not run this loop forever.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: addressing review comments

Thank you for reviewing, and here is fixed patch.

Comment 9

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

Comment on attachment 8370798 [details] [diff] [review]
addressing review comments

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

Given that in latest SeaMonkey builds the OpenBrowserWindow() method returns the new window now, I think this request is not that important anymore for the future. But I still see a benefit for it when it comes to other applications which also have this method available. So I think that we should still get this feature in. Also it would enable us to run tests against older builds of SeaMonkey (e.g. the current release).

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +154,5 @@
>  
>  function newBrowserController () {
> +  var currentWindow = wm.getMostRecentWindow('navigator:browser');
> +  var newWindow = utils.getMethodInWindows('OpenBrowserWindow')();
> +  if (!newWindow) {

nit: please add a blank line here to separate declarations from the following code.

@@ +155,5 @@
>  function newBrowserController () {
> +  var currentWindow = wm.getMostRecentWindow('navigator:browser');
> +  var newWindow = utils.getMethodInWindows('OpenBrowserWindow')();
> +  if (!newWindow) {
> +    var currentWindowID;

I would initialize with null.

@@ +160,5 @@
> +    if (currentWindow) {
> +      currentWindowID = currentWindow
> +                        .QueryInterface(Ci.nsIInterfaceRequestor)
> +                        .getInterface(Ci.nsIDOMWindowUtils)
> +                        .outerWindowID;

You should use utils.getWindowID() here. See:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/stdlib/utils.js#L123

Maybe we should add the validity check (if condition) to getWindowID() and return null there?

@@ +169,5 @@
> +        return false;
> +      }
> +      if (!currentWindow) {
> +        return true;
> +      }

I don't think that this check is necessary. If you compare the ids below and currentWindowID is null, but newWindowID is valid, you will also return true.

@@ +174,5 @@
> +      var newWindowID = newWindow
> +                        .QueryInterface(Ci.nsIInterfaceRequestor)
> +                        .getInterface(Ci.nsIDOMWindowUtils)
> +                        .outerWindowID;
> +      return newWindowID != currentWindowID;

Please use a triple operator '!==' here.

@@ +177,5 @@
> +                        .outerWindowID;
> +      return newWindowID != currentWindowID;
> +    }, "newBrowserController(): Window has been opened.");
> +  }
> +  return new controller.MozMillController(newWindow);

nit: a blank line before the return statement please.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: addressing review comments

Thank you again for reviewing!

I've modified getWindowId to return null if aWindow is null, as you wrote.

Comment 11

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

Comment on attachment 8376181 [details] [diff] [review]
addressing review comments

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

Looks way better! While glancing over the code we can still make it simpler. Please see my inline comments. Have you also run all the mutt tests to ensure nothing is broken? Also against SeaMonkey?

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +153,4 @@
>  
>  
>  function newBrowserController () {
> +  var currentWindow = wm.getMostRecentWindow('navigator:browser');

currentWindow is only used in the if condition. We should better move it in there.

@@ +162,5 @@
> +    assert.waitFor(function () {
> +      newWindow = wm.getMostRecentWindow('navigator:browser');
> +      if (!newWindow) {
> +        return false;
> +      }

I don't think we really need this if condition:

* if no windows are open the check below will wait until a new window is opened

or 

* if the new window is not open yet, its id will be the same as for the current window

@@ +165,5 @@
> +        return false;
> +      }
> +
> +      return utils.getWindowId(newWindow) !== currentWindowID;
> +    }, "newBrowserController(): Window has been opened.");

nit: "New window has ..."

Comment 12

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

(In reply to Henrik Skupin (:whimboo) [away 02/24 - 02/28] from comment #11)
> Looks way better! While glancing over the code we can still make it simpler.
> Please see my inline comments. Have you also run all the mutt tests to
> ensure nothing is broken? Also against SeaMonkey?

Sorry, I forgot about testing.

It seems that "-a" seamonkey" option is not supported by mozrunner,
so i used "-a firefox" option instead.

Then, Some tests failed because of MozMillController.prototype.open does not support SeaMonkey.
However, it seems that Firefox's case also works correctly with SeaMonkey.
>     case "Firefox":
>     case "MetroFirefox":
>+    case "SeaMonkey":
>       // Stop a running page load to not overlap requests
>       if (this.browserObject.selectedBrowser) {
>         this.browserObject.selectedBrowser.stop();
>       }
> 
>       this.browserObject.loadURI(url);
>       break;
I'll prepare a patch for it.

For other failures, it seems to be caused by the UI difference between Firefox and SeaMonkey.
I'll post log file later.

Anyway, no regression is found for following applications (tested on Linux 64bit):
  * Firefox ESR(24.3.0), Stable(27.0.1), Beta(28.0), Aurora(29.0a2 2014-02-20),
      Nightly(30.0a1 2014-02-20)
  * SeaMonkey Stable(2.24), Beta(2.25), Nightly(2.27a1 2014-02-14)

> ::: mozmill/mozmill/extension/resource/driver/mozmill.js
> @@ +153,4 @@
> >  
> >  
> >  function newBrowserController () {
> > +  var currentWindow = wm.getMostRecentWindow('navigator:browser');
> 
> currentWindow is only used in the if condition. We should better move it in
> there.

Can we assume that getMostRecentWindow always returns the "currentWindow" value
instead of "newWindow" even after calling OpenBrowserWindow?

I already confirmed that following applications return "currentWindow" value (tested on Linux 64bit):
  * Firefox ESR(24.3.0), Stable(27.0.1), Beta(28.0), Aurora(29.0a2 2014-02-20),
      Nightly(30.0a1 2014-02-20)
  * SeaMonkey Stable(2.24), Beta(2.25), Nightly(2.27a1 2014-02-14)
Of course, Firefox and SeaMonkey 2.27 returns "newWindow" directly from OpenBrowserWindow,
so we don't need "currentWindow" value.

But I'm not sure about others.
Is there any application (or OS) I should test?

> @@ +162,5 @@
> > +    assert.waitFor(function () {
> > +      newWindow = wm.getMostRecentWindow('navigator:browser');
> > +      if (!newWindow) {
> > +        return false;
> > +      }
> 
> I don't think we really need this if condition:

In following (rare) case, newBrowserController may return null, instead of throwing assertion failure.

1. getMostRecentWindow returns non-null for currentWindow
2. OpenBrowserWindow returns null for newWindow
3. new browser window is opened but all browser windows are closed before next waitFor callback
4. getMostRecentWindow returns null for newWindow

Is it ignorable?

> @@ +165,5 @@
> > +        return false;
> > +      }
> > +
> > +      return utils.getWindowId(newWindow) !== currentWindowID;
> > +    }, "newBrowserController(): Window has been opened.");
> 
> nit: "New window has ..."

Thank you!

Comment 13

[Tooru Fujisawa [:arai]]

Here is the mutt test log file for SeaMonkey.
Applied attachment 8376181 [details] [diff] [review] and "MozMillController.prototype.open" patch in comment #12.

> Exception: "controller.waitForPageLoad(URI=http://localhost:43336/link.html, readyState=complete)" (/home/arai/projects/mozmill/mutt/mutt/tests/js/testController/testDndContentChrome.js)
> js/testController/testDndContentChrome.js | test

SeaMonkey does not load URL by drag and drop link to urlbar,
it inserts link URL into dropped position in urlbar text instead.

> Exception: "elem is null" (/home/arai/projects/mozmill/mutt/mutt/tests/js/testController/testScreenshot.js)
> js/testController/testScreenshot.js | testChrome

"xul:tabs" element in SeaMonkey window does not have "tabbrowser-tabs" as its ID,
but its className.

> Failure: "keypress added letter to location bar. - 'about:blankt' should equal 't'" (/home/arai/projects/mozmill/mutt/mutt/tests/js/testElementsLib/testMozElementWindow.js)
> js/testElementsLib/testMozElementWindow.js | testChrome

When "about:blank" is opened, urlbar is not empty, but contains "about:blank".

> Failure: "Element 'getMeOutButton' has been found. - got 'false'" (/home/arai/projects/mozmill/mutt/mutt/tests/js/testUtils/testPageLoad.js)
> js/testUtils/testPageLoad.js | testWaitForPageLoad

There is "getMeOutOfHereButton" button instead of "getMeOutButton" button
in blacklist page.

> Exception: "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIWebNavigation.loadURI]" (/home/arai/projects/mozmill/mutt/mutt/tests/js/testUtils/testReloadPage.js)
> js/testUtils/testReloadPage.js | testReloadPageAfterTabChange

"about:newtab" URL does not exist.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: mutt test log for seamonkey

Sorry, I forgot to attach the log in comment #13.

Comment 15

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

Looks like I missed the latest comments on this bug. Sorry, Tooru. How important is this feature for you? I ask because we are going to deprecate Mozmill and port all of our tests to Marionette. See bug 1080766 for the current progress.

Comment 16

[Tooru Fujisawa [:arai]]

Thank you for letting me know about mozmill-marionette-e10s :)

Since bug 964220 is already fixed on SeaMonkey's side, this bug is not so important, it's just for compatibility with SeaMonkey 2.26 or older, and current SeaMonkey stable is 2.32, so it's okay to close this bug as WONTFIX.

Comment 17

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

Thank you Tooru for the quick response! Lets close it then!