Last Comment Bug 721731 - Bookmarks don't show up in awesomebar results
: Bookmarks don't show up in awesomebar results
Status: VERIFIED FIXED
[sync]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P2 normal (vote)
: Firefox 14
Assigned To: :Margaret Leibovic
:
Mentors:
: 727491 729969 735169 (view as bug list)
Depends on: 730105
Blocks: 736272 736593
  Show dependency treegraph
 
Reported: 2012-01-27 05:44 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2012-05-22 08:17 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
fixed
fixed
verified
+
11+


Attachments
WIP (12.72 KB, patch)
2012-03-15 17:33 PDT, :Margaret Leibovic
lucasr.at.mozilla: feedback-
Details | Diff | Splinter Review
WIP v2 (12.86 KB, patch)
2012-03-16 17:59 PDT, :Margaret Leibovic
lucasr.at.mozilla: feedback+
Details | Diff | Splinter Review
patch (22.35 KB, patch)
2012-03-19 16:06 PDT, :Margaret Leibovic
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
test (6.03 KB, patch)
2012-03-19 16:08 PDT, :Margaret Leibovic
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
patch v2 (22.52 KB, patch)
2012-03-21 14:47 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
test v2 (13.43 KB, patch)
2012-03-21 14:54 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v3 (22.64 KB, patch)
2012-03-21 16:17 PDT, :Margaret Leibovic
lucasr.at.mozilla: review-
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
test v3 (22.82 KB, patch)
2012-03-21 16:19 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
test v3 (13.46 KB, patch)
2012-03-21 16:22 PDT, :Margaret Leibovic
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
talos test changes (9.91 KB, patch)
2012-03-22 11:18 PDT, :Margaret Leibovic
gbrown: review-
Details | Diff | Splinter Review
talos test changes v2 (10.13 KB, patch)
2012-03-23 14:40 PDT, :Margaret Leibovic
gbrown: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2012-01-27 05:44:54 PST
Tested on the LG Optimus Black, Android 2.2.2.

I used Firefox Sync to Sync my bookmarks and history to my mobile phone for the native Fennec trunk build.
One of the bookmarks that show up in my Native Fennec trunk build is 'http://slashdot.org'.
So when I type in the url bar 'slashdot', I would expect that bookmark entry to appear in my awesome bar results.
But it doesn't show up, I only see the "Search for" entries.
Comment 1 Lucas Rocha (:lucasr) 2012-01-27 06:06:54 PST
We need to change the DB query to include unvisited bookmarks in the awesomebar search.
Comment 2 Kevin Brosnan [:kbrosnan] 2012-02-23 13:56:09 PST
*** Bug 727491 has been marked as a duplicate of this bug. ***
Comment 3 Kevin Brosnan [:kbrosnan] 2012-02-28 08:50:50 PST
*** Bug 731217 has been marked as a duplicate of this bug. ***
Comment 4 Aaron Train [:aaronmt] 2012-02-28 08:52:58 PST
*** Bug 729969 has been marked as a duplicate of this bug. ***
Comment 5 :Margaret Leibovic 2012-03-09 15:55:27 PST
(In reply to Lucas Rocha (:lucasr) from comment #1)
> We need to change the DB query to include unvisited bookmarks in the
> awesomebar search.

Is there a way to have one query return results from both the history table and the bookmarks table using the content provider API? Or do I need to change BrowserProvider/BrowserContract to give me an API to some joint "places" content URI that will return all history/bookmark records? I guess we'll also need to do something to avoid duplicate entries, since a visited bookmark will also be in history.
Comment 6 Richard Newman [:rnewman] 2012-03-09 16:07:09 PST
(In reply to Margaret Leibovic [:margaret] from comment #5)

> Is there a way to have one query return results from both the history table
> and the bookmarks table using the content provider API?

If they're two tables, you would need to add a content URI that represents a join, and builds a query accordingly. You can't specify tables directly in a query.

You could quite easily do this by defining a view, and then just querying it, rather than pissing about with the SQLiteQueryUnhelper.
Comment 7 Richard Newman [:rnewman] 2012-03-09 18:41:59 PST
> You could quite easily do this by defining a view, and then just querying
> it, rather than pissing about with the SQLiteQueryUnhelper.

margaret: some pointers to how to do this, in case you want a short-cut:

https://bugzilla.mozilla.org/show_bug.cgi?id=734425#c3
Comment 8 Lucas Rocha (:lucasr) 2012-03-12 09:31:23 PDT
Just to make it clear: the problem we have now is that awesomebar results don't include bookmarks that were not visited (i.e. no row in the history table). What we probably need is a view that includes all history entries order by frecency + unvisited bookmarks at/close to the "bottom" (as they have the lower or zero frecency). The awesomebar queries would then be done on this view instead of the history table.
Comment 9 Richard Newman [:rnewman] 2012-03-12 12:07:15 PDT
(In reply to Lucas Rocha (:lucasr) from comment #8)
> The awesomebar queries would then be done
> on this view instead of the history table.

Be careful of perf regressions. Views in SQLite are apparently not too good with index usage, so you might need to employ some magic.

Also consider that pruning of the history table might cause a large bias -- for users with 3,000 bookmarks, their bookmarks will be the majority of the matches.
Comment 10 Adrian Tamas (:AdrianT) 2012-03-13 02:01:02 PDT
*** Bug 735169 has been marked as a duplicate of this bug. ***
Comment 11 :Margaret Leibovic 2012-03-15 15:59:48 PDT
(In reply to Lucas Rocha (:lucasr) from comment #8)
> Just to make it clear: the problem we have now is that awesomebar results
> don't include bookmarks that were not visited (i.e. no row in the history
> table). What we probably need is a view that includes all history entries
> order by frecency + unvisited bookmarks at/close to the "bottom" (as they
> have the lower or zero frecency). The awesomebar queries would then be done
> on this view instead of the history table.

We also want to know if a history entry is a bookmark or not, in order to give it more aweseomeness weight if it is (bug 736272). This makes me think we'd need a full join on URL, which sounds like it would be slow.

(In reply to Richard Newman [:rnewman] from comment #9)
> (In reply to Lucas Rocha (:lucasr) from comment #8)
> > The awesomebar queries would then be done
> > on this view instead of the history table.
> 
> Be careful of perf regressions. Views in SQLite are apparently not too good
> with index usage, so you might need to employ some magic.

Given my comment above, this worries me.

> Also consider that pruning of the history table might cause a large bias --
> for users with 3,000 bookmarks, their bookmarks will be the majority of the
> matches.

If a bookmark is unvisited, it will still have frecency 0, since we're multiplying visit count (0 in that case) by a multiplier. Because of this, we'd only be showing unvisited bookmarks at the top of the suggestions list if there are no more history entries that match.
Comment 12 :Margaret Leibovic 2012-03-15 16:17:05 PDT
Also, we're already performing a join in order to get favicons with the history entries, so will we need to make another join on top of that?
Comment 13 Richard Newman [:rnewman] 2012-03-15 16:46:45 PDT
(In reply to Margaret Leibovic [:margaret] from comment #11)

> We also want to know if a history entry is a bookmark or not, in order to
> give it more aweseomeness weight if it is (bug 736272). This makes me think
> we'd need a full join on URL, which sounds like it would be slow.

Probably depends on schema and indices… might be surprisingly fast. This kind of thing is hard to predict!

Or you could do some work in-memory. (Persist a bloom filter of visited URIs?)

Or, when you bookmark or visit a page, do appropriate bookkeeping to make these reads fast? E.g., flag history rows with how many times they've been bookmarked, and query on that column?

(Wild speculation.)
Comment 14 :Margaret Leibovic 2012-03-15 17:33:17 PDT
Created attachment 606412 [details] [diff] [review]
WIP

I'm running into some confusion here, but this is what I've come up with so far. For the sake of a first attempt, I was trying to make a join that just includes images, since I think we'll want images in all the places we'll use this joint query (thinking about it now actually, we probably don't want to be using this query for the top sites on about:home, since those should all be sites you've visited).

Also, we can't do a full outer join in sqlite, so I was starting with a left outer join, then I was going to try to do something like this if I got that to work:
http://stackoverflow.com/questions/1923259/full-outer-join-with-sqlite

This current patch crashes with:
E SQLiteOpenHelper: android.database.sqlite.SQLiteException: ambiguous column name: images.url_key: CREATE VIEW IF NOT EXISTS history_with_bookmarks AS SELECT history.*, folder FROM history LEFT OUTER JOIN images ON history.url = images.url_key LEFT OUTER JOIN bookmarks ON history LEFT OUTER JOIN images ON history.url = images.url_key.url = bookmarks.url

I feel like I'm making a mess here, so help would be appreciated :)
Comment 15 Richard Newman [:rnewman] 2012-03-15 18:07:25 PDT
Comment on attachment 606412 [details] [diff] [review]
WIP

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

::: mobile/android/base/db/BrowserContract.java.in
@@ +133,5 @@
>      }
>  
> +    public static final class Places  {
> +        private Places() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "places");

Someone might punch all of us if we namecrash :D

::: mobile/android/base/db/BrowserProvider.java.in
@@ +110,5 @@
>              qualifyColumn(TABLE_IMAGES, Images.URL);
>  
> +    static final String TABLE_HISTORY_JOIN_BOOKMARKS = TABLE_HISTORY_JOIN_IMAGES + " LEFT OUTER JOIN " +
> +            TABLE_BOOKMARKS + " ON " + qualifyColumn(TABLE_HISTORY_JOIN_IMAGES, History.URL) + " = " +
> +            qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL);

Here is my stab:

https://gist.github.com/2047992
Comment 16 Lucas Rocha (:lucasr) 2012-03-16 04:02:22 PDT
Comment on attachment 606412 [details] [diff] [review]
WIP

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

I'm writing a talos test that will track performance of our awesomebar filter query (as part of bug 730105). I think a patch for this bug should only land once we're confident we're not negatively hitting the performance on awesomebar too much. The talos test will make it easier for you to test performance. I'm giving f- here mostly because performance hit is still unknown and we need an IS_BOOKMARK column (otherwise we'll have to add a migration just for that later)

::: mobile/android/base/db/BrowserContract.java.in
@@ +133,5 @@
>      }
>  
> +    public static final class Places  {
> +        private Places() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "places");

I'd avoid using PLACES just to avoid confusion. Maybe something like COMBINED, FRECENCY_LIST, HISTORY_WITH_BOOKMARKS...?

::: mobile/android/base/db/BrowserProvider.java.in
@@ +110,5 @@
>              qualifyColumn(TABLE_IMAGES, Images.URL);
>  
> +    static final String TABLE_HISTORY_JOIN_BOOKMARKS = TABLE_HISTORY_JOIN_IMAGES + " LEFT OUTER JOIN " +
> +            TABLE_BOOKMARKS + " ON " + qualifyColumn(TABLE_HISTORY_JOIN_IMAGES, History.URL) + " = " +
> +            qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL);

I've quickly hacked something similar some time ago and there was a relevant perf hit...

It would probably be very useful to have something like an IS_BOOKMARK column that could be used to show a star on awesomebar entries that are bookmarks (see bug 701330).

@@ +353,5 @@
> +
> +            db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_HISTORY_WITH_BOOKMARKS + " AS " +
> +                    "SELECT " + qualifyColumn(TABLE_HISTORY, "*") +
> +                    ", " + Bookmarks.IS_FOLDER + " FROM " +
> +                    TABLE_HISTORY_JOIN_BOOKMARKS);

You need to bump the database version and add a migration to ensure this view is created for current users.
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-16 05:30:22 PDT
(In reply to Richard Newman [:rnewman] from comment #15)

> Here is my stab:
> 
> https://gist.github.com/2047992

I tested the updated version of this on an Android device, using a large DB, and the performance was pretty good. Definitely worth putting in code and timing. I would also suggest creating a VIEW using the SQL. Again, performance was the same as the raw SQL in my on device testing. I called mine "frecency_with_images".
Comment 18 Lucas Rocha (:lucasr) 2012-03-16 09:29:15 PDT
FYI: I've just submitted patches to add a talos test for our AwesomeBar frecency query in bug 730105.
Comment 19 Aaron Train [:aaronmt] 2012-03-16 13:34:40 PDT
FWIW: querying by keyword is also affected with the ability to add keywords to bookmarks through edit-mode. Is that taken into account with this bug?
Comment 20 :Margaret Leibovic 2012-03-16 13:57:15 PDT
(In reply to Aaron Train [:aaronmt] from comment #19)
> FWIW: querying by keyword is also affected with the ability to add keywords
> to bookmarks through edit-mode. Is that taken into account with this bug?

Right now what we've discussed in this bug doesn't change the filter to match anything beyond title and url. I think the scope of this bug is large enough, so could you file a follow-up for that?
Comment 21 Aaron Train [:aaronmt] 2012-03-16 14:04:39 PDT
(In reply to Margaret Leibovic [:margaret] from comment #20)
> [...]

bug 736593
Comment 22 Richard Newman [:rnewman] 2012-03-16 14:41:01 PDT
(In reply to Lucas Rocha (:lucasr) from comment #16)

> It would probably be very useful to have something like an IS_BOOKMARK
> column that could be used to show a star on awesomebar entries that are
> bookmarks (see bug 701330).

That doesn't obviate the need for a query like this, but it's easy to add in this query: we're already virtualizing columns. Just return true for the ones that are/have bookmarks.
Comment 23 Richard Newman [:rnewman] 2012-03-16 14:41:40 PDT
Comment on attachment 606412 [details] [diff] [review]
WIP

Clearing flag until next WIP.
Comment 24 :Margaret Leibovic 2012-03-16 17:59:32 PDT
Created attachment 606812 [details] [diff] [review]
WIP v2

So, this is still missing things, but it's getting there (it actually works in my build).

The main problem I ran into was that AboutHomeContent was getting upset that I wasn't providing an _id column with this cursor: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AboutHomeContent.java#364. At first I started by removing the History._ID part of the projections in the calls to filterAllSites, but that didn't work, so a work-around, I decided to just return the id from the history table, but that makes it feel like something could go wrong. I wasn't experiencing any problems using a profile with lots of unvisited bookmarks, so maybe it's okay? At the very least we should know what the consequences of this are.

Still left to address:
-Database migration
-Factor out strings in createCombinedWithImagesView (I assume we're going to want to do this. I preemptively added some strings to Combined for this)
-Figure out if we should be using COMBINED_PROJECTION_MAP (it seems more efficient to just be doing the projection in the view like this patch does)
-Figure out _id issues mentioned above
Comment 25 Lucas Rocha (:lucasr) 2012-03-19 07:59:48 PDT
Comment on attachment 606812 [details] [diff] [review]
WIP v2

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

Looking generally good. Don't forget the tests for the new view :-)

::: mobile/android/base/db/BrowserContract.java.in
@@ +133,5 @@
>          public static final String VISITS = "visits";
>      }
>  
> +    // Combined bookmarks and history entries.
> +    public static final class Combined  {

Shouldn't Combined implement URLColumns and ImageColumns at least?

@@ +137,5 @@
> +    public static final class Combined  {
> +        private Combined() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "combined");
> +
> +        public static final String BOOKMARK_ID = "bid";

No need to abbreviate that much and make the column name look so cryptic :-P bookmarks_id is more obvious.

@@ +138,5 @@
> +        private Combined() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "combined");
> +
> +        public static final String BOOKMARK_ID = "bid";
> +        public static final String HISTORY_ID = "hid";

Ditto.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +181,5 @@
>  
> +        // Combined bookmarks and history
> +        URI_MATCHER.addURI(BrowserContract.AUTHORITY, "combined", COMBINED);
> +
> +        map = new HashMap<String, String>();

I'd still add the corresponding columns here. For clarity.
Comment 26 :Margaret Leibovic 2012-03-19 13:52:34 PDT
(In reply to Lucas Rocha (:lucasr) from comment #25)

> ::: mobile/android/base/db/BrowserProvider.java.in
> @@ +181,5 @@
> >  
> > +        // Combined bookmarks and history
> > +        URI_MATCHER.addURI(BrowserContract.AUTHORITY, "combined", COMBINED);
> > +
> > +        map = new HashMap<String, String>();
> 
> I'd still add the corresponding columns here. For clarity.

Is there a reason to even create a COMBINED_PROJECTION_MAP variable? I'm currently not using it in query's COMBINED case, since the view is already taking care of the projection for us. I was looking at some of the comments from bug 725914, and I hope we're not hurting performance by doing a SELECT in the view, but I think we need to do it in order to rename the image columns.
Comment 27 :Margaret Leibovic 2012-03-19 13:54:31 PDT
(In reply to Margaret Leibovic [:margaret] from comment #26)
> (In reply to Lucas Rocha (:lucasr) from comment #25)
> 
> > ::: mobile/android/base/db/BrowserProvider.java.in
> > @@ +181,5 @@
> > >  
> > > +        // Combined bookmarks and history
> > > +        URI_MATCHER.addURI(BrowserContract.AUTHORITY, "combined", COMBINED);
> > > +
> > > +        map = new HashMap<String, String>();
> > 
> > I'd still add the corresponding columns here. For clarity.
> 
> Is there a reason to even create a COMBINED_PROJECTION_MAP variable? I'm
> currently not using it in query's COMBINED case, since the view is already
> taking care of the projection for us. I was looking at some of the comments
> from bug 725914, and I hope we're not hurting performance by doing a SELECT
> in the view, but I think we need to do it in order to rename the image
> columns.

Talking to myself here, maybe we can get rid of the select in the view, and use the columns in the projection map to help us out, which is mentioned here: http://developer.android.com/reference/android/database/sqlite/SQLiteQueryBuilder.html#setProjectionMap%28java.util.Map%3Cjava.lang.String,%20java.lang.String%3E%29
Comment 28 :Margaret Leibovic 2012-03-19 16:06:27 PDT
Created attachment 607350 [details] [diff] [review]
patch

Talking to mfinkle, it sounds like we want to keep using FRECENCY_PROJECTION_MAP, but it's also still fast to do the select in the view like this. (This is still just rnewman's query, just concatenated together with constant names.)
Comment 29 :Margaret Leibovic 2012-03-19 16:08:12 PDT
Created attachment 607354 [details] [diff] [review]
test

I just added another test to testBrowserProvider. This test just tests the new view, not any of the special frecency sorting (that will be done in bug 736171).
Comment 30 :Margaret Leibovic 2012-03-19 16:17:23 PDT
Also, I switched to using "Frecency" instead of "Combined" for the same, since mfinkle suggested that, but now I'm thinking I like "Combined" more, since it's more literally what's going on here, and frecency is just sorting that's done elsewhere on top of that. So, I can change that back if we think it's a better name.

And another also, I realized I didn't look into the id issues I found from comment 24, so I'm going to do that now. Hopefully if I can figure out what's going wrong, we can just get rid of that bogus frecency id column.
Comment 31 :Margaret Leibovic 2012-03-19 16:32:10 PDT
(In reply to Margaret Leibovic [:margaret] from comment #30)

> And another also, I realized I didn't look into the id issues I found from
> comment 24, so I'm going to do that now. Hopefully if I can figure out
> what's going wrong, we can just get rid of that bogus frecency id column.

It appears the problem is that a SimpleCursorAdapter demands an _id column, so the TopSitesCursorAdapter is getting upset when that's not included:
http://stackoverflow.com/questions/8090425/android-simplecursoradapter-no-such-column-id

Any suggestions for a good way to come up with a unique id for the rows in this view? The current hack I put in place (using the history id as the frecency id) would theoretically cause problems for unvisited bookmark entries, and since I'm not sure what CursorAdapter is actually using this for, we'd probably be safest to just come up with actual unique ids.
Comment 32 Richard Newman [:rnewman] 2012-03-19 16:47:20 PDT
(In reply to Margaret Leibovic [:margaret] from comment #31)

> Any suggestions for a good way to come up with a unique id for the rows in
> this view?

As suggested on IRC:

* Try using 0.
* Compute two distinct sets of values: make bookmark-only results negative bookmark_id, combined use positive history_id. Use COALESCE to combine them. Or if you need positive IDs, add half of maxint to one set.
Comment 33 Lucas Rocha (:lucasr) 2012-03-20 03:59:14 PDT
(In reply to Margaret Leibovic [:margaret] from comment #30)
> Also, I switched to using "Frecency" instead of "Combined" for the same,
> since mfinkle suggested that, but now I'm thinking I like "Combined" more,
> since it's more literally what's going on here, and frecency is just sorting
> that's done elsewhere on top of that. So, I can change that back if we think
> it's a better name.

FWIW, I prefer COMBINED as it better match the contents of the view.
Comment 34 Lucas Rocha (:lucasr) 2012-03-20 04:13:32 PDT
(In reply to Margaret Leibovic [:margaret] from comment #31)
> since I'm not sure what CursorAdapter is actually using this
> for, we'd probably be safest to just come up with actual unique ids.

According to CursorAdapter code, it only uses the _id column as a default implementation for the getItemId() method. This method is used as a simple way to fetch a specific row from its ID. In case of CursorAdapter, it assumes the item ID will come from an _id column in the cursor. We're not using this feature at all in awesomebar and about:home so we don't really care about the _id column: you can just always use 0 for all rows for example (with a comment explaining why).

We do care about the row IDs in the bookmarks tab in awesomebar but that's unrelated to what you're implementing here.
Comment 35 Lucas Rocha (:lucasr) 2012-03-20 04:36:22 PDT
Comment on attachment 607354 [details] [diff] [review]
test

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

Looks generally

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +873,5 @@
> +
> +    class TestFrecencyView extends Test {
> +        public void test() throws Exception {
> +            ContentValues historyEntry1 = createHistoryEntry("History Item", "http://example.com",
> +                10, System.currentTimeMillis());

You probably also want to add history entries and bookmarks that contain images and see if the image value shows up correctly in the view.

@@ +876,5 @@
> +            ContentValues historyEntry1 = createHistoryEntry("History Item", "http://example.com",
> +                10, System.currentTimeMillis());
> +            mProvider.insert(mHistoryUri, historyEntry1);
> +
> +            ContentValues bookmarkEntry1 = createBookmark("Unvisited Bookmark", "http://example.org",

Maybe make http://example.com a local constant FIRST_URL and use it in the assert below? Same applies to the othe urls in the test. Just avoid typos and make it more readable.

@@ +889,5 @@
> +                mMobileFolderId, 0, 0, "tags", "description", "keyword");
> +            mProvider.insert(mBookmarksUri, bookmarkEntry2);
> +
> +            // Sort entries by url so we can check them individually
> +            Cursor c = mProvider.query(mFrecencyUri, null, "", null, mFrecencyUrlCol);

It would be nice if you check the columns that came up from the query and see if they are what you expect.

@@ +902,5 @@
> +                         "Second frecency item has correct url");
> +
> +            mAsserter.is(c.moveToNext(), true, "Found third frecency entry");
> +            mAsserter.is(c.getString(c.getColumnIndex(mFrecencyUrlCol)), "http://examples2.com",
> +                         "Third frecency item has correct url");

What title has precedence in the view, history's or bookmark's? In other words, for a visited bookmark, which title will be used if the history entry has a different title than the bookmark? It would be nice to add a small assert for that too.
Comment 36 Lucas Rocha (:lucasr) 2012-03-20 04:41:44 PDT
Comment on attachment 607350 [details] [diff] [review]
patch

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

Looks pretty good. Just need the new name and rebasing to get a r+.

::: mobile/android/base/db/BrowserContract.java.in
@@ +137,3 @@
>  
> +    // Combined bookmarks and history entries used for frecency queries
> +    public static final class Frecency implements CommonColumns, URLColumns, HistoryColumns, ImageColumns  {

As I said in a comment, I'd prefer Combined over Frecency as it better matches the semantics here.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +590,5 @@
>              createHistoryWithImagesView(db);
>          }
>  
> +        private void upgradeDatabaseFrom3to4(SQLiteDatabase db) {
> +            createFrecencyWithImagesView(db);

I'm about to push a migration from 3 to 4 :-) You'll have to rebase to do 4 to 5.
Comment 37 :Margaret Leibovic 2012-03-21 14:47:26 PDT
Created attachment 608096 [details] [diff] [review]
patch v2

This patch addresses the review comments. However, my new test (which I will upload next) is finding some problems, so I think we should hold off on review until we get that sorted out.
Comment 38 :Margaret Leibovic 2012-03-21 14:54:20 PDT
Created attachment 608103 [details] [diff] [review]
test v2

A bunch of these assertions are failing, and I'm not sure if it's a problem with the test, or actually a problem with the view BrowserProvider. The test bails as soon as one assertion fails, so it's hard to tell how many are failing, but within the basic history entry section, the mCombinedHistoryIdCol, mCombinedTitleCol, and mCombinedVisitsCol checks fail. When I was running the test, it got 10 for the historyId, but we were expecting 8, and then it got 8 for visits, when we expected 10 - is it possible for something to have gone wrong to reverse these values? Also, it got null for the title, when we expected it to be TITLE_1.

And another thing, in createCombinedWithImagesView, we're explictly setting -1 as the history_id for bookmark items, but we don't do anything to set -1 as the bookmark_id for history items that aren't bookmarked. My test was finding 0 as the bookmark_id for an unbookmarked history item, and I'm wondering if we should do something to ensure it's -1 instead.
Comment 39 :Margaret Leibovic 2012-03-21 15:34:23 PDT
Update: It does seem like the columns are getting mixed up somehow. I've been commenting out tests, trying to find more failures, and it seems the image values are also mixed up:
TEST-UNEXPECTED-FAIL | testBrowserProvider - TestCombinedView | Basic history entry has corresponding favicon image - got HISTORY_THUMBNAIL, expected HISTORY_FAVICON

Could this be a problem just with the test? Testing on my own build, favicons and screenshots are working as expected. However, some titles do seem to be null (the url is listed for websites that definitely have titles), so I'm still skeptical of the patch. I'll continue to investigate!
Comment 40 :Margaret Leibovic 2012-03-21 15:49:00 PDT
(In reply to Margaret Leibovic [:margaret] from comment #39)
> Update: It does seem like the columns are getting mixed up somehow. I've
> been commenting out tests, trying to find more failures, and it seems the
> image values are also mixed up:
> TEST-UNEXPECTED-FAIL | testBrowserProvider - TestCombinedView | Basic
> history entry has corresponding favicon image - got HISTORY_THUMBNAIL,
> expected HISTORY_FAVICON
> 
> Could this be a problem just with the test?

Yes, it could be! I used the wrong variable accidentally :/ However, still looking into the other problems.
Comment 41 :Margaret Leibovic 2012-03-21 16:17:04 PDT
Created attachment 608133 [details] [diff] [review]
patch v3

So, I found there were two issues causing my problems: 1) the order of the columns in the union weren't exactly the same, which was causing my swapped-value problem, and 2) we were ignoring the history title in our history/bookmark join. In the second case, I decided we should prioritize using the bookmark title over the history title, since the user might have customized it.
Comment 42 :Margaret Leibovic 2012-03-21 16:19:46 PDT
Created attachment 608135 [details] [diff] [review]
test v3

Fixed my dumb variable name copy/paste-o. All the tests pass! \o/

My only remaining concern is if it's a problem that the bookmark_id is 0 for an un-bookmarked history item.
Comment 43 :Margaret Leibovic 2012-03-21 16:22:32 PDT
Created attachment 608136 [details] [diff] [review]
test v3

(Sorry, wrong patch.)
Comment 44 Lucas Rocha (:lucasr) 2012-03-22 04:39:30 PDT
Comment on attachment 608133 [details] [diff] [review]
patch v3

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

Just realized that I missed 2 important things in my previous review:
1. You have to update the UI bits (awesomebar filter and about:home top sites) that were using History.* to use Combined.* columns names.
2. Before your patch, we only used history entries in the awesomebar query so the talos test (testBrowserProviderPerf) only uses history entries right now. You probably have to update the test to also insert a few hundreds of unvisited and visited bookmarks before running the query. It's important to keep the talos test using all data that is used by the awesomebar filter query. 

Another important thing: could you please run testBrowserProviderPerf talos test (you have uncomment the entry in robocop.ini) with and without this patch and post the results here? Ideally, run 5 times each and average the results. This is just to make sure we're not adding a big performance regression here.
Comment 45 Lucas Rocha (:lucasr) 2012-03-22 04:51:30 PDT
Comment on attachment 608136 [details] [diff] [review]
test v3

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

Awesome.
Comment 46 :Margaret Leibovic 2012-03-22 11:18:16 PDT
Created attachment 608400 [details] [diff] [review]
talos test changes

This adds 500 unvisited bookmarks and 500 visited bookmarks to the test set-up. Do we have any data about how many bookmarks people tend to have?

I ran this talos test with and without my patch applied, and there was definitely some regression:

Without patch: 138, 141, 139, 139, 143
With patch: 365, 372, 363, 366, 360

I'm not sure how much of a hit we're willing to take here, but we do really want bookmarks showing up in awesomebar results. Maybe we can try to see if there are any tricks we can pull to make this query faster.
Comment 47 Gian-Carlo Pascutto [:gcp] 2012-03-22 11:49:32 PDT
>Do we have any data about how many bookmarks people tend to have?

Yes, you can look this up in Telemetry. 60% of the users have < 100 bookmarks. 95% has less than 500. A few percent have *thousands*.

Note that those few percent will tend to be disproportionally represented in Nightly/Beta :-)
Comment 48 Richard Newman [:rnewman] 2012-03-22 12:09:03 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #47)

> Yes, you can look this up in Telemetry. 60% of the users have < 100
> bookmarks. 95% has less than 500. A few percent have *thousands*.

The most of I've heard of for one Sync user was about 30,000 manually bookmarked pages.

https://bugzilla.mozilla.org/show_bug.cgi?id=721713#c11
Comment 49 :Margaret Leibovic 2012-03-22 14:54:43 PDT
Comment on attachment 608400 [details] [diff] [review]
talos test changes

Switching review request to Geoff, since Lucas is away until Tuesday.

Geoff, comment 44 talks about what Lucas wanted me to add to this talos test.
Comment 50 Geoff Brown [:gbrown] 2012-03-22 18:01:38 PDT
Comment on attachment 608400 [details] [diff] [review]
talos test changes

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

This looks good!

Minor concerns:
1. Lucas mentioned visited and unvisited bookmarks, but you have only added visited bookmarks -- is that intentional? Why?
2. I notice an exception on shutdown; it doesn't look serious, but if it can be avoided easily, it may avoid confusion / concern in future:

E/Cursor  (20125): Finalizing a Cursor that has not been deactivated or closed. database = /data/data/org.mozilla.fennec_mozdev/databases/testBrowserProviderPerf.browser.db, table = combined_with_images, query = SELECT _id, url, title, favicon FROM combined_with_images WHERE ((url LIKE ? OR title LIKE ?)) ORDER
E/Cursor  (20125): android.database.sqlite.DatabaseObjectNotClosedException: Application did not close the cursor or database object that was opened here
E/Cursor  (20125): 	at android.database.sqlite.SQLiteCursor.<init>(SQLiteCursor.java:212)
E/Cursor  (20125): 	at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:53)
E/Cursor  (20125): 	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1454)
E/Cursor  (20125): 	at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:330)
E/Cursor  (20125): 	at org.mozilla.fennec_mozdev.db.BrowserProvider.query(BrowserProvider.java:1348)
E/Cursor  (20125): 	at org.mozilla.fennec_mozdev.tests.ContentProviderTest$DelegatingTestContentProvider.query(ContentProviderTest.java:112)
E/Cursor  (20125): 	at android.content.ContentProvider$Transport.query(ContentProvider.java:163)
E/Cursor  (20125): 	at android.content.ContentResolver.query(ContentResolver.java:245)
E/Cursor  (20125): 	at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:167)
E/Cursor  (20125): 	at org.mozilla.gecko.db.LocalBrowserDB.filter(LocalBrowserDB.java:177)
E/Cursor  (20125): 	at org.mozilla.gecko.db.BrowserDB.filter(BrowserDB.java:117)
E/Cursor  (20125): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/Cursor  (20125): 	at java.lang.reflect.Method.invoke(Method.java:521)
E/Cursor  (20125): 	at org.mozilla.fennec_mozdev.tests.testBrowserProviderPerf.testBrowserProviderPerf(testBrowserProviderPerf.java:230)
E/Cursor  (20125): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/Cursor  (20125): 	at java.lang.reflect.Method.invoke(Method.java:521)
E/Cursor  (20125): 	at junit.framework.TestCase.runTest(TestCase.java:154)
E/Cursor  (20125): 	at junit.framework.TestCase.runBare(TestCase.java:127)
E/Cursor  (20125): 	at junit.framework.TestResult$1.protect(TestResult.java:106)
E/Cursor  (20125): 	at junit.framework.TestResult.runProtected(TestResult.java:124)
E/Cursor  (20125): 	at junit.framework.TestResult.run(TestResult.java:109)
E/Cursor  (20125): 	at junit.framework.TestCase.run(TestCase.java:118)
E/Cursor  (20125): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
E/Cursor  (20125): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
E/Cursor  (20125): 	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
E/Cursor  (20125): 	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1449)
D/Cursor  (20125): 
D/Cursor  (20125): Database path: /data/data/org.mozilla.fennec_mozdev/databases/testBrowserProviderPerf.browser.db
D/Cursor  (20125): 
D/Cursor  (20125): Table name   : combined_with_images
D/Cursor  (20125): 
D/Cursor  (20125): SQL          : SQLiteQuery: SELECT _id, url, title, favicon FROM combined_with_images WHERE ((url LIKE ? OR title LIKE ?)) ORDER BY visits * MAX(1, 100 * 225 / ((date - 1332463273036) / 86400000*(date - 1332463273036) / 86400000 + 225)) DESC LIMIT 100
I/dalvikvm(20125): Uncaught exception thrown by finalizer (will be discarded):
I/dalvikvm(20125): Ljava/lang/IllegalStateException;: Finalizing cursor android.database.sqlite.SQLiteCursor@482da098 on combined_with_images that has not been deactivated or closed
I/dalvikvm(20125): 	at android.database.sqlite.SQLiteCursor.finalize(SQLiteCursor.java:613)
I/dalvikvm(20125): 	at dalvik.system.NativeStart.run(Native Method)
Comment 51 :Margaret Leibovic 2012-03-22 22:39:40 PDT
(In reply to Geoff Brown [:gbrown] from comment #50)
> Minor concerns:
> 1. Lucas mentioned visited and unvisited bookmarks, but you have only added
> visited bookmarks -- is that intentional? Why?

I thought I did add unvisited bookmarks. The only way to make a "visited" bookmark is to create a history entry and a bookmark entry with the same url. The first loop that calls createRandomBookmarkEntry() makes unvisited bookmarks, since I'm not making any corresponding history entried, and the later "combined" loop makes the visited bookmarks.

> 2. I notice an exception on shutdown; it doesn't look serious, but if it can
> be avoided easily, it may avoid confusion / concern in future:

Hm, I'll look into that. It looks like the cursor that was already in the test isn't being closed, so I can add a call to close that (as well as the additional cursor I added in loadMobileFolderId().
Comment 52 :Margaret Leibovic 2012-03-23 14:40:56 PDT
Created attachment 608875 [details] [diff] [review]
talos test changes v2

This patch closes the cursors. As I mentioned in my last comment, I believe I am adding both visited/unvisited bookmarks, but if you can point out how I'm wrong, I'll fix it :)
Comment 53 :Margaret Leibovic 2012-03-23 14:50:03 PDT
Comment on attachment 608133 [details] [diff] [review]
patch v3

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

> Just realized that I missed 2 important things in my previous review:
> 1. You have to update the UI bits (awesomebar filter and about:home top
> sites) that were using History.* to use Combined.* columns names.

Lucas and I discussed this on IRC - this isn't necessary until bug 723623 lands, so we could do that later in a different bug.

> 2. Before your patch, we only used history entries in the awesomebar query
> so the talos test (testBrowserProviderPerf) only uses history entries right
> now. You probably have to update the test to also insert a few hundreds of
> unvisited and visited bookmarks before running the query. It's important to
> keep the talos test using all data that is used by the awesomebar filter
> query. 

My talos test patch addresses this issue.

> Another important thing: could you please run testBrowserProviderPerf talos
> test (you have uncomment the entry in robocop.ini) with and without this
> patch and post the results here? Ideally, run 5 times each and average the
> results. This is just to make sure we're not adding a big performance
> regression here.

I listed these results in comment 46. If we think the performance, then there's nothing left to address in this patch.
Comment 54 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-23 15:23:00 PDT
(In reply to Margaret Leibovic [:margaret] from comment #46)

> I ran this talos test with and without my patch applied, and there was
> definitely some regression:
> 
> Without patch: 138, 141, 139, 139, 143
> With patch: 365, 372, 363, 366, 360
> 
> I'm not sure how much of a hit we're willing to take here, but we do really
> want bookmarks showing up in awesomebar results. Maybe we can try to see if
> there are any tricks we can pull to make this query faster.

We are adding new functionality and this "regression" is not out of line.
Comment 55 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-23 15:27:15 PDT
Comment on attachment 608133 [details] [diff] [review]
patch v3

This is good to go
Comment 56 Geoff Brown [:gbrown] 2012-03-23 15:33:46 PDT
Comment on attachment 608875 [details] [diff] [review]
talos test changes v2

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

> As I mentioned in my last comment, I believe I am adding both visited/unvisited bookmarks, but if you can point out how I'm wrong, I'll fix it :)

I see it now -- sorry for the confusion!
Comment 59 :Margaret Leibovic 2012-04-03 08:24:29 PDT
Comment on attachment 608133 [details] [diff] [review]
patch v3

Mobile only. Release blocker.
Comment 60 :Margaret Leibovic 2012-04-03 08:24:54 PDT
Comment on attachment 608136 [details] [diff] [review]
test v3

[Approval Request Comment]
Mobile only. Release blocker.
Comment 61 :Margaret Leibovic 2012-04-03 08:25:13 PDT
Comment on attachment 608875 [details] [diff] [review]
talos test changes v2

[Approval Request Comment]
Mobile only. Release blocker.
Comment 62 Alex Keybl [:akeybl] 2012-04-03 15:08:02 PDT
Comment on attachment 608133 [details] [diff] [review]
patch v3

[Triage Comment]
Mobile only & blocking Fennec 1.0. Approved for Aurora 13.
Comment 64 Cristian Nicolae (:xti) 2012-05-22 08:17:35 PDT
Synced bookmarks are shown up in Awesomebar when a search is performed. Also about:home is updated correctly with the new synced data.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-22)
Device: Galaxy Nexus
OS: Android 4.0.2

Note You need to log in before you can comment on or make changes to this bug.