947018 - Drop some migration code from BrowserProvider

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

BrowserProvider has tons of code for old database migrations. So much code that it makes it really hard to read that file.

I propose we add some telemetry probes to those upgrade methods to see how often they're used, and if they're not used often (I suspect they're not), let's add some logic to throw away the DB and start over if it's version is lower than some value we decide.

To be really cautious, we could pick a version that would mean the user hasn't updated their browser in a year. At that point, I don't think they'd be really sad if their bookmarks and history are after opening Firefox for the first time in over a year.

Looking at blame, we actually haven't even upgraded browser.db since July, I'm willing to bet that the majority of those migrations are from early in the life of Fennec Native, and I'd be happy to see them go (and seriously, they might not work correctly anyway).

Comment 1

[Richard Newman [:rnewman]]

We might be able to short-circuit that telemetry work by simply seeing how many active users of old releases we have. Telemetry work wouldn't tell us much on its own, because it'd only report users who upgrade, right? So pick a cut-off point based on insanity (two years? a year?) and user counts.

The real trick is about users who haven't/can't upgrade for some reason. But presumably they're orphaned anyway...

Comment 2

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #1)
> We might be able to short-circuit that telemetry work by simply seeing how
> many active users of old releases we have. Telemetry work wouldn't tell us
> much on its own, because it'd only report users who upgrade, right? So pick
> a cut-off point based on insanity (two years? a year?) and user counts.
> 
> The real trick is about users who haven't/can't upgrade for some reason. But
> presumably they're orphaned anyway...

Great point. Who has this data about old versions?

But would that data be coming from ADIs? What we really want to know is how often dormant users re-awaken and update their old Firefox profiles.

Comment 3

[Richard Newman [:rnewman]]

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

> Great point. Who has this data about old versions?

Someone with Play access -- mfinkle is the most obvious target.

 
> But would that data be coming from ADIs? What we really want to know is how
> often dormant users re-awaken and update their old Firefox profiles.

We want to know one thing: how many users will be affected by the removal of a particular upgrade path?

We can determine that by this chain:

* Which Firefox versions shipped with that DB version? (Known.)
* How many installs do we have for those Firefox versions? (Known, modulo Aurora and Nightly, and we can ignore those. This is guaranteed to be more than actives.)
* What proportion of those users will ever upgrade?

The last is the only one that we don't know for sure (and it probably falls as the version number shrinks).

Your point is: we can estimate the rate of upgrade by watching how many users actually do go through the upgrade path.

To figure that out for sure, we need release telemetry, which we can't have.

Perhaps we can infer this by watching the old version Play numbers go down week-on-week? My bet is that the oldest are basically static, and thus we can kill all of the year+-old versions.

Or we can decide that we don't care for versions that are 1. really old, 2. have a small number of users anyway, because the number of affected users is guaranteed to be small.

Comment 4

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

I wouldn't be opposed to starting over, since we have not updated the DB in a while and providing a "reset" or "backup/restore" for core history and bookmarks data. We wouldn't be touching the other DBs.

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Make database migration logic less insane.

Roughly 25% of the migration code (by lines-of-code) is creating views, such as VIEW_COMBINED_WITH_IMAGES, VIEW_BOOKMARKS_WITH_IMAGES, etc.

The migration path creates every old version of these views and replaces them incrementally with newer as it goes along. Since view creation/deletion doesn't modify the actual *data*, it is safe to short-circuit view upgrades (omit all creations of the view except the final one) provided the migration path itself never queries the view.

All the createSomethingViewOnX methods are therefore pointless. The only reason you'd need to preserve the definition of the old view (and create it during update) is if you're going to query it in some of your migration logic, which never happens...

... Or did I miss something which makes this amusing dance necessary?


Anyway, here's a patch. I also went ahead and deleted a bunch of the unused stuff from BrowserContract.Obsolete.

Funfact: This reduces apk size by 13.8 kb (with Proguard turned on).

This idea could also be used to do away with things like the redundant creation of the Images table. Since that will require nontrivial modification of portions of the migration pathway which are actually mutating data, I refuse to touch it without this subsystem having unit tests (which I'm not going to write just now). In any case, the benefit possible from that work is somewhat less than what we get from this trivial change to how views are upgraded, so let's take the simple, juicy modification.

Comment 6

[Chris Kitching [:ckitching]]

Attachment: No more loop. Thanks, Duff!

Pointless bonus: Let's kill that loop.

(I think that's correct?)

Comment 7

[Chris Kitching [:ckitching]]

Comment on attachment 8441063 [details] [diff] [review]
No more loop. Thanks, Duff!

It's not, in the face of stupid partial upgrades. Blech.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8441061 [details] [diff] [review]
Make database migration logic less insane.

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

As far as I can tell, you only call createCombinedView in onCreate. That'll leave us view-less or worse in any upgraded database, which isn't acceptable.

You *need* to test this, ideally by installing very old Aurora versions (one per DB version), adding some data, then grabbing the DBs each time you upgrade. Then you're in a position to test this and make sure you get to the right end state.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ -1208,5 @@
>          db.execSQL("ALTER TABLE " + TABLE_BOOKMARKS +
>                     " RENAME TO " + TABLE_BOOKMARKS_TMP);
>  
>          debug("Dropping views and indexes related to " + TABLE_BOOKMARKS);
> -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_BOOKMARKS_WITH_IMAGES);

What happens if a view refers to a table with an incompatible schema? That's a reason to keep this, I think.

@@ -1239,5 @@
>                     " RENAME TO " + TABLE_HISTORY_TMP);
>  
>          debug("Dropping views and indexes related to " + TABLE_HISTORY);
> -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_HISTORY_WITH_IMAGES);
> -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_COMBINED_WITH_IMAGES);

Similarly.

@@ +936,5 @@
>                    " GROUP BY " + History.URL +
>                    " HAVING count(" + History.URL + ") > 1");
>  
>          db.execSQL("CREATE UNIQUE INDEX " + TABLE_DUPES + "_url_index ON " +
> +                TABLE_DUPES + " (" + History.URL + ")");

Unbreak that indenting :)

@@ +954,5 @@
>      }
>  
>      private void upgradeDatabaseFrom8to9(SQLiteDatabase db) {
>          createOrUpdateSpecialFolder(db, Bookmarks.READING_LIST_FOLDER_GUID,
> +                R.string.bookmarks_folder_reading_list, 5);

I believe this whole statement can go. We're only going to delete it again later. It looks like Sola did a shitty job removing the remnants of reading-list-as-bookmarks; please take a look.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Kill COMBINED_WITH_IMAGES

Okay, so let's stop trying to be quite so clever.

A really large amount of the code here is creating views.

Views are great: unlike tables, if you redefine one you haven't got to muck about migrating the data.

One view that takes up lots of source code and doesn't do anything useful: COMBINED_WITH_IMAGES.

It's obsolete: current databases don't have it or use it. All we want to do if we find this view is drop it.
The migration pathway at *no point* ever queries this view. It merely deletes it and recreates it for the new version... before dropping it again, and so on.

Our code contains functions for creating four different versions of it.

Clearly, since we *never* query it, we can safely just excise the whole damn thing and be done with it.

I claim that this transformation is sufficiently obviously safe that we can do it?

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Eliminate redundant edtions of Combined view

Exactly the same concept, now applied to the obsolete versions of the COMBINED view.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Remove HISTORY_WITH_IMAGES view

Found another one: this view is also never queried, only created and dropped. I've deleted every reference to it except the time we drop it. (that is, remove all the drop-and-recreate operations from the update path, but leave the final drop-and-forget one from when we finally stopped caring about it, so anyone with an old database will have this view removed when they update.

Comment 12

[Chris Kitching [:ckitching]]

Attachment: Remove BOOKMARKS_WITH_IMAGES view

And another. Exactly the same reasoning as the last one.

Comment 13

[Chris Kitching [:ckitching]]

Funfact: I've now deleted 27% of BrowserDatabaseHelper.java.

Comment 14

[Chris Kitching [:ckitching]]

Attachment: Kill remaining code for readinglist logic

After Richard's last comment, I had a look: Seems we can kill a lot of old logic now. Review from him because of the Sync implications (I think it's safe to do what I did, but this relies on the assumption that database update always happens before Sync starts trying to do things. If that's false we probably need to keep those conditions I deleted)

Comment 15

[Chris Kitching [:ckitching]]

Attachment: Remove dead code from BrowserContract

This lot is never used. Stabby stabby!

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8488417 [details] [diff] [review]
Kill remaining code for readinglist logic

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

Changes in /sync/ need to be upstreamed.

Comment 17

[Chris Kitching [:ckitching]]

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

Comment 18

[Chris Kitching [:ckitching]]

Attachment: Remove code for creating the images table

And now for something more contrversial...

Comment 19

[Chris Kitching [:ckitching]]

Comment on attachment 8488461 [details] [diff] [review]
Remove code for creating the images table

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

So, this patch is the first one that isn't just mopping up dead views: there's actual data to think about here.

Current databases do not use the images table, so we never want to create it. 

The only way we want to interact with this table is to copy stuff out of it into the favicons/thumbnails tables. We might even decide we don't care about doing that (stuff will just have no favicon or thumbnail in your history until you load it. *shrug*), which would let us prune quite a lot more code.

So, going through the patch...

::: mobile/android/base/db/BrowserContract.java
@@ -364,4 @@
>              public static final String GUID = "guid";
>              public static final String DATE_CREATED = "created";
>              public static final String DATE_MODIFIED = "modified";
> -            public static final String IS_DELETED = "deleted";

I'm removing these two fields because:

* They are never used
* These are the only fields in the Images table that were added since it was initially created:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb39bae574f8#l4.237

This means we can now guarantee that all the columns given in this Images class will really exist in any images table we might encounter, since they were present in the very first version of the images table.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ -181,5 @@
>          db.execSQL("CREATE INDEX history_visited_index ON " + TABLE_HISTORY + "("
>                  + History.DATE_LAST_VISITED + ")");
>      }
>  
> -    private void createImagesTable(SQLiteDatabase db) {

This chunk is pretty obvious: We never want to create the images table any more because our current schema doesn't use it, so we can get rid of the function for doing so.

... But what about its existing usages?

@@ -612,5 @@
>          debug("Dropping history temporary table");
>          db.execSQL("DROP TABLE IF EXISTS " + TABLE_HISTORY_TMP);
>      }
>  
> -    private void migrateImagesTable(SQLiteDatabase db) {

This function is now pointless.
It:

1) Renames the images table.
2) Deletes the views/indexes related to it.
3) Creates the images table again.
4) Copies all the data over.
5) Drops the temporary table.

Because of the way this is written, the only transformation on the database this method can perform is adding new columns (one or more of those two we deleted earlier, presumably).
... But we already established we don't care about either of those new columns. Because the variables naming them in BrowserContract are never used, we cannot possibly be querying those columns, so we don't care about making sure they're created.

So this function can go.

@@ -643,5 @@
>      private void upgradeDatabaseFrom6to7(SQLiteDatabase db) {
>          debug("Removing history visits with NULL GUIDs");
>          db.execSQL("DELETE FROM " + TABLE_HISTORY + " WHERE " + History.GUID + " IS NULL");
>  
> -        debug("Update images with NULL GUIDs");

We already decided we don't care about the GUID column (we never query it!), so we *definitely* don't care about making sure it has correct values in it.

Comment 20

[Chris Kitching [:ckitching]]

Finally, the last remaining uses of TABLE_IMAGES reside in the 12-13 update function:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#1159

Here, we copy favicon urls from the favicon database (which exists at that time) into the images table, import the contents of the images table into the thumbnails/favicons tables, and delete the images table for good.

Hopefully, it's now clear that my last patch doesn't affect this method at all. We can guarantee that all the columns referenced in this method will exist at this point, so we can be sure that if this function worked before my patch, it'll continue to do so afterwards.

A related question: Do we care enough about rescuing favicons/thumbnails from the images table to continue maintaining this code? We could delete almost the entire contents of upgradeDatabaseFrom12to13 if we decided we don't care.

Comment 21

[Richard Newman [:rnewman]]

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

> A related question: Do we care enough about rescuing favicons/thumbnails
> from the images table to continue maintaining this code? We could delete
> almost the entire contents of upgradeDatabaseFrom12to13 if we decided we
> don't care.

Ordinarily, yes. People complain when they upgrade Firefox and their Top Sites grid is empty. 

We might loosen that constraint for very old upgrades. Database version 12 is old; 15 was introduced in April 2013, so anyone still using 12 or earlier hasn't upgraded for at least 18 months.

Anyone in favor of migrating images from very early versions of Fennec, speak now!

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 8485869 [details] [diff] [review]
Kill COMBINED_WITH_IMAGES

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

I think this looks fine, but everyone would probably feel a lot better if we had some tests of these migration paths. I'm going to try and write some today. Until I've ruled that out, I'm going to wait on an r+.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 8485869 [details] [diff] [review]
Kill COMBINED_WITH_IMAGES

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

Sorry for the delay on these. I wrote some super dumb migration tests in bug 1066892 that pass (locally) with and without these, so I'm fine with them. The tests themselves will bitrot you though, and are causing some try headaches, so I'm gonna try to move ahead here while I work that out. They also don't test a TON of in between states. Just make sure we don't crash trying to do an upgrade step.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ -1150,5 @@
>      }
>  
>      private void upgradeDatabaseFrom11to12(SQLiteDatabase db) {
> -        debug("Dropping view: " + Obsolete.VIEW_COMBINED_WITH_IMAGES);
> -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_COMBINED_WITH_IMAGES);

I wonder if we should keep this final drop in? If you're on v 11 and upgrade to 21, it would be nice to kill the table. I wouldn't mind hardcoding the view name here though.

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 8485886 [details] [diff] [review]
Eliminate redundant edtions of Combined view

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

I think it might be nice to add some short historical notes in the file. i.e. something like:

// We originally created this combined view in v12, but removed that code in bug 947018.

I'm not sure if that's useful or not though. I guess if you've got a v13 database with the combined view in it, it might help understand that mystery.

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 8488406 [details] [diff] [review]
Remove HISTORY_WITH_IMAGES view

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ -620,5 @@
>          db.execSQL("ALTER TABLE " + TABLE_HISTORY +
>                     " RENAME TO " + TABLE_HISTORY_TMP);
>  
>          debug("Dropping views and indexes related to " + TABLE_HISTORY);
> -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_HISTORY_WITH_IMAGES);

Again, do we want to keep this in in case you have an old database that you're upgrading?

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 8488407 [details] [diff] [review]
Remove BOOKMARKS_WITH_IMAGES view

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ -581,5 @@
>          db.execSQL("ALTER TABLE " + TABLE_BOOKMARKS +
>                     " RENAME TO " + TABLE_BOOKMARKS_TMP);
>  
>          debug("Dropping views and indexes related to " + TABLE_BOOKMARKS);
> -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_BOOKMARKS_WITH_IMAGES);

Same comment as others.

Comment 27

[Chris Kitching [:ckitching]]

(In reply to Wesley Johnston (:wesj) from comment #23)
> Comment on attachment 8485869 [details] [diff] [review]
> Kill COMBINED_WITH_IMAGES
> 
> Review of attachment 8485869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay on these. I wrote some super dumb migration tests in bug
> 1066892 that pass (locally) with and without these, so I'm fine with them.
> The tests themselves will bitrot you though, and are causing some try
> headaches, so I'm gonna try to move ahead here while I work that out. They
> also don't test a TON of in between states. Just make sure we don't crash
> trying to do an upgrade step.
> 
> ::: mobile/android/base/db/BrowserDatabaseHelper.java
> @@ -1150,5 @@
> >      }
> >  
> >      private void upgradeDatabaseFrom11to12(SQLiteDatabase db) {
> > -        debug("Dropping view: " + Obsolete.VIEW_COMBINED_WITH_IMAGES);
> > -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_COMBINED_WITH_IMAGES);
> 
> I wonder if we should keep this final drop in? If you're on v 11 and upgrade
> to 21, it would be nice to kill the table. I wouldn't mind hardcoding the
> view name here though.

Inlining the obsolete constants (the table names etc.) and deleting them from the source seems like a good idea.

It's a little hard to tell looking just at the patch (because I didn't touch it) but I left in the *latest* drop of VIEW_COMBINED_WITH_IMAGES.

The next thing upgradeDatabaseFrom11to12 does is calls createCombinedViewOn12, which creates VIEW_COMBINED_WITH_IMAGES again.
The very final drop of VIEW_COMBINED_WITH_IMAGES is here, in 12->13:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#1268

Throughout, I aimed to delete all the "create-then-drop" steps, and keep only the latest "drop-and-forget" step in the upgrade path.

(In reply to Wesley Johnston (:wesj) from comment #25)
> ::: mobile/android/base/db/BrowserDatabaseHelper.java
> @@ -620,5 @@
> >          db.execSQL("ALTER TABLE " + TABLE_HISTORY +
> >                     " RENAME TO " + TABLE_HISTORY_TMP);
> >  
> >          debug("Dropping views and indexes related to " + TABLE_HISTORY);
> > -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_HISTORY_WITH_IMAGES);
> 
> Again, do we want to keep this in in case you have an old database that
> you're upgrading?

Of course, but, again, we're dropping it later on:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#1267


(In reply to Wesley Johnston (:wesj) from comment #26)
> Comment on attachment 8488407 [details] [diff] [review]
> Remove BOOKMARKS_WITH_IMAGES view
> 
> Review of attachment 8488407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserDatabaseHelper.java
> @@ -581,5 @@
> >          db.execSQL("ALTER TABLE " + TABLE_BOOKMARKS +
> >                     " RENAME TO " + TABLE_BOOKMARKS_TMP);
> >  
> >          debug("Dropping views and indexes related to " + TABLE_BOOKMARKS);
> > -        db.execSQL("DROP VIEW IF EXISTS " + Obsolete.VIEW_BOOKMARKS_WITH_IMAGES);
> 
> Same comment as others.

Likewise:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#1266

Comment 28

[Chris Kitching [:ckitching]]

Attachment: Drop support for migrating images from very old databases

rnewman suggested this earlier: We don't care about images from this era, so drop all the code that tries to rescue them. This also lets us finally kill off BrowserContract.Obsolete \o/

Comment 29

[Chris Kitching [:ckitching]]

(Funfact: We're up to 40%)

Comment 30

[Chris Kitching [:ckitching]]

Attachment: Only define the history table once

We had two methods that create the history table. The only difference in the table they built was one had the favicon_id field, the other did not. Since that field can safely be null, there is no harm in making the migration step that previously used the old (favicon_id-less) version go straight to the new one - the rows it inserts will just get nulls for that field (which is what happened anyway, but now we need way less code to do it)

Comment 31

[Chris Kitching [:ckitching]]

Attachment: Only define the bookmarks table once

Same idea, but now for the bookmarks table.

Comment 32

[Chris Kitching [:ckitching]]

Attachment: Replace qualifyColumn function delegate with a static import

It's slightly shorter.

Comment 33

[Chris Kitching [:ckitching]]

Attachment: Replace qualifyColumn function delegate with a static import

Now with less junk

Comment 34

[Chris Kitching [:ckitching]]

Attachment: Only define the history table once

Don't attempt to add a column twice.

Comment 35

[Chris Kitching [:ckitching]]

Attachment: Only define the bookmarks table once

Don't attempt to add a column twice.

Comment 36

[Chris Kitching [:ckitching]]

Attachment: Eliminate redundant edtions of Combined view

Missed a spot: This patch incorrectly deleted the combined_with_favicons views. I folded a patch that I shouldn't have into this one. While that view needs to die, there's still some weird code left that uses it. See: Bug 926234

Comment 37

[Chris Kitching [:ckitching]]

Attachment: Only define the history table once

Comment 38

[Chris Kitching [:ckitching]]

Attachment: Only define the bookmarks table once

Comment 39

[Wesley Johnston (:wesj)]

Comment on attachment 8491152 [details] [diff] [review]
Drop support for migrating images from very old databases

I'm busy enough, I'm just going to flip these to rnewman.

Comment 40

[Richard Newman [:rnewman]]

Comment on attachment 8491152 [details] [diff] [review]
Drop support for migrating images from very old databases

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +644,5 @@
>  
>      private void upgradeDatabaseFrom12to13(SQLiteDatabase db) {
> +        // Delete the favicons database.
> +        if (mContext.getDatabasePath("favicon_urls.db").exists()) {
> +            mContext.deleteDatabase("favicon_urls.db");

I suggest *not* doing this here, and leaving it in the "do last" block that you deleted (though probably not throwing an exception on failure!).

It's harmless to leave it until last, and provides a little resilience on upgrade failure.

Comment 41

[Richard Newman [:rnewman]]

Comment on attachment 8491931 [details] [diff] [review]
Only define the history table once

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +642,5 @@
> +        // steps have been performed in this operation). In which case these queries will throw,
> +        // but we don't care.
> +        try {
> +            db.execSQL("ALTER TABLE " + TABLE_HISTORY
> +                     + " ADD COLUMN " + History.FAVICON_ID + " INTEGER");

+ at end of line, not start.

Comment 42

[Richard Newman [:rnewman]]

Comment on attachment 8491932 [details] [diff] [review]
Only define the bookmarks table once

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

Overall comment: make sure that the migration tests that Wes wrote in Bug 1066892 (not yet landed... feel free to reflect my review comments and land it!) pass before landing this.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +619,5 @@
> +
> +
> +        try {
> +            db.execSQL("ALTER TABLE " + TABLE_BOOKMARKS
> +                     + " ADD COLUMN " + Bookmarks.FAVICON_ID + " INTEGER");

+ at end.

Comment 43

[Chris Kitching [:ckitching]]

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

Comment 44

[Chris Kitching [:ckitching]]

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

Comment 45

[Chris Kitching [:ckitching]]

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

Comment 46

[Chris Kitching [:ckitching]]

Attachment: Kill obsolete test code

Someone landed some shiny new tests (JUnit, FINALLY, YAY). I now need to delete the code that tests that the whole readinglist-in-bookmarks thing works.

Comment 47

[Richard Newman [:rnewman]]

Comment on attachment 8500065 [details] [diff] [review]
Kill obsolete test code

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

Pull request to a-s, please!

Comment 48

[Richard Newman [:rnewman]]

By the way: one could make the argument that we should wait to land this until 36, in order to avoid a destabilizing change having a short bake time.

I am *not* making that argument, for one simple reason: our Nightly and Aurora testing audience isn't likely to hit any migrations. We have no new migration code in these patches, or indeed in 35 or 36.

Reaching beta or release in 36 versus 35 -- the first point at which we're likely to see users on older database versions in any quantity at all -- won't affect our ability to respond to any problems.

In short: additional pre-Beta bake time is of no value for this work, so holding it back has no value.

Indeed, we'd rather find problems in 35 (which has no DB upgrades) than 36 (which might).

Comment 49

[Chris Kitching [:ckitching]]

Sounds good. Last thing left in the way is fixing the tests in the other bug, then.
Charge!

Comment 50

[Chris Kitching [:ckitching]]

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

Comment 51

[Chris Kitching [:ckitching]]

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

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b99d1a16e387
https://hg.mozilla.org/mozilla-central/rev/5a4922acc653

Comment 53

[Richard Newman [:rnewman]]

Dammit, Chris, don't forget to upstream your code!

Comment 54

[Chris Kitching [:ckitching]]

https://github.com/mozilla-services/android-sync/pull/511