1065891 - Keep Fennec default search engine pref in sync with search activity default pref

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

No matter what, we'll want to store the default search activity engine in SharedPreferences, so that we don't depend on gecko to launch the search activity, but we could update that when the user changes their default search engine in Fennec.

I don't think this is a high priority right now, but I wanted to file this so we don't lose track.

Comment 1

[:Margaret Leibovic]

MVP: Remove "Search engine" from search activity settings, and whenever you change your search engine in Fennec settings, we also change the search engine setting.

Next level: Keep a "Search engine" item, but make that jump into Fennec settings.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

It still seems a bit weird to me to have the user have to "change apps" (back out and tap another app icon) to change their default search engine. It's also weird to only have 1 thing in the settings that could probably be done without having them enter another page.

Maybe some telemetry on how often people are tapping this settings icon, and how often people actually change default search providers in Firefox for Android, could help shed some light on this.

Thoughts?

Comment 3

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #2)
> It still seems a bit weird to me to have the user have to "change apps"
> (back out and tap another app icon) to change their default search engine.
> It's also weird to only have 1 thing in the settings that could probably be
> done without having them enter another page.

I would argue that the user should think of the search activity as a different view in the same app (which it is). But yeah, not having much in the settings seems kinda awkward, so it would be nice to keep a "Search engine" item that launches the user into Fennec settings.

> Maybe some telemetry on how often people are tapping this settings icon, and
> how often people actually change default search providers in Firefox for
> Android, could help shed some light on this.

We have telemetry for the actions in settings, but not for opening settings itself. Can you file a bug on that?

Comment 4

[Karen Rudnitski [:kar]]

Instead of shoehorning in a 'search engine' item, why not something else? like an 'about' page or ability to turn off search suggestions in this view?

Comment 5

[:Margaret Leibovic]

(In reply to Karen Rudnitski [:kar] from comment #4)
> Instead of shoehorning in a 'search engine' item, why not something else?
> like an 'about' page or ability to turn off search suggestions in this view?

I'm not sure I understand this comment. I think we should still give users the opportunity to change their search engine from the search activity settings, but we'll just do it by launching them into the Fennec settings. So maybe "Search engine" isn't the right term, maybe something like "Firefox search settings" would be more appropriate way to link them to the Fennec settings.

But yes, there are other options we could add in the search activity settings, especially if we add more functionality in the future.

Unfortunately, while working on this bug, I realized there are some new problems we'll need to address if we want to only maintain one default search engine for both the browser and the search activity:

1) The user might have changed their default search engine before updating to a version of Firefox with the search activity, so we'd need to somehow pre-populate the shared pref used for the search activity. However, if the user launches the search activity before launching the browser we wouldn't know their default pref. We could fall back to the default for their locale in this case, but it seems like it would be confusing if the default changes after they open the browser. I'm not sure if there's any good way to address this.

2) The user may set a third-party search engine as their default in Fennec, and right now we only support built-in search plugins in the search activity. Since we're not using the gecko search service, we'd have to add something to the search service to deal with this, and I'm not sure how complicated this would be. Once again, in this case we could fall back to the default engine for the user's locale for the search activity, but then the user would be worse off than they currently are, since they wouldn't be able to choose a different built-in engine.

Comment 6

[:Margaret Leibovic]

Attachment: (Part 1) Remove search engine pref from search activity settings screen

Uploading some WIP patches. However, I want us to figure out answers to the 2 points I raised in my last comment before we land these.

Comment 7

[:Margaret Leibovic]

Attachment: (Part 2) Update the search activity default engine when the gecko default search engine changes

I decided to put this logic inside browser.js instead of the settings screen, since your default engine can change for a number of reasons, including if you remove the current default, or if an add-on changes things. Listening for changes to the search service directly gives us the highest quality information.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 3) Open Fennec search settings from search activity settings

It would be really nice if we could just launch the search preferences fragment directly, but we need to force gecko to launch first for it to work (I tried loading the search preferences directly, and it worked great if gecko was already running, but totally failed if it wasn't).

There are a few issues with this patch currently though:

1) If Fennec isn't running yet, it takes some time for this preferences screen to open.

2) The browser UI gets put in the back stack, so hitting back from the preferences screen will take you to the browser before taking you back to the search activity.

3) The title at the top of the search preferences screen isn't being updated properly.

Comment 9

[Karen Rudnitski [:kar]]

>I think we should still give users
> the opportunity to change their search engine from the search activity
> settings, but we'll just do it by launching them into the Fennec settings.
> So maybe "Search engine" isn't the right term, maybe something like "Firefox
> search settings" would be more appropriate way to link them to the Fennec
> settings.

I agree that this is where we want to be, but in case it couldn't be achieved in the Fx35 timeframe, I didn't want us to come up with a non-optimal way of doing this. If we can jump into the correct settings page in the browser itself without a performance hit in Fx35, fabulous - otherwise let's not squeeze it in for the first release and add that in the following one. 
 
> 1) The user might have changed their default search engine before updating
> to a version of Firefox with the search activity, so we'd need to somehow
> pre-populate the shared pref used for the search activity. However, if the
> user launches the search activity before launching the browser we wouldn't
> know their default pref. We could fall back to the default for their locale
> in this case, but it seems like it would be confusing if the default changes
> after they open the browser. I'm not sure if there's any good way to address
> this.

I would imagine this is more of an edge case, and if we can get the jump-in to the browser search settings solved, this could mitigate the pain for these users who have found themselves in this scenario.

> 2) The user may set a third-party search engine as their default in Fennec,
> and right now we only support built-in search plugins in the search
> activity. Since we're not using the gecko search service, we'd have to add
> something to the search service to deal with this, and I'm not sure how
> complicated this would be. Once again, in this case we could fall back to
> the default engine for the user's locale for the search activity, but then
> the user would be worse off than they currently are, since they wouldn't be
> able to choose a different built-in engine.

Could we know that this is the case? ie that the default in the browser settings isn't supported? if that's the case, could we have a visual indicator to prompt the user to understand why it may be different (and push it out to a sumo article)? Again, I would see this as an edge case, based on current search numbers. I wouldn't see this as a blocker for fx35, and hope we can find a way to notify the user.

Comment 10

[:Margaret Leibovic]

(In reply to Karen Rudnitski [:kar] from comment #9)
> >I think we should still give users
> > the opportunity to change their search engine from the search activity
> > settings, but we'll just do it by launching them into the Fennec settings.
> > So maybe "Search engine" isn't the right term, maybe something like "Firefox
> > search settings" would be more appropriate way to link them to the Fennec
> > settings.
> 
> I agree that this is where we want to be, but in case it couldn't be
> achieved in the Fx35 timeframe, I didn't want us to come up with a
> non-optimal way of doing this. If we can jump into the correct settings page
> in the browser itself without a performance hit in Fx35, fabulous -
> otherwise let's not squeeze it in for the first release and add that in the
> following one. 
>  
> > 1) The user might have changed their default search engine before updating
> > to a version of Firefox with the search activity, so we'd need to somehow
> > pre-populate the shared pref used for the search activity. However, if the
> > user launches the search activity before launching the browser we wouldn't
> > know their default pref. We could fall back to the default for their locale
> > in this case, but it seems like it would be confusing if the default changes
> > after they open the browser. I'm not sure if there's any good way to address
> > this.
> 
> I would imagine this is more of an edge case, and if we can get the jump-in
> to the browser search settings solved, this could mitigate the pain for
> these users who have found themselves in this scenario.

Well, when they open the browser search settings, they would see the engine that they prefer is set there. If we have some sort of update migration path that sets the shared pref when the browser updates to a version with the search activity, the search activity default will be "fixed" as soon as fennec is opened for the first time.

So yes, this is a bit of an edge case, and in practice all that would happen is that the user would have the "wrong" search engine set in the search activity until they open Fennec.

> > 2) The user may set a third-party search engine as their default in Fennec,
> > and right now we only support built-in search plugins in the search
> > activity. Since we're not using the gecko search service, we'd have to add
> > something to the search service to deal with this, and I'm not sure how
> > complicated this would be. Once again, in this case we could fall back to
> > the default engine for the user's locale for the search activity, but then
> > the user would be worse off than they currently are, since they wouldn't be
> > able to choose a different built-in engine.
> 
> Could we know that this is the case? ie that the default in the browser
> settings isn't supported? if that's the case, could we have a visual
> indicator to prompt the user to understand why it may be different (and push
> it out to a sumo article)? Again, I would see this as an edge case, based on
> current search numbers. I wouldn't see this as a blocker for fx35, and hope
> we can find a way to notify the user.

Yes, we'll know this is a problem if we can't find a search plugin for the default engine identifier stored in the pref. However, I feel like our time would be better spent trying to make these added search engines work in the search activity, rather than create messaging around why they're not supported. Since this is an edge case, we could file a bug about this, but it doesn't need to be a blocker for shipping in 35.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 1) Remove search engine pref from search activity settings screen

Oops, uploaded the wrong patch before.

If we decided we're okay with the UX of removing the search engine pref from the search activity settings screen, then this patch should be pretty straightforward to review.

In the medium/long term, the goal would be to replace this with an item to take you to Fennec's search settings (what my part 3 patch attempts to do).

Comment 12

[:Margaret Leibovic]

Attachment: (Part 2) Update the search activity default engine when the gecko default search engine changes

I updated this patch to allow supporting third-party search engines (where the identifier is null), and it actually works!

This will still fail if you added a search engine with a name that's all special characters, then set it as your default, and in that case the search activity will just fall back to its default (currently Yahoo, but eventually whatever the shipped default is for the locale). And this is such an edge case that I don't care. In fact, when I was testing, trying to do this crashed the search preferences activity! So in reality, it's not even possible :)

The one remaining bit here is that we'll want to perform some update migration to set this shared preference in case the user changed their default previously. I'm not sure of the best place to do that, since we don't really have any JS update migration path. We have BrowserApp.onAppUpdated, but that runs every time the browser updates. Maybe I'll add some small version-based migration logic there to avoid doing this on every update. Does this sound like a good approach, or do you have a better idea? I'll write a separate small patch for this.

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8495562 [details] [diff] [review]
(Part 1) Remove search engine pref from search activity settings screen

Review of attachment 8495562 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +85,5 @@
>      private void getEngineFromPrefs(final SearchEngineCallback callback) {
>          final AsyncTask<Void, Void, SearchEngine> task = new AsyncTask<Void, Void, SearchEngine>() {
>              @Override
>              protected SearchEngine doInBackground(Void... params) {
> +                String identifier = GeckoSharedPrefs.forApp(context).getString(Constants.PREF_SEARCH_ENGINE_KEY, null);

Might as well make this final.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8495987 [details] [diff] [review]
(Part 2) Update the search activity default engine when the gecko default search engine changes

Review of attachment 8495987 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Margaret Leibovic from comment #12)
> Maybe I'll add some small version-based migration logic there to avoid doing this
> on every update. Does this sound like a good approach, or do you have a
> better idea?

That seems like a reasonable place to me since we also have the click to play pref migration there.

::: mobile/android/chrome/content/browser.js
@@ +6916,5 @@
> +  // Updates the search activity pref when the default engine changes.
> +  _setSearchActivityDefaultPref: function _setSearchActivityDefaultPref(engine) {
> +    // Helper function copied from nsSearchService.js. This is the logic that is used
> +    // to create file names for search plugin XML serialized to disk.
> +    function sanitizeName(aName) {

It's unfortunate that we have to copy this over...any changes to nsSearchService's filename algorithm could end up breaking this. To be safe, could you add a test that verifies the file exists at the expected path for custom engines?

@@ +6919,5 @@
> +    // to create file names for search plugin XML serialized to disk.
> +    function sanitizeName(aName) {
> +      const maxLength = 60;
> +      const minLength = 1;
> +      var name = aName.toLowerCase();

var -> let

@@ +6923,5 @@
> +      var name = aName.toLowerCase();
> +      name = name.replace(/\s+/g, "-");
> +      name = name.replace(/[^-a-z0-9]/g, "");
> +
> +      // Use a random name if our input had no valid characters.

Nit: Drop (or change) this comment since you aren't actually doing this.

Comment 15

[:Margaret Leibovic]

Attachment: (Part 2.5) Add test to make sure search plugin XML file ends up where we expect it to

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1990991c551f

This passed locally, but failed when I put in a different (nonexistent) filename.

Comment 16

[:Margaret Leibovic]

Attachment: (Part 2.6) Migrate search activity default pref on app update

I modeled this after what desktop does in nsBrowserGlue.js. This click to play migration is so old that we could probably just get rid of it, but I'm leaving it in to be on the safe side.

I can also update my patch in bug 1071747 to use this logic.

Comment 17

[:Margaret Leibovic]

Comment on attachment 8494855 [details] [diff] [review]
(Part 3) Open Fennec search settings from search activity settings

Punting on this for now. We can address this in bug 1074466.

Comment 18

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/fe24327dd38c
https://hg.mozilla.org/integration/fx-team/rev/6349929e80af
https://hg.mozilla.org/integration/fx-team/rev/6a12c3ea84e0
https://hg.mozilla.org/integration/fx-team/rev/135564e9e7f5

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe24327dd38c
https://hg.mozilla.org/mozilla-central/rev/6349929e80af
https://hg.mozilla.org/mozilla-central/rev/6a12c3ea84e0
https://hg.mozilla.org/mozilla-central/rev/135564e9e7f5

Comment 20

[:Margaret Leibovic]

FYI, I backed out the last patch for causing bug 1074933, but I filed bug 1076351 to find a different way to address this migration issue.

https://hg.mozilla.org/integration/fx-team/rev/e45de3a150aa