This is a footgun, based on how many partner code setups work. bug 1181605 is one example of this being a footgun. The easy fix is to default to "searchbar" if there's no purpose passed, similar to how we assume text/html if unspecified. The slightly harder fix is to change the API to obsolete the footgun API with optional args, and deprecate getSubmission. That'd probably impact mobile more, since that's the only non-HTML/non-suggestion use for responseType, but I think the disconnect between how mobile and desktop handle similar cases is relatively busted and another form of risk. Not blocking any release, but we should get on top of this ASAP.
Created attachment 8745598 [details] [diff] [review] Patch to default to searchbar This patch does the first thing Mike requested which is default searches without a purpose or with an invalid purpose to searchbar. It did require some major test changes.
Attachment #8745598 - Flags: review?(florian)
Comment on attachment 8745598 [details] [diff] [review] Patch to default to searchbar Review of attachment 8745598 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks fine (I would like to see green try results for browser-chrome and xpcshell tests before you push it), but it doesn't do what you describe. It'll only fallback to searchbar for purposes not specified in the searchplugin, but keeps a special handling of the empty/unspecified purpose. If you want to remove this special behavior, you should change "var purpose = aPurpose || "";" to "var purpose = aPurpose || "searchbar";", and adapt http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_purpose.js?rev=2bc25894eced#46 I think the new behavior you implement needs to be covered by test_purpose.js anyway (lack of xpcshell test coverage is the reason for not r+'ing).
Attachment #8745598 - Flags: review?(florian) → feedback+
Created attachment 8746632 [details] [diff] [review] Update purpose xpcshell test Addressed comments. Working on the try build now.
Attachment #8746632 - Flags: review?(florian)
Attachment #8746632 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/884060dc6f21feb4da594d02e63e5ded5700fe87 Bug 1181645 - Default invalid or empty purpose in getSubmission to searchbar, r=florian.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Created attachment 8747658 [details] [diff] [review] Patch, as landed Approval Request Comment [Feature/regressing bug #]: Bug 1266462. This patch is the clean/correct fix to replace for 47 and later the workaround that landed in bug 1266462. [User impact if declined]: No direct user impact, the impact is sending incorrect partner codes for searches from the location bar. [Describe test coverage new/current, TreeHerder]: The search service changes in the patch are covered by the test_purpose.js changes in the patch. The google.xml changes are covered by patches from bug 1266783. [Risks and why]: Acceptable for beta in my opinion. The patch slightly changes how we handle purpose parameters in search plugins, but to the best of my knowledge no search plugin ever used the behaviors we removed. It's slightly higher risk than the workaround we landed in bug 1266462, but a much cleaner long term solution. [String/UUID change made/needed]: none.
status-firefox47: --- → affected
status-firefox48: --- → affected
Comment on attachment 8747658 [details] [diff] [review] Patch, as landed This is something planned for Fx47, the sooner we uplift the better, also happy to see the patch includes new automated tests, Beta47+, Aurora48+
status-firefox48: affected → fixed
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.