1119873 - Add UITelemetry for the search-go-button.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We're measuring clicks on a lot of other buttons already, this should be one of them.

Comment 1

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

It looks like we'll need to add an extra check near line https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#461 similar to the if-block above that line, but checking item.getAttribute('anonid') instead of item.id.

Comment 2

[Adrian C]

Hi, first time beginner and I would like to work on this bug :) I will need some mentorship to get setup please

Comment 3

[Adrian C]

(In reply to Blake Winton (:bwinton) from comment #1)
> It looks like we'll need to add an extra check near line
> https://dxr.mozilla.org/mozilla-central/source/browser/modules/
> BrowserUITelemetry.jsm#461 similar to the if-block above that line, but
> checking item.getAttribute('anonid') instead of item.id.

Do you mean similar to this line:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#454

OR this line:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#446

In other words, do you suggest I special case the search-go-button like line 454 or test all anonid's against ALL_BUILTIN_ITEMS like line 446?

Comment 4

[Adrian C]

I should mention that if I were to "test the anonid against ALL_BUILTIN_ITEMS like line 446" it would involve me adding the "search-go-button" to the SPECIAL_CASES of ALL_BUILTIN_ITEMS here: https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#122 

In other words, would it be acceptable to put "search-go-button" in SPECIAL_CASES and check anonid's going forward? Or should I simply treat "search-go-button" as a special case similar to https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#454

Comment 5

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

Hi Adrian, thanks for taking the bug!  :)

> In other words, would it be acceptable to put "search-go-button" in
> SPECIAL_CASES and check anonid's going forward?

I think this is a good plan.  If there were more things like this, then we might want to create a new ALL_ANONYMOUS_ITEMS array, but for now, let's just put it in the SPECIAL_CASES.

(And if you have any other questions, please put my email address in the "needinfo" field, to make sure I see it!)

Comment 6

[Adrian C]

Attachment: fix_add_uitelemetry_for_search.patch

Hi Blake, can you please review my patch :) Unsure if I should be writing a test for this but let me know if I need to. Also I am using Git so double check that I did the patch right (I was on master when I did git format-patch -k fx-team and then I used moz-git-tools to convert to an hg patch)

Comment 7

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

In general, we want to have tests for each patch, but the UITelemetry stuff doesn't have a lot of automated tests, so we're asking our QA volunteers to verify the patch.  If you could post a set of steps for them, like the ones in https://bugzilla.mozilla.org/show_bug.cgi?id=1102937#c31, that would be very helpful!

Comment 8

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

Comment on attachment 8550063 [details] [diff] [review]
fix_add_uitelemetry_for_search.patch

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

The patch looks good.  I might change the commit message to be in the present tense, and add reviewers, like:
Bug 1119873 - Add the search-go-button as a SPECIAL_CASE in UITelemetry and check anonids because search-go-button uses that instead of id.  r=florian.
but other than that, it seems good to me.  (Also, Florian would be a good choice for reviewing the next patch.  ;)

::: browser/modules/BrowserUITelemetry.jsm
@@ +459,5 @@
>        return;
>      }
>  
> +    // If not, we need to check if the item's anonid is in our list
> +    // of built-in items to check 

Nit: There's a trailing space at the end of this line.  We should probably replace it with a ".".  :)

Comment 9

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

Hi Adrian, thanks for your work on this bug!  While we're waiting for the next review, we've got another bug that might be up your alley…  It's the same general idea as this one, but is a little more involved.  Would you like to take a look at bug 1097876 and see if you want to take a stab at fixing it?

Comment 10

[Adrian C]

Attachment: 0134-Bug-1119873-Add-the-search-go-button-as-a-SPECIAL_CA.patch

Hi Blake, I have made the changes you suggested:

(1) I changed the commit message to be in the present tense 
(2) I removed the blank space and replaced it with a "."
(3) I made florian a reviewer in the commit but what is his/her email. Can you please add her/him as a reviewer in Bugzilla.

Let me know if there is anything else.

Comment 11

[Adrian C]

Attachment: Screen Shot 2015-01-16 at 9.38.34 PM.png

(In reply to Blake Winton (:bwinton) from comment #7)
> In general, we want to have tests for each patch, but the UITelemetry stuff
> doesn't have a lot of automated tests, so we're asking our QA volunteers to
> verify the patch.  If you could post a set of steps for them, like the ones
> in https://bugzilla.mozilla.org/show_bug.cgi?id=1102937#c31, that would be
> very helpful!

Hi Blake,

I am actually a bit confused what is the "search-go-button" I have attached a screenshot of what I think it is: the magnifying glass. Is this the "search-go-button"? Can you please let me know how to get to the "search-go-button" so I can write the verification steps.

Comment 12

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

Comment on attachment 8550656 [details] [diff] [review]
0134-Bug-1119873-Add-the-search-go-button-as-a-SPECIAL_CA.patch

(In reply to Adrian C from comment #10)
> (3) I made florian a reviewer in the commit but what is his/her email. Can
> you please add her/him as a reviewer in Bugzilla.

As a reasonably standard convention, people put ":ircname" in their bugzilla name, so you can put ":florian" in the review request field, and it will auto-complete to Florian's email address.  (I've done it for you in this case, but it's a useful tip.  :)

> I am actually a bit confused what is the "search-go-button" I have attached
> a screenshot of what I think it is: the magnifying glass. Is this the
> "search-go-button"? Can you please let me know how to get to the
> "search-go-button" so I can write the verification steps.

If you type some text in the field, a little right-pointing arrow will appear on the right-hand side of the field.  That's the search-go-button!

Comment 13

[Adrian C]

(In reply to Blake Winton (:bwinton) from comment #12)
> Comment on attachment 8550656 [details] [diff] [review]
> 0134-Bug-1119873-Add-the-search-go-button-as-a-SPECIAL_CA.patch
> 
> (In reply to Adrian C from comment #10)
> > (3) I made florian a reviewer in the commit but what is his/her email. Can
> > you please add her/him as a reviewer in Bugzilla.
> 
> As a reasonably standard convention, people put ":ircname" in their bugzilla
> name, so you can put ":florian" in the review request field, and it will
> auto-complete to Florian's email address.  (I've done it for you in this
> case, but it's a useful tip.  :)
> 
> > I am actually a bit confused what is the "search-go-button" I have attached
> > a screenshot of what I think it is: the magnifying glass. Is this the
> > "search-go-button"? Can you please let me know how to get to the
> > "search-go-button" so I can write the verification steps.
> 
> If you type some text in the field, a little right-pointing arrow will
> appear on the right-hand side of the field.  That's the search-go-button!

Hi Blake,

I am trying to test my patch but I cannot get about:telemetry to track anything. My steps are:

1. Enable telemetry via about:telemetry
2. Click search-go-button I also tried other buttons that I think have tracking i.e. back-button / forward-button
3. Wait a few hours and check back. Nothing

Btw I am on master when I did the above steps. Furthermore, I should mention that I have done this to build:

1. In my repo I have directly modified https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm
2. I did ./mach build

Am I doing something wrong?

Comment 14

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

If you hit ctrl-R on the about:telemetry page, it should show up immediately.
Does anything show up in the SimpleMeasurements section?
It sounds like you're doing everything fine, so there's definitely something odd going on…  :)
(Also you could try adding some console.log statements, and then opening up the Browser Console (under Tools >> Web Developer) to see if you're getting to the code…

Comment 15

[Adrian C]

(In reply to Blake Winton (:bwinton) from comment #14)
> If you hit ctrl-R on the about:telemetry page, it should show up immediately.
> Does anything show up in the SimpleMeasurements section?
> It sounds like you're doing everything fine, so there's definitely something
> odd going on…  :)
> (Also you could try adding some console.log statements, and then opening up
> the Browser Console (under Tools >> Web Developer) to see if you're getting
> to the code…

I managed to get it working :) I can now write the verification steps :)

Comment 16

[Adrian C]

Steps to verify:

Note: The event shows up in about:telemetry under Simple Measurements in UITelemetry

------------------------
Type something in the search bar via the toolbar and click the search-go-button that appears when the field is not empty
  Should log a "toolbars" > "countableEvents" > "__DEFAULT__" > "click-builtin-item" > "search-go-button" > "left" > 1

The 1 is a counter so this will increment each type you click the search-go-button.
------------------------

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8550656 [details] [diff] [review]
0134-Bug-1119873-Add-the-search-go-button-as-a-SPECIAL_CA.patch

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

Looks good to me. I just pointed out one coding style detail to fix.

In the future, could you please configure your mercurial to generate patches with 8 lines of context? See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F thanks!

::: browser/modules/BrowserUITelemetry.jsm
@@ +461,5 @@
>  
> +    // If not, we need to check if the item's anonid is in our list
> +    // of built-in items to check.
> +    if (ALL_BUILTIN_ITEMS.indexOf(item.getAttribute('anonid')) != -1) {
> +      this._countMouseUpEvent("click-builtin-item", item.getAttribute('anonid'), aEvent.button);

nit: The rest of this file uses double quotes ("), please change the single quotes (') around anonid to double quotes on these two lines.

Comment 18

[Adrian C]

Finally making the changes you requested, sorry I have been sick and busy with school.

I have changed the patch:

(a) I removed single quotes
(b) I have or at least I hope I have used 8 lines of context (I use git so what I did was git config --global diff.context 8 which I hope applies to git format-patch but let me know if this is not the case)

Hope this patch is good :)

Comment 19

[Adrian C]

Attachment: 1325-Bug-1119873-Add-the-search-go-button-as-a-SPECIAL_CA.patch

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8554342 [details] [diff] [review]
1325-Bug-1119873-Add-the-search-go-button-as-a-SPECIAL_CA.patch

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

Looks good, thanks!

Comment 21

[Florian Quèze [:florian]]

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

Comment 22

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/0dc8aa096aca