1025128 - Regularize TabsProvider's databases

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks a few small changes that would make TabsProvider much more "Android standard".

1) At the moment, the tabs and clients tables are JOINed when querying just tabs.  This makes sense for the monolithic tabs tree UI shown in the Remote Tabs tray, but isn't the standard way of doing this.  It particularly doesn't support the split view yuan proposes in Bug 1014994.  If we want to keep the monolithic query, we should make it its own view; but we shouldn't keep it at all.

In addition, it's not possible to delete a single client by guid because there's a column mismatch in the JOIN.

2) At the moment, the clients table uses "rowid" instead of "_id".  That's not good for using the standard Android UI views, which assume column "_id".

Comment 1

[Nick Alexander :nalexander [he/him]]

rnewman: can you greenlight these two changes before I spend time implementing them properly?

Comment 2

[Richard Newman [:rnewman]]

Machete approved.

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1025128: Regularize Tabs table r?rnewman,nalexander

Bug 1025128: Regularize Tabs table r?rnewman,nalexander

* Added for foreign key reference for tabs table
* Bumped database version to 25
* Fixed non-local client deletion
* Fixed rowid to _id for clients in TabsProvider

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1216100 : Launch sync settings from sync welcome page. r?nalexander

Bug 1216100 : Launch sync settings from sync welcome page. r?nalexander

Comment 6

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/28bd6fb286b4123945ff571d0371e078148a4206
Bug 1025128 - Part 1: Regularize tabs and clients tables. r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/ebc2cde9c8a1974091c95c10cf671fbe095c4791
Bug 1025128 - Part 2: Fix JUnit 3 tests. r=me,vivek

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679540 [details]
MozReview Request: Bug 1025128: Regularize Tabs table r?rnewman,nalexander

https://reviewboard.mozilla.org/r/23467/#review20913

vivek nad I colloborated on an updated version.  I think I missed these nits, though :(  Follow-up mentor bug, vivek?

::: mobile/android/base/db/BrowserDatabaseHelper.java:1032
(Diff revision 1)
> +        didCreateTabsTable =true;

nit: space.

::: mobile/android/base/db/TabsProvider.java:171
(Diff revision 1)
>                  // Delete from both TABLE_TABS and TABLE_CLIENTS.

nit: remove comment.

::: mobile/android/base/sync/repositories/android/FennecTabsRepository.java:373
(Diff revision 1)
>        Logger.info(LOG_TAG, "Clearing all non-local tabs for default profile.");

nit: "non-local tabs" => "non-local clients and tabs".

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28bd6fb286b4
https://hg.mozilla.org/mozilla-central/rev/ebc2cde9c8a1

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8679540 [details]
MozReview Request: Bug 1025128: Regularize Tabs table r?rnewman,nalexander

https://reviewboard.mozilla.org/r/23467/#review21035

::: mobile/android/base/db/BrowserDatabaseHelper.java:200
(Diff revision 1)
> -    private void createTabsTable(SQLiteDatabase db) {
> +    private boolean didCreateTabsTable = false;

I knew I was going to regret letting this pattern exist in the codebase :)

::: mobile/android/base/db/BrowserDatabaseHelper.java:1001
(Diff revision 1)
> +        if (didCreateTabsTable) {

I don't believe this is necessary.

You'll be in one of three situations.

* We're at DB version 25. Do nothing.
* We're at DB 24 upgrading to 25. Migrate and add the foreign key constraint.
* We have no DB. Create it at 25 with the foreign key constraint.

We should never call this upgrade method without needing to do this work, right?

This looks fine, but I don't know why the control flow needs to be this complicated. Can you kill that new boolean?

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679551 [details]
MozReview Request: Bug 1216100 : Launch sync settings from sync welcome page. r?nalexander

vivek and I landed this work in the other ticket.