Closed Bug 1308309 Opened 8 years ago Closed 7 years ago

Display permissions prompt for webextensions installed using mozAddonManager

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Version:
51 Branch
Type:
defect

Tracking

(firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: [triaged][permissions])

Attachments

(4 files, 1 obsolete file)

mock
45.44 KB, image/png
Details
Bug 1308309 Prompt for webextensions permissions
58 bytes, text/x-review-board-request
rhelmer
: review+
florian
: review+
Details
long-name_edgecase.jpg
73.38 KB, image/jpeg
Details
webextensions@2x.jpg
440.40 KB, image/jpeg
Details 
This is the "confirm" step from the disco pane flows detailed at https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions
Attached image Screenshot 2016-10-06 10.11.07.png (obsolete) — 

    
  
Priority: -- → P2
Whiteboard: [triaged]

    
  
Depends on: 1004061

    
  
Whiteboard: [triaged] → [triaged][permissions]

    
  
Summary: Display permissions prompt for webextensions installed from the disco pane → Display permissions prompt for webextensions installed using mozAddonManager

    
    

    
  

      
      
      
      

        Attached image
          
        mock
        — 
      

    


  
Assignee: nobody → aswan

        Attachment #8798661 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

There are some unfinished bits here but hoping to get some feedback to make sure I'm on the right track so far with all the front-end bits.
First of all, we don't yet have the actual text we'll be using for permissions, that work is tracked in bug 1316996 so this just has placeholder text for now.
Next, since permissions prompts originate from within AddonManager but we're only displaying them in the desktop browser, we use an observer to communicate between those two components with a preference to indicate if that communication should occur.  We won't turn that preference on for desktop until the text and all the other blockers for bug 1308292 land.  But for testing for now, you can set the pref (extensions.webextPermissionPrompts) to true in about:config.
This patch only adds permission prompts to mozAddonManager, the easiest way to test it is to go to about:addons and install awesome screenshot (which is a webextension with a set of required permissions).
Beyond that, this needs automated tests of course but there's another problem that I'm not sure about, the styling for the <li> items in the popup doesn't seem to apply, and I can't figure out why.  Help on that issue in addition to overall feedback would be welcome...

        Attachment #8814635 -
        Flags: feedback?(jhofmann)

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review96174

This is some feedback just from a cursory look and a bit of trying out your patch. I mostly tried to be helpful with front-end things. I particularly ignored codestyle, variable names, module architecture etc.

I'd recommend having someone from the desktop team who's more experienced than me review this in the end. Paolo, MattN or Florian are probably great for that.

Overall the UI works great already, as far as I can tell from trying to install Awesome Screenshot :)

::: browser/base/content/browser.css:830
(Diff revision 1)
> +.addon-webext-perm-header {
> +  font-weight: bold;
> +  font-size: 14px;
> +}
> +
> +.addon-webext-perm-list > html|li {

I don't think this selector matches but I don't know how to match html:li elements by tagname via CSS either, so I'd recommend just assigning your element a className and matching by that. (Actually sounds cleaner)

This is also why the list is not properly displayed.

::: browser/modules/ExtensionsUI.jsm:61
(Diff revision 1)
> +
> +    try {
> +      let bundle = win.gBrowserBundle;
> +
> +      let header = bundle.GetStringFromName("webextPerms.header")
> +          .replace("#1", info.addon.name);

You shouldn't do the replace manually, you could use e.g. formatStringFromName instead.

Although I wonder if there would be an easy way to get the stringbundle (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/stringbundle), which is a bit more ergonomic to use.

::: browser/modules/ExtensionsUI.jsm:81
(Diff revision 1)
> +        }
> +        let match = /^[htps*]+:\/\/([^/]+)\//.exec(perm);
> +        if (!match) { /* XXX uh oh */ return ""; }
> +        if (match[1].startsWith("*.")) {
> +          return bundle.GetStringFromName("webextPerms.hostDescription.wildcard")
> +            .replace("#1", match[1].slice(2));

I guess there are probably multiple steps of sanitization before this text actually gets injected into the DOM, I just wanted to draw awareness to the fact that we're injecting arbirtrary input from the manifest here and we should at least do a sanity check and/or a test that this ends up properly escaped.

::: browser/modules/ExtensionsUI.jsm:88
(Diff revision 1)
> +
> +        return bundle.GetStringFromName("webextPerms.hostDescription.oneSite")
> +          .replace("#1", match[1]);
> +      };
> +
> +      let perms = info.addon.userPermissions;

This is currently null for non-webext addons, preventing me to install them because it causes a TypeError in the line below.

::: browser/modules/ExtensionsUI.jsm:119
(Diff revision 1)
> +            }
> +
> +            for (let msg of msgs) {
> +              let item = win.document.createElement("html:li");
> +              list.appendChild(item);
> +              let label = win.document.createElement("label");

Why do you need the label here? You could just set item.textContent instead. Also I think a label should only be used if there's a corresponding control element it refers to.

::: browser/modules/ExtensionsUI.jsm:146
(Diff revision 1)
> +                      },
> +                    ], popupOpts);
> +      });
> +    } catch (err) {
> +      Cu.reportError(err);
> +      return Promise.reject();

You might want to reject with the error description.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5829
(Diff revision 1)
> +    if (!icon) {
> +      return null;
> +    }
> +
> +    return buildJarURI(this.file, icon).spec;
> +  }

There are already two functions that can do this. (http://searchfox.org/mozilla-central/search?q=getpreferredicon&case=false&regexp=false&path=)

Not sure if you need another one :)

        Attachment #8814635 -
        Flags: feedback?(jhofmann) → feedback+

    
    

    
      
  

    
  

        Attachment #8814635 -
        Flags: review?(rhelmer)

        Attachment #8814635 -
        Flags: review?(MattN+bmo)

    
    

    
  
Rob can you look at the bits under toolkit/mozapps/extensions and Matt all the rest?
Also see comment 4 for some useful context (minus the bit about list styling)

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review97306

Addon manager bits lgtm.

I glanced over the rest and looks reasonable but will leave that to Matt.

        Attachment #8814635 -
        Flags: review?(rhelmer) → review+

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review98962

::: browser/base/content/browser.css:826
(Diff revision 2)
> +  font-weight: bold;
> +  font-size: 14px;

All of these style seems more relevant for a skin stylesheet rather than a content one but I'm not seeing an existing shared one in browser/ yet. We only have browser/themes/shared/notification-icons.inc.css for the icons. Can you either move the font-size and list-style-type change (maybe font-weight too?) to the browser.css for each desktop skin or create a new file in browser/themes/shared/ such as permission-prompts.inc.css or if you think you will add a lot more for webextensions then you can make webextensions-doorhanger.inc.css (like we have with plugin-doorhanger.inc.css[1]

[1] https://dxr.mozilla.org/mozilla-central/search?q=plugin-doorhanger.inc.css

::: browser/base/content/browser.css:827
(Diff revision 2)
>    -moz-binding: url("chrome://browser/content/urlbarBindings.xml#addon-progress-notification");
>  }
>  
> +.addon-webext-perm-header {
> +  font-weight: bold;
> +  font-size: 14px;

This should probably be in "em" so that it grows with the users system font changes.

::: browser/base/content/popup-notifications.inc:75
(Diff revision 2)
>  
>      <popupnotification id="addon-install-confirmation-notification" hidden="true">
>        <popupnotificationcontent id="addon-install-confirmation-content" orient="vertical"/>
>      </popupnotification>
> +
> +    <popupnotification id="addon-webext-permissions-notification" hidden="true">

I would recommend adding to https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm#80-87 to get automated screenshots of the dialog and eventually change alerts.

::: browser/base/content/test/general/browser_extension_permissions.js:1
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +

Nit: no longer necessary since tests are PD by default.

::: browser/base/content/test/general/browser_extension_permissions.js:7
(Diff revision 2)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +const BASE = "https://example.com/browser/browser/base/content/test/general";
> +const PAGE = `${BASE}/file_install_extensions.html`;
> +const XPI_URL = `${BASE}/browser_webext_permissions.xpi`;

I don't know whether it's common to land .xpi files rather than have the build system build them. I think for the add-on SDK they get built which makes the contents easily readable/searchable.

::: browser/base/content/test/general/browser_extension_permissions.js:31
(Diff revision 2)
> +    AddonManager.getAddonByID(id, resolve);
> +  });
> +}
> +
> +function checkNotification(panel) {
> +  let uls = panel.firstChild.getElementsByTagName("html:ul");

You want `getElementsByTagNameNS`

::: browser/locales/en-US/chrome/browser/browser.properties:40
(Diff revision 2)
> +# These are to be filled in via bug 1316996
> +webextPerms.description.activeTab=Description of the activeTab permission

Note that you shouldn't land these in m-c with placeholder strings unless you coordinate with l10n since localizers may start to localize the placeholder text.

::: browser/modules/ExtensionsUI.jsm:24
(Diff revision 2)
> +    Services.obs.addObserver(this, "webextension-permission-prompt", false);
> +  },
> +
> +  observe(subject, topic, data) {
> +    if (topic == "webextension-permission-prompt") {
> +      let {target, info} = subject.wrappedJSObject;

I don't think this is going to work across process boundaries when this ends up getting called for installs from content pages. Well I guess the observer notification also wouldn't be received from the content process IIRC so this could work if the notification was sent from the parent process.

Really `_getWin` and sending a target are things you would get for free with a message from a message manager. Why aren't you using a message instead of an observer notification? message.target would give you the <browser>.

::: browser/modules/ExtensionsUI.jsm:77
(Diff revision 2)
> +
> +    let formatHostPermission = perm => {
> +      if (perm == "<all_urls>") {
> +        return bundle.getString("webextPerms.hostDescription.allUrls");
> +      }
> +      let match = /^[htps*]+:\/\/([^/]+)\//.exec(perm);

Any reason not to use `new URL()`? See `Cu.importGlobalProperties(["URL"]);`

::: browser/modules/ExtensionsUI.jsm:103
(Diff revision 2)
> +    let popupOpts = {
> +      hideClose: true,
> +      popupIconURL: info.icon,
> +      persistent: true,
> +      eventCallback(topic) {
> +        if (topic == "showing" && !rendered) {

An early return for `rendered` would be preferred. I still need to look into why `rendered` is even necessary as there is no comment describing this quirk.

::: browser/modules/ExtensionsUI.jsm:106
(Diff revision 2)
> +            .setAttribute("value", header);
> +          win.document
> +            .getElementById("addon-webext-perm-text")
> +            .setAttribute("value", listHeader);

Nit: Using a property setter should always be preferred over an attribute (when possible… there are quirks with XBL bindings) as it's more performant and  can do extra sanitization.

::: browser/modules/ExtensionsUI.jsm:117
(Diff revision 2)
> +          while (list.firstChild != null) {
> +            list.removeChild(list.firstChild);
> +          }
> +
> +          for (let msg of msgs) {
> +            let item = win.document.createElement("html:li");

The reason the selector wasn't working is because I don't think you can pass namespaces to createElement. See createElementNS.

::: browser/modules/ExtensionsUI.jsm:118
(Diff revision 2)
> +            list.removeChild(list.firstChild);
> +          }
> +
> +          for (let msg of msgs) {
> +            let item = win.document.createElement("html:li");
> +            item.setAttribute("class", "addon-webext-perm-item");

Nit: .className

::: browser/modules/ExtensionsUI.jsm:128
(Diff revision 2)
> +        }
> +      },
> +    };
> +
> +    return new Promise(resolve => {
> +      win.PopupNotifications.show(target, "addon-webext-permissions", "",

Why are you passing `""` as the message instead of part of the message? I'm thinking about it affect accessiibility.

::: browser/modules/ExtensionsUI.jsm:132
(Diff revision 2)
> +    return new Promise(resolve => {
> +      win.PopupNotifications.show(target, "addon-webext-permissions", "",
> +                                  "addons-notification-icon",
> +                                  {
> +                                    label: acceptText,
> +                                    accessKey: "A",

This has to be localized too. "A" may not be part of the label in other languages.

::: browser/modules/ExtensionsUI.jsm:137
(Diff revision 2)
> +                                      label: cancelText,
> +                                      accessKey: "C",

Ditto

        Attachment #8814635 -
        Flags: review?(MattN+bmo)

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review98962

> I don't know whether it's common to land .xpi files rather than have the build system build them. I think for the add-on SDK they get built which makes the contents easily readable/searchable.

We can do that for xpcshell tests, but for mochitests that are going to eventually run on beta/release, the xpi needs to be signed.  We have bug 1294235 to check in the source and signatures and bundle them at build time, but for the time being this is how these sorts of tests are typically written.

> I don't think this is going to work across process boundaries when this ends up getting called for installs from content pages. Well I guess the observer notification also wouldn't be received from the content process IIRC so this could work if the notification was sent from the parent process.
> 
> Really `_getWin` and sending a target are things you would get for free with a message from a message manager. Why aren't you using a message instead of an observer notification? message.target would give you the <browser>.

Right, the code that triggers this is called from the AddonManager, which always runs in the parent process.  The content-visible methods for installing add-ons (mozAddonManager and InstallTrigger) already handle IPC to coordindate between calls from a content process and the AOM in the parent process.

> Any reason not to use `new URL()`? See `Cu.importGlobalProperties(["URL"]);`

This is a good idea, I need to make sure that the various wildcards used in host permission patterns are compatible with URL, I'll look in to it.

> An early return for `rendered` would be preferred. I still need to look into why `rendered` is even necessary as there is no comment describing this quirk.

The specific case I ran into was changing focus away from the browser while the prompt was displayed, then changing focus back to the browser.  We get the "showing" event twice (once when it is initially displayed and again when focus comes back)

> Why are you passing `""` as the message instead of part of the message? I'm thinking about it affect accessiibility.

The UX spec calls for a bunch of styling (title in bold and a individual permissions displayed in a list) so it wasn't clear how to use message for some meaningful portion of the contents.  I hadn't thought about accessibility though, let me think this through a bit more.

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review98962

> This is a good idea, I need to make sure that the various wildcards used in host permission patterns are compatible with URL, I'll look in to it.

Hm, these site permissions / host permissions let you write something like `*://mozilla.org/*` (to cover both http and https) but `new URL()` can't parse that...

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review98962

> All of these style seems more relevant for a skin stylesheet rather than a content one but I'm not seeing an existing shared one in browser/ yet. We only have browser/themes/shared/notification-icons.inc.css for the icons. Can you either move the font-size and list-style-type change (maybe font-weight too?) to the browser.css for each desktop skin or create a new file in browser/themes/shared/ such as permission-prompts.inc.css or if you think you will add a lot more for webextensions then you can make webextensions-doorhanger.inc.css (like we have with plugin-doorhanger.inc.css[1]
> 
> [1] https://dxr.mozilla.org/mozilla-central/search?q=plugin-doorhanger.inc.css

Since we're down to a single rule, I replicated it in each of the platform-specific css files for now.  There are a bunch of other addon doorhanger related rules that are getting the same treatment, I'll look at moving those to a shared file in a subsequent bug that touches more of that code.

> Note that you shouldn't land these in m-c with placeholder strings unless you coordinate with l10n since localizers may start to localize the placeholder text.

I added further annotation to the properties file, but will also be sure to send an email to the l10n list before this lands.

    
  

        Attachment #8814635 -
        Flags: review?(florian)

    
  

        Attachment #8814635 -
        Flags: review?(MattN+bmo)

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #13)
> I added further annotation to the properties file, but will also be sure to
> send an email to the l10n list before this lands.

Fair warning: if you land placeholders, you'll need to update the string IDs when you land the updated strings, which is going to make this even messier
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Why are we landing this with placeholder strings in the first place? Can it just wait for bug 1316996 to be fixed?

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95800/#review99216

::: browser/modules/ExtensionsUI.jsm:66
(Diff revision 3)
> +                                           [info.addon.name]);
> +    let listHeader = bundle.getString("webextPerms.listHeader");
> +
> +    let formatPermission = perm => {
> +      try {
> +        return bundle.getString(`webextPerms.description.${perm}`);

I suggest to return the string ID here and leave the strings out of browser.properties. Then fix the code and add the proper strings in bug 1316996. The current approach is extremely messy.

    
    

    
  
(In reply to Francesco Lodolo [:flod] (mostly out of office until Dec 19) from comment #14)
> Fair warning: if you land placeholders, you'll need to update the string IDs
> when you land the updated strings, which is going to make this even messier
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#Changing_existing_strings

Hm, I wasn't aware of that constraint (fwiw I didn't actually interpret the docs you referenced that way but I believe you that this is how it works in practice).

I don't want to wait for bug 1316996 to land, I have a large stack of permissions-related patches and I don't want to keep them up to date until all the text is ready.  I hope things go smoothly on that front but already some folks in the community have been agitating about the content.  I'm staying out of it but I fear that it might delay having the content ready.

All that said, I agree that the best course here is to try to land this without any localization, the hard-coded strings won't be visible since this is all behind a pref.  I'll update the patch accordingly.

Thanks for the pointers.

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review99270

::: browser/locales/en-US/chrome/browser/browser.properties:35
(Diff revisions 3 - 4)
>  xpinstallDisabledMessage=Software installation is currently disabled. Click Enable and try again.
>  xpinstallDisabledButton=Enable
>  xpinstallDisabledButton.accesskey=n
>  
>  # LOCALIZATION NOTE (webextPerms.header)
>  # #1 is the localized name of the addon being installed.

Sorry, just noticed this one in the interdiff: variable here is %1$S, not #1, so the comment should be updated.

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review100684

Looks reasonable overall from looking at the code; I haven't tested the patch yet (I expect to do it on the next version).

How are we handling situations where the add-on name is really loooong? Or where the add-on requests several dozen permissions, to the point that the buttons at the bottom of the panel would be off-screen (especially on netbook small screens) ?

Lots of comments below but they are all details.

::: browser/base/content/popup-notifications.inc:79
(Diff revision 4)
> +
> +    <popupnotification id="addon-webext-permissions-notification" hidden="true">
> +      <popupnotificationcontent orient="vertical">
> +        <label id="addon-webext-perm-header" class="addon-webext-perm-header"/>
> +        <label id="addon-webext-perm-text" class="addon-webext-perm-text"/>
> +        <html:ul id="addon-webext-perm-list" class="addon-webext-perm-list">

nit: maybe just <html:ul ... /> ?

::: browser/locales/en-US/chrome/browser/browser.properties:36
(Diff revision 4)
>  xpinstallDisabledButton=Enable
>  xpinstallDisabledButton.accesskey=n
>  
> +# LOCALIZATION NOTE (webextPerms.header)
> +# #1 is the localized name of the addon being installed.
> +webextPerms.header=Add "%1$S"?

Normal double quotes won't pass our tests, you need quotes like this: “%1$S”

::: browser/locales/en-US/chrome/browser/browser.properties:37
(Diff revision 4)
>  xpinstallDisabledButton.accesskey=n
>  
> +# LOCALIZATION NOTE (webextPerms.header)
> +# #1 is the localized name of the addon being installed.
> +webextPerms.header=Add "%1$S"?
> +webextPerms.listHeader=It can:

I think this needs a localization note. When looking at this string, I can't figure out how to translate it into French because I don't know what 'it' refers to, and what's coming after ':'.

::: browser/modules/ExtensionsUI.jsm:14
(Diff revision 4)
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
> +                                  "resource://gre/modules/AddonManager.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",

You can Cu.import Services.jsm without lazy getter. It's most likely already loaded by the time ExtensionsUI.jsm is loaded, and the first thing we do with this file in browser.js is calling init(), which will use Services.obs immediately.

::: browser/modules/ExtensionsUI.jsm:36
(Diff revision 4)
> +
> +  _getWin(target) {
> +    let tbrowser = target.ownerGlobal.gBrowser;
> +
> +    let win;
> +    let iter = Services.wm.getEnumerator(null);

"navigator:browser" instead of null as the parameter; you want only browser.xul windows that will have gBrowser defined.

::: browser/modules/ExtensionsUI.jsm:48
(Diff revision 4)
> +    }
> +    if (!win) {
> +      throw new Error("cannot find top level window");
> +    }
> +
> +    return win;

How is 'win' here different from 'target.ownerGlobal'?

::: browser/modules/ExtensionsUI.jsm:66
(Diff revision 4)
> +                                           [info.addon.name]);
> +    let listHeader = bundle.getString("webextPerms.listHeader");
> +
> +    let formatPermission = perm => {
> +      try {
> +        //return bundle.getString(`webextPerms.description.${perm}`);

There will be lots of different permissions with localized descriptions, right? If so I think it'll make sense to move these to a new .properties file.

::: browser/modules/ExtensionsUI.jsm:100
(Diff revision 4)
> +        ...perms.permissions.map(formatPermission),
> +        ...perms.hosts.map(formatHostPermission),
> +    ];
> +
> +    let acceptText = bundle.getString("webextPerms.accept");
> +    let acceptKey = bundle.getString("webextPerms.acceptAccessKey");

to match the pattern of the surrounding strings in the same properties file, this should be webextPerms.accept.accesskey

::: browser/modules/ExtensionsUI.jsm:105
(Diff revision 4)
> +    let acceptKey = bundle.getString("webextPerms.acceptAccessKey");
> +    let cancelText = bundle.getString("webextPerms.cancel");
> +    let cancelKey = bundle.getString("webextPerms.cancelAccessKey");
> +
> +    let rendered = false;
> +    let popupOpts = {

nit: abreviating Options to Opts here doesn't save us any linebreak here, so we could as well make this variable name more explicit.

::: browser/modules/ExtensionsUI.jsm:107
(Diff revision 4)
> +    let cancelKey = bundle.getString("webextPerms.cancelAccessKey");
> +
> +    let rendered = false;
> +    let popupOpts = {
> +      hideClose: true,
> +      popupIconURL: info.icon,

Have you handled the fallback for when info.icon will be null? (I would assume there's an existing add-on icon used for other existing add-on panels that can be re-used)

::: browser/modules/ExtensionsUI.jsm:111
(Diff revision 4)
> +      hideClose: true,
> +      popupIconURL: info.icon,
> +      persistent: true,
> +      eventCallback(topic) {
> +        if (topic == "showing") {
> +          if (rendered) {

This 'showing' event being triggered a second time when reselecting the browser window seems to be an unintented side effect of bug 1004061. I think we should file a bug to fix this (or get this fixed) instead of working around it. If I remember correctly this unwanted second showing event had unfortunate consequences for the code I implemented in bug 1284877 too.

Or at the very least, this needs a comment.

::: browser/modules/ExtensionsUI.jsm:115
(Diff revision 4)
> +        if (topic == "showing") {
> +          if (rendered) {
> +            return;
> +          }
> +          win.document
> +            .getElementById("addon-webext-perm-header").value = header;

Do you expect these two strings to change based on the context? If not they value attribute should be set in popup-notifications.inc and the localized strings should then be in browser.dtd

::: browser/modules/ExtensionsUI.jsm:117
(Diff revision 4)
> +            return;
> +          }
> +          win.document
> +            .getElementById("addon-webext-perm-header").value = header;
> +          win.document
> +            .getElementById("addon-webext-perm-text").value = listHeader;

According to the mockups I've seen, we need to hide this for the no-permission-requested situation (ie. if (!msgs.length)).

::: browser/modules/ExtensionsUI.jsm:120
(Diff revision 4)
> +            .getElementById("addon-webext-perm-header").value = header;
> +          win.document
> +            .getElementById("addon-webext-perm-text").value = listHeader;
> +
> +          let list = win.document.getElementById("addon-webext-perm-list");
> +          while (list.firstChild != null) {

nit: you don't need the "!= null"

::: browser/modules/ExtensionsUI.jsm:121
(Diff revision 4)
> +          win.document
> +            .getElementById("addon-webext-perm-text").value = listHeader;
> +
> +          let list = win.document.getElementById("addon-webext-perm-list");
> +          while (list.firstChild != null) {
> +            list.removeChild(list.firstChild);

list.firstChild.remove();

::: browser/modules/ExtensionsUI.jsm:126
(Diff revision 4)
> +            list.removeChild(list.firstChild);
> +          }
> +
> +          for (let msg of msgs) {
> +            let item = win.document.createElementNS("http://www.w3.org/1999/xhtml", "li");
> +            item.setAttribute("class", "addon-webext-perm-item");

item.className = (as MattN said in comment 9)

::: browser/modules/ExtensionsUI.jsm:141
(Diff revision 4)
> +      win.PopupNotifications.show(target, "addon-webext-permissions", "",
> +                                  "addons-notification-icon",
> +                                  {
> +                                    label: acceptText,
> +                                    accessKey: acceptKey,
> +                                    callback: () => resolve(true),

Not sure if it's a good idea, but we could simplify to resolve("true") (and resolve("false") or even resolve() in the other case), and skip the JSON.stringify / JSON.parse (we would just test answer == "true" on the other side).

::: toolkit/mozapps/extensions/AddonManager.jsm:2950
(Diff revision 4)
> +          if (Preferences.get(PREF_WEBEXT_PREF_PROMPTS, false)) {
> +            install._permHandler = info => new Promise((resolve, reject) => {
> +              const observer = {
> +                observe(subject, topic, data) {
> +                  if (topic == "webextension-permission-response"
> +                      && subject.wrappedJSObject.info.addon == info.addon) {

nit: when doing a line break like this, the convention is to put && at the end of the previous line.

        Attachment #8814635 -
        Flags: review?(florian) → review-

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review100714

I had forgotten to look at the test.

::: browser/base/content/test/general/browser_extension_permissions.js:7
(Diff revision 4)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +"use strict";
> +
> +const BASE = "https://example.com/browser/browser/base/content/test/general";

I dislike the path being hardcoded; instead you can do:

const BASE = getRootDirectory(gTestPath).replace("chrome://mochitests/content/",
                                                 "https://example.com/");

::: browser/base/content/test/general/browser_extension_permissions.js:15
(Diff revision 4)
> +const ID = "permissions@test.mozilla.org";
> +
> +function promisePopupNotificationShown(name) {
> +  return new Promise(resolve => {
> +    PopupNotifications.panel.addEventListener("popupshown", function popupNotifShown() {
> +      PopupNotifications.panel.removeEventListener("popupshown", popupNotifShown);

You can use the {once: true} option in the addEventListener call to skip the removeEventListener call: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

::: browser/base/content/test/general/browser_extension_permissions.js:18
(Diff revision 4)
> +  return new Promise(resolve => {
> +    PopupNotifications.panel.addEventListener("popupshown", function popupNotifShown() {
> +      PopupNotifications.panel.removeEventListener("popupshown", popupNotifShown);
> +
> +      let notification = PopupNotifications.getNotification(name);
> +      isnot(notification, null, `${name} notification shown`);

isnot(stuff, null, ...) seems equivalent to ok(stuff, ...)

::: browser/base/content/test/general/browser_extension_permissions.js:43
(Diff revision 4)
> +}
> +
> +const INSTALL_FUNCTIONS = [
> +  function installMozAM() {
> +    return ContentTask.spawn(gBrowser.selectedBrowser, XPI_URL, function*(url) {
> +      return content.window.wrappedJSObject.installMozAM(url);

I think you can skip the .window here and get the same result.

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review100684

Thanks for the review!
I'll check with Markus about long names, I assume we would just truncate.
As for a very long list of permissions, I think Scott will be addressing that along with bug 1316996, I'd prefer to defer any implementation for that until that lands rather than guessing what the requirements will be.

I pushed a new version that addresses a bunch of the comments.  I still need to add new tests for the "no permissions" and "no icon" scenarios, feel free to look now if you like or to wait until those tests are written to do another review pass.

> There will be lots of different permissions with localized descriptions, right? If so I think it'll make sense to move these to a new .properties file.

I guess it depends on how many "lots" is.  There are fewer than 20 supported right now.  In any case, we can tackle that when bug 1316996 comes around.

> This 'showing' event being triggered a second time when reselecting the browser window seems to be an unintented side effect of bug 1004061. I think we should file a bug to fix this (or get this fixed) instead of working around it. If I remember correctly this unwanted second showing event had unfortunate consequences for the code I implemented in bug 1284877 too.
> 
> Or at the very least, this needs a comment.

Okay, it wasn't clear to me that this was unexpected behavior.  Filed bug 1325225 (and I'll add a comment here referencing that bug but will wait for the next revision to avoid unneeded mozreview churn)

> Do you expect these two strings to change based on the context? If not they value attribute should be set in popup-notifications.inc and the localized strings should then be in browser.dtd

Well this string has the (localized) extension name embedded in it.  The following one (listHeader) doesn't change but it seemed helpful to have all the messages that appear in this dialog right next to each other so that transltors can look at them together in one place.  But the whole translation process is pretty unfamiliar to me, if there are other advantages to moving that string to the dtd file, let me know and I'll move it.

> nit: you don't need the "!= null"

I've always preferred being explicit about adding comparison operators for integers and objects rather than using implicit casts to boolean to test for 0 / null.  If the coding standard under browser/ dictates not having the explicit test, I can remove it but I'm guessing that since you said "nit" that there's not a hard standard here?

> item.className = (as MattN said in comment 9)

Whoops, I removed that css rule altogether in response to Matt's comments but forgot to remove that here.  Its gone now.

> Not sure if it's a good idea, but we could simplify to resolve("true") (and resolve("false") or even resolve() in the other case), and skip the JSON.stringify / JSON.parse (we would just test answer == "true" on the other side).

I have a slight preference for the way it is written now as it is less vulernable to breaking due to fat-fingering one of those strings...

    
    

    
  
Markus, Florian pointed out that we need to define appearance here for extensions with extremely long names (currently they just make the doorhanger window stretch out and eventually just get clipped).  How should we handle them?
Flags: needinfo?(mjaritz)

    
    

    
  
Great catch. Thanks. Emanuela is working on those cases.
Flags: needinfo?(mjaritz) → needinfo?(emanuela)

    
    

    
  

      
      
      
      

        Attached image
          
        long-name_edgecase.jpg
        — 
      

    


  
(In reply to Andrew Swan [:aswan] from comment #23)
> Markus, Florian pointed out that we need to define appearance here for
> extensions with extremely long names (currently they just make the
> doorhanger window stretch out and eventually just get clipped).  How should
> we handle them?

I'd prefer to not stretch the window: the width should be about 385px.

If the extensions' name is longer than 26 character, truck it and add an ellipsis "…".
Flags: needinfo?(emanuela)

    
    

    
  
Emanuala, the prompt that is being implemented here looks like attachment 8814612 [details] (the add-on name is displayed on the first line, with a bigger font size, and I think that's on a single line that doesn't wrap). This was from https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions

Is the style on your attachment newer, and something we should use instead? Are you implying we should make the text of the first line wrap, and display the add-on name in bold?
Flags: needinfo?(emanuela)

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #22)

> > Do you expect these two strings to change based on the context? If not they value attribute should be set in popup-notifications.inc and the localized strings should then be in browser.dtd
> 
> Well this string has the (localized) extension name embedded in it.  The
> following one (listHeader) doesn't change but it seemed helpful to have all
> the messages that appear in this dialog right next to each other so that
> transltors can look at them together in one place.  But the whole
> translation process is pretty unfamiliar to me, if there are other
> advantages to moving that string to the dtd file, let me know and I'll move
> it.

If it's just one string that could be moved to the dtd file, let's keep it in the properties file to have everything in the same place.

> > nit: you don't need the "!= null"
> 
> I've always preferred being explicit about adding comparison operators for
> integers and objects rather than using implicit casts to boolean to test for
> 0 / null.  If the coding standard under browser/ dictates not having the
> explicit test, I can remove it but I'm guessing that since you said "nit"
> that there's not a hard standard here?

I said 'nit' because it's only a coding style issue and doesn't affect the behavior.

The coding style at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices says:
[...]Use (x) or (!x) [...] Compare objects to null, numbers to 0 or strings to "" if there is chance for confusion.

I see no chance for confusion here.

> > Not sure if it's a good idea, but we could simplify to resolve("true") (and resolve("false") or even resolve() in the other case), and skip the JSON.stringify / JSON.parse (we would just test answer == "true" on the other side).
> 
> I have a slight preference for the way it is written now as it is less
> vulernable to breaking due to fat-fingering one of those strings...

Fair enough.

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review101078

::: browser/locales/en-US/chrome/browser/browser.properties:37
(Diff revision 5)
>  xpinstallDisabledButton.accesskey=n
>  
> +# LOCALIZATION NOTE (webextPerms.header)
> +# %1$$ is the localized name of the addon being installed.
> +# See https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612
> +# for an example of how the strings here are combined in the

flod should have a look at this l10n note.

I think in some localization tools it may only get shown for the "webextPerms.header" string, so "an example of how the strings here are combined" may not turn out to be as helpful as we would hope to understand the context of the "It can:" string.

::: browser/modules/ExtensionsUI.jsm:10
(Diff revision 5)
> +
> +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> +
> +this.EXPORTED_SYMBOLS = ["ExtensionsUI"];
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Do you still use XPCOMUtils anywhere in this file?

::: browser/modules/ExtensionsUI.jsm:36
(Diff revision 5)
> +    let perms = info.addon.userPermissions;
> +    if (!perms) {
> +      return Promise.resolve();
> +    }
> +
> +    let win = target.ownerGlobal.wrappedJSObject;

do we actually need the .wrappedJSObject here?

::: browser/modules/ExtensionsUI.jsm:45
(Diff revision 5)
> +                                           [info.addon.name]);
> +    let listHeader = bundle.getString("webextPerms.listHeader");
> +
> +    let formatPermission = perm => {
> +      try {
> +        // return bundle.getString(`webextPerms.description.${perm}`);

Mention the bug number of where this will be cleaned up?

::: browser/modules/ExtensionsUI.jsm:83
(Diff revision 5)
> +    let acceptText = bundle.getString("webextPerms.accept.label");
> +    let acceptKey = bundle.getString("webextPerms.accept.accessKey");
> +    let cancelText = bundle.getString("webextPerms.cancel.label");
> +    let cancelKey = bundle.getString("webextPerms.cancel.accessKey");
> +
> +    let rendered = false;

reminder to add a comment referencing bug 1325223 (thanks for filing!) in the next version of the patch :-)

::: browser/modules/ExtensionsUI.jsm:89
(Diff revision 5)
> +    let popupOptions = {
> +      hideClose: true,
> +      popupIconURL: info.icon || DEFAULT_EXTENSION_ICON,
> +      persistent: true,
> +      eventCallback(topic) {
> +        if (topic == "showing") {

I think you want to preserve the notification when tearing off the tab to another window. If so, you need to add:
  if (topic == "swapping")
     return true;

::: browser/modules/ExtensionsUI.jsm:90
(Diff revision 5)
> +      hideClose: true,
> +      popupIconURL: info.icon || DEFAULT_EXTENSION_ICON,
> +      persistent: true,
> +      eventCallback(topic) {
> +        if (topic == "showing") {
> +          if (rendered) {

I think you need to set 'rendered' back to false when receiving the "dismissed" event.

If you don't, I think you'll have a problem with these steps:
1. Tab1 shows the notification for add-on1
2. Users opens and selects Tab2 where the notification is shown for add-on2.
3. The user selects Tab1 again.

At this point, the notification will reopen automatically (as it's a persistent notification), but I think it'll still show the content related to add-on2.

::: browser/modules/ExtensionsUI.jsm:106
(Diff revision 5)
> +          let listHeaderEl = win.document
> +                                .getElementById("addon-webext-perm-text");
> +          if (msgs.length == 0) {
> +            listHeaderEl.hidden = true;
> +          } else {
> +            listHeaderEl.value = listHeader;

You need to set hidden = false here to handle the case I mentioned above of reshowing the notification when switching back and forth between tabs.

listHeaderEl.hidden = !msgs.length;

I see no problem with setting listHeaderEl.value unconditionally like the previous version of the patch did, and then you can do the for loop unconditionally too.

        Attachment #8814635 -
        Flags: review?(florian) → review-

    
    

    
  

      
      
      
      

        Attached image
          
        webextensions@2x.jpg
        — 
      

    


  
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Emanuala, the prompt that is being implemented here looks like attachment
> 8814612 [details] (the add-on name is displayed on the first line, with a
> bigger font size, and I think that's on a single line that doesn't wrap).
> This was from
> https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-
> Permissions
> 
> Is the style on your attachment newer, and something we should use instead?
> Are you implying we should make the text of the first line wrap, and display


(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Emanuala, the prompt that is being implemented here looks like attachment
> 8814612 [details] (the add-on name is displayed on the first line, with a
> bigger font size, and I think that's on a single line that doesn't wrap).
> This was from
> https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-
> Permissions
> 
> Is the style on your attachment newer, and something we should use instead?
> Are you implying we should make the text of the first line wrap, and display
> the add-on name in bold?

Sorry for the confusion. The final style is the one attached (is also in figma, on the right side of the flow — you can find it in this comment as well). 

Tomorrow CEST morning I will update the screens for the flow as well to avoid even more confusions in the future.

All the text should wrap and only the extension's name should be bold.
Flags: needinfo?(emanuela)

    
    

    
  
(In reply to emanuela [ux team] from comment #29)
> All the text should wrap and only the extension's name should be bold.

The popup is implemented in XUL and I'm told that the mixed styling (some plain text and some bold text) isn't feasible in XUL.

    
    

    
  
:flod, can you take a look at Florian's comments about localization (the first line item in comment 28) and the comment that is there?  If it is insufficient, a specific suggestion for how to word it would be greatly appreciated.
Flags: needinfo?(francesco.lodolo)

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review100714

> isnot(stuff, null, ...) seems equivalent to ok(stuff, ...)

Addressed above, I can change it if you feel strongly.

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review101078

> I think you want to preserve the notification when tearing off the tab to another window. If so, you need to add:
>   if (topic == "swapping")
>      return true;

I tried adding this but the notification still disappears when I move the tab either into another existing window or into a new window...

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #33)
> Addressed above, I can change it if you feel strongly.

whoops, this was an old comment that reviewboard held onto for a while, please disregard it

    
    

    
  
Nit: it's add-on, not addon. 

I would try to be more explicit, even if having a link to a screenshot definitely helps. Here's my take

# LOCALIZATION NOTE (webextPerms.header)
# This string is used as a header in the add-on installation dialog,
# %1$$ is the localized name of the add-on being installed.
# See https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612
# for an example of the full dialog.
webextPerms.header=Add “%1$S”?

# LOCALIZATION NOTE (webextPerms.listHeader)
# This string will be followed by a list of permissions associated to 
# the add-on.
webextPerms.listHeader=It can:

webextPerms.accept.label=Add extension
webextPerms.accept.accessKey=A
webextPerms.cancel.label=Cancel
Flags: needinfo?(francesco.lodolo)

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #30)
> (In reply to emanuela [ux team] from comment #29)
> > All the text should wrap and only the extension's name should be bold.
> 
> The popup is implemented in XUL and I'm told that the mixed styling (some
> plain text and some bold text) isn't feasible in XUL.

Who told you that, where, in which context?
Flags: needinfo?(aswan)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #37)
> (In reply to Andrew Swan [:aswan] from comment #30)
> > (In reply to emanuela [ux team] from comment #29)
> > > All the text should wrap and only the extension's name should be bold.
> > 
> > The popup is implemented in XUL and I'm told that the mixed styling (some
> > plain text and some bold text) isn't feasible in XUL.
> 
> Who told you that, where, in which context?

I would try replacing the xul:label with a xul:description, and then do something similar to http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/browser/modules/webrtcUI.jsm#513 (ie. use .innerHTML to include a <label> with a class that would cause the different styling inside the description text content).

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review101208

Clearing review flag until comment 37 is answered.

::: browser/modules/ExtensionsUI.jsm:101
(Diff revisions 5 - 6)
> +      /* eslint-disable consistent-return */
>        eventCallback(topic) {
>          if (topic == "showing") {
> +          // This check can be removed when bug 1325223 is resolved.
>            if (rendered) {
>              return;

return false;

::: browser/modules/ExtensionsUI.jsm:130
(Diff revisions 5 - 6)
> +          rendered = false;
> +        } else if (topic == "swapping") {
> +          return true;
>          }
>        },
> +      /* eslint-enable consistent-return */

return false; And get rid of these ugly eslint anotations.

        Attachment #8814635 -
        Flags: review?(florian)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #37)
> (In reply to Andrew Swan [:aswan] from comment #30)
> > (In reply to emanuela [ux team] from comment #29)
> > > All the text should wrap and only the extension's name should be bold.
> > 
> > The popup is implemented in XUL and I'm told that the mixed styling (some
> > plain text and some bold text) isn't feasible in XUL.
> 
> Who told you that, where, in which context?

It was :kmag, answering a question about how to create this exact style of text in xul.  I managed to make this work with some <html:span> elements inside the <description> but he insisted this was a bad idea.  I suspect he feels similarly about innerHTML but we can give it a shot.

(In reply to Florian Quèze [:florian] [:flo] from comment #38)
> I would try replacing the xul:label with a xul:description, and then do
> something similar to
> http://searchfox.org/mozilla-central/rev/
> ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/browser/modules/webrtcUI.jsm#513
> (ie. use .innerHTML to include a <label> with a class that would cause the
> different styling inside the description text content).

This means the localized extension name needs to be sanitized.  It is safe to assume that the localized browser strings are safe and don't need to be sanitized?  Whats the preferred method for sanitizing arbitrary strings before injecting them as html?
Flags: needinfo?(aswan)

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review101208

> return false; And get rid of these ugly eslint anotations.

Doing this felt unsafe to me since the events that can be indicated here are such a hodge-podge, they aren't all documented, and the ones that are documented are inconsistent with how they treat return values.  But if you believe this is a safe thing to do, I'll change it to that.

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #40)

> I managed to make this work with some <html:span> elements
> inside the <description>

Why html:span instead of a xul label?

> (In reply to Florian Quèze [:florian] [:flo] from comment #38)
> > [use .innerHTML]
> 
> This means the localized extension name needs to be sanitized.  It is safe
> to assume that the localized browser strings are safe and don't need to be
> sanitized?  Whats the preferred method for sanitizing arbitrary strings
> before injecting them as html?

You probably just need to replace the < > & characters; not sure if there's a preferred method for it.

But where is the extension name coming from? Are we sure entities aren't already escaped? (They may be if it's coming from an xml file, like install.rdf)

(In reply to Andrew Swan [:aswan] from comment #41)

> > return false; And get rid of these ugly eslint anotations.
> 
> Doing this felt unsafe to me since the events that can be indicated here are
> such a hodge-podge, they aren't all documented, and the ones that are
> documented are inconsistent with how they treat return values.  But if you
> believe this is a safe thing to do, I'll change it to that.

I'm confident. I added the cases where return true matters. When returning false or not returning any value (ie. undefined), it's the default behavior, to be backward compatible with callers that were implemented before we added return values. In the cases that aren't documented, the return value is ignored.

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #42)
> But where is the extension name coming from? Are we sure entities aren't
> already escaped? (They may be if it's coming from an xml file, like
> install.rdf)

It comes from a json file contained in the extension (either the manifest or one of the locale files.  In either case, the contents of the string are not constrained (even if AMO refused to sign extensions with markup in the name, I think we would still want to be cautious here...)

    
    

    
  
So Scott's first draft of permissions copy shows the header entirely in bold with text like:
Add "Extension Name"?
Emanuela's mocks have different text but more importantly, they do not have quotes around the addon name.  Instead they put the name in bold text.
I'm stumped about how to make the quotes work at all.  If the quotes are meant to be in bold, then we have a terrible localization problem (since the quotes need to be localized, but they need to be localized separately from the rest of the sentence so that we can make them bold).  If the quotes are not meant to be in bold, I don't think Florian's suggestion from comment 38 will work since we appear to get breaking space placed between the text content and the <label> element.

I think the realistic options are:
1. Entire header in bold with quotes around the extension name
2. Extension name in bold but no quotes

I'm not even sure who's decision this is.  Lets got with Scott, what do you say?

    
  
Flags: needinfo?(sdevaney)

    
    

    
      
  

    
    

    
  
In the interest of making some progress, I've dropped all the localized strings for now, I'll pick those all up at part of but 1316996.  For now we show the whole header in bold, fussing with the exact presentation can also happen once we have the actual text to work with.  As before, this is still all behind a hidden preference so nobody will see this yet, but there are several more permissions related patches that will need to land, getting this in will hopefully clear the way to get those reviewed and landed.
Flags: needinfo?(sdevaney)

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #46)
> In the interest of making some progress, I've dropped all the localized
> strings for now, I'll pick those all up at part of but 1316996.

Do you know which version of Firefox is this feature targeting?

    
    

    
  
(In reply to Francesco Lodolo [:flod] from comment #47)
> (In reply to Andrew Swan [:aswan] from comment #46)
> > In the interest of making some progress, I've dropped all the localized
> > strings for now, I'll pick those all up at part of but 1316996.
> 
> Do you know which version of Firefox is this feature targeting?

53

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #44)

> 
> I think the realistic options are:
> 1. Entire header in bold with quotes around the extension name
> 2. Extension name in bold but no quotes
> 
> I'm not even sure who's decision this is. Lets got with Scott, what do you
> say?

I suggest going with the second option: Extension name in bold but no quotes. 
Let's wait for Scott input as well.

Regarding the different copy, I will talk with Scott asap :)

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #44)

> If the quotes are
> meant to be in bold, then we have a terrible localization problem (since the
> quotes need to be localized, but they need to be localized separately from
> the rest of the sentence so that we can make them bold).

In that case you need a separate localizable string for the quotes, that will look like this:
stringId=“%S”

> If the quotes are
> not meant to be in bold, I don't think Florian's suggestion from comment 38
> will work since we appear to get breaking space placed between the text
> content and the <label> element.

Are you sure you are not just seeing the <label>'s default margins?

    
    

    
  
Agree to go with option #2! (in bold, no quotes)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #50)
> > If the quotes are
> > not meant to be in bold, I don't think Florian's suggestion from comment 38
> > will work since we appear to get breaking space placed between the text
> > content and the <label> element.
> 
> Are you sure you are not just seeing the <label>'s default margins?

Or an extra space caused by the two elements being on different lines in the XUL file.

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review102270

I would like to try the patch locally before r+'ing. I only found one trivial issue while reviewing the code.

::: browser/themes/osx/browser.css:3094
(Diff revision 7)
>  
>  .addon-install-confirmation-name {
>    font-weight: bold;
>  }
>  
> +.addon-webext-perm-addon-name {

The class name in the CSS file is addon-webext-perm-header on the other 2 OSes.

        Attachment #8814635 -
        Flags: review?(florian)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review102282

::: browser/modules/ExtensionsUI.jsm:50
(Diff revision 8)
> +    // bundle.get*String() calls as part of bug 1316996.
> +
> +    // let bundle = win.gNavigatorBundle;
> +    // let header = bundle.getFormattedString("webextPerms.header", [name])
> +    // let listHeader = bundle.getString("webextPerms.listHeader");
> +    let header = "Add “ADDON”?".replace("ADDON", name);

There's an encoding issue here. You can just use "Add \u201CADDON\u201D?" for now.

::: browser/modules/ExtensionsUI.jsm:110
(Diff revision 8)
> +          // This check can be removed when bug 1325223 is resolved.
> +          if (rendered) {
> +            return false;
> +          }
> +
> +          win.document

You can't re-use this 'win' variable here, it points to the window in which the notification was initially created. We allowed swapping so the window may be different at the time we show the notification.
From the eventCallback, 'this' is the notification object, from which you can access the browser that the notification is currently attached to, so this would work:
let doc = this.browser.ownerDocument;

And then win would be doc.defaultView, but it seems you only need access to the document.

        Attachment #8814635 -
        Flags: review?(florian) → review-

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review102288

r=me to let you move forward, but this can't be preff'ed on before we have the strings handled correctly (bug 1316996).

        Attachment #8814635 -
        Flags: review?(florian) → review+

    
    

    
      
  

    
    

    
  
Comment on attachment 8814635 [details]
Bug 1308309 Prompt for webextensions permissions

https://reviewboard.mozilla.org/r/95802/#review98962

> I would recommend adding to https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm#80-87 to get automated screenshots of the dialog and eventually change alerts.

Filed bug 1328339 to do this for all the permissions dialogs.

    
    

    
  
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f75d989006e7
Prompt for webextensions permissions r=florian,rhelmer

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f75d989006e7
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

    
    

    
  
Verified on Firefox 53.0a1 (2017-01-08) under Windows 10 64-bit and Ubuntu 16.04 32-bit and noticed the following: 
 
 - Canceling the permissions doorhanger, the webextension will not be installed but the radio button will remain enabled. After that the button will become unresponsive: http://screencast.com/t/cgQTX0QsRH

 - The icon and the webextension name are misaligned: http://screencast.com/t/hAup1RhpZo6 

 - Duplicated permission line: http://screencast.com/t/lpNEyhFO 

Andrew are these issues already tracked by another bugs or should I file new issues for them?
Flags: needinfo?(aswan)

    
    

    
  
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #63)
>  - Canceling the permissions doorhanger, the webextension will not be
> installed but the radio button will remain enabled. After that the button
> will become unresponsive: http://screencast.com/t/cgQTX0QsRH

This is tracked here:
https://github.com/mozilla/addons-frontend/issues/1435

>  - The icon and the webextension name are misaligned:
> http://screencast.com/t/hAup1RhpZo6 

Can you file a followup for this?

>  - Duplicated permission line: http://screencast.com/t/lpNEyhFO 

Lets hold off on this one until we get the final copy for permissions in
Flags: needinfo?(aswan)
Depends on: 1329942

    
    

    
  
(In reply to Andrew Swan [:aswan] from comment #64)
> >  - The icon and the webextension name are misaligned:
> > http://screencast.com/t/hAup1RhpZo6 
> 
> Can you file a followup for this?

Thanks for your reply!

Filed Bug 1329942 for this issue.

Based on Comment 63 and Comment 64 I am marking this bug as Verified since the other issues are tracked separately.
Status: RESOLVED → VERIFIED

          status-firefox53:
          fixed → verified

    
  
No longer blocks: 1331618

    
  
No longer depends on: 1329942

    
  
Product: Toolkit → WebExtensions

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: