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.

Restoring the logic from http://hg.mozilla.org/mozilla-central/rev/75d9df61d0e1#l4.18.

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.

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.

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.

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

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

(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

Thanks Ed, my fault for not paying close enough attention. The newly-added test needed to be tweaked since bug 925892 wasn't uplifted.

I have to step away; if someone could land this revised patch for me that'd be great!

Verified as fixed on Firefox 26b1 - 20131028225529 - on Windows 7, Ubuntu 13.04 and Mac OS X 10.9.