author | Kit Cambridge <kit@yakshaving.ninja> |
Thu, 16 Nov 2017 16:49:03 -0800 | |
changeset 394933 | dd84a4ad910edc4f587dc08c722bb54c4a3d1a3c |
parent 394895 | 8ed94b47791c36059fa4af120ec6343465dd088b |
child 394934 | cc8eaa26f2ed97c7a481f5a8a7e325f5d7794fe7 |
push id | 97987 |
push user | nerli@mozilla.com |
push date | Tue, 05 Dec 2017 13:52:50 +0000 |
treeherder | mozilla-inbound@8842dba7396b [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | mak |
bugs | 1417101 |
milestone | 59.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
--- a/toolkit/components/places/nsAnnotationService.cpp +++ b/toolkit/components/places/nsAnnotationService.cpp @@ -42,16 +42,42 @@ const int32_t nsAnnotationService::kAnno const int32_t nsAnnotationService::kAnnoIndex_NameID = 2; const int32_t nsAnnotationService::kAnnoIndex_Content = 3; const int32_t nsAnnotationService::kAnnoIndex_Flags = 4; const int32_t nsAnnotationService::kAnnoIndex_Expiration = 5; const int32_t nsAnnotationService::kAnnoIndex_Type = 6; const int32_t nsAnnotationService::kAnnoIndex_DateAdded = 7; const int32_t nsAnnotationService::kAnnoIndex_LastModified = 8; +namespace { + +// Fires `onItemChanged` notifications for bookmark annotation changes. +void +NotifyItemChanged(const BookmarkData& aBookmark, const nsACString& aName, + uint16_t aSource, bool aDontUpdateLastModified) +{ + if (aBookmark.id < 0) { + return; + } + + ItemChangeData changeData; + changeData.bookmark = aBookmark; + changeData.isAnnotation = true; + changeData.updateLastModified = !aDontUpdateLastModified; + changeData.source = aSource; + changeData.property = aName; + + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); + if (bookmarks) { + bookmarks->NotifyItemChanged(changeData); + } +} + +} + namespace mozilla { namespace places { //////////////////////////////////////////////////////////////////////////////// //// AnnotatedResult AnnotatedResult::AnnotatedResult(const nsCString& aGUID, nsIURI* aURI, @@ -149,27 +175,31 @@ nsAnnotationService::Init() } return NS_OK; } nsresult nsAnnotationService::SetAnnotationStringInternal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, const nsAString& aValue, int32_t aFlags, uint16_t aExpiration) { mozStorageTransaction transaction(mDB->MainConn(), false); nsCOMPtr<mozIStorageStatement> statement; - nsresult rv = StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration, + + nsresult rv = StartSetAnnotation(aURI, aItemId, aBookmark, aName, aFlags, + aExpiration, nsIAnnotationService::TYPE_STRING, statement); NS_ENSURE_SUCCESS(rv, rv); + mozStorageStatementScoper scoper(statement); rv = statement->BindStringByName(NS_LITERAL_CSTRING("content"), aValue); NS_ENSURE_SUCCESS(rv, rv); rv = statement->Execute(); NS_ENSURE_SUCCESS(rv, rv); @@ -351,17 +381,17 @@ NS_IMETHODIMP nsAnnotationService::SetPageAnnotationString(nsIURI* aURI, const nsACString& aName, const nsAString& aValue, int32_t aFlags, uint16_t aExpiration) { NS_ENSURE_ARG(aURI); - nsresult rv = SetAnnotationStringInternal(aURI, 0, aName, aValue, + nsresult rv = SetAnnotationStringInternal(aURI, 0, nullptr, aName, aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnPageAnnotationSet(aURI, aName)); return NS_OK; } @@ -375,40 +405,45 @@ nsAnnotationService::SetItemAnnotationSt uint16_t aSource, bool aDontUpdateLastModified) { NS_ENSURE_ARG_MIN(aItemId, 1); if (aExpiration == EXPIRE_WITH_HISTORY) return NS_ERROR_INVALID_ARG; - nsresult rv = SetAnnotationStringInternal(nullptr, aItemId, aName, aValue, - aFlags, aExpiration); + BookmarkData bookmark; + nsresult rv = SetAnnotationStringInternal(nullptr, aItemId, &bookmark, aName, + aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aItemId, aName, aSource, aDontUpdateLastModified)); + NotifyItemChanged(bookmark, aName, aSource, aDontUpdateLastModified); return NS_OK; } nsresult nsAnnotationService::SetAnnotationInt32Internal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, int32_t aValue, int32_t aFlags, uint16_t aExpiration) { mozStorageTransaction transaction(mDB->MainConn(), false); nsCOMPtr<mozIStorageStatement> statement; - nsresult rv = StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration, + nsresult rv = StartSetAnnotation(aURI, aItemId, aBookmark, aName, aFlags, + aExpiration, nsIAnnotationService::TYPE_INT32, statement); NS_ENSURE_SUCCESS(rv, rv); + mozStorageStatementScoper scoper(statement); rv = statement->BindInt32ByName(NS_LITERAL_CSTRING("content"), aValue); NS_ENSURE_SUCCESS(rv, rv); rv = statement->Execute(); NS_ENSURE_SUCCESS(rv, rv); @@ -423,17 +458,17 @@ NS_IMETHODIMP nsAnnotationService::SetPageAnnotationInt32(nsIURI* aURI, const nsACString& aName, int32_t aValue, int32_t aFlags, uint16_t aExpiration) { NS_ENSURE_ARG(aURI); - nsresult rv = SetAnnotationInt32Internal(aURI, 0, aName, aValue, + nsresult rv = SetAnnotationInt32Internal(aURI, 0, nullptr, aName, aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnPageAnnotationSet(aURI, aName)); return NS_OK; } @@ -447,40 +482,45 @@ nsAnnotationService::SetItemAnnotationIn uint16_t aSource, bool aDontUpdateLastModified) { NS_ENSURE_ARG_MIN(aItemId, 1); if (aExpiration == EXPIRE_WITH_HISTORY) return NS_ERROR_INVALID_ARG; - nsresult rv = SetAnnotationInt32Internal(nullptr, aItemId, aName, aValue, - aFlags, aExpiration); + BookmarkData bookmark; + nsresult rv = SetAnnotationInt32Internal(nullptr, aItemId, &bookmark, aName, + aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aItemId, aName, aSource, aDontUpdateLastModified)); + NotifyItemChanged(bookmark, aName, aSource, aDontUpdateLastModified); return NS_OK; } nsresult nsAnnotationService::SetAnnotationInt64Internal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, int64_t aValue, int32_t aFlags, uint16_t aExpiration) { mozStorageTransaction transaction(mDB->MainConn(), false); nsCOMPtr<mozIStorageStatement> statement; - nsresult rv = StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration, + nsresult rv = StartSetAnnotation(aURI, aItemId, aBookmark, aName, aFlags, + aExpiration, nsIAnnotationService::TYPE_INT64, statement); NS_ENSURE_SUCCESS(rv, rv); + mozStorageStatementScoper scoper(statement); rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("content"), aValue); NS_ENSURE_SUCCESS(rv, rv); rv = statement->Execute(); NS_ENSURE_SUCCESS(rv, rv); @@ -495,17 +535,17 @@ NS_IMETHODIMP nsAnnotationService::SetPageAnnotationInt64(nsIURI* aURI, const nsACString& aName, int64_t aValue, int32_t aFlags, uint16_t aExpiration) { NS_ENSURE_ARG(aURI); - nsresult rv = SetAnnotationInt64Internal(aURI, 0, aName, aValue, + nsresult rv = SetAnnotationInt64Internal(aURI, 0, nullptr, aName, aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnPageAnnotationSet(aURI, aName)); return NS_OK; } @@ -519,40 +559,45 @@ nsAnnotationService::SetItemAnnotationIn uint16_t aSource, bool aDontUpdateLastModified) { NS_ENSURE_ARG_MIN(aItemId, 1); if (aExpiration == EXPIRE_WITH_HISTORY) return NS_ERROR_INVALID_ARG; - nsresult rv = SetAnnotationInt64Internal(nullptr, aItemId, aName, aValue, - aFlags, aExpiration); + BookmarkData bookmark; + nsresult rv = SetAnnotationInt64Internal(nullptr, aItemId, &bookmark, aName, + aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aItemId, aName, aSource, aDontUpdateLastModified)); + NotifyItemChanged(bookmark, aName, aSource, aDontUpdateLastModified); return NS_OK; } nsresult nsAnnotationService::SetAnnotationDoubleInternal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, double aValue, int32_t aFlags, uint16_t aExpiration) { mozStorageTransaction transaction(mDB->MainConn(), false); nsCOMPtr<mozIStorageStatement> statement; - nsresult rv = StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration, + nsresult rv = StartSetAnnotation(aURI, aItemId, aBookmark, aName, aFlags, + aExpiration, nsIAnnotationService::TYPE_DOUBLE, statement); NS_ENSURE_SUCCESS(rv, rv); + mozStorageStatementScoper scoper(statement); rv = statement->BindDoubleByName(NS_LITERAL_CSTRING("content"), aValue); NS_ENSURE_SUCCESS(rv, rv); rv = statement->Execute(); NS_ENSURE_SUCCESS(rv, rv); @@ -567,17 +612,17 @@ NS_IMETHODIMP nsAnnotationService::SetPageAnnotationDouble(nsIURI* aURI, const nsACString& aName, double aValue, int32_t aFlags, uint16_t aExpiration) { NS_ENSURE_ARG(aURI); - nsresult rv = SetAnnotationDoubleInternal(aURI, 0, aName, aValue, + nsresult rv = SetAnnotationDoubleInternal(aURI, 0, nullptr, aName, aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnPageAnnotationSet(aURI, aName)); return NS_OK; } @@ -591,21 +636,23 @@ nsAnnotationService::SetItemAnnotationDo uint16_t aSource, bool aDontUpdateLastModified) { NS_ENSURE_ARG_MIN(aItemId, 1); if (aExpiration == EXPIRE_WITH_HISTORY) return NS_ERROR_INVALID_ARG; - nsresult rv = SetAnnotationDoubleInternal(nullptr, aItemId, aName, aValue, - aFlags, aExpiration); + BookmarkData bookmark; + nsresult rv = SetAnnotationDoubleInternal(nullptr, aItemId, &bookmark, + aName, aValue, aFlags, aExpiration); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aItemId, aName, aSource, aDontUpdateLastModified)); + NotifyItemChanged(bookmark, aName, aSource, aDontUpdateLastModified); return NS_OK; } NS_IMETHODIMP nsAnnotationService::GetPageAnnotationString(nsIURI* aURI, const nsACString& aName, nsAString& _retval) @@ -1362,16 +1409,17 @@ nsAnnotationService::ItemHasAnnotation(i /** * @note We don't remove anything from the moz_anno_attributes table. If we * delete the last item of a given name, that item really should go away. * It will be cleaned up by expiration. */ nsresult nsAnnotationService::RemoveAnnotationInternal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName) { bool isItemAnnotation = (aItemId > 0); nsCOMPtr<mozIStorageStatement> statement; if (isItemAnnotation) { statement = mDB->GetStatement( "DELETE FROM moz_items_annos " "WHERE item_id = :item_id " @@ -1399,46 +1447,58 @@ nsAnnotationService::RemoveAnnotationInt NS_ENSURE_SUCCESS(rv, rv); rv = statement->BindUTF8StringByName(NS_LITERAL_CSTRING("anno_name"), aName); NS_ENSURE_SUCCESS(rv, rv); rv = statement->Execute(); NS_ENSURE_SUCCESS(rv, rv); + if (isItemAnnotation) { + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); + if (bookmarks) { + MOZ_ASSERT(aBookmark); + if (NS_FAILED(bookmarks->FetchItemInfo(aItemId, *aBookmark))) { + aBookmark->id = -1; + } + } + } + return NS_OK; } NS_IMETHODIMP nsAnnotationService::RemovePageAnnotation(nsIURI* aURI, const nsACString& aName) { NS_ENSURE_ARG(aURI); - nsresult rv = RemoveAnnotationInternal(aURI, 0, aName); + nsresult rv = RemoveAnnotationInternal(aURI, 0, nullptr, aName); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnPageAnnotationRemoved(aURI, aName)); return NS_OK; } NS_IMETHODIMP nsAnnotationService::RemoveItemAnnotation(int64_t aItemId, const nsACString& aName, uint16_t aSource) { NS_ENSURE_ARG_MIN(aItemId, 1); - nsresult rv = RemoveAnnotationInternal(nullptr, aItemId, aName); + BookmarkData bookmark; + nsresult rv = RemoveAnnotationInternal(nullptr, aItemId, &bookmark, aName); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationRemoved(aItemId, aName, aSource)); + NotifyItemChanged(bookmark, aName, aSource, false); return NS_OK; } NS_IMETHODIMP nsAnnotationService::RemovePageAnnotations(nsIURI* aURI) { @@ -1464,34 +1524,44 @@ nsAnnotationService::RemovePageAnnotatio return NS_OK; } NS_IMETHODIMP nsAnnotationService::RemoveItemAnnotations(int64_t aItemId, uint16_t aSource) { + nsresult rv = RemoveItemAnnotationsWithoutNotifying(aItemId); + NS_ENSURE_SUCCESS(rv, rv); + + NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationRemoved(aItemId, EmptyCString(), + aSource)); + + return NS_OK; +} + + +nsresult +nsAnnotationService::RemoveItemAnnotationsWithoutNotifying(int64_t aItemId) +{ NS_ENSURE_ARG_MIN(aItemId, 1); // Should this be precompiled or a getter? nsCOMPtr<mozIStorageStatement> statement = mDB->GetStatement( "DELETE FROM moz_items_annos WHERE item_id = :item_id" ); NS_ENSURE_STATE(statement); mozStorageStatementScoper scoper(statement); nsresult rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId); NS_ENSURE_SUCCESS(rv, rv); rv = statement->Execute(); NS_ENSURE_SUCCESS(rv, rv); - NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationRemoved(aItemId, EmptyCString(), - aSource)); - return NS_OK; } /** * @note If we use annotations for some standard items like GeckoFlags, it * might be a good idea to blacklist these standard annotations from this * copy function. @@ -1644,16 +1714,24 @@ nsAnnotationService::CopyItemAnnotations NS_ENSURE_SUCCESS(rv, rv); rv = copyStmt->BindInt64ByName(NS_LITERAL_CSTRING("date"), PR_Now()); NS_ENSURE_SUCCESS(rv, rv); rv = copyStmt->Execute(); NS_ENSURE_SUCCESS(rv, rv); NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aDestItemId, annoName, aSource, false)); + + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); + if (bookmarks) { + BookmarkData bookmark; + if (NS_SUCCEEDED(bookmarks->FetchItemInfo(aDestItemId, bookmark))) { + NotifyItemChanged(bookmark, annoName, aSource, false); + } + } } rv = transaction.Commit(); NS_ENSURE_SUCCESS(rv, rv); return NS_OK; } @@ -1833,16 +1911,17 @@ nsAnnotationService::StartGetAnnotation( * * @note The aStatement RESULT IS NOT ADDREFED. This is just one of the class * vars, which control its scope. DO NOT RELEASE. * The caller must take care of resetting the statement if this succeeds. */ nsresult nsAnnotationService::StartSetAnnotation(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, int32_t aFlags, uint16_t aExpiration, uint16_t aType, nsCOMPtr<mozIStorageStatement>& aStatement) { bool isItemAnnotation = (aItemId > 0); @@ -1870,18 +1949,19 @@ nsAnnotationService::StartSetAnnotation( // - whether the annotation already exists. // - the nameID associated with the annotation name. // - the id and dateAdded of the old annotation, if it exists. nsCOMPtr<mozIStorageStatement> stmt; if (isItemAnnotation) { stmt = mDB->GetStatement( "SELECT b.id, " "(SELECT id FROM moz_anno_attributes WHERE name = :anno_name) AS nameid, " - "a.id, a.dateAdded " + "a.id, a.dateAdded, b.parent, b.type, b.lastModified, b.guid, p.guid " "FROM moz_bookmarks b " + "JOIN moz_bookmarks p ON p.id = b.parent " "LEFT JOIN moz_items_annos a ON a.item_id = b.id " "AND a.anno_attribute_id = nameid " "WHERE b.id = :item_id" ); } else { stmt = mDB->GetStatement( "SELECT h.id, " @@ -1921,16 +2001,29 @@ nsAnnotationService::StartSetAnnotation( if (isItemAnnotation) { aStatement = mDB->GetStatement( "INSERT OR REPLACE INTO moz_items_annos " "(id, item_id, anno_attribute_id, content, flags, " "expiration, type, dateAdded, lastModified) " "VALUES (:id, :fk, :name_id, :content, :flags, " ":expiration, :type, :date_added, :last_modified)" ); + + // Since we're already querying `moz_bookmarks`, we fetch the changed + // bookmark's info here, instead of using `FetchItemInfo`. + MOZ_ASSERT(aBookmark); + aBookmark->id = fkId; + aBookmark->parentId = stmt->AsInt64(4); + aBookmark->type = stmt->AsInt64(5); + + aBookmark->lastModified = static_cast<PRTime>(stmt->AsInt64(6)); + if (NS_FAILED(stmt->GetUTF8String(7, aBookmark->guid)) || + NS_FAILED(stmt->GetUTF8String(8, aBookmark->parentGuid))) { + aBookmark->id = -1; + } } else { aStatement = mDB->GetStatement( "INSERT OR REPLACE INTO moz_annos " "(id, place_id, anno_attribute_id, content, flags, " "expiration, type, dateAdded, lastModified) " "VALUES (:id, :fk, :name_id, :content, :flags, " ":expiration, :type, :date_added, :last_modified)"
--- a/toolkit/components/places/nsAnnotationService.h +++ b/toolkit/components/places/nsAnnotationService.h @@ -108,54 +108,61 @@ protected: nsresult StartGetAnnotation(nsIURI* aURI, int64_t aItemId, const nsACString& aName, nsCOMPtr<mozIStorageStatement>& aStatement); nsresult StartSetAnnotation(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, int32_t aFlags, uint16_t aExpiration, uint16_t aType, nsCOMPtr<mozIStorageStatement>& aStatement); nsresult SetAnnotationStringInternal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, const nsAString& aValue, int32_t aFlags, uint16_t aExpiration); nsresult SetAnnotationInt32Internal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, int32_t aValue, int32_t aFlags, uint16_t aExpiration); nsresult SetAnnotationInt64Internal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, int64_t aValue, int32_t aFlags, uint16_t aExpiration); nsresult SetAnnotationDoubleInternal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName, double aValue, int32_t aFlags, uint16_t aExpiration); nsresult RemoveAnnotationInternal(nsIURI* aURI, int64_t aItemId, + BookmarkData* aBookmark, const nsACString& aName); public: nsresult GetPagesWithAnnotationCOMArray(const nsACString& aName, nsCOMArray<nsIURI>* _results); nsresult GetItemsWithAnnotationTArray(const nsACString& aName, nsTArray<int64_t>* _result); nsresult GetAnnotationNamesTArray(nsIURI* aURI, int64_t aItemId, nsTArray<nsCString>* _result); + nsresult RemoveItemAnnotationsWithoutNotifying(int64_t aItemId); }; #endif /* nsAnnotationService_h___ */
--- a/toolkit/components/places/nsIAnnotationService.idl +++ b/toolkit/components/places/nsIAnnotationService.idl @@ -107,17 +107,18 @@ interface nsIAnnotationService : nsISupp * If there is an important annotation that the user or extension wants to * keep, you should add a bookmark for the page and use an EXPIRE_NEVER * annotation. This will ensure the annotation exists until the item is * removed by the user. * See EXPIRE_* constants above for further information. * * For item annotations, aSource should be a change source constant from * nsINavBookmarksService::SOURCE_*, and defaults to SOURCE_DEFAULT if - * omitted. + * omitted. Setting an item annotation also notifies + * `nsINavBookmarkObserver::onItemChanged` for the affected item. * * The annotation "favicon" is special. Favicons are stored in the favicon * service, but are special cased in the protocol handler so they look like * annotations. Do not set favicons using this service, it will not work. * * Only C++ consumers may use the type-specific methods. * * @throws NS_ERROR_ILLEGAL_VALUE if the page or the bookmark doesn't exist. @@ -335,27 +336,33 @@ interface nsIAnnotationService : nsISupp boolean pageHasAnnotation(in nsIURI aURI, in AUTF8String aName); boolean itemHasAnnotation(in long long aItemId, in AUTF8String aName); /** * Removes a specific annotation. Succeeds even if the annotation is * not found. + * + * Removing an item annotation also notifies + * `nsINavBookmarkObserver::onItemChanged` for the affected item. */ void removePageAnnotation(in nsIURI aURI, in AUTF8String aName); void removeItemAnnotation(in long long aItemId, in AUTF8String aName, [optional] in unsigned short aSource); /** * Removes all annotations for the given page/item. * We may want some other similar functions to get annotations with given * flags (once we have flags defined). + * + * Unlike the other item methods, `removeItemAnnotations` does *not* notify + * `nsINavBookmarkObserver::onItemChanged` for the affected item. */ void removePageAnnotations(in nsIURI aURI); void removeItemAnnotations(in long long aItemId, [optional] in unsigned short aSource); /** * Copies all annotations from the source to the destination URI/item. If * the destination already has an annotation with the same name as one on
--- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -179,17 +179,16 @@ nsNavBookmarks::~nsNavBookmarks() if (gBookmarksService == this) gBookmarksService = nullptr; } NS_IMPL_ISUPPORTS(nsNavBookmarks , nsINavBookmarksService , nsINavHistoryObserver -, nsIAnnotationObserver , nsIObserver , nsISupportsWeakReference ) Atomic<int64_t> nsNavBookmarks::sLastInsertedItemId(0); @@ -204,27 +203,21 @@ nsNavBookmarks::StoreLastInsertedId(cons nsresult nsNavBookmarks::Init() { mDB = Database::GetDatabase(); NS_ENSURE_STATE(mDB); nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); if (os) { - (void)os->AddObserver(this, TOPIC_PLACES_SHUTDOWN, true); (void)os->AddObserver(this, TOPIC_PLACES_CONNECTION_CLOSED, true); } mCanNotify = true; - // Observe annotations. - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); - annosvc->AddObserver(this); - // Allows us to notify on title changes. MUST BE LAST so it is impossible // to fail after this call, or the history service will have a reference to // us and we won't go away. nsNavHistory* history = nsNavHistory::GetHistoryService(); NS_ENSURE_STATE(history); history->AddObserver(this, true); // DO NOT PUT STUFF HERE that can fail. See observer comment above. @@ -682,23 +675,25 @@ nsNavBookmarks::RemoveItem(int64_t aItem NS_ENSURE_ARG(!IsRoot(aItemId)); BookmarkData bookmark; nsresult rv = FetchItemInfo(aItemId, bookmark); NS_ENSURE_SUCCESS(rv, rv); mozStorageTransaction transaction(mDB->MainConn(), false); - // First, if not a tag, remove item annotations. + // First, if not a tag, remove item annotations. We remove annos without + // notifying to avoid firing `onItemAnnotationRemoved` for an item that + // we're about to remove. int64_t tagsRootId = TagsRootId(); bool isUntagging = bookmark.grandParentId == tagsRootId; if (bookmark.parentId != tagsRootId && !isUntagging) { nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); - rv = annosvc->RemoveItemAnnotations(bookmark.id, aSource); + rv = annosvc->RemoveItemAnnotationsWithoutNotifying(bookmark.id); NS_ENSURE_SUCCESS(rv, rv); } if (bookmark.type == TYPE_FOLDER) { // Remove all of the folder's children. rv = RemoveFolderChildren(bookmark.id, aSource); NS_ENSURE_SUCCESS(rv, rv); } @@ -3087,52 +3082,50 @@ nsNavBookmarks::NotifyItemVisited(const } } void nsNavBookmarks::NotifyItemChanged(const ItemChangeData& aData) { // A guid must always be defined. MOZ_ASSERT(!aData.bookmark.guid.IsEmpty()); + + PRTime lastModified = aData.bookmark.lastModified; + if (aData.updateLastModified) { + lastModified = RoundedPRNow(); + MOZ_ALWAYS_SUCCEEDS(SetItemDateInternal( + LAST_MODIFIED, DetermineSyncChangeDelta(aData.source), + aData.bookmark.id, lastModified)); + } + NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver, OnItemChanged(aData.bookmark.id, aData.property, aData.isAnnotation, aData.newValue, - aData.bookmark.lastModified, + lastModified, aData.bookmark.type, aData.bookmark.parentId, aData.bookmark.guid, aData.bookmark.parentGuid, aData.oldValue, - // We specify the default source here because - // this method is only called for history - // visits, and we don't track sources in - // history. - SOURCE_DEFAULT)); + aData.source)); } //////////////////////////////////////////////////////////////////////////////// //// nsIObserver NS_IMETHODIMP nsNavBookmarks::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData) { NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread"); - if (strcmp(aTopic, TOPIC_PLACES_SHUTDOWN) == 0) { - // Stop Observing annotations. - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - if (annosvc) { - annosvc->RemoveObserver(this); - } - } - else if (strcmp(aTopic, TOPIC_PLACES_CONNECTION_CLOSED) == 0) { + if (strcmp(aTopic, TOPIC_PLACES_CONNECTION_CLOSED) == 0) { // Don't even try to notify observers from this point on, the category // cache would init services that could try to use our APIs. mCanNotify = false; mObservers.Clear(); } return NS_OK; } @@ -3301,68 +3294,8 @@ nsNavBookmarks::OnDeleteVisits(nsIURI* a changeData.bookmark.type = TYPE_BOOKMARK; RefPtr< AsyncGetBookmarksForURI<ItemChangeMethod, ItemChangeData> > notifier = new AsyncGetBookmarksForURI<ItemChangeMethod, ItemChangeData>(this, &nsNavBookmarks::NotifyItemChanged, changeData); notifier->Init(); } return NS_OK; } - - -// nsIAnnotationObserver - -NS_IMETHODIMP -nsNavBookmarks::OnPageAnnotationSet(nsIURI* aPage, const nsACString& aName) -{ - return NS_OK; -} - - -NS_IMETHODIMP -nsNavBookmarks::OnItemAnnotationSet(int64_t aItemId, const nsACString& aName, - uint16_t aSource, bool aDontUpdateLastModified) -{ - BookmarkData bookmark; - nsresult rv = FetchItemInfo(aItemId, bookmark); - NS_ENSURE_SUCCESS(rv, rv); - - if (!aDontUpdateLastModified) { - bookmark.lastModified = RoundedPRNow(); - rv = SetItemDateInternal(LAST_MODIFIED, DetermineSyncChangeDelta(aSource), - bookmark.id, bookmark.lastModified); - NS_ENSURE_SUCCESS(rv, rv); - } - - NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers, - nsINavBookmarkObserver, - OnItemChanged(bookmark.id, - aName, - true, - EmptyCString(), - bookmark.lastModified, - bookmark.type, - bookmark.parentId, - bookmark.guid, - bookmark.parentGuid, - EmptyCString(), - aSource)); - return NS_OK; -} - - -NS_IMETHODIMP -nsNavBookmarks::OnPageAnnotationRemoved(nsIURI* aPage, const nsACString& aName) -{ - return NS_OK; -} - - -NS_IMETHODIMP -nsNavBookmarks::OnItemAnnotationRemoved(int64_t aItemId, const nsACString& aName, - uint16_t aSource) -{ - // As of now this is doing the same as OnItemAnnotationSet, so just forward - // the call. - nsresult rv = OnItemAnnotationSet(aItemId, aName, aSource, false); - NS_ENSURE_SUCCESS(rv, rv); - return NS_OK; -}
--- a/toolkit/components/places/nsNavBookmarks.h +++ b/toolkit/components/places/nsNavBookmarks.h @@ -2,17 +2,16 @@ /* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ #ifndef nsNavBookmarks_h_ #define nsNavBookmarks_h_ #include "nsINavBookmarksService.h" -#include "nsIAnnotationService.h" #include "nsITransaction.h" #include "nsNavHistory.h" #include "nsToolkitCompsCID.h" #include "nsCategoryCache.h" #include "nsTHashtable.h" #include "nsWeakReference.h" #include "mozilla/Attributes.h" #include "prtime.h" @@ -23,43 +22,45 @@ namespace mozilla { namespace places { enum BookmarkStatementId { DB_FIND_REDIRECTED_BOOKMARK = 0 , DB_GET_BOOKMARKS_FOR_URI }; struct BookmarkData { - int64_t id; + int64_t id = -1; nsCString url; nsCString title; - int32_t position; - int64_t placeId; - int64_t parentId; - int64_t grandParentId; - int32_t type; - int32_t syncStatus; + int32_t position = -1; + int64_t placeId = -1; + int64_t parentId = -1; + int64_t grandParentId = -1; + int32_t type = 0; + int32_t syncStatus = nsINavBookmarksService::SYNC_STATUS_UNKNOWN; nsCString serviceCID; - PRTime dateAdded; - PRTime lastModified; + PRTime dateAdded = 0; + PRTime lastModified = 0; nsCString guid; nsCString parentGuid; }; struct ItemVisitData { BookmarkData bookmark; int64_t visitId; uint32_t transitionType; PRTime time; }; struct ItemChangeData { BookmarkData bookmark; + bool isAnnotation = false; + bool updateLastModified = false; + uint16_t source = nsINavBookmarksService::SOURCE_DEFAULT; nsCString property; - bool isAnnotation; nsCString newValue; nsCString oldValue; }; struct TombstoneData { nsCString guid; PRTime dateRemoved; }; @@ -72,25 +73,23 @@ namespace places { , LAST_MODIFIED }; } // namespace places } // namespace mozilla class nsNavBookmarks final : public nsINavBookmarksService , public nsINavHistoryObserver - , public nsIAnnotationObserver , public nsIObserver , public nsSupportsWeakReference { public: NS_DECL_ISUPPORTS NS_DECL_NSINAVBOOKMARKSSERVICE NS_DECL_NSINAVHISTORYOBSERVER - NS_DECL_NSIANNOTATIONOBSERVER NS_DECL_NSIOBSERVER nsNavBookmarks(); /** * Obtains the service's object. */ static already_AddRefed<nsNavBookmarks> GetSingleton();
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js @@ -226,18 +226,16 @@ add_task(async function remove_bookmark( let bm = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK, parentGuid: PlacesUtils.bookmarks.unfiledGuid, url: new URL("http://remove.example.com/") }); let itemId = await PlacesUtils.promiseItemId(bm.guid); let parentId = await PlacesUtils.promiseItemId(bm.parentGuid); let observer = expectNotifications(); await PlacesUtils.bookmarks.remove(bm.guid); - // TODO (Bug 653910): onItemAnnotationRemoved notified even if there were no - // annotations. observer.check([ { name: "onItemRemoved", arguments: [ itemId, parentId, bm.index, bm.type, bm.url, bm.guid, bm.parentGuid, Ci.nsINavBookmarksService.SOURCE_DEFAULT ] } ]); }); add_task(async function remove_multiple_bookmarks() { @@ -249,18 +247,16 @@ add_task(async function remove_multiple_ url: "http://remove1.example.com/" }); let itemId1 = await PlacesUtils.promiseItemId(bm1.guid); let parentId1 = await PlacesUtils.promiseItemId(bm1.parentGuid); let itemId2 = await PlacesUtils.promiseItemId(bm2.guid); let parentId2 = await PlacesUtils.promiseItemId(bm2.parentGuid); let observer = expectNotifications(); await PlacesUtils.bookmarks.remove([bm1, bm2]); - // TODO (Bug 653910): onItemAnnotationRemoved notified even if there were no - // annotations. observer.check([ { name: "onItemRemoved", arguments: [ itemId1, parentId1, bm1.index, bm1.type, bm1.url, bm1.guid, bm1.parentGuid, Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }, { name: "onItemRemoved", arguments: [ itemId2, parentId2, bm2.index, bm2.type, bm2.url, bm2.guid, bm2.parentGuid, Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }
--- a/toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js +++ b/toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js @@ -206,17 +206,17 @@ add_task(async function onItemAdded_fold add_task(async function onItemChanged_title_bookmark() { let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, 0); const TITLE = "New title"; let promise = Promise.all([ gBookmarkSkipObserver.setup([ "onItemChanged" ]), gBookmarksObserver.setup([ - { name: "onItemChanged", // This is an unfortunate effect of bug 653910. + { name: "onItemChanged", args: [ { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, { name: "property", check: v => v === "title" }, { name: "isAnno", check: v => v === false }, { name: "newValue", check: v => v === TITLE }, { name: "lastModified", check: v => typeof(v) == "number" && v > 0 }, { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_BOOKMARK }, { name: "parentId", check: v => v === PlacesUtils.unfiledBookmarksFolderId }, @@ -388,33 +388,19 @@ add_task(async function onItemMoved_book add_task(async function onItemRemoved_bookmark() { let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, 0); let guid = await PlacesUtils.promiseItemGuid(id); let url = (await PlacesUtils.bookmarks.fetch(guid)).url; let uri = Services.io.newURI(url); let promise = Promise.all([ gBookmarkSkipObserver.setup([ - "onItemChanged", "onItemRemoved" + "onItemRemoved" ]), gBookmarksObserver.setup([ - { name: "onItemChanged", // This is an unfortunate effect of bug 653910. - args: [ - { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, - { name: "property", check: v => v === "" }, - { name: "isAnno", check: v => v === true }, - { name: "newValue", check: v => v === "" }, - { name: "lastModified", check: v => typeof(v) == "number" && v > 0 }, - { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_BOOKMARK }, - { name: "parentId", check: v => v === PlacesUtils.unfiledBookmarksFolderId }, - { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "parentGuid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "oldValue", check: v => typeof(v) == "string" }, - { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) }, - ] }, { name: "onItemRemoved", args: [ { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, { name: "parentId", check: v => v === PlacesUtils.unfiledBookmarksFolderId }, { name: "index", check: v => v === 0 }, { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_BOOKMARK }, { name: "uri", check: v => v instanceof Ci.nsIURI && v.equals(uri) }, { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, @@ -425,33 +411,19 @@ add_task(async function onItemRemoved_bo PlacesUtils.bookmarks.removeItem(id); await promise; }); add_task(async function onItemRemoved_separator() { let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, 0); let promise = Promise.all([ gBookmarkSkipObserver.setup([ - "onItemChanged", "onItemRemoved" + "onItemRemoved" ]), gBookmarksObserver.setup([ - { name: "onItemChanged", // This is an unfortunate effect of bug 653910. - args: [ - { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, - { name: "property", check: v => v === "" }, - { name: "isAnno", check: v => v === true }, - { name: "newValue", check: v => v === "" }, - { name: "lastModified", check: v => typeof(v) == "number" && v > 0 }, - { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_SEPARATOR }, - { name: "parentId", check: v => typeof(v) == "number" && v > 0 }, - { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "parentGuid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "oldValue", check: v => typeof(v) == "string" }, - { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) }, - ] }, { name: "onItemRemoved", args: [ { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, { name: "parentId", check: v => typeof(v) == "number" && v > 0 }, { name: "index", check: v => v === 0 }, { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_SEPARATOR }, { name: "uri", check: v => v === null }, { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, @@ -462,33 +434,19 @@ add_task(async function onItemRemoved_se PlacesUtils.bookmarks.removeItem(id); await promise; }); add_task(async function onItemRemoved_folder() { let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, 0); let promise = Promise.all([ gBookmarkSkipObserver.setup([ - "onItemChanged", "onItemRemoved" + "onItemRemoved" ]), gBookmarksObserver.setup([ - { name: "onItemChanged", // This is an unfortunate effect of bug 653910. - args: [ - { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, - { name: "property", check: v => v === "" }, - { name: "isAnno", check: v => v === true }, - { name: "newValue", check: v => v === "" }, - { name: "lastModified", check: v => typeof(v) == "number" && v > 0 }, - { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_FOLDER }, - { name: "parentId", check: v => typeof(v) == "number" && v > 0 }, - { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "parentGuid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "oldValue", check: v => typeof(v) == "string" }, - { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) }, - ] }, { name: "onItemRemoved", args: [ { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, { name: "parentId", check: v => typeof(v) == "number" && v > 0 }, { name: "index", check: v => v === 0 }, { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_FOLDER }, { name: "uri", check: v => v === null }, { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, @@ -502,17 +460,17 @@ add_task(async function onItemRemoved_fo add_task(async function onItemRemoved_folder_recursive() { const TITLE = "Folder 3"; const BMTITLE = "Bookmark 1"; let uri = NetUtil.newURI("http://1.mozilla.org/"); let promise = Promise.all([ gBookmarkSkipObserver.setup([ "onItemAdded", "onItemAdded", "onItemAdded", "onItemAdded", - "onItemChanged", "onItemRemoved" + "onItemRemoved" ]), gBookmarksObserver.setup([ { name: "onItemAdded", args: [ { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, { name: "parentId", check: v => v === PlacesUtils.unfiledBookmarksFolderId }, { name: "index", check: v => v === 0 }, { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_FOLDER }, @@ -557,30 +515,16 @@ add_task(async function onItemRemoved_fo { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_BOOKMARK }, { name: "uri", check: v => v instanceof Ci.nsIURI && v.equals(uri) }, { name: "title", check: v => v === BMTITLE }, { name: "dateAdded", check: v => typeof(v) == "number" && v > 0 }, { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, { name: "parentGuid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) }, ] }, - { name: "onItemChanged", // This is an unfortunate effect of bug 653910. - args: [ - { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, - { name: "property", check: v => v === "" }, - { name: "isAnno", check: v => v === true }, - { name: "newValue", check: v => v === "" }, - { name: "lastModified", check: v => typeof(v) == "number" && v > 0 }, - { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_FOLDER }, - { name: "parentId", check: v => typeof(v) == "number" && v > 0 }, - { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "parentGuid", check: v => typeof(v) == "string" && GUID_RE.test(v) }, - { name: "oldValue", check: v => typeof(v) == "string" }, - { name: "source", check: v => Object.values(PlacesUtils.bookmarks.SOURCES).includes(v) }, - ] }, { name: "onItemRemoved", args: [ { name: "itemId", check: v => typeof(v) == "number" && v > 0 }, { name: "parentId", check: v => typeof(v) == "number" && v > 0 }, { name: "index", check: v => v === 0 }, { name: "itemType", check: v => v === PlacesUtils.bookmarks.TYPE_BOOKMARK }, { name: "uri", check: v => v instanceof Ci.nsIURI && v.equals(uri) }, { name: "guid", check: v => typeof(v) == "string" && GUID_RE.test(v) },
--- a/toolkit/components/places/tests/unit/test_null_interfaces.js +++ b/toolkit/components/places/tests/unit/test_null_interfaces.js @@ -9,19 +9,18 @@ // Make an array of services to test, each specifying a class id, interface // and an array of function names that don't throw when passed nulls var testServices = [ ["browser/nav-history-service;1", ["nsINavHistoryService"], ["queryStringToQueries", "removePagesByTimeframe", "removePagesFromHost", "getObservers"] ], ["browser/nav-bookmarks-service;1", - ["nsINavBookmarksService", "nsINavHistoryObserver", "nsIAnnotationObserver"], - ["createFolder", "getObservers", "onFrecencyChanged", "onTitleChanged", - "onPageAnnotationSet", "onPageAnnotationRemoved", "onDeleteURI"] + ["nsINavBookmarksService", "nsINavHistoryObserver"], + ["createFolder", "getObservers", "onFrecencyChanged", "onTitleChanged", "onDeleteURI"] ], ["browser/livemark-service;2", ["mozIAsyncLivemarks"], ["reloadLivemarks"]], ["browser/annotation-service;1", ["nsIAnnotationService"], ["getObservers"]], ["browser/favicon-service;1", ["nsIFaviconService"], []], ["browser/tagging-service;1", ["nsITaggingService"], []], ]; do_print(testServices.join("\n"));