Closed Bug 1379833 Opened 7 years ago Closed 7 years ago

Implement install-time permissions dialog for extensions on Android

Categories

(WebExtensions :: Android, enhancement, P1)

55 Branch
enhancement

Tracking

(firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(7 files, 1 obsolete file)

A webextension can have anywhere from 0 to ~20 permissions, the strings we currently show at install time for promptable permissions are here:
http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/locales/en-US/chrome/browser/browser.properties#98-141

My first question is if we can re-use the same strings or if we need to do anything special for Android.
If we can re-use the existing strings, can just move the existing strings into a .properties file in toolkit/ or will that cause trouble with the tools that we use for localization?
Flags: needinfo?(gandalf)
My gut feeling is that those strings will be too long for vertical view on a phone screen.

Wrt. moving the strings - I believe that you can, but I don't know if there's any additional meta-info magic to help toolchain pick it up and help localizers with the move.

NI'ing Pike
Flags: needinfo?(gandalf) → needinfo?(l10n)
I'd like to get flod's take next week on the re-use and what we can do to help.

That we threw the strings into the big old browser.properties bucket to begin with makes this a bit harder. We had an easier time if the strings were in their own file already, and we could just move them around as one.

Before we go too deep into the logistics, I think it'd be really good to have a mock-up of how those permissions would actually look on the phone. I share gandalf's concern here.

https://transvision.mozfr.org/string/?entity=browser/chrome/browser/browser.properties:webextPerms.description.downloads&repo=central gives an idea on which languages to include in such a mock-up. French and Greek sound like good candidates on length, Armenian and Thai for line breaks. Language codes would be fr, el, hy-AM, th.
Flags: needinfo?(l10n)
Markus/Jack, can you guys respond to the issues that gandalf and Pike brought up?
Flags: needinfo?(mjaritz)
Flags: needinfo?(jalin)
Jack, can you please create that mock-up. From you presentation, I assume you have a template you can past those strings into, so that we can quickly get a sense of how those screens might actually look, using those languages. Thanks.
If we need to modify anything, please follow up with Emanuela. Thanks.
Flags: needinfo?(mjaritz)
Hi Emanuela,

I shared the sketch file (mock-up) with you, https://drive.google.com/drive/folders/0BwbmH5cX2W6OUVVGSFFVcldrZ3c 
So you can redesign the mock-up or do any modification for the request.

Please let me know if you have any questions
Thank you
Jack
Flags: needinfo?(jalin) → needinfo?(emanuela)
Jack, I am unable to view the document from comment 6 due to lack of permissions.  Can you just attach an image to the bug?
Flags: needinfo?(jalin)
Hi Andrew,

The document from comment 6 is the old design and just for Emanuela. I think she will update the design and share with you later.

Thank you
Flags: needinfo?(jalin)
Here how the modal looks like with the permissions in different languages. I wasn't able to find the Thai translations at all,  and the string 'Add [extensions name]' + the action, but I hope what we have here it's good enough to decide.

According to Jack, the modal can be as height as the screen. In any case, when we need it, we can stick the action footer at the bottom of the modal and make the element scrollable.


The solution is to follow Material guidelines and use Noto (in its version for the different languages) to manage better any languages peculiarity.
Flags: needinfo?(emanuela)
And, btw, after the mocks I think we can go with the same strings on android and desktop.
I'm going to table this until :flod is back.  Francisco, when you dig out of your post-PTO backlog, can you help me with the question from comment 1?
Flags: needinfo?(francesco.lodolo)
This is a screenshot on desktop, for a test XPI that I've created and it's probably already missing some permissions. So, things can get complicated.
Flags: needinfo?(francesco.lodolo)
Attachment #8886933 - Attachment description: permissions.png → Permission dialog on desktop (12 permissions, unverified)
(In reply to Andrew Swan [:aswan] from comment #1)
> If we can re-use the existing strings, can just move the existing strings
> into a .properties file in toolkit/ or will that cause trouble with the
> tools that we use for localization?

If we simply move strings in /toolkit for en-US, localization tools will show them as untranslated, and localizers will only be able to use Translation Memory to "retranslate them". There are no ways around it, besides actually moving strings in the repositories.

As Axel pointed out, one problem is that those strings are buried inside a file that contains hundreds of other strings. We have code to migrate strings from .DTD to .properties in DevTools, I don't think we have anything ready for .properties alone. In case, it needs to be created and tested.

The UI doc says that we're going to use the same text on Android and Desktop. I think the screenshot above from desktop shows that, while the actual permissions might be the same, it's hard to keep the exact same UI and text: on mobile the first permission could potentially end up out of screen on a small phone (also, "Cancel" vs "CANCEL"). 
So, permission strings can potentially be still scattered across multiple places (/browser, /mobile, and some shared in /toolkit).

There's one big pro in having permissions in a shared file, and that's consistency across platforms. I'm not completely sure if that's worth the effort in this specific case. Translation memory will still help localizers, and give them the flexibility to shorten strings if necessary (or change tone, which seems unlikely for this specific set of strings).

At risk of asking a dumb question, is this going to be managed on the Gecko side, not Java side in Android? Because if it's Java, it makes the whole discussion unnecessary.
(In reply to Francesco Lodolo [:flod] from comment #13)
> At risk of asking a dumb question, is this going to be managed on the Gecko
> side, not Java side in Android? Because if it's Java, it makes the whole
> discussion unnecessary.

Its not a dumb question.  The answer might be dumb though: I was intending to do as much as possible (including all the l10n work) from js and use minimal java.  But that's mostly due to my own limited java skills...

So to sum up, it seems like there's still some uncertainty but the best option looks like copying the existing strings to an appropriate place under mobile/.  Some translators will recognize that they recently translated these same strings and they can either copy the existing translations or they may adjust them to account for the fact that there is less space on mobile devices.  Did I get this right?
Flags: needinfo?(francesco.lodolo)
(In reply to Andrew Swan [:aswan] from comment #14)
> Its not a dumb question.  The answer might be dumb though: I was intending
> to do as much as possible (including all the l10n work) from js and use
> minimal java.  But that's mostly due to my own limited java skills...

I'm not much of a developer, but if you end up needing to work on the Java side, we still have a ton of l10n limitations there: no plural forms support, need to avoid straight quotes (single or double), escape the world, etc.

> So to sum up, it seems like there's still some uncertainty but the best
> option looks like copying the existing strings to an appropriate place under
> mobile/.  Some translators will recognize that they recently translated
> these same strings and they can either copy the existing translations or
> they may adjust them to account for the fact that there is less space on
> mobile devices.  Did I get this right?

I think so. It's a bit more painful (translation memory will help), but it gives us flexibility if we ever need it (e.g. to shorten strings, or change tone).
Flags: needinfo?(francesco.lodolo)
Attachment #8893078 - Flags: feedback?(lgreco)
Attachment #8893079 - Flags: feedback?(lgreco)
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

https://reviewboard.mozilla.org/r/164078/#review169852

::: mobile/android/chrome/content/ExtensionPermissions.js:45
(Diff revision 1)
> +      let listener = {
> +        onEvent(event, data, callback) {
> +          switch (event) {
> +            case "Extension:PermissionPromptResult": {
> +              if (data.accepted) {
> +                info.resolve();
> +              } else {
> +                info.reject();
> +              }
> +              EventDispatcher.instance.unregisterListener(listener, "Extension:PermissionPromptResult");
> +            }
> +          }
> +        }
> +      };
> +      EventDispatcher.instance.registerListener(listener, "Extension:PermissionPromptResult");
> +
> +      EventDispatcher.instance.sendRequest({
> +        type: "Extension:PermissionPrompt",
> +        header: strings.header,
> +        message: message,
> +        icon,
> +      });

it looks like we can probably simplify this by using `EventDispatcher.instance.sendRequestForResult`:

- http://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/Messaging.jsm#120-136

e.g. here is how we use it for another existent message: 

- http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/mobile/android/components/ContentDispatchChooser.js#61-69
- http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#417-432
- http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#480
Comment on attachment 8893078 [details]
Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm

https://reviewboard.mozilla.org/r/164076/#review170252

::: browser/modules/ExtensionsUI.jsm:255
(Diff revision 1)
>  
>    // Create a set of formatted strings for a permission prompt
>    _buildStrings(info) {
> -    let result = {};
> -
>      let bundle = Services.strings.createBundle(BROWSER_PROPERTIES);

I'm wondering if we could define these `bundle` and `brandBundle` (or `appName`) as `XPCOMUtils.defineLazyGetter`, so that we lazily create them the first time that we need them and then we reuse them for the next calls.

::: browser/modules/ExtensionsUI.jsm:260
(Diff revision 1)
> -      result.acceptKey = bundle.GetStringFromName("webextPerms.optionalPermsAllow.accessKey");
> -      result.cancelText = bundle.GetStringFromName("webextPerms.optionalPermsDeny.label");
> -      result.cancelKey = bundle.GetStringFromName("webextPerms.optionalPermsDeny.accessKey");
> -    }
>  
> -    return result;
> +    let format = name => `<span class="addon-webext-name">${this._sanitizeName(name)}</span>`;

Nit, both `format` and `_sanitizeName` seems to be just pure functions (which only use their parameters), we could move both of them out of this object as helper functions, and pass the shared `format` (maybe renamed to `function formatAddonName(name) {...}`) to the `ExtensionData.formatPermissionString` method, instead of creating a new arrow function at every call of `_buildStrings`.

::: toolkit/components/extensions/Extension.jsm:806
(Diff revision 1)
> +      } else {
> +        sites.push(match[1]);
> +      }
> +    }
> +
> +    // Format the host permissions.  If we have a wildcard for all urls,

Nit (very very nit), multiple spaces in the inline comment (it was actually just moved, the previous version already has multiple spaces in these comments), same for the inline comment next at line 813.

::: toolkit/components/extensions/Extension.jsm:817
(Diff revision 1)
> +    } else {
> +      // Formats a list of host permissions.  If we have 4 or fewer, display
> +      // them all, otherwise display the first 3 followed by an item that
> +      // says "...plus N others"
> +      function format(list, itemKey, moreKey) {
> +        function formatItems(items) {

Nit, it seems that with an additional `itemKey` parameter we can avoid to nest the `formatItems` closure inside the `format` closure.

As a related Nit, it seems a bit weird that a function called `formatItems` pushed the formatted item directly in the `result.msgs` array as a side effect instead of returning the result and let the caller to push the formatted items into the `result.msgs` array.

All of this was already there in the previous version of this code, and so I don't consider any of this as a blocker.

::: toolkit/components/extensions/Extension.jsm:848
(Diff revision 1)
> +    }
> +
> +    // Finally, show remaining permissions, in any order.
> +    for (let permission of perms.permissions) {
> +      // Handled above
> +      if (permission == "nativeMessaging") {

Nit, we could also use NATIVE_MSG_PERM instead of the plain permission string here.

::: toolkit/components/extensions/Extension.jsm:855
(Diff revision 1)
> +      }
> +      try {
> +        result.msgs.push(bundle.GetStringFromName(permissionKey(permission)));
> +      } catch (err) {
> +        // We deliberately do not include all permissions in the prompt.
> +        // So if we don't find one then just skip it.

Nit, it could be reasonable to log the error when Firefox is built in debug mode (only when `AppConstants.DEBUG` is true)? 

(mostly to make it easier to spot any issue that could be related to a missing string from the bundle, e.g. we do something similar in Schemas.jsm)
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

https://reviewboard.mozilla.org/r/164078/#review170270

::: mobile/android/base/java/org/mozilla/gecko/extensions/PermissionsHelper.java:31
(Diff revision 1)
> +    private final Context mContext;
> +
> +    public PermissionsHelper(Context context) {
> +        mContext = context;
> +
> +        Log.d("AddonManager", "Instantiating PermissionHelper");

it looks like there are some conventions around the logging which is shared by the other java classes near to this:

- declare a LOGTAG as a `private static final` on the class (e.g. http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java#40 or http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#198)
- set the LOGTAG as `GeckoCLASSNAME` (e.g. BrowserApp set the LOGTAG to "GeckoBrowserApp", and so this could be "GeckoPermissionsHelper")
- All the `Log.d` / `Log.w` calls use `LOGTAG` as the first parameter

It is probably a good idea to follow the same convention in this new class (especially given that `adb logcat` produces a huge amount of logs and being able to consistently and easily filter them is pretty useful while debugging issues on Firefox for Android)

::: mobile/android/chrome/content/ExtensionPermissions.js:24
(Diff revision 1)
> +
> +      // Ugh, Extension.getPreferredIcon() returns this desktop-browser-specific
> +      // URL and we don't appear to have any single URL that is common across
> +      // desktop and mobile.  But trying to render an svg into an Android
> +      // dialog would be an adventure, so just hack around it here.
> +      if (icon == "chrome://browser/content/extension.svg") {

could we make `Extension.getPreferredIcon()` able to conditionally return the desktop specific or the android specific URL instead?

(e.g. by using `AppConstants.platform == "android"` as we already do elsewhere to detect on which platform is currently running).
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

https://reviewboard.mozilla.org/r/164078/#review170270

> could we make `Extension.getPreferredIcon()` able to conditionally return the desktop specific or the android specific URL instead?
> 
> (e.g. by using `AppConstants.platform == "android"` as we already do elsewhere to detect on which platform is currently running).

My order of preference would actually be:
1. Just have a shared icon with a fixed url and no platform-specific switch
2. If that's not feasible, have `getPreferredIcon()` return null or something and have callers insert their own defaults
Comment on attachment 8893078 [details]
Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm

https://reviewboard.mozilla.org/r/164076/#review170252

Thanks, I think I agree with all your suggestions but this is just moving existing code, and I'd like to keep unrelated changes separate for now.  Sorry if that wasn't clear from the feedback request...
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

Julian, can you both take a look at this patch for general issues but also help me with a specific question?  The specific question is how we can render svg icons into a drawable so they can be displayed in the install confirmation dialog.  I'm using ResourceDrawableUtils.getDrawable() here but it doesn't appear to handle svg...
If you're not the right person to review and/or answer the svg question, can you help me find somebody appropriate?  Thanks!
Attachment #8893079 - Flags: feedback?(walkingice0204)
Andrew,

We cannot use SVG directly for Android native widget, instead we convert it to VectorDrawable[1], then use it like a normal drawable resources. It seems you are trying to show `extension.svg` in a dialog, I did some try see whether match your expectation.

1. Use Android Studio to convert extension.svg to ic_extension.xml(as a VectorDrawable)
2. change content of xml file(set fillColor to #FF66CC52, original svg file is gradient and this is close enough)
3. put the xml file under `mobile/android/app/src/main/res/drawable/`
4. Let PermissionsHelper to get drawable from "drawable://ic_extension"

[1] https://developer.android.com/studio/write/vector-asset-studio.html
Attached file ic_extension.xml
convert from extension.svg by Android Studio, and change fillColor to green
A screenshot for VectorDrawable in AlertDialog, on Nexus5 Android 4.4
Ah thanks for the information and for doing all that work!  But this isn't just about the generic puzzle piece icon -- an extension xpi file can also include an icon in svg format.  If there's no practical way to get those into a Drawable at runtime then I think we should adopt the method outlined in comment 28 and just show the puzzle piece for xpis that have only svg icons.
The mocked up dialog has the buttons in orange and in a different font -- is that a style that is used elsewhere in Fennec?  If so can you point me to it so I just re-use the existing style rather than trying to recreate it.
Flags: needinfo?(jalin)
For old version of alert dialog, Fennec uses buttons in orange and different font. But for latest design, it will be different.

I discussed with Julian, he suggests that you can use V7 version of the alert dialog design.

Julian, is that right?
Flags: needinfo?(jalin) → needinfo?(walkingice0204)
I don't have any idea what V7 means in this context.  Can you provide a reference?  (or even better, just existing dialog styles in the source tree)
For the svg part, I think we should fallback to the green puzzle icon if extension xpi file only provides a svg icon.

I was thinking about to import **android.support.v7.app.AlertDialog**, but actually in our source code we only use **android.app.AlertDialog**. So please forget the comment Jack mentioned.

I see you create the dialog in standard way so it should use the default theme(in my personal opinion, it should be fine). The color of buttons might vary depends on different Android version. (as in my previous screenshot, it is not orange color but a black color).

We have AlertDialog in SearchPreferenceActivity[1]. To see the dialog

1) go to phone home screen
2) long-press to add a widget
3) to add a search widget which provided by Fennec
4) click the widget, click gear icon in right-bottom corner, click 'clear search history'

[1] https://dxr.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139/mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java#60

Your mock up dialog buttons looks same as that one, so I feel it is good enough. If you find out any other existing dialog which match your expectation but no clue how to achieve that, please let me know!
Flags: needinfo?(walkingice0204)
Blocks: 1392924
Attachment #8897455 - Attachment is obsolete: true
Attachment #8893078 - Flags: review?(tomica)
Attachment #8893079 - Flags: review?(walkingice0204)
:flod, heads up on the new strings to be (hopefully) landing soon in this patch.  I think everything matches what we talked about earlier (ie comment 13) but if I've goofed up on anything, lets straighten it out before landing.
Flags: needinfo?(francesco.lodolo)
Thanks, double checked and I can't spot any issue.
Flags: needinfo?(francesco.lodolo)
Hi Andrew,

Here are svgs to vector drawable documents, maybe you need them.
https://developer.android.com/studio/write/vector-asset-studio.html#types
https://www.ahunt.org/2017/02/fun-with-vectordrawables/

So that you can put icon to alert dialog.
Comment on attachment 8893078 [details]
Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm

https://reviewboard.mozilla.org/r/164076/#review177086

My first instinct was that this should go into ExtensionUtils, but I can't really express why.

::: browser/modules/ExtensionsUI.jsm:260
(Diff revision 2)
> -      result.acceptKey = bundle.GetStringFromName("webextPerms.optionalPermsAllow.accessKey");
> -      result.cancelText = bundle.GetStringFromName("webextPerms.optionalPermsDeny.label");
> -      result.cancelKey = bundle.GetStringFromName("webextPerms.optionalPermsDeny.accessKey");
> -    }
>  
> -    return result;
> +    let format = name => `<span class="addon-webext-name">${this._sanitizeName(name)}</span>`;

why not just pass the formatted addon name instead of jumping through hoops?

::: toolkit/components/extensions/Extension.jsm:761
(Diff revision 2)
> +   * Formats all the strings for a permissions dialog/notification.
> +   *
> +   * @param {object} info  Information about the permissions being requested.
> +   * @param {array<string>} info.permissions.origins Origin permissions
> +   *                                                 requested.
> +   * @param {array<string>} info.permissions.permissions Regular (non-origin)

nit: "Api permission"

also, all of these descriptions are fairly unreadable all bunched up to the right, maybe just start them on the new line indented?
Attachment #8893078 - Flags: review?(tomica) → review+
Comment on attachment 8893078 [details]
Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm

https://reviewboard.mozilla.org/r/164076/#review177086

> nit: "Api permission"
> 
> also, all of these descriptions are fairly unreadable all bunched up to the right, maybe just start them on the new line indented?

not sure what you mean by "Api permission"?
Julian, I'd like to get this landed soon as there are several other bugs dependent on it.  Any chance of getting a review in the next day or two?  If not, can you suggest an alternate reviewer?
Flags: needinfo?(walkingice0204)
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

https://reviewboard.mozilla.org/r/164078/#review181732

::: mobile/android/base/java/org/mozilla/gecko/extensions/PermissionsHelper.java:33
(Diff revision 5)
> +
> +    public PermissionsHelper(Context context) {
> +        mContext = context;
> +
> +        Log.d(LOGTAG, "Instantiating PermissionHelper");
> +        EventDispatcher.getInstance().registerUiThreadListener(this,

should we unregister this in BrowserApp.onDestroy?
Attachment #8893079 - Flags: review?(walkingice0204) → review+
Java part looks good to me apart from listener-unregistration. But I am not pretty confident for the js part, could you find another reviewer for that?
Flags: needinfo?(walkingice0204)
Attachment #8893079 - Flags: review?(s.kaspari)
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

https://reviewboard.mozilla.org/r/164078/#review182354

r+ to unblock landing this. However we should make sure to unregister this new component - before landing the patch. Everything else I consider a nit and could be moved to follow-up bugs if needed.

::: mobile/android/base/java/org/mozilla/gecko/extensions/PermissionsHelper.java:25
(Diff revision 5)
> +import android.view.LayoutInflater;
> +import android.view.View;
> +import android.widget.ImageView;
> +import android.widget.TextView;
> +
> +public class PermissionsHelper implements BundleEventListener {

The name of the class is a bit confusing because we already have two classes named:
* (org.mozilla.gecko.permissions.)Permissions
* (org.mozilla.gecko.permissions.)PermissionsHelper
^-- Those are about Android's runtime permissions. Maybe it makes sense to use "Extensions" in the name somewhere?

::: mobile/android/base/java/org/mozilla/gecko/extensions/PermissionsHelper.java:33
(Diff revision 5)
> +
> +    public PermissionsHelper(Context context) {
> +        mContext = context;
> +
> +        Log.d(LOGTAG, "Instantiating PermissionHelper");
> +        EventDispatcher.getInstance().registerUiThreadListener(this,

We register this component as a listener here but we are never unregistering this. This means we could end up with a bunch of those registered (whenever a BrowserApp is created - those can come and go during the process lifetime). See AccountsHelper and others using the same pattern here.

::: mobile/android/base/java/org/mozilla/gecko/extensions/PermissionsHelper.java:51
(Diff revision 5)
> +            builder.setView(view);
> +            ((TextView) view.findViewById(R.id.extension_permission_header)).setText(message.getString("header"));
> +            ((TextView) view.findViewById(R.id.extension_permission_body)).setText(message.getString("message"));
> +
> +            final String iconUrl = message.getString("icon");
> +            Log.d(LOGTAG, "Load icon from " + iconUrl);

nit: Do we want to ship with those debug log messages or could we remove them?

::: mobile/android/base/java/org/mozilla/gecko/extensions/PermissionsHelper.java:76
(Diff revision 5)
> +                    public void onBitmapFound(final Drawable d) {
> +                        iconView.setImageDrawable(d);
> +                    }
> +                });
> +
> +            final AlertDialog dialog = builder.create();

If we want this dialog to get restored when the app goes to the background and comes back to the foreground (and the activity was destroyed in the background) then we should use a DialogFragment instead of a plain dialog here. Can be a follow-up bug though.
Attachment #8893079 - Flags: review?(s.kaspari) → review+
Comment on attachment 8893079 [details]
Bug 1379833 Part 2: Display permissions dialog during extension install on Android

https://reviewboard.mozilla.org/r/164078/#review182354

> If we want this dialog to get restored when the app goes to the background and comes back to the foreground (and the activity was destroyed in the background) then we should use a DialogFragment instead of a plain dialog here. Can be a follow-up bug though.

I'm displaying my Android ignorance here but I'm not sure what "the activity was destroyed in the background" means exactly?  Does that mean the process exits and a new process is started?  In that case I would expect we would lose the associated AddonInstall so if we did re-display the dialog, it wouldn't actually be able to complete the installation if the user accepted the permissions...
If I've misundertsood, please clarify and I'll open a follow-up.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8ea15f3dae71 -d 2e5bb7e3c040: rebasing 418712:8ea15f3dae71 "Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm r=zombie"
merging toolkit/components/extensions/Extension.jsm
rebasing 418713:51a7f09e1852 "Bug 1379833 Part 2: Display permissions dialog during extension install on Android r=sebastian,walkingice" (tip)
merging mobile/android/app/mobile.js
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/moz.build
merging mobile/android/chrome/content/browser.js
merging mobile/android/chrome/jar.mn
merging mobile/android/locales/en-US/chrome/browser.properties
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8e62c284cf7c -d fc717f341681: rebasing 418713:8e62c284cf7c "Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm r=zombie"
rebasing 418714:0be5cb44d256 "Bug 1379833 Part 2: Display permissions dialog during extension install on Android r=sebastian,walkingice" (tip)
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/moz.build
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c06bc2f7acd3
Part 1: Move common code to format permission dialog strings to Extension.jsm r=zombie
https://hg.mozilla.org/integration/autoland/rev/8bbf531110dd
Part 2: Display permissions dialog during extension install on Android r=sebastian,walkingice
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0cd09b9e7881fd87c7d9ce29e407e029cfcf4b4
Bug 1379833 Part 1: Move common code to format permission dialog strings to Extension.jsm

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cccaa8e877760f9a6f4f0d124ab816290e49f4d
Bug 1379833 Part 2: Display permissions dialog during extension install on Android
I think at this point the patch is already out of sync with permissions on desktop (e.g. the Find API, or bug 1394134).
I assume they're supposed to stay in sync?
(In reply to Francesco Lodolo [:flod] from comment #59)
> I think at this point the patch is already out of sync with permissions on
> desktop (e.g. the Find API, or bug 1394134).
> I assume they're supposed to stay in sync?

For reference, these are the strings missing compared to browser

webextPerms.description.browsingData
webextPerms.description.downloads.open
webextPerms.description.find
webextPerms.description.proxy
Depends on: 1398326
(In reply to Francesco Lodolo [:flod] from comment #60)
> (In reply to Francesco Lodolo [:flod] from comment #59)
> > I think at this point the patch is already out of sync with permissions on
> > desktop (e.g. the Find API, or bug 1394134).
> > I assume they're supposed to stay in sync?
> 
> For reference, these are the strings missing compared to browser
> 
> webextPerms.description.browsingData
> webextPerms.description.downloads.open
> webextPerms.description.find
> webextPerms.description.proxy

Arg, thanks for catching that, I've filed bug 1398327
Though note that the find and browsingData APIs are not supported on Android.  But the other two definitely need to be updated.
Flags: needinfo?(aswan)
Depends on: 1398714
Depends on: 1398831
Depends on: 1399133
Attached image PermissionsDialog.png
This issue is verified as fixed on Fennec 57.0a1 (2017-09-19) under Android 7.1.2

The permission dialog is displayed in the pop-up after the install was initiated.

Please see the attached screenshot.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.