1024289 - Cleanup the "combined" bookmarks and history view

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Chris Kitching [:ckitching]]

Much of Fennec's database access, notably Awesomebar queries, use the "combined" view: a join on the history and bookmarks tables:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#672

It is of the form:

SELECT
... $A...
FROM (
    SELECT
    ... $B...
    UNION ALL
    SELECT
    ... $C...
)

Where $A lists every column in $B and $C. This query is therefore equivalent to:

SELECT
... $B...
UNION ALL
SELECT
... $C...

Provided the columns in $B and $C are appropriately reordered to match the order in $A. (In case there's some ridiculous code somewhere selecting columns by number...)

A clever query planner could perform this optimisation for us. SQLite's does not. The output of "EXPLAIN QUERY PLAN" for the above reveals:

Old:

order from  detail
0     0     "TABLE bookmarks WITH INDEX bookmarks_type_deleted_index"
0     0     "TABLE history"
1     1     "TABLE bookmarks WITH INDEX bookmarks_url_index"
0     0     "TABLE "

New:

order from  detail
0     0     "TABLE bookmarks WITH INDEX bookmarks_type_deleted_index"
0     0     "TABLE history"
1     1     "TABLE bookmarks WITH INDEX bookmarks_url_index"

The optimisation saves a table scan on the in-memory table created by the join.

The output of "EXPLAIN" (Showing the database virtual machine instructions generated for each of these queries):
Old: https://pastebin.mozilla.org/5394307
New: https://pastebin.mozilla.org/5394308

The new approach has approximately 20% fewer instructions.

This looks promising. These patches were used to evaluate the concrete pointfulness of the change:
https://www.dropbox.com/sh/fdq2c7nez6xk5x3/AAAtwUw_NhymCSPVNbLU9B9ja

Patch 1 introduces a downgrade path from database version 19 to 18 (the current one) assuming it differs only in the combined view. This allows a database that has had the redefined view applied to gracefully be reverted to the original format (so you can test without destroying your profile)

Patch 2 adds instrumentation code to database operations. An approximate SQL representation of each query is printed along with the time it took to execute. An average of all queries on the combined view is calculated and printed after each such query (this is the metric used to determine the performance gain from this change).

Patch 3 adds the redefined view and upgrade path for it (this is the only patch that I propose to land: the remainder lend credibility)

A build containing patches 1 and 2 will print measurements for the old "combined" view. Adding patch 3 causes it to apply the new view on startup and print measurements for that one instead.
The method used for testing was to type nonsense in the AwesomeBar until the average stopped changing significantly. A performance gain of approximately 30% was observed using the improved view on my small profile.

Readers with nontrivially-sized profiles are encouraged to try this out for themselves:

An instrumented APK using the old combined view can be found here (patches 1 and 2):
https://www.dropbox.com/s/6mj8o61mw5dayom/instrumentedSlow.apk

Equivalently instrumented, but with the new view (all three patches):
https://www.dropbox.com/s/053djuiwxpiv1yf/fasterCombined.apk

Go forth and mash buttons!
To revert your profile to the old view when done, merely load the one using the old view (it'll run the downgrade path).
In theory, no harm will come from having the new view and using old code, however...


In general, the AwesomeBar needs to stop using the combined view. The time cost of the query it performs on each keypress is linear in the sum of the sizes of the bookmarks and history databases! Fixing that is nontrivial, so I'm proposing this change to the combined view as a way to make it suck very slightly less (as well as to aid other parts of the app which use the combined view).

Comment 1

[Richard Newman [:rnewman]]

A 30% perf win would be nice.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Part 1: Remove redundant subquery from combined view.

Patch 3, for review.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8438891 [details] [diff] [review]
Part 1: Remove redundant subquery from combined view.

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

Quick skim:

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +676,4 @@
>  
>          db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_COMBINED + " AS" +
> +
> +                // Bookmarks without history.

Can we correct the indenting while we're here? Align under the quote.

@@ +732,5 @@
> +                        // list folder is not marked as deleted.
> +                        "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " " +
> +                            "WHEN 0 THEN " +
> +                                "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " " +
> +                                    "WHEN " + Bookmarks.FIXED_READING_LIST_ID + " THEN " +

I think this can go altogether: we no longer store reading list items in the bookmark table!

Part 2, tho. Leave good trails.

@@ +1580,5 @@
>  
>      private void upgradeDatabaseFrom15to16(SQLiteDatabase db) {
> +        // No harm in creating the v19 combined view here: means we don't need to almost-identical
> +        // functions to define both the v16 and v19 ones. The upgrade path will redundantly drop
> +        // and recreate the view again. *shrug*

Extra points for verifying this. I don't want the upgrade path to bomb out if we removed or added a column in 17 or 18.

Comment 4

[Chris Kitching [:ckitching]]

Attachment: Part 2: Remove redundant "IS NOT NULL" check.

While we're here, checking for history.url to be non-null is redundant: That column is declared NOT NULL.
Since database version 13 makes this declaration, any transition from it being nullable to NOT NULL will have been handled in migration code before then, so no new migration code need be added here.

A similar transformation is applied in the same where clause for bookmarks.type: there was logic in that clause that was equivalent to declaring type as "NOT NULL DEFAULT 1". Which it already is.

The query planner, amazingly, is not clever enough to optimise this out for us, either: two instructions removed from the output of "EXPLAIN" when applying this transformation, suggesting a minute performance gain might be had here, too.

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Add index on history.deleted.

Another interesting discovery: let's "EXPLAIN QUERY PLAN" on the awesomebar queries:

0,0,"TABLE bookmarks WITH INDEX bookmarks_type_deleted_index"
0,0,"TABLE history"
1,1,"TABLE bookmarks WITH INDEX bookmarks_url_index"
0,0,"TABLE "

Since these are SELECTs on combined, the first three rows are expected, and the last is the unavoidable table scan to evaluate those evil "LIKE" expressions.
But: this is two table scans. By creating an index on history.deleted we can avoid the first one:

0,0,"TABLE bookmarks WITH INDEX bookmarks_type_deleted_index"
0,0,"TABLE history WITH INDEX history_deleted_index"
1,1,"TABLE bookmarks WITH INDEX bookmarks_url_index"
0,0,"TABLE "

Comment 6

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #5)

> But: this is two table scans. By creating an index on history.deleted we can
> avoid the first one:

I don't think we want to index on deleted. (At least, unless you can give me concrete measurements on how it affects insertion time and disk space versus query gains.)

Indices are far from free, so I'd be tempted to look for alternate solutions.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #4)

> Since database version 13 makes this declaration, any transition from it
> being nullable to NOT NULL will have been handled in migration code before
> then, so no new migration code need be added here.

Let's verify that. I know that sqlite doesn't always verify existing rows match new constraints in ALTER TABLE, so I'm leery of assuming that everything is fine. Existing migration code should include the answer (e.g., deleting rows prior to ALTER TABLE).


> The query planner, amazingly, is not clever enough to optimise this...

Yeah, this definitely ain't postgres.

Comment 8

[Richard Newman [:rnewman]]

We got preliminary telemetry from some Nightly users.

13:57:19 < rnewman> mean time to load the cursor for top sites, amongst Nightly users: 353msec
13:57:33 < rnewman> 95th percentile: 1.3 seconds
13:57:48 < rnewman> longest time: 6 seconds

tracking?

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8438891 [details] [diff] [review]
Part 1: Remove redundant subquery from combined view.

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

r+ with a comment and verification that the v19 view can be safely created in the migration to 16, and running all the tests!

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +676,5 @@
>  
>          db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_COMBINED + " AS" +
> +
> +                // Bookmarks without history.
> +                " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + "," +

One of the values of the SELECT ... FROM () that we're removing here is a concise list of columns. Please add a comment that lists the selected columns in order.

@@ +687,5 @@
> +                        " WHEN " + Bookmarks.FIXED_READING_LIST_ID + " THEN " +
> +                            Combined.DISPLAY_READER +
> +                        " ELSE " +
> +                            Combined.DISPLAY_NORMAL +
> +                    " END AS " + Combined.DISPLAY + "," +

DISPLAY can go entirely. There are no longer reading list items stored as bookmarks. As an interim step (before amending consumers), you can always return Combined.DISPLAY_NORMAL.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8438918 [details] [diff] [review]
Part 2: Remove redundant "IS NOT NULL" check.

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

Add an UPDATE in the migration to do migrate null types to either bookmark or something else, based on some sane attribute. If anything snuck in, this'll save us; if not, the UPDATE will be free.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8438990 [details] [diff] [review]
Add index on history.deleted.

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

One step at a time. As I mentioned in an earlier comment, I want to be confident that we're making the right tradeoffs, and I doubt adding a fat index here will be the right one. Prove me wrong, or pick a different approach?

Comment 12

[Chris Kitching [:ckitching]]

Attachment: Part 3: Remove "display" field from "combined" view and update reading list logic as appropriate.

Apologies for the high latency. Been busy falling off of punts while trying to watch fireworks.

(In reply to Richard Newman [:rnewman] from comment #3)
> @@ +732,5 @@
> > +                        // list folder is not marked as deleted.
> > +                        "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " " +
> > +                            "WHEN 0 THEN " +
> > +                                "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " " +
> > +                                    "WHEN " + Bookmarks.FIXED_READING_LIST_ID + " THEN " +
> 
> I think this can go altogether: we no longer store reading list items in the
> bookmark table!

...

> DISPLAY can go entirely. There are no longer reading list items stored as
> bookmarks. As an interim step (before amending consumers), you can always
> return Combined.DISPLAY_NORMAL.

I appear to have omitted to explain my work in this direction.

The one case I'm unsure about is:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#122

Delete this menu item entirely?
We don't want to just let people open arbitrary things in reader mode as it may be mangled horribly. The check to determine if it is reader-mode-able occurs on page load, sooo... we're stuck.

Possible followup: Add "open in reader mode" to this menu for every bookmark and fail to an error page if, after loading the page, we can't reader-mode-ify it at all? Or is that just not worth the effort?

Whatever, removing it entirely in this patch, let me know if I missed something or there's a better way.
This involves destroying a number of tests entirely, and there's nothing sane to replace them with.

Try push of the first 3 parts (reassigning the reader-mode stuff as part 3, we can argue about the "deleted" table scan later):
https://tbpl.mozilla.org/?tree=Try&rev=2edb00c87119

Comment 13

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #12)

> > DISPLAY can go entirely. There are no longer reading list items stored as
> > bookmarks. As an interim step (before amending consumers), you can always
> > return Combined.DISPLAY_NORMAL.
> 
> I appear to have omitted to explain my work in this direction.
> 
> The one case I'm unsure about is:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeFragment.java#122


N.B., that code only applies to Most Recent, specifically when you opened a reader mode bookmark. No other makeInfoForCursor method sets display.

But even so, that doesn't work at all now, because there is no such thing as a reader mode bookmark.

There are no bookmarks with PARENT = FIXED_READING_LIST_ID, so there is never an item with Display = DISPLAY_READER. Try it: find anywhere in the interface where "Open in Reader" appears.

(I just added a page to my reading list. It shows up in reading list, of course, but the page in History is just a page, and there's no bookmark. Feature gone!)

The original goal here, as I recall, was: if you had opened a page in reader rode, you should be able to get back to it in reader mode directly. We knew it was a reader page because it was in your reading list.

If we still want that behavior, we need to reimplement it by joining against the reading list table (and instead we will probably implement multi-sourced lists/awesomebar instead); it's no longer relevant to the combined view.

So:

> Delete this menu item entirely?

I think so, because it's of no use right now. We don't store reader-mode-ness in History, and we don't store reading list items in bookmarks, so everything is DISPLAY_NORMAL.

Lucas, do you have any thoughts here?

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8441019 [details] [diff] [review]
Part 3: Remove "display" field from "combined" view and update reading list logic as appropriate.

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

I'm fine with this if lucasr and margaret are. Let's check with them first.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +484,3 @@
>      @Override
>      public boolean isBookmark(ContentResolver cr, String uri) {
>          // This method is about normal bookmarks, not the Reading List.

Correct this comment.

@@ -496,3 @@
>                                   Bookmarks.PARENT + " != ?",
>                           new String[] { uri,
> -                                 String.valueOf(Bookmarks.FIXED_READING_LIST_ID),

This chunk is valid regardless of whether there's a display field -- it should have been part of Bug 959290.

@@ -1064,3 @@
>                                    new String[] { title,
> -                                                 Long.toString(parent),
> -                                                 String.valueOf(Bookmarks.FIXED_READING_LIST_ID)

Likewise.

@@ +1058,3 @@
>                                    });
>          } else if (type == Bookmarks.TYPE_SEPARATOR) {
>              // Or their their position (seperators)

Fix comment: separators.

@@ +1065,3 @@
>                                    });
>          } else {
>              Log.e(LOGTAG, "Bookmark entry without url or title and not a seperator, not added.");

Fix.

::: mobile/android/base/home/TopSitesPanel.java
@@ +278,5 @@
>          // Long pressed item was a Top Sites GridView item, handle it.
>          MenuInflater inflater = new MenuInflater(view.getContext());
>          inflater.inflate(R.menu.home_contextmenu, menu);
>  
>          // Hide ununsed menu items.

Fix this comment typo.

Comment 15

[Chris Kitching [:ckitching]]

Attachment: Part 1 (V2): Remove redundant subquery from combined view.

Adding the requested comment.

Don't land this until we're finished mucking about with the combined view! We don't want Nightly users to migrate to 19 before we're done changing things.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8441019 [details] [diff] [review]
Part 3: Remove "display" field from "combined" view and update reading list logic as appropriate.

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

I would suggest that you also get rid of DISPLAY, DISPLAY_NORMAL, and DISPLAY_READER in BrowerContract, but I suppose we need those for the old migrations to compile :/ I suggest you just add a comment in BrowserContract explaining that these fields are deprecated.

Looks fine to me, but we should double check with Lucas as well, since he wrote a lot of this.

::: mobile/android/base/resources/menu/home_contextmenu.xml
@@ -11,5 @@
>      <item android:id="@+id/home_open_private_tab"
>            android:title="@string/contextmenu_open_private_tab"/>
>  
> -    <item android:id="@+id/home_open_in_reader"
> -          android:title="@string/contextmenu_open_in_reader"/>

You should also remove this unused string.

We should also file a follow-up bug to address the fact that this feature is missing now. I don't think the feature is very missed, but it looks like removing it was an accident rather than a deliberate decision, so we should follow up on what we want to do about that.

Comment 17

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #16)

> I would suggest that you also get rid of DISPLAY, DISPLAY_NORMAL, and
> DISPLAY_READER in BrowerContract, but I suppose we need those for the old
> migrations to compile :/ I suggest you just add a comment in BrowserContract
> explaining that these fields are deprecated.

We can move those to Obsolete.

> We should also file a follow-up bug to address the fact that this feature is
> missing now. I don't think the feature is very missed, but it looks like
> removing it was an accident rather than a deliberate decision, so we should
> follow up on what we want to do about that.

Filed Bug 1027858.

Comment 18

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #10)
> Add an UPDATE in the migration to do migrate null types to either bookmark
> or something else, based on some sane attribute. If anything snuck in,
> this'll save us; if not, the UPDATE will be free.

Isn't that impossible?

You want me to write a query to replace NULL values in a field declared as NOT NULL with some sane default value?
Surely no such values can possibly exist (by definition!), so such a query cannot possibly do anything.

Presumably you're trying to deal with the situation where an old database format from the dawn of time allowed for null values here and some of these have sneaked into our 18->19 migration step.

But, we know from inspection of the code that (at the latest) v13 of the schema declares this field as non-null, so any valid migration from a version < 13 to version 13 will necessarily eliminate all null values from the table.

As a result, no database of version 13 or greater can possibly contain any null values for a query in the 18->19 migration to fix, so there's no point writing it. If earlier migrations didn't make sensible decisions about default values for such nulls, well, tough: the damage is already done.

This, of course, assumes that nobody has ever sneakily deployed different schemas with the same version number allowing for hilarious inconsistencies to appear.
If that assumption doesn't hold, we're completely, hopelessly screwed: a changeset-by-changeset inspection of every change to the database code there has ever been will be required to figure out what the *real* schema migrations that have occurred over time have been and behave accordingly. *shudder*

... Or did I miss something obvious, again?

Comment 19

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #18)

> As a result, no database of version 13 or greater can possibly contain any
> null values for a query in the 18->19 migration to fix, so there's no point
> writing it. If earlier migrations didn't make sensible decisions about
> default values for such nulls, well, tough: the damage is already done.

You're assuming that every version of sqlite on every version of Android enforces constraints at insertion time or when altering a table, and that earlier migrations were correct. You've already agreed that verifying every migration is impossible, so saying "no point" is somewhat trusting to luck.

Given that we have concrete experience with sqlite on some Android versions *not* enforcing constraints, particularly foreign keys (heck, that's documented!), I'd rather be safe than sorry.

That is, I don't think it's wise to assume that a column made NOT NULL in some migration is guaranteed not to contain any null values, especially given that (a) we're removing a CASE that would have hidden a problem, and (b) the check should be as trivial as:

  UPDATE bookmarks SET type = 'bookmark' WHERE type IS NULL;

I *expect* everything to be fine. But adding this migration *proves* it for 19+, and thus firewalls the doubt.


> ... Or did I miss something obvious, again?

You're just assuming that it's possible or worthwhile to prove universal correctness, with that as the fundament for each decision. I'm asserting that a cheap and cheerful fix will either confirm that there are no monsters under the bed, or save us if there are, will give us confidence in the future, and that in situations of doubt it's better to double check than to assume correctness.

(This is the DB equivalent of having null checks at module boundaries -- provably redundant, if you're willing to spend the time to do the proof, but the "consequence gradient" is very biased!)

Comment 20

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #19)

I see. That makes sense, though is nonpretty.

That said, we are accumulating a very large amount of this sort of probably-redundant code all over the place. Would it be worthwhile for me to make automated tests of this system so we can prove large chunks of it pointless and throw them away? 
This would have the added bonus of us being able to assert other, similar constraints on the database going forwards, saving us a little sanity.
Do we know if this migration code even works at all? Without tests, isn't the whole upgrade pathway a huge liability?

Also, if we are going to assume that the database engine doesn't enforce constrains, surely we're compelled to write into the upgrade path assertive queries like the one you asked for for every constraint we wish to have on our database? Scattering "belt-and-braces" probably-hopefully-mostly-usually-redundant checks into queries throughout the application is something that must be avoided: when someone forgets one you have a failure that occurs only on certain devices, with certain SQLite versions, on Tuesdays...

That is, of course, rather out of scope for this bug.


> (This is the DB equivalent of having null checks at module boundaries --
> provably redundant, if you're willing to spend the time to do the proof, but
> the "consequence gradient" is very biased!)

Familiar, though you can make a lot of headway on that front using static analysis, so it'd make a lot of sense to have a system that does one or both of:

Deleting such null checks as can be proven to be pointless (hooray!, you're effectively using null checks as assertions to prove correctness)

Printing warnings when something you want to be not null might be (or the reverse). Useful for spotting bugs.


Bolting these sorts of things on as runtime checks is usually better than not having them at all, but is still sort of awful in some cases.

Ideally you want to be able to assert "Module X will never give me a null here" and have that either be proven at compile-time (and the check removed, because your condition holds), or, if we're unable to do the proof (it is undecidable in general, after all...), to stick in a runtime check. This would seem to provide the best of both worlds (and is something that could be extended using constraint analysis to more sophisticated predicates than simply "is null" or "is not null".
... Somebody *must* have written such a thing for Java... Riiiight? If not it'd be a fun thing to do :P

Comment 21

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #20)

> I see. That makes sense, though is nonpretty.

Aye, like so much in the world!


> That said, we are accumulating a very large amount of this sort of
> probably-redundant code all over the place. Would it be worthwhile for me to
> make automated tests of this system so we can prove large chunks of it
> pointless and throw them away? 

Tests are almost always welcome. This is one of the particularly welcome areas!


> This would have the added bonus of us being able to assert other, similar
> constraints on the database going forwards, saving us a little sanity.
> Do we know if this migration code even works at all? Without tests, isn't
> the whole upgrade pathway a huge liability?

Yes. However: we have generally relied on (a) inspection, and (b) induction, with induction being the main guarantor of safety -- that is, if a migration worked step by step for us and millions of users as we deployed new versions, then if we don't break any of the steps, it'll work to call those same migrations in order for all users, even if they come late to the table.

You plan to add some risk here by changing the steps, so we can no longer rely on induction.

(Being clear: these migrations *only* affect existing users migrating from old versions. New users start up to date, and most old users typically hop one at a time as we release updates, so they're not affected. The only reason we're maintaining this code is that we have non-trivial numbers of users with old versions installed, and we don't want to cut their upgrade path.)


> Also, if we are going to assume that the database engine doesn't enforce
> constrains, surely we're compelled to write into the upgrade path assertive
> queries like the one you asked for for every constraint we wish to have on
> our database?

No, only when we add a constraint to a field that didn't have one -- and typically those manual migrations are needed anyway, because either the ALTER TABLE will fail due to a constraint violation, or it won't, which is even worse!

E.g., if we make a column unique, we first need to ensure it contains unique values, and handle the collisions. If we make a column non-null, we need to fix the nulls.


> Scattering "belt-and-braces"
> probably-hopefully-mostly-usually-redundant checks into queries throughout
> the application is something that must be avoided: when someone forgets one
> you have a failure that occurs only on certain devices, with certain SQLite
> versions, on Tuesdays...

Yup, that's why I advocate having a firewalling migration, especially when it should be free. Migrations happen once; query guards and null checks happen all the time.


> ... Somebody *must* have written such a thing for Java... Riiiight? If not
> it'd be a fun thing to do :P

Eyes on the prize! Good enough is good enough.

Comment 22

[Chris Kitching [:ckitching]]

Attachment: Part 2 (V2): Remove redundant "IS NOT NULL" check.

Adding the extra evil check...

Comment 23

[Chris Kitching [:ckitching]]

Attachment: Part 3 (V2): Remove "display" field from "combined" view and update reading list logic.

Applied Richard's comments, shifted the dead fields to Obsolete.Combined, deleted Margaret's redundant string, and am continuing to try very hard not to burst into tears every time I look at the sequence of near-identical view creation functions we now have in BrowserDatabaseHelper.



(In reply to Richard Newman [:rnewman] from comment #14)
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +484,3 @@
> >      @Override
> >      public boolean isBookmark(ContentResolver cr, String uri) {
> >          // This method is about normal bookmarks, not the Reading List.
> 
> Correct this comment.

Deleting it entirely seems to make most sense.

> @@ -496,3 @@
> >                                   Bookmarks.PARENT + " != ?",
> >                           new String[] { uri,
> > -                                 String.valueOf(Bookmarks.FIXED_READING_LIST_ID),
> 
> This chunk is valid regardless of whether there's a display field -- it
> should have been part of Bug 959290.
> 
> @@ -1064,3 @@
> >                                    new String[] { title,
> > -                                                 Long.toString(parent),
> > -                                                 String.valueOf(Bookmarks.FIXED_READING_LIST_ID)
> 
> Likewise.

Whoops!

> @@ +1058,3 @@
> >                                    });
> >          } else if (type == Bookmarks.TYPE_SEPARATOR) {
> >              // Or their their position (seperators)
> 
> Fix comment: separators.

"seperators" no longer occurs in the Java code.

This whole method is sort of... Ick. 
For instance: That bunch of null checks a few lines up from this chunk - isn't "put" null safe, so you could just omit the checks? Indeed, passing a null here is how you insert nulls, and in the case where you've got a null input to this function and the field is not null, we want to know about it instead of silently leaving that field at its default value!
 
> ::: mobile/android/base/home/TopSitesPanel.java
> @@ +278,5 @@
> >          // Long pressed item was a Top Sites GridView item, handle it.
> >          MenuInflater inflater = new MenuInflater(view.getContext());
> >          inflater.inflate(R.menu.home_contextmenu, menu);
> >  
> >          // Hide ununsed menu items.
> 
> Fix this comment typo.

Also fixed an identical one in HomeFragment.java.

Finally, a try push to ensure nothing caught fire along the way:
https://tbpl.mozilla.org/?tree=Try&rev=9c5433ea45c4

And I think that's about it? 
More radical changes would be beneficial in this area, but they're probably involved enough to warrant their own bug(s).
Further work should probably begin with creating a decent set of unit tests for this code so we can confidently prune redundant migration code. Otherwise, further development in this area is going to become increasingly silly as the probably-mostly-dead code accumulates.

Comment 24

[Chris Kitching [:ckitching]]

... Apologies for managing to clobber absolutely every flag...

See Comment 13 for the thing input is requested on, Lucas.

Comment 25

[Richard Newman [:rnewman]]

Looks like you need to fix a couple of tests (or bugs):

6 INFO TEST-UNEXPECTED-FAIL | testBrowserProvider - TestSpecialFolders | Right number of special folders - got 6, expected 7

And troboprovider:

 TalosError: Retrieved results: got 0, expected 100 [browser_output.txt]

Comment 26

[Chris Kitching [:ckitching]]

The failure appears to be caused by the part 2 patch. It's not yet clear why.

Comment 27

[Chris Kitching [:ckitching]]

Attachment: Part 2 (V3): Remove redundant "IS NOT NULL" check.

Fixing fatal typo in part 2. Still unclear why it's killing Talos.

Comment 28

[Chris Kitching [:ckitching]]

Attachment: Part 3 (V3): Remove "display" field from "combined" view and update reading list logic.

No longer breaks the robocop tests (Just needed to update the expected number of special entries to account for the now nonexistent reading list folder).

Comment 29

[Richard Newman [:rnewman]]

I'll take this through to landing.

Comment 30

[Chris Kitching [:ckitching]]

And because you volunteered to do the head-smashing about talos yourself... Merry christmas.

Comment 31

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #13)
> (In reply to Chris Kitching [:ckitching] from comment #12)
> 
> > > DISPLAY can go entirely. There are no longer reading list items stored as
> > > bookmarks. As an interim step (before amending consumers), you can always
> > > return Combined.DISPLAY_NORMAL.
> > 
> > I appear to have omitted to explain my work in this direction.
> > 
> > The one case I'm unsure about is:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> > HomeFragment.java#122
> 
> 
> N.B., that code only applies to Most Recent, specifically when you opened a
> reader mode bookmark. No other makeInfoForCursor method sets display.
> 
> But even so, that doesn't work at all now, because there is no such thing as
> a reader mode bookmark.
> 
> There are no bookmarks with PARENT = FIXED_READING_LIST_ID, so there is
> never an item with Display = DISPLAY_READER. Try it: find anywhere in the
> interface where "Open in Reader" appears.
> 
> (I just added a page to my reading list. It shows up in reading list, of
> course, but the page in History is just a page, and there's no bookmark.
> Feature gone!)
> 
> The original goal here, as I recall, was: if you had opened a page in reader
> rode, you should be able to get back to it in reader mode directly. We knew
> it was a reader page because it was in your reading list.
> 
> If we still want that behavior, we need to reimplement it by joining against
> the reading list table (and instead we will probably implement multi-sourced
> lists/awesomebar instead); it's no longer relevant to the combined view.
> 
> So:
> 
> > Delete this menu item entirely?
> 
> I think so, because it's of no use right now. We don't store
> reader-mode-ness in History, and we don't store reading list items in
> bookmarks, so everything is DISPLAY_NORMAL.
> 
> Lucas, do you have any thoughts here?

The relevance of this menu item is more of a design question I think. ibarlow, thoughts?

We added it to allow users to go straight to reader mode for reading list items in the search results. We didn't have the reading list panel back then.

Given that the feature has been hidden for some time now (i.e. broken) and no one noticed, I think we can just drop it.

Comment 32

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #27)
> Created attachment 8449723 [details] [diff] [review]
> Part 2 (V3): Remove redundant "IS NOT NULL" check.
> 
> Fixing fatal typo in part 2. Still unclear why it's killing Talos.

Interestingly, if I run the test in Eclipse, things work. That implies that DB setup is broken.

Comment 33

[Richard Newman [:rnewman]]

I don't see a difference in behavior between fx-team and your patches wrt test failure. So here's a try push:

https://tbpl.mozilla.org/?tree=Try&rev=a48523ed4063

Comment 34

[Chris Kitching [:ckitching]]

Attachment: Part 4: Strip more logic handling reading lists inside the bookmarks table.

Stumbled across some more junk logic you may like to kill off.

Talos seems to still be broken in just the same way, unclear why.
Robocop is again failing, but now for a different reason: Something to do with favicons, apparently.

Comment 35

[Chris Kitching [:ckitching]]

Attachment: Part 5?: Remove unnecessary autoboxing from testBrowserProvider

Lastly, that generated patch I showed you removing the unnecessary autoboxing from the test. Not bad for three seconds of work. :P

Comment 36

[Richard Newman [:rnewman]]

Comment on attachment 8450648 [details] [diff] [review]
Part 5?: Remove unnecessary autoboxing from testBrowserProvider

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

r+ with nits.

::: mobile/android/base/tests/testBrowserProvider.java
@@ +528,4 @@
>                           "Should not be able to insert bookmark with null position");
>  
>              id = insertWithNullCol(BrowserContract.Bookmarks.TYPE);
> +            mAsserter.is(id, (long) -1,

I would prefer "-1L" here and throughout, instead of (or as well as) the cast.

@@ +999,5 @@
>              long id = insertOneHistoryEntry();
>  
>              int deleted = mProvider.delete(BrowserContract.History.CONTENT_URI,
>                                             BrowserContract.History._ID + " = ?",
> +                                           new String[]{String.valueOf(id)});

Undo this change.

@@ +1020,5 @@
>              id = insertOneHistoryEntry();
>  
>              deleted = mProvider.delete(ContentUris.withAppendedId(BrowserContract.History.CONTENT_URI, id), null, null);
>              mAsserter.is((deleted == 1), true,
> +                    "Inserted history entry was deleted using URI with id");

Don't break indenting.

Comment 37

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #34)

> Robocop is again failing, but now for a different reason: Something to do
> with favicons, apparently.

I think this is actually fallout from my work in Bug 1016611; testBrowserProvider(Perf) follow a very non-standard approach for handling test profiles, and we've noticed oddness around this before (Bug 975969).

For example: testBrowserProviderPerf *fails without your patches* unless I run it in Eclipse (which reuses profiles).

These tests are broken.

Comment 38

[Richard Newman [:rnewman]]

I just ran a Robocop test locally, and got a DB downgrade error:

PROCESS-CRASH | java-exception | android.database.sqlite.SQLiteException: Can't downgrade database from version 19 to 18 at android.database.sqlite.SQLiteOpenHelper.onDowngrade(SQLiteOpenHelper.java:361)


**** THIS SHOULD NEVER HAPPEN ****. We should never be reusing a database for a test run. That's probably contributing to problems.

Comment 39

[Richard Newman [:rnewman]]

Aha.

                     " FROM " + TABLE_HISTORY + " LEFT OUTER JOIN " + TABLE_BOOKMARKS +
                     " ON " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " = " + qualifyColumn(TABLE_HISTORY, History.URL) +
                     " WHERE " +
-                        qualifyColumn(TABLE_HISTORY, History.URL) + " IS NOT NULL AND " +
                         qualifyColumn(TABLE_HISTORY, History.IS_DELETED) + " = 0 AND " +
-                        "(" +
-                            qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " IS NULL OR " +
-                            qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK +
-                        ")"
+                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK
         );


Your Part 2 is taking the LEFT OUTER JOIN and then making sure we only return matching bookmarks -- Bookmarks.TYPE is NULL for rows that only exist in history, so they'll never be TYPE_BOOKMARK.

The "my mozilla test" pages are all history items. Refusing to allow TYPE = NULL effectively turns the LEFT OUTER JOIN into a JOIN, and you get 0 results.

Comment 40

[Richard Newman [:rnewman]]

Part 4 needs to be upstreamed to android-sync, if we take it.

Try build for the rest, with Part 3 fixed:

https://tbpl.mozilla.org/?tree=Try&rev=93e2b987a35b

Comment 41

[Richard Newman [:rnewman]]

One more -- a different original patch needed fixing.

https://tbpl.mozilla.org/?tree=Try&rev=5164a7fcb1fe

Comment 42

[Chris Kitching [:ckitching]]

Attachment: Part 5: Remove unnecessary autoboxing from testBrowserProvider

Cleanup of the cleanup patch. Looks like I didn't set up IDEA's code generation to perfectly enough! Resolves your complaints.

Comment 43

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #39)
> Your Part 2 is taking the LEFT OUTER JOIN and then making sure we only
> return matching bookmarks -- Bookmarks.TYPE is NULL for rows that only exist
> in history, so they'll never be TYPE_BOOKMARK.
> 
> The "my mozilla test" pages are all history items. Refusing to allow TYPE =
> NULL effectively turns the LEFT OUTER JOIN into a JOIN, and you get 0
> results.

*facepalm*

That has to be my stupidest one yet.

Seems that everything is finally in order here - the try push went green.

Comment 44

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/6711d62510d5
https://hg.mozilla.org/integration/fx-team/rev/334ffd994ee9

Comment 45

[Richard Newman [:rnewman]]

(Those didn't really go out -- tree closed, but the push script is dumb.)

Comment 46

[Richard Newman [:rnewman]]

Comment on attachment 8451437 [details] [diff] [review]
Part 5: Remove unnecessary autoboxing from testBrowserProvider

I cleaned this up and pushed it as a part 0.

Comment 47

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/830832639683

Comment 48

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/4680831563a2
https://hg.mozilla.org/integration/fx-team/rev/1afd31610f81
https://hg.mozilla.org/integration/fx-team/rev/087013249cb7

Comment 49

[Richard Newman [:rnewman]]

Comment on attachment 8449723 [details] [diff] [review]
Part 2 (V3): Remove redundant "IS NOT NULL" check.

Landed this with fixes.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6711d62510d5
https://hg.mozilla.org/mozilla-central/rev/334ffd994ee9
https://hg.mozilla.org/mozilla-central/rev/830832639683
https://hg.mozilla.org/mozilla-central/rev/4680831563a2
https://hg.mozilla.org/mozilla-central/rev/1afd31610f81
https://hg.mozilla.org/mozilla-central/rev/087013249cb7