UITour function for changing selected search provider

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 36
Points:
---
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox34+ verified, firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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+
Created attachment 8521653 [details] [diff] [review]
totally untested PoC

I guess this needs a test and associated mozilla-uitour pull request.
Attachment #8521653 - Flags: feedback?(dolske)
Attachment #8521653 - Flags: feedback?(bmcbride)

Updated

4 years ago
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
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+
(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.
Created attachment 8523455 [details] [diff] [review]
patch with test

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 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+
(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!
Blocks: 1101642
Created attachment 8525684 [details] [diff] [review]
beta patch

Had to rework the test to not be dependent on taskify().
Attachment #8523455 - Attachment is obsolete: true
Created attachment 8525687 [details] [diff] [review]
updated trunk patch

Made a slight tweak to use .defaultEngine consistently (rather than .currentEngine).
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?
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.
Created attachment 8526173 [details] [diff] [review]
final beta patch

Ready to land.
Attachment #8525684 - Attachment is obsolete: true
Attachment #8525687 - Attachment is obsolete: true
status-firefox34: --- → affected
tracking-firefox34: --- → +
status-firefox34: affected → fixed
Depends on: 1103127
No longer depends on: 1103127
https://hg.mozilla.org/mozilla-central/rev/f70b0aed4237
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37

Updated

4 years ago
Target Milestone: Firefox 37 → Firefox 36

Updated

4 years ago
Iteration: --- → 37.1
https://hg.mozilla.org/releases/mozilla-beta/rev/eb8b2a6c5ea0
status-firefox35: --- → fixed
status-firefox36: --- → fixed
QA Contact: catalin.varga
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.
status-firefox34: fixed → verified
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)
(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)
Perfect, many thanks Gavin!
We won;t be verifying this any further.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.