1547670 - Implement the top sites experiment extension

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Mike de Boer [:mikedeboer]]

For more info, see the design doc over at https://docs.google.com/document/d/11c-OTsscmArnqL02Kq1TUPKtkHl4avmC3pDdSJDC6JI/edit

Comment 1

[Drew Willcoxon :adw]

Notes to self from discussion with Verdi:

• Don't show popup on about:newtab
• Sites should use custom titles when they have them
• Should show 8 sites max

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Drew Willcoxon :adw from comment #1)

• Don't show popup on about:newtab

What does this mean? Is it about not showing it automatically when the user focuses the field?
Because we already should not be showing it automatically when opening about:newtab.

Comment 3

[Drew Willcoxon :adw]

When openViewOnFocus is true and you focus the urlbar while the current page is newtab, the popup opens. That shouldn't happen for the top-sites popup, Verdi says, since newtab is already showing your top sites.

Now that I think about it, I guess I'll have to add a tabs listener, listen for active tab switches, and set openViewOnFocus according to whether the active tab is newtab?

Comment 4

[Drew Willcoxon :adw]

Or maybe it should have never been openViewOnFocus, but instead we should fire an event when the urlbar is focused and then you could call an openView function. Sigh.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Drew Willcoxon :adw from comment #3)

When openViewOnFocus is true and you focus the urlbar while the current page is newtab, the popup opens. That shouldn't happen for the top-sites popup, Verdi says, since newtab is already showing your top sites.

And so, what do we show?

Now that I think about it, I guess I'll have to add a tabs listener, listen for active tab switches, and set openViewOnFocus according to whether the active tab is newtab?

The original specs were to open the panel when it was an explicit user focus, and don't open for auto-focus.
This looks like an unneeded complication, considered we don't have alternative content.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

My question is whether that requirement is strictly necessary to confirm experiment success, because we could always fix the newtab page in the product (just not opening on explicit focus if the page is newtab, but retain topsites content) for the final implementation.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

and indeed, even if it's a strict requirement, I'd just fix it in the product, it's much easier.

Comment 8

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] (Away 7-25 Aug) from comment #5)

And so, what do we show?

We don't open the popup at all.

The original specs were to open the panel when it was an explicit user focus, and don't open for auto-focus.
This looks like an unneeded complication, considered we don't have alternative content.

Yeah. One lesson I've learned is that it would be great if we could rapidly prototype experiments (or maybe just all new UX in general) so that Verdi/UX could play with them ASAP. Do that before actually landing anything. There are always gaps between what's spec'ed and what's expected. As it is now, when we're designing and implementing all these APIs, we may end up designing and implementing the wrong thing.

Good point about just fixing it in the product. (You mean modify the behavior of openViewOnFocus, I assume.)

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes. just make it not open the panel if the current page is newtab.

Comment 10

[Drew Willcoxon :adw]

Marco, could you look over the extension code, please? Especially src/background.js. It's pretty small. It's also basically done except for possibly new telemetry, if we end up needing it. But since it's not officially "done," I guess, I'm not looking for a final review, just a first review pass, especially since you'll be on PTO soon. Thanks.

https://github.com/0c0w3/top-sites-experiment

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The "top-sites-experiment" id is a bit generic, I think all of our experiments should start with a "urlbar-" prefix.

Are openViewOnFocus and engagementTelemetry set to false automatically on unload? it looks like background is setting them every time to true, they flip the pref, but I suspect then you should invoke .clear() on uninstall/unload. (I couldn't find any reference to auto-unset on getSettingsAPI... maybe you did?)

I like how simple this ends up being.

Comment 12

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] (Away 7-25 Aug) from comment #11)

The "top-sites-experiment" id is a bit generic, I think all of our experiments should start with a "urlbar-" prefix.

Good point, I renamed it and added "urlbar" and "address bar" to the various descriptions.

Are openViewOnFocus and engagementTelemetry set to false automatically on unload?

Yes, the settings API implementation handles that for us. I tested and also confirmed with Shane. He says there's some complexity when multiple extensions set the same pref:

mixedpuppy: yeah, there is a precedence order based on how the extensions set the setting.
mixedpuppy: iirc
mixedpuppy: and you can determine things like your extension has control, some extension has control, you may gain control

I tested with two extensions (actually the same extension, top sites, with differrent IDs) and the prefs were reset when the second one was unloaded.

Comment 13

[Drew Willcoxon :adw]

Dão, Marco looked over an earlier version of the add-on before he went on vacation. It's now done, and it hasn't changed substantially since then. Most changes are related to properly handling study enrollment/unenrollment and the control branch. Would you mind giving it a final review, please? I've also asked mythmon to look it over.

https://github.com/0c0w3/urlbar-top-sites-experiment

The main file is src/background.js. There's a browser mochitest in tests.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #13)

Dão, Marco looked over an earlier version of the add-on before he went on vacation. It's now done, and it hasn't changed substantially since then. Most changes are related to properly handling study enrollment/unenrollment and the control branch. Would you mind giving it a final review, please? I've also asked mythmon to look it over.

https://github.com/0c0w3/urlbar-top-sites-experiment

The main file is src/background.js. There's a browser mochitest in tests.

I've never written a web extension let alone a study / experiment add-on. With that big caveat, the (un-)enrollement and control branch stuff looks good to me. The test gives me extra confidence.

Comment 15

[Drew Willcoxon :adw]

Attachment: urlbar_top_sites_experiment-1.0.0.zip (unsigned)

mythmon, could you sign this for QA testing please?

Comment 16

[Drew Willcoxon :adw]

Attachment: urlbar_top_sites_experiment-1.0.0.zip v2 (unsigned)

Add applications.gecko to the manifest, and keep browser_specific_settings too

Comment 17

[Drew Willcoxon :adw]

Whoops, forgot to needinfo

Comment 18

[Michael Cooper [:mythmon]]

Note: the first version of this add-on couldn't be signed because it specifies the add-on ID using browser_specific_settings.gecko.id, whereas the signing tools only support application.gecko.id.

Comment 19

[Michael Cooper [:mythmon]]

Attachment: [e-mail]

Comment 20

[Drew Willcoxon :adw]

Attachment: urlbar_top_sites_experiment-1.0.0.zip v3 (unsigned)

mythmon, QA requested that the add-on ID use shield.mozilla.org instead of mozilla.org. That's the only change in this xpi. Could you sign it for testing, please?

Comment 21

[Michael Cooper [:mythmon]]

Attachment: [e-mail]

Comment 22

[Drew Willcoxon :adw]

QA found some bugs that are being tracked over on the GitHub repo: https://github.com/0c0w3/urlbar-top-sites-experiment/issues

Comment 23

[Drew Willcoxon :adw]

Attachment: urlbar_top_sites_experiment-1.0.0.zip v4 (unsigned)

mythmon, could you please sign this for testing again? This fixes some bugs QA found, most notably https://github.com/0c0w3/urlbar-top-sites-experiment/issues/2 which was fixed by https://github.com/0c0w3/urlbar-top-sites-experiment/commit/3c6759b6345034217521e4cb2b370f23559e4337

Thanks!

Comment 24

[Michael Cooper [:mythmon]]

Attachment: [e-mail]

Comment 25

[Drew Willcoxon :adw]

Shane, could you look over this commit to the top-sites urlbar extension please? https://github.com/0c0w3/urlbar-top-sites-experiment/commit/f5e0e282183b1dca7488f21ce54812ec5ec41287

It adds an experimental API for setting default prefs.

mconnor pointed out today that we can fix the safe mode telemetry problem by setting prefs on the default branch instead of the user branch. And we can do that using an experimental API so that we can still ship in 69.

At first I made new BrowserSettings, and in the setCallbacks I manually set the prefs on the default branch and returned {}. That fixed the safe mode problem -- the prefs were set to false in safe mode. But it introduced another problem. When the extension is uninstalled/disabled -- in normal mode -- the prefs are set to false, but on the user branch. And if you then open about:config and clear them, they're reset back to the default -- but the default remains true. (If you then restart the browser, they're false on the user branch but also false on the default branch. So when you clear them again, they go back to the default of false.)

So I just added a setDefaultPref function. On shutdown, the API "clears" any prefs that were set by setting them to their initial default values.

Comment 26

[Drew Willcoxon :adw]

Comments address on GitHub, thanks Shane.

Comment 27

[Drew Willcoxon :adw]

Marco, Shane looked at this, but it would be good if you could too. The background script hasn't changed, except I added an experimental API for the pref handling, so instead of calling browser.urlbar.engagementTelemetry.set() I call browser.experiments.urlbar.engagementTelemetry.set(). Same for openViewOnFocus. The substantive change is the addition of the experimental API, implementation here: https://github.com/0c0w3/urlbar-top-sites-experiment/tree/master/src/experiments/urlbar

If you're OK with this I'll get this signed for testing again and ask QA to look at it again.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We wrote similar code :)
You may clean up a few things, like just defining the Map in the global scope and using the pref as "name" in _getDefaultSettingsAPI, so you don't need 2 arguments, just for the unique id of a pref.
The only thing I'm not sure about is the initial value, you read it from the default branch, I make the api pass it in, because I assumed the default pref may not exist and another experiment may have modified the default value. It may not be a problem, considered we decided to always provide a default for these prefs, and that we're unlikely to run 2 concurrent experiments on the same profile.

It looks good.

Comment 29

[Drew Willcoxon :adw]

Attachment: urlbar_top_sites_experiment-1.0.0.zip v5 (unsigned)

Rehan, could you sign this for testing, please? Thank you.

Comment 30

[Drew Willcoxon :adw]

Clearing NI, will post new zip shortly

Comment 31

[Drew Willcoxon :adw]

Attachment: urlbar_top_sites_experiment-1.0.0.zip v6 (unsigned)

OK, Rehan, could you sign this for testing, please? Thank you.

Comment 32

[Rehan Dalal (Inactive)]

Attachment: [e-mail]

Comment 33

[Drew Willcoxon :adw]

Rehan, could you sign attachment 9091833 [details] (from comment 31) for release, please? Thank you.

Comment 34

[Rehan Dalal (Inactive)]

The file attached is actually signed for release.

Comment 35

[Drew Willcoxon :adw]

The experiment is live, marking this as done.