Closed
Bug 738961
Opened 11 years ago
Closed 11 years ago
Awesomebar filter should treat space-separated words as separate filters
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(blocking-fennec1.0 +)
RESOLVED
FIXED
Firefox 14
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | + |
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(2 files, 1 obsolete file)
1.28 KB,
patch
|
lucasr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.51 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.)
Updated•11 years ago
|
blocking-fennec1.0: --- → ?
Updated•11 years ago
|
blocking-fennec1.0: ? → +
Comment 1•11 years ago
|
||
Let's just be careful with performance
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
(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 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d30a86bb5408 https://hg.mozilla.org/integration/mozilla-inbound/rev/823ab3b9d814
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d30a86bb5408 https://hg.mozilla.org/mozilla-central/rev/823ab3b9d814
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Assignee | ||
Comment 10•11 years ago
|
||
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?
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 610941 [details] [diff] [review] upated patch [Approval Request Comment] Mobile only. Release blocker.
Attachment #610941 -
Flags: approval-mozilla-aurora?
Comment 13•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #610941 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•