1490333 - Move whenIdle to sync module

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

The private whenIdle function in testing/marionette/driver.js has
since I wrote it been picked up in other contexts that initially
intended, which means we now need to access it from different places
throughout Marionette.

In preparation for the window tracking refactoring we need to make
it available across JSMs.  Moving it to testing/marionette/sync.js
is an obvious fix.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: marionette: move whenIdle to sync module

This patch moves the private whenIdle function to sync so it can
be shared across JSMs.

It also changes its semantics somewhat, so that instead of taking
a callback function (suitable for DOM event callbacks) it returns
a promise that is resolved when the main thread becomes idle and
the window has completed an animation frame tick.

Comment 2

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d27fcebc030478694210de760cd797fef4ec1dbe

Comment 3

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

Comment on attachment 9008085 [details] [diff] [review]
marionette: move whenIdle to sync module

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

::: testing/marionette/driver.js
@@ +3663,3 @@
>      window.fullScreen = false;
>    });
> +  await new IdlePromise(window);

Should we better move the assignment of `window.fullscreen` to after this line? Otherwise we could end-up in a race if something fails in `IdlePromise`.

::: testing/marionette/sync.js
@@ +15,5 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "logger", Log.get);
>  
>  this.EXPORTED_SYMBOLS = [
> +  "IdlePromise",

Mind adding at least a basic xpcshell test for it?

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9008085 [details] [diff] [review]
marionette: move whenIdle to sync module

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

::: testing/marionette/driver.js
@@ +3663,3 @@
>      window.fullScreen = false;
>    });
> +  await new IdlePromise(window);

We shouldn’t do this, because the effect we want to achieve is to
induce a delay _after_ switching out of fullscreen that is long
enough for the browser to not think it’s in fullscreen mode anymore.
Waiting for the main thread to become idle before exiting fullscreen
would have the opposite effect.

::: testing/marionette/sync.js
@@ +15,5 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "logger", Log.get);
>  
>  this.EXPORTED_SYMBOLS = [
> +  "IdlePromise",

Sorry, I forgot to do this.  It seems Services.tm is available, so
this shouldn’t be a problem.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3624871612dd
marionette: move whenIdle to sync module; r=whimboo

Comment 6

[Arthur Iakab [arthur_iakab]]

Backed out changeset 3624871612dd (bug 1490333) for Eslint failure 
Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/1c19ca93207d4cf1439602194b7a14a530204d9e
Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=3624871612dd4a23c604cadc5c67cdded584e881
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199160373&repo=mozilla-inbound

Comment 7

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #4)
> ::: testing/marionette/driver.js
> @@ +3663,3 @@
> >      window.fullScreen = false;
> >    });
> > +  await new IdlePromise(window);
> 
> We shouldn’t do this, because the effect we want to achieve is to
> induce a delay _after_ switching out of fullscreen that is long
> enough for the browser to not think it’s in fullscreen mode anymore.
> Waiting for the main thread to become idle before exiting fullscreen
> would have the opposite effect.

That is not what I was asking. My point was that `window.fullScreen` should be set to `false` **after** the idle promise has been resolved.

Comment 8

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

Please scratch my last comment because I totally misread the code. Sorry.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/567ccbeb5a4f
marionette: move whenIdle to sync module; r=whimboo

Comment 10

[Andreas Tolfsen ❲:ato❳]

Fixed the dangling comma.

Addressed misunderstanding with whimboo on IRC.

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/567ccbeb5a4f