386368 - Use getters and setters for database schema version

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Shawn Wilsher :sdwilsh]

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.139&root=/cvsroot#523

Probably just replace this function with the right call to the new function:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.139&root=/cvsroot#708

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[(not reading, please use [e-mail] instead)]

Comment on attachment 270558 [details] [diff] [review]
v1.0

shawn, with an existing places.sqlite file, where the user_version has not been set (so sqlite returns 0 for user_version, right?) does the existing code return NS_ERROR_FAILURE?

with your patch, it looks like we will return failure now, since you do:

NS_ENSURE_TRUE(0 != DBSchemaVersion, NS_ERROR_FAILURE);

If I am right, I think you just want to remove that line, as DBSchemaVersion == 0 is ok.

this won't happen with a new profile, as I think if moz_places doesn't exist we call UpdateSchemaVersion() to set it, but it would if you were upgrading from an old trunk build, before we started doing user_version.

To try it out, you could take a profile and set user_schema to 0 and start up, right?

Comment 3

[Shawn Wilsher :sdwilsh]

I only put that in because of this line:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/places/src/nsNavHistory.cpp&rev=1.139&root=/cvsroot#533

Comment 4

[(not reading, please use [e-mail] instead)]

I can't remember off the top of my head, but from bug #386366, what does your code in mozStorageConnection::GetSchemaVersion() do when user_schema is not set?

+    PRBool hasResult;
+    if (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult)
+        *version = stmt->AsInt32(0);

Is hasResult set to true?

From the existing code in nsNavHistor.cpp (that you are replacing), it seems like it would set it to true, otherwise "NS_ENSURE_TRUE(hasResult, NS_ERROR_FAILURE);" would cause us to bail out.  

It doesn't seem like your patch and the existing code behave the same.

Can you test and confirm?

Comment 5

[Shawn Wilsher :sdwilsh]

actually, hasResult is always true for this query, so that check before was useless.  New patch in just a moment.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 270595 [details] [diff] [review]
v1.1

r=sspitzer, thanks shawn

Comment 8

[Shawn Wilsher :sdwilsh]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
new revision: 1.140; previous revision: 1.139

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Is there any particular way to verify that this is working correctly?

Comment 10

[Shawn Wilsher :sdwilsh]

Database migration would be broken if this didn't work correctly.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Dietrich, do you know how far back we'd have to go to get a version of places that would migrate to exercise this code? A5?

Comment 12

[Dietrich Ayala (:dietrich)]

There haven't been any schema changes since bookmarks were turned on (afaicr), so I'm not sure you can verify this until either bug 319455 or bug 360134 land.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Those have landed now. Would migrating from an earlier alpha exercise this?

Comment 14

[Dietrich Ayala (:dietrich)]

(In reply to comment #13)
> Those have landed now. Would migrating from an earlier alpha exercise this?
> 

Yes, bug 319455 uses these.

Comment 15

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv