Closed Bug 966810 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified

People

(Reporter: liuche, Assigned: sebastian)

Details

(Whiteboard: [mentor=liuche])

Attachments

(2 files, 1 obsolete file)

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.
Assigning to sebastian per IRC conversation; he's interested in working on this.
Assignee: nobody → s.kaspari
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.
Status: NEW → ASSIGNED
Attached patch 966810.patch (obsolete) — Splinter Review
Attachment #8369535 - Flags: review?(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!
Attachment #8369535 - Flags: review?(liuche) → review+
Comment on attachment 8369535 [details] [diff] [review]
966810.patch

>From: Sebastian Kaspari <s.kaspari@gmail.com>
>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();
>     }
Attachment #8369535 - Attachment is obsolete: true
Attachment #8369620 - Flags: review?(liuche)
Attachment #8369620 - Flags: review?(liuche) → review+
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
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/31e118554919
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.