Closed Bug 1236387 Opened 4 years ago Closed 4 years ago

Limit length of returned search history suggestions

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified
fennec + ---

People

(Reporter: mcomella, Assigned: varunnaganathan912, Mentored)

References

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(5 files)

Attached image Screenshot of the issue
I accidentally searched a url and it's ugly.
The screenshot is of a search history item, so updating the summary.
Summary: Limit length of returned search suggestions → Limit length of returned search history suggestions
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.
Assignee: nobody → ahunt
(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.
This looks ugly, and would be good to fix, but it's not super high priority, since it's an edge case.
tracking-fennec: ? → +
Assignee: ahunt → nobody
Mentor: ahunt
Whiteboard: [lang=java][good next bug]
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.
(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.
Attached patch Trial patchSplinter Review
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
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.
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?
Andrew, comment 9?
Flags: needinfo?(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
Flags: needinfo?(ahunt)
Attachment #8709184 - Attachment is patch: true
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?
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.
(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
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.
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.
Could also someone assign the bug to me.Thanks!
Attachment #8711295 - Flags: review?(ahunt)
Assignee: nobody → varunnaganathan912
Status: NEW → ASSIGNED
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.
Attachment #8711295 - Flags: review?(ahunt) → feedback+
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)
Attachment #8713511 - Flags: review?(ahunt)
Hi,
any updates on the patch?
Flags: needinfo?(ahunt)
Flags: needinfo?(rnewman)
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
Flags: needinfo?(rnewman)
Flags: needinfo?(ahunt)
Attachment #8713511 - Flags: review?(ahunt) → review+
https://hg.mozilla.org/integration/fx-team/rev/e86f91dd77be96bc473c02acf581a4a19d3073ef
Bug 1236387 - Add a URL and length filter for search history suggestions r=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!
https://hg.mozilla.org/mozilla-central/rev/e86f91dd77be
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1251740
Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Beta - 47.0b6
Since this is verified I'll mark it as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.