Closed
Bug 889239
Opened 11 years ago
Closed 11 years ago
Add a fast upgrade path for the Contacts DB
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: reuben, Assigned: reuben)
Details
Attachments
(2 files, 2 obsolete files)
16.42 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
2.73 KB,
patch
|
reuben
:
review+
|
Details | Diff | Splinter Review |
No reason to go through all the steps when we know we're creating the final schema.
Attachment #770068 -
Flags: review?(anygregor)
Comment 1•11 years ago
|
||
+++ b/dom/contacts/fallback/ContactDB.jsm @@ -104,16 +104,23 @@ this.ContactDB = function ContactDB(aGlo + if (aOldVersion == 0) { + this.createFinalSchema(aTransaction, aDb); + this.addDefaultContacts(aTransaction); + return; + } + @@ -379,65 +386,87 @@ ContactDB.prototype = { // Add default contacts if (aOldVersion == 0) { + this.addDefaultContacts(aTransaction); + } + }, Will this second `if` statement ever get run? It looks like the first `if` returns if the same condition (aOldVersion == 0) is true. Also, do we have tests that can verify that both the long-upgrade path and the new quick-upgrade produce the same schema? The reason I ask is because looking at this I would be worried about regressions as we make schema changes down the line. (now we must modify the schema in two places)
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Michael Henretty [:mikehenrty] from comment #1) > Will this second `if` statement ever get run? It looks like the first `if` > returns if the same condition (aOldVersion == 0) is true. It won't, nice catch. Second one should just be removed. > Also, do we have tests that can verify that both the long-upgrade path and > the new quick-upgrade produce the same schema? The reason I ask is because > looking at this I would be worried about regressions as we make schema > changes down the line. (now we must modify the schema in two places) Can't think of a way to do that without starting tests with an existing DB. The idea here is that if you want to check what the schema is you can quickly look at createFinalSchema. As you can see, it's small and easy to read, so it shouldn't be a problem. It also doesn't change the situation a lot, since we don't currently test partial upgrades.
Comment 3•11 years ago
|
||
(In reply to Reuben Morais [:reuben] from comment #2) > As you can see, it's small and easy to > read, so it shouldn't be a problem. True. > It also doesn't change the situation a lot, since we don't currently test > partial upgrades. I see. Let's add some? :) You guys know this system way better than I do, but would it be extremely difficult to create those default contacts with the initial version of the DB, run through the long-upgrade, and then separately create the final schema, add the same defaults contacts, and then compare the results, all in the context of a mochitest?
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Michael Henretty [:mikehenrty] from comment #3) > I see. Let's add some? :) You guys know this system way better than I do, > but would it be extremely difficult to create those default contacts with > the initial version of the DB, run through the long-upgrade, and then > separately create the final schema, add the same defaults contacts, and then > compare the results, all in the context of a mochitest? I guess we could do that as a mochitest-chrome that loads ContactDB directly. And we can just steal fakecontacts.json from Gaia.
Assignee | ||
Comment 5•11 years ago
|
||
Note that to be really extensive we'd need to do the same thing for every DB version so we test every incremental upgrade.
Comment 6•11 years ago
|
||
(In reply to Reuben Morais [:reuben] from comment #5) > Note that to be really extensive we'd need to do the same thing for every DB > version so we test every incremental upgrade. To me, it would be enough to just compare the final results of the long-upgrade and quick-upgrade paths. Yes it is not comprehensive, but having this sanity check would help us find problems in both the single version upgrades and the final schema code for any revisions going forward.
Assignee | ||
Comment 7•11 years ago
|
||
I'm working on the tests discussed here.
Attachment #770068 -
Attachment is obsolete: true
Attachment #770068 -
Flags: review?(anygregor)
Attachment #772949 -
Flags: review?(anygregor)
Comment 9•11 years ago
|
||
Comment on attachment 772949 [details] [diff] [review] Patch Review of attachment 772949 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #772949 -
Flags: review?(anygregor) → review+
Comment 10•11 years ago
|
||
I guess this needs a rebase now?
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #772949 -
Attachment is obsolete: true
Attachment #776687 -
Flags: review+
Comment 12•11 years ago
|
||
Comment on attachment 773693 [details] [diff] [review] Tests Review of attachment 773693 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/fallback/ContactDB.jsm @@ +980,5 @@ > enableSubstringMatching: function enableSubstringMatching(aDigits) { > this.substringMatching = aDigits; > }, > > + init: function init(aGlobal, aDbVersion) { When do you call it with a DB version?
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #12) > When do you call it with a DB version? In some past version of this patch ;) Please ignore that chunk, I removed it locally.
Comment 14•11 years ago
|
||
Comment on attachment 773693 [details] [diff] [review] Tests Review of attachment 773693 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/fallback/ContactDB.jsm @@ +3,5 @@ > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > +this.EXPORTED_SYMBOLS = ["ContactDB", "DB_NAME", "STORE_NAME", "SAVED_GETALL_STORE_NAME", "REVISION_STORE", "DB_VERSION"]; Add a comment saying everything except ContactDB is only needed for testing.
Attachment #773693 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Thanks! https://hg.mozilla.org/integration/b2g-inbound/rev/fd676bc1011c
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd676bc1011c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•