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

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P1)

Description

[David Durst [:ddurst]]

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).

Comment 1

[Andrew Swan [:aswan]]

(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?

Comment 2

[David Durst [:ddurst]]

(adding relman as FYI)

Comment 3

[Mike Kaply [:mkaply]]

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.

Comment 4

[David Durst [:ddurst]]

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).

Comment 5

[Andrew Swan [:aswan]]

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?

Comment 6

[David Durst [: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.)

Comment 7

[Andrew Swan [:aswan]]

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.

Comment 8

[David Durst [: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?

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1441271 Show permissions notifications for distribution addons

As described in the bug, this is intended as a temporary solution to
enable some experiments.  If this becomes a real feature, UX will
put some thought into a better startup experience.

Review commit: https://reviewboard.mozilla.org/r/225190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225190/

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8956317 [details]
Bug 1441271 Show permissions notifications for distribution addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225190/diff/1-2/

Comment 11

[Andrew Swan [:aswan]]

Attachment: distro addon prompt.mov

Comment 12

[Kris Maglione [:kmag]]

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`?

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8956317 [details]
Bug 1441271 Show permissions notifications for distribution addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225190/diff/2-3/

Comment 14

[Andrew Swan [:aswan]]

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 15

[Andrew Swan [:aswan]]

Comment on attachment 8956317 [details]
Bug 1441271 Show permissions notifications for distribution addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225190/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30d415ab2378
Show permissions notifications for distribution addons r=kmag

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/30d415ab2378

Comment 18

[Amy Tsay [:amyt]]

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.

Comment 19

[u554753]

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.

Comment 20

[u554753]

Per Comment 19.

Comment 21

[Andrew Swan [: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?

Comment 22

[Molly Howell (she/her) (no longer active)]

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.

Comment 23

[Andrew Swan [:aswan]]

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

[u554753]

Ok, we'll hold off on testing for now until this issue is resolved.

Comment 25

[Andrew Swan [:aswan]]

Taking the follow-ups to bug 1444189