1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry

Status: RESOLVED FIXED

(Firefox for Android :: Reader View, defect)

Description

[:Margaret Leibovic]

UI telemetry is much more appropriate for this.

Comment 1

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

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

Comment 2

[:Margaret Leibovic]

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?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 4

[Barbara Bermes [:barbara] - NI please!]

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 5

[:Margaret Leibovic]

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/

Comment 6

[:Margaret Leibovic]

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.

Comment 7

[Barbara Bermes [:barbara] - NI please!]

What if you called it "reader_on" and "reader_off"?

Comment 8

[: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?

Comment 9

[Barbara Bermes [:barbara] - NI please!]

> Or maybe reader_available/reader_unavailable?
that's better! I like that

Comment 10

[:Margaret Leibovic]

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 11

[Barbara Bermes [:barbara] - NI please!]

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.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/52749e84bf25fb6aed91ab1339b74cc0e7aae5e7
Bug 1266163 - Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry. r=mfinkle,barbara

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52749e84bf25