If it's actually worthwhile. Check the "cancel" use case versus the "back-back" use case and probably try to determine how confused people still get with "back-back".

If UX wants this, it would be a nice small UI telemetry project for Mr. Comella.

Does UX want this? :)

MOAR DATA

Assuming the implementation in bug 998000 is largerly unchanged... Since this is only implemented on phone at the moment, we need to know the device type when editing mode opens so that we can decide whether to count this open session as one that has a "cancel" button, otherwise we're invalidating the overall stats. As such, we can: * Record a UI event on the start of an "editing" Session saying whether or not the cancel button is present * Wait until bug 965371 is implemented, which will include device type in the telemetry payload * Wait until bug 997477 is implemented, which will add the cancel button to tablets Declaring blocking on both of the above bugs, just so the people responsible for those are aware - we can assign the proper dependencies when we decide which path to go down.

Blocked the FHR bug by accident.

Orientation != device config. x_x

Note: this is copied closely from the obsolete patch in bug 998000 [1]. lucasr for the editing mode entry points, liuche for the telemetry bits. Note that stopping the EDITING_MODE session in BrowserToolbar seems inconsistent to me when starting and committing editing mode occurs in BrowserApp. Bug 998000 is intended to make the refactor such that the entry point to cancelEditingMode is also in BrowserApp. [1]: https://bugzilla.mozilla.org/attachment.cgi?id=8408662&action=edit

Note that bug 1007836 doesn't need to land before this does - we just need it in order for the data to be useful (I'm not sure the best way to mark that dependency, so I'll leave it as is).

Comment on attachment 8419781 [details] [diff] [review] Add telemetry: editing mode session and cancel button. Sessions in the URLBar are getting a bit crowded. We have "urlbar" which is active when you tap into the urlbar. We have "frecency" which is active when you start typing. What is "editing" and how is it different?

"editing" mode encompasses anytime that the url bar is expanded to accept user input, independent of selection/focus on the url bar. I spoke with liuche and she believes "urlbar" was added to differentiate urls loaded from the urlbar and those loaded from about:home panels (e.g. top sites thumbnails, history items, etc.). However, we agreed that it isn't a very useful metric because the scope is so limited (i.e. the only action you can take during the focus is typing or committing to load a url). "Editing" mode is a superset of "urlbar" and thus more useful. We thought it would make sense to change the appropriate LOAD_URL events from bug 977196 part 3 to have a new "urlbar" method and remove the "urlbar" session. Similarly, we should probably apply a "panels" method to LOAD_URL events on the home panels. Note that those telemetry changes are currently approved (but not yet landed) for Aurora in bug 977196, meaning if we go ahead with a refactor, we should uplift this too! What say you, mfinkle? Shall I begin the refactor? Note that for the purposes of this bug, I really only need to know when editing mode is cancelled with the cancel button or not, which could be accomplished with ordinary UIEvents - the "editing" session just seemed more generally useful.

(In reply to Michael Comella (:mcomella) from comment #10) > "editing" mode encompasses anytime that the url bar is expanded to accept > user input, independent of selection/focus on the url bar. > > I spoke with liuche and she believes "urlbar" was added to differentiate > urls loaded from the urlbar and those loaded from about:home panels (e.g. > top sites thumbnails, history items, etc.). However, we agreed that it isn't > a very useful metric because the scope is so limited (i.e. the only action > you can take during the focus is typing or committing to load a url). > "Editing" mode is a superset of "urlbar" and thus more useful. I still prefer "urlbar" as a Session, mainly because it's more specific than "editing". The "urlbar" Session is active when the URLBar UI (as the user might see it, but BrowserSearch specifically) is active. This makes sense to me. I do have some questions though: The "urlbar" is started and stopped here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#653 1. Your patch starts and stops "editing" in different places. Is your patch location "better"? If so, let's remove the old "urlbar" Session start/stop and use your "editing" locations - just renamed to Sessions.URLBAR 2. I like your Reason codes. Let's keep using them. 3. Your "editing.cancel" seems a bit too specific. Remember the Session can add context. Why not just use "cancel". Better yet, why even use this if the Session has a "commit" or "cancel" code. That should be enough. If you want a simple probe to tell us the button was pressed, let's use the "action" Event and add a "actionbar" Method and pass the stringified R.id of the button. If the button has no id, just use "close". That along with the "urlbar" Session is enough to tell us what happened. > We thought it would make sense to change the appropriate LOAD_URL events > from bug 977196 part 3 to have a new "urlbar" method and remove the "urlbar" > session. Similarly, we should probably apply a "panels" method to LOAD_URL > events on the home panels. Note that those telemetry changes are currently > approved (but not yet landed) for Aurora in bug 977196, meaning if we go > ahead with a refactor, we should uplift this too! Tell me your thoughts on my proposal first. > Note that for the purposes of this bug, I really only need to know when > editing mode is cancelled with the cancel button or not, which could be > accomplished with ordinary UIEvents - the "editing" session just seemed more > generally useful. I like that you bring the discussion back to "what's needed to fix this bug" since it's easy to let things grow in scope. I think the Session with a Reason is all we need to answer the question. If your start/stop points are more robust than the existing points, let's switch. If we feel like we really need the actual button press, let's consider the generic "action" Event.

Comment on attachment 8419781 [details] [diff] [review] Add telemetry: editing mode session and cancel button. Review of attachment 8419781 [details] [diff] [review]: ----------------------------------------------------------------- I think liuche should do this review as she is a lot more involved in our UI telemetry code. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +1073,5 @@ > * @return the url that was entered > */ > public String cancelEdit() { > + Telemetry.stopUISession(TelemetryContract.Session.EDITING_MODE, > + TelemetryContract.Reason.CANCEL); I'd prefer to keep all EDITING_MODE session handling in BrowserApp (if possible) so that it's easier to follow where the session is being started and stopped.

(In reply to Mark Finkle (:mfinkle) from comment #11) > I still prefer "urlbar" as a Session, mainly because it's more specific than > "editing". If you referring specifically to the name, then sure, I agree. However, note that the code refers to "editing mode" and that's how we often refer to the concept (e.g. the bug title), so it might make more sense to call it "editing_mode" (in accordance with the multiple word convention) than "urlbar". > 1. Your patch starts and stops "editing" in different places. Is your patch > location "better"? If so, let's remove the old "urlbar" Session start/stop > and use your "editing" locations - just renamed to Sessions.URLBAR "urlbar" currently is only applicable when the urlbar is focused, which isn't applicable broadly, imo. I'll move the activation points. > 3. Your "editing.cancel" seems a bit too specific. Remember the Session can > add context. Why not just use "cancel". Better yet, why even use this if the > Session has a "commit" or "cancel" code. That should be enough. If you want > a simple probe to tell us the button was pressed, let's use the "action" > Event and add a "actionbar" Method and pass the stringified R.id of the > button. If the button has no id, just use "close". That along with the > "urlbar" Session is enough to tell us what happened. "editing.cancel" might be too specific, but on the other hand, "action" seems really generic to me: e.g. every item in TelemetryContract can be considered an "action". I like the idea of using the view id, so perhaps: Event: TOOLBAR_TAPPED (this is new) Method: BUTTON EXTRA: <button-id> > I think the Session with a Reason is all we need to answer the question. If > your start/stop points are more robust than the existing points, let's > switch. If we feel like we really need the actual button press, let's > consider the generic "action" Event. I think the question we want to answer is, "How often do people cancel editing mode with the cancel button, rather than hitting back twice?" The Session doesn't tell us which method cancelled editing mode, so the button press event is important.

Spoke with mfinkle on IRC: (In reply to Michael Comella (:mcomella) from comment #13) > > I still prefer "urlbar" as a Session, mainly because it's more specific than > > "editing". Ended up with "awesomescreen". > "editing.cancel" might be too specific, but on the other hand, "action" > seems really generic to me: e.g. every item in TelemetryContract can be > considered an "action". Ended up with: Event: CANCEL Method: ACTIONBAR EXTRA: <button-id>

Comment on attachment 8419781 [details] [diff] [review] Add telemetry: editing mode session and cancel button. Review of attachment 8419781 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I'd like to r? the version with the new naming (and maybe see a patch that replaces urlbar), so clearing the review flag. One thing that I noticed is that cancelEdit() closes a session with "cancel" as the reason, but not every call to cancelEdit is an explicit cancel (clicking a bookmark or history item, for example). Consider leaving out "cancel" as a reason, or try to be more specific with where it's called (though this might not be worth adding it everywhere and the added risk of missing Telemetry in new calls to cancelEdit).

(In reply to Lucas Rocha (:lucasr) from comment #12) > I'd prefer to keep all EDITING_MODE session handling in BrowserApp (if > possible) so that it's easier to follow where the session is being started > and stopped. The refactoring necessary for this change is expected to take place in bug 998000.

(In reply to Chenxia Liu [:liuche] from comment #15) > One thing that I noticed is that cancelEdit() closes a session with "cancel" > as the reason, but not every call to cancelEdit is an explicit cancel > (clicking a bookmark or history item, for example). Consider leaving out > "cancel" as a reason, or try to be more specific with where it's called > (though this might not be worth adding it everywhere and the added risk of > missing Telemetry in new calls to cancelEdit). Spoke on IRC. We decided: * Remove the CANCEL Reason when closing the Session, to avoid ambiguity between taking no action and opening a url - if we decide we need a Reason later, we can add it * Add an event to cancel on back pressed, so we can determine the difference between canceling via the X and pressing back

Comment on attachment 8422071 [details] [diff] [review] Add UITelemetry: awesomescreen session and cancel button. Review of attachment 8422071 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +378,5 @@ > @Override > public void onClick(View v) { > + Telemetry.sendUIEvent(TelemetryContract.Event.CANCEL, > + TelemetryContract.Method.ACTIONBAR, > + Integer.toString(editCancel.getId())); What is this extra differentiating? There's only one button for canceling, right? I'd be inclined to leave this out - if we add more buttons, we can track them at some later point, right?

(In reply to Chenxia Liu [:liuche] from comment #19) > What is this extra differentiating? There's only one button for canceling, > right? Nothing and, currently, yes. > I'd be inclined to leave this out - if we add more buttons, we can > track them at some later point, right? I hesitate to add the extras later because "CANCEL"/"ACTIONBAR" is fairly generic - what if the dev to add the next "CANCEL"/"ACTIONBAR" event doesn't realize this event/method combination already exists? We'll likely invalidate our cancel button data, and/or complicate the analysis scripts. It seems safer to just throw it in there now. The only con is transmitting more data, right? This seems like a negligible safety net. (Disclaimer: I haven't been counting packets)

Rebase.

Comment on attachment 8422604 [details] [diff] [review] Add UITelemetry: awesomescreen session and cancel button. f=mfinkle [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 965548 User impact if declined: This patch adds additional UI telemetry - we'd have to wait an additional cycle to do analysis on the telemetry, meaning it'd take at least an extra cycle before we can use this data to make beneficial changes for our users. Testing completed (on m-c, etc.): Tested locally. Risk to taking this patch (and alternatives if risky): Low risk - we're only touching Telemetry code and not impacting functionality here. However, there is the possibility of screwing up our telemetry probes. String or IDL/UUID changes made by this patch: None

(In reply to Michael Comella (:mcomella) from comment #20) > > I'd be inclined to leave this out - if we add more buttons, we can > > track them at some later point, right? > > I hesitate to add the extras later because "CANCEL"/"ACTIONBAR" is fairly > generic - what if the dev to add the next "CANCEL"/"ACTIONBAR" event doesn't > realize this event/method combination already exists? We'll likely > invalidate our cancel button data, and/or complicate the analysis scripts. > It seems safer to just throw it in there now. From the scripts I have written, I can say having the extra in there now makes it easier to deal with in a future where we have more extras. Less jiggling of scripts.

For the analysis, bug 997477 is helpful, but not necessary.

The following telemetry probes are logged using device: * The "back-back" use case: SendUIEvent: event = cancel.1 method = back timestamp = 18051597 extras = null * Tapping the X button: SendUIEvent: event = cancel.1 method = actionbar timestamp = 18056838 extras = edit_cancel The following telemetry probes are logged using tablet: * The "back-back" use case: SendUIEvent: event = cancel.1 method = back timestamp = 29591683 extras = null * The "back-back" use case: SendUIEvent: event = cancel.1 method = actionbar timestamp = 29608613 extras = edit_cancel Verified as fixed in builds: - 32.0a1 (2014-06-06); Devices: - Motorola Razr (Android 4.0.4) - Asus Transformer Tab (Andorid 4.0.3).