966810 - Don't prompt to enable search suggestions if search.suggestions is true

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Search suggestions prompt

If search suggestions (search.suggest.enabled) is set from about:config, we still display the "Turn on search suggestions" prompt. (We do set search.suggest.prompted to true when the user sets the search suggestions pref from the Settings UI.)

We should either check for search.suggest.enabled pref when displaying the search suggestion prompt, or set the search.suggest.prompted pref when search.suggest.enabled is set from about:config.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Assigning to sebastian per IRC conversation; he's interested in working on this.

Comment 2

[Chenxia Liu [:liuche]]

Hi Sebastian,

Thanks for picking up this bug, let me know if you have any questions!

In case you're just getting started, take a look at https://wiki.mozilla.org/Mobile/Fennec/Android for the basics for setting up a build.

Another thing that is useful for getting to know the code base is http://mxr.mozilla.org/mozilla-central/ . This is an indexed source of the code base, so it's very useful for searching quickly. You might want to start there to find where the relevant prefs are set and used.

Again, feel free to ping me on #mobile, or NEEDINFO me here in this bug.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 966810.patch

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8369535 [details] [diff] [review]
966810.patch

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

Looks great - thanks for updating the comment too!

Update the patch message to include me as the reviewer, and then add "checkin-needed" to the keywords bugzilla field. (So the message reads: Bug 966810 - Don't prompt to enable search suggestions if search.suggestions is true. r=liuche )

In the future, you can include the reviewer in the original patch message. "checkin-needed" is set for bugs that have been reviewed with an r+, indicating that it's ready for someone to land in the tree.

Thanks for the patch!

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8369535 [details] [diff] [review]
966810.patch

>From: Sebastian Kaspari <[e-mail]>
>Bug 966810 - Don't prompt to enable search suggestions if search suggestions are already enabled. r=liuche
>
>
>diff --git a/mobile/android/base/home/BrowserSearch.java b/mobile/android/base/home/BrowserSearch.java
>index 95ba83c..9c73503 100644
>--- a/mobile/android/base/home/BrowserSearch.java
>+++ b/mobile/android/base/home/BrowserSearch.java
>@@ -528,19 +528,19 @@ public class BrowserSearch extends HomeFragment
>             }
> 
>             mSearchEngines = searchEngines;
> 
>             if (mAdapter != null) {
>                 mAdapter.notifyDataSetChanged();
>             }
> 
>-            // Show suggestions opt-in prompt only if user hasn't been prompted
>-            // and we're not on a private browsing tab.
>-            if (!suggestionsPrompted && mSuggestClient != null) {
>+            // Show suggestions opt-in prompt only if suggestions are not enabled yet,
>+            // user hasn't been prompted and we're not on a private browsing tab.
>+            if (!mSuggestionsEnabled && !suggestionsPrompted && mSuggestClient != null) {
>                 showSuggestionsOptIn();
>             }
>         } catch (JSONException e) {
>             Log.e(LOGTAG, "Error getting search engine JSON", e);
>         }
> 
>         filterSuggestions();
>     }

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 966810-updated.patch

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

You will need to use the checkin-needed keyword to have someone check in your code. https://developer.mozilla.org/en-US/docs/Introduction#Step_6_-_Actually_get_the_code_into_the_tree

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I think a lot of first-time contributors don't have access to edit the keywords field. Setting the flag in case that is a problem here.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks for the help. I was a bit confused if the patch with the updated patch message would need another review before I can add the keyword.

Comment 10

[Matthew N. [:MattN]]

Your patch has been pushed to the integration repo:
https://hg.mozilla.org/integration/fx-team/rev/31e118554919

Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31e118554919

Comment 12

[Ioana Chiorean]

I do not see the prompt while the setting is set to true. 

Verified fixed on:
Nightly 33.0a1
Aurora 32.0a2
Beta 31.0b1