1036912 - Log selections of searchSuggestTable in newtab.

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

…when it lands, that is.

Comment 1

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

"Selections" mean both "clicks on" or "enter keypresses when focused".
Also, be sure to log the type of selection (enter key or mouse click).

Comment 2

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

Attachment: Log search suggestions in about:newtab.

This patch depends on the one for bug 1036908 to provide a bunch of the common code.

Comment 3

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

(Hey Petruta, I've assigned this bug to you because it's so similar to bug 1036908.  Feel free to re-assign it if you want.  :)

To test this change:

Open a new tab
Make sure the search box is using Google.
Type "test".
Hit enter.

Open about:telemetry in a new tab.
Expand the "Simple Measurements" section.
Look for the "UITelemetry" key.
Make sure it contains:
"search":{"newtab":1}

Open a new tab
Type "test"
Hit arrow-down, then enter.

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"newtab":2,"selection":{"newtab":{"0":{"key":1}}}}

Open a new tab
Type "test"
Click on the first entry

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"newtab":3,"selection":{"newtab":{"0":{"key":1,"mouse":1}}}}

Open a new tab
Type "test"
Click on the second entry

Go back to about:telemetry
Refresh the page
Re-expand the "Simple Measurements" section.
Make sure the "UITelemetry" key now contains:
"search":{"newtab":4,"selection":{"newtab":{"0":{"key":1,"mouse":1},"1":{"mouse":1}}}}

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8484208 [details] [diff] [review]
Log search suggestions in about:newtab.

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

::: browser/base/content/newtab/search.js
@@ +44,5 @@
>          searchString: searchStr,
>          whence: "newtab",
> +      }
> +
> +      if (selectionIndex && selectionIndex != "-1") {

my comment is basically the same as for Bug 1036908, the selectionindex check is wrong, and -1 could "probably" be avoided by not setting it in the contentsearch at all.

Comment 5

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

Attachment: The next iteration.

Fixed the selectionIndex check to match the other bug.  :)

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8484552 [details] [diff] [review]
The next iteration.

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

::: browser/base/content/newtab/search.js
@@ +44,5 @@
>          searchString: searchStr,
>          whence: "newtab",
> +      }
> +
> +      if (selectionIndex !== "") {

same comment about using hasAttribute I made in the other bug.

Apart that, lgtm!

Comment 7

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

Attachment: The final version, ready for checkin.

And the same change made as in the other bug.

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Carsten Book [:Tomcat]]

sorry had to backout this changes (for Bug 1036914, Bug 1036912 and Bug 1036908) since one of this changes caused test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=47585310&tree=Fx-Team

Comment 10

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

Attachment: A version of the patch which doesn't break the tests.

Looks like this was it (by inspection)

Comment 11

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

Try run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8be8501f1bf2
If it's successful, I'll re-ask for checkin.

(Although, I'm getting a lot of seemingly-unrelated errors from a run with the previous version of the patch at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=14f042ccc93d so I'll probably re-ask for checkin if it fails and I can't see any related errors…)

Comment 12

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

Okay, given the green try-run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8be8501f1bf2 , I'm re-requesting checkin.  Thanks!

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f22e16ee4c5a

Comment 15

[Petruta Horea [:phorea], Desktop QA]

Thanks for the clear steps, Blake!

I've obtained the results specified in comment 3 using Nightly 35.0a1 20140912030202 under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 10.9.5.

Comment 16

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

You're very welcome, Petruta!  It seemed like a hard thing to test if you didn't know what the code was trying to do…  :)