Closed Bug 1547670 Opened 6 years ago Closed 5 years ago

Implement the top sites experiment extension

Categories

(Firefox :: Address Bar, task, P1)

Product:
Firefox
Component:
Address Bar
Type:
task
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Iteration:
70.4 - Aug 19 - Sep 1

People

(Reporter: mikedeboer, Assigned: adw)

References

(
URL
)

Details

Attachments

(2 files, 8 obsolete files)

urlbar_top_sites_experiment-1.0.0.zip v6 (unsigned)
4.14 KB, application/zip
Details
urlbar-top-sites-experiment@shield.mozilla.org-1.0.0-signed.xpi
8.29 KB, application/x-xpinstall
Details
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 70.1 - Jul 8 - 21
Priority: P3 → P1
Summary: Implement the experiment extension → Implement the top sites experiment extension
Iteration: 70.1 - Jul 8 - 21 → 70.2 - Jul 22 - Aug 4
Iteration: 70.2 - Jul 22 - Aug 4 → 70.3 - Aug 5 - 18

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

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

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?

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.

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

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.

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

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

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

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

Flags: needinfo?(mak77)

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.

Flags: needinfo?(mak77)

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

Iteration: 70.3 - Aug 5 - 18 → 70.4 - Aug 19 - Sep 1

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.

Flags: needinfo?(dao+bmo)

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

Flags: needinfo?(dao+bmo)

mythmon, could you sign this for QA testing please?

Attachment #9086183 - Attachment is obsolete: true

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

Whoops, forgot to needinfo

Flags: needinfo?(mcooper)

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.

Flags: needinfo?(mcooper)

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?

Attachment #9086189 - Attachment is obsolete: true
Flags: needinfo?(mcooper)
Attachment #9086190 - Attachment is obsolete: true
Flags: needinfo?(mcooper)

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

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!

Attachment #9086722 - Attachment is obsolete: true
Attachment #9086724 - Attachment is obsolete: true
Flags: needinfo?(mcooper)
Flags: needinfo?(mcooper)

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.

Flags: needinfo?(mixedpuppy)

Comments address on GitHub, thanks Shane.

Flags: needinfo?(mixedpuppy)

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.

Flags: needinfo?(mak77)

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.

Flags: needinfo?(mak77)

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

Flags: needinfo?(rdalal)

Clearing NI, will post new zip shortly

Flags: needinfo?(rdalal)
Attachment #9091803 - Attachment is obsolete: true

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

Flags: needinfo?(rdalal)
Flags: needinfo?(rdalal)
Attachment #9087625 - Attachment is obsolete: true
Attachment #9088230 - Attachment is obsolete: true

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

Flags: needinfo?(rdalal)

The file attached is actually signed for release.

Flags: needinfo?(rdalal)

The experiment is live, marking this as done.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: