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)
Tracking
(firefox57 verified)
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: aswan, Assigned: aswan)
References
Details
Attachments
(7 files, 1 obsolete file)
This bug is to implement the dialog shown in the 3rd panel at: https://docs.google.com/presentation/d/1NSqxUiZDUdFrha0TvuNVEV1v2OqmBPc17XaPbzIwWmQ/edit?ts=595487bf#slide=id.g23ac2aa0df_0_5
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
Markus/Jack, can you guys respond to the issues that gandalf and Pike brought up?
Flags: needinfo?(mjaritz)
Flags: needinfo?(jalin)
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
And, btw, after the mocks I think we can go with the same strings on android and desktop.
Assignee | ||
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8886933 -
Attachment description: permissions.png → Permission dialog on desktop (12 permissions, unverified)
Comment 13•7 years ago
|
||
(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.
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Comment 15•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8893078 -
Flags: feedback?(lgreco)
Assignee | ||
Updated•7 years ago
|
Attachment #8893079 -
Flags: feedback?(lgreco)
Comment 18•7 years ago
|
||
mozreview-review |
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 19•7 years ago
|
||
mozreview-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/#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 20•7 years ago
|
||
mozreview-review |
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).
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
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)
Comment 28•7 years ago
|
||
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
Comment 29•7 years ago
|
||
convert from extension.svg by Android Studio, and change fillColor to green
Comment 30•7 years ago
|
||
A screenshot for VectorDrawable in AlertDialog, on Nexus5 Android 4.4
Assignee | ||
Comment 31•7 years ago
|
||
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.
Assignee | ||
Comment 32•7 years ago
|
||
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)
Comment 33•7 years ago
|
||
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)
Assignee | ||
Comment 34•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8897455 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8893078 -
Flags: review?(tomica)
Attachment #8893079 -
Flags: review?(walkingice0204)
Assignee | ||
Comment 37•7 years ago
|
||
: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)
Comment 38•7 years ago
|
||
Thanks, double checked and I can't spot any issue.
Flags: needinfo?(francesco.lodolo)
Comment 39•7 years ago
|
||
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 40•7 years ago
|
||
mozreview-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 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+
Assignee | ||
Comment 41•7 years ago
|
||
mozreview-review-reply |
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"?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
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 45•7 years ago
|
||
mozreview-review |
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+
Comment 46•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8893079 -
Flags: review?(s.kaspari)
Comment 47•7 years ago
|
||
mozreview-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 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 hidden (mozreview-request) |
Assignee | ||
Comment 49•7 years ago
|
||
mozreview-review-reply |
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.
Comment 50•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 53•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 56•7 years ago
|
||
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
I had to back this out for android lint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=129357059&repo=autoland https://hg.mozilla.org/integration/autoland/rev/e3bd210d60af2c2c23bb0cf8b0f33c7dbe9ec898
Flags: needinfo?(aswan)
Assignee | ||
Comment 58•7 years ago
|
||
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
Comment 59•7 years ago
|
||
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?
Comment 60•7 years ago
|
||
(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
Comment 61•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0cd09b9e788 https://hg.mozilla.org/mozilla-central/rev/3cccaa8e8777
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 62•7 years ago
|
||
(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)
Comment 63•7 years ago
|
||
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
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•