Closed Bug 1101790 Opened 10 years ago Closed 9 years ago

UITour: ability to set prefs and data in FHR

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
37.1
Tracking Status
firefox34 + fixed
firefox35 + fixed
firefox36 + fixed

People

(Reporter: Gavin, Assigned: Felipe)

References

Details

Attachments

(2 files, 2 obsolete files)

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).
Group: mozilla-employee-confidential
Attached patch WIP patch (obsolete) — Splinter Review
Benjamin, can you comment on the "FHR stuff" that's needed here?
Attachment #8525614 - Flags: feedback?(dolske)
Attachment #8525614 - Flags: feedback?(benjamin)
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.
Attachment #8525614 - Flags: feedback?(benjamin) → feedback+
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Attached patch FHR Provider for UITour (obsolete) — Splinter Review
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?
Attachment #8526095 - Flags: review?(benjamin)
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);
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
Attachment #8526095 - Attachment is obsolete: true
Attachment #8526095 - Flags: review?(benjamin)
Attachment #8526127 - Flags: review?(benjamin)
Attachment #8526127 - Flags: review?(benjamin) → review+
I think you are correct about packaging; a try build would confirm.
Replaced the "Do some FHR stuff" line from gavin's WIP patch with the call to the FHR provider. Changing to review
Attachment #8525614 - Attachment is obsolete: true
Attachment #8525614 - Flags: feedback?(dolske)
Attachment #8526148 - Flags: review?(dolske)
(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
Blocks: 1102404
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
Attachment #8526148 - Attachment description: setTreatmentTag API for UITour → setTreatmentTag API for UITour (patch by Gavin)
Attachment #8526148 - Flags: review+
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.
Attachment #8526148 - Flags: review?(dolske) → review+
(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?
Depends on: 1103127
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.
No longer depends on: 1103127
Has there been a decision on what the identifiers will look like for <name> and <value>?
The specifics of the search tour data is tracked in bug 1102610. There's a spreadsheet linked from there.
Depends on: 1104823
(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".
Whiteboard: [qa-]
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Whiteboard: [qa-]
https://hg.mozilla.org/mozilla-central/rev/898046f0bd99
https://hg.mozilla.org/mozilla-central/rev/065d72f2ee84
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 36
Iteration: --- → 37.1
You need to log in before you can comment on or make changes to this bug.