Closed Bug 1097876 Opened 5 years ago Closed 5 years ago

Collect UITelemetry about the Panic Button.

Categories

(Firefox :: Toolbars and Customization, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: bwinton, Assigned: abhilashmhaisne)

Details

Attachments

(1 file, 5 obsolete files)

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?".
Is this bug about this : https://addons.mozilla.org/en-US/firefox/addon/panic-button/
Flags: needinfo?(bwinton)
Good guess, but no.  It's about https://support.mozilla.org/en-US/kb/forget-button-quickly-delete-your-browsing-history instead.  :)
Flags: needinfo?(bwinton)
So what we need to do is collect UI Telemetry about quick forget button, such as how many times it was used, right? :)
Flags: needinfo?(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).
Flags: needinfo?(bwinton)
Alright.
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?
Flags: needinfo?(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.  :)
Flags: needinfo?(bwinton)
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
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.  ;)
Assignee: nobody → abhilashmhaisne
Status: NEW → ASSIGNED
Just to make sure, this function "countPanicEvent" should also be written in BrowserUITelemetry.jsm, right? :)
Yep.  Perhaps after the countSearchSettingsEvent function…
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]);
  },
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.
Okay, so like this :
  countPanicEvent: function(timeId) {
    this._countEvent(["forget-button", timeId]);
  },
Flags: needinfo?(bwinton)
Looks good to me!  Do you know where you'll be calling the new function?
Flags: needinfo?(bwinton)
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?
Flags: needinfo?(bwinton)
That looks right, and yes, you will probably need to declare a new variable.  :)
Flags: needinfo?(bwinton)
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?
(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!  :)
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?
Flags: needinfo?(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.  :)
Flags: needinfo?(bwinton)
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.
Flags: needinfo?(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.  :)
Flags: needinfo?(bwinton)
Okay, so I did that, but it now shows an error in the console, BrowserUITelemetry not defined. :)
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…)
Attached patch bug_1097876.diff (obsolete) — Splinter Review
Flags: needinfo?(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!  :)
Flags: needinfo?(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.  ;)
Flags: needinfo?(bwinton)
Sir, I passed "group.id" to the function, so now the appearance is like this :
"forget-button":{"PanelUI-panic-timeSpan":2}
Good to proceed?
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?
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? :)
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?
Attached patch bug_1097876.diff (obsolete) — Splinter Review
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?  ;)
I think it defaults the selected item to 5 min radio button, does it? :D :)
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!
Done sir! :) I am attaching the patch.
Attached patch bug_1097876.diff (obsolete) — Splinter Review
Collected UITelemetry about the panic(forget) button.
Flags: needinfo?(bwinton)
Attachment #8564685 - Flags: review?(florian)
Awesome!  I'm looking forward to seeing what Florian says!  :)
Flags: needinfo?(bwinton)
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.
Attachment #8564685 - Flags: review?(florian) → feedback+
Sorry, "return cnt" is never used. I am adding the new patch with changes mentioned above.
Flags: needinfo?(florian)
Flags: needinfo?(bwinton)
Attachment #8563490 - Attachment is obsolete: true
Attachment #8564557 - Attachment is obsolete: true
Attachment #8564685 - Attachment is obsolete: true
Attachment #8565346 - Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8565347 - Flags: review?(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.
Attachment #8565347 - Flags: review?(florian) → feedback+
Attachment #8565347 - Attachment is obsolete: true
Attachment #8565365 - Flags: review?(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! :-)
Attachment #8565365 - Flags: review?(florian) → review+
Thank you sir! :)
Flags: needinfo?(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.)
(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.)
So I just checked, and the two errors we got seem intermittent, so you should be okay to add the checkin-needed keyword, Abhilash.  :)
https://hg.mozilla.org/mozilla-central/rev/80eced23e8e0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.