Closed Bug 997487 Opened 6 years ago Closed 6 years ago

Add UI telemetry to editing mode cancel button

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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? :)
Flags: needinfo?(ibarlow)
MOAR DATA
Flags: needinfo?(ibarlow)
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.
Assignee: nobody → michael.l.comella
Depends on: 965371, 997477
Blocked the FHR bug by accident.
Depends on: 988635
No longer depends on: 965371
Orientation != device config. x_x
Depends on: 1007836
No longer depends on: 988635
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
Attachment #8419781 - Flags: review?(lucasr.at.mozilla)
Attachment #8419781 - Flags: review?(liuche)
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).
Status: NEW → ASSIGNED
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.
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
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.
Attachment #8419781 - Flags: review?(lucasr.at.mozilla)
(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).
Attachment #8419781 - Flags: review?(liuche)
(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
Attachment #8419781 - Attachment is obsolete: true
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?
Attachment #8422071 - Flags: review?(liuche) → review+
(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)
Attachment #8422071 - Attachment is obsolete: true
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
Attachment #8422604 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ccabfe66be68
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
(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.
Attachment #8422604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer depends on: 997477
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).
You need to log in before you can comment on or make changes to this bug.