1293181 - Opening Sync Prefs may specify an entry-point of "uitour" instead of the actual entry-point

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

via bug #1293128:

> * Start a profile that's not logged in to sync
> * Click on the "synced tabs" toolbar button; since no devices 
> are connected you should get a "sign in the sync" solicitation
> * Click on "sign in to sync"
> 
> Actual Result:
>
> You wind up at the sync preferences pane with entrypoint=uitour:

For some reason, Ryan is seeing entrypoint=uitour for most entry-points into Sync prefs (apart from the sync button in the hamburger menu, but that's subtly different.

Ryan started with a new profile, and for some reason the code in browser-syncui believes a UITour is in progress - even after a restart.

IOW:
* Bug 1293128 is tracking that "menupanel" is used when opening prefs from the SyncedTabs button.

* This bug is tracking that most entry-points sometimes use "uitour" when there's no apparent UITour in progress.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

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

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

Matt/Jaws,
 We've found that the existing code for checking if a UITour is "active" returns many false positives - eg, starting on about:home, then moving to some other page - the old code still thought we were in a UITour, even though no tour was actually started.

The intent of the code was always "if a visible tour underway" and I think this patch captures this. Alternatively, seeing the existing tours don't actually do anything Sync specific, we could consider dropping it completely - but I think this is OK and should work if we reinstate the tour for Sync later.

I pinged jaws on IRC and he said he'd need to think a little more and that maybe Matt had thoughts - so I flagged you both for review - but I'll take it from either ;)

Thanks!

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

https://reviewboard.mozilla.org/r/70180/#review67480

::: browser/base/content/browser-syncui.js:299
(Diff revision 1)
> -      if (UITour.tourBrowsersByWindow.get(window) &&
> -          UITour.tourBrowsersByWindow.get(window).has(gBrowser.selectedBrowser)) {
> +      // (UITour has grown to be more than just a tour, but for the purposes
> +      // of this code, we want to know "are we currently in a tour?")
> +      // The best indicator for this seems to be "is UITour highlighting
> +      // anything?"
> +      try {
> +        if (window.document.getElementById("UITourHighlight").getAttribute("active")) {

I'm not a fan of this as it's relying on even more internal details of UITour and it only handles highlights which aren't necessarily shown at all times in a tour e.g. showMenu/showInfo could be used instead. This is really just paving over bug 1123010 which IMO is the proper fix and this code would stay as-is.

I can take a stab at fixing that bug now since I don't think there's major tour development going on now.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

https://reviewboard.mozilla.org/r/70180/#review67480

> I'm not a fan of this as it's relying on even more internal details of UITour and it only handles highlights which aren't necessarily shown at all times in a tour e.g. showMenu/showInfo could be used instead. This is really just paving over bug 1123010 which IMO is the proper fix and this code would stay as-is.
> 
> I can take a stab at fixing that bug now since I don't think there's major tour development going on now.

If we don't want to count about:home as a tour even though it uses UITour (and perhaps the active snippet talks about sync) then we can exclude that here.

Comment 5

[Edwin Wong [:edwong]]

Nominate for P1 in iteration 51.2

Comment 6

[Lina Butler [:lina]]

Alex, we can also consider removing `uitour` in the short term. WDYT?

Comment 7

[Alex Davis [:adavis]]

Ultimately, the reason we want to fix this is to properly track the entrypoint and to measure the number of new accounts we are creating via Synced Tabs. From what I understand, removing `uitour` wouldn't help us more. We still wouldn't be able to track new accounts referred by this feature in FxA metrics.

Kit, let me know if I've properly understood everything here.

If so, removing it won't resolve the problem.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Alex Davis [:adavis] from comment #7)
> Ultimately, the reason we want to fix this is to properly track the
> entrypoint and to measure the number of new accounts we are creating via
> Synced Tabs. From what I understand, removing `uitour` wouldn't help us
> more. We still wouldn't be able to track new accounts referred by this
> feature in FxA metrics.

It depends on what you mean by "this feature" here. If you mean "the uitour feature", that's correct, but IIUC is rarely used. If you mean "the synced tabs" feature, then I think removing it *would* help us.

IOW:
1) current state is that uitour detection code is buggy, so often we will specify "uitour" instead of something more concrete, such as "synced-tabs".
2) removing the special casing for uitour would mean we suddenly *would* be getting, say, "synced-tabs", but would not know if the user found it by themselves or due to a real UITour prompting them to find and discover it.

I'm not sure we are getting much value from the "uitour" entry-point at the moment, so removing that while it remains buggy would make the entrypoint more accurate than it is today (but not totally accurate, as it would *never* report uitour in the very few cases when it should)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(another possibility is that we treat uitour as a different param - IOW, the urls become something like "https://blah?entrypoint=synced-tabs&uitour=true" - but I've no idea how that would fit with the back-end metrics generation.)

Comment 10

[Alex Davis [:adavis]]

When I look at fxa signup from UItour in our stats, it seems non-negligible.

If we remove uitour from "the uitour feature", I'd be worried that we increase our unknowns source by 50%. It would allow us to better track synced-tabs which is great but I'm not really pleased by the trade off.

@rfk I'm a little bit at a lost here. Do you have any recommendations on how we could accurately continue to measure sign-ups from ui-tour and to also measure fxa sign-ups from synced tabs within the limits of this technical challenge? I want to make sure we continue to have accurate metrics in FxA.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to Alex Davis [:adavis] from comment #10)
> When I look at fxa signup from UItour in our stats, it seems non-negligible.

That's basically my point - it is non-negligible, and currently not accurate - it will report uitour for many actions that have nothing to do with the uitour.

> If we remove uitour from "the uitour feature", I'd be worried that we
> increase our unknowns source by 50%.

Note I'm not suggesting we *remove* uitour - there are things that explicitly say "uitour" and they would remain.

However, there are other parts of the code that explicitly say "sync-button" - however, there is faulty code that *replaces* sync-button with uitour as it believes a uitour is in progress. However, it typically is not in progress.

IOW, with the change I'm proposing, many URLs that currently say "entrypoint=uitour" would change to, say, "entrypoint=sync-button". In no cases would a URL that currently says "entrypoint=uitour" have the entrypoint removed entirely.

Adding needinfo back as there still seems a little confusion here - maybe we should have a quick vidyo to clarify?

Comment 12

[Ryan Kelly [:rfkelly]]

clearing my ni? pending further discussion of Mark's point above

Comment 13

[Matthew N. [:MattN]]

FWIW, I started fixing bug 1123010 (the ideal fix) around a month ago after talking to markh but won't have time to finish it this quarter. I will attach the WIP patch there in case someone wants to finish it.

Comment 14

[Alex Davis [:adavis]]

I spoke to Mark and I agree with his proposed solution that MattN is working on.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

To be clear, we agreed that in the short term, we'd just remove the attempt to second-guess that a UITour is in progress. This is safe given no such tours are active, and we could uplift this simple patch as it increases the accuracy of our engagement metrics. Matt's patch can then progress at its own pace, at which point we will re-add the second-guessing and let that ride the trains (or uplift that too if Matt decides to uplift his).

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

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

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

https://reviewboard.mozilla.org/r/70180/#review79084

LGTM! Should we remove http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/browser/base/content/browser-fxaccounts.js#348-354, too?

Comment 18

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #17)
> LGTM! Should we remove
> http://searchfox.org/mozilla-central/rev/
> 8910ca900f826a9b714607fd23bfa1b37a191eca/browser/base/content/browser-
> fxaccounts.js#348-354, too?

Great catch, thanks!

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

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

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8779138 [details]
Bug 1293181 - remove the override of entrypoint=uitour for FxA-server collected metrics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70180/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41e054b0c963
remove the override of entrypoint=uitour for FxA-server collected metrics. r=kitcambridge

Comment 22

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/41e054b0c963