1097876 - Collect UITelemetry about the Panic Button.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

We're interested in how users engage with the PanelUI-panicView.
Specifically, "Which time-option do they choose?" and "Do they click the Forget button, or cancel out of the panel?".

Comment 1

[Blake Winton (:bwinton) (:☕️)]

The panel is defined at https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#187 and the code that handles it is at https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#990

Comment 2

[canaar]

Is this bug about this : https://addons.mozilla.org/en-US/firefox/addon/panic-button/

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Good guess, but no.  It's about https://support.mozilla.org/en-US/kb/forget-button-quickly-delete-your-browsing-history instead.  :)

Comment 4

[canaar]

So what we need to do is collect UI Telemetry about quick forget button, such as how many times it was used, right? :)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Partially how often the button was clicked, but if you try it yourself, you can see that clicking the button opens a panel like this one:
https://www.dropbox.com/s/rnakzparozmygfx/Screenshot%202015-02-09%2011.39.34.png?dl=0
and we're also interested in which option people choose at the top, and whether they click the "Forget!" button at the bottom (or not).

Comment 6

[canaar]

Alright.

Comment 7

[canaar]

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#990 : In the code, there is a mention of sanitize.js, and frequent use of sanitize functions. What exactly does "sanitize" do?

Comment 8

[Blake Winton (:bwinton) (:☕️)]

The Sanitizer is the bit of code that clears out the requested parts of your history.
I don't think we need to worry about it too much if we're logging UI events.  :)

Comment 9

[canaar]

Okayz, so we need the count of how many times the "forget" button is pressed, and which option is selected out of the 3. UI events right :D

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Yep, exactly!  :D

I think we'll want to make a function called something like countPanicEvent, and then call it when the Forget button is pressed.

You can probably copy some of the code from https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#594 and edit it to make it do what you want.  ;)

Comment 11

[canaar]

Just to make sure, this function "countPanicEvent" should also be written in BrowserUITelemetry.jsm, right? :)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Yep.  Perhaps after the countSearchSettingsEvent function…

Comment 13

[canaar]

Umm, I wanted to ask, is this how you count the times panic button has been pressed :

countPanicEvent: function(id, type, view_Id) {
    this._countEvent(["panic-button", id, type, view_Id]);
  },

Comment 14

[Blake Winton (:bwinton) (:☕️)]

It would be something like that, but there are a couple of changes…

We already have something called "panic-button", so let's call it "forget-button" instead.
And instead of passing in the id, type, and view_Id, let's pass in the id of the timeSpan they chose.

Comment 15

[canaar]

Okay, so like this :
  countPanicEvent: function(timeId) {
    this._countEvent(["forget-button", timeId]);
  },

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Looks good to me!  Do you know where you'll be calling the new function?

Comment 17

[canaar]

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#1010 I guess, inside the forgetButtonCalled function here. And I will have to declare a variable too right?

Comment 18

[Blake Winton (:bwinton) (:☕️)]

That looks right, and yes, you will probably need to declare a new variable.  :)

Comment 19

[canaar]

So, this would be the variable and function call :
   let cnt  = countPanicEvent(group);
Now, how do I add it to UI Telemetry? I mean like panicButtonCount = result.something?

Comment 20

[Blake Winton (:bwinton) (:☕️)]

(In reply to Abhilash Mhaisne from comment #19)
> So, this would be the variable and function call :
>    let cnt  = countPanicEvent(group);

I think we only want the "id" attribute of the group, not the whole element.

> Now, how do I add it to UI Telemetry? I mean like panicButtonCount =
> result.something?

The "this._countEvent" call in "countPanicEvent" will add it for you!  :)

Comment 21

[canaar]

Okay. So here we go : https://drive.google.com/file/d/0Bxoyzo1JAHpRNnVmZlE5eElaNHc/view?usp=sharing
The number besides left, is nicely indicating how many times I click the forget button. However , the option selected is not being shown. And what is that "left" in panic-button?

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Hmm.  I don't see "forget-button" anywhere.  Can you open the Browser Console (with Ctrl-Shift-J), and see if there are any errors when you click the forget button in the panel?

The "left" tells us how many times you opened the panel with the left mouse button.  :)

Comment 23

[canaar]

Yep, there is an error present, its called : Reference Error : countPanicEvent is not defined. "panic-button" is shown besides UI Telemetry, and the count is also coming.

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Ah!  So you'll notice that when we call the countSearchSettings function on this line:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1014
we need to tell Javascript that it's defined on the BrowserUITelemetry object.
I bet we need to do the same with the countPanicEvent function.  :)

Comment 25

[canaar]

Okay, so I did that, but it now shows an error in the console, BrowserUITelemetry not defined. :)

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Excellent!  Progress!  :)

Can you post the diff of the code you have so far, just so that I can understand where we are?
('hg diff > bug1097876.diff' should show all your changes…)

Comment 27

[canaar]

Attachment: bug_1097876.diff

Comment 28

[Blake Winton (:bwinton) (:☕️)]

Ah, I see what's happening…

You'll need to import the BrowserUITelemetry into the CustomizableWidgets module.
It should go up near the top of the file, and you can copy the couple of lines from here:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#15

Let me know if that fixes the problem!  :)

Comment 29

[canaar]

https://drive.google.com/file/d/0Bxoyzo1JAHpRQk9jWUEtSWZlS0k/view?usp=sharing
Done! :)

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Alright, so one more change, and I think it will be good to ask for review!

You'll notice in the UITelemetry that you have:
  "forget-button":{"[object XULElement]": 2}
That's because you're passing the whole "group" element in to countPanicEvent.  I think we only want the "id" attribute of the group, because that should be a more readable (and useful) value.  ;)

Comment 31

[canaar]

Sir, I passed "group.id" to the function, so now the appearance is like this :
"forget-button":{"PanelUI-panic-timeSpan":2}
Good to proceed?

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Well, it looks much more readable, but it doesn't tell us which time span was selected…  Could you get the id of the selected radio element instead?

Comment 33

[canaar]

Ok, so I passed this to the function :

BrowserUITelemetry.counPanicEvent(group.selectedItem.id)

What happens is, no matter whatever radio is selected, it always shows:

"forget-button":{"PanelUI-panic-5min":2}
It always says 5min. How to fix that? :)

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Hmm.  I'm not sure.  Can you post your latest set of changes, and I'll try running them here to see what I get?

Comment 35

[canaar]

Attachment: bug_1097876.diff

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8564557 [details] [diff] [review]
bug_1097876.diff

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

I think I found it…  :)

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1014,5 @@
>        this._ensureSanitizer();
>        this._sanitizer.range = this._getSanitizeRange(doc);
>        let group = doc.getElementById("PanelUI-panic-timeSpan");
>        group.selectedItem = doc.getElementById("PanelUI-panic-5min");
> +      let cnt  = BrowserUITelemetry.countPanicEvent(group.selectedItem.id);

What does the line immediately above this one do?  ;)

Comment 37

[canaar]

I think it defaults the selected item to 5 min radio button, does it? :D :)

Comment 38

[Blake Winton (:bwinton) (:☕️)]

It does!  But it also changes the selectedItem away from the one the user selected.  :)
So, if you move the countPanicEvent line up above that line, it should fix the problem!

Comment 39

[canaar]

Done sir! :) I am attaching the patch.

Comment 40

[canaar]

Attachment: bug_1097876.diff

Collected UITelemetry about the panic(forget) button.

Comment 41

[Blake Winton (:bwinton) (:☕️)]

Awesome!  I'm looking forward to seeing what Florian says!  :)

Comment 42

[Florian Quèze [:florian]]

Comment on attachment 8564685 [details] [diff] [review]
bug_1097876.diff

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1013,5 @@
>        let doc = aEvent.target.ownerDocument;
>        this._ensureSanitizer();
>        this._sanitizer.range = this._getSanitizeRange(doc);
>        let group = doc.getElementById("PanelUI-panic-timeSpan");
> +      let cnt  = BrowserUITelemetry.countPanicEvent(group.selectedItem.id);

nit: double space between "cnt" and "=".

@@ +1033,5 @@
>          } else {
>            otherWindow.PanicButtonNotifierShouldNotify = true;
>          }
>        });
> +      return cnt;

Is this returned value ever used?

::: browser/modules/BrowserUITelemetry.jsm
@@ +589,5 @@
>  
>    countSearchSettingsEvent: function(source) {
>      this._countEvent(["click-builtin-item", source, "search-settings"]);
>    },
> +  

nit: trailing whitespace.

Comment 43

[canaar]

Sorry, "return cnt" is never used. I am adding the new patch with changes mentioned above.

Comment 44

[canaar]

Attachment: Collected UITelemetry about panic(forget) button. [Improved patch

Comment 45

[canaar]

Attachment: Collected UITelemetry about panic(forget) button. [Improved patch]

Comment 46

[Florian Quèze [:florian]]

Comment on attachment 8565347 [details] [diff] [review]
Collected UITelemetry about panic(forget) button. [Improved patch]

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

Thanks for the updated patch.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1013,5 @@
>        let doc = aEvent.target.ownerDocument;
>        this._ensureSanitizer();
>        this._sanitizer.range = this._getSanitizeRange(doc);
>        let group = doc.getElementById("PanelUI-panic-timeSpan");
> +      let cnt = BrowserUITelemetry.countPanicEvent(group.selectedItem.id);

If it's never going to be used, you don't need the 'cnt' variable at all.

::: browser/modules/BrowserUITelemetry.jsm
@@ +592,5 @@
>    },
>  
> +  countPanicEvent: function(timeId) {
> +    this._countEvent(["forget-button", timeId]);
> +  },

nit: keep an empty line before the _logAwesomeBarSearchResult method.

Comment 47

[canaar]

Attachment: Collected UITelemetry about panic(forget) button. [Improved patch]

Comment 48

[Florian Quèze [:florian]]

Comment on attachment 8565365 [details] [diff] [review]
Collected UITelemetry about panic(forget) button. [Improved patch]

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

Thanks, looks good! :-)

Comment 49

[canaar]

Thank you sir! :)

Comment 50

[Blake Winton (:bwinton) (:☕️)]

Okay, almost done.  The next steps are to make sure the patch passes all our tests, and then add "checkin-needed" to the keywords, and someone will check it in for you!  :)

To make sure it passes all the tests, you should push the code to our Try Server, but since I don't think you can do that yet, I've taken the liberty of doing it for you.  ;)
You can watch it run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f85c5acc521

Once it shows all green, feel free to add the checkin-needed keyword.  (If there are errors, needinfo me here, and I'll check them out and let you know what the next steps are.)

Comment 51

[Blake Winton (:bwinton) (:☕️)]

(As a side note, for your next patch, you probably want to read https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to make the patch easier for people to check in.  ;)  You'll also want to skip to the "If you don't want to use Mercurial Queues" section, because we're recommending people don't use Mercurial Queues these days.)

Comment 52

[Blake Winton (:bwinton) (:☕️)]

So I just checked, and the two errors we got seem intermittent, so you should be okay to add the checkin-needed keyword, Abhilash.  :)

Comment 53

[Florian Quèze [:florian]]

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

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/80eced23e8e0