Last Comment Bug 675996 - Extend moz_favicons with GUID column to support Sync
: Extend moz_favicons with GUID column to support Sync
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Firefox 11
Assigned To: Richard Newman [:rnewman]
:
:
Mentors:
Depends on: 674238 688279 715268
Blocks: 428378 706347
  Show dependency treegraph
 
Reported: 2011-08-02 10:23 PDT by Richard Newman [:rnewman]
Modified: 2012-01-04 12:10 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Sketch implementation for feedback purposes. v1 (11.21 KB, patch)
2011-09-02 22:30 PDT, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Sketch implementation of test modifications. v1 (3.70 KB, patch)
2011-09-02 22:32 PDT, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 1: implementation. v1 (12.62 KB, patch)
2011-09-21 17:08 PDT, Richard Newman [:rnewman]
mak77: feedback+
Details | Diff | Splinter Review
Part 2: migration test changes. v1 (4.92 KB, patch)
2011-09-21 17:09 PDT, Richard Newman [:rnewman]
mak77: feedback+
Details | Diff | Splinter Review
Part 3: tests for GUID generation on insertion. v1 (4.67 KB, patch)
2011-09-21 17:11 PDT, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 1: implementation. v2 (14.48 KB, patch)
2011-09-23 16:12 PDT, Richard Newman [:rnewman]
mak77: feedback+
Details | Diff | Splinter Review
Part 2: tests and migration test changes. v2 (10.82 KB, patch)
2011-09-23 16:13 PDT, Richard Newman [:rnewman]
mak77: feedback+
Details | Diff | Splinter Review
Part 1: guids. Unbitrotted. v3 (17.54 KB, patch)
2011-11-28 13:00 PST, Richard Newman [:rnewman]
mak77: review-
Details | Diff | Splinter Review
Part 2: tests. v3 (6.13 KB, patch)
2011-11-28 13:01 PST, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 1: guids. v4 (19.44 KB, patch)
2011-11-29 19:24 PST, Richard Newman [:rnewman]
mak77: review-
Details | Diff | Splinter Review
Part 2: tests. v4 (8.13 KB, patch)
2011-11-29 19:26 PST, Richard Newman [:rnewman]
mak77: review+
Details | Diff | Splinter Review
Part 1. guids. v5 (12.49 KB, patch)
2011-12-01 13:56 PST, Richard Newman [:rnewman]
mak77: review+
Details | Diff | Splinter Review
Part 2. tests. v5 (6.65 KB, patch)
2011-12-01 13:56 PST, Richard Newman [:rnewman]
mak77: review+
Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2011-08-02 10:23:31 PDT
Follow-up from Bug 674238, to actually cover doing the work :D
Comment 1 Richard Newman [:rnewman] 2011-09-02 22:30:41 PDT
Created attachment 558036 [details] [diff] [review]
Sketch implementation for feedback purposes. v1

This is a first draft of what I believe to be necessary to support GUIDs for favicons. In particular:

* Extending the table definition in nsPlacesTables.h
* Defining a statement to create an index on favicons.guid in nsPlacesIndexes.h
* Defining a migration path to v12, which adds the column and index
* Extended mDB{Insert,Update}Icon to coalesce/accept/preserve/generate GUIDs as appropriate.

I also fixed a couple of typos.

The next attachment will include modifications to a couple of migration tests. I definitely want some feedback on that decision.

Note that I'm only asking for feedback here; I haven't yet determined whether lastModified should be added or not (or whether it should replace expiration), and that should occur at the same time as this schema change.
Comment 2 Richard Newman [:rnewman] 2011-09-02 22:32:12 PDT
Created attachment 558037 [details] [diff] [review]
Sketch implementation of test modifications. v1

These tests don't seem to have a way to override the default schema version, and so their expectation that the DB will be upgraded to v11 is now incorrect.

This patch renames and adjusts the tests accordingly.
Comment 3 Marco Bonardo [::mak] 2011-09-21 06:33:50 PDT
Comment on attachment 558036 [details] [diff] [review]
Sketch implementation for feedback purposes. v1

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

This is missing changes to some insert queries, like the insert or replace in AsyncFaviconHelpers.
No code is currently using the new queries, but I assume you wanted to do that in a separate patch.

::: toolkit/components/places/nsFaviconService.cpp
@@ +196,1 @@
>      "WHERE id = :icon_id"));

Ideally we should never hit this query on a line without a guid, so why do we need the GENERATE_GUID()? Either it has a guid we want to retain or it has a guid we want to overwrite.

::: toolkit/components/places/nsNavHistory.cpp
@@ +890,1 @@
>        // Schema Upgrades must add migration code here.

add newline here between these comments. I assume the todo is a reminder to set the right FX version.

@@ +1744,5 @@
> +  nsCOMPtr<mozIStorageStatement> hasGuidStatement;
> +  nsresult rv = aDBConn->CreateStatement(NS_LITERAL_CSTRING(
> +      "SELECT guid FROM moz_favicons"
> +    ),
> +    getter_AddRefs(hasGuidStatement));

per Places code style coherence
...CreateStatement(NS_LITERAL_CSTRING(
  "QUERY"
), getter_AddRefs(...
Comment 4 Marco Bonardo [::mak] 2011-09-21 06:39:24 PDT
Comment on attachment 558037 [details] [diff] [review]
Sketch implementation of test modifications. v1

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

I'd love a test that there is a guid column and that inserting a favicon synchronously or asynchronously fills it up

::: toolkit/components/places/tests/migration/test_v11_from_v10.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  /**
>   * This file tests migration invariants from schema version 10 to schema version
> + * 12.

"to current schema version."

@@ +295,5 @@
>  
>    do_check_true(db.indexExists("moz_bookmarks_guid_uniqueindex"));
>    do_check_true(db.indexExists("moz_places_guid_uniqueindex"));
>  
> +  do_check_eq(db.schemaVersion, 12);

we should define current schema version in a const in head_common.js and use that one here.

::: toolkit/components/places/tests/migration/test_v11_from_v10_migrated_from_v11.js
@@ +3,5 @@
>  
>  /**
>   * This file tests migration invariants from a database with schema version 11
>   * that was then downgraded to a database with a schema version 10.  Places
> + * should then migrate this database to one with a schema version of 12.

here as well replace with "current schema version"

@@ +108,5 @@
>    let dbFile = gProfD.clone();
>    dbFile.append(kDBName);
>    let db = Services.storage.openUnsharedDatabase(dbFile);
>  
> +  do_check_eq(db.schemaVersion, 12);

we should define current schema version in a const in head_common.js and use that one here.

::: toolkit/components/places/tests/migration/xpcshell.ini
@@ +2,5 @@
>  head = head_migration.js
>  tail = 
>  
> +[test_v12_from_v10.js]
> +[test_v12_from_v10_migrated_from_v11.js]

As things stand we will have to edit these names each time. bad initial choice, what about renaming to test_current_from_v10.js and test_current_from_v10_downgraded_from_current.js (practically s/v11/current/)
Comment 5 Richard Newman [:rnewman] 2011-09-21 11:42:50 PDT
(In reply to Marco Bonardo [:mak] from comment #3)

> This is missing changes to some insert queries, like the insert or replace
> in AsyncFaviconHelpers.

Will fix, ta.

> No code is currently using the new queries, but I assume you wanted to do
> that in a separate patch.

Multiple levels of patches. Sync will start adding GUIDs directly, before we spend a bunch of time implementing a more complete async favicons API.

> Ideally we should never hit this query on a line without a guid, so why do
> we need the GENERATE_GUID()? Either it has a guid we want to retain or it
> has a guid we want to overwrite.

I think you're right.

> add newline here between these comments. I assume the todo is a reminder to
> set the right FX version.

Correct. Don't know when this will land!
Comment 6 Richard Newman [:rnewman] 2011-09-21 12:52:32 PDT
(In reply to Marco Bonardo [:mak] from comment #4)

> > +[test_v12_from_v10.js]
> > +[test_v12_from_v10_migrated_from_v11.js]
> 
> As things stand we will have to edit these names each time. bad initial
> choice, what about renaming to test_current_from_v10.js and
> test_current_from_v10_downgraded_from_current.js (practically s/v11/current/)

The trailing "v11" should remain: the test loads a sqlite file that was created by downgrading a v11 DB into a v10, so "test_current_from_v10_migrated_from_v11" is accurate.

Otherwise, done. Working on additional tests now.
Comment 7 Richard Newman [:rnewman] 2011-09-21 17:08:34 PDT
Created attachment 561617 [details] [diff] [review]
Part 1: implementation. v1

First part. (Requires test modifications in order for tests to pass.)
Comment 8 Richard Newman [:rnewman] 2011-09-21 17:09:05 PDT
Created attachment 561618 [details] [diff] [review]
Part 2: migration test changes. v1
Comment 9 Richard Newman [:rnewman] 2011-09-21 17:11:34 PDT
Created attachment 561620 [details] [diff] [review]
Part 3: tests for GUID generation on insertion. v1

This very much depends on the patch for the test cleanup bug I submitted today.

Note that there's one thing that has stumped me: the only "add a favicon" method that's async requires a fetch from the URI, and I simply don't get the callback (thanks to nsIFaviconDataCallback's "no callback on failure" behavior) if I use a file: URI.

This patch uses mozilla.org's favicon, which is not good. Any suggestions? I don't really want to fire up an HTTP server for this.
Comment 10 Marco Bonardo [::mak] 2011-09-22 15:35:58 PDT
(In reply to Richard Newman [:rnewman] from comment #9)
> Note that there's one thing that has stumped me: the only "add a favicon"
> method that's async requires a fetch from the URI, and I simply don't get
> the callback (thanks to nsIFaviconDataCallback's "no callback on failure"
> behavior) if I use a file: URI.

hm strange, so you don't get anything passing in a NetUtil.newURI(do_get_file("icon"))? newURI should automatically invoke newFileURI for local files. Will check this, if it doesn't work should be fixed.
Comment 11 Marco Bonardo [::mak] 2011-09-22 16:13:20 PDT
Comment on attachment 561617 [details] [diff] [review]
Part 1: implementation. v1

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

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +761,3 @@
>          "VALUES ((SELECT id FROM moz_favicons WHERE url = :icon_url), "
> +                ":icon_url, :data, :mime_type, :expiration, "
> +                "COALESCE(:guid, GENERATE_GUID())) "

In this case having :guid, guid, GENERATE_GUID() may be more correct... we don't want to override an existing guid, but I'm not sure if it works so easily in INSERT OR REPLACE. We may have to pay much attention about that (that means an aborting assertion will have to appear somewhere). For now if the multi coalesce doesn't work a nice warning comment above the query will be useful till code is implemented.

::: toolkit/components/places/nsFaviconService.cpp
@@ +229,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  // Selecting by GUID needs to be fast.
> +  rv = aDBConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID);

no error check! what a pain!

::: toolkit/components/places/nsNavHistory.cpp
@@ +1738,5 @@
> +
> +nsresult
> +nsNavHistory::MigrateV12Up(mozIStorageConnection *aDBConn)
> +{
> +  mozStorageTransaction transaction(aDBConn, PR_FALSE);

all the migration is already in a transaction, you don't need another one

@@ +1739,5 @@
> +nsresult
> +nsNavHistory::MigrateV12Up(mozIStorageConnection *aDBConn)
> +{
> +  mozStorageTransaction transaction(aDBConn, PR_FALSE);
> +  // Added in Bug 675996: a GUID column for moz_favicons to support Sync.

the blame on the method is probably enough to find out the bug number, I'd not repeat it in the comment.
Comment 12 Marco Bonardo [::mak] 2011-09-22 16:16:12 PDT
Comment on attachment 561618 [details] [diff] [review]
Part 2: migration test changes. v1

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

::: toolkit/components/places/tests/head_common.js
@@ +34,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +const CURRENT_SCHEMA_VERSION = 12;

<3
Comment 13 Marco Bonardo [::mak] 2011-09-22 16:26:33 PDT
Comment on attachment 561620 [details] [diff] [review]
Part 3: tests for GUID generation on insertion. v1

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

comments for the cleanup are valid for the new functions here clearly...

I think we may want also a couple tests hitting the update and the replace cases of the queries

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +450,5 @@
> +function guidForFaviconURI(iconURIString, cb) {
> +  let query = "SELECT guid FROM moz_favicons WHERE url = :url";
> +  let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +                      .DBConnection;
> +  let stmt  = db.createAsyncStatement(query);

directly use DBConn().createAsync...

@@ +452,5 @@
> +  let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +                      .DBConnection;
> +  let stmt  = db.createAsyncStatement(query);
> +  stmt.params.url = iconURIString;
> +  stmt.executeAsync(cb);

finalize() me plz

@@ +493,5 @@
> +    title
> +  );
> +}
> +
> +add_test(function test_insert_synchronous_mints_guid() {

insert mints? candies?
Comment 14 Richard Newman [:rnewman] 2011-09-22 22:50:36 PDT
(In reply to Marco Bonardo [:mak] from comment #13)

> > +add_test(function test_insert_synchronous_mints_guid() {
> 
> insert mints? candies?

---
verb 
minted, past participle; minted, past tense; minting, present participle; mints, 3rd person singular present

    1. Make (a coin) by stamping metal

    2. Produce for the first time
        - an example of newly minted technology
---
Comment 15 Richard Newman [:rnewman] 2011-09-22 22:56:43 PDT
(In reply to Marco Bonardo [:mak] from comment #10)

> hm strange, so you don't get anything passing in a
> NetUtil.newURI(do_get_file("icon"))?

This seems to work. Thanks for the pointer!
Comment 16 Marco Bonardo [::mak] 2011-09-23 02:47:36 PDT
Comment on attachment 561620 [details] [diff] [review]
Part 3: tests for GUID generation on insertion. v1

waiting new version
Comment 17 Richard Newman [:rnewman] 2011-09-23 15:52:21 PDT
The following three new patches address all of the review comments so far.

Notable changes and omissions:

* There is no test for async GUID replacing, because there's no API that does that! This will come later, when the async API is massively enhanced. For now I propose just leaving a TODO and addressing that functionality when it exists in an API.

* Test added for async GUID preservation.

* COALESCE in AsyncFaviconHelpers.cpp extended. It has to make a subquery to fetch the old GUID, of course; messy but necessary.

* Add a guid field to IconData, ready to at some point not be null! This is bound in the query above.


Remaining higher-level issues:

* Before we bump the schema version, I'd like to get to closure on lastModified. In essence, if you're happy with how these patches have turned out, I'd like to extend them to resolve that issue, too, and move towards r? instead of f?.

* My pending Sync patches directly hit SQL to fiddle with favicon GUIDs. I'm fine with that for now, but we'll eventually want a better API.

* We also use synchronous calls. The async API needs extension. Again, this is a future work item.
Comment 18 Richard Newman [:rnewman] 2011-09-23 16:12:44 PDT
Created attachment 562181 [details] [diff] [review]
Part 1: implementation. v2
Comment 19 Richard Newman [:rnewman] 2011-09-23 16:13:41 PDT
Created attachment 562182 [details] [diff] [review]
Part 2: tests and migration test changes. v2

I rolled these two parts together, because Bugzilla uploads are a pain.
Comment 20 Marco Bonardo [::mak] 2011-09-24 06:08:21 PDT
A thing I was thinking about today, while looking at a new schema... each page may have a favicon, favicons don't exist without at least one associated page.  Would it be plausible for Sync to consider a favicon as an annotation of a page and sync the icon with the page, as if it was the page title or such? Then we may completely avoid guids and send favicon data encrypted in the page payload.
I honestly don't remember if we discussed this in the design part of this implementation.
Comment 21 Richard Newman [:rnewman] 2011-09-24 10:31:20 PDT
(In reply to Marco Bonardo [:mak] from comment #20)
> A thing I was thinking about today, while looking at a new schema... each
> page may have a favicon, favicons don't exist without at least one
> associated page.  Would it be plausible for Sync to consider a favicon as an
> annotation of a page and sync the icon with the page, as if it was the page
> title or such? Then we may completely avoid guids and send favicon data
> encrypted in the page payload.
> I honestly don't remember if we discussed this in the design part of this
> implementation.

I suspect it was discussed and dismissed fairly quickly, for the following reasons:

* Favicons are fairly large, and will only get larger to support modern features. Putting a 1KB data URI in every history item (say five thousand) seems wasteful, in server space (that's 20% of a user's quota!), encryption time, client memory consumption, and upload/download bandwidth. It's even worse when you consider that every one of philikon's hundreds of Gmail history items (one per message) will have the same favicon... and we will eventually be moving towards large favicons. What will we do when Gmail has a 128x128 icon for a web app wrapper?

* What do we do when a favicon changes? Re-upload every affected places item? Or process the downloaded favicons in timestamp order, and rely on the client happening to get the last *.google.com history item last (a faulty assumption, I think)?

* We are already signing ourselves up for an additional DB hit for every incoming record ("associate this bookmark with this favicon"), unless we extend some more APIs. I'd be very leery of additionally writing (or testing whether to write) a redundant downloaded favicon for every changed record. That seems like it would be prohibitively expensive. Sync is already slower than we would like.

These three points alone suggest that preserving in Sync the indirection that exists in places.sqlite is the best choice, despite the additional effort of extending Places and implementing a new Sync engine.

I'd be very happy to have holes poked in these...
Comment 22 Richard Newman [:rnewman] 2011-09-24 11:11:28 PDT
There is a possible "80%" solution: determine which bookmarks and history items are the ones that really affect UX (i.e., toolbar, and those that appear in the Fennec "awesome list"), and embed favicon data in those. Embed just the favicon URI in the rest.

This should achieve somewhat greater coverage than just those records, thanks to shared favicons.

There's still the risk of overflowing a WBO with big icons, but it might be a decent partial solution.

Opinions, anyone?
Comment 23 Marco Bonardo [::mak] 2011-09-26 07:08:10 PDT
Yes, the icon filesize is an issue. I mostly reported my thoughts since in the schema favicons table is practically identical to annotations table, the only difference is the guid. I'm still iterating over this merging and right now is mostly a schema on paper with lots of "thoughts up in the air". Merging in the end may be unefficient though, based on numbers. I'm evaluating how much space data re-use is saving on my database, and it's a lot, I have 4MB of icon data, 62MB are saved by re-use:

SELECT SUM (LENGTH(data)) FROM moz_favicons;

SELECT SUM(datalen * (usecount-1)) FROM  (
  SELECT LENGTH(f.data) AS datalen, count(*) AS usecount FROM moz_favicons f
  JOIN moz_places h ON f.id = h.favicon_id
  GROUP BY f.id HAVING usecount >= 2
);

guids will add 87KB (12bytes per icon), so avoiding guids is probably not worth it, since those 87KB are going to be overheaded by the 2 annotations rows needed by each page on a merge (45% of my icons are reused, thus they don't need further rows).
In the end I think numbers suggest to continue having a separate icons table. thank you for being my nemesis on this reasoning :)

The suggested solution of syncing UI items is something that already came up in the past, but it's hard to figure all stuff that matters in the UI (that potentially may show EVERY single page in the DB), and at each UI change we would have to re-evaluate all of that.

PS: may be worth to telemetry icons size and reuse statistics!
Comment 24 Marco Bonardo [::mak] 2011-09-26 08:20:26 PDT
Comment on attachment 562182 [details] [diff] [review]
Part 2: tests and migration test changes. v2

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

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +365,5 @@
> + * Provide a mozIStorageStatementCallback, such as a SingleGUIDCallback.
> + */
> +function guidForFaviconURI(iconURIString, cb) {
> +  let query = "SELECT guid FROM moz_favicons WHERE url = :url";
> +  let stmt = cb.statement = DBConn().createAsyncStatement(query);

nit: kill useless query temp var, just put the string in createAsyncStatement

@@ +380,5 @@
> +  this.called = 0;
> +  this.cb     = cb;
> +}
> +SingleGUIDCallback.prototype = {
> +  guid: null,

_guid, since intended for internal use

@@ +384,5 @@
> +  guid: null,
> +  handleCompletion: function handleCompletion(reason) {
> +    do_log_info("Completed single GUID callback.");
> +    do_check_eq(this.called, 1);
> +    this.statement.finalize();

you should finalize() just after executeAsync, there is no need to wait for completion

@@ +389,5 @@
> +    this.cb(this.guid);
> +  },
> +  handleError: function handleError(err) {
> +    _("Got error: " + err);
> +    do_throw(err);

do_throw already prints out, what's the point of _() (also, I though that magic method had been removed)

@@ +396,5 @@
> +    this.called++;
> +    this.guid = resultSet.getNextRow().getResultByName("guid");
> +    do_log_info("Retrieved GUID is " + this.guid);
> +    do_check_true(!!this.guid);
> +    do_check_valid_places_guid(this.guid);

I'm pretty sure the second check is enough

@@ +397,5 @@
> +    this.guid = resultSet.getNextRow().getResultByName("guid");
> +    do_log_info("Retrieved GUID is " + this.guid);
> +    do_check_true(!!this.guid);
> +    do_check_valid_places_guid(this.guid);
> +    do_check_eq(null, resultSet.getNextRow());  // No more rows.

nit: since favicons url has a unique constraint this test seems of low value, btw if you like for sanity that's fine

@@ +463,5 @@
> +
> +  let forceReload = false;
> +  do_log_info("Asynchronously setting page favicon.");
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(
> +    pageURI, iconURI, forceReload, faviconDataCallback

there's a nice thing about faviconDataCallback, that is annotated with [function] so you actually may completely avoid building an object for it.

@@ +492,5 @@
> +
> +    let forceReload = true;
> +    do_log_info("Asynchronously re-setting page favicon.");
> +    PlacesUtils.favicons.setAndFetchFaviconForPage(
> +      pageURI, iconURI, forceReload, faviconDataCallback

ditto
Comment 25 Marco Bonardo [::mak] 2011-09-26 08:31:49 PDT
Comment on attachment 562181 [details] [diff] [review]
Part 1: implementation. v2

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

Looks good.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +765,5 @@
> +                ":icon_url, :data, :mime_type, :expiration, "
> +                "COALESCE(:guid, "
> +                          "(SELECT guid FROM moz_favicons "
> +                           "WHERE url = :icon_url "
> +                           "LIMIT 1), "

the limit 1 is useless, doesn't moz_favicons have a unique constraint on url?

@@ +781,5 @@
>      rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("expiration"), mIcon.expiration);
>      NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Optionally override the current (or generated) GUID.
> +    if (mIcon.guid.IsVoid()) {

comment may be improved, saying that _binding a guid allows_ to override the current or generated guid
Also, let's use IsEmpty() for added flexibility

::: toolkit/components/places/nsFaviconService.cpp
@@ +182,5 @@
>      "SELECT f.data, f.mime_type FROM moz_favicons f WHERE url = :icon_url"));
>  
>    RETURN_IF_STMT(mDBInsertIcon, NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_favicons (id, url, data, mime_type, expiration, "
> +                              "guid) "

nit: I think you went on newline to cope with VALUES, we usually don't, btw I won't care.
Comment 26 Marco Bonardo [::mak] 2011-10-07 12:42:14 PDT
Richard, I stoled great part of your tests patch in bug 691509 since I bumped the schema. I annotated that in the commit message, hope that's fine. So you can remove some code from here if that bug makes central.
Comment 27 Richard Newman [:rnewman] 2011-11-28 13:00:44 PST
Created attachment 577339 [details] [diff] [review]
Part 1: guids. Unbitrotted. v3

We're going to land lastModified at a later date.
Comment 28 Richard Newman [:rnewman] 2011-11-28 13:01:16 PST
Created attachment 577341 [details] [diff] [review]
Part 2: tests. v3
Comment 29 Marco Bonardo [::mak] 2011-11-29 07:27:30 PST
Comment on attachment 577339 [details] [diff] [review]
Part 1: guids. Unbitrotted. v3

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

Sorry, you'll have to bump up the schema version again and move to 14, 13 has been taken by dynamic containers removal.
Btw, this should be the easy part, actually I think we may expose the schema version in nsPIPlacesDatabase so we don't have to mirror and bump it in head_common.js. But that's for another bug.

Please add another UNION ALL check for moz_favicons at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Database.cpp#1336

You should also update SetFaviconUrlForPage since it's doing an INSERT you are not handling. Looks like your test is not catching this, so please add a test for it.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +726,5 @@
>    // If there is no entry for this icon, or the entry is obsolete, replace it.
>    if (mIcon.id == 0 || (mIcon.status & ICON_STATUS_CHANGED)) {
> +    // N.B., the 'multi-coalesce' here ensures that replacing a favicon without
> +    // specifying a :guid parameter doesn't cause it to be allocated a new
> +    // GUID.

nit: s/N.B., t/T/ (I don't think there's need for NB prefix!)

::: toolkit/components/places/Database.cpp
@@ +632,5 @@
>        // Firefox 4 uses schema version 11.
>  
>        // Firefox 8 uses schema version 12.
>  
> +      // Firefox 11 uses schema version 13.

this comment should go after the migrator, btw you'll just have to add your migrator before the existing comment and bump the number.

@@ +703,5 @@
>      // moz_favicons.
>      rv = mMainConn->ExecuteSimpleSQL(CREATE_MOZ_FAVICONS);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID);
> +    NS_ENSURE_SUCCESS(rv, rv);

ditto: not needed

@@ +1272,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  // For existing profiles, we may not have a moz_favicons.guid column,
> +  // or lastModified.

no lastModified yet

@@ +1276,5 @@
> +  // or lastModified.
> +  nsCOMPtr<mozIStorageStatement> hasGuidStatement;
> +  nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +      "SELECT guid FROM moz_favicons"
> +    ), getter_AddRefs(hasGuidStatement));

indent less, it's ok to have the ), lined up with nsresult

@@ +1281,5 @@
> +
> +  if (NS_FAILED(rv)) {
> +    rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +      "ALTER TABLE moz_favicons "
> +      "ADD COLUMN guid TEXT"

TEXT UNIQUE

@@ +1287,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Also create the indices.
> +    rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_FAVICONS_GUID);
> +    NS_ENSURE_SUCCESS(rv, rv);

ditto: nope

::: toolkit/components/places/nsFaviconService.cpp
@@ +446,5 @@
> +        "UPDATE moz_favicons SET "
> +               "data       = :data, "
> +               "guid       = COALESCE(:guid, guid), "
> +               "mime_type  = :mime_type, "
> +               "expiration = :expiration "

nit: mime and expiration are more related to data, so make guid either the first or the last entry.

in Sync you'll likely use the new interface Felix is working in bug 699843 rather than this one, in that regard you may want to (in the sense you should :) ) give your additive feedback on the API (currently those APIs don't take guid into account while they should, somehow).

Please explicitly bind guid to null.

@@ +465,5 @@
>        // Insert a new entry.
>        statement = mDB->GetStatement(
> +       "INSERT INTO moz_favicons (id, url, data, mime_type, expiration, guid) "
> +       "VALUES (:icon_id, :icon_url, :data, :mime_type, :expiration, "
> +               "COALESCE(:guid, GENERATE_GUID()))");

please explicitly bind guid to null

::: toolkit/components/places/nsPlacesIndexes.h
@@ +151,5 @@
> +
> +#define CREATE_IDX_MOZ_FAVICONS_GUID \
> +  CREATE_PLACES_IDX( \
> +    "guid_uniqueindex", "moz_favicons", "guid", "UNIQUE" \
> +  )

ditto: unneeded

::: toolkit/components/places/nsPlacesTables.h
@@ +127,5 @@
>      ", url LONGVARCHAR UNIQUE" \
>      ", data BLOB" \
>      ", mime_type VARCHAR(32)" \
>      ", expiration LONG" \
> +    ", guid TEXT" \

guid TEXT UNIQUE and remove the custom index from nsPlacesIndexes.h
Comment 30 Marco Bonardo [::mak] 2011-11-29 07:38:42 PST
Comment on attachment 577341 [details] [diff] [review]
Part 2: tests. v3

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

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +384,5 @@
> +  guid: null,
> +  handleCompletion: function handleCompletion(reason) {
> +    do_log_info("Completed single GUID callback.");
> +    do_check_eq(this.called, 1);
> +    this.statement.finalize();

there's no need for this roundup, as soon as you invoke .executeAsync you can invoke .finalize() on a statement.

@@ +388,5 @@
> +    this.statement.finalize();
> +    this.cb(this.guid);
> +  },
> +  handleError: function handleError(err) {
> +    _("Got error: " + err);

this is not the Sync test harness, doubt this would work!

@@ +393,5 @@
> +    do_throw(err);
> +  },
> +  handleResult: function handleResult(resultSet) {
> +    this.called++;
> +    this.guid = resultSet.getNextRow().getResultByName("guid");

just for sanity
let row = resultSet.getNextRow();
do_check_true(!!row);

@@ +396,5 @@
> +    this.called++;
> +    this.guid = resultSet.getNextRow().getResultByName("guid");
> +    do_log_info("Retrieved GUID is " + this.guid);
> +    do_check_true(!!this.guid);
> +    do_check_valid_places_guid(this.guid);

the latter check probably makes the former pointless

@@ +397,5 @@
> +    this.guid = resultSet.getNextRow().getResultByName("guid");
> +    do_log_info("Retrieved GUID is " + this.guid);
> +    do_check_true(!!this.guid);
> +    do_check_valid_places_guid(this.guid);
> +    do_check_eq(null, resultSet.getNextRow());  // No more rows.

do_check_false(resultSet.getNextRow()); should just work

@@ +433,5 @@
> +  // Check that the URI has been set correctly.
> +  do_check_eq(PlacesUtils.favicons.getFaviconForPage(pageURI).spec,
> +              testIconURI);
> +
> +  guidForFaviconURI(testIconURI, new SingleGUIDCallback(run_next_test));

interesting, I would have expected this to fail since the patch didn't seem to handle setFaviconUrlForPage...
Comment 31 Richard Newman [:rnewman] 2011-11-29 14:43:12 PST
(In reply to Marco Bonardo [:mak] from comment #29)

> Please add another UNION ALL check for moz_favicons at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> Database.cpp#1336
> 
> You should also update SetFaviconUrlForPage since it's doing an INSERT you
> are not handling. Looks like your test is not catching this, so please add a
> test for it.

...

> > +      "ADD COLUMN guid TEXT"
> 
> TEXT UNIQUE

I was viewing it as OK for a favicon to not have a GUID yet, because it'll be assigned one lazily during sync reconciliation.

That might not be a good idea; I'm not sure.

I'll make these changes and we can ponder :)
Comment 32 Richard Newman [:rnewman] 2011-11-29 15:16:21 PST
(In reply to Marco Bonardo [:mak] from comment #30)

> @@ +393,5 @@
> > +    do_throw(err);
> > +  },
> > +  handleResult: function handleResult(resultSet) {
> > +    this.called++;
> > +    this.guid = resultSet.getNextRow().getResultByName("guid");
> 
> just for sanity
> let row = resultSet.getNextRow();
> do_check_true(!!row);

Already there:

    do_check_eq(null, resultSet.getNextRow());  // No more rows.



> @@ +396,5 @@
> > +    this.called++;
> > +    this.guid = resultSet.getNextRow().getResultByName("guid");
> > +    do_log_info("Retrieved GUID is " + this.guid);
> > +    do_check_true(!!this.guid);
> > +    do_check_valid_places_guid(this.guid);
> 
> the latter check probably makes the former pointless

I'm pretty sure it used to pass for null, even if right now it looks like it won't. I'm inclined to leave it :D

> do_check_false(resultSet.getNextRow()); should just work

Nope. do_check_{true,false} check for real equality, not truthiness. That's why you'll see

  do_check_true(!!something)

throughout Sync's tests. Also, getNextRow is explicitly defined in terms of null, IIRC, so testing for null seems more thorough.
Comment 33 Marco Bonardo [::mak] 2011-11-29 15:32:45 PST
(In reply to Richard Newman [:rnewman] from comment #31)
> I was viewing it as OK for a favicon to not have a GUID yet, because it'll
> be assigned one lazily during sync reconciliation.
> 
> That might not be a good idea; I'm not sure.

I think whatever makes icons a special case compared to bookmarks or pages should be seen with suspicion at this point, we come from fancy designing and it didn't bring really good gains :)

(In reply to Richard Newman [:rnewman] from comment #32)
> (In reply to Marco Bonardo [:mak] from comment #30)
> > > +    this.guid = resultSet.getNextRow().getResultByName("guid");
> > > +    do_log_info("Retrieved GUID is " + this.guid);
> > > +    do_check_true(!!this.guid);
> > > +    do_check_valid_places_guid(this.guid);
> > 
> > the latter check probably makes the former pointless
> 
> I'm pretty sure it used to pass for null, even if right now it looks like it
> won't. I'm inclined to leave it :D

I'm pretty sure it can't pass, since I can't see how null could pass a regexp. btw whatever.

> > do_check_false(resultSet.getNextRow()); should just work
> 
> Nope. do_check_{true,false} check for real equality, not truthiness. That's
> why you'll see
> 
>   do_check_true(!!something)

uh right, I confused with executeStep() :(
Comment 34 Richard Newman [:rnewman] 2011-11-29 19:24:29 PST
Created attachment 577834 [details] [diff] [review]
Part 1: guids. v4

(In reply to Marco Bonardo [:mak] from comment #29)

> > +  if (NS_FAILED(rv)) {
> > +    rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> > +      "ALTER TABLE moz_favicons "
> > +      "ADD COLUMN guid TEXT"
> 
> TEXT UNIQUE

"The ADD COLUMN syntax is used to add a new column to an existing table. … The column may not have a PRIMARY KEY or UNIQUE constraint."

I have attempted to do an equivalent thing by using CREATE UNIQUE INDEX instead of ADD COLUMN UNIQUE.
Comment 35 Richard Newman [:rnewman] 2011-11-29 19:25:18 PST
Fixing summary. lastModified will come later.
Comment 36 Richard Newman [:rnewman] 2011-11-29 19:26:04 PST
Created attachment 577835 [details] [diff] [review]
Part 2: tests. v4
Comment 37 Marco Bonardo [::mak] 2011-11-30 02:27:40 PST
(In reply to Richard Newman [:rnewman] from comment #34)
> "The ADD COLUMN syntax is used to add a new column to an existing table. …
> The column may not have a PRIMARY KEY or UNIQUE constraint."
> 
> I have attempted to do an equivalent thing by using CREATE UNIQUE INDEX
> instead of ADD COLUMN UNIQUE.

ugh, it's a stupid limitation, than the old code was correct, the new one is not (since you don't use anymore nsPlacesIndexes.h). My fault, I did not remember that crazy SQLite limit.
Comment 38 Richard Newman [:rnewman] 2011-11-30 11:51:05 PST
(In reply to Marco Bonardo [:mak] from comment #37)

> ugh, it's a stupid limitation, than the old code was correct, the new one is
> not (since you don't use anymore nsPlacesIndexes.h). My fault, I did not
> remember that crazy SQLite limit.

Would you prefer a version which puts a UNIQUE INDEX into nsPlacesIndexes, and generates GUIDs, or a version that simply adds a non-UNIQUE index, or something else?
Comment 39 Marco Bonardo [::mak] 2011-11-30 12:02:44 PST
the index must be UNIQUE, its definition must be in nsPlacesIndexes.h (since we can't enforce it in the ALTER syntax), we should have all valid GUIDs for icons. If you have any doubt that requires large changes, ping me, I don't want to steal more of your time on miscomprehension :)
Comment 40 Marco Bonardo [::mak] 2011-12-01 09:23:08 PST
Comment on attachment 577834 [details] [diff] [review]
Part 1: guids. v4

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

Looks good, apart these few things

::: toolkit/components/places/Database.cpp
@@ +638,4 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
> +
> +      // Firefox 11 uses schema version 14.

you just killed the v13 migration function :(

you migrator should be additive

@@ +1291,5 @@
> +Database::MigrateV14Up()
> +{
> +  // For existing profiles, we may not have a moz_favicons.guid column.
> +  // Add it here. We want it to be unique, but ALTER TABLE doesn't allow
> +  // a uniqueness constraint, so do this in several steps.

s/so do this in several steps/so the index has to be created separately/

@@ +1313,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // And now we can make the column unique.
> +    rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +      "CREATE UNIQUE INDEX moz_favicons_guid_uniqueindex ON moz_favicons (guid)"

move back to nsPlacesIndices.h (sorry)

::: toolkit/components/places/Database.h
@@ +291,3 @@
>    nsresult MigrateV13Up();
> +  nsresult MigrateV14Up();
> +  nsresult CheckAndUpdateGUIDs();

while here please newline between migrators and helpers

::: toolkit/components/places/nsPlacesTables.h
@@ +127,5 @@
>      ", url LONGVARCHAR UNIQUE" \
>      ", data BLOB" \
>      ", mime_type VARCHAR(32)" \
>      ", expiration LONG" \
> +    ", guid TEXT UNIQUE" \

please revert to the previous separate index (my fault)
Comment 41 Marco Bonardo [::mak] 2011-12-01 09:28:19 PST
Comment on attachment 577835 [details] [diff] [review]
Part 2: tests. v4

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

Tests look good

::: toolkit/components/places/tests/migration/test_current_from_v10.js
@@ +296,5 @@
>    do_check_true(db.indexExists("moz_bookmarks_guid_uniqueindex"));
>    do_check_true(db.indexExists("moz_places_guid_uniqueindex"));
>  
> +  // We manually create the unique index, so make sure we now have it.
> +  do_check_true(db.indexExists("moz_favicons_guid_uniqueindex"));

well, at this point kill the comment and just add your check to the existing ones (no newlines in the middle)

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +381,5 @@
> +  this.called = 0;
> +  this.cb     = cb;
> +}
> +SingleGUIDCallback.prototype = {
> +  guid: null,

nit: internal properties should be prefixed with _... btw, being a test I may ignore that :)

@@ +389,5 @@
> +    this.cb(this.guid);
> +  },
> +  handleError: function handleError(err) {
> +    do_log_info("Got error: " + err);
> +    do_throw(err);

the info is likely useless since there is a do_throw
Comment 42 Marco Bonardo [::mak] 2011-12-01 09:28:57 PST
Comment on attachment 577834 [details] [diff] [review]
Part 1: guids. v4

well this was actually a minus for the migrator murder
Comment 43 Richard Newman [:rnewman] 2011-12-01 13:56:10 PST
Created attachment 578381 [details] [diff] [review]
Part 1. guids. v5

Should be a rubberstamp.
Comment 44 Richard Newman [:rnewman] 2011-12-01 13:56:38 PST
Created attachment 578382 [details] [diff] [review]
Part 2. tests. v5
Comment 45 Marco Bonardo [::mak] 2011-12-01 14:04:57 PST
Comment on attachment 578381 [details] [diff] [review]
Part 1. guids. v5

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

I couldn't say no.
Comment 46 Richard Newman [:rnewman] 2011-12-01 14:09:51 PST
INBOUND!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2dd9486d41
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c47ef9419ac

Let's see if they make it through the trial of orange.

Thanks for the timely reviews, mak.

Note You need to log in before you can comment on or make changes to this bug.