[META] Firefox Engineering Review

biancadanforth commented 4 years ago

Engineering Review Checklist for Firefox WebExtension Experiments

This is a working checklist for reviewing WebExtension experiments in Firefox. Items may be added, edited or removed at any time.

[ ] README:
- [ ] Explains clearly what the extension does for each experimental branch and control.
- [ ] Lists any extension configuration settings useful for testing.
- [ ] Links out to a META bug and/or other documents with more context, including what Firefox channel and version the experiment is targeted for.
[ ] manifest.json contains the following fields:
- [ ] name
- [ ] version
- [ ] author
- [ ] description
- [ ] homepage_url
- [ ] incognito (recommended value: "not_allowed")
- [ ] hidden (value: true)
- [ ] applications.gecko.id (typically of the form "study-name@mozilla.org")
- [ ] applications.gecko.strict_min_version (preferable)
- [ ] applications.gecko.strict_max_version (preferable)
[ ] Extension follows guidelines for modifying Firefox Prefs.
[ ] WE API engineers confirm that the extension's use of WE APIs (public, privileged or experimental) will not trigger known bugs.
[ ] Extension does not cause feature regressions in Try server tests.
[ ] Extension does not cause significant[1] performance regressions.
[ ] Only source files used by the extension are included in the XPI.
[ ] Additional reviews (e.g. data, legal, privacy, security, ...) may be required if:
- [ ] The extension makes use of third party libraries and/or services.
- [ ] The extension modifies web page content in any way.
- [ ] Data is transmitted outside of Mozilla's telemetry pipeline.

[1]: As a reference, for a production application (i.e. not an experiment), a 2-3% worst case scenario regression in page load time would generally be considered acceptable. Since experiments are temporary and typically only go out to a very small percentage of users, we can often be more lenient.

biancadanforth commented 4 years ago

Additional considerations

What the extension needs to handle

I will help you with much of this, but we should keep it in mind.

Encrypt and package telemetry pings.
- TBD if this or some other modification is needed. Pending Pioneer v2 specification.
Check that the user is in the Pioneer cohort if using Pioneer for recruitment.
Record study-specific telemetry.
Implement user consent UI.
Modify behavior in different privacy contexts as needed.

Open questions

What channel and version of Firefox is this shipping in?
What are the eligibility requirements for a user to participate in the study?
- E.g. The user is in an en-us locale, part of the Pioneer cohort, ... Most of these can be handled upstream by Normandy, but some may require the experiment code to check.
From our 11/27 e-mail discussion, the preferred deployment mechanism is to use Normandy with Pioneer. Is our Pioneer cohort large enough to draw from for this study?
Is there a survey?
- If so, how is that going to be integrated with the experiment?
What telemetry do you want to record?
- The Normandy client already records some general telemetry.
Where can users learn more about the study?
- The Options page will not be accessible via about:addons if the study is marked as hidden. One option is a SUMO (support.mozilla.org) page referenced in the study's description in about:studies.
How is the extension supposed to behave in different privacy contexts?

Recommendations

Read through Contributing to the Mozilla code base.
Create a METRICS.md (example) document for any telemetry you want to record.
- Get data collection approval.
Work with the Mozilla project owner (rweiss or rhelmer) to create a tracking ticket for this study (likely in Experimenter or Bugzilla).
Get UX and policy approval of user consent UI and user flow.

biancadanforth commented 4 years ago

@jonathanmayer , I have some initial comments for the extension, and I was hoping to open a PR, merging master into an empty branch to benefit from GitHub's code review UI[1]. The idea is we can close the PR once all the blocking issues are resolved. Does this sound like a good approach to you? If so, could you give me push access to the repo?

[1]: See approach 3.

biancadanforth commented 4 years ago

Also @jonathanmayer , would it be possible to make this repo public? In the meantime, could you give a couple more Mozilla folks read access to the repo: @mixedpuppy and @rpl. They are also Firefox engineers that I am consulting.

jonathanmayer commented 4 years ago

@jonathanmayer , I have some initial comments for the extension, and I was hoping to open a PR, merging master into an empty branch to benefit from GitHub's code review UI[1]. The idea is we can close the PR once all the blocking issues are resolved. Does this sound like a good approach to you? If so, could you give me push access to the repo?

[1]: See approach 3.

Sounds good. Done.

Also @jonathanmayer , would it be possible to make this repo public? In the meantime, could you give a couple more Mozilla folks read access to the repo: @mixedpuppy and @rpl. They are also Firefox engineers that I am consulting.

Added. Will follow up about a public vs. private repo via email.

(Sorry for being slow, I had GitHub notifications turned off. Fixed.)

biancadanforth commented 4 years ago

I have updated the Engineering checklist and the Additional information posts above to reflect my latest (and evolving) understanding of the engineering and feature requirements for the study. Changelogs are below. Please let me know if you have any questions.

Changelog for Engineering Checklist

[12/17/19]

manifest.json contains the following fields:
- Remove icons ("48" and "96")
- Addon studies, when properly hidden, are no longer displayed in about:addons or about:debugging.
- Add incognito key
- This determines how the extension behaves in private browsing windows.
- Add hidden key
- This hides the addon in about:addons and about:debugging after it is signed.
Add "Extension follows guidelines for modifying Firefox Prefs." list item.

Changelog for Additional considerations

[12/17/19]:

What the extension needs to handle
- Add "TBD if this or some other modification is needed. Pending Pioneer v2 specification." under "Encrypt and package telemetry pings.".
- Replace "Check that the user is in the Pioneer cohort." with "Check that the user is in the Pioneer cohort if using Pioneer for recruitment."
- Add "Implement user consent UI."
- Add "Modify behavior in different privacy contexts as needed."
Open questions
- Add "Where can users learn more about the study?
- The Options page will not be accessible via about:addons if the study is marked as hidden. One option is a SUMO (support.mozilla.org) page referenced in the study's description in about:studies."
- Add "How is the extension supposed to behave in different privacy contexts?"
Recommendations
- Add "Get data collection approval."
- Add "Get UX and policy approval of user consent UI and user flow."

jonathanmayer commented 3 years ago

Closing this out, since the (exceedingly helpful) review was of a study version that's now over a year old.

citp / news-disinformation-study