997487 - Add UI telemetry to editing mode cancel button

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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".

Comment 1

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

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

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Does UX want this? :)

Comment 3

[Ian Barlow (:ibarlow)]

MOAR DATA

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Blocked the FHR bug by accident.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Orientation != device config. x_x

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add telemetry: editing mode session and cancel button.

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

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 9

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

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?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

"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.

Comment 11

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

(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 12

[Lucas Rocha (:lucasr)]

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.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-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.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 15

[Chenxia Liu [:liuche]]

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).

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add UITelemetry: awesomescreen session and cancel button.

Comment 19

[Chenxia Liu [:liuche]]

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?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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)

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Add UITelemetry: awesomescreen session and cancel button. f=mfinkle

Rebase.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/ccabfe66be68

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ccabfe66be68

Comment 25

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

(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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd1c4f80a39c

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 28

[Flaviu Cos]

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).