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

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Mike Kaply [:mkaply]]

Our partners are reporting that with Firefox 59, they are seeing the "Your New Tab has changed." message.

This message should not display for partner builds. These builds are downloaded with the expectation that the new tab page has changed.

Also, this message is showing for existing users that are upgrading to 59.

If a user already has the extension installed (and has for while), it should not suddenly show this message.

Comment 1

[Mark Striemer [:mstriemer]]

(In reply to Mike Kaply [:mkaply] from comment #0)
> Our partners are reporting that with Firefox 59, they are seeing the "Your
> New Tab has changed." message.
> 
> This message should not display for partner builds. These builds are
> downloaded with the expectation that the new tab page has changed.

Is there an example of one of these browsers I can download somewhere?

> Also, this message is showing for existing users that are upgrading to 59.
> 
> If a user already has the extension installed (and has for while), it should
> not suddenly show this message.

This was discussed in bug 1421802 and we decided that the user should see the doorhanger when they update to 59.

Comment 2

[Mike Kaply [:mkaply]]

Builds that show this problem are here:

http://archive.mozilla.org/pub/firefox/candidates/59.0-candidates/build5/partner-repacks/mailru/v2/

Comment 3

[Andrew Swan [:aswan]]

Mike, what do you want to do there?

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Screenshot of new prompt

After discussion with Kev, we'd like to pursue making this popup more aware of the fact that a distribution was the source of the change (not just an extension).

Comment 5

[Mike Conca [:mconca]]

(In reply to Andrew Swan [:aswan] from comment #3)
> Mike, what do you want to do there?

:aswan see comment 4

Comment 6

[Meridel [:meridel]]

Spoke with Kev about this. In addition to not showing this to partner builds, can we explore routing the user to manage their extensions in Add-ons Manager, rather than disabling the extension? Kev was concerned about making this detonation option so prominent -- if the user selects it, there is no way for the user to know how to reverse that decision. So, copy would be:

Your New Tab Changed

An extension, [extension name], changed the page you see when you open a new tab. Learn more 

Manage Extensions           Keep Changes 


Since this is an interaction issue, bringing Philip into the discussion!

Comment 7

[Meridel [:meridel]]

Per Mark's suggestion, an alternative flow:

Keep Disable Extension button.
If user selects this option, change button copy to: “Re-enable Extension" and link to Add-ons Manager or show iconized instructions - Pref icon > Add-ons Manager icon (which seem less effective to me).  

Detonation is still an option here but then user learns where to manage extensions.

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Simple patch using distro pref

This is a simple patch that uses the existing distro pref mechanism.

We'd probably also want to add in an extra check that it was truly a distribution by looking at distrbution.id or better yet checking some of the other values since those tend not to be used by non partner distros or funnelcakes 

app.distributor=
app.distributor.channel=
app.partner.partnername="partnername"
browser.search.distributionID=
mozilla.partner.id=

Comment 9

[Mike Kaply [:mkaply]]

Comment on attachment 8967525 [details] [diff] [review]
Simple patch using distro pref

Looking for feedback on this approaching.

Also whether it's worth adding an explicit distribution ID check. 

The extensions.installedDistroAddon is only set when distributions install add-ons.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8967525 [details] [diff] [review]
Simple patch using distro pref

Review of attachment 8967525 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see any obvious problems, but a couple of note:
1. This is going to clash with bug 1397809, you should sync with mstrimer
2. The set of installed distribution extensions can't change during a browser session, you should create a lazy getter that just builds that list once the first time it is needed, rather than re-building it every time isDistributionAddon is called

Comment 11

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: This is a big issue for our partners.

Comment 12

[Mike Kaply [:mkaply]]

Attachment: Version that caches

Here's what the caching will look like. I'll wait to land until after bug 1397809 lands.

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 8968335 [details] [diff] [review]
Version that caches

I was wrong, mstriemer's patch isn't going in until 61.

So this will be a beta only patch (since his new patch will change it a bit).

So asking for final review then beta approval,

Andrew: Could you also offer a risk assessment for relman?

Thanks!

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8968335 [details] [diff] [review]
Version that caches

Review of attachment 8968335 [details] [diff] [review]:
-----------------------------------------------------------------

This is not landable as-is since it isn't an exported hg commit...

::: browser/components/extensions/parent/ext-url-overrides.js
@@ +247,5 @@
> +    gDistributionAddonsList = [];
> +    let list = Services.prefs.getChildList(PREF_BRANCH_INSTALLED_ADDON, {});
> +    if (list.length > 0) {
> +      for (let childPref of list) {
> +        gDistributionAddonsList.push(childPref.replace(PREF_BRANCH_INSTALLED_ADDON, ""));

the entire contents of the outer if could be simplified to
```
gDistributionAddonsList = Services.prefs.getChildList(...)
                                  .map(id => id.replace(...));
```

Comment 15

[Andrew Swan [:aswan]]

(In reply to Mike Kaply [:mkaply] from comment #13)
> Andrew: Could you also offer a risk assessment for relman?

The risk here is low since the change is pretty straightforward and confined to the code that handles extension-related notifications for the newtab page.
The patch does not include any tests but the existing notifications are covered by automated tests and I assume there will be manual testing for distribution addons.

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8968335 [details] [diff] [review]
Version that caches

Clearing r? for now, please re-request review for an actual hg commit attachment (or just use reviewboard?)

Comment 17

[Mike Kaply [:mkaply]]

Attachment: Bug 1445630 - Don't show new tab message for distribution add-ons.

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

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8969093 [details]
Bug 1445630 - Don't show new tab message for distribution add-ons.

https://reviewboard.mozilla.org/r/237780/#review243900

::: browser/components/extensions/ext-url-overrides.js:71
(Diff revision 1)
>  
> +let gDistributionAddonsList;
> +
> +function isDistributionAddon(id) {
> +  if (!gDistributionAddonsList) {
> +    gDistributionAddonsList = Services.prefs.getChildList(PREF_BRANCH_INSTALLED_ADDON, {})

what is the second argument here?

Comment 19

[Mike Kaply [:mkaply]]

Comment on attachment 8969093 [details]
Bug 1445630 - Don't show new tab message for distribution add-ons.

https://reviewboard.mozilla.org/r/237780/#review243900

> what is the second argument here?

cargoculting. It appears there's a mix of using it and not using it in the code:

https://searchfox.org/mozilla-central/search?q=getChildList&case=false&regexp=false&path=*.js

I'll remove and open a bug to get some consistency there.

Comment 20

[Julien Cristau [:jcristau] (back April 22)]

Mike says we need this in 60; tracking.

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8969093 [details]
Bug 1445630 - Don't show new tab message for distribution add-ons.

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

Comment 22

[Mike Kaply [:mkaply]]

Comment on attachment 8969093 [details]
Bug 1445630 - Don't show new tab message for distribution add-ons.

Approval Request Comment
[Feature/Bug causing the regression]: Distribution builds show the new tab popup
[User impact if declined]: Asked for new tab even though their build include ones.
[Is this code covered by automated tests?]: No
[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?]: No
[Why is the change risky/not risky?]: See comments from andrew.
[String changes made/needed]:

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

How can that be verified on nightly, it's not landed yet.

Comment 24

[Mike Kaply [:mkaply]]

> How can that be verified on nightly, it's not landed yet.

Sorry, typo. It actually can't be verified on nightly, it's a different patch.

Comment 25

[Mike Kaply [:mkaply]]

Sorry, I'm being to terse in my comments here.

This prompt was originally only for the new tab. Bug 1397809 was created to use this prompt for the homepage as well.

I originally thought bug 1397809 was going to be uplifted to 60, so I held off on checking my patch into central. I then learned it was 61 only, but by then the code had landed in 61, so this patch became obsolete on central.

So this patch fixes the problem on beta for the new tab. I'll be writing a new patch today in response to 1397809 that handles this on central for new tab and homepage.

So this is a beta only patch because of bug 1397809 which moved the code to another file.

Hope that makes the situation more clear.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's uplift this for beta 16, and verify the fix in beta. 

Mike, can you describe how to test this with a partner build and fill out a PI request? We also might want to confirm that the new tab page and the warning work normally in non-partner builds.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8969093 [details]
Bug 1445630 - Don't show new tab message for distribution add-ons.

Fix for a regression from 59. This should only affect partner builds.

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4153d5337260

Comment 29

[Mike Kaply [:mkaply]]

To verify this in a partner build, all you have to do is install this build:

archive.mozilla.org/pub/firefox/candidates/60.0b16-candidates/build1/partner-repacks/mailru/mailru/v1/

when it comes out and verify that the new popup doesn't appear when new tab is opened. To see the old behavior, you can install this build:

archive.mozilla.org/pub/firefox/candidates/60.0b15-candidates/build1/partner-repacks/mailru/mailru/v1/

For non partner builds, just installing an addon like Tabby Cat and opening the new tab is enough.

I'll open a pi-request.

Comment 30

[Oana Botisan, Desktop QA]

*Partner builds:

 - I manage to reproduce the issue using the build archive.mozilla.org/pub/firefox/candidates/60.0b15-candidates/build1/partner-repacks/mailru/mailru/v1/ on Windows 10 x64. The doorhanger appeared every time a new tab was opened in a session.

 - I retested using the build archive.mozilla.org/pub/firefox/candidates/60.0b16-candidates/build1/partner-repacks/mailru/mailru/v1/ on the same platform and the bug is not reproducing anymore.
  
*Non-partner builds:

 - I tested on beta 60.0b15, beta 60.0b16 and latest Nightly using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 and got the same behaviour on all the builds. 

 - I tested this by installing the Tabby Cat add-on. The doorhanger is displayed when you open the first time a new tab in a session. On the other hand, if you close and open Firefox with the same profile, the doorhanger is still present in the first new tab opened. 

Is that behaviour expected?

Comment 31

[Mike Kaply [:mkaply]]

> On the other hand, if you close and open Firefox with the same profile, the doorhanger is still present in the first new tab opened. 

The doorhanger continues to show until you interact with it. Once you make a selection, it won't show again.

Comment 32

[Oana Botisan, Desktop QA]

I got the same results. I just wanted to make sure they are the correct ones. 
Thank you for your answer.

Comment 33

[Mike Kaply [:mkaply]]

I'm marking this bug fixed against beta since we can't do another mozreview patch.

I'll clone to a patch for nightly/61.

Comment 34

[marius.santa]

Adding the “qe-verify-“ flag as per https://bugzilla.mozilla.org/show_bug.cgi?id=1445630#c22