666245 - Update shared modules for correct handling of the document loaded state introduced by bug 661408

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

As given on bug 661408 we have to remove the documentLoaded property from the DOMWindow due to namespace conflicts. Therefore our shared modules have to be updated to handle both cases for now. Later we can remove the support for documentLoaded inside the modal dialog class.

Comment 1

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

Attachment: Patch v1

I was able to get rid of the documentloaded state check for the handleWindow case but so far I haven't found a possibility for the modalDialog observer class. With this patch we support both ways (pre/post) the patch on bug 661408.

Once it has been reviewed I also have to update the file for the API refactored module.

Comment 2

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

Attachment: Patch v1.1 [checked-in]

Missed to revert to the try/catch clause for the check if the window has been loaded. Here the real version.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 541052 [details] [diff] [review]
Patch v1.1 [checked-in]

>+    try {
>+      controller = new mozmill.controller.MozMillController(window);
>+    ...
>+    }
>+    catch (ex) {
>+    }

I'm really not a fan of catch-all exception handlers that don't reraise, though I'd let it go by for a workaround.

Otherwise looks fine. r+

Comment 4

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/e54ecdbf732a (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/1269a42b420d (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/30fca7f88046 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/a8dcaae46b01 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/a4e440fbc5b0 (2.0)
http://hg.mozilla.org/qa/mozmill-tests/rev/48e29d8b0837 (1.9.2)

I will attach the patch for the API refactored tests/libs tomorrow on this bug.

Comment 5

[u279076]

This has caused a regression with all the Discovery Pane tests. Specifically, it appears that the test is timing out in modalDialog.observe() waiting for window.isLoaded even though the dialog is appearing.

Comment 6

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

The issue here is not my patch but Mozmill itself. With my code change we also pass a null value as window into its constructor. That causes Mozmill to stop execution and the whole thread stalls. I will attach a workaround patch for now in a couple of minutes. The real issue is now handled on bug 668202.

Comment 7

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

Attachment: Follow-up to circumvent bug 668202 [checked-in]

Comment 8

[Aaron Train [:aaronmt]]

Comment on attachment 542802 [details] [diff] [review]
Follow-up to circumvent bug 668202 [checked-in]

Check for non null window looks fine to me, r+

Comment 9

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

Comment on attachment 542802 [details] [diff] [review]
Follow-up to circumvent bug 668202 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/6d838befdc7b
http://hg.mozilla.org/qa/mozmill-tests/rev/ecd7c5500c0d
http://hg.mozilla.org/qa/mozmill-tests/rev/dca3e274ff8a
http://hg.mozilla.org/qa/mozmill-tests/rev/ef4ca3255c5d
http://hg.mozilla.org/qa/mozmill-tests/rev/88f4a0b168f3
http://hg.mozilla.org/qa/mozmill-tests/rev/52120216e324
http://hg.mozilla.org/qa/mozmill-tests/rev/054809e243b2

Comment 10

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15285157

Comment 11

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

For Mozmill 2 and our API refactoring the isLoaded() method should be available on the window wrapper. I will come up with a patch on Monday.

Comment 12

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

Attachment: Patch v1 (API refactor)

Patch against the API refactoring repository. I have had to always use static calls to avoid extra instantiation of a window wrapper class, and there is no way to call a method bound to the prototype from the constructor of the same class.

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 546750 [details] [diff] [review]
Patch v1 (API refactor)

A couple of problems.

First,

Static methods in Javascript are typically added to the function itself, not to the prototype. The idea is that it's not something owned by an instance at all, it's owned by the "class" (that is, the constructor function, in Javascript).

http://stackoverflow.com/questions/1635116/javascript-class-method-vs-class-prototype-method

function SomeClass() { ... }

SomeClass.staticMethod = function () { ... }

foo = SomeClass.staticMethod()

Second,

You can't have a static method that refers to "this" where "this" is an object instance (in your case, a window wrapper). You don't have an object instance. In a static method, "this" would refer to the constructor function (in a more classical OOP language, it'd be the "class" entity).

If someone were to ever call that without defining win, it'd fail spectacularly due to the default handling code.

A couple of options:

You can move this up to be a true static method and take out any default handling so there are no references to "this". But we don't have many static methods.

My preference would probably be for you to make this a (probably) non-exported utility function within the module, then expose it as an object method:

function winIsLoaded(aDOMWindow) {
  return ("mozmillDocumentLoaded" in aDOMWindow) && aDOMWindow.mozmillDocumentLoaded;
}

ChromeWindowWrapper.prototype.isLoaded() {
  return winIsLoaded(this.innerWindow);
}

If you want something available in our API to use against arbitrary DOMWindows, expose the utility method publicly (or make it a static method, I guess). It's a little weird to have the wrapper instance take other windows and tell you if they're loaded; it should only deal with itself.

BTW, did you mean for a window not to be considered loaded if we're not on Mozmill tip? Wasn't there supposed to be an either-or to the logic there, rather than just looking for the new property?

A smaller issue, btw: we're inconsistent as to what aWindow means in that file in particular. Sometimes it means a DOMWindow, and sometimes it means a window wrapper. I'd like us to find consistent naming we like and use it everywhere. 

Let's pick names and then I can make the changes globally. I'd prefer aDOMWindow vs. either aWrapper or aWindow.

Comment 14

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

(In reply to comment #13)
> Static methods in Javascript are typically added to the function itself, not
> to the prototype. The idea is that it's not something owned by an instance
> at all, it's owned by the "class" (that is, the constructor function, in
> Javascript).

In that case we will not be able to have a static method because with the upcoming WindowWrapper base class there would be no way to call the method from the sub classes. I see it now.

> My preference would probably be for you to make this a (probably)
> non-exported utility function within the module, then expose it as an object
> method:

Right. That's the best way to go. Will come up with a new patch shortly.

> BTW, did you mean for a window not to be considered loaded if we're not on
> Mozmill tip? Wasn't there supposed to be an either-or to the logic there,
> rather than just looking for the new property?

No-one except us is using the API refactored code. There is no need for the fallback code. We will have to always use at least Mozmill 1.5.4b6.

> A smaller issue, btw: we're inconsistent as to what aWindow means in that
> file in particular. Sometimes it means a DOMWindow, and sometimes it means a
> window wrapper. I'd like us to find consistent naming we like and use it
> everywhere. 

Lets do it in the patch when I create the new base class. I can then take care of it.

Comment 15

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

Attachment: Patch v2 (API refactor)

Updated patch. I have exported isWindowLoaded because it could still happen that a test would need direct access to it.

Comment 16

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

Attachment: Patch v2.1 (API refactor)

Comment 17

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 547049 [details] [diff] [review]
Patch v2.1 (API refactor)

Looks great, r+. Thanks for the changes and the explanation on the fallback. I agree.

Comment 18

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

Landed on the API refactoring repository:
https://github.com/geoelectric/mozmill-api-refactor/commit/fe07bd3ab8b9f2448db0ef850689ab987c881ee9