360133 - Bookmarks must be internally uniquely identifiable

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

The current Places bookmarks implementation ties bookmarks directly to the history entry of the bookmarked URI, including all properties of the bookmark (title, microsummaries, etc). This is problematic for reasons listed at http://wiki.mozilla.org/Places:BookmarksComments#The_Singleton_Model.

Fix:
* add auto-incrementing ID col to moz_bookmarks
* move moz_places.user_title to moz_bookmarks.title
* make interface changes to support bookmark IDs instead of place IDs
* update queries, components and caller code to use bookmark IDs instead of place IDs

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: back-end changes v1

This patch implements the core changes to the back-end code. Front-end patch coming, as well as more back-end changes. Here are a few changes that have yet to be made:

- implement place URIs for bookmarks
- replaceItem should include id of bookmark that replaced it, not the
  URIs
- add title as an input param of InsertItem? (zero instances of
  InsertItem w/o a corresponding SetItemTitle call in the front-end)
- InsertItem should get default bookmark title from moz_places
- remove moz_places.usertitle and corresponding code
- rename bookmarks.item_child to bookmarks.place_id?
- myk: rename moz_anno_attributes to moz_keys?
- make a getBookmarkProperties public api

Comment 3

[O. Atsushi (Torisugari)]

nsNavHistoryResult::OnItemChanged

>   PRUint32 folderCount;
>   PRInt64* folders;
>   rv = bookmarkService->GetBookmarkFolders(aBookmark, &folderCount, &folders);
>   NS_ENSURE_SUCCESS(rv, rv);
>   for (PRUint32 i = 0; i < folderCount; i ++) {
>     ENUMERATE_BOOKMARK_OBSERVERS_FOR_FOLDER(folders[i],
>-        OnItemChanged(aBookmark, aProperty, aValue));
>+        OnItemChanged(aBookmarkId, aBookmark, aProperty, aValue));
>   }
>   if (folders)
>     nsMemory::Free(folders);

nsINavBookmarksService::getBookmarkFolders(...) is a URI-base method,
so this part seems something problematic (too many observer callings).
Now we have bookmarkID, so its parent folder in this context is
unique, isn't it?

Comment 4

[(not reading, please use [e-mail] instead)]

dietrich, I am still reviewing, but here are some comments so far.  most are nits, and my apologies for repeating my nits.  (I'll stop doing that as i continue to review, and if you decide to fix a nit, have you find all occurances.)

1)


-    mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink, -1);
+  if (! hasBookmark) {
+    PRInt64 Id;
+    PRInt32 index = -1;
+    mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink, index, &Id);
+  }

some nits / questions:

a) why is it Id, and not id?
b) why have a local var for index?
c) since -1 is a special value for "append", what about making APPEND a const on the interface, that way the code comments itself, from both C++ and JS?
d) for the return value of InsertItem(), i think we sould consider doing:

rv = mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink, index, &Id);
NS_ASSERTION(NS_SUCCEEDED(rv), "InsertItem failed");

or:

(void)mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink, index, &Id);


2)

   if (! exists) {

not your code, but is it just me, or is that an odd convention?  (! foo) instead of (!foo)?

3)

-nsNavBookmarks::InsertItem(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex)
+nsNavBookmarks::InsertItem(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex, PRInt64 *aNewBookmarkId)
 {
   // You can pass -1 to indicate append, but no other negative number is allowed
   if (aIndex < -1)
     return NS_ERROR_INVALID_ARG;

Am i adding bloat to suggest adding NS_ENSURE_ARG_POINTER(aNewBookmarkId)?

4)

-    // We _should_ always have a result here, but maybe we don't if the table
-    // has become corrupted.  Just silently skip adjusting indices.
-    childIndex = results ? mDBIndexOfItem->AsInt32(0) : -1;
+    if (!results)
+      return NS_ERROR_INVALID_ARG; // invalid bookmark id

Should this assert, or is this an expected scenario?

5)

+//XXXDietrich - the only existing use of this api was internal, from ChangeBookmarkURI.
+// However, that API now just changes the value of item_child (the moz_places entry it points at).
+// NOOP for now. Need to review use-cases, possibly remove it.
 NS_IMETHODIMP
-nsNavBookmarks::ReplaceItem(PRInt64 aFolder, nsIURI *aItem, nsIURI *aNewItem)
+nsNavBookmarks::ReplaceItem(PRInt64 aFolder, PRInt64 aItemId, PRInt64 aNewItemId)
 {
+  return NS_OK;

Should we just "#if 0 this code, instead of return silently?
 

6)
+    rv = mDBGetBookmarkProperties->ExecuteStep(&results);
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (!results)
+      return NS_ERROR_INVALID_ARG; // invalid bookmark id

again, should we assert?

7)

-      item = mDBGetChildAt->AsInt64(0);
+      item = mDBGetChildAt->AsInt64(2);

can you elaborate on that change?

8)

-  for (i = 0; i < PRUint32(itemChildren.Count()); ++i) {
-    rv = RemoveItem(aFolder, itemChildren[i]);
+  for (i = itemChildren.Length() - 1; i != PRUint32(-1); --i) {
+    rv = RemoveItem(itemChildren[i]);
 
why change from 0 -> n-1 to n-1 -> 0?  Do we have to go in reverse order? can you rewrite this loop so that we don't have to cast like that?   

9)

+  mozIStorageConnection *dbConn = DBConn();
+  mozStorageTransaction transaction(dbConn, PR_FALSE);
+
+  nsresult rv;
+  nsCOMPtr<mozIStorageStatement> statement;
+  rv = dbConn->CreateStatement(NS_LITERAL_CSTRING("UPDATE moz_bookmarks SET title = ?2 WHERE id = ?1"),
+                               getter_AddRefs(statement));

as a stil nit, I think I prefer:


+  nsCOMPtr<mozIStorageStatement> statement;
+  nsresult rv = dbConn->CreateStatement(...

Any reason why you did ?2 then ?1, instead of the reverse?

+  // get URI
+  nsCOMPtr<nsIURI> uri;
+  nsCAutoString spec;
+  PRBool results;
+  {
+    mozStorageStatementScoper scope(mDBGetBookmarkProperties);
+    rv = mDBGetBookmarkProperties->BindInt64Parameter(0, aItemId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = mDBGetBookmarkProperties->ExecuteStep(&results);
+    NS_ENSURE_SUCCESS(rv, rv);
+    mDBGetBookmarkProperties->GetUTF8String(kGetBookmarkPropertiesIndex_URI, spec);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = NS_NewURI(getter_AddRefs(uri), spec);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
why the extra { } block?


x)

+  nsresult rv;
+  mozStorageStatementScoper scope(mDBGetBookmarkProperties);
 
-  mozIStorageStatement *statement = DBGetURLPageInfo();
-  nsresult rv = BindStatementURI(statement, 0, aURI);
+  rv = mDBGetBookmarkProperties->BindInt64Parameter(0, aItemId);
   NS_ENSURE_SUCCESS(rv, rv);

again, I think prefer:

nsresult rv =  mDBGetBookmarkProperties->Bind...

x)

+  if (!results)
+    return NS_ERROR_INVALID_ARG; // invalid bookmark id

should we assert?
 
x)

+    // Add bookmark ID
+    PRInt64 id = mDBGetChildren->AsInt64(kGetChildrenIndex_ID);
+    node->mBookmarkId = id;


why the local id variable, and not:

+ node->mBookmarkId = mDBGetChildren->AsInt64(kGetChildrenIndex_ID);

x)


+  PRBool more;
+  while (NS_SUCCEEDED((rv = mDBFindURIBookmarks->ExecuteStep(&more))) && more) {
+    if (! aResult->AppendElement(
+        mDBFindURIBookmarks->AsInt64(kFindBookmarksIndex_ID)))
+      return NS_ERROR_OUT_OF_MEMORY;
+  }

Should we check that "mDBFindURIBookmarks->AsInt64(kFindBookmarksIndex_ID)" did not fail, before we append it?


+NS_IMETHODIMP
+nsNavBookmarks::GetBookmarksForURI(nsIURI *aURI, PRUint32 *aCount,
+                                   PRInt64 **aBookmarks)

Do you think this should be GetBookmarkIDsForURI()?

x)

+    nsresult rv;
+    nsTArray<PRInt64> bookmarks;
+
+    rv = GetBookmarksForURITArray(aURI, &bookmarks);
+    NS_ENSURE_SUCCESS(rv, rv);


again, I like the "nsresult rv = GetBookmarksForURITArray(..." style.

+      for (PRUint32 i = 0; i < bookmarks.Length(); i ++)

nit:  "i ++" -> "i++"

x)


+    // query for all bookmarks for that URI, notify for each 
+    nsresult rv;
+    nsTArray<PRInt64> bookmarks;
+
+    rv = GetBookmarksForURITArray(aURI, &bookmarks);
+    NS_ENSURE_SUCCESS(rv, rv);

nsresult-on-same-line style nit.


x)


+      if (bookmarks.Length()) {
+        for (PRUint32 i = 0; i < bookmarks.Length(); i ++)
+          ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                              OnItemChanged(1, aURI, NS_LITERAL_CSTRING("favicon"),
+                                            aValue));
+      }

why "1"? and not bookmarks[i]?

Comment 5

[(not reading, please use [e-mail] instead)]

ok, done looking over your patch.  only found one more nit:

+  NS_IMETHOD GetBookmarkId(PRInt64* aId) \
+    { *aId= mBookmarkId; return NS_OK; }

minor nit:  "aId=" -> "aID ="

Comment 6

[(not reading, please use [e-mail] instead)]

Comment on attachment 253524 [details] [diff] [review]
back-end changes v1

clearing review, waiting to hear back from dietrich.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: back-end changes v2 - addresses c3 & c4, adds unit-tests and place URIs for bookmarks

(In reply to comment #3)
> nsNavHistoryResult::OnItemChanged
> 
> >   PRUint32 folderCount;
> >   PRInt64* folders;
> >   rv = bookmarkService->GetBookmarkFolders(aBookmark, &folderCount, &folders);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   for (PRUint32 i = 0; i < folderCount; i ++) {
> >     ENUMERATE_BOOKMARK_OBSERVERS_FOR_FOLDER(folders[i],
> >-        OnItemChanged(aBookmark, aProperty, aValue));
> >+        OnItemChanged(aBookmarkId, aBookmark, aProperty, aValue));
> >   }
> >   if (folders)
> >     nsMemory::Free(folders);
> 
> nsINavBookmarksService::getBookmarkFolders(...) is a URI-base method,
> so this part seems something problematic (too many observer callings).
> Now we have bookmarkID, so its parent folder in this context is
> unique, isn't it?
> 

yes, this is correct. i went back and looked, and found that there are no use-cases in the current code for fetching folders by URI, given the removal of bookmark URI as singleton. i've implemented getFolderIdForBookmark as a replacement for getBookmarkFolders, and updated all callers to use it.

(In reply to comment #4)
> 
> 1)
>
> -    mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink,
> -1);
> +  if (! hasBookmark) {
> +    PRInt64 Id;
> +    PRInt32 index = -1;
> +    mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink,
> index, &Id);
> +  }
> 
> some nits / questions:
> 
> a) why is it Id, and not id?

no reason, changed.

> b) why have a local var for index?

not necessary, changed.

> c) since -1 is a special value for "append", what about making APPEND a const
> on the interface, that way the code comments itself, from both C++ and JS?

fixed.

> d) for the return value of InsertItem(), i think we sould consider doing:
> 
> rv = mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink,
> index, &Id);
> NS_ASSERTION(NS_SUCCEEDED(rv), "InsertItem failed");
> 
> or:
> 
> (void)mBookmarksService->InsertItem(frame.mContainerID, frame.mPreviousLink,
> index, &Id);

good point, switched to the first.

> 2)
> 
>    if (! exists) {
> 
> not your code, but is it just me, or is that an odd convention?  (! foo)
> instead of (!foo)?

yeah, it's odd. but it's all over the Places code. a change for separate patch perhaps.

> 3)
> 
> -nsNavBookmarks::InsertItem(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex)
> +nsNavBookmarks::InsertItem(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex,
> PRInt64 *aNewBookmarkId)
>  {
>    // You can pass -1 to indicate append, but no other negative number is
> allowed
>    if (aIndex < -1)
>      return NS_ERROR_INVALID_ARG;
> 
> Am i adding bloat to suggest adding NS_ENSURE_ARG_POINTER(aNewBookmarkId)?

definitely a valid suggestion. i'll add this where appropriate moving forward.

> 4)
> 
> -    // We _should_ always have a result here, but maybe we don't if the table
> -    // has become corrupted.  Just silently skip adjusting indices.
> -    childIndex = results ? mDBIndexOfItem->AsInt32(0) : -1;
> +    if (!results)
> +      return NS_ERROR_INVALID_ARG; // invalid bookmark id
> 
> Should this assert, or is this an expected scenario?

well, if callers have stale data it's possible this could happen. i'm going to leave it as-is for now, because changing to an assert causes the unit tests to die when running against a debug build, which is really a pain.

> 5)
> 
> +//XXXDietrich - the only existing use of this api was internal, from
> ChangeBookmarkURI.
> +// However, that API now just changes the value of item_child (the moz_places
> entry it points at).
> +// NOOP for now. Need to review use-cases, possibly remove it.
>  NS_IMETHODIMP
> -nsNavBookmarks::ReplaceItem(PRInt64 aFolder, nsIURI *aItem, nsIURI *aNewItem)
> +nsNavBookmarks::ReplaceItem(PRInt64 aFolder, PRInt64 aItemId, PRInt64
> aNewItemId)
>  {
> +  return NS_OK;
> 
> Should we just "#if 0 this code, instead of return silently?

yes, that's a better way to handle it, done.

> 
> 6)
> +    rv = mDBGetBookmarkProperties->ExecuteStep(&results);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (!results)
> +      return NS_ERROR_INVALID_ARG; // invalid bookmark id
> 
> again, should we assert?
> 

no, see earlier comment about this

> 7)
> 
> -      item = mDBGetChildAt->AsInt64(0);
> +      item = mDBGetChildAt->AsInt64(2);
> 
> can you elaborate on that change?
> 

now we're accessing bookmark id instead of place id, to pass to RemoveItem.

> 8)
> 
> -  for (i = 0; i < PRUint32(itemChildren.Count()); ++i) {
> -    rv = RemoveItem(aFolder, itemChildren[i]);
> +  for (i = itemChildren.Length() - 1; i != PRUint32(-1); --i) {
> +    rv = RemoveItem(itemChildren[i]);
> 
> why change from 0 -> n-1 to n-1 -> 0?  Do we have to go in reverse order?

see the comment at:

http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavBookmarks.cpp#1385

i thought we might reap the same benefit here. however, i haven't profiled it to see if it makes a tangible difference. i've reverted this, can revisit later if necessary.

> 9)
> 
> +  mozIStorageConnection *dbConn = DBConn();
> +  mozStorageTransaction transaction(dbConn, PR_FALSE);
> +
> +  nsresult rv;
> +  nsCOMPtr<mozIStorageStatement> statement;
> +  rv = dbConn->CreateStatement(NS_LITERAL_CSTRING("UPDATE moz_bookmarks SET
> title = ?2 WHERE id = ?1"),
> +                               getter_AddRefs(statement));
> 
> as a stil nit, I think I prefer:
> 
> 
> +  nsCOMPtr<mozIStorageStatement> statement;
> +  nsresult rv = dbConn->CreateStatement(...

fixed

> Any reason why you did ?2 then ?1, instead of the reverse?

fixed

> +  // get URI
> +  nsCOMPtr<nsIURI> uri;
> +  nsCAutoString spec;
> +  PRBool results;
> +  {
> +    mozStorageStatementScoper scope(mDBGetBookmarkProperties);
> +    rv = mDBGetBookmarkProperties->BindInt64Parameter(0, aItemId);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = mDBGetBookmarkProperties->ExecuteStep(&results);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    mDBGetBookmarkProperties->GetUTF8String(kGetBookmarkPropertiesIndex_URI,
> spec);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = NS_NewURI(getter_AddRefs(uri), spec);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> 
> why the extra { } block?

was for statement scoping, but is unnecessary, removed.

> 
> x)
> 
> +  nsresult rv;
> +  mozStorageStatementScoper scope(mDBGetBookmarkProperties);
> 
> -  mozIStorageStatement *statement = DBGetURLPageInfo();
> -  nsresult rv = BindStatementURI(statement, 0, aURI);
> +  rv = mDBGetBookmarkProperties->BindInt64Parameter(0, aItemId);
>    NS_ENSURE_SUCCESS(rv, rv);
> 
> again, I think prefer:
> 
> nsresult rv =  mDBGetBookmarkProperties->Bind...

fixed

> x)
> 
> +  if (!results)
> +    return NS_ERROR_INVALID_ARG; // invalid bookmark id
> 
> should we assert?

no, see earlier comment about this

> 
> x)
> 
> +    // Add bookmark ID
> +    PRInt64 id = mDBGetChildren->AsInt64(kGetChildrenIndex_ID);
> +    node->mBookmarkId = id;
> 
> 
> why the local id variable, and not:
> 
> + node->mBookmarkId = mDBGetChildren->AsInt64(kGetChildrenIndex_ID);

fixed

> x)
> 
> +  PRBool more;
> +  while (NS_SUCCEEDED((rv = mDBFindURIBookmarks->ExecuteStep(&more))) && more)
> {
> +    if (! aResult->AppendElement(
> +        mDBFindURIBookmarks->AsInt64(kFindBookmarksIndex_ID)))
> +      return NS_ERROR_OUT_OF_MEMORY;
> +  }
> 
> Should we check that "mDBFindURIBookmarks->AsInt64(kFindBookmarksIndex_ID)" did
> not fail, before we append it?

we're checking the result for that row above, which is good enough to know that there's a result there.

> 
> +NS_IMETHODIMP
> +nsNavBookmarks::GetBookmarksForURI(nsIURI *aURI, PRUint32 *aCount,
> +                                   PRInt64 **aBookmarks)
> 
> Do you think this should be GetBookmarkIDsForURI()?

sure, it's self-commenting and more explicit that way, fixed.

> x)
> 
> +    nsresult rv;
> +    nsTArray<PRInt64> bookmarks;
> +
> +    rv = GetBookmarksForURITArray(aURI, &bookmarks);
> +    NS_ENSURE_SUCCESS(rv, rv);
> 
> 
> again, I like the "nsresult rv = GetBookmarksForURITArray(..." style.

fixed

> 
> +      for (PRUint32 i = 0; i < bookmarks.Length(); i ++)
> 
> nit:  "i ++" -> "i++"

fixed

> 
> x)
> 
> 
> +    // query for all bookmarks for that URI, notify for each
> +    nsresult rv;
> +    nsTArray<PRInt64> bookmarks;
> +
> +    rv = GetBookmarksForURITArray(aURI, &bookmarks);
> +    NS_ENSURE_SUCCESS(rv, rv);
> 
> nsresult-on-same-line style nit.
> 
fixed

> 
> x)
> 
> 
> +      if (bookmarks.Length()) {
> +        for (PRUint32 i = 0; i < bookmarks.Length(); i ++)
> +          ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
> +                              OnItemChanged(1, aURI,
> NS_LITERAL_CSTRING("favicon"),
> +                                            aValue));
> +      }
> 
> why "1"? and not bookmarks[i]?
> 

ugh, because i forgot to update that placeholder. fixed.

> 
> +  NS_IMETHOD GetBookmarkId(PRInt64* aId) \
> +    { *aId= mBookmarkId; return NS_OK; }
> 
> minor nit:  "aId=" -> "aID ="
> 

fixed the space. not sure if you meant to uppercase to aID or not, but i left it as-is due to convention elsewhere in this code.

Comment 8

[(not reading, please use [e-mail] instead)]

>> c) since -1 is a special value for "append", what about making APPEND a const
>> on the interface, that way the code comments itself, from both C++ and JS?
> fixed.

Did you end up deciding not the make that change?

Comment 9

[(not reading, please use [e-mail] instead)]

actually, i see you added DEFAULT_INDEX, but you aren't using it from all the possible C++ / js callers, right?

I think that would help make the code more self commenting.

Comment 10

[O. Atsushi (Torisugari)]

(In reply to comment #7)
>i've implemented getFolderIdForBookmark
Thanks.

(In reply to comment #2)
> - replaceItem should include id of bookmark that replaced it, not the
>   URIs
(In reply to comment #7)
>>> +// NOOP for now. Need to review use-cases, possibly remove it.
>> Should we just "#if 0 this code, instead of return silently?
>
>yes, that's a better way to handle it, done.

Then, probably nsINavBookmarksService::changeBookmarkURI(...) is
missing valid callbacks, because replaceItem(...) was calling
onItemReplaced(...) and it's gone away.

I guess we'd have 2 options:
a) Reuse onItemReplaced(...) and call it from changeBookmarkURI.
b) Add a new property "uri" to onItemChanged(...) and call it.
c) Leave as it is, because it's not critical for UI anyway...
  (I'm not sure about transactions)

In any case, imho, the old strategy

  |replace| = |remove old| + |add new|

as below:
>nsNavHistoryFolderResultNode::OnItemReplaced(...) {
> ...
>-  nsresult rv = OnItemRemoved(aOldItem, aFolder, nodeIndex);
>+  nsresult rv = OnItemRemoved(aBookmarkId, aOldItem, aFolder, nodeIndex);
>   NS_ENSURE_SUCCESS(rv, rv);
>-  return OnItemAdded(aNewItem, aFolder, nodeIndex);
>+  return OnItemAdded(aBookmarkId, aNewItem, aFolder, nodeIndex);
>}

would be a bit too expensive for id-driven.

Comment 11

[Dietrich Ayala (:dietrich)]

(In reply to comment #10)
> (In reply to comment #7)
> >i've implemented getFolderIdForBookmark
> Thanks.
> 
> (In reply to comment #2)
> > - replaceItem should include id of bookmark that replaced it, not the
> >   URIs
> (In reply to comment #7)
> >>> +// NOOP for now. Need to review use-cases, possibly remove it.
> >> Should we just "#if 0 this code, instead of return silently?
> >
> >yes, that's a better way to handle it, done.
> 
> Then, probably nsINavBookmarksService::changeBookmarkURI(...) is
> missing valid callbacks, because replaceItem(...) was calling
> onItemReplaced(...) and it's gone away.
> 
> I guess we'd have 2 options:
> a) Reuse onItemReplaced(...) and call it from changeBookmarkURI.
> b) Add a new property "uri" to onItemChanged(...) and call it.
> c) Leave as it is, because it's not critical for UI anyway...
>   (I'm not sure about transactions)
> 
> In any case, imho, the old strategy
> 
>   |replace| = |remove old| + |add new|
> 
> as below:
> >nsNavHistoryFolderResultNode::OnItemReplaced(...) {
> > ...
> >-  nsresult rv = OnItemRemoved(aOldItem, aFolder, nodeIndex);
> >+  nsresult rv = OnItemRemoved(aBookmarkId, aOldItem, aFolder, nodeIndex);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >-  return OnItemAdded(aNewItem, aFolder, nodeIndex);
> >+  return OnItemAdded(aBookmarkId, aNewItem, aFolder, nodeIndex);
> >}
> 
> would be a bit too expensive for id-driven.
> 

Good catch, thanks! After reviewing the current usage, it seems that OnItemReplaced really only makes sense in the context of uri-as-singleton: it was used to remove/replace bookmarks in folders in query results, or to just to rebuild UI. Now that a URI can be bookmarked >1 times in a folder, i'm going to use OnItemChanged to notify when ChangeBookmarkURI is called, as you suggested above. I'm going to remove OnItemReplaced altogether.

Comment 12

[(not reading, please use [e-mail] instead)]

> I'm going to remove OnItemReplaced altogether.

dietrich, do you want me to review the existing patch, and spin that issue off to a follow up bug?

Comment 13

[O. Atsushi (Torisugari)]

(In reply to comment #11)
> i'm going to use OnItemChanged to notify when ChangeBookmarkURI is called,

I agree that is the best.

(In reply to comment #12)
> dietrich, do you want me to review the existing patch, and spin that
> issue off to a follow up bug?

I feel I'm disturbing your work. I'm sorry.

Besides onItemChanged and onItemReplaced as described at comment #11,
onItemRemoved and onItemVisited (note: a history observer) need some
modification, I think. If dietrich is going to address left-out issues
at comment #2, probably also onItemInserted etc. etc. ...

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: back-end changes v3 - addresses c10 & c13

this patch contains the updated unit tests (missing from the previous patches)

(In reply to comment #9)
> actually, i see you added DEFAULT_INDEX, but you aren't using it from all the
> possible C++ / js callers, right?
> 
> I think that would help make the code more self commenting.
> 

callers in the backend are now updated. front-end caller changes will be in the forthcoming (and soon, i swear) front-end patch.

(In reply to comment #13)
> (In reply to comment #11)
> > i'm going to use OnItemChanged to notify when ChangeBookmarkURI is called,
> 
> I agree that is the best.
> 
> (In reply to comment #12)
> > dietrich, do you want me to review the existing patch, and spin that
> > issue off to a follow up bug?
> 
> I feel I'm disturbing your work. I'm sorry.
> 

no problem! it must be corrected sooner or later. thanks very much for noticing these things.

> Besides onItemChanged and onItemReplaced as described at comment #11,
> onItemRemoved and onItemVisited (note: a history observer) need some
> modification, I think. If dietrich is going to address left-out issues
> at comment #2, probably also onItemInserted etc. etc. ...
> 

I've updated these event handlers in the result containers. Now I'm searching for child nodes by bookmark id instead of URI where appropriate.

Comment 15

[O. Atsushi (Torisugari)]

(In reply to comment #14)
> Now I'm searching for child nodes by bookmark id instead of URI
> where appropriate.

Yes, that's exactly what I'd like to say! And finally we got ready to
fight against another URI-base enemy "ChangeTitles(...)".

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp&rev=1.79&mark=2868-2874#2868

In the end, it seems to me all callbacks other than onItemInserted do
not require URI argument at interface level. And probably it is a kind
of beautiful interface design, because only |insertItem| requires URI,
and only relevant |onItemInserted| is to be called with URI. Does it
make some sense? In theory, only insertItem and changeBookmarkURI
do "URI" -> "places_id" conversion, and the latter has "uri"
property, to generate a query for db table.

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 254354 [details] [diff] [review]
back-end changes v3 - addresses c10 & c13

r=sspitzer, sorry for the delay.

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: back-end changes v4

Summary of Backend Changes:

+ Added an id column to the moz_bookmarks table, which is the primary key for the table.
+ Added a title column to the moz_bookmarks table, to replace the user_title column in the moz_places table. 
+ All relevant bookmark-related apis in ns(I)NavBookmarksService have been converted to use a bookmark id instead of URI.
+ Added various bookmark property accessors to the bookmarks service
+ nsIBookmarkObserver has been modified to use bookmark id instead of URI
+ nsNavHistoryResult* - implementations of nsIBookmarkObserver have been updated to the new changes. added helper for finding children by id instead of URI.
+ Added an accessor for page titles to nsNavHistoryService

(Seth, the delta of v4 is primarily the addition of some bookmark property accessors for the front-end, as querying via the nsNavHistory* infrastructure is overkill in certain circumstances.)

Some follow-up bugs I need to file:
- The keyword apis are still uri-centric
- Need to remove moz_places.user_title, and all related code
- Need to add bookmarkId to exported bookmarks.html, and handle imports
- Should replace individual property accessors in nsinavbookmarksservice with row-level accessor, and same for nsinavhistoryservice for a moz_places row

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: front-end changes v1

Front-end Changes:
+ Updated all callers of the changed nsINavBookmarks.
+ Restructured various parts of the bookmark properties dialog now that URI is not the unique identifier when editing bookmarks.
+ Updated migrators to use bookmark ids

Some follow-up bugs I need to file when this lands:
- Update microsummaries in widgets and dialog to use bookmark id, place URI
- Might not be related to this bug, but undoMoveItems fails with: MIN_TRANSACTIONS_FOR_BATCH is not defined" {file: "chrome://browser/content/places/controller.js" line: 1521}]

Comment 19

[Asaf Romano (gone)]

Hrm, I would really like to see a new interface (thus a new node type) for bookmark items.

This interface could also wrap access to bookmark annotations.

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to comment #19)
> Hrm, I would really like to see a new interface (thus a new node type) for
> bookmark items.
> 
> This interface could also wrap access to bookmark annotations.
> 

Yes, that would be nice, esp. from the front-end. Not strictly necessary for this though. A concern I have about it is that hardening what a bookmark "is" removes some of the flexibility that using annotations for metadata provides. Would need to find a balance there. However, we can add this is a follow-up issue, as we flesh out the changes for making Places more accessible as a platform.

Comment 21

[Asaf Romano (gone)]

 * Why is get/setKeywordFor still uri-based?
 * Please update the documentation and argument names for PlacesUtils.changeBookmarkURI
 * the backend patch refers to nsIWritablePropertyBag2 but doesn't use it.

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: back-end changes v5 - addresses mano's comments

(In reply to comment #21)
>  * Why is get/setKeywordFor still uri-based?

Was still working on it. It's included in this patch.

>  * Please update the documentation and argument names for
> PlacesUtils.changeBookmarkURI
>  * the backend patch refers to nsIWritablePropertyBag2 but doesn't use it.
> 

Fixed.

Comment 23

[Dietrich Ayala (:dietrich)]

Attachment: front-end changes v2 - addresses mano's comments

Comment 24

[Asaf Romano (gone)]

Comment on attachment 256049 [details] [diff] [review]
back-end changes v5 - addresses mano's comments

API-only review. May I keep the rest of this to sspitzer? ;)

Feel free to do any of these changes in follow up bugs, we should land this ASAP.

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================

>   /**
>    * Notify this observer that the bookmark was added.  Called after the actual
>    * add took place. The rest of the bookmarks will be shifted down, but no
>    * additional notifications will be sent.
>    *
>-   * @param bookmark   The bookmark item that was added.
>+   * @param bookmarkId The id of the bookmark that was added.
>+   * @param bookmark   The URI that was bookmarked.
>    * @param folder     The folder that the item was added to.
>    * @param index      The item's index in the folder.
>    */
>-  void onItemAdded(in nsIURI bookmark, in PRInt64 folder, in PRInt32 index);
>+  void onItemAdded(in PRInt64 bookmarkId, in nsIURI bookmark, in PRInt64 folder, in PRInt32 index);
>

I don't think we need the URI argument.

>   /**
>    * Notify this observer that the bookmark was removed.  Called after the
>    * actual remove took place. The bookmarks following the index will be
>    * shifted up, but no additional notifications will be sent.
>    *
>-   * @param bookmark   The bookmark item will be removed.
>+   * @param bookmarkId The id of the bookmark that was removed.
>+   * @param bookmark   The URI of the bookmark that will be removed.
>    * @param folder     The folder that the item was removed from.
>    * @param index      The bookmark's index in the folder.
>    */
>-  void onItemRemoved(in nsIURI bookmark, in PRInt64 folder, in PRInt32 index);
>+  void onItemRemoved(in PRInt64 bookmarkId, in nsIURI bookmark, in PRInt64 folder, in PRInt32 index);
> 

nither here.

>   /**
>    * Notify this observer that a bookmark's information has changed.  This
>    * will be called whenever any attributes like "title" are changed.
>    *
>-   * @param bookmark The bookmark which changed.
>+   * @param bookmarkId The id of the bookmark that was changed.
>+   * @param bookmark The URI of the bookmark which changed.
>    * @param property The property which changed.
>    *
>    * property = "cleartime" (history was deleted, there is no last visit date):
>    *                        value = none
>    * property = "title": value = new title
>    * property = "favicon": value = new "moz-anno" URL of favicon image
>    */
>-  void onItemChanged(in nsIURI bookmark, in ACString property,
>+  void onItemChanged(in PRInt64 bookmarkId, in nsIURI bookmark, in ACString property,
>                      in AString value);


please document property="uri".

>   /**
>    * Notify that the item was visited. Normally in bookmarks we use the last
>    * visit date, and normally the time will be a new visit that will be more
>    * recent, but this is not guaranteed. You should check to see if it's
>    * actually more recent before using this new time.
>    *
>+   * @param bookmarkId The id of the bookmark that was visited.
>    * @see onItemChanged properth = "cleartime" for when all visit dates are
>    * deleted for the URI.
>    */
>-  void onItemVisited(in nsIURI bookmark, in PRInt64 aVisitID, in PRTime time);
>-
>-  /**
>-   * Notify this observer that a bookmark has been replaced.
>-   *
>-   * @param folder   The folder in which the bookmark was replaced
>-   * @param item     The item which was replaced
>-   * @param newItem  The new item which replaced item
>-   */
>-  void onItemReplaced(in PRInt64 folder, in nsIURI item, in nsIURI newItem);
>+  void onItemVisited(in PRInt64 bookmarkId, in nsIURI bookmark, in PRInt64 aVisitID, in PRTime time);

We should remove this API. Even if there is a use case for this, it could listen to onVisit (in a history observer) and query the DB for bookmarks.


>+  /**
>    * Inserts a child item into the given folder. If this item already exists in
>    * the given folder, it will be moved to the new position.
>    *  @param folder     The id of the parent folder
>    *  @param item       The URI to insert
>    *  @param index      The index to insert at, or -1 to append
>+   *  @returns          The ID of the newly-created bookmark
>    */
>-  void insertItem(in PRInt64 folder, in nsIURI item, in PRInt32 index);
>+  PRInt64 insertItem(in PRInt64 folder, in nsIURI item, in PRInt32 index);
> 

please file a bug on using xpidl-friendly types.


> 
>   /**
>    * Returns the index of the given item in the given folder.
>    * Returns -1 if the item is not present in the folder.
>    */
>   PRInt32 indexOfItem(in PRInt64 folder, in nsIURI uri);
> 

<Mano>	does this still make sense?
<dietrich>	hrm, probably not. i added getItemIndex, which takes an id.
<dietrich>	and there's not really a use-case that i can think of for finding the index of the first occurence of a uri in a folder :P



>-   * Associates the given keyword with the given URI.
>-   *
>-   * Use an empty keyword to clear the keyword associated with the URI. Use an
>-   * empty URI to clear the URI associated with that keyword. In both of these
>-   * cases, succeeds but does nothing if the URL/keyword is not found.
>+   * Associates the given keyword with the given bookmark.
>    *
>-   * When setting a keyword (both URI and keyword are specified), the URI must
>-   * be bookmarked for the keyword to be persistent.
>+   * Use an empty keyword to clear the keyword associated with the URI.
>+   * In both of these cases, succeeds but does nothing if the URL/keyword is not found.
>    */
>-  void setKeywordForURI(in nsIURI uri, in AString keyword);
>+  void setKeywordForBookmark(in PRInt64 bookmarkId, in AString keyword);
> 
>   /**
>    * Retrieves the keyword for the given URI. Will be void string
>    * (null in JS) if no such keyword is found.
>    */
>   AString getKeywordForURI(in nsIURI uri);
> 
>   /**
>+   * Retrieves the keyword for the given bookmark. Will be void string
>+   * (null in JS) if no such keyword is found.
>+   */
>+  AString getKeywordForBookmark(in PRInt64 bookmarkId);

Thinking about this again, when would we use getKeywordForBookmark?

Hrm, maybe we should keep this uri-based and move it to the history service?

Comment 25

[Asaf Romano (gone)]

Comment on attachment 256050 [details] [diff] [review]
front-end changes v2 - addresses mano's comments

>Index: browser/components/places/content/bookmarkProperties.js
>===================================================================


>@@ -93,23 +94,20 @@ var BookmarkPropertiesPanel = {
> 
>   /**
>    * Returns true if this variant of the dialog uses a URI as a primary
>    * identifier for the item being edited.
>    */
> 
>   _identifierIsURI: function BPP__identifierIsURI() {
>     switch(this._variant) {
>-    case this.EDIT_FOLDER_VARIANT:
>-    case this.ADD_MULTIPLE_BOOKMARKS_VARIANT:
>-    case this.ADD_LIVEMARK_VARIANT:
>-    case this.EDIT_LIVEMARK_VARIANT:
>-      return false;
>-    default:
>+    case this.ADD_BOOKMARK_VARIANT:
>       return true;
>+    default:
>+      return false;
>     }
>   },
> 

remove this function and simply do |if (this._variant ==/!= this.ADD_BOOKMARK_VARIANT)|.


>   _determineVariant: function BPP__determineVariant(identifier, action) {
...
>+    else if (typeof(identifier) == "number") {
...
>     }
>+
>+    NS_ASSERT(PlacesUtils.bookmarks.isBookmarked(identifier),
>+              "Bookmark Properties dialog instantiated with " +
>+              "non-bookmarked URI: \"" + identifier + "\"");
>+    return this.EDIT_BOOKMARK_VARIANT;

We should just disallow opening this dialog for editing a bookmark with a uri identifier.

>   /**
>    * This method returns the title string corresponding to a given URI.
>    * If none is available from the bookmark service (probably because
>    * the given URI doesn't appear in bookmarks or history), we synthesize
>    * a title from the first 100 characters of the URI.
>    *
>    * @param uri  a nsIURI object for which we want the title
>    *
>    * @returns a title string
>    */
> 
>   _getURITitle: function BPP__getURITitle(uri) {

Please rename this to something more descriptive (getTitleFromHistory?)

>Index: browser/components/places/content/controller.js
>===================================================================
...
>-      var nodesInList = 0;
>-      // Sadly, this is O(N^2). 
>-      for (var i = 0; i < nodes.length; ++i) {
>-        if (nodeIsInList(nodes[i]))
>-          ++nodesInList;
>-      }
>-      return (nodesInList != nodes.length);
>+      return ip ? true : false;

return ip != null;


> PlacesMoveItemTransaction.prototype = {
>   __proto__: PlacesBaseTransaction.prototype,
> 
>   doTransaction: function PMIT_doTransaction() {
>     this.LOG("Move Item: " + this._uri.spec + " from: " + this._oldContainer + "," + this._oldIndex + " to: " + this._newContainer + "," + this._newIndex);
>-    this.bookmarks.removeItem(this._oldContainer, this._uri);
>-    this.bookmarks.insertItem(this._newContainer, this._uri, this._newIndex);
>+    var title = this.bookmarks.getItemTitle(this._id);
>+    this.bookmarks.removeItem(this._id);
>+    this._newId = this.bookmarks.insertItem(this._newContainer, this._uri, this._newIndex);
>+    this.bookmarks.setItemTitle(this._newId, title);

As noted on IRC, this kills the annotations for the moved bookmark.



>Index: browser/components/places/tests/bookmarks/head_bookmarks.js
>===================================================================
>Index: browser/components/places/tests/unit/head_bookmarks.js
>===================================================================
> 
>+function LOG(aMsg) {
>+  aMsg = ("*** PLACES TESTS: " + aMsg);
>+  Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).
>+                                      logStringMessage(aMsg);
>+  print(aMsg);
>+}
>+

Is this used?

>Index: browser/locales/en-US/profile/localstore.rdf
>===================================================================


I thought that bug is fixed.

Comment 26

[(not reading, please use [e-mail] instead)]

Comment on attachment 256049 [details] [diff] [review]
back-end changes v5 - addresses mano's comments

r=sspitzer

based on https://bugzilla.mozilla.org/attachment.cgi?oldid=254354&action=interdiff&newid=256049&headers=0

I have just a few comments / questions:

1)

  {
    mozStorageStatementScoper scope(mDBGetBookmarkProperties);
    ...
  }

why did you add extra { } block around this scope?  (Could you add a comment explaining it, or remove it?)

2)

    // it should be fast to lookup by keyword
    // XXXDietrich - is this necessary? isn't UNIQUE itself an index?

Can you log a spin off bug about that, or remove the comment (and remove the code to create the index)

3)

keyword VARCHAR(32) UNIQUE

I don't think FX2 has a 32 limit on keywords.  

I know you didn't add this code, but perhaps you can log a spin off bug about fixing this issue?

4)

nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId, const nsAString& aKeyword)

{
  if (!aBookmarkId)
    return NS_OK;

two questions:

a)  is it really NS_OK, or NS_ERROR_INVALID_ARG?  (what do the callers expect?)
b)  is it really "if (aBookmarkId < 1)"?

5) GetKeywordForBookmark() and GetPageTitle(), I think:

a) both should call x.Truncate(0)
b) both should have the "return NS_OK; // not found: return void x string" comment

6)  /* XXXDietrich - vestigial? is not currently in nsINavBookmarksService

it looks like you are adding this commented out code.

Comment 27

[Dietrich Ayala (:dietrich)]

Attachment: front-end changes v3 - addresses mano's comments

(In reply to comment #25)
> > 
> >   _identifierIsURI: function BPP__identifierIsURI() {
> >     switch(this._variant) {
> >-    case this.EDIT_FOLDER_VARIANT:
> >-    case this.ADD_MULTIPLE_BOOKMARKS_VARIANT:
> >-    case this.ADD_LIVEMARK_VARIANT:
> >-    case this.EDIT_LIVEMARK_VARIANT:
> >-      return false;
> >-    default:
> >+    case this.ADD_BOOKMARK_VARIANT:
> >       return true;
> >+    default:
> >+      return false;
> >     }
> >   },
> > 
> 
> remove this function and simply do |if (this._variant ==/!=
> this.ADD_BOOKMARK_VARIANT)|.

fixed

> 
> >   _determineVariant: function BPP__determineVariant(identifier, action) {
> ...
> >+    else if (typeof(identifier) == "number") {
> ...
> >     }
> >+
> >+    NS_ASSERT(PlacesUtils.bookmarks.isBookmarked(identifier),
> >+              "Bookmark Properties dialog instantiated with " +
> >+              "non-bookmarked URI: \"" + identifier + "\"");
> >+    return this.EDIT_BOOKMARK_VARIANT;
> 
> We should just disallow opening this dialog for editing a bookmark with a uri
> identifier.

Removed. The controller now will only open the dialog if dialog if the node has a valid bookmark id (for edits).

> >   /**
> >    * This method returns the title string corresponding to a given URI.
> >    * If none is available from the bookmark service (probably because
> >    * the given URI doesn't appear in bookmarks or history), we synthesize
> >    * a title from the first 100 characters of the URI.
> >    *
> >    * @param uri  a nsIURI object for which we want the title
> >    *
> >    * @returns a title string
> >    */
> > 
> >   _getURITitle: function BPP__getURITitle(uri) {
> 
> Please rename this to something more descriptive (getTitleFromHistory?)

changed to getURITitleFromHistory

> >Index: browser/components/places/content/controller.js
> >===================================================================
> ...
> >-      var nodesInList = 0;
> >-      // Sadly, this is O(N^2). 
> >-      for (var i = 0; i < nodes.length; ++i) {
> >-        if (nodeIsInList(nodes[i]))
> >-          ++nodesInList;
> >-      }
> >-      return (nodesInList != nodes.length);
> >+      return ip ? true : false;
> 
> return ip != null;
> 
> 

fixed

> > PlacesMoveItemTransaction.prototype = {
> >   __proto__: PlacesBaseTransaction.prototype,
> > 
> >   doTransaction: function PMIT_doTransaction() {
> >     this.LOG("Move Item: " + this._uri.spec + " from: " + this._oldContainer + "," + this._oldIndex + " to: " + this._newContainer + "," + this._newIndex);
> >-    this.bookmarks.removeItem(this._oldContainer, this._uri);
> >-    this.bookmarks.insertItem(this._newContainer, this._uri, this._newIndex);
> >+    var title = this.bookmarks.getItemTitle(this._id);
> >+    this.bookmarks.removeItem(this._id);
> >+    this._newId = this.bookmarks.insertItem(this._newContainer, this._uri, this._newIndex);
> >+    this.bookmarks.setItemTitle(this._newId, title);
> 
> As noted on IRC, this kills the annotations for the moved bookmark.

fixed. PlacesRemoveItemTransaction wraps up all annotations, and then copies them back to the new bookmark on undo. i added the get/set methods to PlacesUtils.

> 
> >Index: browser/components/places/tests/bookmarks/head_bookmarks.js
> >===================================================================
> >Index: browser/components/places/tests/unit/head_bookmarks.js
> >===================================================================
> > 
> >+function LOG(aMsg) {
> >+  aMsg = ("*** PLACES TESTS: " + aMsg);
> >+  Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).
> >+                                      logStringMessage(aMsg);
> >+  print(aMsg);
> >+}
> >+
> 
> Is this used?

I use it when testing the tests.

Comment 28

[Dietrich Ayala (:dietrich)]

Attachment: front-end changes v4 - addresses mano's comments

now carries annotations over when moving bookmarks.

Comment 29

[Dietrich Ayala (:dietrich)]

Attachment: back-end changes v6 - addresses seth's comments

(In reply to comment #24)

I'll file follow-up bugs for these issues.

(In reply to comment #26)
> (From update of attachment 256049 [details] [diff] [review])
> r=sspitzer
> 
> based on
> https://bugzilla.mozilla.org/attachment.cgi?oldid=254354&action=interdiff&newid=256049&headers=0
> 
> I have just a few comments / questions:
> 
> 1)
> 
>   {
>     mozStorageStatementScoper scope(mDBGetBookmarkProperties);
>     ...
>   }
> 
> why did you add extra { } block around this scope?  (Could you add a comment
> explaining it, or remove it?)
> 

the scoping ensures that the statement gets reset. i've added a comment.

> 2)
> 
>     // it should be fast to lookup by keyword
>     // XXXDietrich - is this necessary? isn't UNIQUE itself an index?
> 
> Can you log a spin off bug about that, or remove the comment (and remove the
> code to create the index)

looked it up, UNIQUE does create an index on that column, so removed the comment and code.

> 
> 3)
> 
> keyword VARCHAR(32) UNIQUE
> 
> I don't think FX2 has a 32 limit on keywords.  
> 
> I know you didn't add this code, but perhaps you can log a spin off bug about
> fixing this issue?

sqlite doesn't actually care if you put 32 or 32000 chars in that field:

http://www.sqlite.org/faq.html#q11

i updated the query to use the TEXT keyword instead of VARCHAR(32)

> 
> 4)
> 
> nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId, const nsAString&
> aKeyword)
> 
> {
>   if (!aBookmarkId)
>     return NS_OK;
> 
> two questions:
> 
> a)  is it really NS_OK, or NS_ERROR_INVALID_ARG?  (what do the callers expect?)
> b)  is it really "if (aBookmarkId < 1)"?

fixed (NS_ERROR_INVALID_ARG) and fixed
> 
> 5) GetKeywordForBookmark() and GetPageTitle(), I think:
> 
> a) both should call x.Truncate(0)
> b) both should have the "return NS_OK; // not found: return void x string"
> comment

fixed

> 
> 6)  /* XXXDietrich - vestigial? is not currently in nsINavBookmarksService
> 
> it looks like you are adding this commented out code.
> 

the patch removes that commented-out code.

Comment 30

[Asaf Romano (gone)]

Comment on attachment 256329 [details] [diff] [review]
front-end changes v4 - addresses mano's comments

It looks like you've backed out myk's patch from bookmarksProperties.js...

Comment 31

[Asaf Romano (gone)]

Also, why are the new helpers in the global scope? should go to PlacesUtils.

Comment 32

[Asaf Romano (gone)]

And also, I thought we can implement moveBookmark in a more efficient way (as in, change the parent and index of the given item).

Comment 33

[Dietrich Ayala (:dietrich)]

Attachment: front-end changes v5 - addresses mano's comments

(In reply to comment #30)
> (From update of attachment 256329 [details] [diff] [review])
> It looks like you've backed out myk's patch from bookmarksProperties.js...
> 

fixed

(In reply to comment #31)
> Also, why are the new helpers in the global scope? should go to PlacesUtils.
> 

I added them back to PlacesUtils, and added PlacesUtils as a |utils| property of PlacesBaseTransaction.

(In reply to comment #32)
> And also, I thought we can implement moveBookmark in a more efficient way (as
> in, change the parent and index of the given item).
> 

Yes, but optimizing |move| is not a high priority atm. I'll file a follow-up bug for this.

Comment 34

[Asaf Romano (gone)]

Comment on attachment 256379 [details] [diff] [review]
front-end changes v5 - addresses mano's comments

just some nits, we'll use follow-up for real bugs

>Index: browser/base/content/browser-sets.inc
>===================================================================
>     <!-- The command is disabled for the hidden window. Otherwise its enabled
>-         state is handler by the BookmarkAllTabsHandler object. -->
>+         state is handled by the BookmarkAllTabsHandler object. -->


thanks :)

>Index: browser/components/migration/...
>===================================================================
> 
> #ifdef MOZ_PLACES_BOOKMARKS
>-    bms->CreateFolder(parentFolder, importedOperaHotlistTitle, -1, &parentFolder);
>+    bms->CreateFolder(parentFolder, importedOperaHotlistTitle,
>+                      bms->DEFAULT_INDEX, &parentFolder);

It's a bit more correct to use nsINavBookmarkObserver::DEFAULT_INDEX (in all other places too).

>Index: browser/components/migration/src/nsSafariProfileMigrator.cpp
>===================================================================

>@@ -1066,19 +1068,21 @@ nsSafariProfileMigrator::ParseBookmarksF
>       // Encountered a Bookmark, so add it to the current folder...
>       CFDictionaryRef URIDictionary = (CFDictionaryRef)
>                       ::CFDictionaryGetValue(entry, CFSTR("URIDictionary"));
>       nsAutoString title, url;
>       if (GetDictionaryStringValue(URIDictionary, CFSTR("title"), title) &&
>           GetDictionaryStringValue(entry, CFSTR("URLString"), url)) {
> #ifdef MOZ_PLACES_BOOKMARKS
>         nsCOMPtr<nsIURI> uri;
>+        PRInt64 id;
>         rv |= NS_NewURI(getter_AddRefs(uri), url);
>-        rv |= aBookmarksService->InsertItem(aParentFolder, uri, -1);
>-        rv |= aBookmarksService->SetItemTitle(uri, title);
>+        rv |= aBookmarksService->InsertItem(aParentFolder, uri,
>+                                            aBookmarksService->DEFAULT_INDEX, &id);
>+        rv |= aBookmarksService->SetItemTitle(id, title);

This code was/is kinda wrong... we shouldn't set the item title if adding it failed. Please file a follow up.
> #else

>Index: browser/components/places/content/bookmarkProperties.js
>===================================================================


Note I hardly reviewed changes to this dialog.

>Index: browser/components/places/content/controller.js
>===================================================================


> /**
>  * Method and utility stubs for Place Edit Transactions
>  */
> function PlacesBaseTransaction() {
> }
> PlacesBaseTransaction.prototype = {
>+  utils: PlacesUtils,
>+
we could get rid of the bookmarks/history/livemarks getters now.


>-function PlacesRemoveItemTransaction(uri, oldContainer, oldIndex) {
>+function PlacesRemoveItemTransaction(id, uri, oldContainer, oldIndex) {

>+  this._title;
>+  this._placeURI;
>+  this._annotations = [];

remove those please.



>Index: browser/components/places/content/utils.js
>===================================================================

>   nodeIsBookmark: function PU_nodeIsBookmark(aNode) {
>     NS_ASSERT(aNode, "null node");
>-
>-    if (!this.nodeIsURI(aNode))
>-      return false;
>-    var uri = this._uri(aNode.uri);
>-    return this.bookmarks.isBookmarked(uri);
>+    return (aNode.bookmarkId > 0);

remove extra parentheses.

>+  },
>+
>+  getAnnotationsForURI: function PU_getAnnotationsForURI(aURI) {

documentaion?

...
>+  },

please get PlacesUtils.annotations only once in this method.

>+
>+  setAnnotationsForURI: function PU_setAnnotationsForURI(aURI, aAnnos) {

ditto and ditto.

Comment 35

[Asaf Romano (gone)]

 function run_test() {
+  // test roots
+  do_check_true(bmsvc.bookmarksRoot > 0);
+  do_check_true(bmsvc.toolbarRoot > 0);
+

test placesRoot too?


+    var options = histsvc.getNewQueryOptions();
+    options.maxResults = 1;
+    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
+    var query = histsvc.getNewQuery();
+    query.setFolders([tmpFolder], 1);
+    var result = histsvc.executeQuery(query, options);
+    var rootNode = result.root;
+    rootNode.containerOpen = true;
+    do_check_eq(rootNode.childCount, 3);
+    rootNode.containerOpen = false;

Do we have a test for containerOpen? is there any reason you're closing containers (here and in few other tests)?

Index: browser/components/places/tests/unit/test_history.js
===================================================================
+  var cc = root.childCount;
+  root.containerOpen = false;
+  do_check_eq(cc, 1);

|do_check_eq(root.childCount, 1);| would be eaier to fllow.

Comment 36

[Dietrich Ayala (:dietrich)]

Checking in browser/base/content/browser-sets.inc;
/cvsroot/mozilla/browser/base/content/browser-sets.inc,v  <--  browser-sets.inc
new revision: 1.94; previous revision: 1.93
done
Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v  <--  nsIEProfileMigrator.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in browser/components/migration/src/nsOperaProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp,v  <--  nsOperaProfileMigrator.cpp
new revision: 1.60; previous revision: 1.59
done
Checking in browser/components/migration/src/nsSafariProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp,v  <--  nsSafariProfileMigrator.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.30; previous revision: 1.29
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.127; previous revision: 1.126
done
Checking in browser/components/places/content/moveBookmarks.js;
/cvsroot/mozilla/browser/components/places/content/moveBookmarks.js,v  <--  moveBookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.73; previous revision: 1.72
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.55; previous revision: 1.54
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.12; previous revision: 1.11
done
Checking in browser/components/places/tests/bookmarks/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/bookmarks/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/bookmarks/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/places/tests/unit/test_annotations.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_annotations.js,v  <--  test_annotations.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/unit/test_history.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_history.js,v  <--  test_history.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/places/public/nsINavBookmarksService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v  <--  nsINavBookmarksService.idl
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <--  nsINavHistoryService.idl
new revision: 1.52; previous revision: 1.51
done
Checking in toolkit/components/places/src/nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v  <--  nsBookmarksHTML.cpp
new revision: 1.30; previous revision: 1.29
done
Checking in toolkit/components/places/src/nsLivemarkService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v  <--  nsLivemarkService.js
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.69; previous revision: 1.68
done
Checking in toolkit/components/places/src/nsNavBookmarks.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v  <--  nsNavBookmarks.h
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.106; previous revision: 1.105
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.69; previous revision: 1.68
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.80; previous revision: 1.79
done
Checking in toolkit/components/places/src/nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNavHistoryResult.h
new revision: 1.28; previous revision: 1.27
done

Comment 37

[Dietrich Ayala (:dietrich)]

(In reply to comment #34)
> (From update of attachment 256379 [details] [diff] [review])
> >Index: browser/components/migration/...
> >===================================================================
> > 
> > #ifdef MOZ_PLACES_BOOKMARKS
> >-    bms->CreateFolder(parentFolder, importedOperaHotlistTitle, -1, &parentFolder);
> >+    bms->CreateFolder(parentFolder, importedOperaHotlistTitle,
> >+                      bms->DEFAULT_INDEX, &parentFolder);
> 
> It's a bit more correct to use nsINavBookmarkObserver::DEFAULT_INDEX (in all
> other places too).

fixed

> 
> >Index: browser/components/migration/src/nsSafariProfileMigrator.cpp
> >===================================================================
> 
> >@@ -1066,19 +1068,21 @@ nsSafariProfileMigrator::ParseBookmarksF
> >       // Encountered a Bookmark, so add it to the current folder...
> >       CFDictionaryRef URIDictionary = (CFDictionaryRef)
> >                       ::CFDictionaryGetValue(entry, CFSTR("URIDictionary"));
> >       nsAutoString title, url;
> >       if (GetDictionaryStringValue(URIDictionary, CFSTR("title"), title) &&
> >           GetDictionaryStringValue(entry, CFSTR("URLString"), url)) {
> > #ifdef MOZ_PLACES_BOOKMARKS
> >         nsCOMPtr<nsIURI> uri;
> >+        PRInt64 id;
> >         rv |= NS_NewURI(getter_AddRefs(uri), url);
> >-        rv |= aBookmarksService->InsertItem(aParentFolder, uri, -1);
> >-        rv |= aBookmarksService->SetItemTitle(uri, title);
> >+        rv |= aBookmarksService->InsertItem(aParentFolder, uri,
> >+                                            aBookmarksService->DEFAULT_INDEX, &id);
> >+        rv |= aBookmarksService->SetItemTitle(id, title);
> 
> This code was/is kinda wrong... we shouldn't set the item title if adding it
> failed. Please file a follow up.

ok, i'm compiling a list of the follow-up bugs from this, will post as a separate comment after filing them all.

> > #else
> 
> >Index: browser/components/places/content/bookmarkProperties.js
> >===================================================================
> 
> 
> Note I hardly reviewed changes to this dialog.
> 
> >Index: browser/components/places/content/controller.js
> >===================================================================
> 
> 
> > /**
> >  * Method and utility stubs for Place Edit Transactions
> >  */
> > function PlacesBaseTransaction() {
> > }
> > PlacesBaseTransaction.prototype = {
> >+  utils: PlacesUtils,
> >+
> we could get rid of the bookmarks/history/livemarks getters now.
> 

will file follow-up
> 
> >-function PlacesRemoveItemTransaction(uri, oldContainer, oldIndex) {
> >+function PlacesRemoveItemTransaction(id, uri, oldContainer, oldIndex) {
> 
> >+  this._title;
> >+  this._placeURI;
> >+  this._annotations = [];
> 
> remove those please.
> 

fixed

> 
> >Index: browser/components/places/content/utils.js
> >===================================================================
> 
> >   nodeIsBookmark: function PU_nodeIsBookmark(aNode) {
> >     NS_ASSERT(aNode, "null node");
> >-
> >-    if (!this.nodeIsURI(aNode))
> >-      return false;
> >-    var uri = this._uri(aNode.uri);
> >-    return this.bookmarks.isBookmarked(uri);
> >+    return (aNode.bookmarkId > 0);
> 
> remove extra parentheses.

fixed
> 
> >+  },
> >+
> >+  getAnnotationsForURI: function PU_getAnnotationsForURI(aURI) {
> 
> documentaion?
> 

added

> ...
> >+  },
> 
> please get PlacesUtils.annotations only once in this method.
> 
> >+
> >+  setAnnotationsForURI: function PU_setAnnotationsForURI(aURI, aAnnos) {
> 
> ditto and ditto.
> 
fixed

(In reply to comment #35)
>  function run_test() {
> +  // test roots
> +  do_check_true(bmsvc.bookmarksRoot > 0);
> +  do_check_true(bmsvc.toolbarRoot > 0);
> +
> 
> test placesRoot too?

fixed

> 
> 
> +    var options = histsvc.getNewQueryOptions();
> +    options.maxResults = 1;
> +    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER],
> 1);
> +    var query = histsvc.getNewQuery();
> +    query.setFolders([tmpFolder], 1);
> +    var result = histsvc.executeQuery(query, options);
> +    var rootNode = result.root;
> +    rootNode.containerOpen = true;
> +    do_check_eq(rootNode.childCount, 3);
> +    rootNode.containerOpen = false;
> 
> Do we have a test for containerOpen? is there any reason you're closing
> containers (here and in few other tests)?

No tests for containerOpen yet. Closing the container stops live updating for that container.

> 
> Index: browser/components/places/tests/unit/test_history.js
> ===================================================================
> +  var cc = root.childCount;
> +  root.containerOpen = false;
> +  do_check_eq(cc, 1);
> 
> |do_check_eq(root.childCount, 1);| would be eaier to fllow.
> 

fixed

Comment 38

[Dietrich Ayala (:dietrich)]

Attachment: front-end changes v6 - addresses mano's comments

as checked in.

Comment 39

[Robert Sayre]

(In reply to comment #38)
>
> as checked in.

This checkin caused the Seneca windows bookmarks tinderbox to catch fire

http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest

Perhaps they are using an unsupported Visual Studio version. If that's the case, I say let them eat cake, I mean, let it burn. But is it?

Comment 40

[[e-mail] (:bhearsum)]

(In reply to comment #39)
> (In reply to comment #38)
> >
> > as checked in.
> 
> This checkin caused the Seneca windows bookmarks tinderbox to catch fire
> 
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest
> 
> Perhaps they are using an unsupported Visual Studio version. If that's the
> case, I say let them eat cake, I mean, let it burn. But is it?
> 

The Seneca builders are running VS2005, so that shouldn't be an issue.

Comment 41

[Robert Sayre]

(In reply to comment #40)
> 
> The Seneca builders are running VS2005, so that shouldn't be an issue.

Ah, right. I had forgetten that this box is the only test coverage we have for places with bookmarks at the moment, since it's not built by default. Real error, then.

Comment 42

[Dietrich Ayala (:dietrich)]

Attachment: fix build error

(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #38)
> > >
> > > as checked in.
> > 
> > This checkin caused the Seneca windows bookmarks tinderbox to catch fire
> > 
> > http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest
> > 
> > Perhaps they are using an unsupported Visual Studio version. If that's the
> > case, I say let them eat cake, I mean, let it burn. But is it?
> > 
> 
> The Seneca builders are running VS2005, so that shouldn't be an issue.
> 

This is my fault, missed some changes in the IE migrator. Ugh, why was this not killing my build?

Mano, carrying over your r+, as these are the same changes as in the rest of the migrator code.

Comment 43

[Dietrich Ayala (:dietrich)]

(In reply to comment #42)
> 
> This is my fault, missed some changes in the IE migrator. Ugh, why was this not
> killing my build?
> 

Because you were building on a mac, dumbass.

Comment 44

[Asaf Romano (gone)]

Comment on attachment 256436 [details] [diff] [review]
fix build error

InsertItem takes a pointer, thus &id in all places.

Comment 45

[Dietrich Ayala (:dietrich)]

Attachment: fix build error - v2

tested on winxp, builds ok and bookmarks import successfully.

Comment 46

[Asaf Romano (gone)]

Comment on attachment 256532 [details] [diff] [review]
fix build error - v2

r=mano.

Comment 47

[Dietrich Ayala (:dietrich)]

landed, regressions and dependent bugs fixed, so marking fixed.

Comment 48

[Gervase Markham [:gerv]]

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