Closed Bug 1441271 Opened 7 years ago Closed 7 years ago

Force Firefox to show permissions/prompt for add-ons installed by the stub installer on first run

Categories

(WebExtensions :: Frontend, enhancement, P1)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: ddurst, Assigned: aswan)

References

Details

Attachments

(2 files)

This is for prototyping a feature in order to test it with the release audience. We want to show the standard add-on install permissions/prompt doorhanger on first run for add-ons that are currently silently "installed" by the stub installer. To properly gate this not-final-UX, this change should be controlled by a pref (that we will turn off for ESR and distribution builds).
(In reply to David Durst [:ddurst] from comment #0) > To properly gate this not-final-UX, this change should be controlled by a > pref (that we will turn off for ESR and distribution builds). I can make the default setting of the pref on ESR not show the notifications, but what is the default we want for non-ESR? I would assume the default should be don't show the notifications and we flip it in the stub installer? Or do we want the opposite so that all the distributions out there need to flip the pref to disable notifications?
Flags: needinfo?(ddurst)
(adding relman as FYI)
If we could not update all the distributions, that would be helpful. There's also the issue that we don't know how many people are using this in the wild right now (enterprise, etc.) So I'd rather keep the default behavior for the experiment.
So... OFF by default, and we'll set to ON via a distribution.ini in the stub installer (since we're making one for each experimented add-on anyway).
Flags: needinfo?(ddurst)
A few notes as I get into the weeds on this: - If there are multiple distribution addons installed, we'll show the prompts one after another in an arbitrary order - If the user quits the browser without responding to a prompt, the corresponding extension will remain in their profile but disabled - Same thing if the user closes their browser window before we manage to put up the prompt More importantly, do we want a context-free "Add (extension name)?" notification or do we want to put some additional explanatory text in the notification? For example, we use this string in the notification for sideloaded extensions: https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/locales/en-US/chrome/browser/browser.properties#66 I don't think we have an existing string that is appropriate but we can add a new string here if that's desired. :ddurst can you either make a call on that or redirect to the right person?
Flags: needinfo?(ddurst)
For the purposes of this experiment (and because I expect the permissions prompting to change): - multiple: totally fine, because for these, there will be only one addon. If this were a real distribution, where there might be more than one, we'd have this pref'd off anyway. - quits/closes: got it. Good question on the string; NI amyt for that one. (I'm fine with no additional text, knowing that they're getting some context prior to downloading the installer; caveat might be that we should be aware of the way we refer to the add-on in that prior context so it's clear when it shows up again.)
Flags: needinfo?(ddurst) → needinfo?(atsay)
We've got a snag. I believe the issue is described here: https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/base/content/browser.js#183-184 When we start a new profile, we load about:home and focus the URL bar, which suppresses the notification (doorhanger). The timing of trying to get a doorhanger to show up at startup is also turning out to be a PITA but I think the above is the bigger issue. I'm not sure who should evaluate this so throwing the hot potato into durst's lap for now.
Flags: needinfo?(ddurst)
My best idea here (copying from IRC) is to disable the location focus and conditionalize it on the pref setting (since only for this experiment will the pref be "on"). Can't speak to the timing, and I wonder if there's going to be an issue hanging the notification from an empty space?
Flags: needinfo?(ddurst)
Attachment #8956317 - Flags: review?(kmaglione+bmo)
Attached video distro addon prompt.mov (deleted) —
Comment on attachment 8956317 [details] Bug 1441271 Show permissions notifications for distribution addons https://reviewboard.mozilla.org/r/225190/#review231410 This could use a test or two... Can we add some based on the sideload tests? ::: browser/modules/ExtensionsUI.jsm:109 (Diff revision 2) > + let strings = this._buildStrings({ > + addon, > + permissions: addon.userPermissions, > + }); Can we do this after all the `await`s and such, just before the prompt? ::: browser/modules/ExtensionsUI.jsm:119 (Diff revision 2) > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + if (!win) { > + return; > + } > + > + let tabbrowser = win.ownerGlobal.gBrowser; s/.ownerGlobal// Also, I'd rather we call this `gBrowser`. It's not even a <tabbrowser> element anymore. ::: browser/modules/ExtensionsUI.jsm:144 (Diff revision 2) > + } > + > + // If we're at about:newtab and the url bar gets focus, that will > + // prevent a doorhanger from displaying. > + // Our elegant solution is to ... take focus away from the url bar. > + win.ownerGlobal.gURLBar.blur(); s/.ownerGlobal// ::: browser/modules/ExtensionsUI.jsm:148 (Diff revision 2) > + // Our elegant solution is to ... take focus away from the url bar. > + win.ownerGlobal.gURLBar.blur(); > + > + let answer = await this.showPermissionsPrompt(browser, strings, > + addon.iconURL); > + if (answer) { `answer` is a bit vague. Maybe `accepted`?
Attachment #8956317 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8956317 [details] Bug 1441271 Show permissions notifications for distribution addons https://reviewboard.mozilla.org/r/225190/#review231410 We can't use the same technique as the sideload tests since `AddonManagerPrivate.getNewDistroAddons()` returns a Set that is built during browser startup. We could mock that I guess. But I think the important testing here is the timing of what happens during startup, that's not really feasible from a mochitest.
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30d415ab2378 Show permissions notifications for distribution addons r=kmag
Sorry late to the party, but what ddurst said: no additional string needed for this current scope. We will strive to provide adequate context before the user installs Firefox using the stub.
Flags: needinfo?(atsay)
What is the pref to be able to check the doorhanger is available? I'm not seeing it appear, so I want to ensure the value is set to true from the customized installer.
Flags: needinfo?(ddurst)
Flags: needinfo?(aswan)
The preference is called extensions.distroAddons.promptForPermissions This just landed yesterday, do we already have a stub installer that sets up the right distribution.ini etc?
Flags: needinfo?(aswan)
Yeah, I built an installer that writes the distribution.ini and put it up at https://drive.google.com/file/d/1rfSGnF-y8bh1bj9XaphdK0cCFftvxoxt/view. I'm also not seeing the prompt, the extension just gets silently installed as usual. I do see the pref in about:config, so the distribution.ini seems to be working.
Ugh, from inspection it looks like distribution.ini is processed after the addons manager starts up. Let me think about this a bit.
Ok, we'll hold off on testing for now until this issue is resolved.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
QA Contact: gwimberly
Blocks: 1444189
Taking the follow-ups to bug 1444189
No longer blocks: 1444189
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: Toolkit → WebExtensions
Flags: needinfo?(ddurst)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: