Closed Bug 1172077 Opened 5 years ago Closed 5 years ago

Merge tabs.db into browser.db

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: rnewman, Assigned: ahmedibrahimkhali, Mentored)

References

Details

Attachments

(1 file, 4 obsolete files)

As nalexander summarized:

08:54:40 < R4md4c> nalexander: Why did Fennec go with two separate DBs in the first place ? Why not just one DB and several tables ?
08:54:48 < nalexander> R4md4c: hysterical raisins.
08:55:00 < nalexander> R4md4c: at the time, we didn't understand CP's and the trade offs that well.
08:55:29 < nalexander> R4md4c: also, browser.db has a long history.  It originally was the *system* browser database, which doesn't hold tabs.
08:55:34 < nalexander> R4md4c: so a new DB was needed.  IIRC.
08:55:53 < nalexander> R4md4c: rnewman and I discussed this -- we know of no particular reason that a separate DB was chosen.
Attached patch tabs_provider_merging.patch (obsolete) — Splinter Review
I've wrote a patch that could make the tabs and clients tables to be written to the browser.db, instead of being written into a separate DB file.
Attachment #8616135 - Flags: review?(nalexander)
Attachment #8616135 - Flags: review?(mark.finkle)
Comment on attachment 8616135 [details] [diff] [review]
tabs_provider_merging.patch

In general this is looking OK. Is this patch enough to make everything work?
Attachment #8616135 - Flags: review?(mark.finkle) → feedback+
Comment on attachment 8616135 [details] [diff] [review]
tabs_provider_merging.patch

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

Ahmed: First, sorry for the slow (!) review.  Slow, even by my standards.

Second, this looks great.

Third, I kinda can't believe it Just Works, with essentially no change to TabsProvider.  The magic of the ContentResolver URI indirection and matching, I guess?  But in any case I'd like to see a JUnit 3 tests.  I realize now I never landed some of your older code!  (Bug 1159020)  Can you make sure that applies over top of this, and that the tests in that patch work with your changes?  That'd certainly convince me this is working.

Again, sorry for the slow review.
Attachment #8616135 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8616135 [details] [diff] [review]
> tabs_provider_merging.patch
> 
> Review of attachment 8616135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ahmed: First, sorry for the slow (!) review.  Slow, even by my standards.
> 
> Second, this looks great.
> 
> Third, I kinda can't believe it Just Works, with essentially no change to
> TabsProvider.  The magic of the ContentResolver URI indirection and
> matching, I guess?  But in any case I'd like to see a JUnit 3 tests.  I
> realize now I never landed some of your older code!  (Bug 1159020)  Can you
> make sure that applies over top of this, and that the tests in that patch
> work with your changes?  That'd certainly convince me this is working.
> 
> Again, sorry for the slow review.

Hi Nick, 

In the patch I've posted it is rebased on top your last review[1] that had the TestRemoteTabs.java and I've run your JUnit 3 test and the result was a success. So I guess the patch is working, but perhaps a try build would make us more certain ?


[1] https://reviewboard.mozilla.org/r/9595/
Flags: needinfo?(nalexander)
(In reply to Ahmed Khalil from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Comment on attachment 8616135 [details] [diff] [review]
> > tabs_provider_merging.patch
> > 
> > Review of attachment 8616135 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Ahmed: First, sorry for the slow (!) review.  Slow, even by my standards.
> > 
> > Second, this looks great.
> > 
> > Third, I kinda can't believe it Just Works, with essentially no change to
> > TabsProvider.  The magic of the ContentResolver URI indirection and
> > matching, I guess?  But in any case I'd like to see a JUnit 3 tests.  I
> > realize now I never landed some of your older code!  (Bug 1159020)  Can you
> > make sure that applies over top of this, and that the tests in that patch
> > work with your changes?  That'd certainly convince me this is working.
> > 
> > Again, sorry for the slow review.
> 
> Hi Nick, 
> 
> In the patch I've posted it is rebased on top your last review[1] that had
> the TestRemoteTabs.java and I've run your JUnit 3 test and the result was a
> success. So I guess the patch is working, but perhaps a try build would make
> us more certain ?

Yep, awesome.  I just landed the underlying work -- good job, BTW -- so I'd love to see a try build for this.  As for migration, we need to add an interface to reset some Sync timestamps, and then force a Sync during the DB migration.  This could be tricky.

Reading links doesn't show a great point for this.  I think we might want to implement Bug 815773 to do this.
Flags: needinfo?(nalexander)
Attached patch tabs_provider_merging.patch (obsolete) — Splinter Review
Hi Nick, I've added the migration that we've agreed on, where it reads all the records from "tabs.db" and insert them into "browser.db"

Let me know what you think about it.
Attachment #8616135 - Attachment is obsolete: true
Attachment #8622802 - Flags: review?(nalexander)
Attached patch tabs_provider_merging.patch (obsolete) — Splinter Review
Here is a more pruned version from the same patch as it contained modified lines that I've never changed and no idea why did Android Studio formatted these lines for me.
Attachment #8622802 - Attachment is obsolete: true
Attachment #8622802 - Flags: review?(nalexander)
Attachment #8622805 - Flags: review?(nalexander)
Are we sure we need to do migration? I think the application deletes all rows and inserts new records when writing the open tabs out to the DB.

Are we worried about a Sync happening before the application has a chance to push data into the new DB table?
I think it's easier to do a migration than it is to tell Sync to start over. See Comment 5.
(In reply to Richard Newman [:rnewman] from comment #9)
> I think it's easier to do a migration than it is to tell Sync to start over.
> See Comment 5.

I have seen Fennec crash heroically during migrations. Removing the complexity might be a win, and stamping out complexity is my mission.
Comment on attachment 8622805 [details] [diff] [review]
tabs_provider_merging.patch

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

This is a good approach.  Let's do two things:

1) Extract a DatabaseUtils.copyTable or something, to make the internals available to others.

2) Make the migration function just call a general function that takes the DB file as a parameter.  I want to be able to test easier.

So migrate -> copy-and-delete-function -> {call two copy helper functions}.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +939,5 @@
> +    private void upgradeDatabaseFrom23to24(SQLiteDatabase db) {
> +        createTabsTable(db);
> +        createClientsTable(db);
> +
> +        Cursor tabsCursor = null, clientsCursor = null;

nit: separate lines, please.

@@ +959,5 @@
> +
> +            ContentValues contentValues = new ContentValues();
> +            while (tabsCursor.moveToNext()) {
> +                contentValues.clear();
> +                DatabaseUtils.cursorRowToContentValues(tabsCursor, contentValues);

nifty.  Are you confident that there's no "bleed", i.e. that old values are removed (even when there's a NULL in the table?)

@@ +966,5 @@
> +
> +            while (clientsCursor.moveToNext()) {
> +                contentValues.clear();
> +                DatabaseUtils.cursorRowToContentValues(clientsCursor, contentValues);
> +                db.insertOrThrow(TabsProvider.TABLE_CLIENTS, null, contentValues);

Do we care about throwing?  It might be best to ignore a bad row.

@@ +970,5 @@
> +                db.insertOrThrow(TabsProvider.TABLE_CLIENTS, null, contentValues);
> +            }
> +
> +            // If no exception has occurred till now, we delete the old tabs.db file.
> +            FileUtils.delete(oldTabsDBFile);

We never want this to live after we /try/ to migrate.

@@ +975,5 @@
> +
> +            // We are done, commit!
> +            db.setTransactionSuccessful();
> +        } catch (SQLException e) {
> +            debug("Exception occurred while trying to copy from tabs.db to browser.db: " + e);

Log.e, for sure.  Might as well know.

@@ +977,5 @@
> +            db.setTransactionSuccessful();
> +        } catch (SQLException e) {
> +            debug("Exception occurred while trying to copy from tabs.db to browser.db: " + e);
> +        } catch (IOException e) {
> +            debug("Exception happened while trying to delete tabs.db file: " + e);

ditto, Log.e.

@@ +986,5 @@
> +            if (clientsCursor != null) {
> +                clientsCursor.close();
> +            }
> +            if (oldTabsDB != null) {
> +                oldTabsDB.close();

Presumably you need to delete the file *after* you close.
Attachment #8622805 - Flags: review?(nalexander) → feedback+
Attached patch tabs_provider_merging.patch (obsolete) — Splinter Review
Separated the migrate method into a separate function so that testing could be easy, also added a utility method to copy tables from a SQLiteDatabase to another.
Attachment #8622805 - Attachment is obsolete: true
Attachment #8623367 - Flags: review?(nalexander)
Sorry for the amount of white spaces changes that happens without me knowing, it won't happen again as I know now how to resolve them.
Attachment #8623367 - Attachment is obsolete: true
Attachment #8623367 - Flags: review?(nalexander)
Attachment #8623392 - Flags: review?(nalexander)
Comment on attachment 8623392 [details] [diff] [review]
tabs_provider_merging.patch

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

I made some small changes to this, tested locally quite a bit, and pushed a try build.  Let's see how that goes, and then we're ready to land!

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +197,5 @@
> +                " ON " + TABLE_CLIENTS + "(" + BrowserContract.Clients.GUID + ")");
> +    }
> +
> +    private void createTabsTable(SQLiteDatabase db) {
> +        debug("Creating tabs.db: " + db.getPath());

This is no longer accurate.
Attachment #8623392 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/6b80a1818bf1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.