Implement WebPermissionClient for Document and Worker contexts

Implement WebPermissionClient for Document and Worker contexts

It does enable Permissions.query() in Document and Worker context.

It is using the mojo pipe associated to the RenderFrameImpl or BlinkPlatformImpl
so the PermissionManager doesn't need to know in which context it lives.
However, it needs to do some thread jumping to be back in the main thread.

This is based on top of https://codereview.chromium.org/804553003

BUG=437770
Committed: https://crrev.com/670a86def14a54e127fdad702be51572a81532a8
Cr-Commit-Position: refs/heads/master@{#320927}

[mlamouri (slow - plz ping), 2015-03-06 12:21:51]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org, tsepez@chromium.org

[mlamouri (slow - plz ping), 2015-03-06 12:21:51]

mlamouri@chromium.org changed required reviewers:
+ jochen@chromium.org, tsepez@chromium.org

[mlamouri (slow - plz ping), 2015-03-06 12:21:52]

jochen@, can you have a look at the content/ changes.

tsepez@, can you have a look at the *.mojom changes.

mkwst@, could you have a look at this CL as reviewer of the CL it is depending
on.

Thanks all :)

[Tom Sepez, 2015-03-06 17:48:27]

Mojom LGTM

[jochen (gone - plz use gerrit), 2015-03-09 15:36:32]

permissionClient vs permission*s*Manager is a bit odd, don't you think?

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
File content/child/permissions/permissions_manager.h (right):

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
content/child/permissions/permissions_manager.h:72: };
disallow copy/assign

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
File content/child/permissions/permissions_manager_thread_proxy.h (right):

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
content/child/permissions/permissions_manager_thread_proxy.h:17: };
no ;

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
content/child/permissions/permissions_manager_thread_proxy.h:24: public
NON_EXPORTED_BASE(blink::WebPermissionClient),
the NON_EXPORTED_BASE() should not be required since
PermissionsManagerThreadProxy isn't exported either, no?

[mlamouri (slow - plz ping), 2015-03-11 11:08:14]

I can't agree more regarding the name. Even I was confused (something was
writing PermissionManager and sometimes PermissionsManager). I meant to change
it but I ended up working on that weeks after the initial writing.

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
File content/child/permissions/permissions_manager.h (right):

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
content/child/permissions/permissions_manager.h:72: };
On 2015/03/09 at 15:36:31, jochen (slow) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
File content/child/permissions/permissions_manager_thread_proxy.h (right):

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
content/child/permissions/permissions_manager_thread_proxy.h:17: };
On 2015/03/09 at 15:36:31, jochen (slow) wrote:
> no ;

Done.

https://codereview.chromium.org/801613004/diff/60001/content/child/permission...
content/child/permissions/permissions_manager_thread_proxy.h:24: public
NON_EXPORTED_BASE(blink::WebPermissionClient),
On 2015/03/09 at 15:36:31, jochen (slow) wrote:
> the NON_EXPORTED_BASE() should not be required since
PermissionsManagerThreadProxy isn't exported either, no?

Terrible copy-pasta from the non-proxy class.

[mlamouri (slow - plz ping), 2015-03-11 11:08:23]

PTAL

[jochen (gone - plz use gerrit), 2015-03-11 16:06:56]

lgtm

https://codereview.chromium.org/801613004/diff/80001/content/child/permission...
File content/child/permissions/permission_manager.h (right):

https://codereview.chromium.org/801613004/diff/80001/content/child/permission...
content/child/permissions/permission_manager.h:27: public
NON_EXPORTED_BASE(blink::WebPermissionClient) {
more copy/paste?

[mlamouri (slow - plz ping), 2015-03-11 17:13:18]

https://codereview.chromium.org/801613004/diff/80001/content/child/permission...
File content/child/permissions/permission_manager.h (right):

https://codereview.chromium.org/801613004/diff/80001/content/child/permission...
content/child/permissions/permission_manager.h:27: public
NON_EXPORTED_BASE(blink::WebPermissionClient) {
On 2015/03/11 at 16:06:56, jochen (slow) wrote:
> more copy/paste?

No, that's just me being stupid :)

[mlamouri (slow - plz ping), 2015-03-11 17:13:19]

https://codereview.chromium.org/801613004/diff/80001/content/child/permission...
File content/child/permissions/permission_manager.h (right):

https://codereview.chromium.org/801613004/diff/80001/content/child/permission...
content/child/permissions/permission_manager.h:27: public
NON_EXPORTED_BASE(blink::WebPermissionClient) {
On 2015/03/11 at 16:06:56, jochen (slow) wrote:
> more copy/paste?

No, that's just me being stupid :)

[mlamouri (slow - plz ping), 2015-03-12 10:45:36]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-03-12 10:45:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/801613004/#ps100001
(title: "review commetns")

[commit-bot: I haz the power, 2015-03-12 10:46:11]

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

[commit-bot: I haz the power, 2015-03-12 11:21:49]

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

[commit-bot: I haz the power, 2015-03-12 11:21:50]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[mlamouri (slow - plz ping), 2015-03-12 11:26:25]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-03-12 11:26:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/801613004/#ps120001
(title: "stupid compilerS")

[commit-bot: I haz the power, 2015-03-12 11:26:36]

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

[commit-bot: I haz the power, 2015-03-12 15:01:25]

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

[commit-bot: I haz the power, 2015-03-12 15:01:27]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2015-03-16 12:27:30]

jochen@, I had to do some changes in how I connect to the mojo service. I'm not
connecting lazily which requires a ServiceRegistry pointer to be kept. I believe
it is fine because the SeriveRegistry will always outlive PermissionManager but
the change is significant enough that I would prefer you to have another look.

[jochen (gone - plz use gerrit), 2015-03-17 14:16:50]

still lgtm

[mlamouri (slow - plz ping), 2015-03-17 14:17:17]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-03-17 14:17:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/801613004/#ps160001
(title: "with comments")

[commit-bot: I haz the power, 2015-03-17 14:17:28]

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

[commit-bot: I haz the power, 2015-03-17 16:06:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-17 16:07:04]

Patchset 9 (id:??) landed as
https://crrev.com/670a86def14a54e127fdad702be51572a81532a8
Cr-Commit-Position: refs/heads/master@{#320927}

[Bernhard Bauer, 2015-03-23 18:27:41]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-03-23 18:27:42]

Post-commit driveby review! Re-posting some comments from
https://codereview.chromium.org/990303002/, where this commit snuck in:

https://codereview.chromium.org/801613004/diff/160001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/801613004/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.cc:1087: if (!permission_client_.get())
scoped_ptr has an implicit conversion to bool, so the .get() isn't necessary.

https://codereview.chromium.org/801613004/diff/160001/content/child/blink_pla...
content/child/blink_platform_impl.cc:1094: main_thread_task_runner_.get(),
permission_client_.get());
What will happen if |main_thread_task_runner_| is null? IsMainThread() will
return false, but this will pass a null task runner.

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
File content/child/permissions/permission_manager.cc (right):

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager.cc:104: // callback will not leak.
In the case of |this| gets deleted, the
s/In the case of/In case/ (or "In the case of |this| getting deleted").

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager.cc:134: // gracefully fail,
destroying the callback too.
I don't understand this comment. The return value of PostTask is not checked.
And which callback is this about? The |callback| variable or the closure bound
below?

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager.cc:138: base::Unretained(callback),
Could you pass this base::Owned or in a scoped_ptr?

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
File content/child/permissions/permission_manager.h (right):

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager.h:43: protected:
Why protected? Is this class meant to be subclassed?

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager.h:53: static void
RunCallbackOnWorkerThread(
You could move this to an anonymous namespace in the .cc file.

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager.h:59: class CallbackInformation {
Can you only forward-declare this class here?

Also, types should go first in each section.

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
File content/child/permissions/permission_manager_thread_proxy.cc (right):

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager_thread_proxy.cc:24:
LazyInstance<ThreadLocalPointer<PermissionManagerThreadProxy>>::Leaky
Why do we need a separate instance of this class for each thread?

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
File content/child/permissions/permission_manager_thread_proxy.h (right):

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager_thread_proxy.h:23: class
PermissionManagerThreadProxy :
Can you add a comment that explains what this class does?

https://codereview.chromium.org/801613004/diff/160001/content/child/permissio...
content/child/permissions/permission_manager_thread_proxy.h:24: public
blink::WebPermissionClient,
Does this not fit on the previous line anymore?