Add permissions helper class to carry out promise based testing

Add permissions helper class to carry out promise based testing

There are numerous flaky tests which are likely caused by race conditions
between testRunner.setPermission and subsequent operations.

Introduce a helper class which wraps testRunner.setPermission
with a promise based system which can be used to prevent races

BUG=(multiple)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201057

[Lalit Maganti, 2015-08-21 15:39:01]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[Lalit Maganti, 2015-08-21 15:39:01]

I've gone down the route of splitting up the commits after the situation with
ManifestIconDownloader and the number of iterations we had to do there.

I'll have the follow up patches up soon.

[mlamouri (slow - plz ping), 2015-08-21 17:15:23]

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
File LayoutTests/resources/permissions-helper.js (right):

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:1: function
permissionNameToPermissionObject(permissionName) {
It would be good if we could make sure this is not accessible from outside this
file.

I'm not a JS expert but what about doing:

var PermissionsHelper = (function() {
  function permissionNameToObject(name) {
    [...]
  }

  return {
    setPermission: function() {
      [...] // alternatively, you have it define above and you just use the
internal one here.
    }
  }
});

I think that will make PermissionsHelper.setPermission() available from script
include this file.

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:10: return {name: "midi"}
midi?

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:12: return {name: "geolocation",
sysex: true}
sysex?

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:15: // We can't deal with these two
yet.
These 'two'?

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:16: return {name: "midi"}
Can you actually throw in that case?

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:20: function
setPermission(testRunner, permissionName, newState) {
Can we use testRunner directly instead of taking it? We should be in the same
scope, right?

nit: s/newState/state/
nit: s/permissionName/name/

[Lalit Maganti, 2015-08-21 18:30:19]

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
File LayoutTests/resources/permissions-helper.js (right):

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:1: function
permissionNameToPermissionObject(permissionName) {
On 2015/08/21 17:15:23, Mounir Lamouri wrote:
> It would be good if we could make sure this is not accessible from outside
this
> file.
> 
> I'm not a JS expert but what about doing:
> 
> var PermissionsHelper = (function() {
>   function permissionNameToObject(name) {
>     [...]
>   }
> 
>   return {
>     setPermission: function() {
>       [...] // alternatively, you have it define above and you just use the
> internal one here.
>     }
>   }
> });
> 
> I think that will make PermissionsHelper.setPermission() available from script
> include this file.

Done.

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:10: return {name: "midi"}
On 2015/08/21 17:15:23, Mounir Lamouri wrote:
> midi?

Done.

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:12: return {name: "geolocation",
sysex: true}
On 2015/08/21 17:15:23, Mounir Lamouri wrote:
> sysex?

Done.

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:15: // We can't deal with these two
yet.
On 2015/08/21 17:15:23, Mounir Lamouri wrote:
> These 'two'?

Done.

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:16: return {name: "midi"}
On 2015/08/21 17:15:23, Mounir Lamouri wrote:
> Can you actually throw in that case?

Done.

https://codereview.chromium.org/1305043004/diff/20001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:20: function
setPermission(testRunner, permissionName, newState) {
On 2015/08/21 17:15:23, Mounir Lamouri wrote:
> Can we use testRunner directly instead of taking it? We should be in the same
> scope, right?
> 
> nit: s/newState/state/
> nit: s/permissionName/name/

Done.

[mlamouri (slow - plz ping), 2015-08-22 10:19:29]

lgtm with comments applied.

I think it might be great if you could add a quick documentation at the top of
the file explaining how to use this and the testRunner requirements. Something
like:
(I've added "use strict" too)

// This file provides a PermissionsHelper object which can be used by
LayoutTests using
// testRunner to handle permissions.
//
// The current available methods are:
// - setPermission: given a permission name (known by testRunner) and a state,
it will
// set the permission to the specified state and resolve the promise when done.
// Example:
// PermissionsHelper.setPermission('geolocation', 'prompt').then(runTest);

"use strict";

https://codereview.chromium.org/1305043004/diff/60001/LayoutTests/resources/p...
File LayoutTests/resources/permissions-helper.js (right):

https://codereview.chromium.org/1305043004/diff/60001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:5: return {name: "midi"}
nit: add ;

same for all the following return statements.

Maybe you can add "use strict;" at the beginning of the file to prevent that
kind of mistakes?

https://codereview.chromium.org/1305043004/diff/60001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:15: throw "Invalid "
nit: could you throw something that would be more helpful to the caller? :)

[Lalit Maganti, 2015-08-24 09:18:23]

https://codereview.chromium.org/1305043004/diff/60001/LayoutTests/resources/p...
File LayoutTests/resources/permissions-helper.js (right):

https://codereview.chromium.org/1305043004/diff/60001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:5: return {name: "midi"}
On 2015/08/22 10:19:29, Mounir Lamouri wrote:
> nit: add ;
> 
> same for all the following return statements.
> 
> Maybe you can add "use strict;" at the beginning of the file to prevent that
> kind of mistakes?

Done.

https://codereview.chromium.org/1305043004/diff/60001/LayoutTests/resources/p...
LayoutTests/resources/permissions-helper.js:15: throw "Invalid "
On 2015/08/22 10:19:29, Mounir Lamouri wrote:
> nit: could you throw something that would be more helpful to the caller? :)

I really shouldn't write CLs late on Friday :)

Done.

[Lalit Maganti, 2015-08-24 09:25:55]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-24 09:25:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1305043004/#ps80001
(title: "Fix review comments")

[commit-bot: I haz the power, 2015-08-24 09:26:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305043004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305043004/80001

[mlamouri (slow - plz ping), 2015-08-24 09:27:09]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-08-24 09:27:38]

I think you missed the first comment.

[Lalit Maganti, 2015-08-24 09:34:01]

Sorry about that. Will be more careful from now on.

Added the comment.

[Lalit Maganti, 2015-08-24 09:34:21]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-24 09:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1305043004/#ps100001
(title: "Added file level comment")

[commit-bot: I haz the power, 2015-08-24 09:34:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305043004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305043004/100001

[commit-bot: I haz the power, 2015-08-24 10:15:30]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201057