1236387 - Limit length of returned search history suggestions

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot of the issue

I accidentally searched a url and it's ugly.

Comment 1

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

The screenshot is of a search history item, so updating the summary.

Comment 2

[:Margaret Leibovic]

That looks awful, we should definitely fix that.

We could avoid showing search history entries that are this long, but maybe we should also avoid storing search history entries if they're over a certain length. I don't know when we would ever want to use or display a search query this long.

Comment 3

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

(In reply to :Margaret Leibovic from comment #2)
> That looks awful, we should definitely fix that.
> 
> We could avoid showing search history entries that are this long, but maybe
> we should also avoid storing search history entries if they're over a
> certain length. I don't know when we would ever want to use or display a
> search query this long.

Yeah. We should just ignore URLs when saving search terms. Somehow Mike actually searched for a long ass URL. I don't know how he did it, but he did. We should just ignore it.

Comment 4

[:Margaret Leibovic]

This looks ugly, and would be good to fix, but it's not super high priority, since it's an edge case.

Comment 5

[varunnaganathan912]

Hi,I want to work on this bug as my first bug.Could anyone guide me on the build process as well as the location of the code.Till now i have just cloned the mozilla central repo.Thanks.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to varunnaganathan912 from comment #5)
> Hi,I want to work on this bug as my first bug.Could anyone guide me on the
> build process as well as the location of the code.Till now i have just
> cloned the mozilla central repo.Thanks.

Hello – welcome to Bugzilla! You can find our build instructions at:
  https://wiki.mozilla.org/Mobile/Fennec/Android

The search screen code is located at:
  mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java
  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java

We'll assign you to the bug once a patch gets posted.

Please let Andrew (ahunt) know if you have any questions.

Comment 7

[varunnaganathan912]

Attachment: Trial patch

Hey,Here's the trial patch.I have used the regex module to control urls in suggestions as well as limited length of sugegstions to 50 chars.Please review and suggest required changes

Comment 8

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

Just a drive-by. Mike can help move this forward.

Looks like this patch tries to filter out URL or long search history suggestions when we attempt display the suggestions.

I think we should stop adding URLs when we save search history.

Comment 9

[varunnaganathan912]

I think savedSuggestions(the array on which im applying the filter) is the list of the suggestions that are saved.mSearchHistorySuggestions and mList are responsible for the displaying of the suggestions.Do correct me if I have misunderstood?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Andrew, comment 9?

Comment 11

[Andrzej Hunt :ahunt]

(In reply to varunnaganathan912 from comment #9)
> I think savedSuggestions(the array on which im applying the filter) is the
> list of the suggestions that are saved.mSearchHistorySuggestions and mList
> are responsible for the displaying of the suggestions.Do correct me if I
> have misunderstood?

savedSuggestions is used when we're retrieving previous searches (i.e searches that have already been saved), and preparing to display them - what :mfinkle is suggesting is that we should instead filter out the suggestions before they reach the database. I think it's probably worth looking at the code we run when the user selects a search suggestion, and follow that to wherever we actually save the search history:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java#96


As an aside: I'd recommend having a look at the mozilla mercurial guide [1] for a simpler way to prepare patches, that let's us apply them straight to the source tree (it looks like you might be using diff directly?).

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8709184 [details] [diff] [review]
Trial patch

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

::: BrowserSearch.java
@@ -1011,4 @@
>                      final int searchColumn = result.getColumnIndexOrThrow(BrowserContract.SearchHistory.QUERY);
>                      do {
>                          final String savedSearch = result.getString(searchColumn);
> -                        if (savedSearch.length() <= 50 && !Pattern.matches("^(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]",savedSearch)) {

Could this regex be made simpler? I think all we should do is check whether the string begins with http://, ftp://, or file:// (I'm not sure the latter two would even appear as search suggestions) - we can then safely ignore the rest of the string?

Comment 13

[varunnaganathan912]

Attachment: filter urls and large suggestions

I added the URL filter to the array saving the search history rather than the one displaying the search results.Simplified the regex as asked.Please review.

Comment 14

[Andrzej Hunt :ahunt]

(In reply to varunnaganathan912 from comment #13)
> Created attachment 8710349 [details] [diff] [review]
> filter urls and large suggestions
> 
> I added the URL filter to the array saving the search history rather than
> the one displaying the search results.Simplified the regex as asked.Please
> review.

Almost there - but I think that's still filtering the searches after being retrieved the database. We want to stop the searches from being saved before they reach the database, which I think we could do here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2360

Comment 15

[Richard Newman [:rnewman]]

We want to do both things: filter out any long results that come from the DB (whether saved by earlier versions of Firefox, or synced from another device), and also avoid saving ones we don't want to save.

Comment 16

[varunnaganathan912]

Attachment: Added filter for url's and long searches

As suggested by Richard Newman,I have added the filter both before saving any unwanted search queries(URL's and long ones) to the database as well as kept the filter for any such queries coming from the database and we don't want to display.(If stored by previous versions of firefox).Please review.

Comment 17

[varunnaganathan912]

Could also someone assign the bug to me.Thanks!

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8711295 [details] [diff] [review]
Added filter for url's and long searches

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

::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineRow.java
@@ +381,5 @@
>  
>          // Remove duplicates of search engine suggestions from saved searches.
>          List<String> searchHistorySuggestions = (rawSearchHistorySuggestions != null) ? rawSearchHistorySuggestions : new ArrayList<String>();
> +
> +        // Remove any long suggestions or any URL suggestions

This patch seems to be modifying a previous addition of the loop (i.e. this patch by itself won't apply) - I think you might need to fold all your commits into one?

('hg histedit' lets you fold commits - feel free to ask on the #mobile IRC channel if you need help with this!)

@@ +386,1 @@
>          for (int i=0;i<searchHistorySuggestions.size();i++)

I think this could be dangerous: we're modifying the list during the loop, using ArrayList.iterator ( https://docs.oracle.com/javase/7/docs/api/java/util/ArrayList.html#iterator%28%29 ) is probably a better way to remove items.

Comment 19

[varunnaganathan912]

Attachment: Added filter for url's and long searches

modified the iterator.
Just to clarify again, as suggested i have kept the filter both before storing suggestions to the database as well as before displaying results retrieved from the database(to take care of any previous versions of firefox)

Comment 20

[varunnaganathan912]

Hi,
any updates on the patch?

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8713511 [details] [diff] [review]
Added filter for url's and long searches

Sorry this has taken so long, this looks like the right approach to me!

Just one minor nit for the future (I'll fix that for this patch): we keep our opening braces on the same line - you might want to have a quick look at a summary of our coding style (although we're not entirely consistent with that...):
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Java_practices

Comment 22

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/e86f91dd77be96bc473c02acf581a4a19d3073ef
Bug 1236387 - Add a URL and length filter for search history suggestions r=ahunt

Comment 23

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #22)
> https://hg.mozilla.org/integration/fx-team/rev/
> e86f91dd77be96bc473c02acf581a4a19d3073ef
> Bug 1236387 - Add a URL and length filter for search history suggestions
> r=ahunt

I pushed the patch since I already had it locally! Feel free to ping if there are any other bugs you're interested in, but I've noticed you're already working on Bug 1188271 which is awesome!

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e86f91dd77be

Comment 25

[Alex Caziuc]

Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Beta - 47.0b6

Comment 26

[Alex Caziuc]

Since this is verified I'll mark it as verified fixed.