Closed
Bug 1097942
Opened 10 years ago
Closed 9 years ago
UITour function for changing selected search provider
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
People
(Reporter: Gavin, Assigned: Gavin)
References
Details
Attachments
(1 file, 4 obsolete files)
4.88 KB,
patch
|
Details | Diff | Splinter Review |
It would be useful to have the ability to allow users to change their selected search engine from within a product tour. I envision an API like: Mozilla.UITour.selectEngine("Foo Engine"); whose implementation just sends a "selectEngine" message with the engine name as a parameter.
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
I guess this needs a test and associated mozilla-uitour pull request.
Attachment #8521653 -
Flags: feedback?(dolske)
Attachment #8521653 -
Flags: feedback?(bmcbride)
Updated•10 years ago
|
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
Comment on attachment 8521653 [details] [diff] [review] totally untested PoC Review of attachment 8521653 [details] [diff] [review]: ----------------------------------------------------------------- For the other search engine stuff in UITour, we only allow interacting with/detecting built-in search engines. I think we should do the same here, as it helps limit the privacy/security implications. ::: browser/modules/UITour.jsm @@ +459,5 @@ > + Cu.reportError("UITour: selectSearchEngine failed, engine not found"); > + return; > + } > + Services.search.currentEngine = engine; > + }).then(null, Cu.reportError); Use .catch() for error handling. @@ +1317,5 @@ > + getSearchEngine(aName) { > + return new Promise(resolve => { > + Services.search.init((rv) => { > + if (!Components.isSuccessCode(rv)) { > + resolve(null); Should be rejecting if we can't fulfill the promise.
Attachment #8521653 -
Flags: feedback?(dolske)
Attachment #8521653 -
Flags: feedback?(bmcbride)
Attachment #8521653 -
Flags: feedback+
Comment 3•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1) > associated mozilla-uitour pull request. I've retired that repo - we're just using the in-tree copy now (browser/modules/test/uitour.js). See also bug 1098071.
Assignee | ||
Comment 4•10 years ago
|
||
Switched to using the identifier, for consistency with the highlight method and getConfiguration (which allows testing for engine existence before trying to select it).
Attachment #8521653 -
Attachment is obsolete: true
Attachment #8523455 -
Flags: review?(dolske)
Attachment #8523455 -
Flags: review?(bmcbride)
Comment 5•10 years ago
|
||
Comment on attachment 8523455 [details] [diff] [review] patch with test Review of attachment 8523455 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/UITour.jsm @@ +484,5 @@ > }).catch(log.error); > break; > } > + > + case "selectSearchEngine": { Nit: Trailing whitespace. ::: browser/modules/test/browser_UITour.js @@ +378,5 @@ > + }); > + > + let searchChangePromise = promiseSearchTopic("engine-current"); > + gContentAPI.selectSearchEngine(otherEngineID); > + yield searchChangePromise; In the future, we may wish selectSearchEngine to take a callback to deal with the async-ness of it. Think it's fine for now though.
Attachment #8523455 -
Flags: review?(dolske)
Attachment #8523455 -
Flags: review?(bmcbride)
Attachment #8523455 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #5) > In the future, we may wish selectSearchEngine to take a callback to deal > with the async-ness of it. Think it's fine for now though. Yeah, it's a good idea, but I think not needed now. Thanks for the review!
Assignee | ||
Comment 7•10 years ago
|
||
Had to rework the test to not be dependent on taskify().
Attachment #8523455 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Made a slight tweak to use .defaultEngine consistently (rather than .currentEngine).
Comment 9•10 years ago
|
||
Hmm. The name "selectSearchEngine" doesn't make it obvious that this is permanently changing the default search engine for the user. Call it "setDefaultSearchEngine" instead? I'm a bit wary of some future tour/snippet code use this thinking it's just an ephemeral setting that reverts itself, and us waking up to find that we accidentally a half billion users to Wikipedia. Changing the name would go a small ways to making it clearer what this is actually doing. On that note, this is actually a pretty big footgun to expose. Did you consider limiting its scope? For example, only allowing selecting Google or Yahoo (instead of any installed engine), or having it open the new Search prefs UI instead of actually changing the default? I assume the only two interesting use cases are "revert to the old Google dangit" and "ok, let me try Yahoo instead of my non-Google default". Similarly, do we want to worry about add-ons using UITour as a sneaky way to change the user's search engine? Or is that already prohibited by the AMO policy around that?
Assignee | ||
Comment 10•10 years ago
|
||
Good thinking. I will rename to setDefaultSearchEngine. In the short term, I want to err on the side of flexibility vs. safety - we can always clap this down (or remove it wholesale) in a new release. Not concerned about abuse of this by add-ons, they already have sneakier ways to do it.
Assignee | ||
Comment 11•10 years ago
|
||
Ready to land.
Attachment #8525684 -
Attachment is obsolete: true
Attachment #8525687 -
Attachment is obsolete: true
Updated•10 years ago
|
status-firefox34:
--- → affected
tracking-firefox34:
--- → +
Assignee | ||
Updated•10 years ago
|
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f70b0aed4237
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•9 years ago
|
Target Milestone: Firefox 37 → Firefox 36
Updated•9 years ago
|
Iteration: --- → 37.1
Comment 15•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/eb8b2a6c5ea0
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
Updated•9 years ago
|
QA Contact: catalin.varga
Comment 16•9 years ago
|
||
Verified as fixed using the following environment: FF 34 Build Id:20141125180439 OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64 Manual verification was possible only on FF 34 the searchbar ui tour is available.
Comment 17•9 years ago
|
||
Hi Gavin, We're currently adding this to our UITour documentation - can you please point us to where the list of search engine identifiers is stored? We'd like to keep a list or reference a link to the possible options. Many thanks
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Alex Gibson [:agibson] from comment #17) > We're currently adding this to our UITour documentation - can you please > point us to where the list of search engine identifiers is stored? We'd like > to keep a list or reference a link to the possible options. There is no single list, unfortunately - it is based on the filename of the engines that we ship by default. For en-US builds, they are listed here: https://mxr.mozilla.org/mozilla-release/source/browser/locales/en-US/searchplugins/list.txt (with the exception of "ddg" for DuckDuckGo, which isn't listed there but is available. This discrepancy will be fixed by bug 1105092.) Other localized builds can have other engines specified, based on the list.txt files from those locales: https://mxr.mozilla.org/l10n-mozilla-release/find?string=browser%2Fsearchplugins%2Flist.txt
Flags: needinfo?(gavin.sharp)
Comment 19•9 years ago
|
||
Perfect, many thanks Gavin!
You need to log in
before you can comment on or make changes to this bug.
Description
•