Closed Bug 1461805 Opened 3 years ago Closed 3 years ago

New Tab notification should not be shown for partner builds

Categories

(WebExtensions :: Frontend, defect, P3)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox60 unaffected, firefox61+ fixed, firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox60 --- unaffected
firefox61 + fixed
firefox62 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file)

Bug 1461805 - Don't show doorhanger for distribution add-ons.
59 bytes, text/x-review-board-request
mstriemer
: review+
aswan
: review+
RyanVM
: approval-mozilla-beta+
Details 
See Bug #1445630

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

Adding a patch for beta.
[Tracking Requested - why for this release]: Needed for partner builds.
tracking-firefox61: --- → ?
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.
Attachment #8975951 - Flags: review?(mstriemer)
Status: NEW → ASSIGNED
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.");
```
Attachment #8975951 - Flags: review?(mstriemer) → review+
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 = "ext-distribution@mochi.test";
> +  Services.prefs.setCharPref(`extensions.installedDistroAddon.${distId}`, true);

This should use `SpecialPowers.pushPrefEnv()`
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?
> 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 on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

https://reviewboard.mozilla.org/r/244178/#review251114
Attachment #8975951 - Flags: review?(aswan) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/692f80296f22
Don't show doorhanger for distribution add-ons. r=aswan,mstriemer
https://hg.mozilla.org/mozilla-central/rev/692f80296f22
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(mozilla)
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]:
Flags: needinfo?(mozilla)
Attachment #8975951 - Flags: approval-mozilla-beta?
Comment on attachment 8975951 [details]
Bug 1461805 - Don't show doorhanger for distribution add-ons.

Fix for partner builds. Approved for 61.0b8.
Attachment #8975951 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.