Closed
Bug 986095
Opened 10 years ago
Closed 10 years ago
Add UI Telemetry for Reading List
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Tracking
(firefox31 fixed, firefox32 fixed)
RESOLVED
FIXED
Firefox 32
People
(Reporter: liuche, Assigned: mfinkle)
References
Details
Attachments
(5 files)
1.81 KB,
patch
|
liuche
:
review+
rnewman
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.55 KB,
patch
|
rnewman
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.92 KB,
patch
|
liuche
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
rnewman
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
774 bytes,
patch
|
rnewman
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Some basic things to track: * Clicking on the Reader Mode page action * Reader mode * Actions from Reader Mode (Add, Reading List, etc)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → liuche
Assignee | ||
Comment 1•10 years ago
|
||
This patch adds a basic session for Reader. We only want to track a Session if the Reader viewer is in the foreground. We do not want to track all the open Reader viewers that might be in the background. Reader.updatePageAction is called whenever the toolbar pageaction needs to be changed. This matches our Session behaivor too. If the reader icon is orange (active) we start a Session. If not, we stop the Session.
Assignee: liuche → mark.finkle
Attachment #8413958 -
Flags: review?(rnewman)
Attachment #8413958 -
Flags: review?(liuche)
Assignee | ||
Comment 2•10 years ago
|
||
This patch adds an Event for sharing via Reader toolbar. I added it to Java, but now I am thinking of moving it to aboutReader.js so the timestamps are based on the same relative start. Agreed?
Attachment #8413960 -
Flags: review?(rnewman)
Assignee | ||
Comment 3•10 years ago
|
||
This patch adds Events for adding and removing an article from the Reading List. I used "saveforlater.1" and "unsaveforlater.1" as the Event names since these can be used for Bookmarking too. The "reader.1" Session should be active and I throw in a "reader" extra.
Attachment #8413965 -
Flags: review?(liuche)
Assignee | ||
Comment 4•10 years ago
|
||
Even though the Events and Session are used from JS for now, I added them to the Java TelemetryContract file.
Attachment #8413966 -
Flags: review?(liuche)
Assignee | ||
Comment 5•10 years ago
|
||
We don't log errors for a duplication Session on startSession. Let's not log errors for missing Session on stopSession. The "reader.1" Session will be noisy otherwise. I could have made UITelemetry.activeSessions public and done the check myself, but why bother?
Attachment #8413968 -
Flags: review?(rnewman)
Updated•10 years ago
|
Attachment #8413968 -
Flags: review?(rnewman) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8413958 [details] [diff] [review] uitelemetry-reader-session v0.1 Review of attachment 8413958 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +7492,5 @@ > delete this.pageAction.id; > } > > + // Create a relative timestamp for telemetry > + let uptime = Date.now() - Services.startup.getStartupInfo().linkerInitialized; TODO: Bug XXXXXXX: get uptime from a stable source.
Attachment #8413958 -
Flags: review?(rnewman) → review+
Updated•10 years ago
|
Attachment #8413960 -
Flags: review?(rnewman) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8413966 [details] [diff] [review] uitelemetry-reader-contract v0.1 Review of attachment 8413966 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/TelemetryContract.java @@ +41,5 @@ > + public static final String SAVE_FOR_LATER = "saveforlater.1"; > + > + // Stop holding a resource (reader, bookmark, etc) for viewing later. > + // Note: Only used in JavaScript for now, but here for completeness. > + public static final String UNSAVE_FOR_LATER = "unsaveforlater.1"; These are supposed to be being documented in m/a/b/docs/index.rst. Please do so. If you want to feel particularly old, name the section "Data dictionary". Man, that makes me feel old.
Attachment #8413966 -
Flags: review?(liuche) → review+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Component: General → Reader Mode
Hardware: ARM → All
Assignee | ||
Comment 8•10 years ago
|
||
> ::: mobile/android/base/TelemetryContract.java
> @@ +41,5 @@
> > + public static final String SAVE_FOR_LATER = "saveforlater.1";
> > + public static final String UNSAVE_FOR_LATER = "unsaveforlater.1";
After talking to Chenxia, I changed these to "save.1" and "unsave.1" since it is more generic and works just as well with bookmarks and pdfs. We are using the Extras field to hold the extra context: "reader", "bookmark", and "pdf".
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8413960 [details] [diff] [review] uitelemetry-reader-sharing v0.1 Review of attachment 8413960 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +1212,5 @@ > GeckoAppShell.openUriExternal(url, "text/plain", "", "", > Intent.ACTION_SEND, title); > + > + // Context: Sharing via Reader toolbar (reader session is active) > + Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.LIST); Do we need this LIST method? Is this to delineate if we add sharing through the browser menu?
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Chenxia Liu [:liuche] from comment #9) > Comment on attachment 8413960 [details] [diff] [review] > uitelemetry-reader-sharing v0.1 > > Review of attachment 8413960 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/BrowserApp.java > @@ +1212,5 @@ > > GeckoAppShell.openUriExternal(url, "text/plain", "", "", > > Intent.ACTION_SEND, title); > > + > > + // Context: Sharing via Reader toolbar (reader session is active) > > + Telemetry.sendUIEvent(TelemetryContract.Event.SHARE, TelemetryContract.Method.LIST); > > Do we need this LIST method? Is this to delineate if we add sharing through > the browser menu? The LIST here is the manner in which we are performing the SHARE: * LIST = a list of sharing apps will be displayed and the user picks one. (non-QuickShare) * BUTTON = the sharing app is displayed in a single button and the user tapped it. (QuickShare)
Assignee | ||
Comment 11•10 years ago
|
||
That said, I would be OK with removing the LIST if/when we decide how we will deal with normal vs exceptions. In the case of SHARE, LIST is the norm and BUTTON is the exception. In the case of LOAD_URL, LIST_ITEM is the norm and GRID_ITEM is the exception. Although, sometimes we skip setting LIST_ITEM since it's really, really the norm. We can change this later if we decide to skip the "norm" and focus on the "exception". It won't break the telemetry data and we won't even need to increment our versioning. We'll just start ignoring the "norm" Methods in our analysis scripts.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #10) > > Do we need this LIST method? Is this to delineate if we add sharing through > > the browser menu? > > The LIST here is the manner in which we are performing the SHARE: > * LIST = a list of sharing apps will be displayed and the user picks one. > (non-QuickShare) > * BUTTON = the sharing app is displayed in a single button and the user > tapped it. (QuickShare) Oh, I forgot that this was the Reader bug. In the Share bug, the patches use LIST for any telemetry where a SHARE displays the LIST.
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8413958 [details] [diff] [review] uitelemetry-reader-session v0.1 Review of attachment 8413958 [details] [diff] [review]: ----------------------------------------------------------------- I'm seeing lots of extraneous start/stop events, but I think this is okay, and there's not really a better place to put them. UITelemetry.jsm will handle duplicate events as well.
Attachment #8413958 -
Flags: review?(liuche) → review+
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8413965 [details] [diff] [review] uitelemetry-reader-saveforlater v0.1 Review of attachment 8413965 [details] [diff] [review]: ----------------------------------------------------------------- Make sure to handle the long-press "save to reading list" case as well. ::: mobile/android/chrome/content/aboutReader.js @@ +298,5 @@ > > + let result = gChromeWin.Reader.READER_ADD_FAILED; > + if (success) { > + result = gChromeWin.Reader.READER_ADD_SUCCESS; > + UITelemetry.addEvent("saveforlater.1", "button", uptime, "reader"); Just the note to change this to "save" and "unsave"
Attachment #8413965 -
Flags: review?(liuche) → review+
Assignee | ||
Comment 15•10 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/1885c2bb0cc0 remote: https://hg.mozilla.org/integration/fx-team/rev/c7ba6c5851c9 remote: https://hg.mozilla.org/integration/fx-team/rev/d0f12137f270 remote: https://hg.mozilla.org/integration/fx-team/rev/331ff822db4e remote: https://hg.mozilla.org/integration/fx-team/rev/d08e05dc6253
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1885c2bb0cc0 https://hg.mozilla.org/mozilla-central/rev/c7ba6c5851c9 https://hg.mozilla.org/mozilla-central/rev/d0f12137f270 https://hg.mozilla.org/mozilla-central/rev/331ff822db4e https://hg.mozilla.org/mozilla-central/rev/d08e05dc6253
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8413958 [details] [diff] [review] uitelemetry-reader-session v0.1 [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: We miss out on some "before changes" telemetry that could show affect of Fx32 changes Testing completed (on m-c, etc.): Working Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none (request for all patches)
Attachment #8413958 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8413960 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8413965 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8413966 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8413968 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Updated•10 years ago
|
Attachment #8413958 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8413960 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8413965 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8413966 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8413968 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 18•10 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/75f2bf79d647 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d75417cae251 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/9aa82a0b4dcf remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/0e94baa4a108 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/ed1cea8a8abd
Updated•3 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
•