1081006 - Improve windows.waitForWindowState to also accept ID of the window to catch

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

As I've seen in bug 908649, on the unknownContentType dialog, this doesn't have a specific type but an ID. There might be also other dialogs for which would be great to wait for the ID.

Let's improve the waitForWindowState to also accept catching the windows by their ID.

Comment 1

[Daniel Gherasim]

Attachment: fix.patch

Here is the fix.

Comment 2

[Daniel Gherasim]

Attachment: fix_updated.patch

Updated the comment a bit.

Comment 3

[Daniel Gherasim]

Attachment: fix_2.patch

Working on this I just noticed we missed to add curly braces in that if statement when implementing the method. :D

Added it here.

Comment 4

[Andrei Eftimie]

Comment on attachment 8503024 [details] [diff] [review]
fix_2.patch

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

lgtm.

As noted below, if you can add a short test for the ID as well it would be great.
With that ask a review from Henrik please.

::: lib/windows.js
@@ +215,5 @@
>        assert.waitFor(() => {
>          return observer.getWindows().some(aWindow => {
>            var windowType = aWindow.document.documentElement.getAttribute("windowtype");
> +          var windowId = aWindow.document.documentElement.id;
> +          if (windowType === aSpec.type || windowId === aSpec.id) {

It would be great to also have a short test for ID (if it's feasible).

Comment 5

[Daniel Gherasim]

Attachment: fix_v2.patch

Thanks, updated!

Comment 6

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

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

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

::: lib/windows.js
@@ +180,5 @@
>   *        Callback function that triggers the window's state changing
>   * @param {object} aSpec
>   *        Information about the window to handle
> + * @param {string} [aSpec.id]
> + *        Id of the window to wait for

remove 'to wait for'.

@@ +195,5 @@
>  
>    var aEventType = OBSERVER_WINDOW_READY;
>    if (aSpec.state === WINDOW_STATES.open) {
> +    assert.ok(aSpec.type || aSpec.id,
> +              "Type or Id of the window to open has been specified");

'ID'

@@ +215,5 @@
>        assert.waitFor(() => {
>          return observer.getWindows().some(aWindow => {
>            var windowType = aWindow.document.documentElement.getAttribute("windowtype");
> +          var windowId = aWindow.document.documentElement.id;
> +          if (windowType === aSpec.type || windowId === aSpec.id) {

What if the window doesn't have a type (undefined) set, and the call to this method also passes in undefined for aSpec.type? We would get the wrong window. Actually we even missed this with the original landing, but given that type was required for open it was not that terrible. Here we have to fix that for sure.

Comment 7

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> What if the window doesn't have a type (undefined) set, and the call to this
> method also passes in undefined for aSpec.type? We would get the wrong
> window. Actually we even missed this with the original landing, but given
> that type was required for open it was not that terrible. Here we have to
> fix that for sure.

Definitively worth looking into. From my (limited) testing if `windowtype` does not exist, we get `null`, not `undefined`. So this might be a non issue.

> document.documentElement.getAttribute("windowtype") // null

Comment 8

[Daniel Gherasim]

Indeed getAttribute returns null or "" in case the attribute doesn't exist, so the comparison will return false as expected.
https://developer.mozilla.org/en-US/docs/Web/API/element.getAttribute

Thanks Andrei and Henrik for review! Will update shortly.

Comment 9

[Daniel Gherasim]

Attachment: fix_v3.patch

Fixed those 2 nits.

Comment 10

[Andrei Eftimie]

Comment on attachment 8504549 [details] [diff] [review]
fix_v3.patch

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

Passing the r flag back to Henrik for comment 7 and comment 8 regarding undefined vs null windowtype

Comment 11

[Daniel Gherasim]

Comment on attachment 8504549 [details] [diff] [review]
fix_v3.patch

Applies cleanly on all branches (default, aurora & beta).

Comment 12

[Andrei Eftimie]

remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/709838de6f38 (default)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/2086648a2082 (mozilla-aurora)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/11146005f037 (mozilla-beta)