Closed
Bug 1266163
Opened 7 years ago
Closed 7 years ago
Replace FENNEC_READER_VIEW_BUTTON histogram with UI telemetry
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Firefox for Android Graveyard
Reader View
Tracking
(firefox49 fixed)
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.
Assignee | ||
Comment 1•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47837/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47837/
Attachment #8743485 -
Flags: review?(mark.finkle)
Attachment #8743485 -
Flags: review?(bbermes)
Assignee | ||
Comment 2•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8743485 -
Flags: review?(mark.finkle) → review+
Comment 3•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8743485 -
Flags: review?(bbermes)
Comment 4•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
What if you called it "reader_on" and "reader_off"?
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 8•7 years ago
|
||
(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)
Comment 9•7 years ago
|
||
> Or maybe reader_available/reader_unavailable?
that's better! I like that
Flags: needinfo?(bbermes)
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52749e84bf25
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•