Bookmark-specific metadata that is in Fx2, but not in Places: description, dateAdded, lastModified.

Adding to the blockers list, at least for the potential schema changes.

this adds dateAdded and lastModified fields, and the triggers to manage them. descriptions are handled via annotations at the moment. need to figure out if description changes (or any annotation of a bookmark) should trigger an update of lastModified.

This implements the schema changes for the date fields. The other part of the bug is to push those values out into query results, and utilize them in the view. Before doing that I'd like to clean up bookmark identity in the query results (bug 373971). So I'd like to check in the schema changes now so I can group schema-changing checkins together.

Comment on attachment 258585 [details] [diff] [review] schema changes mano reminded that i should add the folder and separator triggers now as well.

Comment on attachment 258585 [details] [diff] [review] schema changes der, those triggers should work for any type inserted into the table.

Comment on attachment 258585 [details] [diff] [review] schema changes r=sspitzer

Checked in the schema changes: Checking in nsNavBookmarks.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavBookmarks.cpp new revision: 1.73; previous revision: 1.72 done

(In reply to comment #7) > Checked in the schema changes: > > Checking in nsNavBookmarks.cpp; > /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- > nsNavBookmarks.cpp > new revision: 1.73; previous revision: 1.72 > done > backed out due to causing 374241.

New patch, revs schema version.

Comment on attachment 263065 [details] [diff] [review] wip patch this patch adds the date cols, in the back-end and front. still needs: - migration code needs to be updated, is rotted - import/export of these fields (low-priority imo) - i'm munging the values somewhere, the end-result dates in the UI are wrong - need to figure out what editing events don't throw the trigger, but should update the fields (eg: description/keyword anno edits), and implement (also low priority) - would be nicer implementation if we had bookmark node types (bug 373971), but not required

starting with the schema part, per dietrich

> - i'm munging the values somewhere, the end-result dates in the UI are wrong I've figured this out. We're using DATETIME in our code, but we think it is a PRTime. When we do strftime("%s"), that is seconds since 1970-01-01, and not in microseconds (like a PRTime) as a quick fix for the UI problem, I just made this simple change to treeView.js -var dateAdded = new Date(node.dateAdded / 1000); +var dateAdded = new Date(node.dateAdded * 1000); (and the same for lastModified). we may not want to use DATETIME, and instead use INTEGER like we do with columns like visit_date. But maybe dietrich wanted to use DATETIME for the useful CURRENT_TIMESTAMP default value and the DATETIME('NOW') trigger? For now, I'll just change to mulitplying by 1000 to get a valid time in treeView.js.

1) set dateAdded on item creation. 2) show nothing in the tree view if we don't have a dateAdded or lastModified value, like we do for lastVisited

after this, I'd like to break out the import/export of ADD_DATE.

includes fix for updating the dateAdded and lastModified in the UI, a fix for a problem where GetItemLastModified() was returning the value of dateAdded (because I was using the wrong column, kGetItemPropertiesIndex_DateAdded instead of kGetItemPropertiesIndex_LastModified), and some tests.

Comment on attachment 265222 [details] [diff] [review] updated patch (excludes import / export) r=me, with the nits below fixed. > >+ nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); >+ NS_ENSURE_TRUE(bookmarks, NS_ERROR_UNEXPECTED); >+ >+ PRTime lastModified; >+ nsresult rv = bookmarks->GetItemLastModified(aItemId, &lastModified); >+ if (NS_SUCCEEDED(rv)) >+ node->mLastModified = lastModified; >+ else >+ node->mLastModified = 0; iirc, toolkit style is to use brackets. >+ >+ PRTime dateAdded; >+ rv = bookmarks->GetItemDateAdded(aItemId, &dateAdded); >+ if (NS_SUCCEEDED(rv)) >+ node->mDateAdded = dateAdded; >+ else >+ node->mDateAdded = 0; >+ ditto > > // insert a bookmark >+ // the time before we insert, in microseconds >+ var beforeInsert = Date.now() * 1000; >+ do_check_true(beforeInsert > 0); >+ > var newId = bmsvc.insertItem(testRoot, uri("http://google.com/"), bmsvc.DEFAULT_INDEX); > do_check_eq(observer._itemAddedId, newId); > do_check_eq(observer._itemAddedParent, testRoot); > do_check_eq(observer._itemAddedIndex, testStartIndex); > do_check_eq(bmsvc.getBookmarkURI(newId).spec, "http://google.com/"); > >+ var dateAdded = bmsvc.getItemDateAdded(newId); >+ do_check_true(dateAdded > 0); given the test below, isn't this test superfluous? >+ // dateAdded can equal beforeInsert >+ do_check_true(dateAdded >= beforeInsert); >+ >- // test setKeywordForURI >+ // test setKeywordForBookmark > var kwTestItemId = bmsvc.insertItem(testRoot, uri("http://keywordtest.com"), bmsvc.DEFAULT_INDEX); > try { >+ var dateAdded = bmsvc.getItemDateAdded(kwTestItemId); >+ // after just inserting, modified should not be set >+ var lastModified = bmsvc.getItemLastModified(kwTestItemId); >+ do_check_eq(lastModified, 0); per discussion in RL, i think these tests for get* apis are overkill. might be relevant if we used triggers, but we're not atm. >+ try { >+ var lastModified = bmsvc.getItemLastModified(testItemId); >+ var annoVal = annosvc.getItemAnnotationString(testItemId, testAnnoName); >+ // verify the anno value >+ do_check_eq(testAnnoVal, annoVal); >+ var lastModified2 = bmsvc.getItemLastModified(testItemId); >+ // verify that getting the annotation doesn't update the last modified time >+ do_check_eq(lastModified2, lastModified); >+ } catch(ex) { >+ do_throw("unable to get item annotation"); >+ } ditto

(In reply to comment #26) > Created an attachment (id=265230) [details] > the import export parts > note that we have to set last-modified twice for bookmarks/livemarks with descriptions, due to the wack nature of the bookmarks.html format.

some comments: 1) for // casting from PRInt64 -> PRInt32, data loss, fix me covered by bug #380449. 2) can you add some tests to test dateAdded / lastModified for folders? 3) + + // XXX todo set the ADD_DATE if we have it on the separator + // fx 2 style bookmarks.html don't have this, but fx 3 style will + // XXX todo set the LAST_MODIFIED for separators? can you remove that? as we found out tonight, we can only do the name attribute on separators (<HR>). anything else will break older versions of firefox. if thunder didn't already, can you comment about how we can only do name for separators? 4) can we create a helper function that takes the attribute string, a PRTime, and aOutput and then writes out the value? this code is duplicated a few times. + rv = aOutput->Write(kLastModifiedAttribute, sizeof(kLastModifiedAttribute)-1, &dummy); + NS_ENSURE_SUCCESS(rv, rv); + nsCAutoString lastModifiedInSeconds; + lastModified /= 1000000; // in bookmarks.html this value is in seconds, not microseconds + lastModifiedInSeconds.AppendInt(lastModified); // casting from PRInt64 -> PRInt32, data loss, fix me + rv = aOutput->Write(lastModifiedInSeconds.get(), lastModifiedInSeconds.Length(), &dummy); + NS_ENSURE_SUCCESS(rv, rv); + rv = aOutput->Write(kQuoteStr, sizeof(kQuoteStr)-1, &dummy); + NS_ENSURE_SUCCESS(rv, rv);

Comment on attachment 265293 [details] [diff] [review] complete patch >+ >+ // Set last-modified a 2nd time for all items with descriptions >+ PRTime lastModified; >+ if (!frame.mPreviousLink) { >+ lastModified = PreviousFrame().mPreviousLastModifiedDate; >+ } else { >+ lastModified = frame.mPreviousLastModifiedDate; >+ } >+ >+ if (itemId > 0 && lastModified > 0) { >+ rv = mBookmarksService->SetItemLastModified(itemId, lastModified); >+ NS_ASSERTION(NS_SUCCEEDED(rv), "SetItemLastModified failed"); >+ } rationale: we need to set last-modified as the *last* step in processing any item type in the bookmarks.html file, so that we do not overwrite the imported value. for items without descriptions, setting this value after setting the item title is that last point at which we can save this value before it gets reset. for items with descriptions, it must set after that point. however, at the point at which we set the title, there's no way to determine if there will be a description following, so we need to set the last-modified-date at both places. an alternative might be to cache all of the values, and process it in HandleContainerBegin, but i haven't tested that yet.

Comment on attachment 265293 [details] [diff] [review] complete patch another round is coming.

Comment on attachment 265307 [details] [diff] [review] updated patch, per comments from mano r=mano with the terminology fix in the result-node interface we've discussed; and with the uuid of the options interface not rev'ed.

and please also fix the indent in _convertPRTimeToString.

fixed. there are some spin off bugs, and a few more coming. Checking in browser/components/places/content/places.xul; /cvsroot/mozilla/browser/components/places/content/places.xul,v <-- places.xul new revision: 1.73; previous revision: 1.72 done Checking in browser/components/places/content/treeView.js; /cvsroot/mozilla/browser/components/places/content/treeView.js,v <-- treeView. js new revision: 1.9; previous revision: 1.8 done Checking in browser/components/places/src/nsPlacesImportExportService.cpp; /cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v <-- nsPlacesImportExportService.cpp new revision: 1.16; previous revision: 1.15 done Checking in browser/components/places/tests/unit/bookmarks.preplaces.html; /cvsroot/mozilla/browser/components/places/tests/unit/bookmarks.preplaces.html,v <-- bookmarks.preplaces.html new revision: 1.4; previous revision: 1.3 done Checking in browser/components/places/tests/unit/test_bookmarks_html.js; /cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v <-- test_bookmarks_html.js new revision: 1.8; previous revision: 1.7 done Checking in browser/locales/en-US/chrome/browser/places/places.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v <-- places.dtd new revision: 1.22; previous revision: 1.21 done Checking in toolkit/components/places/public/nsINavBookmarksService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v <-- nsINavBookmarksService.idl new revision: 1.40; previous revision: 1.39 done Checking in toolkit/components/places/public/nsINavHistoryService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v <- - nsINavHistoryService.idl new revision: 1.60; previous revision: 1.59 done Checking in toolkit/components/places/src/nsNavBookmarks.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavB ookmarks.cpp new revision: 1.92; previous revision: 1.91 done Checking in toolkit/components/places/src/nsNavBookmarks.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v <-- nsNavBoo kmarks.h new revision: 1.41; previous revision: 1.40 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHis tory.cpp new revision: 1.125; previous revision: 1.124 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHisto ry.h new revision: 1.78; previous revision: 1.77 done Checking in toolkit/components/places/src/nsNavHistoryResult.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- ns NavHistoryResult.cpp new revision: 1.96; previous revision: 1.95 done Checking in toolkit/components/places/src/nsNavHistoryResult.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v <-- nsNa vHistoryResult.h new revision: 1.38; previous revision: 1.37 done Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js; /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v <-- test_bookmarks.js new revision: 1.13; previous revision: 1.12 done Checking in toolkit/components/places/tests/unit/test_annotations.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_annotations.js,v <-- test_annotations.js new revision: 1.5; previous revision: 1.4 done Checking in toolkit/components/places/tests/unit/test_result_sort.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_result_sort.js,v <-- test_result_sort.js new revision: 1.4; previous revision: 1.3 done

I've verified this in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200708020808 Minefield/3.0a7pre. I looked in both the places.sqlite and also at the same bookmark in organize bookmarks. Is there a way to test the import/export functionality for these new fields?

(In reply to comment #42) > I've verified this in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; > rv:1.9a7pre) Gecko/200708020808 Minefield/3.0a7pre. > > I looked in both the places.sqlite and also at the same bookmark in organize > bookmarks. > > Is there a way to test the import/export functionality for these new fields? > both fields are in the source of bookmarks.html in your profile.

Ah yes. So they are. I verified that they continue to exist when imported. We should probably add a test case to Litmus to make sure the fields survive import and export processes.

Test cases for 3.x test runs were updated for regression testing this bug. For 3.0, https://litmus.mozilla.org/show_test.cgi?id=4132 For 3.1, https://litmus.mozilla.org/show_test.cgi?id=6090

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