Closed Bug 1266163 Opened 4 years ago Closed 4 years ago

Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry

Categories

(Firefox for Android :: Reader View, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

UI telemetry is much more appropriate for this.
I decided not to include an event for the case where we *don't* show the reader view button in the toolbar, partially because I was uncertain of what we would even name that event. I'm open to suggestions if you think we should include that, but I don't think it's necessary.

I realize we also have reader sessions. Are those useful? Should we remove those? Or should we keep those and not proceed with adding the events I'm adding here?
Attachment #8743485 - Flags: review?(mark.finkle) → review+
Comment on attachment 8743485 [details]
MozReview Request: Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

https://reviewboard.mozilla.org/r/47837/#review44575

Looks good. Let's leave the "reader.1" session in for now. It's not broken like other sessions and we could use it for confirming "bookmark saves while in reader" types of events.
Attachment #8743485 - Flags: review?(bbermes)
Comment on attachment 8743485 [details]
MozReview Request: Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

https://reviewboard.mozilla.org/r/47837/#review44907

overall beautiful code, but let's add that additional probe in there to indicate when the reader view button is hidden

::: mobile/android/chrome/content/Reader.js:204
(Diff revision 1)
>      // Only stop a reader session if the foreground viewer is not visible.
>      UITelemetry.stopSession("reader.1", "", null);
>  
>      if (browser.isArticle) {
>        showPageAction("drawable://reader", Strings.reader.GetStringFromName("readerView.enter"));
> -      this._buttonHistogram.add(this._buttonHistogramValues.SHOWN);
> +      UITelemetry.addEvent("show.1", "button", "reader");

please add also the hidden case, i.e. reader_hidden, and reader_show for showing it
Comment on attachment 8743485 [details]
MozReview Request: Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47837/diff/1-2/
Attachment #8743485 - Flags: review?(bbermes)
https://reviewboard.mozilla.org/r/47837/#review44915

::: mobile/android/chrome/content/Reader.js:206
(Diff revision 2)
>  
>      if (browser.isArticle) {
>        showPageAction("drawable://reader", Strings.reader.GetStringFromName("readerView.enter"));
> -      this._buttonHistogram.add(this._buttonHistogramValues.SHOWN);
> +      UITelemetry.addEvent("show.1", "button", "reader_shown");
>      } else {
> -      this._buttonHistogram.add(this._buttonHistogramValues.HIDDEN);
> +      UITelemetry.addEvent("show.1", "button", "reader_hidden");

It's kinda awkward that this is a "show" event for hiding a button, but I couldn't think of a more appropriate action.
What if you called it "reader_on" and "reader_off"?
Flags: needinfo?(margaret.leibovic)
(In reply to Barbara Bermes [:barbara] from comment #7)
> What if you called it "reader_on" and "reader_off"?

Is that confusing because it's only about the button being shown or not (vs. about actually being in reader view or not)?

Or maybe reader_available/reader_unavailable?
Flags: needinfo?(margaret.leibovic) → needinfo?(bbermes)
> Or maybe reader_available/reader_unavailable?
that's better! I like that
Flags: needinfo?(bbermes)
Comment on attachment 8743485 [details]
MozReview Request: Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47837/diff/2-3/
Comment on attachment 8743485 [details]
MozReview Request: Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

https://reviewboard.mozilla.org/r/47837/#review46293

Ok by me.
Attachment #8743485 - Flags: review?(bbermes) → review+
https://hg.mozilla.org/integration/fx-team/rev/52749e84bf25fb6aed91ab1339b74cc0e7aae5e7
Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara
https://hg.mozilla.org/mozilla-central/rev/52749e84bf25
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1273173
You need to log in before you can comment on or make changes to this bug.