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
Created attachment 270558 [details] [diff] [review] v1.0
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
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?
11 years ago
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
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?
actually, hasResult is always true for this query, so that check before was useless. New patch in just a moment.
Created attachment 270595 [details] [diff] [review] v1.1
Comment on attachment 270595 [details] [diff] [review] v1.1 r=sspitzer, thanks shawn
Attachment #270595 - Flags: review?(sspitzer) → review+
Checking in toolkit/components/places/src/nsNavHistory.cpp; new revision: 1.140; previous revision: 1.139
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Is there any particular way to verify that this is working correctly?
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.