1250707 - Implement URL annotations table

Status: RESOLVED FIXED

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

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 2

[Richard Newman [:rnewman]]

(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.

Comment 3

[Richard Newman [:rnewman]]

> 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.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Create url annotations table. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/36481/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36481/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Add test for browser.db upgrades. r=sebastian

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 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8723318 [details]
MozReview Request: Bug 1250707 - Create url annotations table. r=sebastian

Richard, what do you think of the table schema?

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Register url annotations table with BrowserProvider; add query. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/36751/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36751/

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Add insert method to url annotations table in BrowserProvider. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/36753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36753/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Test insert to url annotations table through BrowserProvider. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/36755/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36755/

Comment 12

[Richard Newman [: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`.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Add UrlAnnotations with insertion. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/36789/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36789/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - Add test for UrlAnnotations.insert. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/36791/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36791/

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Still TODO:
* Delete method (by url? by key?)
* Update method (by url? by key?)
* Richard's comments

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8723749 [details]
MozReview Request: Bug 1250707 - Add test for browser.db upgrades. r=sebastian

https://reviewboard.mozilla.org/r/36699/#review33397

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

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 21

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8723902 [details]
MozReview Request: Bug 1250707 - Add test for UrlAnnotations.insert. r=sebastian

https://reviewboard.mozilla.org/r/36791/#review33447

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1250707 - review: Url -> URL. r=me

Review commit: https://reviewboard.mozilla.org/r/36883/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36883/

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?).

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/ed84f2d29c0e729abd2930608853c84ebd15ce77
Bug 1250707 - Create url annotations table. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/6bf5e3d73e4a4083d751198b0a71576bb1fffbb3
Bug 1250707 - Add test for browser.db upgrades. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/9bba5814be611ea3155521718984bcf9ccaeb9bc
Bug 1250707 - Register url annotations table with BrowserProvider; add query. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/f55ec17dfbfafdf3b616cbf9968b835041ad3c62
Bug 1250707 - Add insert method to url annotations table in BrowserProvider. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/d8316642b8021f3bc1c054dbeaa2d9ca239bdd30
Bug 1250707 - Test insert to url annotations table through BrowserProvider. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/c57441126163dc9384ac1e2c6589e97e3a018c28
Bug 1250707 - Add UrlAnnotations with insertion. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/3693e541f0ee18223dd00a1745a4cd09416722a9
Bug 1250707 - Add test for UrlAnnotations.insert. r=sebastian

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed84f2d29c0e
https://hg.mozilla.org/mozilla-central/rev/6bf5e3d73e4a
https://hg.mozilla.org/mozilla-central/rev/9bba5814be61
https://hg.mozilla.org/mozilla-central/rev/f55ec17dfbfa
https://hg.mozilla.org/mozilla-central/rev/d8316642b802
https://hg.mozilla.org/mozilla-central/rev/c57441126163
https://hg.mozilla.org/mozilla-central/rev/3693e541f0ee