bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Log selections of searchSuggestTable in newtab.

VERIFIED FIXED in Firefox 35

Status

()

Firefox
General
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: bwinton, Assigned: bwinton)

Tracking

Trunk
Firefox 35
x86
All
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
…when it lands, that is.
Flags: firefox-backlog+
(Assignee)

Comment 1

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

Updated

4 years ago
Blocks: 1036925

Updated

4 years ago
QA Whiteboard: [qa+]

Updated

4 years ago
Points: --- → 2

Updated

4 years ago
QA Whiteboard: [qa+]
Flags: qe-verify+

Updated

4 years ago
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 35.1
(Assignee)

Comment 2

4 years ago
Created attachment 8484208 [details] [diff] [review]
Log search suggestions in about:newtab.

This patch depends on the one for bug 1036908 to provide a bunch of the common code.
Attachment #8484208 - Flags: review?(mak77)
(Assignee)

Comment 3

4 years ago
(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}}}}
Depends on: 1036908
QA Contact: petruta.rasa
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.
Attachment #8484208 - Flags: review?(mak77)
(Assignee)

Comment 5

4 years ago
Created attachment 8484552 [details] [diff] [review]
The next iteration.

Fixed the selectionIndex check to match the other bug.  :)
Attachment #8484208 - Attachment is obsolete: true
Attachment #8484552 - Flags: review?(mak77)
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!
Attachment #8484552 - Flags: review?(mak77) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8485216 [details] [diff] [review]
The final version, ready for checkin.

And the same change made as in the other bug.
Attachment #8484552 - Attachment is obsolete: true
Attachment #8485216 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ceb9328526e0
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 10

4 years ago
Created attachment 8485759 [details] [diff] [review]
A version of the patch which doesn't break the tests.

Looks like this was it (by inspection)
Attachment #8485216 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
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…)
(Assignee)

Comment 12

4 years ago
Okay, given the green try-run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8be8501f1bf2 , I'm re-requesting checkin.  Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f22e16ee4c5a
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f22e16ee4c5a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 16

4 years ago
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…  :)
You need to log in before you can comment on or make changes to this bug.