Closed Bug 1250707 Opened 4 years ago Closed 4 years ago

Implement URL annotations table

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 3 open bugs)

Details

Attachments

(7 files, 1 obsolete file)

58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Richard proposed adding a url metadata table that is more or less a key-value store associated with a given url. The proposed schema:

URL  type/key  value  time_modified  sync_status

For a screenshot:
url = http://...
type = screenshot
value = file:// (or the content uri)
...

Notes:
  * antlam additionally wants a time_created column for bug 1243558.
  * The url should have no foreign key constraint because we can have metadata associated with with a url that we have not visited (or visited but cleared the history for)
  * The table should probably be sorted by url & there should be an index on the url (if not a dual index on (url, type)
  * The sync_status field will be unused at landing time and is added now so we don't have to spend time planning migrations later
  * type can be a tinyint on disk but (for future-reference) should be translated before being sent over the wire (so other implementations can use different values for their representations, e.g. iOS adds pdf support & we add shared-data support, both using the same key. This could be avoided if we shared a common file, but we don't)
  * It's unclear how a user might clear this table (e.g. clear private data) – we should ask UX
Richard suggested that this table can also be used for Sebastian's content notification bug 1234314 & ahunt's combined bookmark/reader list view bug 1238087.
(In reply to Michael Comella (:mcomella) from comment #0)

>   * type can be a tinyint on disk but (for future-reference) should be
> translated before being sent over the wire 

I propose just using a string here. The overhead from storing repeated URLs dwarfs this cost.
Component: General → Data Providers
> The overhead from storing repeated URLs dwarfs this cost.

And to briefly explain this:

We store URLs throughout browser.db as keys in history, bookmarks, favicons, and will again do so here.

That's wasteful: those URLs are repeated, they're internally repeated in each index, and joining between these tables on strings isn't as efficient as doing so on an integer. The ideal is that URL strings are mentioned only once in the DB.

At some point in the future we'll want to alter our database schema to shift closer to the Places design: a 'places' table maps URLs to numeric IDs, and every URL mentioned in the DB gets a place, so we can use that ID wherever we would have used a URL.

We're nearly there, particularly after Bug 1046709, but we'd need to stop treating the history table as something we can just delete from arbitrarily.

Don't consider making this change now, but when you're analyzing performance, be aware of the causes and this future remedy.
Blocks: 1251010
No longer depends on: 1251010
This commit writes the sql to create the table (untested). Still TODO:
* Write the automated tests for database version migration (in-progress, will test ^)
* Add methods to access the database (e.g. insert, update, delete) via the ContentProvider
Comment on attachment 8723318 [details]
MozReview Request: Bug 1250707 - Create url annotations table. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36481/diff/1-2/
Currently tests version 27 to 28 and is abstract enough to test future
upgrades. Additionally, the test should fail if the database is upgraded but a
database for the old version is not supplied.

Review commit: https://reviewboard.mozilla.org/r/36699/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36699/
Comment on attachment 8723318 [details]
MozReview Request: Bug 1250707 - Create url annotations table. r=sebastian

Richard, what do you think of the table schema?
Attachment #8723318 - Flags: feedback?(rnewman)
https://reviewboard.mozilla.org/r/36481/#review33315

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:362
(Diff revision 2)
> +        createUrlAnnotationsTable(db);

URL

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:470
(Diff revision 2)
> +                UrlAnnotations.SYNC_STATUS + " TEXT NOT NULL DEFAULT 'NEW' " +

Make an enum for SyncStatus, make this `"TINYINT NOT NULL DEFAULT " + SyncStatus.New`.
Attachment #8723318 - Flags: feedback?(rnewman) → review?(s.kaspari)
Still TODO:
* Delete method (by url? by key?)
* Update method (by url? by key?)
* Richard's comments
Attachment #8723318 - Flags: review?(s.kaspari) → review+
Comment on attachment 8723318 [details]
MozReview Request: Bug 1250707 - Create url annotations table. r=sebastian

https://reviewboard.mozilla.org/r/36481/#review33395

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1140
(Diff revision 2)
>                  case 26:
>                      upgradeDatabaseFrom25to26(db);
>                      break;
> +
> +                case 28:
> +                    upgradeDatabaseFrom27to28(db);
> +                    break;

At first I was confused that 27 is missing but then I saw that tables can have their own upgrade code.
Comment on attachment 8723749 [details]
MozReview Request: Bug 1250707 - Add test for browser.db upgrades. r=sebastian

https://reviewboard.mozilla.org/r/36699/#review33397
Attachment #8723749 - Flags: review?(s.kaspari) → review+
Comment on attachment 8723830 [details]
MozReview Request: Bug 1250707 - Register url annotations table with BrowserProvider; add query. r=sebastian

https://reviewboard.mozilla.org/r/36751/#review33399
Attachment #8723830 - Flags: review?(s.kaspari) → review+
Comment on attachment 8723831 [details]
MozReview Request: Bug 1250707 - Add insert method to url annotations table in BrowserProvider. r=sebastian

https://reviewboard.mozilla.org/r/36753/#review33439
Attachment #8723831 - Flags: review?(s.kaspari) → review+
Attachment #8723832 - Flags: review?(s.kaspari) → review+
Comment on attachment 8723832 [details]
MozReview Request: Bug 1250707 - Test insert to url annotations table through BrowserProvider. r=sebastian

https://reviewboard.mozilla.org/r/36755/#review33441
Comment on attachment 8723901 [details]
MozReview Request: Bug 1250707 - Add UrlAnnotations with insertion. r=sebastian

https://reviewboard.mozilla.org/r/36789/#review33445

::: mobile/android/base/java/org/mozilla/gecko/db/UrlAnnotations.java:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Some files seem to start with (reviewboard scambles the line):
/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */

and others don't (e.g. BrowserDB). Do we have a guideline here? What editors actually read this configuration?
Attachment #8723901 - Flags: review?(s.kaspari) → review+
Attachment #8723902 - Flags: review?(s.kaspari) → review+
Comment on attachment 8723902 [details]
MozReview Request: Bug 1250707 - Add test for UrlAnnotations.insert. r=sebastian

https://reviewboard.mozilla.org/r/36791/#review33447
Comment on attachment 8723318 [details]
MozReview Request: Bug 1250707 - Create url annotations table. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36481/diff/2-3/
Comment on attachment 8723749 [details]
MozReview Request: Bug 1250707 - Add test for browser.db upgrades. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36699/diff/1-2/
Comment on attachment 8723830 [details]
MozReview Request: Bug 1250707 - Register url annotations table with BrowserProvider; add query. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36751/diff/1-2/
Comment on attachment 8723831 [details]
MozReview Request: Bug 1250707 - Add insert method to url annotations table in BrowserProvider. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36753/diff/1-2/
Comment on attachment 8723832 [details]
MozReview Request: Bug 1250707 - Test insert to url annotations table through BrowserProvider. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36755/diff/1-2/
Comment on attachment 8723901 [details]
MozReview Request: Bug 1250707 - Add UrlAnnotations with insertion. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36789/diff/1-2/
Comment on attachment 8723902 [details]
MozReview Request: Bug 1250707 - Add test for UrlAnnotations.insert. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36791/diff/1-2/
https://reviewboard.mozilla.org/r/36789/#review33445

> Some files seem to start with (reviewboard scambles the line):
> /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */
> 
> and others don't (e.g. BrowserDB). Do we have a guideline here? What editors actually read this configuration?

Since the primary developers all use IDEs, the additional header is probably not necessary anymore. That being said, I just copy whichever header is closest. I'll aim to remove those lines from now on though.
https://reviewboard.mozilla.org/r/36481/#review33315

> URL

This rename is causing me problems and taking a lot of time to fix so I'm going to drop the change. I don't find the difference in capitalization particularly important (as opposed to a semantic rename) but if you feel it's important enough, file a follow-up (mentorable?).
Attachment #8724135 - Attachment is obsolete: true
Blocks: 1243558
No longer depends on: 1243558
Summary: Implement URL metadata table → Implement URL annotations table
Blocks: 1270209
You need to log in before you can comment on or make changes to this bug.