1120957 - Searches from the "Paste & Search" context menu item should be counted in telemetry.

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Florian Quèze [:florian]]

Bug 1102937 added telemetry support for the new search bar UI from bug 1088660, but it didn't take into account the "Paste & Search" context menu item case. Searches performed from this context menu of the searchbar are currently counted as "unknown".

We should change the code at http://hg.mozilla.org/mozilla-central/annotate/bb8d6034f5f2/browser/components/search/content/search.xml#l819 to pass the command event to handleSearchCommand, and then handleSearchCommand should be modified to support the command event and look at the anonid of the node that received the event.

See bug 1120389 for the related discussion that caused me to file this. Blake offered to mentor this :-).

Comment 1

[Vikram Jadhav]

Hi,
I want to work for this bug.
I already build the Firefox code.

Comment 2

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

Welcome Vikram!

I've assigned the bug to you, so please feel free to start working on it!  Do the instructions from Florian give you enough information to get started?  If you have any questions about the code or the process of getting it checked in, you can ask me in a comment here, and I'll reply as soon as I can.

Thanks,
Blake.

Comment 3

[Vikram Jadhav]

Hi
Can you tell me little about what actualy I have to do? and what is the function of the search.xml code snipet.
Actually i recently joined the mozilla community, and solved plenty of bugs.
Thanks.

Comment 4

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

Hmm.  So, I've thought about it a little, and I think we want to call BrowserUITelemetry.registerContextMenuInteraction (in addition to all the stuff that's already there) when the code snippet at https://dxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#838 runs.  We'll also need to figure out what we should pass in to that function.

Comment 5

[Vikram Jadhav]

BrowserUITelemetry and registerContextMenuInteraction are not int the given search.xml file.
Actually I am not getting what to do!!
and how can I reproduce the bug in browser?
Thanks.

Comment 6

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

Okay, let's start with reproducing the bug.

Here are the steps I'm thinking of:
Select some text on a page, and copy it to the clipboard.
Right-click on the search bar and select the "Paste & Search" option.
Open about:telemetry in a tab.
Expand the "Simple Measurements" section, and look for the "UITelemetry" entry.
In the data beside it should be the word "contextmenu", and near there should be the word "paste-and-search".  (Currently the word "paste-and-search" isn't in there.  This bug is about adding it. :)

Does that make sense to you?  Let me know if it makes sense, or if you have any questions, and I'll explain a little more about what the fix should look like, and how it will work.

Thanks!

Comment 7

[Vikram Jadhav]

Yes! Great.
I got the idea! Thanks.
Now please tell me how to fix this.

Comment 8

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

Great!  So, registerContextMenuInteraction is a function defined on the BrowserUITelemetry object, which is in the global scope (which is why you don't see it defined).

So I think the next step is to add:
console.log('Got here!!!', BrowserUITelemetry.registerContextMenuInteraction);
to the string at https://dxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#838 and rebuild Firefox.  (As a side note, you can type "./mach build browser/components && ./mach run" to only rebuild the changed files, which will be _way_ faster!  :)

Once your changed version of Firefox is running, if you open the Browser Console (under Tools >> Web Developer >> Browser Console), then when you choose the "Paste & Search" menu item, you should see a line that has "Got Here!!!" and "function ()" in it.

If that works for you, then the next step will be to change the "console.log" to instead call "BrowserUITelemetry.registerContextMenuInteraction", and pass something in as parameters.

Comment 9

[Vikram Jadhav]

ok so when i do this step
console.log('Got here!!!', BrowserUITelemetry.registerContextMenuInteraction); in string
then paste and  search dosent work!!!

Comment 10

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

Do you see the console message?  And did you replace the entire string, or just add that to the front of the string?

Comment 11

[Vikram Jadhav]

replacement:

element.setAttribute("oncommand",
              "console.log('Got here!!!', BrowserUITelemetry.registerContextMenuInteraction);");

error:
(firefox:30868): Gtk-CRITICAL **: IA__gtk_clipboard_set_with_data: assertion `targets != NULL' failed

Comment 12

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

Okay.  So if you add it to the front instead of replacing it, it should work a little better.  Give that a try, and let me know how it goes!

Comment 13

[Vikram Jadhav]

is it like this.
element.setAttribute("oncommand",
              "console.log('Got here!!!', BrowserUITelemetry.registerContextMenuInteraction);
               BrowserSearch.searchBar.select(); goDoCommand('cmd_paste');      BrowserSearch.searchBar.handleSearchCommand();");

if yes so after paste and search option disappears.

Comment 14

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

I think the string needs to all be on one line, but other than that it looks okay to me…

Comment 15

[Vikram Jadhav]

here what i did
1) element.setAttribute("oncommand",
              "BrowserUITelemetry.registerContextMenuInteraction("Hieee");           BrowserSearch.searchBar.select();goDoCommand('cmd_paste');BrowserSearch.searchBar.handleSearchCommand();");
2) mach build
3) mach run
4) copy something to keyboard and right click and select paste and search

BUT the paste and search option is not there.

Comment 16

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

Ah!  Excellent!  So, "BrowserUITelemetry.registerContextMenuInteraction("Hieee")" isn't going to work, because registerContextMenuInteraction is expecting an eventKey and a target!  :)

The target should probably be 'paste-and-search'.
And let's try passing in ['["search"]', 'withoutcustom']

Comment 17

[Vikram Jadhav]

ya i did 
element.setAttribute("oncommand",
              "BrowserUITelemetry.registerContextMenuInteraction(['["search"]', 'withoutcustom']);
               BrowserSearch.searchBar.select(); goDoCommand('cmd_paste'); BrowserSearch.searchBar.handleSearchCommand();");

but still the option is not visible

Comment 18

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

Okay, I think we need a couple more tweaks…
Let's try:
element.setAttribute("oncommand", "BrowserUITelemetry.registerContextMenuInteraction('[\"search\"]', 'withoutcustom');BrowserSearch.searchBar.select(); goDoCommand('cmd_paste'); BrowserSearch.searchBar.handleSearchCommand();");

and see what happens…

(If that also doesn't work, let's undo all the changes, and see if we still get the problem.)

Comment 19

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

Whoops, I messed up the arguments in that code…

The first argument to registerContextMenuInteraction should be the array ['["search"]', 'withoutcustom'] (But note you'll need to escape the "s, because it's in a string!)

The second argument should be the string 'paste-and-search'.

(If you're on IRC and are still running into problems, feel free to message me directly.  That might be faster than trying to do it in the bug…  :)

Comment 20

[Vikram Jadhav]

Hi,
here what i did,

element.setAttribute("oncommand","BrowserUITelemetry.registerContextMenuInteraction(['[\"search\"]', 'withoutcustom'], 'paste-and-search');BrowserSearch.searchBar.select(); goDoCommand('cmd_paste'); BrowserSearch.searchBar.handleSearchCommand();");

But same problem is there the paste and search option is not there in right click menu!!

Comment 21

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

Okay, two more things to try:
1) You're sure you're right-clicking in the search bar, not the url bar…  (I know, it's a long shot, but it's worth asking!  ;)
2) Undo all the changes you made, and rebuild, and see if it shows up then.  (It should, since it will be the default Firefox code…)

Thank you for your persistence on this bug!  I'm sure we'll figure out what's going wrong soon…

Comment 22

[Vikram Jadhav]

Hi,
The problem in the ./mach build browser/components doesn't work when i did ./mach build it paste and search worked and i got the telemetry http://pastebin.mozilla.org/8261661 but the search gets failed even URL bar doesn't work.
i got (firefox:25658): Gtk-CRITICAL **: IA__gtk_clipboard_set_with_data: assertion `targets != NULL' failed this error.

Comment 23

[Vikram Jadhav]

Attachment: Bug1120957_context_menu_item_should_be_counted_in_telemetry.diff

here i uploaded the patch file.
Thanks.

Comment 24

[Vikram Jadhav]

HI,
could you review this patch?

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8551937 [details] [diff] [review]
Bug1120957_context_menu_item_should_be_counted_in_telemetry.diff

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

I have nothing against adding a BrowserUITelemetry.registerContextMenuInteraction call, but the initial intent of this bug was to stop logging these searches as "unknown" and have a meaningful value instead. See the code at http://hg.mozilla.org/mozilla-central/annotate/540077a30866/browser/components/search/content/search.xml#l508 and the discussion in bug 1120389 comment 2 and before.

::: browser/components/search/content/search.xml
@@ +833,5 @@
>            element = document.createElementNS(kXULNS, "menuitem");
>            label = this._stringBundle.getString("cmd_pasteAndSearch");
>            element.setAttribute("label", label);
>            element.setAttribute("anonid", "paste-and-search");
> +          

Please avoid adding trailing whitespace. I'm also not sure why an empty line is needed here.

@@ +838,2 @@
>            element.setAttribute("oncommand",
> +              "BrowserUITelemetry.registerContextMenuInteraction(['search', 'withoutcustom'], 'paste-and-search');BrowserSearch.searchBar.select(); goDoCommand('cmd_paste'); BrowserSearch.searchBar.handleSearchCommand();");

So much code inlined here isn't readable, could you please move this to a separate function somewhere?

Comment 26

[Vikram Jadhav]

Thanks for review!!
so where should i do changes?
can i talk with you on irc now? i unable to find you in #fx-team.

Comment 27

[Florian Quèze [:florian]]

(In reply to Vikram Jadhav from comment #26)
> Thanks for review!!
> so where should i do changes?

I'll let Blake continue to mentor you through this bug :-).

Comment 28

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

I believe Vikram and I have talked about this on IRC.

The one remaining question was "what do we pass", and the answer to that I think is "event", which should be defined for us in the event handler.  Vikram, please ping me on IRC if that doesn't work.  :)

Comment 29

[Vikram Jadhav]

Hi,
Here i made a first change, I passed the "event" in the handleSearchCommand method.
so is this right change
http://pastebin.mozilla.org/8440956

Comment 30

[Vikram Jadhav]

please check my recent hg diff! is this look good?
http://pastebin.mozilla.org/8441024

Comment 31

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

For the record, Vikram and I chatted about this patch on IRC, and he's moving forward with an approach we agreed on.

Comment 32

[Vikram Jadhav]

Hi,
The paste-and-search got logged, what's next step?
Thnaks

Comment 33

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

Huh.  I could have sworn I replied to this.  My apologies for the delay!

So the next step will be to put your patch in pastebin, and I'll take a look at it, and we can see what still needs doing.  (The final goal is to pass something better than "unknown" as the source parameter at hg.mozilla.org/mozilla-central/annotate/540077a30866/browser/components/search/content/search.xml#l526 )

Comment 34

[Vikram Jadhav]

Hi,
sorry for late reply, actually i was busy in my exams !!
here is the patch file- http://pastebin.mozilla.org/8760500
Thanks.

Comment 35

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

Okay!  So, we'll want to add an else case at line 523 that checks if the target's anonId attribute is 'paste-and-search', and if it is, set the "source" variable to "paste".  Sound good?

Comment 36

[Vikram Jadhav]

Ok here i made some changes...
http://pastebin.mozilla.org/8783734

Comment 37

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

I think we might be good to post it to the bug, with a little cleanup…

1) We can use "target" instead of "aEvent.originalTarget", to be more like the other cases.
2) We should probably use double-quotes (") around 'paste-and-search', to be consistent.
3) The spacing on the "source = "paste";" line looks way off…  Can you double-check that you're not using tabs?

Once those are fixed, post the full set of changes, and ask Florian for a review!

Thanks!  :)

Comment 38

[Vikram Jadhav]

Attachment: BUG1120957_paste_and_search_context_menu.diff

Here are the changes attachd the diff file
thanks.

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8565415 [details] [diff] [review]
BUG1120957_paste_and_search_context_menu.diff

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

(Florian Quèze [:florian] [:flo] from comment #25)

> @@ +838,2 @@
> >            element.setAttribute("oncommand",
> > +              "BrowserUITelemetry.registerContextMenuInteraction(['search', 'withoutcustom'], 'paste-and-search');BrowserSearch.searchBar.select(); goDoCommand('cmd_paste'); BrowserSearch.searchBar.handleSearchCommand();");
> 
> So much code inlined here isn't readable, could you please move this to a
> separate function somewhere?

It looks like this review comment hasn't been addressed yet.

Comment 40

[Vikram Jadhav]

Ok thanks,
So should i remove this?
element.setAttribute("oncommand",
> > +              "BrowserUITelemetry.registerContextMenuInteraction(['search', 'withoutcustom'], 'paste-and-search');BrowserSearch.searchBar.select(); goDoCommand('cmd_paste'); BrowserSearch.searchBar.handleSearchCommand();");

Comment 41

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

I think Florian is saying that you should add a new function, probably on the BrowserSearch object, and then move that code to the new function, and call the new function from the oncommand attribute.  Sound good?

Comment 42

[Vikram Jadhav]

Attachment: BUG1120957_paste_and_search.diff

Comment 43

[Florian Quèze [:florian]]

Comment on attachment 8565483 [details] [diff] [review]
BUG1120957_paste_and_search.diff

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

Moving the review request to Gijs who wrote the context-menu-telemetry code and will know better how we are supposed to interact with it.

::: browser/base/content/browser.js
@@ +3479,5 @@
>      if (engine) {
>        BrowserSearch.recordSearchInHealthReport(engine, "contextmenu");
>      }
>    },
> +  

nit: trailing whitespace.

@@ +3481,5 @@
>      }
>    },
> +  
> +  pasteAndSearch: function (event) {
> +    BrowserUITelemetry.registerContextMenuInteraction(["search", "withoutcustom"], "paste-and-search");

Without the change you used to have at https://bugzilla.mozilla.org/attachment.cgi?id=8551937&action=diff#a/browser/modules/BrowserUITelemetry.jsm_sec1 this line will count the event as "other-item".

Gijs, is this something we can call for any context menu, or is it meant only for the context menu of the content area?

Comment 44

[Vikram Jadhav]

Hello,
can you tell me what florian sir want's to say?
actually I didn't understand the comment.
Thanks.

Comment 45

[:Gijs (he/him)]

Comment on attachment 8565483 [details] [diff] [review]
BUG1120957_paste_and_search.diff

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

::: browser/base/content/browser.js
@@ +3481,5 @@
>      }
>    },
> +  
> +  pasteAndSearch: function (event) {
> +    BrowserUITelemetry.registerContextMenuInteraction(["search", "withoutcustom"], "paste-and-search");

It's currently meant only for the content area's context menu, so I don't think this is a good idea.

Looks like this should be doing the same as https://bug1102937.bugzilla.mozilla.org/attachment.cgi?id=8544913 and define a function that counts an event like "search-paste" in the same way it counts search-oneoff?

Comment 46

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

Gijs and Florian are saying that we should probably remove the "BrowserUITelemetry.registerContextMenuInteraction" call, since that function is only for tracking right-clicks in the web page, not on the browser.

(Also, the next review request should go to Gijs.  ;)

Comment 47

[Vikram Jadhav]

hello vikram, from BUG 1120957 here are the recent changes https://pastebin.mozilla.org/8824452

Comment 48

[:Gijs (he/him)]

Can you attach this as a regular patch, including a commit message with the bug number and a short sentence describing what the patch does? 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 .

Comment 49

[Vikram Jadhav]

Attachment: In search.xml i first checked if the event is paste-and-searh then the source value will be "search", and created a new function in browser.js i.e pasteAndSearch

Comment 50

[:Gijs (he/him)]

Comment on attachment 8573907 [details] [diff] [review]
In search.xml i first checked if the event is paste-and-searh then the source value will be "search", and created a new function in browser.js i.e pasteAndSearch

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

I think this makes sense. Blake, can you doublecheck the telemetry stuff?

Note the spacing issue below. Can you update the patch to use the correct indentation? Your patch also doesn't have a commit message. If you're using hg qnew/qrefresh, use the --edit or --message option to set one. If you're using real commits, you can change it by using hg commit --amend --message followed by your commit message.

::: browser/base/content/browser.js
@@ +3483,5 @@
>  
> +  pasteAndSearch: function (event) {
> +  	BrowserSearch.searchBar.select();
> +  	goDoCommand("cmd_paste");
> +  	BrowserSearch.searchBar.handleSearchCommand(event);

You should indent the function body with only spaces (specifically: two spaces more than the "pasteAndSearch" line above this), and not use tabs.

Comment 51

[Vikram Jadhav]

Attachment: Changed the indendation

Comment 52

[:Gijs (he/him)]

Comment on attachment 8573944 [details] [diff] [review]
Changed the indendation

(In reply to Vikram Jadhav from comment #51)
> Created attachment 8573944 [details] [diff] [review]
> Changed the indendation

Can you use a commit message of the form "Bug 1120957 - fix the source of searches made with paste & search, r=gijs,bwinton" and request the next review from bwinton? :-)

Comment 53

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

Comment on attachment 8573944 [details] [diff] [review]
Changed the indendation

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

::: browser/components/search/content/search.xml
@@ +520,5 @@
>                } else if (target.classList.contains("search-panel-header") ||
>                           target.parentNode.classList.contains("search-panel-header")) {
>                  source = "header";
> +              } else if (target.getAttribute("anonid") == "paste-and-search") {
> +                source = "paste";

So, I tried to run this, but when I selected the "paste-and-search" menu item, I didn't see the string "paste" in about:telemetry…

Digging in a little, it looks like the event we get isn't a mouse event, but is a XULCommandEvent instead, so we'll need to move this else-if out to a new block which tests for XULCommandEvent (like the way this tests for MouseEvent).

Comment 54

[Vikram Jadhav]

Attachment: Paste and search bug

Comment 55

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

Comment on attachment 8574770 [details] [diff] [review]
Paste and search bug

The code is good, but you forgot to update the commit message.  Please post a new patch with the better commit message before marking the bug as checkin-needed.  :)

Thank you!

Comment 56

[Vikram Jadhav]

Attachment: bug1120957_paste_and_search_March6.diff

Bug 1120957 - fix the source of searches made with paste & search

Comment 57

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

Comment on attachment 8574796 [details] [diff] [review]
bug1120957_paste_and_search_March6.diff

Excellent, thank you!

Comment 58

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

And https://treeherder.mozilla.org/#/jobs?repo=try&revision=006051a5106c just in case.

Comment 59

[Carsten Book [:Tomcat]]

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

Comment 60

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

https://hg.mozilla.org/mozilla-central/rev/52f076261baa