1297475 - Provide a way for system add-ons to add handlers for nsIContentPermissionRequests

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

nsContentPermissionHelper.cpp and nsIContentPermissionRequest exist as a way for platform to request permission from the user to access various devices.

nsIContentPermissionRequest isn't used by everybody. For example, WebRTC uses their own home-brew permission mechanism, specifically because they have more sophisticated demands from the user when they request permissions (they need to know which device(s) the user wants to allow the site to access).

The only two permission types that still use nsIContentPermissionRequest are the geolocation permission prompt and desktop permissions.

Bug 1292639 needs to request the user's permission to open a server. Bug 1289974 needs to request the user's permission to present content on a nearby known device.

Both of those bugs are related to UI that is currently being developed as system add-ons.

Instead of forcing those system add-on devs to modify the nsIContentPermissionPrompt inside nsBrowserGlue.js (and the Fennec equivalent), I suggest we alter nsIContentPermissionPrompt so that handlers can be set (and unset) dynamically. This way, the permission prompting logic for, example, FlyWeb, can stay strictly within the FlyWeb system add-on. Same goes for Presentation API.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Thanks for writing in at https://mail.mozilla.org/pipermail/firefox-dev/2016-August/004533.html, paolo.

So this is the bug for the hook I'm hoping to add. I've studied Integration.jsm, and I think I understand it.

Integration.jsm seems to be great at allowing add-ons to override getters, setters and functions in a nice, codified way... what I want, however, is for an add-on to be able to _add_ something to a list (a handler for a particular type of nsIContentPermissionRequest), without disturbing the rest of the handlers that might be in the list. How would you suggest we do that with Integration.jsm? If we override a "list of handlers" getter, making the overriders remember to return the list including the base's original list seems like a potential foot-gun. Is there a better pattern that we could use instead with Integration.jsm? Or would it be simpler to add jsval handler registration and unregistration methods to nsIContentPermissionPrompt?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I think this following part is where I'm mostly confused:

> I'd normally recommend to move most of the code in nsBrowserGlue.js to
> its own module and make it the integration point, but as you noted
> we're actively working on that code and also doing uplifts, so you can
> add a separate ad-hoc integration point.

What do you mean when you say "ad-hoc integration point"? Can you give me an example of what you're talking about?

Comment 3

[:Paolo Amadini]

Here is a simple example. Define this somewhere in nsBrowserGlue.js:

  const PermissionIntegration = {
    prompt() {},
  };

Then add this at the beginning of the existing "prompt" function:

  let combined = Integration.permissions.getCombined(PermissionIntegration);
  combined.prompt(request);

Done. Here is what the system add-on could do to register:

  Integration.permissions.register(base => ({
    __proto__: base,
    prompt(request) {
      if (weCanHandle(request)) {
        // Prompt!
      }
      return super.prompt(request);
    },
  }));

The advantage over calling a method to register a handler is that the module you're integrating with doesn't need to be loaded when you do the registration. Since today the code is in nsBrowserGlue.js, which is loaded at startup anyways, this doesn't make a difference, but when we move it out of it, your system add-on won't impact startup performance.

This is "ad-hoc" in the sense that it solves just the problem at hand. Ideally the default implementation would be inside the PermissionsIntegration object, but this possibility is still left open, as the registration code on the add-on side would not change.

You can obviously do things like adding a boolean return value to suppress the default behavior, or you can avoid calling the base class, but the default implementation already does nothing if the request type isn't one it can handle.

Obviously as you've pointed out you need to remember to call the base class, but we're talking system add-ons here, and it's easy to be well behaved :-)

Comment 4

[:Paolo Amadini]

By the way, you can unregister integration points as well, mainly useful for restartless add-ons.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Thanks paolo, I'll give that a shot!

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Okay, having talked with paolo about this today, I think we've got an approach.

1. I'm going to move the majority of the ContentPermissionPrompt code into a browser/modules JSM - likely called ContentPermissionPrompt.jsm or ContentPermissionIntegration.jsm. There'll be a ContentPermissionIntegration (we'll call it that for now) singleton object in that JSM which overridable methods (via Integration.jsm).

We'll have a stub nsIContentPermissionPrompt implementation in nsBrowserGlue that will do something like the following:

Integration.contentPermission.defineModuleGetter(this, “ContentPermissionIntegration",
            "resource:///modules/ContentPermissionIntegration.jsm");
// ...
prompt: function(request) {
  let permissionRequest = ContentPermissionIntegration.createPermissionRequest(request);
  ContentPermissionIntegration.prompt(permissionRequest);
}

createPermissionRequest will take an nsIContentPermissionRequest and construct a well-defined wrapper around it with some convenience methods and accessors (for example, it should have a browser getter for the browser that made the request - this way we can get rid of _getBrowserForRequest in the ContentPermissionIntegration singleton.

createPermissionRequest is what a System Add-on can override. In the FlyWeb case, it will override it and check to see if the request is a FlyWeb request. If not, it'll forward it along to the base class. If so, it'll construct the PermissionRequest (which we'll expose a general implementation of in ContentPermissionIntegration.jsm), and return it.

ContentPermissionIntegration.prompt will do the work of making sure we need to prompt (based on whether or not permissions should even be asked), do show the notification, and allow / deny as appropriate.

The interface for a PermissionRequest is what most System Add-ons will need to know how to provide - and they don't need to use the provided general implementation if they have some special things they need to do.

For example, in bug 1289974, there's no nsIContentPermissionRequest. It's a nsIPresentationDeviceRequest instead. There's no reason why the System Add-on in that bug can't wrap the nsIPresentationDeviceRequest such that it provides the appropriate PermissionRequest interface, and call directly into ContentPermissionIntegration.prompt, and have the same PopupNotification mechanism work.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c20a39db71e

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

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

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

So this appears to do the job. I have a local patch that I can use, for example, to get the FlyWeb publish server permission prompt to show without requiring the System Add-on to leak into nsBrowserGlue.js.

There's still some documentation to fill out and some tests to write, but I think this is mostly done.

Hoping to get feedback from paolo on the Integration.jsm usage and general API design.

I'm flagging johannh for feedback chiefly because I know he's doing work in bug 1291642 in this area that is targeted to land on Elm only. There are going to be merge conflicts to deal with if he lands his changes in that bug before this stuff lands. Luckily, he very wisely broke up his patch into several commits! Either we deal with the merge conflicts when they happen (I can help there), or the work in bug 1291642 will need to be rebased on this patch once it lands.

johann, assuming this patch is going to land, do you think the work in bug 1291642 can be adapted to these changes? And how would you like to resolve the conflict - wait for this to land first, or deal with it at merge time?

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

Hey Mike, it's great that you're refactoring this into its own module.

We actually decided to not make any incompatible changes in bug 1291642 and focus on extending the PopupNotifications API, which will also enable us to land in central directly. The new bug for the changes that would be incompatible with this patch is bug 1282768. There we'll transition the permission doorhangers to use two buttons (allow/deny) and a checkbox instead of a dropdown menu. I renamed the bugs for clarity.

I think for bug 1282768 it won't really matter if we're rebasing from your changes or from the old version and I'd personally prefer this patch to land first.

If you check out https://reviewboard.mozilla.org/r/74112/diff/1#0 you can see that the new "binary" decision model gets rid of a lot of code. So don't get hung up on details around the actions stuff, much of this is going to change and hopefully get simpler anyway.

Thanks!

Comment 11

[:Paolo Amadini]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #9)
> Hoping to get feedback from paolo on the Integration.jsm usage and general
> API design.

Great start! I guess removing the code duplication is the next step.

We could modify createPermissionPrompt to match this pseudo-code:

switch (type) {
  case "geolocation":
    return new GeolocationPermissionPrompt(request);
  case "desktop-notification":
    return new DesktopNotificationPermissionPrompt(request);
  default:
    return new PermissionPrompt(request);
}

function PermissionPrompt(request) {
  this.request = request;
}

PermissionPrompt.prototype = {
  // Most of the code goes here...
};

function GeolocationPermissionPrompt(request) {
  this.request = request;
}

GeolocationPermissionPrompt.prototype = {
  __proto__: PermissionPrompt,
  // ...overrides only...
}

Overrides of createPermissionPrompt can do this:

let permissionPrompt = super.createPermissionPrompt(...);
if (type != "mypermission") {
  return permissionPrompt;
}

return {
  __proto__: permissionPrompt,
  // ...overrides only...
};

We can even make prompt() a method of PermissionPrompt. So if you need a standard notification and only need to change things like the message of the prompt, you only have to override the associated property which we would add to the object, and not the "prompt" method itself.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

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

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1297475 - Tests for PermissionUI.jsm.

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

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

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

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

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

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

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

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786117 [details]
Bug 1297475 - Tests for PermissionUI.jsm.

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

Comment 18

[:Paolo Amadini]

There's a pitfall to having the integration module also imported as an ordinary module somewhere else, that is if you inadvertently copy and paste the code that imports the module traditionally but then access the Integration object it exposes, you'll get an object without any integration. And you may need to use two different import paths for the same module if for some reason you need to do both in the same calling code.

That's why I suggested exposing helper objects in a different module. However, a single module may also be better for performance, even though both modules are lazily loaded. In the end I don't have a strong preference.

Comment 19

[:Paolo Amadini]

By the way, the patch looks great! The review just needs some time given we're moving code around and I need to check we still handle all the cases of the old code.

Comment 20

[:Paolo Amadini]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

https://reviewboard.mozilla.org/r/74790/#review73962

Thanks so much Mike for coding this. I like the direction this is going and I have a few more thoughts that I hope can make this even easier and clearer.

I think we can address these major thoughts before the rest of the review, although a few nits creeped in already :-)

::: browser/components/nsBrowserGlue.js:2494
(Diff revision 3)
> +   * will be thrown.
> +   *
> +   * Any time an error is thrown, the nsIContentPermissionRequest is
> +   * cancelled automatically.
>     *
> -   * @param aRequest               The permission request.
> +   * @param request (nsIContentPermissionRequest)

Seems like the "@param {nsIContentPermissionRequest} request" convention is more common.

::: browser/modules/ContentPermissionIntegration.jsm:43
(Diff revision 3)
> + * for a new request for getting more low-level access to the user's
> + * sound card. The system add-on could then do the following:
> + *
> + * Cu.import("resource://gre/modules/Integration.jsm");
> + * const { PermissionPromptPrototype } =
> + *   Cu.import("resource:///modules/ContentPermissionIntegration");

This way of using Cu.import is not recommended. In this case, it would leak the exported objects to this context, including the base integration object that shouldn't ever be called directly. You need to specify the target object as an argument to Cu.import.

::: browser/modules/ContentPermissionIntegration.jsm:48
(Diff revision 3)
> + *   Cu.import("resource:///modules/ContentPermissionIntegration");
> + *
> + * const SoundCardIntegration = (base) => ({
> + *   createPermissionPrompt(type, request) {
> + *     if (type != "sound-api") {
> + *       return base.createPermissionPrompt(type, request);

Should be "base.createPermissionPrompt.call(this, type, request)", or you could use "super" + "__proto__" + maybe "...arguments" like in the Integration.jsm examples and test cases.

::: browser/modules/ContentPermissionIntegration.jsm:68
(Diff revision 3)
> + * It is also possible to take advantage of PermissionPrompt without
> + * having to go through nsIContentPermissionPrompt or with a
> + * nsIContentPermissionRequest. The PermissionPromptPrototype can be
> + * imported, subclassed, and called directly, without the caller
> + * having called createPermissionPrompt.

The fact it's independent from the integration is another reason why the implementation of PermissionPrompt might be in a different module.

If you're concerned about having two modules, I'd say that maybe you don't need the integration JSM, and you can move the now very small base implementation to a "const ContentPermissionIntegration =" in nsBrowserGlue.js.

For how to expose PermissionPrompt, GeolocationPermissionPrompt, and so on, I'd recommend a module with multiple exports similar to this one:

https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/DownloadsViewUI.jsm

Cu.import("resource:///modules/PermissionUI.jsm");

let prompt = new PermissionUI.GeolocationPermissionPrompt();

let MyPrompt = {
  __proto__: PermissionUI.PermissionPrompt.prototype,
  // ...overrides...  
};

That said, having createPermissionPrompt() as the only way to construct the base object is also a way to encourage all callers to go through the integration instead of creating the object directly through a code path that may ignore the overrides. This might not be a serious enough concern in practice though, and it might be outweighted by the flexibility of using different base classes like I suggest below.

::: browser/modules/ContentPermissionIntegration.jsm:137
(Diff revision 3)
> +  /**
> +   * Returns the nsIPrincipal associated with the request.
> +   *
> +   * This assumes that this.request is an nsIContentPermissionRequest.
> +   */
> +  get principal() {
> +    return this.request.principal;
> +  },

We can simply put the methods that rely on having an nsIContentPermissionRequest close to each other, so it's clear what you need to override.

But we can also have a PermissionPrompt abstract class that throws new Error("Not implemented.") for these methods. This is the class that implements the "prompt" method and the one from which non-nsIPermissionRequest based permissions will inherit.

We can then have a concrete derived class from which the other specific internal permission prompts will inherit. It could be named PermissionPromptForNSIPermissionRequest but we can maybe find a better name :-)

::: browser/modules/ContentPermissionIntegration.jsm:277
(Diff revision 3)
> +      let result =
> +        Services.perms.testExactPermissionFromPrincipal(this.principal,
> +                                                        this.permissionKey);

Is it safe to assume that the system add-ons will still use the permission manager to store the result of the permission request, even if it does not use nsIPermissionRequest as a way to request the permission?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

https://reviewboard.mozilla.org/r/74790/#review73962

> The fact it's independent from the integration is another reason why the implementation of PermissionPrompt might be in a different module.
> 
> If you're concerned about having two modules, I'd say that maybe you don't need the integration JSM, and you can move the now very small base implementation to a "const ContentPermissionIntegration =" in nsBrowserGlue.js.
> 
> For how to expose PermissionPrompt, GeolocationPermissionPrompt, and so on, I'd recommend a module with multiple exports similar to this one:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/DownloadsViewUI.jsm
> 
> Cu.import("resource:///modules/PermissionUI.jsm");
> 
> let prompt = new PermissionUI.GeolocationPermissionPrompt();
> 
> let MyPrompt = {
>   __proto__: PermissionUI.PermissionPrompt.prototype,
>   // ...overrides...  
> };
> 
> That said, having createPermissionPrompt() as the only way to construct the base object is also a way to encourage all callers to go through the integration instead of creating the object directly through a code path that may ignore the overrides. This might not be a serious enough concern in practice though, and it might be outweighted by the flexibility of using different base classes like I suggest below.

Alright - I've moved the integration into nsBrowserGlue.js, and renamed ContentPermissionIntegration.jsm to PermissionUI.jsm, which exposes a single object (whose properties are the prototypes and built-ins).

> We can simply put the methods that rely on having an nsIContentPermissionRequest close to each other, so it's clear what you need to override.
> 
> But we can also have a PermissionPrompt abstract class that throws new Error("Not implemented.") for these methods. This is the class that implements the "prompt" method and the one from which non-nsIPermissionRequest based permissions will inherit.
> 
> We can then have a concrete derived class from which the other specific internal permission prompts will inherit. It could be named PermissionPromptForNSIPermissionRequest but we can maybe find a better name :-)

Went with the latter plan by introducing a subclass of PermissionPromptPrototype that provides the nsIContentPermissionRequest utilities.

> Is it safe to assume that the system add-ons will still use the permission manager to store the result of the permission request, even if it does not use nsIPermissionRequest as a way to request the permission?

For now, I'm assuming that if the class provides the permissionKey, then it's going to use the nsIPermissionManager, and should be exposing a principal. Acceptable?

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1297475 - Have FlyWeb publish server permission prompt use ContentPermissionIntegration.

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

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74790/diff/3-4/

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

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

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786117 [details]
Bug 1297475 - Tests for PermissionUI.jsm.

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

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

https://reviewboard.mozilla.org/r/74790/#review77662

::: browser/modules/PermissionUI.jsm:11
(Diff revision 4)
> +/**
> + * PermissionUI is responsible for exposing both a prototype
> + * PermissionPrompt that can be used by arbitrary browser
> + * components an add-ons, but also hosts the implementations of
> + * built-in permission prompts.
> + *
> + * If you're developing a feature that requires web content to ask
> + * for special permissions from the user, this module is for you.
> + *
> + * Suppose a system add-on wants to add a new prompt for a new request
> + * for getting more low-level access to the user's sound card. The
> + * system add-on could then do the following:
> + *
> + * Cu.import("resource://gre/modules/Integration.jsm");
> + * Cu.import("resource:///modules/PermissionUI.jsm");
> + *
> + * const SoundCardIntegration = (base) => ({
> + *   __proto__: base,
> + *   createPermissionPrompt(type, request) {

This documentation should refer to PermissionPromptForRequestPrototype.

::: browser/modules/PermissionUI.jsm:54
(Diff revision 4)
> + * It is also possible to take advantage of PermissionPrompt without
> + * having to go through nsIContentPermissionPrompt or with a
> + * nsIContentPermissionRequest. The PermissionPromptPrototype can be
> + * imported, subclassed, and have prompt() called directly, without
> + * the caller having called into createPermissionPrompt.

This documentation should refer to PermissionPromptPrototype.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

https://reviewboard.mozilla.org/r/75072/#review77664

::: browser/components/tests/browser/browser_contentpermissionprompt.js:19
(Diff revision 3)
> +                                   "nsIContentPermissionPrompt");
> +
> +/**
> + * This is a partial implementation of nsIContentPermissionType.
> + *
> + * @param type (string)

This, and throughout the rest of these patches, should use:

```
@param {<type>} <var name>
```

formatting.

Comment 28

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8791774 [details]
Bug 1297475 - Have FlyWeb publish server permission prompt use ContentPermissionIntegration.

https://reviewboard.mozilla.org/r/79062/#review78152

LGTM. See my inline comments for some questions I had. Thanks :-)

::: browser/extensions/flyweb/bootstrap.js:128
(Diff revision 1)
> +    if (type != "flyweb-publish-server") {
> +      return base.createPermissionPrompt(type, request);
> +    }
> +
> +    return {
> +      __proto__: PermissionUI.PermissionPromptPrototype,

Is this the typical pattern for inheriting a prototype in add-on code? Couldn't we use `Object.create(PermissionUI.PermissionPromptPrototype)`?

::: browser/extensions/flyweb/bootstrap.js:137
(Diff revision 1)
> +      get permissionKey() {
> +        return "flyweb-publish-server";
> +      },
> +      get popupOptions() {
> +        return {
> +          learnMoreURL: "https://flyweb.github.io",

I probably should create a page somewhere for this specific purpose. This is probably fine for now though.

::: browser/locales/en-US/chrome/browser/browser.properties
(Diff revision 1)
>  geolocation.neverShareLocation=Never Share Location
>  geolocation.neverShareLocation.accesskey=N
>  geolocation.shareWithSite2=Would you like to share your location with this site?
>  geolocation.shareWithFile2=Would you like to share your location with this file?
>  
> -# FlyWeb UI

Why aren't we using these strings anymore?

Comment 29

[:Paolo Amadini]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

https://reviewboard.mozilla.org/r/74790/#review78462

r+ with the changes below, feel free to re-request review or needinfo if you have any questions.

I've looked at the code and it seems to me you've ported every functionality, I also hope the current regression tests would catch anything we might have missed.

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #21)
> For now, I'm assuming that if the class provides the permissionKey, then
> it's going to use the nsIPermissionManager, and should be exposing a
> principal. Acceptable?

Sounds good to me.

::: browser/components/nsBrowserGlue.js:2448
(Diff revision 4)
> -   * @param aAnchorId              The id for the PopupNotification anchor.
> -   * @param aOptions               Options for the PopupNotification
> -   */
> + */
> -  _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aActions,
> -                                       aNotificationId, aAnchorId, aOptions) {
> -    var browser = this._getBrowserForRequest(aRequest);
> +const ContentPermissionIntegration = {
> +  /**
> +   * Creates a PermissionPrompt for a given nsIContentPermissionType

"given nsIContentPermissionType" => "given permission type"

::: browser/components/nsBrowserGlue.js:2452
(Diff revision 4)
> -                                       aNotificationId, aAnchorId, aOptions) {
> -    var browser = this._getBrowserForRequest(aRequest);
> -    var chromeWin = browser.ownerGlobal;
> -    var requestPrincipal = aRequest.principal;
> -
> -    // Transform the prompt actions into PopupNotification actions.
> +  /**
> +   * Creates a PermissionPrompt for a given nsIContentPermissionType
> +   * and nsIContentPermissionRequest.
> +   *
> +   * @param {string} type
> +   *        The type of the permission request from content.

You can say here that it normally matches the "type" field of nsIContentPermissionType, but it can be something else if the permission does not use the nsIPermissionRequest model.

Specify that this can be different from the permission key in the database, for example "geo" and "geolocation".

::: browser/components/nsBrowserGlue.js:2456
(Diff revision 4)
> -
> -    // Transform the prompt actions into PopupNotification actions.
> -    var popupNotificationActions = [];
> -    for (var i = 0; i < aActions.length; i++) {
> -      let promptAction = aActions[i];
> -
> +   * @param {string} type
> +   *        The type of the permission request from content.
> +   *        Example: "geolocation"
> +   * @param {nsIContentPermissionRequest} request
> +   *        The request for a permission from content.
> +   * @returns {PermissionPrompt} (see PermissionUI.jsm),

nit: @return

::: browser/components/nsBrowserGlue.js:2494
(Diff revision 4)
> +   * cancelled automatically.
> +   *
> +   * @param {nsIContentPermissionRequest} request
> +   *        The request that we're to show a prompt for.
> +   */
>    prompt: function CPP_prompt(request) {

nit: update the syntax to "prompt(request)" since you're changing most of this code anyways.

::: browser/components/nsBrowserGlue.js:2507
(Diff revision 4)
> -                         };
> -
> -    // Make sure that we support the request.
> +    } catch (e) {
> +      request.cancel();
> +      throw e;
> -    if (!(perm.type in kFeatureKeys)) {
> -      return;
>      }

nit: ex instead of e

Maybe we should surround all the rest of this function with this try-catch?

A Cu.reportError(ex) before request.cancel() can also be useful because I think it reports some information that would be lost on the XPCOM boundary.

::: browser/modules/PermissionUI.jsm:14
(Diff revision 4)
> +];
> +
> +/**
> + * PermissionUI is responsible for exposing both a prototype
> + * PermissionPrompt that can be used by arbitrary browser
> + * components an add-ons, but also hosts the implementations of

and

::: browser/modules/PermissionUI.jsm:98
(Diff revision 4)
> +   * Subclasses must override this.
> +   *
> +   * @returns <xul:browser>
> +   */
> +  get browser() {
> +    throw new Error("get browser() not implemented");

nit: new Error() gives us a stack trace, so we don't need the function name in the message. To reduce cut-and-paste error potential, just write:

throw new Error("Not implemented.");

::: browser/modules/PermissionUI.jsm:105
(Diff revision 4)
> +
> +  /**
> +   * Returns the nsIPrincipal associated with the request.
> +   *
> +   * Subclasses must override this.
> +   *

Missing @return type

::: browser/modules/PermissionUI.jsm:134
(Diff revision 4)
> +   * If there's a popupnotification node that PopupNotification
> +   * should use, set its ID here, otherwise, PopupNotification
> +   * will generate one for you.

Is this what this property does? The documentation of PopupNotifications.jsm is unclear.

If this is the case, saying "custom <popupnotification> element in browser.xul" and "otherwise the default <popupnotification> binding will be used" might be clearer. Also point to PopupNotifications.jsm for details.

::: browser/modules/PermissionUI.jsm:153
(Diff revision 4)
> +
> +  /**
> +   * The message to show the user in the PopupNotification.
> +   */
> +  get message() {
> +    return "";

This doesn't look optional, maybe we should return something like "<unspecified>" or just throw?

We should probably give this property a more meaningful name, about which type of message this is.

::: browser/modules/PermissionUI.jsm:177
(Diff revision 4)
> +   * The actions that will be displayed in the PopupNotification
> +   * via a dropdown menu. The first item in this array will be
> +   * the default selection. Each action is an Object with the
> +   * following properties:

Looks good to me. I expect some API changes in this area due to the work we're doing on permissions, but this is fine for now.

::: browser/modules/PermissionUI.jsm:210
(Diff revision 4)
> +  onBeforeShow() {
> +  },

nit: "onBeforeShow() {}," on the same line.

::: browser/modules/PermissionUI.jsm:225
(Diff revision 4)
> +   * @returns Notification opened via the PopupNotification API,
> +   *          or undefined if the prompt was not opened.

I would just set the notification on a property of the object, or simply not store it because it seems unused anyways.

This way the content permission integration API is independent from which UI is used for the prompt.

::: browser/modules/PermissionUI.jsm:231
(Diff revision 4)
> +   *          or undefined if the prompt was not opened.
> +   */
> +  prompt() {
> +    let chromeWin = this.browser.ownerGlobal;
> +    if (!chromeWin.PopupNotifications) {
> +      return undefined;

I wonder if we want a console log message for these early return cases? There isn't one currently, so also the current behavior of ignoring silently is fine.

::: browser/modules/PermissionUI.jsm:241
(Diff revision 4)
> +      // If we're reading and setting permissions, then we need
> +      // to check to see if we already have a permission setting
> +      // for this particular principal.

Please document this early return behavior in the permissionKey property.

::: browser/modules/PermissionUI.jsm:325
(Diff revision 4)
> + * A subclass of PermissionPromptPrototype that assumes
> + * that this.request is an nsIContentPermissionRequest
> + * and fills in some of the required properties on the
> + * PermissionPrompt. For callers that are wrapping an
> + * nsIContentPermissionRequest, this should be subclassed
> + * rather than PermissionpromptPrototype.

nit: PermissionPromptPrototype

::: browser/modules/PermissionUI.jsm:377
(Diff revision 4)
> +    let learnMoreURL =
> +      Services.urlFormatter.formatURLPref("browser.geolocation.warning.infoURL");
> +    let popupOptions = { learnMoreURL };
> +
> +    return popupOptions;

Might simplify to a simple return and object initializer. Same for the other prompt.

::: browser/modules/PermissionUI.jsm:513
(Diff revision 4)
> +    let message =
> +      gBrowserBundle.GetStringFromName("webNotifications.receiveFromSite");
> +    return message;

Simplify?

::: browser/modules/PermissionUI.jsm:523
(Diff revision 4)
> +      promptActions = [
> +        {

nit: While moving this code, indent it consistently with the other cases.

::: browser/modules/PermissionUI.jsm:530
(Diff revision 4)
> +          label: gBrowserBundle.GetStringFromName("webNotifications.receiveForSession"),
> +          accessKey:
> +            gBrowserBundle.GetStringFromName("webNotifications.receiveForSession.accesskey"),
> +          action: Ci.nsIPermissionManager.ALLOW_ACTION,
> +          expireType: Ci.nsIPermissionManager.EXPIRE_SESSION,
> +          callback: function() {},

And the empty callback is probably not required?

::: browser/modules/PermissionUI.jsm:558
(Diff revision 4)
> +this.PermissionUI = {
> +  PermissionPromptPrototype,
> +  PermissionPromptForRequestPrototype,
> +  DesktopNotificationPermissionPrompt,
> +  GeolocationPermissionPrompt,
> +};

I prefer the model used by DownloadsViewUI.jsm, with "this.PermissionUI = {};" at the beginning, so you don't have to add an element to the list at the end when you add a new object.

Comment 30

[:Paolo Amadini]

Comment on attachment 8791774 [details]
Bug 1297475 - Have FlyWeb publish server permission prompt use ContentPermissionIntegration.

https://reviewboard.mozilla.org/r/79062/#review78470

::: browser/extensions/flyweb/bootstrap.js:124
(Diff revision 1)
>  }
>  
> +const FlyWebPermissionPromptIntegration = (base) => ({
> +  createPermissionPrompt(type, request) {
> +    if (type != "flyweb-publish-server") {
> +      return base.createPermissionPrompt(type, request);

You need to use "__proto__: base" and "super" here so that the "this" reference is propagated properly, just like in the usage example in the documentation.

(In reply to Justin D'Arcangelo [:justindarc] from comment #28)
> > +      __proto__: PermissionUI.PermissionPromptPrototype,
> 
> Is this the typical pattern for inheriting a prototype in add-on code?
> Couldn't we use `Object.create(PermissionUI.PermissionPromptPrototype)`?

This is the correct way, using an object intitalizer. Object.create instead takes descriptors that are difficult to read and set up correctly.

Comment 31

[:Paolo Amadini]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

https://reviewboard.mozilla.org/r/75072/#review78474

I haven't run the tests myself, but they look good assuming they pass :-)

::: browser/components/tests/browser/browser_contentpermissionprompt.js:75
(Diff revision 3)
> + */
> +add_task(function* test_empty_types() {
> +  let mockRequest = new MockContentPermissionRequest([]);
> +  Assert.throws(() => { ContentPermissionPrompt.prompt(mockRequest); },
> +                /NS_ERROR_UNEXPECTED/,
> +                "Should have thrown NS_ERROR_UNEXPECTED.");

nit: You can omit assertion descriptions that are obvious.

::: browser/components/tests/browser/browser_contentpermissionprompt.js:159
(Diff revision 3)
> +  Integration.contentPermission.register(integration);
> +  registerCleanupFunction(() => {
> +    Integration.contentPermission.unregister(integration);
> +  });

A try-finally is better here. registerCleanupFunction operates at the end of all tests, not just this test case.

Comment 32

[:Paolo Amadini]

Comment on attachment 8786117 [details]
Bug 1297475 - Tests for PermissionUI.jsm.

https://reviewboard.mozilla.org/r/75074/#review78478

::: browser/modules/test/browser_PermissionUI.js:53
(Diff revision 3)
> +
> +/**
> + * Tests the PermissionPromptForRequest prototype to ensure that a prompt
> + * can be displayed. Does not test permission handling.
> + */
> +add_task(function*() {

Add a test function name.

::: browser/modules/test/browser_PermissionUI.js:56
(Diff revision 3)
> + * can be displayed. Does not test permission handling.
> + */
> +add_task(function*() {
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: "http://example.com",

nit: "http://example.com/"

::: browser/modules/test/browser_PermissionUI.js:79
(Diff revision 3)
> +      promptActions: [mainAction, secondaryAction],
> +    };
> +
> +    let shownPromise =
> +      BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshown");
> +    let notification = TestPrompt.prompt();

I see you're using the return value of prompt() for tests. Is there another way to get the most recently displayed notification object?

::: browser/modules/test/browser_PermissionUI.js:171
(Diff revision 3)
> +                 "Should be no permission set to begin with.");
> +
> +    // First test denying the permission request.
> +    Assert.equal(notification.secondaryActions.length, 1,
> +                 "There should only be 1 secondary action");
> +    notification.secondaryActions[0].callback();

I wonder if we should use an actual UI interaction? What do other permission tests do?

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

https://reviewboard.mozilla.org/r/75072/#review78474

> nit: You can omit assertion descriptions that are obvious.

Good call. :)

> A try-finally is better here. registerCleanupFunction operates at the end of all tests, not just this test case.

Ah, I always forget that! Thanks!

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786117 [details]
Bug 1297475 - Tests for PermissionUI.jsm.

https://reviewboard.mozilla.org/r/75074/#review78478

> I see you're using the return value of prompt() for tests. Is there another way to get the most recently displayed notification object?

Yeah, I can find an alternative.

> I wonder if we should use an actual UI interaction? What do other permission tests do?

Looks like there are [a few that trigger mouse events](http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/browser/base/content/test/popupNotifications/head.js#261). I'll try that too.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8791774 [details]
Bug 1297475 - Have FlyWeb publish server permission prompt use ContentPermissionIntegration.

https://reviewboard.mozilla.org/r/79062/#review78152

> Is this the typical pattern for inheriting a prototype in add-on code? Couldn't we use `Object.create(PermissionUI.PermissionPromptPrototype)`?

See paolo's response below.

> Why aren't we using these strings anymore?

They were originally added because the permission prompting code that was originally in nsBrowserGlue.js (which I tear out in the first commit in this series) required string keys instead of the strings themselves. That restricted us by requiring us to always have our strings for permissions inside browser.properties.

With the changes I landed, we can pass strings directly, so we don't need these anymore.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74790/diff/4-5/

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75072/diff/3-4/

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786117 [details]
Bug 1297475 - Tests for PermissionUI.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75074/diff/3-4/

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8791774 [details]
Bug 1297475 - Have FlyWeb publish server permission prompt use ContentPermissionIntegration.

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

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

https://reviewboard.mozilla.org/r/74790/#review78462

> Is this what this property does? The documentation of PopupNotifications.jsm is unclear.
> 
> If this is the case, saying "custom <popupnotification> element in browser.xul" and "otherwise the default <popupnotification> binding will be used" might be clearer. Also point to PopupNotifications.jsm for details.

Not sure if the mention of "binding" muddies the water here. What this comment is referring to [is this](http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/toolkit/modules/PopupNotifications.jsm#658) - PopupNotifications.jsm will generate a <xul:popupnotification> unless you've provided the ID for one that's already in the document.

> Might simplify to a simple return and object initializer. Same for the other prompt.

I've made these getters lazy and memoized. Is that what you meant?

Comment 41

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #40)
> PopupNotifications.jsm will generate a <xul:popupnotification>
> unless you've provided the ID for one that's already in the document.

And the code actually uses n.id + "-notification" to find the element. This is undocumented. I think we can just document this in our code, using the clearest wording we can find :-)

> > Might simplify to a simple return and object initializer. Same for the other prompt.
> 
> I've made these getters lazy and memoized. Is that what you meant?

I was thinking simply of:

return {
  learnMoreURL: Services.urlFormatter
                        .formatURLPref("browser.geolocation.warning.infoURL"),
};

But memoized is also good :-)

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785645 [details]
Bug 1297475 - Move content permission prompts into a JSM and add an Integration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74790/diff/5-6/

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786116 [details]
Bug 1297475 - Add tests for /browser's implementation of nsIContentPermissionPrompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75072/diff/4-5/

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786117 [details]
Bug 1297475 - Tests for PermissionUI.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75074/diff/4-5/

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8791774 [details]
Bug 1297475 - Have FlyWeb publish server permission prompt use ContentPermissionIntegration.

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

Comment 46

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Paolo Amadini from comment #41)
> (In reply to Mike Conley (:mconley) - (needinfo me!) from comment #40)
> > PopupNotifications.jsm will generate a <xul:popupnotification>
> > unless you've provided the ID for one that's already in the document.
> 
> And the code actually uses n.id + "-notification" to find the element. This
> is undocumented. I think we can just document this in our code, using the
> clearest wording we can find :-)
> 

Ah, you're right! I'd not noticed the -notification suffix complication. I've updated the documentation - thanks.

> > > Might simplify to a simple return and object initializer. Same for the other prompt.
> > 
> > I've made these getters lazy and memoized. Is that what you meant?
> 
> I was thinking simply of:
> 
> return {
>   learnMoreURL: Services.urlFormatter
>                        
> .formatURLPref("browser.geolocation.warning.infoURL"),
> };
> 
> But memoized is also good :-)

Turns out memoized is hard because straight-up deleting getters on instances from the prototype isn't allowed. I've reverted the memoization, and made the above change.

Note that I didn't change the other prompt because there's some string concatenation in there that I thought would be cleaner to keep outside of the object initializer. Feel free to push back!

Thanks for all of your help and guidance with this, Paolo. How do you feel about these patches now? Do you think I'm okay to land?

Comment 47

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #46)
> Thanks for all of your help and guidance with this, Paolo. How do you feel
> about these patches now? Do you think I'm okay to land?

Fabulous! Sure, go ahead and land!

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0db5e128e753
Move content permission prompts into a JSM and add an Integration. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/032477ef3c8f
Add tests for /browser's implementation of nsIContentPermissionPrompt. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/5448cbebb76a
Tests for PermissionUI.jsm. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/3464b86eb1cf
Have FlyWeb publish server permission prompt use ContentPermissionIntegration. r=justindarc

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0db5e128e753
https://hg.mozilla.org/mozilla-central/rev/032477ef3c8f
https://hg.mozilla.org/mozilla-central/rev/5448cbebb76a
https://hg.mozilla.org/mozilla-central/rev/3464b86eb1cf

Comment 50

[Andrei Vaida [:avaida]]

Mike, is this something that manual QA could help test? Adding the feature keyword for tracking.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #50)
> Mike, is this something that manual QA could help test? Adding the feature
> keyword for tracking.

The things that could be manually tested that this thing touched are:

1. The Desktop Notifications permission prompt
2. The Geolocation permission prompt
3. The FlyWeb server permission prompt (dom.flyweb.enabled will need to be set to true, and then you can visit http://flyweb.github.io/ to see the prompt)

Thanks!

Comment 52

[Andrei Vaida [:avaida]]

Thank you for following up on this, Mike! I think this could be treated as bug work instead of a full blown feature, meaning that there won't be sign offs from my team while it rides the trains, but in depth regression testing will be performed on this bug (and any eligible dependencies) as it's made available on each channel.

Let me know if you have any concerns about this plan.

Comment 53

[Camelia Badau [:cbadau], Desktop Test Engineering]

Based on comment 51, I've tested permission prompt using several websites on Windows 7 x64, Ubuntu 14.04 x64 and Mac OSX 10.11.5 using latest Nightly 52.0a1 (buildID: 20161113030203). 

Please see here the testing report: https://public.etherpad-mozilla.org/p/bug1297475.

Comment 54

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've tested permission prompt using several websites on Windows 7 x64, Ubuntu 14.04 x64 and Mac OSX 10.11.5 using Aurora 52.0a2 (buildID: 20170103004005).

Please see here the testing report: https://public.etherpad-mozilla.org/p/bug1297475

Comment 55

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've tested permission prompt using several websites on Windows 7 x64, Ubuntu 14.04 x64 and Mac OSX 10.11.6 using Firefox 52 RC (buildID: 20170302120751).

I have one mention here: on http://flyweb.github.io/ , the prompt isn't displayed on Firefox 52 Beta 8 and Firefox 52 RC. 
The following error/exception is displayed in Browser Console: 
"[Exception... "Failed to handle permission of type flyweb-publish-server"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://app/components/nsBrowserGlue.js :: prompt :: line 2510"  data: no]
NS_ERROR_FAILURE: Failed to handle permission of type flyweb-publish-server" 

On latest Nightly 55.0a1 (2017-03-13), the prompt is correctly displayed. 

Do you know something about this issue? 

Please see here the testing report: https://public.etherpad-mozilla.org/p/bug1297475

Comment 56

[Justin D'Arcangelo [:justindarc]]

Kannan, see comment 55.

Comment 57

[Mike Conley (:mconley) (:⚙️)]

I suspect this is because the FlyWeb add-on is only packaged on Nightly and Aurora:

http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/browser/extensions/moz.build#15-18

Comment 58

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #57)
> I suspect this is because the FlyWeb add-on is only packaged on Nightly and
> Aurora:
> 
> http://searchfox.org/mozilla-central/rev/
> ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/browser/extensions/moz.build#15-18

To be clear, this is expected. FlyWeb should only work on Nightly and Aurora channels for now.

Comment 59

[Camelia Badau [:cbadau], Desktop Test Engineering]

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #58)
> (In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos)
> from comment #57)
> > I suspect this is because the FlyWeb add-on is only packaged on Nightly and
> > Aurora:
> > 
> > http://searchfox.org/mozilla-central/rev/
> > ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/browser/extensions/moz.build#15-18
> 
> To be clear, this is expected. FlyWeb should only work on Nightly and Aurora
> channels for now.

Thank you for your answer!