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 :-).

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

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.

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.

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.

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.

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!

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

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.

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

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

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

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!

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.

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

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.

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']

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

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.)

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… :)

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!!

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…

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.

here i uploaded the patch file. Thanks.

HI, could you review this patch?

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?

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.

(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 :-).

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. :)

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

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

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

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

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 )

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

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?

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

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! :)

Here are the changes attachd the diff file thanks.

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.

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();");

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 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?

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

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?

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. ;)

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

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 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 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 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 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!

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

Comment on attachment 8574796 [details] [diff] [review] bug1120957_paste_and_search_March6.diff Excellent, thank you!

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