no longer sending homepage search params for about:home searches

VERIFIED FIXED in Firefox 26

Status

()

Firefox
Search
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

({regression})

26 Branch
Firefox 27
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 unaffected, firefox26+ verified, firefox27+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The patch in bug caused us to stop sending the "homepage" reason to getSubmission, for about:home-triggered searches. That's bad!
This has the effect of altering the search params sent for Google searches and prevents us from being able to count about:home searches separately.
status-firefox25: --- → unaffected
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: --- → +
tracking-firefox27: --- → +
Created attachment 816047 [details] [diff] [review]
patch

Restoring the logic from http://hg.mozilla.org/mozilla-central/rev/75d9df61d0e1#l4.18.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #816047 - Flags: review?(mnoorenberghe+bmo)
I'll try to write a test.
Comment on attachment 816047 [details] [diff] [review]
patch

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

Sounds like we need a test. I though there was one before.
Attachment #816047 - Flags: review?(mnoorenberghe+bmo) → review+
Created attachment 816106 [details] [diff] [review]
with test

Adds a Google-specific test that covers actual search touch points. In retrospect, a generic test that just tests that we pass the appropriate "purpose" arguments with a custom search plugin would probably be sufficient and a bit cleaner, but I guess it doesn't hurt that much to be specific (gets us coverage of the case we actually care about).

I copied some of this from the existing browser_google.js, and some bits and pieces from browser_aboutHome.js and browser_keywordSearch.js.

I'm not quite happy with the "context menu" test - it's cheating a bit by calling the relevant function directly - but actually testing the context menu was a pain in the ass so I bailed.
Attachment #816047 - Attachment is obsolete: true
Attachment #816106 - Flags: review?(mnoorenberghe+bmo)
https://tbpl.mozilla.org/?tree=Try&rev=46836e970196
Comment on attachment 816106 [details] [diff] [review]
with test

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

The about:home part of the test seems overly complicated but I trust you that it's necessary.
Attachment #816106 - Flags: review?(mnoorenberghe+bmo) → review+
https://hg.mozilla.org/integration/fx-team/rev/c85aeddc20e4
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/mozilla-central/rev/c85aeddc20e4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox27: affected → fixed
Resolution: --- → FIXED
Flags: in-testsuite+
Comment on attachment 816106 [details] [diff] [review]
with test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 900865
User impact if declined: none (but we still care)
Testing completed (on m-c, etc.): N/A
Risk to taking this patch (and alternatives if risky): no risk, well tested
String or IDL/UUID changes made by this patch: none
Attachment #816106 - Flags: approval-mozilla-aurora?
Attachment #816106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed

Updated

4 years ago
Keywords: verifyme

Updated

4 years ago
Whiteboard: [checkin-needed-aurora]
gavin, what are the steps to verify this?  

this is what I see in url's:

urlbar search: https://www.google.com/search?q=football&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=firefox-aurora&channel=fflb

searchbar search: https://www.google.com/search?q=football&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=firefox-aurora

about:home search: https://www.google.com/search?q=football&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=firefox-aurora

Nightly is similar, except with client=firefox-nightly
My nightly loads:

https://www.google.com/search?q=test&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=firefox-a&channel=np&source=hp

when I search for "test" from about:home.

That "channel=np&source=hp" would be missing prior to this bug being fixed.

(The patch hasn't landed on Aurora yet.)
ok, I get the correct URL on Nightly:

https://www.google.com/search?q=test&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=firefox-nightly&channel=np&source=hp
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
https://hg.mozilla.org/releases/mozilla-aurora/rev/4253a66382fa
status-firefox26: affected → fixed
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/4253a66382fa

Test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29665507&tree=Mozilla-Aurora

Backed out:
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/844d1594df32
status-firefox26: fixed → affected
Thanks Ed, my fault for not paying close enough attention. The newly-added test needed to be tweaked since bug 925892 wasn't uplifted.
Created attachment 822421 [details] [diff] [review]
aurora patch

I have to step away; if someone could land this revised patch for me that'd be great!
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f5c9b25baf4
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
status-firefox26: affected → fixed

Comment 19

4 years ago
Verified as fixed on Firefox 26b1 - 20131028225529 - on Windows 7, Ubuntu 13.04 and Mac OS X 10.9.
status-firefox26: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.