blink: permissions: add plumbing to revoke permissions

blink: permissions: add plumbing to revoke permissions

Add plumbing code in Blink to allow revocation of permissions.

NOTE: this is a part of a three sided patch:
https://codereview.chromium.org/1234023002 (this)
https://codereview.chromium.org/1238673002
https://codereview.chromium.org/1241133002

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

[Lalit Maganti, 2015-07-16 08:32:59]

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

[mlamouri (slow - plz ping), 2015-07-17 13:16:18]

Given that you are still working on the tests I have not looked at them yet. I
left some comments on the implementation. Looks good!

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/PermissionCallback.h (left):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/PermissionCallback.h:27: explicit
PermissionQueryCallback(PassRefPtr<ScriptPromiseResolver>, WebPermissionType);
nit: can you remove explicit as long as you are here. It seems that this ctor
has two arguments now.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.cpp:39: bool quickHandlePermissions(
Could you call that "handleDefaultBehaviour()" and add a comment that explains
what the method is doing.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.cpp:83: return promise;
if (handleDefaultBehaviour(...))
    return promise;

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.cpp:113: return promise;
ditto

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.h:29: WebPermissionType
convertPermissionStringToType(const String& name);
Could you have this method in an anonymous namespace inside the cpp file? It
doesn't need to be exposed outside of Permissions and isn't using |this|.

[mlamouri (slow - plz ping), 2015-07-17 13:18:04]

https://codereview.chromium.org/1234023002/diff/110001/public/platform/module...
File public/platform/modules/permissions/WebPermissionClient.h (right):

https://codereview.chromium.org/1234023002/diff/110001/public/platform/module...
public/platform/modules/permissions/WebPermissionClient.h:14: using
WebPermissionCallback = WebCallbacks<WebPermissionStatus, void>;
I think this change will break Chromium. You might want to have the two types
defined then remove the QueryCallback when Chromium has been updated.

[Lalit Maganti, 2015-07-17 13:47:35]

Mounir: please see my remarks on your comments as well as my Javascript in the
tests.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/PermissionCallback.h (left):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/PermissionCallback.h:27: explicit
PermissionQueryCallback(PassRefPtr<ScriptPromiseResolver>, WebPermissionType);
On 2015/07/17 at 13:16:18, Mounir Lamouri wrote:
> nit: can you remove explicit as long as you are here. It seems that this ctor
has two arguments now.

Done. I didn't actually know what the explicit keyword does but I looked it up
and I'm in shock that implicit constructor calling a C++ "feature".

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.cpp:39: bool quickHandlePermissions(
On 2015/07/17 at 13:16:18, Mounir Lamouri wrote:
> Could you call that "handleDefaultBehaviour()" and add a comment that explains
what the method is doing.

Done. The name you suggest is nice - I couldn't really think of one which summed
up the behavior. Also I'm not sure if my comment is 100% accurate.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.cpp:83: return promise;
On 2015/07/17 at 13:16:18, Mounir Lamouri wrote:
> if (handleDefaultBehaviour(...))
>     return promise;

Done. This is a pattern I've got to get used to.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.cpp:113: return promise;
On 2015/07/17 at 13:16:18, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/Permissions.h:29: WebPermissionType
convertPermissionStringToType(const String& name);
On 2015/07/17 at 13:16:18, Mounir Lamouri wrote:
> Could you have this method in an anonymous namespace inside the cpp file? It
doesn't need to be exposed outside of Permissions and isn't using |this|.

Done.

https://codereview.chromium.org/1234023002/diff/110001/public/platform/module...
File public/platform/modules/permissions/WebPermissionClient.h (right):

https://codereview.chromium.org/1234023002/diff/110001/public/platform/module...
public/platform/modules/permissions/WebPermissionClient.h:14: using
WebPermissionCallback = WebCallbacks<WebPermissionStatus, void>;
On 2015/07/17 at 13:18:03, Mounir Lamouri wrote:
> I think this change will break Chromium. You might want to have the two types
defined then remove the QueryCallback when Chromium has been updated.

Done.

[mlamouri (slow - plz ping), 2015-07-20 14:14:12]

lgtm with the tests moved to the third CL and the comments applied.

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
File Source/modules/permissions/PermissionCallback.h (left):

https://codereview.chromium.org/1234023002/diff/110001/Source/modules/permiss...
Source/modules/permissions/PermissionCallback.h:27: explicit
PermissionQueryCallback(PassRefPtr<ScriptPromiseResolver>, WebPermissionType);
On 2015/07/17 at 13:47:35, Lalit Maganti wrote:
> On 2015/07/17 at 13:16:18, Mounir Lamouri wrote:
> > nit: can you remove explicit as long as you are here. It seems that this
ctor has two arguments now.
> 
> Done. I didn't actually know what the explicit keyword does but I looked it up
and I'm in shock that implicit constructor calling a C++ "feature".

The joys of backward compatibility :)

https://codereview.chromium.org/1234023002/diff/130001/Source/modules/permiss...
File Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/1234023002/diff/130001/Source/modules/permiss...
Source/modules/permissions/Permissions.h:11: #include
"public/platform/modules/permissions/WebPermissionType.h"
nit: I think you can remove that header.

https://codereview.chromium.org/1234023002/diff/130001/public/platform/module...
File public/platform/modules/permissions/WebPermissionClient.h (right):

https://codereview.chromium.org/1234023002/diff/130001/public/platform/module...
public/platform/modules/permissions/WebPermissionClient.h:15: using
WebPermissionCallback = WebPermissionQueryCallback;
nit: add a comment explaining that you keep WebPermissionQueryCallback for
backward compatibility.

[Lalit Maganti, 2015-07-21 09:32:32]

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

[Lalit Maganti, 2015-07-21 09:32:33]

mkwst@: could you please review the UseCounter and RuntimeEnabledFeatures?

[Mike West, 2015-07-21 09:53:07]

On 2015/07/21 at 09:32:33, lalitm wrote:
> mkwst@: could you please review the UseCounter and RuntimeEnabledFeatures?

LGTM % the layout tests that break with the new idl entry.

[Lalit Maganti, 2015-07-21 13:09:18]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-07-21 13:09:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1234023002/#ps210001
(title: "Fix failing webexposed tests")

[commit-bot: I haz the power, 2015-07-21 13:09:24]

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

[commit-bot: I haz the power, 2015-07-21 13:11:58]

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

[commit-bot: I haz the power, 2015-07-21 13:11:59]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/63480)

[Lalit Maganti, 2015-07-21 13:14:35]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-07-21 13:14:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1234023002/#ps230001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-21 13:14:45]

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

[commit-bot: I haz the power, 2015-07-21 15:13:01]

Committed patchset #13 (id:230001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199226