Closed Bug 914687 Opened 11 years ago Closed 11 years ago

API for presetting GUIDs on bookmarks

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: asaf, Assigned: asaf)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch presetGUIs.diff (obsolete) — Splinter Review
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.
Attachment #802397 - Flags: review?(mak77)
Attached patch tests pass (obsolete) — Splinter Review
Attachment #802397 - Attachment is obsolete: true
Attachment #802397 - Flags: review?(mak77)
Attachment #802402 - Flags: review?(mak77)
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
Attachment #802402 - Flags: review?(mak77) → review+
Attachment #802402 - Flags: superreview?(gavin.sharp)
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?
Attachment #802402 - Flags: superreview?(gavin.sharp) → superreview+
Yes, it's enforced (and that's covered by the provided test).
there's a unique index on all guids
Attached patch for checkinSplinter Review
The tree is closed right now.
Attachment #802402 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/424d6f2a4ea7
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: