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)
WebExtensions
Frontend
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).
Assignee | ||
Comment 1•7 years ago
|
||
(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)
Reporter | ||
Comment 2•7 years ago
|
||
(adding relman as FYI)
Comment 3•7 years ago
|
||
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.
Reporter | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Reporter | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Reporter | ||
Comment 8•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8956317 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30d415ab2378
Show permissions notifications for distribution addons r=kmag
Comment 17•7 years ago
|
||
bugherder |
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
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.
Assignee | ||
Comment 23•7 years ago
|
||
Ugh, from inspection it looks like distribution.ini is processed after the addons manager starts up.
Let me think about this a bit.
Comment 24•7 years ago
|
||
Ok, we'll hold off on testing for now until this issue is resolved.
Assignee | ||
Comment 25•7 years ago
|
||
Taking the follow-ups to bug 1444189
No longer blocks: 1444189
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(ddurst)
You need to log in
before you can comment on or make changes to this bug.
Description
•