738961 - Awesomebar filter should treat space-separated words as separate filters

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I knew two non-consecutive words in the title of a webpage, but the aweseomebar couldn't find it because the filter will only search for the entire string.

We should handle this case as awesome-ly as desktop does.

(On a more general note, maybe we should take a peek at the desktop aweseomebar code and see what else we should make sure we don't miss.)

Comment 1

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

Let's just be careful with performance

Comment 2

[:Margaret Leibovic]

Good thing we have a talos test! Also I started working on correctness tests for bug 736171, which would be good to land with this.

Comment 3

[:Margaret Leibovic]

Attachment: patch

I'll can a few runs of testBrowserProviderPerf with and without this to see if it affects performance, although I don't know if that's a good test, since there isn't a space in the constraint word in that test. I don't think this should really hurt us, though, especially since most users probably won't enter a bunch of space-separated words.

I'll also add a testcase for this to the tests I'm writing for bug 736171. I just tested locally using the default bookmarks, and it filtered them as expected.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 610777 [details] [diff] [review]
patch

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

Looks good but please alter the talos test locally to have multiple words and post the results here (just split the search word in two?). I think we won't have any performance issues from this change but it still good to know the numbers we get from the test.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +148,4 @@
>  
> +        // The combined history/bookmarks selection queries for sites with a url or title containing
> +        // the constraint string(s), treating space-separated words as separate constraints
> +        String[] constraintWords = constraint.toString().split(" ");

I assume split() will return empty list of words if the string only has spaces?

@@ +151,5 @@
> +        String[] constraintWords = constraint.toString().split(" ");
> +        for (int i = 0; i < constraintWords.length; i++) {
> +            selection = DBUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " +
> +                                                                  Combined.TITLE + " LIKE ?)");
> +            String word =  "%" + constraintWords[i] + "%";

"constraint" (instead of "word") would probably be more appropriate name.

Comment 5

[:Margaret Leibovic]

Attachment: update KNOWN_PREFIX in talos test

I ran this with and without my patch applied, and the results were statistically the same, so I think we're good to go. Here are the results:

Without patch: 369, 378, 374, 386, 354
With patch: 371, 374, 377, 374, 370

Comment 6

[:Margaret Leibovic]

Attachment: upated patch

(In reply to Lucas Rocha (:lucasr) from comment #4)

> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +148,4 @@
> >  
> > +        // The combined history/bookmarks selection queries for sites with a url or title containing
> > +        // the constraint string(s), treating space-separated words as separate constraints
> > +        String[] constraintWords = constraint.toString().split(" ");
> 
> I assume split() will return empty list of words if the string only has
> spaces?

Yep, I tested to confirm this, and it's true.

> @@ +151,5 @@
> > +        String[] constraintWords = constraint.toString().split(" ");
> > +        for (int i = 0; i < constraintWords.length; i++) {
> > +            selection = DBUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " +
> > +                                                                  Combined.TITLE + " LIKE ?)");
> > +            String word =  "%" + constraintWords[i] + "%";
> 
> "constraint" (instead of "word") would probably be more appropriate name.

I decided to use "constraintWord", since constraint is already used as one of the parameter names.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 610939 [details] [diff] [review]
update KNOWN_PREFIX in talos test

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

::: mobile/android/base/tests/testBrowserProviderPerf.java.in
@@ +27,5 @@
>      private final int BATCH_SIZE = 500;
>  
> +    // Include spaces in prefix to test performance querying with
> +    // multiple constraint words
> +    private final String KNOWN_PREFIX = "my mozilla test";

I'm tempted to add a space at the end :-)

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d30a86bb5408
https://hg.mozilla.org/integration/mozilla-inbound/rev/823ab3b9d814

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/d30a86bb5408
https://hg.mozilla.org/mozilla-central/rev/823ab3b9d814

Comment 10

[:Margaret Leibovic]

Comment on attachment 610939 [details] [diff] [review]
update KNOWN_PREFIX in talos test

[Approval Request Comment]
Mobile only. Release blocker.

Comment 11

[:Margaret Leibovic]

Comment on attachment 610941 [details] [diff] [review]
upated patch

[Approval Request Comment]
Mobile only. Release blocker.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 610939 [details] [diff] [review]
update KNOWN_PREFIX in talos test

[Triage Comment]
Mobile only & blocking Fennec 1.0. Approved for Aurora 13.