Closed Bug 738961 Opened 8 years ago Closed 8 years ago

Awesomebar filter should treat space-separated words as separate filters

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(2 files, 1 obsolete file)

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.)
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Let's just be careful with performance
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.
Assignee: nobody → margaret.leibovic
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #610777 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #610777 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #610939 - Flags: review?(lucasr.at.mozilla)
Attached patch upated patchSplinter Review
(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.
Attachment #610777 - Attachment is obsolete: true
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 :-)
Attachment #610939 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/d30a86bb5408
https://hg.mozilla.org/mozilla-central/rev/823ab3b9d814
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Comment on attachment 610939 [details] [diff] [review]
update KNOWN_PREFIX in talos test

[Approval Request Comment]
Mobile only. Release blocker.
Attachment #610939 - Flags: approval-mozilla-aurora?
Comment on attachment 610941 [details] [diff] [review]
upated patch

[Approval Request Comment]
Mobile only. Release blocker.
Attachment #610941 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 739551
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.
Attachment #610939 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #610941 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.