Closed Bug 1025128 Opened 5 years ago Closed 4 years ago

Regularize TabsProvider's databases

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: nalexander, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

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".
rnewman: can you greenlight these two changes before I spend time implementing them properly?
Flags: needinfo?(rnewman)
Blocks: 1025131
Machete approved.
Flags: needinfo?(rnewman)
Duplicate of this bug: 1217163
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
Attachment #8679540 - Flags: review?(rnewman)
Attachment #8679540 - Flags: review?(nalexander)
Bug 1216100 : Launch sync settings from sync welcome page. r?nalexander
Attachment #8679551 - Flags: review?(nalexander)
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".
Attachment #8679540 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/28bd6fb286b4
https://hg.mozilla.org/mozilla-central/rev/ebc2cde9c8a1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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?
Attachment #8679540 - Flags: review?(rnewman)
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.
Attachment #8679551 - Flags: review?(nalexander)
You need to log in before you can comment on or make changes to this bug.