Use getters and setters for database schema version

RESOLVED FIXED in Firefox 3 alpha7

Status

()

RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
Firefox 3 alpha7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Comment 1

12 years ago
Created attachment 270558 [details] [diff] [review]
v1.0
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #270558 - Flags: review?(sspitzer)
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?
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?
(Assignee)

Comment 5

12 years ago
actually, hasResult is always true for this query, so that check before was useless.  New patch in just a moment.
(Assignee)

Comment 6

12 years ago
Created attachment 270595 [details] [diff] [review]
v1.1
Attachment #270558 - Attachment is obsolete: true
Attachment #270595 - Flags: review?(sspitzer)
Comment on attachment 270595 [details] [diff] [review]
v1.1

r=sspitzer, thanks shawn
Attachment #270595 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 8

12 years ago
Checking in toolkit/components/places/src/nsNavHistory.cpp;
new revision: 1.140; previous revision: 1.139
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Is there any particular way to verify that this is working correctly?
(Assignee)

Comment 10

12 years ago
Database migration would be broken if this didn't work correctly.
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?
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.
Those have landed now. Would migrating from an earlier alpha exercise this?
(In reply to comment #13)
> Those have landed now. Would migrating from an earlier alpha exercise this?
> 

Yes, bug 319455 uses these.
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.