Closed Bug 1097942 Opened 5 years ago Closed 5 years ago

UITour function for changing selected search provider

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
37.1
Tracking Status
firefox34 + verified
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

Attachments

(1 file, 4 obsolete files)

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+
Attached patch totally untested PoC (obsolete) — Splinter Review
I guess this needs a test and associated mozilla-uitour pull request.
Attachment #8521653 - Flags: feedback?(dolske)
Attachment #8521653 - Flags: feedback?(bmcbride)
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.
Attached patch patch with test (obsolete) — Splinter Review
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!
Attached patch beta patch (obsolete) — Splinter Review
Had to rework the test to not be dependent on taskify().
Attachment #8523455 - Attachment is obsolete: true
Attached patch updated trunk patch (obsolete) — Splinter Review
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.
Attached patch final beta patchSplinter Review
Ready to land.
Attachment #8525684 - Attachment is obsolete: true
Attachment #8525687 - Attachment is obsolete: true
Depends on: 1103127
No longer depends on: 1103127
https://hg.mozilla.org/mozilla-central/rev/f70b0aed4237
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 36
Iteration: --- → 37.1
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.
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.