Closed Bug 1303405 Opened 8 years ago Closed 8 years ago

`RemoveFolderTransaction::UndoTransaction` passes an empty GUID to `onItemAdded`

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.
58 bytes, text/x-review-board-request
mak
: review+
Details 
`undoTransaction` currently reinserts the folder with the same ID, but a different GUID: http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/toolkit/components/places/nsNavBookmarks.h#394-396 It's not clear to me if that's another bug, and it should actually use the same GUID, but the existing behavior has a subtle bug.

http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/toolkit/components/places/nsNavBookmarks.cpp#407-408 only fetches the ID and GUID if *_itemId == -1, which isn't true if we're reinserting. This fires `onItemAdded` with an empty GUID, confusing the GuidHelper's observer (http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/toolkit/components/places/PlacesUtils.jsm#2512-2513).
Comment on attachment 8792071 [details]
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.

https://reviewboard.mozilla.org/r/79298/#review79402

::: toolkit/components/places/nsNavBookmarks.cpp:415
(Diff revision 2)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (*_itemId == -1) {
>      // Get the newly inserted item id and GUID.
>      nsCOMPtr<mozIStorageStatement> lastInsertIdStmt = mDB->GetStatement(
>        "SELECT id, guid "

looks like there's no more need to fetch the guid here.

Actually, for History I went a little bit further (with the idea one day to do the same for bookmarks) and implemented the sql function store_last_inserted_id.

it could be easily extended using the CREATE_BOOKMARKS_FOREIGNCOUNT_AFTERINSERT_TRIGGER, so that on every INSERT we store the last bookmark id, and then here we could avoid a SELECT query completley and use sLastInsertedBookmarkId;

You could clearly either finally fix that, or ignore me, it's not critical but may be a good thing to do.

::: toolkit/components/places/tests/bookmarks/test_1303405.js:26
(Diff revision 2)
> +    folderId,
> +    uri("http://getthunderbird.com"),
> +    PlacesUtils.bookmarks.DEFAULT_INDEX,
> +    "Get Thunderbird!"
> +  );
> +  let tbGuid = yield PlacesUtils.promiseItemGuid(tbId);

Afaict, you can use the new API here, indeed I'd suggest that. We should not use the old API in new code unless it's critically needed.
We already have enough consumers to convert :)

::: toolkit/components/places/tests/bookmarks/xpcshell.ini:30
(Diff revision 2)
>  [test_818587_compress-bookmarks-backups.js]
>  [test_818593-store-backup-metadata.js]
>  [test_992901-backup-unsorted-hierarchy.js]
>  [test_997030-bookmarks-html-encode.js]
>  [test_1129529.js]
> +[test_1303405.js]

if possible, please give tests a meaningful name. Adding the bug# is optional, we decided some time ago to stop naming tests after bug# cause it complicates finding tests relevant to something.
Comment on attachment 8792071 [details]
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.

https://reviewboard.mozilla.org/r/79298/#review79408

I'd like to have a very quick look at a revised patch. The lastInsert bits are optional, but bonu points if implemented.
Attachment #8792071 - Flags: review?(mak77)
Comment on attachment 8792071 [details]
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.

https://reviewboard.mozilla.org/r/79298/#review79504

it looks great!

::: toolkit/components/places/nsNavBookmarks.h:218
(Diff revision 3)
>  
>    static const int32_t kGetChildrenIndex_Guid;
>    static const int32_t kGetChildrenIndex_Position;
>    static const int32_t kGetChildrenIndex_Type;
>    static const int32_t kGetChildrenIndex_PlaceID;
> +  static mozilla::Atomic<int64_t> sLastInsertedItemId;

nit: Add newline before it, since it's not related to the previous properties.

::: toolkit/components/places/nsNavBookmarks.h:220
(Diff revision 3)
>    static const int32_t kGetChildrenIndex_Position;
>    static const int32_t kGetChildrenIndex_Type;
>    static const int32_t kGetChildrenIndex_PlaceID;
> +  static mozilla::Atomic<int64_t> sLastInsertedItemId;
> +
> +  static void StoreLastInsertedId(const nsACString& aTable,

nit: while I don't care much abour a newline here, since the 2 are strictly related

::: toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js:14
(Diff revision 3)
> +    parentGuid: PlacesUtils.bookmarks.menuGuid,
> +    title: "Test folder",
> +  });
> +  let folderId = yield PlacesUtils.promiseItemId(folder.guid);
> +  let fx = yield PlacesUtils.bookmarks.insert({
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

nit: when url is defined, type is optional (url is enough to know it's a bookmark)

::: toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js:21
(Diff revision 3)
> +    title: "Get Firefox!",
> +    url: "http://getfirefox.com",
> +  });
> +  let fxId = yield PlacesUtils.promiseItemId(fx.guid);
> +  let tb = yield PlacesUtils.bookmarks.insert({
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

ditto

::: toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js:42
(Diff revision 3)
> +      notifications.push(["onItemAdded", itemId, parentId, guid, parentGuid]);
> +    },
> +    onItemRemoved(itemId, parentId, index, type, uri, guid, parentGuid) {
> +      notifications.push(["onItemRemoved", itemId, parentId, guid, parentGuid]);
> +    },
> +  }, false);

somewhere this observer should be removed...
Attachment #8792071 - Flags: review?(mak77) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2146f5e5c5df
Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers. r=mak
https://hg.mozilla.org/mozilla-central/rev/2146f5e5c5df
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: