Closed Bug 1231792 Opened 4 years ago Closed 4 years ago

Experiment: Add bookmarks/history to 3-dot menu

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 + fixed
firefox46 + verified
firefox47 + verified
firefox48 --- verified
relnote-firefox --- 46+
fennec 46+ ---

People

(Reporter: mcomella, Assigned: Margaret)

References

Details

Attachments

(2 files, 4 obsolete files)

So they're easier to find.
Bug 1231792 - Add bookmarks & history list menu item strings. r=liuche
Anthony gave the okay irl and is okay to land it.

Margaret, do you feel we should we A/B test it? Then again, what would our metrics be? Counts of uses of these panels?

Still TODO: add telemetry.
Flags: needinfo?(margaret.leibovic)
(In reply to Michael Comella (:mcomella) from comment #3)
> Anthony gave the okay irl and is okay to land it.
> 
> Margaret, do you feel we should we A/B test it? Then again, what would our
> metrics be? Counts of uses of these panels?

A/B would be showing these items or not? That could be an interesting test. And yes, the metrics would be how often users are on these panels. We do have telemetry probes already in place to tell us how often people visit different panels.

> Still TODO: add telemetry.

Let's definitely land telemetry when we land this. Or will this automatically have telemetry from our current menu telemetry?

I think we should hold off on landing this until after the merge, or land it behind a Nightly only flag.
Flags: needinfo?(margaret.leibovic)
https://reviewboard.mozilla.org/r/27579/#review24865

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3311
(Diff revision 1)
> +        }

Why do the other code blocks return true, but yours do not. Returning early seems good.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3317
(Diff revision 1)
> +

This block should be wrapped in a Switchboard block to enable A/B testing and stagged rollout.
(In reply to :Margaret Leibovic from comment #5)

> > Still TODO: add telemetry.
> 
> Let's definitely land telemetry when we land this. Or will this
> automatically have telemetry from our current menu telemetry?

We will get telemetry from the menu code.

> I think we should hold off on landing this until after the merge, or land it
> behind a Nightly only flag.

Using Switchboard means it will be disabled until we turn it on for Nightly.
NI-ing Barbara here for context
Flags: needinfo?(bbermes)
Duplicate of this bug: 795331
Good stuff also in regards to A/B as this will be a different way of testing a feature and its success.

I assume we should work with Alex to fill out one of his A/B testing templates. Alex, do you want to do that together?
Flags: needinfo?(bbermes) → needinfo?(adavis)
Hm... From a UX point of view it's an obvious quick win though. :) This is something I've seen an appetite for in the wild and something we've noticed from our competitive analysis.

Do we need to A/B test this?

Or, maybe we could land it behind a flag for now and get telemetry?
Getting a useful A/B test out of this requires thought.

Note that using SwitchBoard for staged rollouts is still useful! Measuring is still useful! Telemetry is great! But it's not an A/B test, and we don't have to A/B test things just for fun. It has a cost.


We can test absolute usage through normal UI telemetry.

It's not all that useful measuring how much time users spend on the bookmarks panel; time on the bookmarks panel isn't valuable. We can assume that users will spend _some_ amount of additional time if we add a menu item, precisely because we can only get a positive number of clicks.

If we're A/B testing bookmark discovery we need:

* A useful definition of success. I'm not convinced that "visits to bookmarks panel" is that. Perhaps zero-to-tap-bookmark for affected subsets of sessions is? Is there a way we can measure retention for this population? Do users with the menu create or visit more bookmarks? Do they do so more than people who *do* use bookmarks already via swiping to the panel? (We think that bookmarks are hard to find, not just high-friction.)

* Useful alternatives to compare so we can understand a baseline. "No menu item" isn't really a useful alternative -- we're confident that the menu item will be a discoverability improvement, so we won't learn anything.
(In reply to Anthony Lam (:antlam) from comment #11)
> Hm... From a UX point of view it's an obvious quick win though. :) This is
> something I've seen an appetite for in the wild and something we've noticed
> from our competitive analysis.
> 
> Do we need to A/B test this?
> 
> Or, maybe we could land it behind a flag for now and get telemetry?

To be clear, I would want us to land this behind a switchboard "flag", not a Nightly flag. This will allow us to do a staged rollout and gather feedback.

So far our main A/B test metric has been retention, which means we'd be asking "do these extra menu items lead to increased retention?"

I'm not sure whether we need an A/B test for these items, but a staged rollout would be great. We have general telemetry for menu items, so I don't think we'll need any additional probes here.

I think we should land this with switchboard controls in place, uplift it to beta, and see what a few beta users do when it's enabled.
https://reviewboard.mozilla.org/r/27579/#review25581

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3192
(Diff revision 1)
>          return true;

Probably add something like:

if (SwitchBoard.isInExperiment(this, "panelmenuitems") == false) {
  bookmarksList.setVisible(false);
  historyList.setVisible(false);
}

Where "panelmenuitems" is the experiment name we need to add to mozilla-switchboard
(In reply to Barbara Bermes [:barbara] from comment #10)
> Good stuff also in regards to A/B as this will be a different way of testing
> a feature and its success.
> 
> I assume we should work with Alex to fill out one of his A/B testing
> templates. Alex, do you want to do that together?

Yes Barbara, I think we should continue to work on these together.
Flags: needinfo?(adavis)
I'll setup a meeting in the new year for us to fill out one of your templates.

Invite coming soon.
For measuring success, I would look at:
-how many people see each variation (so we can establish rates and make sure both variations are being distributed properly)
-how many times does history and bookmarks get used per variation (grand total). We will be able to see if users are using the features more.
-for version B, how many times do people access bookmarks and history via menu vs panels when they have the choice. This will allow us to see if we are cannibalizing or reaching out to more users.
-Number of times people bookmark. If accessing bookmarks is easier, perhaps people will bookmark more.

If we can get these by unique users and absolute totals (count), it would give us the full portrait. But if we just have counts, we should still be able to determine success. We just won't know if more people are using, but rather that it is being used more.

With this type of A/B test, we need to be sure that a user that is exposed to a specific variation continues to see only that variation. This is less important in firstrun since everyone just sees it once but for a feature, the UX needs to be consistent for the length of the test. Does Switchboard properly manage this or did make sure of this?
Flags: needinfo?(mark.finkle)
(In reply to Alex Davis [:adavis] from comment #17)
> For measuring success, I would look at:
> -how many people see each variation (so we can establish rates and make sure
> both variations are being distributed properly)
> -how many times does history and bookmarks get used per variation (grand
> total). We will be able to see if users are using the features more.
> -for version B, how many times do people access bookmarks and history via
> menu vs panels when they have the choice. This will allow us to see if we
> are cannibalizing or reaching out to more users.
> -Number of times people bookmark. If accessing bookmarks is easier, perhaps
> people will bookmark more.

We have all of these metrics, via UI Telemetry

> If we can get these by unique users and absolute totals (count), it would
> give us the full portrait. But if we just have counts, we should still be
> able to determine success. We just won't know if more people are using, but
> rather that it is being used more.

We only have absolute counts, via UI Telemetry.

> With this type of A/B test, we need to be sure that a user that is exposed
> to a specific variation continues to see only that variation. This is less
> important in firstrun since everyone just sees it once but for a feature,
> the UX needs to be consistent for the length of the test. Does Switchboard
> properly manage this or did make sure of this?

Yes. Switchboard manages this via it's segmentation method.
Flags: needinfo?(mark.finkle)
Sounds good. 
When it gets into nightly, let's make it a habit to just double check the tracking and make sure everything is there. We had a bit of trouble with the telemetry in our firstrun A/B test so we should approach with additional caution.
Let's see if I find time to update this patch before mcomella gets back to it.
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #20)
> Let's see if I find time to update this patch before mcomella gets back to
> it.

Something else to remember: start a UI Telemetry Session with the experiment name if a person uses either of the menus. We should end the session when they switch home panels [1] (or close the homepanels). You should be able to stop the session regardless if it's active or not. We ignore the call if the session is not active.

Example of starting an experiment session [2]

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#525
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPagerConfig.java#30
liuche said she would look at carrying this across the finish line.

This has strings, so let's try to get it landed by the end of next week to avoid missing Fx46.

Alternately, could we just use the same strings that we use for the home panel titles? Yes, they appear in different contexts, but they are very directly related. If we did this, then we could even uplift this change.
Assignee: michael.l.comella → liuche
Flags: needinfo?(margaret.leibovic)
Let's try to get this in 46.
tracking-fennec: --- → 46+
Zoink!
Assignee: liuche → margaret.leibovic
This change also depends on changes to the switchboard server config.

Review commit: https://reviewboard.mozilla.org/r/31785/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31785/
Attachment #8710471 - Flags: review?(liuche)
PR for the switchboard config changes: https://github.com/mozilla-services/switchboard-experiments/pull/1
https://reviewboard.mozilla.org/r/31785/#review28511

I only looked at some of the experiement aspects.

::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:532
(Diff revision 1)
> +            Telemetry.startUISession(TelemetryContract.Session.EXPERIMENT, SwitchboardExperiments.BOOKMARKS_HISTORY_MENU);

I think you meant Telemetry.stopUISession

I am trying to think of how we want to analyze these types of experiment. Creating a session tags all events with that session name.

What you have here should work OK. We'll be able to see any clicks or loadurls on the Bookmark or History panel that happened if the user displayed the panel via the menu. Makes sense.

::: mobile/android/base/java/org/mozilla/gecko/util/SwitchboardExperiments.java:9
(Diff revision 1)
> + * https://github.com/leibovic/switchboard-experiments

Use the real repo?

::: mobile/android/base/java/org/mozilla/gecko/util/SwitchboardExperiments.java:11
(Diff revision 1)
> +public class SwitchboardExperiments {

Could just be "Experiments" too
Attachment #8697373 - Attachment is obsolete: true
Attachment #8697374 - Attachment is obsolete: true
Comment on attachment 8710471 [details]
MozReview Request: Bug 1231792 - Add Bookmarks and History 3-dot menu items. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31785/diff/1-2/
Comment on attachment 8710470 [details]
MozReview Request: Bug 1231792 - Add bookmarks & history list menu item strings. r=liuche

https://reviewboard.mozilla.org/r/31783/#review28727
Attachment #8710470 - Flags: review?(liuche) → review+
Comment on attachment 8710471 [details]
MozReview Request: Bug 1231792 - Add Bookmarks and History 3-dot menu items. r=liuche

https://reviewboard.mozilla.org/r/31785/#review28739

This looks good to me - is there a follow-up to turn this on server-side?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:65
(Diff revision 2)
> -import org.mozilla.gecko.util.ActivityUtils;
> +import org.mozilla.gecko.util.*;

Our Java style doesn't support * imports - is this intentional?
Attachment #8710471 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #32)
> Comment on attachment 8710471 [details]
> MozReview Request: Bug 1231792 - Add Bookmarks and History 3-dot menu items.
> r=liuche
> 
> https://reviewboard.mozilla.org/r/31785/#review28739
> 
> This looks good to me - is there a follow-up to turn this on server-side?

That's the PR I linked above:

https://github.com/mozilla-services/switchboard-experiments/pull/1

> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:65
> (Diff revision 2)
> > -import org.mozilla.gecko.util.ActivityUtils;
> > +import org.mozilla.gecko.util.*;
> 
> Our Java style doesn't support * imports - is this intentional?

My IDE this this. Probably because we're actually using everything in the package. I can revert it manually, but this probably doesn't have any real performance impact (I believe that's the reason to avoid global imports).
I landed a new version of the patch that doesn't add new strings, so that we can uplift this.

The behavior is controlled by switchboard, so we could uplift this all the way to 45 and turn it on/off remotely.
Attachment #8710470 - Attachment is obsolete: true
Attachment #8710471 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2357fa4fe0a8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1243148
Comment on attachment 8711656 [details] [diff] [review]
patch that landed

This is breaking new ground for us - this feature is controlled by a switchboard flag, so by default, uplifting this patch won't have any affect on the UI. When we want to enable this on aurora/beta, we'll need to land a change to our switchboard experiments config:
https://github.com/mozilla-services/switchboard-experiments

I will work to create a process around this, but the policy should be that we won't make an experiment changes on aurora/beta/release without approval from relman and product.

Approval Request Comment
[Feature/regressing bug #]: None.

[User impact if declined]: Hard to access history and bookmarks.

[Describe test coverage new/current, TreeHerder]: No automated test coverage, tested locally.

[Risks and why]: Low-risk, this adds two simple menu items, and it is disabled by default. There may be some risk around the fact that we're using Switchboard to control this, but I want to take this risk now, rather than when we have a more complex feature we absolutely must ship.

[String/UUID change made/needed]: None.
Attachment #8711656 - Flags: approval-mozilla-beta?
Attachment #8711656 - Flags: approval-mozilla-aurora?
Adding tracking flags (as I think we should track all experimental features)
Comment on attachment 8711656 [details] [diff] [review]
patch that landed

UI feature work, disabled by default, for switchboard experiment.
Please uplift to aurora and beta. (Doing this while Sylvestre is on PTO, so it can make it to the Beta 2 build on Monday)
Attachment #8711656 - Flags: approval-mozilla-beta?
Attachment #8711656 - Flags: approval-mozilla-beta+
Attachment #8711656 - Flags: approval-mozilla-aurora?
Attachment #8711656 - Flags: approval-mozilla-aurora+
Depends on: 1247366
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Add “History” and “Bookmarks” to Menu
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
We have already this:
'Optimized & re-organized "Settings" under "Menu"'
in the release notes
(In reply to Sylvestre Ledru [:sylvestre] from comment #45)
> We have already this:
> 'Optimized & re-organized "Settings" under "Menu"'
> in the release notes

This is a different feature, and one that will be in Firefox 46, not 45.

The settings reorg feature in 45 is about the settings, not the menu itself (maybe we should update the wording for that to be less confusing?).
My bad, merci Margaret!
Adding a relnote for 46 beta with the wording in comment 44.
Depends on: 1253588
We need to actually make a change to our switchboard config for this to appear in beta, I just filed bug 1253588 about that.
QA Contact: mihai.ninu
Depends on: 1257513
Verified as implemented on all versions : 48.0a1, 47.0a2, and 46.0b2
Depends on: 1263941
You need to log in before you can comment on or make changes to this bug.