Closed Bug 820473 Opened 10 years ago Closed 2 years ago

Don't request search suggestions for urls


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: wesj, Unassigned)




(1 file, 1 obsolete file)

Attached file Patch (obsolete) —
In order to maintain some user data privacy, we shouldn't be requesting search suggestions for things that look like urls.
Attached patch PatchSplinter Review
Attachment #690958 - Attachment is obsolete: true
Attachment #690959 - Flags: review?(bnicholson)
Comment on attachment 690959 [details] [diff] [review]

WARNING: I read this patch in reverse, so more context is included in the last comments

>diff --git a/mobile/android/base/ b/mobile/android/base/

>+    private boolean mIsSearchUrl = false;

>+            mIsSearchUrl = false;
>+        } else {
>+            mIsSearchUrl = true;
>         }

This makes me cry. Do we really need to keep this state? And remember, I really dislike "mIsSearchURL". Prefer "mUseSuggestions"

>-        mGoButton.setImageResource(imageResource);
>-        mGoButton.setContentDescription(contentDescription);
>         int actionBits = mText.getImeOptions() & EditorInfo.IME_MASK_ACTION;
>         if (actionBits != imeAction) {
>             InputMethodManager imm = (InputMethodManager) mText.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
>             int optionBits = mText.getImeOptions() & ~EditorInfo.IME_MASK_ACTION;
>             mText.setImeOptions(optionBits | imeAction);
>             imm.restartInput(mText);
>+            mGoButton.setImageResource(imageResource);
>+            mGoButton.setContentDescription(contentDescription);
>         }

Why move this code?

>diff --git a/mobile/android/base/ b/mobile/android/base/

>+        filter("", false);

>+    public void filter(String searchTerm, boolean isSearchUrl) {

I don't like "isSearclURL". I like "useSuggestions". It's better context.

>diff --git a/mobile/android/base/awesomebar/ b/mobile/android/base/awesomebar/

>+    public void filter(String searchTerm, boolean isSearchUrl) {

>+        // If we're searching for a url, don't notify the suggestions provider of it
>+        if (isSearchUrl) {
>+            filterSuggestions("");
>+            return;

I don't like the context of "is searching for url" that you are using here. Really, we are telling the code to "useSuggestions" or not. That seems like a better context. Then when we decide to not use suggestion when the text looks like a person's name (or some other crazy crap), the variables still make sense.

>     private void filterSuggestions(String searchTerm) {

>+        if (TextUtils.isEmpty(searchTerm)) {
>+            setSuggestions(new ArrayList<String>());
>+            return;
>+        }

This is just an optimization?

>         if (mSuggestClient != null && mSuggestionsEnabled) {
>             mSuggestTask = new AsyncTask<String, Void, ArrayList<String>>() {
>                 protected ArrayList<String> doInBackground(String... query) {
>                     return mSuggestClient.query(query[0]);
>                 }
>                 protected void onPostExecute(ArrayList<String> suggestions) {
>                     setSuggestions(suggestions);
Attachment #690959 - Flags: review-
Duplicate of this bug: 774517
Comment on attachment 690959 [details] [diff] [review]

Clearing review for now since we may not want this after bug 820576.
Attachment #690959 - Flags: review?(bnicholson)
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly]( an issue can be reported at the [Fenix GitHub project]( If you want to discuss your report please use [Mozilla's chat]( server and join the [#fenix]( channel.
Closed: 2 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.