Permissions: glue between Mojo and Blink for permission observing.

Permissions: glue between Mojo and Blink for permission observing.

BUG=437770
Committed: https://crrev.com/5534c62553b7da15c7a1aa8ece7ac4f0628847bf
Cr-Commit-Position: refs/heads/master@{#323559}

[mlamouri (slow - plz ping), 2015-04-02 15:32:38]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[mlamouri (slow - plz ping), 2015-04-02 15:32:46]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org

[commit-bot: I haz the power, 2015-04-02 15:33:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1057453004/1

[commit-bot: I haz the power, 2015-04-02 15:46:11]

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

[commit-bot: I haz the power, 2015-04-02 15:46:11]

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

[whywhat, 2015-04-02 16:20:24]

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_dispatcher.cc (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:128: // We initialize we an
arbitrary value because the
nits:
s/we/with
s/worse/worst

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:154:
GetPermissionServicePtr()->GetNextPermissionChange(
shouldn't you call the internal GetNextPermissionChange method to avoid some
redundancy and confusion? it could take the callback as an argument and you
could move the default callback to some extra helper function?

Right now there's three methods that seem to be used without a clear pattern:
GetNextPermissionChange (looks like a helper function, called from a couple of
places without the callback)
GetNextPermissionChangeForWorker (public for some reason, does almost exactly
the same thing as startListeningForWorker())
GetPermissionServicePtr()->GetNextPermissionChange() that's called from three
places with slightly different params.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:157: // We initialize we an
arbitrary value because the mojo service wants a
nits:
s/we/with
s/worse/worst

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:265: worker_thread_id,
base::Bind(callback, GetWebPermissionStatus(status)));
You don't use RunCallbackOnWorkerThread here?

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_dispatcher.h (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.h:30: static bool
IsObservable(blink::WebPermissionType type);
Comment?

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.h:51: void
StartListeningForWorker(
why there's GetNextPermissionChangeForWorker but no StopListeningForWorker and
vice versa (no GetNextPermissionChange)?

If these are only helpers for PermissionDispatcherThreadProxy, maybe make them
private but friends to the proxy class?

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.h:66: static void
RunCallbackOnWorkerThread(
nit: I think the comment needs to clarify that it's not called from the main
thread but posted by the main thread on the worker thread.
Reading the comment I assumed that this is a helper method that posts the
callback on the right thread and wanted to suggest a rename to
PostCallbackOnWorkerThread until I read the implementation.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_dispatcher_thread_proxy.h (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher_thread_proxy.h:23: class
PermissionDispatcherThreadProxy :
The class name has the word Thread in it, indicating a potential multithreading
danger. Yet, there's no description of the class.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_observers_registry.h (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_observers_registry.h:16: class
PermissionObserversRegistry {
add class description
(e.g that it's not thread safe)

https://codereview.chromium.org/1057453004/diff/1/content/content_child.gypi
File content/content_child.gypi (right):

https://codereview.chromium.org/1057453004/diff/1/content/content_child.gypi#...
content/content_child.gypi:170:
'child/permissions/permission_observers_registry.cc',
do you need to change a .gn file?

[mlamouri (slow - plz ping), 2015-04-02 17:02:43]

PTAL

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_dispatcher.cc (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:128: // We initialize we an
arbitrary value because the
On 2015/04/02 at 16:20:23, whywhat wrote:
> nits:
> s/we/with
> s/worse/worst

Done.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:154:
GetPermissionServicePtr()->GetNextPermissionChange(
On 2015/04/02 at 16:20:23, whywhat wrote:
> shouldn't you call the internal GetNextPermissionChange method to avoid some
redundancy and confusion? it could take the callback as an argument and you
could move the default callback to some extra helper function?
> 
> Right now there's three methods that seem to be used without a clear pattern:
> GetNextPermissionChange (looks like a helper function, called from a couple of
places without the callback)
> GetNextPermissionChangeForWorker (public for some reason, does almost exactly
the same thing as startListeningForWorker())
> GetPermissionServicePtr()->GetNextPermissionChange() that's called from three
places with slightly different params.

I see what you mean but they are slightly different. Would you mind if I do that
in a follow-up?

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:157: // We initialize we an
arbitrary value because the mojo service wants a
On 2015/04/02 at 16:20:23, whywhat wrote:
> nits:
> s/we/with
> s/worse/worst

Done.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.cc:265: worker_thread_id,
base::Bind(callback, GetWebPermissionStatus(status)));
On 2015/04/02 at 16:20:23, whywhat wrote:
> You don't use RunCallbackOnWorkerThread here?

The reason is that I need to check whether the observer is still registered
before running the callback (which requires going back to the thread proxy).
Then, I need to go back to the main thread in order to get the next permission
change.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_dispatcher.h (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.h:30: static bool
IsObservable(blink::WebPermissionType type);
On 2015/04/02 at 16:20:23, whywhat wrote:
> Comment?

done.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.h:51: void
StartListeningForWorker(
On 2015/04/02 at 16:20:24, whywhat wrote:
> why there's GetNextPermissionChangeForWorker but no StopListeningForWorker and
vice versa (no GetNextPermissionChange)?
> 
> If these are only helpers for PermissionDispatcherThreadProxy, maybe make them
private but friends to the proxy class?

These three methods, as said in the comments are meant to be called from worker
threads. We usually avoid |friend| in Chromium. I don't really care much but I
generally would prefer if PermissionDispatcher has no knowledge of
PermissionDispatcherThreadProxy.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher.h:66: static void
RunCallbackOnWorkerThread(
On 2015/04/02 at 16:20:23, whywhat wrote:
> nit: I think the comment needs to clarify that it's not called from the main
thread but posted by the main thread on the worker thread.
> Reading the comment I assumed that this is a helper method that posts the
callback on the right thread and wanted to suggest a rename to
PostCallbackOnWorkerThread until I read the implementation.

Done.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_dispatcher_thread_proxy.h (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_dispatcher_thread_proxy.h:23: class
PermissionDispatcherThreadProxy :
On 2015/04/02 at 16:20:24, whywhat wrote:
> The class name has the word Thread in it, indicating a potential
multithreading danger. Yet, there's no description of the class.

Done.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
File content/child/permissions/permission_observers_registry.h (right):

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_observers_registry.h:16: class
PermissionObserversRegistry {
On 2015/04/02 at 16:20:24, whywhat wrote:
> add class description
> (e.g that it's not thread safe)

Done.

https://codereview.chromium.org/1057453004/diff/1/content/child/permissions/p...
content/child/permissions/permission_observers_registry.h:16: class
PermissionObserversRegistry {
On 2015/04/02 at 16:20:24, whywhat wrote:
> add class description
> (e.g that it's not thread safe)

Done.

https://codereview.chromium.org/1057453004/diff/1/content/content_child.gypi
File content/content_child.gypi (right):

https://codereview.chromium.org/1057453004/diff/1/content/content_child.gypi#...
content/content_child.gypi:170:
'child/permissions/permission_observers_registry.cc',
On 2015/04/02 at 16:20:24, whywhat wrote:
> do you need to change a .gn file?

No. Gypi files are often used by BUILD.gn files.

[whywhat, 2015-04-02 17:05:49]

lgtm

[mlamouri (slow - plz ping), 2015-04-02 17:05:53]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-04-02 17:07:00]

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

[commit-bot: I haz the power, 2015-04-02 21:10:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-03 20:28:56]

Patchset 2 (id:??) landed as
https://crrev.com/5534c62553b7da15c7a1aa8ece7ac4f0628847bf
Cr-Commit-Position: refs/heads/master@{#323559}