Closed
Bug 925898
Opened 9 years ago
Closed 9 years ago
no longer sending homepage search params for about:home searches
Categories
(Firefox :: Search, defect)
Tracking
()
VERIFIED
FIXED
Firefox 27
Tracking | Status | |
---|---|---|
firefox25 | --- | unaffected |
firefox26 | + | verified |
firefox27 | + | verified |
People
(Reporter: Gavin, Assigned: Gavin)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
9.14 KB,
patch
|
MattN
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.94 KB,
patch
|
Details | Diff | Splinter Review |
The patch in bug caused us to stop sending the "homepage" reason to getSubmission, for about:home-triggered searches. That's bad!
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
status-firefox25:
--- → unaffected
status-firefox26:
--- → affected
status-firefox27:
--- → affected
tracking-firefox26:
--- → +
tracking-firefox27:
--- → +
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
I'll try to write a test.
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=46836e970196
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c85aeddc20e4
Target Milestone: --- → Firefox 27
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c85aeddc20e4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 10•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #816106 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Whiteboard: [checkin-needed-aurora]
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
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.)
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4253a66382fa
Comment 15•9 years ago
|
||
(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
Assignee | ||
Comment 16•9 years ago
|
||
Thanks Ed, my fault for not paying close enough attention. The newly-added test needed to be tweaked since bug 925892 wasn't uplifted.
Assignee | ||
Comment 17•9 years ago
|
||
I have to step away; if someone could land this revised patch for me that'd be great!
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f5c9b25baf4
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora]
Updated•9 years ago
|
Comment 19•9 years ago
|
||
Verified as fixed on Firefox 26b1 - 20131028225529 - on Windows 7, Ubuntu 13.04 and Mac OS X 10.9.
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•