914687 - API for presetting GUIDs on bookmarks

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

Attachment: presetGUIs.diff

So as it turns out setting guids directly into the db isn't an options because the initial guid is exposed through observers and results.

NOTE: for some reason the last test fails. On that now.

Comment 1

[Asaf Romano (gone)]

Attachment: tests pass

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 802402 [details] [diff] [review]
tests pass

Review of attachment 802402 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsINavBookmarksService.idl
@@ +277,5 @@
>     *  @param aTitle
>     *         The title for the new bookmark
> +   *  @param [optional] aGUID
> +   *         The GUID to be set for the new item.  If not set,
> +   *         a new GUID is generated.

I think it would be nice to scare people thinking to (ab)use this, stating that generally it's a bad idea to pass a guid!

@@ +305,5 @@
>     *  @param aIndex
>     *         The index to insert at, or DEFAULT_INDEX to append
> +   *  @param [optional] aGUID
> +   *         The GUID to be set for the new item.  If not set,
> +   *         a new GUID is generated.

ditto

@@ +363,5 @@
>     *  @param aIndex
>     *         The separator's index under folder, or DEFAULT_INDEX to append
> +   *  @param [optional] aGUID
> +   *         The GUID to be set for the new item.  If not set,
> +   *         a new GUID is generated.

ditto

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +481,5 @@
>      rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("last_modified"), aDateAdded);
>    }
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Could use IsEmpty becuase our callers check for GUID validity,

typo: becuase

@@ +482,5 @@
>    }
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Could use IsEmpty becuase our callers check for GUID validity,
> +  // but it doesn't hurt.

I'd probably also MOZ_ASSERT an additional IsValidGUID check, it doesn't hurt :)

::: toolkit/components/places/nsNavBookmarks.h
@@ +396,5 @@
>        nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>        NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>        int64_t newFolder;
>        return bookmarks->CreateContainerWithID(mID, mParent, mTitle, true,
> +                                              &mIndex, EmptyCString(), &newFolder); 

while here please fix the trailing space

::: toolkit/components/places/tests/unit/test_pageGuid_bookmarkGuid.js
@@ +163,5 @@
> +                       "1 title", BOOKMARK_GUID);
> +  bmsvc.insertSeparator(folder, bmsvc.DEFAULT_INDEX, SEPARATOR_GUID);
> +
> +  let result = PlacesUtils.getFolderContents(folder);
> +  let root = result.root;

can do in one step

@@ +164,5 @@
> +  bmsvc.insertSeparator(folder, bmsvc.DEFAULT_INDEX, SEPARATOR_GUID);
> +
> +  let result = PlacesUtils.getFolderContents(folder);
> +  let root = result.root;
> +  root.containerOpen = true;

iirc getFolderContents returns an already open container

@@ +179,5 @@
> +add_task(function test_emptyGUIDIgnored() {
> +  let folder = bmsvc.createFolder(bmsvc.placesRoot, "test folder",
> +                                  bmsvc.DEFAULT_INDEX, "");
> +  do_check_eq(PlacesUtils.getFolderContents(folder)
> +                         .root.bookmarkGuid.length, 12);

you can use do_check_valid_places_guid() that is in head_common.js

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 802402 [details] [diff] [review]
tests pass

Do we ever make any attempt at detecting duplicate GUIDs? Does the DB schema enforce the uniqueness somehow?

Comment 4

[Asaf Romano (gone)]

Yes, it's enforced (and that's covered by the provided test).

Comment 5

[Marco Bonardo [:mak]]

there's a unique index on all guids

Comment 6

[Asaf Romano (gone)]

Attachment: for checkin

The tree is closed right now.

Comment 7

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/424d6f2a4ea7

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/424d6f2a4ea7