Closed Bug 1255202 Opened 4 years ago Closed 4 years ago

Add UI Telemetry probes for DoorHangers

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 --- affected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle, Mentored)

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(1 file, 1 obsolete file)

We do not have any probes for doorhangers. We use doorhangers primarily for permission prompts, but I'm sure other reasons exist.

I'd at least like to see UI Telemetry probes for:
1. Showing a doorhanger (SHOW, DOORHANGER, <context>)
2. Primary actions, probably BUTTON + context
3. Closing the doorhanger by tapping somewhere (CANCEL, DOORHANGER, ?)
Mentor: liuche, mark.finkle
Whiteboard: [good next bug][lang=java]
Assignee: nobody → mark.finkle
Attached patch doorhanger-uitelemetry v0.1 (obsolete) — Splinter Review
This patch adds:
1. Probe for showing a doorhanger. This is fired during the creation of the doorhanger, not on any show/hide magic we support for hiding doorhangers when tabs change. I include the doorhanger type as a string extra.

2. Probe for positive and negative actions. I used DOORHANGER for the method, since it seemed a little clearer and the UI is not always a button. The doorhanger type and the positive/negative action is passed as the extra. Given I had to let the Concrete Java class handle firing the probe, since it makes the onClick handler, we could make these more specific than "positive" and "negative", but it works for now.

3. Probe for handling link loads from the doorhanger.

Yes, I added DOORHANGER as a method. These might not always be DoorHangers, just like TOAST is no longer really a Toast. We know what it means and we don't plan on changing the probe names since it screws up the history and makes it harder to track across different Fx versions. If we have a more generic term, POPUP_PROMPT?, we could start using it. Otherwise, it's DOORHANGER for life and we just adapt ourselves as it changes implementation. Nothing is ever perfect :)

I did not find an easy place to add the CANCEL probe, but what this patch adds is good enough to track what people are "allowing" and "denying". I also do not track "checkbox" state used in some DoorHangers. We can add that later if we find a need.
Attachment #8729179 - Flags: review?(margaret.leibovic)
Attachment #8729179 - Flags: review?(liuche)
(In reply to Mark Finkle (:mfinkle) from comment #1)

> Yes, I added DOORHANGER as a method. These might not always be DoorHangers,
> just like TOAST is no longer really a Toast. We know what it means and we
> don't plan on changing the probe names since it screws up the history and
> makes it harder to track across different Fx versions. If we have a more
> generic term, POPUP_PROMPT?, we could start using it. Otherwise, it's
> DOORHANGER for life and we just adapt ourselves as it changes
> implementation. Nothing is ever perfect :)

I don't see us changing these any time soon, so I'm fine with DOORHANGER. If we do change it in the future, we'll know what it means.
Comment on attachment 8729179 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Review of attachment 8729179 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/widget/DefaultDoorHanger.java
@@ +133,4 @@
>          return new Button.OnClickListener() {
>              @Override
>              public void onClick(View v) {
> +                final String expandedExtra = mType.toString().toLowerCase() + "-" + extra;

Does this distinguish between the different doorhanger types that we get from JS? I think you want to use mIdentifier instead here, since I believe that will give us the "value" parameter that's passed in from JS.

You should test with one of these JS prompts, like WebRTC permissions.
(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8729179 [details] [diff] [review]
> doorhanger-uitelemetry v0.1
> 
> Review of attachment 8729179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/widget/DefaultDoorHanger.java
> @@ +133,4 @@
> >          return new Button.OnClickListener() {
> >              @Override
> >              public void onClick(View v) {
> > +                final String expandedExtra = mType.toString().toLowerCase() + "-" + extra;
> 
> Does this distinguish between the different doorhanger types that we get
> from JS? I think you want to use mIdentifier instead here, since I believe
> that will give us the "value" parameter that's passed in from JS.
> 
> You should test with one of these JS prompts, like WebRTC permissions.

I looked into this and you are right. The "Type" is based on the "category" param passed into the JS API. It's optional. It also gets very tricky, as IDs do, appending the host of the domain to some. 

After thinking about this, I decided we don't want to log _every_ doorhanger usage. Only the features we care about tracking. We'll use the "category" to determine what doorhangers are relevant.

I'll update the patch to add a "category" and enum value for WebRTC. That will allow us to track WebRTC prompts too.
Updated to support WebRTC prompts.
Attachment #8729843 - Flags: review?(margaret.leibovic)
Attachment #8729843 - Flags: review?(liuche)
I should note, we'll track all doorhangers, but we only identify doorhangers by type. Any non-typed doorhangers are grouped together in "default".
Comment on attachment 8729179 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Review of attachment 8729179 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I think there are a few more doorhanger actions that probably need to be added. Take a look at SiteIdentityPopup, which doesn't inherit from the other Doorhangers because it holds different information and should always be able to be brought up by the user, and has buttons for turning off Tracking Protection for a site. (You can try espn.go.com with TP enabled.)

There are also "links" in doorhangers that will open dialogs for editing the login, or selecting a login to fill in. One of them is the "edit" dialog for a Login doorhanger, and there are a few more in SiteIdentityPopup such as "edit site settings", "learn more" (about tracking protection), etc.

::: mobile/android/base/java/org/mozilla/gecko/TelemetryContract.java
@@ +147,5 @@
>  
>          // Action triggered from a dialog.
>          DIALOG("dialog"),
>  
> +        // Action triggered from a doorhanger popup prompts.

grammar nit: singular "prompt" or remove the singular "a"

::: mobile/android/base/java/org/mozilla/gecko/widget/ContentSecurityDoorHanger.java
@@ +96,5 @@
>          }
>      }
>  
>      @Override
> +    protected OnClickListener makeOnButtonClickListener(final int id, final String extra) {

Nit: rename extra to be telemetryExtra

::: mobile/android/base/java/org/mozilla/gecko/widget/DefaultDoorHanger.java
@@ +128,5 @@
>          }
>      }
>  
>      @Override
> +    protected OnClickListener makeOnButtonClickListener(final int id, final String extra) {

Same
Comment on attachment 8729179 [details] [diff] [review]
doorhanger-uitelemetry v0.1

I'm going to obsolete this patch, because it looks like there's another version with more work in it.
Attachment #8729179 - Attachment is obsolete: true
Attachment #8729179 - Flags: review?(margaret.leibovic)
Attachment #8729179 - Flags: review?(liuche)
Comment on attachment 8729843 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Review of attachment 8729843 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/widget/DoorHanger.java
@@ +36,5 @@
>          return new DefaultDoorHanger(context, config, type);
>      }
>  
>      // Doorhanger types created from Gecko are checked against enum strings to determine type.
> +    public static enum Type { DEFAULT, LOGIN, TRACKING, GEOLOCATION, DESKTOPNOTIFICATION2, WEBRTC }

I agree with adding this type here - I think it would be good to have another metric of how often people are using webrtc (by proxy of interacting with a doorhanger that pops up).

Are there other types of doorhangers that we care about?
Attachment #8729843 - Flags: review?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #7)
> Comment on attachment 8729179 [details] [diff] [review]
> doorhanger-uitelemetry v0.1
> 
> Review of attachment 8729179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but I think there are a few more doorhanger actions that
> probably need to be added. Take a look at SiteIdentityPopup, which doesn't
> inherit from the other Doorhangers because it holds different information
> and should always be able to be brought up by the user, and has buttons for
> turning off Tracking Protection for a site. (You can try espn.go.com with TP
> enabled.)

SiteIdentityPopup parallels DoorhangerPopup, in that they both hold DoorHanger objects. I believe the patch here would handle clicks on those buttons, but we can verify.

For example, the ContentSecurityDoorHanger, which is handled in this patch, is only shown in SiteIdentityPopup.

> There are also "links" in doorhangers that will open dialogs for editing the
> login, or selecting a login to fill in. One of them is the "edit" dialog for
> a Login doorhanger, and there are a few more in SiteIdentityPopup such as
> "edit site settings", "learn more" (about tracking protection), etc.

I think we could file a follow-up bug for getting data about clicks on these links.
Comment on attachment 8729843 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Review of attachment 8729843 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks reasonable to me. We can address adding additional probes in follow-up bugs.
Attachment #8729843 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #10)
> (In reply to Chenxia Liu [:liuche] from comment #7)
> > Comment on attachment 8729179 [details] [diff] [review]
> > doorhanger-uitelemetry v0.1
> > 
> > Review of attachment 8729179 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me, but I think there are a few more doorhanger actions that
> > probably need to be added. Take a look at SiteIdentityPopup, which doesn't
> > inherit from the other Doorhangers because it holds different information
> > and should always be able to be brought up by the user, and has buttons for
> > turning off Tracking Protection for a site. (You can try espn.go.com with TP
> > enabled.)
> 
> SiteIdentityPopup parallels DoorhangerPopup, in that they both hold
> DoorHanger objects. I believe the patch here would handle clicks on those
> buttons, but we can verify.

Confirmed, I see a "tracking-negative" event get logged if I disable tracking protection.

This makes me think that we should get rid of our TRACKING_PROTECTION_EVENTS and TRACKING_PROTECTION_SHIELD histogram probes, I don't think those are as useful.
https://hg.mozilla.org/mozilla-central/rev/1fde3cca31b4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8729843 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Would give us insight into permission prompts. Currently, we have none.
[Describe test coverage new/current, TreeHerder]: Working fine on Nightly.
[Risks and why]: Fairly low. Most of the changes are for adding the probes.
[String/UUID change made/needed]: None
Attachment #8729843 - Flags: approval-mozilla-beta?
Attachment #8729843 - Flags: approval-mozilla-aurora?
Hi Mark, have we reviewed the data from Nightly to conclude that we are getting the new telemetry data? Just wondering if we should assess that data before uplifting to Aurora/Beta.
Flags: needinfo?(mark.finkle)
(In reply to Ritu Kothari (:ritu) from comment #16)
> Hi Mark, have we reviewed the data from Nightly to conclude that we are
> getting the new telemetry data? Just wondering if we should assess that data
> before uplifting to Aurora/Beta.

Yes. We are getting telemetry data for a variety of doorhanger permission prompts.
Flags: needinfo?(mark.finkle)
Comment on attachment 8729843 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Mark has verified that the telemetry data looks good after this landed on Nightly. Seems like a good idea to uplift to Aurora 47.
Attachment #8729843 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems with uplifting:

grafting 335251:1fde3cca31b4 "Bug 1255202 - Add UI Telemetry probes for DoorHangers. r=margaret"
merging mobile/android/base/java/org/mozilla/gecko/widget/ContentSecurityDoorHanger.java
merging mobile/android/base/java/org/mozilla/gecko/widget/DefaultDoorHanger.java
merging mobile/android/base/java/org/mozilla/gecko/widget/DoorHanger.java
merging mobile/android/chrome/content/WebrtcUI.js
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/widget/DoorHanger.java! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(mark.finkle)
Comment on attachment 8729843 [details] [diff] [review]
doorhanger-uitelemetry v0.1

Given this had problems uplifting and we are heading into beta 8 I'd like to keep this aimed at 47 release.
Attachment #8729843 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Seems like an easy enough conflict fix to work around no Bug 1193208 on aurora.
Flags: needinfo?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.