Stub implementation of Permissions API module.

Stub implementation of Permissions API module.

This is adding the Permissions interface with the query()
method and implements it by always rejecting.

This CL also introduces a test framework to be able to
test the API in different scopes easily and uses it to
test the above mentioned implementation.

Intent to implement thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/wfszE6uUmk8/r9ucFjbfvY4J

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

[mlamouri (slow - plz ping), 2014-12-01 10:33:25]

mlamouri@chromium.org changed reviewers:
+ mkwst@chromium.org, peter@chromium.org

[Peter Beverloo, 2014-12-01 11:30:35]

Mostly lg, thanks! Have a series of comments.

Can you include a link to the Intent to Implement on blink-dev in your CLs
description?

On a tangent, I'm not a fan of using an (OWP) launch bug as the BUG= for the
engineering effort. Perhaps you can split an Eng bug from the launch bug?

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/resources/helpers.js (right):

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:2: //
http://creativecommons.org/publicdomain/zero/1.0/
Please don't add this copyright header. (Nor in the other files.)

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:15: return 'Window';
indentation

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:19: return
'DedicatedWorker';
indentation

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/test-api-surface.html (right):

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/test-api-surface.html:13: resources/
directory.
Can we make this explicit, rather than magical?

<script>
  runTest('resources/test-api-surface.js');
</script>

Imagine a situation in which this test is flaky. Explicitly calling a method
executing the tests is much clearer and easier to triage for people unfamiliar
with these tests than an if-statement in a file that's being included by both
the .html and .js pages.

It's also not obvious that the test will be ran in four different contexts:
Documents, Dedicated Workers, Shared Workers and Service Workers. When running
the test manually, what does the output look like? Is it clear which part of the
test is failing?

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/PermissionStatus.h (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.h:9: #include
"core/events/EventTarget.h"
IWYU:

platform/heap/Handle.h
wtf/text/AtomicString.h
wtf/text/WTFString.h

class ExecutionContext;

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.h:22: virtual ~PermissionStatus();
~PermissionStatus() override;

At least EventTargetWithInlineData provides a destructor.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/PermissionStatus.idl (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.idl:10: 
Please include a link to the specification in this file.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/Permissions.h:9: 
+ bindings/core/v8/ScriptPromise.h
+ platform/heap/Handle.h

And AtomicString if you feel adventurous.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/Permissions.idl (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/Permissions.idl:10: 
Please include a link to the specification in this file.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/Permissions.idl:17: static Promise<PermissionStatus>
query(PermissionName permission);
nit: single line

[mlamouri (slow - plz ping), 2014-12-01 12:36:01]

Comments applied.

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/resources/helpers.js (right):

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:2: //
http://creativecommons.org/publicdomain/zero/1.0/
On 2014/12/01 at 11:30:34, Peter Beverloo wrote:
> Please don't add this copyright header. (Nor in the other files.)

This is unfortunate.

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:15: return 'Window';
On 2014/12/01 at 11:30:34, Peter Beverloo wrote:
> indentation

Fixed.

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:19: return
'DedicatedWorker';
On 2014/12/01 at 11:30:34, Peter Beverloo wrote:
> indentation

Fixed.

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/test-api-surface.html (right):

https://codereview.chromium.org/760223003/diff/40001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/test-api-surface.html:13: resources/
directory.
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> Can we make this explicit, rather than magical?
> 
> <script>
>   runTest('resources/test-api-surface.js');
> </script>
> 
> Imagine a situation in which this test is flaky. Explicitly calling a method
executing the tests is much clearer and easier to triage for people unfamiliar
with these tests than an if-statement in a file that's being included by both
the .html and .js pages.
> 
> It's also not obvious that the test will be ran in four different contexts:
Documents, Dedicated Workers, Shared Workers and Service Workers. When running
the test manually, what does the output look like? Is it clear which part of the
test is failing?

Yeah, I agree, it's a bit too magical. I've added a runTest() method.

The error will specify the scope.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/PermissionStatus.h (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.h:9: #include
"core/events/EventTarget.h"
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> IWYU:
> 
> platform/heap/Handle.h
> wtf/text/AtomicString.h
> wtf/text/WTFString.h
> 
> class ExecutionContext;

Done.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.h:22: virtual ~PermissionStatus();
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> ~PermissionStatus() override;
> 
> At least EventTargetWithInlineData provides a destructor.

Done.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/PermissionStatus.idl (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.idl:10: 
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> Please include a link to the specification in this file.

Sure.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/PermissionStatus.idl:10: 
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> Please include a link to the specification in this file.

Sure.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/Permissions.h:9: 
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> + bindings/core/v8/ScriptPromise.h
> + platform/heap/Handle.h
> 
> And AtomicString if you feel adventurous.

Done. I fwd declared AtomicString.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
File Source/modules/permissions/Permissions.idl (right):

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/Permissions.idl:10: 
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> Please include a link to the specification in this file.

Sure.

https://codereview.chromium.org/760223003/diff/40001/Source/modules/permissio...
Source/modules/permissions/Permissions.idl:17: static Promise<PermissionStatus>
query(PermissionName permission);
On 2014/12/01 at 11:30:35, Peter Beverloo wrote:
> nit: single line

Done but I think it's really not readable in one line. Two lines separate the
flags with the actual content.

[Peter Beverloo, 2014-12-01 13:26:07]

lgtm

Your CL description now mentions "Intent to Ship", but I'm sure you mean "Intent
to Implement"?

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/resources/helpers.js (right):

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:38: // Run the tests on
{Dedicated,Shared,Shared}Worker.
nit: s/Shared\}/Service\}/

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/resources/test-api-surface.js (right):

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/test-api-surface.js:1: if
(self.importScripts) {
Would it make sense to include this block (lines 1-8) in helpers.js, since it's
going to be repeated for every test?

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/test-query.html (right):

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/test-query.html:14: runTest();
I still have a preference of explicitly stating the filename here, but do as you
please.

[mlamouri (slow - plz ping), 2014-12-01 13:33:18]

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/resources/helpers.js (right):

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/helpers.js:38: // Run the tests on
{Dedicated,Shared,Shared}Worker.
On 2014/12/01 at 13:26:07, Peter Beverloo wrote:
> nit: s/Shared\}/Service\}/

Done.

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/resources/test-api-surface.js (right):

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/resources/test-api-surface.js:1: if
(self.importScripts) {
On 2014/12/01 at 13:26:07, Peter Beverloo wrote:
> Would it make sense to include this block (lines 1-8) in helpers.js, since
it's going to be repeated for every test?

As discussed f2f, it's unlikely that we will have more than two tests in the
near future. Let's refactor the 4 lines when needed. (The issue is that it will
make helpers.js running code at insertion time which it doesn't for now and will
have code running depending on the context, which it doesn't too.)

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/permissions/test-query.html (right):

https://codereview.chromium.org/760223003/diff/80001/LayoutTests/http/tests/p...
LayoutTests/http/tests/permissions/test-query.html:14: runTest();
On 2014/12/01 at 13:26:07, Peter Beverloo wrote:
> I still have a preference of explicitly stating the filename here, but do as
you please.

I think no filename has the benefit of preventing errors in copy-paste (ie.
test-foo.html running test-bar.js).

[Mike West, 2014-12-02 12:39:38]

LGTM

https://codereview.chromium.org/760223003/diff/120001/Source/modules/permissi...
File Source/modules/permissions/OWNERS (right):

https://codereview.chromium.org/760223003/diff/120001/Source/modules/permissi...
Source/modules/permissions/OWNERS:1: mlamouri@chromium.org
It's probably a good idea to find someone else who would also be willing to
review patches in this module, just so you're not stuck doing all the work. :)

[mlamouri (slow - plz ping), 2014-12-02 12:41:31]

On 2014/12/02 at 12:39:38, mkwst wrote:
> LGTM
> 
>
https://codereview.chromium.org/760223003/diff/120001/Source/modules/permissi...
> File Source/modules/permissions/OWNERS (right):
> 
>
https://codereview.chromium.org/760223003/diff/120001/Source/modules/permissi...
> Source/modules/permissions/OWNERS:1: mlamouri@chromium.org
> It's probably a good idea to find someone else who would also be willing to
review patches in this module, just so you're not stuck doing all the work. :)

timvolodine@ is going to work with me on that. I will add him after his first
patches.

Thanks for the review! :)

[mlamouri (slow - plz ping), 2014-12-02 12:41:40]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-12-02 12:42:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/760223003/120001

[commit-bot: I haz the power, 2014-12-02 13:29:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-02 13:29:13]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[mlamouri (slow - plz ping), 2014-12-02 14:10:34]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-12-02 14:10:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/760223003/140001

[commit-bot: I haz the power, 2014-12-02 16:43:35]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186310