1101790 - UITour: ability to set prefs and data in FHR

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

We want the UITour page to be able to do something like:

setTreatmentTag(name, value);
var tag = getTreatmentTag(name);

So that we can later determine which version of the tour the user saw, and have that influence behavior going forward.

The setTreatmentTag function should record the arbitrary key value pair in prefs (e.g. in a browser.uitour.treatmenttag.<name> = value), and should also be recorded in FHR (not sure how to do that exactly).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP patch

Benjamin, can you comment on the "FHR stuff" that's needed here?

Comment 2

[Benjamin Smedberg]

Here's what I think we want:

When the tour calls setTreatmentTag(name, value) we record the following in the current FHR "day":

org.mozilla.uitour.treatment: {
  "_v": 1,
  "<name>": "<value>"
},

That way the treatment will expire in FHR 180 days after setting.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: FHR Provider for UITour

My understanding is that FIELD_DAILY_DISCRETE_TEXT is the proper field type for this measurement.

Also, there's nothing to block that multiple calls to the same tag be done in the same day, so the <value> ends up as an array of strings instead of just a string. E.g.:

org.mozilla.uitour.treatment: {
  "_v": 1,
  "<name>": ["<value 1>", "<value 2>"]
}


Benjamin: it looks like I don't need to explicitly add the manifest file to package-manifest.in because there's a * for the modules folder here: http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#616
Is that correct?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8525614 [details] [diff] [review]
WIP patch

Review of attachment 8525614 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/UITour.jsm
@@ +461,5 @@
> +        let string = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
> +        string.data = value;
> +        Services.prefs.setComplexValue("browser.uitour.treatment." + name,
> +                                       Ci.nsISupportsString, string);
> +        // Do some FHR thing

// Do some FHR thing becomes simply:

UITourHealthReport.recordTreatmentTag(name, value);

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: FHR Provider for UITour

Needed to add a `delete window.UITourMetricsProvider` to head.js as the browser_UITour.js tests import UITour.jsm and report a leak if this is not cleaned up

Comment 6

[Benjamin Smedberg]

I think you are correct about packaging; a try build would confirm.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: setTreatmentTag API for UITour (patch by Gavin)

Replaced the "Do some FHR stuff" line from gavin's WIP patch with the call to the FHR provider. Changing to review

Comment 8

[:Felipe Gomes (needinfo for replies!)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I think you are correct about packaging; a try build would confirm.

I just tested a packaged version locally and it worked fine

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8526148 [details] [diff] [review]
setTreatmentTag API for UITour (patch by Gavin)

I never reviewed UITour code before but this looks really straightforward, and there's a simple test. So I'm setting r+ to land. I'll leave the request for Dolske in case he wants to look at it further and I can follow-up with changes

Comment 10

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/659ccf9aac7a
https://hg.mozilla.org/releases/mozilla-beta/rev/166866d8cfcf

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 8526148 [details] [diff] [review]
setTreatmentTag API for UITour (patch by Gavin)

Review of attachment 8526148 [details] [diff] [review]:
-----------------------------------------------------------------

I probably would have pondered using more descriptive like setFHRTreatmentTag, but meh.

Comment 12

[Dave Zeber [:dzeber]]

(In reply to :Felipe Gomes (behind on reviews until the end of the week) from comment #3)

> Also, there's nothing to block that multiple calls to the same tag be done
> in the same day, so the <value> ends up as an array of strings instead of
> just a string. E.g.:
> 
> org.mozilla.uitour.treatment: {
>   "_v": 1,
>   "<name>": ["<value 1>", "<value 2>"]
> }

What would be the interpretation of multiple treatment values? Would users be able to switch treatments?

Comment 13

[Benjamin Smedberg]

In the cases we're designing for right now, that would be unusual, although it might be possible if the user reloaded the whatsnew page multiple times and we didn't check for that.

Comment 14

[Dave Zeber [:dzeber]]

Has there been a decision on what the identifiers will look like for <name> and <value>?

Comment 15

[Benjamin Smedberg]

The specifics of the search tour data is tracked in bug 1102610. There's a spreadsheet linked from there.

Comment 16

[Dave Zeber [:dzeber]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> The specifics of the search tour data is tracked in bug 1102610. There's a
> spreadsheet linked from there.

Ah ok, so we should generally be seeing two keys in the FHR payload, "srch-chg-treatment" and "srch-chg-action".

Comment 17

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/898046f0bd99
https://hg.mozilla.org/integration/fx-team/rev/065d72f2ee84

Includes fix for bug 1104823

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/898046f0bd99
https://hg.mozilla.org/mozilla-central/rev/065d72f2ee84

Comment 19

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e019d4f4601
https://hg.mozilla.org/releases/mozilla-beta/rev/995d8356802c