1461805 - New Tab notification should not be shown for partner builds

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Mike Kaply [:mkaply]]

See Bug #1445630

The patch that was done there was only good on beta due to 1397809.

Adding a patch for beta.

Comment 1

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: Needed for partner builds.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Bug 1461805 - Don't show doorhanger for distribution add-ons.

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

Comment 3

[Mark Striemer [:mstriemer]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

https://reviewboard.mozilla.org/r/244178/#review250134

::: browser/components/extensions/ExtensionControlledPopup.jsm:120
(Diff revision 1)
>      return Services.wm.getMostRecentWindow("navigator:browser");
>    }
>  
> +  isDistributionAddon(id) {
> +    if (!this.distributionAddonsList) {
> +      this.distributionAddonsList = Services.prefs.getChildList(PREF_BRANCH_INSTALLED_ADDON)

Can you make this a `Set`? Hopefully it isn't a big list or it's empty but a `Set` could be faster still.

::: browser/components/extensions/ExtensionControlledPopup.jsm:128
(Diff revision 1)
> +    return this.distributionAddonsList.includes(id);
> +  }
> +
>    userHasConfirmed(id) {
> +    // We don't show a doorhanger for distribution installed add-ons.
> +    if (this.isDistributionAddon(id)) {

Can you add a unit test for this? You should be able push the pref, install an extension and check that `new ExtensionControlledPopup({confirmedType}).userHasConfirmed(extensionId)` is true.

Should be fine to add it to https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ExtensionControlledPopup.js in by mind.

Comment 4

[Mike Kaply [:mkaply]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

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

Comment 5

[Mark Striemer [:mstriemer]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

https://reviewboard.mozilla.org/r/244178/#review250472

This looks good to me now with the nits here fixed. I think you'll want to run it past aswan, though.

::: browser/components/extensions/ExtensionControlledPopup.jsm:120
(Diff revision 2)
>      return Services.wm.getMostRecentWindow("navigator:browser");
>    }
>  
> +  isDistributionAddon(id) {
> +    if (!this.distributionAddonsList) {
> +      this.distributionAddonsList = new Set(Services.prefs.getChildList(PREF_BRANCH_INSTALLED_ADDON)

This is pretty long, can you wrap it earlier?

```
this.distributionAddonsList = new Set(
  Services.prefs.getChildList(PREF_BRANCH_INSTALLED_ADDON)
    .map(id => id.replace(PREF_BRANCH_INSTALLED_ADDON, "")));
```

::: browser/components/extensions/test/browser/browser_ExtensionControlledPopup.js:242
(Diff revision 2)
> +  await extension.startup();
> +
> +  let confirmedType = "extension-controlled-confirmed";
> +  is(new ExtensionControlledPopup({confirmedType}).userHasConfirmed(id), true, "The popup has been disabled.");
> +  Services.prefs.clearUserPref(`extensions.installedDistroAddon.${id}`);
> +

Can you also test the other case and wrap the assertion so it isn't so long.

```
let userExtension = ExtensionTestUtils.loadExtension({
  manifest: {
    name: "User installed",
  },
});
await userExtension.startup();

// Wrapping like this is probably good, too.
is(new ExtensionControlledPopup({confirmedType}).userHasConfirmed(id),
   false, "The popup has been disabled.");
```

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

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

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

https://reviewboard.mozilla.org/r/244178/#review250524

The title of the bug is about "New Tab notification" but this affects *all* users of ExtensionControlledPopup including things like tab hiding, is that intended?

::: browser/components/extensions/ExtensionControlledPopup.jsm:119
(Diff revision 3)
> +    if (!this.distributionAddonsList) {
> +      let addonList = Services.prefs.getChildList(PREF_BRANCH_INSTALLED_ADDON)
> +                              .map(id => id.replace(PREF_BRANCH_INSTALLED_ADDON, ""));
> +      this.distributionAddonsList = new Set(addonList);
> +    }

This could just be a top-level lazy getter, but this is fine as-is if you don't want to bother.

::: browser/components/extensions/test/browser/browser_ExtensionControlledPopup.js:229
(Diff revision 3)
> +/*
> + * This function is a unit test for distributions disabling the ExtensionControlledPopup.
> + */
> +add_task(async function testDistributionPopup() {
> +  let distId = "[e-mail]";
> +  Services.prefs.setCharPref(`extensions.installedDistroAddon.${distId}`, true);

This should use `SpecialPowers.pushPrefEnv()`

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

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

Comment 9

[Andrew Swan [:aswan]]

The revision looks good to me, but waiting for an answer to:

> The title of the bug is about "New Tab notification" but this affects *all*
> users of ExtensionControlledPopup including things like tab hiding, is that
> intended?

Comment 10

[Mike Kaply [:mkaply]]

> The title of the bug is about "New Tab notification" but this affects *all*
> users of ExtensionControlledPopup including things like tab hiding, is that
> intended?

Yes for homepage and new tab, Not for tab hiding, but there is no good way to exclude it.

No distribution add-ons currently do tab hiding and after discussion with mstriemer, we're not going to allow it as a matter of policy for distributions.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

https://reviewboard.mozilla.org/r/244178/#review251114

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/692f80296f22
Don't show doorhanger for distribution add-ons. r=aswan,mstriemer

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/692f80296f22

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 15

[Mike Kaply [:mkaply]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

Approval Request Comment
[Feature/Bug causing the regression]: Block doorhanger for partner builds
[User impact if declined]: Partner builds see door hanger
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Very low
[Why is the change risky/not risky?]: Affects partner builds only. Has test
[String changes made/needed]:

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

Fix for partner builds. Approved for 61.0b8.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c23e9fb230f