820473 - Don't request search suggestions for urls

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch

In order to maintain some user data privacy, we shouldn't be requesting search suggestions for things that look like urls.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 690959 [details] [diff] [review]
Patch

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

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java

>+    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/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>+        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/AllPagesTab.java b/mobile/android/base/awesomebar/AllPagesTab.java

>+    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);

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 690959 [details] [diff] [review]
Patch

Clearing review for now since we may not want this after bug 820576.

Comment 5

[BMO Automation]

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](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.