1322586 - [geckoview] Add PermissionsDelegate

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

This will allow consumers of GeckoView to respond to web permissions requests. Needs more thorough thought as to how that interacts with Android permissions, but in generaly the App would requests those simultaneously or beforehand.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1322586 - 1. Add "always listen" option for GeckoViewHandler;

It would be nice to reuse GeckoViewHandler for delegates too (e.g. for
the new PermissionDelegate), but delegates need to always listen to
their events. This patch adds an "always listen" option for
GeckoViewHandler that still keeps track of the current listener/delegate
instance, but registers for all events at initialization rather than
only when there is a listener.

Review commit: https://reviewboard.mozilla.org/r/158512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158512/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1322586 - 2. Support content event forwarding in Messaging.jsm;

For a lot of GeckoView content process code, we send a message from
content to chrome, using a message manager, only to forward the message
to Java as an event, using an event dispatcher. This patch gives
Messaging.jsm the ability to be used in content process directly, and
lets events transparently pass through to Java.

Review commit: https://reviewboard.mozilla.org/r/158514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158514/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1322586 - 3. Add GeckoViewPermission.js for handling permissions;

Add GeckoViewPermission.js, which is responsible for handling requests
from Gecko for permissions, and passing those requests to GeckoView.

Review commit: https://reviewboard.mozilla.org/r/158516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158516/

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1322586 - 4. Add and implement PermissionDelegate support;

Add PermissionDelegate interface and implement support for Android and
content permissions (WebRTC permissions will be implemented later).

Review commit: https://reviewboard.mozilla.org/r/158518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158518/

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1322586 - 5. Implement PermissionDelegate for geckoview_example;

Add a sample implementation of PermissionDelegate for geckoview_example;
Because the prompt code has some existing boilerplate, the actual
prompts are implemented in BasicGeckoViewPrompt.

Review commit: https://reviewboard.mozilla.org/r/158520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158520/

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8887628 [details]
Bug 1322586 - 1. Add "always listen" option for GeckoViewHandler;

https://reviewboard.mozilla.org/r/158512/#review164140

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewHandler.java:55
(Diff revision 1)
> +        if (mListener == listener) {
> +            return;
> +        }
>  
> -        if (mListener != null && mListener != listener) {
> +        if (!mAlwaysListen && mListener != null) {
>              final GeckoBundle msg = new GeckoBundle(1);

Although we only use it here, it might be nicer to move out unregister into its own method for symmetry.

Comment 7

[Eugen Sawin [:esawin]]

Comment on attachment 8887629 [details]
Bug 1322586 - 2. Support content event forwarding in Messaging.jsm;

https://reviewboard.mozilla.org/r/158514/#review164148

This is very useful, we'll be applying this pattern a lot. Thanks!

We might want to move this out into its own bug.

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8887630 [details]
Bug 1322586 - 3. Add GeckoViewPermission.js for handling permissions;

https://reviewboard.mozilla.org/r/158516/#review164170

There is some more complexity here compared to other GeckoView components, function comments would help with understanding.
We should also think about adding specific tests for this.

Comment 9

[Eugen Sawin [:esawin]]

Comment on attachment 8887631 [details]
Bug 1322586 - 4. Add and implement PermissionDelegate support;

https://reviewboard.mozilla.org/r/158518/#review164176

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:194
(Diff revision 1)
>          };
>  
> +    private final GeckoViewHandler<PermissionDelegate> mPermissionHandler =
> +        new GeckoViewHandler<PermissionDelegate>(
> +            "GeckoViewPermission", this,
> +            new String[] { "GeckoView:AndroidPermission",

Put this on a new line for consistency.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8887630 [details]
Bug 1322586 - 3. Add GeckoViewPermission.js for handling permissions;

https://reviewboard.mozilla.org/r/158516/#review164196

::: mobile/android/components/geckoview/GeckoViewPermission.js:153
(Diff revision 1)
> +      denyRequest();
> +    });
> +  },
> +
> +  handlePeerConnectionRequest: function(aRequest) {
> +    Services.obs.notifyObservers(null, "PeerConnection:response:deny", aRequest.callID);

In Fennec we always allow here. Why do we deny in this instance?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8887630 [details]
Bug 1322586 - 3. Add GeckoViewPermission.js for handling permissions;

https://reviewboard.mozilla.org/r/158516/#review164198

Also, do we really need JS for this? It seems like we could handle these observer events in native code pretty easily.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

I think the handlers themselves are easier to write in JS, especially when doing EventDispatcher/async stuff.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> Comment on attachment 8887630 [details]
> Bug 1322586 - 3. Add GeckoViewPermission.js for handling permissions;
> 
> https://reviewboard.mozilla.org/r/158516/#review164196
> 
> > +  handlePeerConnectionRequest: function(aRequest) {
> > +    Services.obs.notifyObservers(null, "PeerConnection:response:deny", aRequest.callID);
> 
> In Fennec we always allow here. Why do we deny in this instance?

Nice catch. Should be allow.

Comment 13

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8887632 [details]
Bug 1322586 - 5. Implement PermissionDelegate for geckoview_example;

https://reviewboard.mozilla.org/r/158520/#review164682

Looks good to me.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/BasicGeckoViewPrompt.java:174
(Diff revision 1)
>      private LinearLayout addStandardLayout(final AlertDialog.Builder builder,
>                                             final String title, final String msg,
>                                             final AlertCallback callback) {
>          final ScrollView scrollView = new ScrollView(builder.getContext());
>          final LinearLayout container = new LinearLayout(builder.getContext());
>          final int padding = getViewPadding(builder);

Nit: Rename to horizontalPadding for consistency.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8887628 [details]
Bug 1322586 - 1. Add "always listen" option for GeckoViewHandler;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158512/diff/1-2/

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8887629 [details]
Bug 1322586 - 2. Support content event forwarding in Messaging.jsm;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158514/diff/1-2/

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8887630 [details]
Bug 1322586 - 3. Add GeckoViewPermission.js for handling permissions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158516/diff/1-2/

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8887631 [details]
Bug 1322586 - 4. Add and implement PermissionDelegate support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158518/diff/1-2/

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8887632 [details]
Bug 1322586 - 5. Implement PermissionDelegate for geckoview_example;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158520/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/446be06fd42e
1. Add "always listen" option for GeckoViewHandler; r=esawin
https://hg.mozilla.org/integration/autoland/rev/1ca88db9fe80
2. Support content event forwarding in Messaging.jsm; r=esawin
https://hg.mozilla.org/integration/autoland/rev/f93c592a2810
3. Add GeckoViewPermission.js for handling permissions; r=esawin
https://hg.mozilla.org/integration/autoland/rev/44b8d2f6c838
4. Add and implement PermissionDelegate support; r=esawin
https://hg.mozilla.org/integration/autoland/rev/942cdcab7f79
5. Implement PermissionDelegate for geckoview_example; r=droeh

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/446be06fd42e
https://hg.mozilla.org/mozilla-central/rev/1ca88db9fe80
https://hg.mozilla.org/mozilla-central/rev/f93c592a2810
https://hg.mozilla.org/mozilla-central/rev/44b8d2f6c838
https://hg.mozilla.org/mozilla-central/rev/942cdcab7f79