Shawn Wilsher :sdwilsh Assignee
	Description • 14 years ago

onItemAdded needs to include the URI of the bookmark we added so when we create an async API (bug 519514) consumers can find out which bookmark has which id.  They could make an educated guess now, but I'd rather let them be sure with a decent API.

I can do this without breaking JS add-ons, but binary add-ons are out of luck.

	Shawn Wilsher :sdwilsh Assignee
	Comment 2 • 14 years ago

This is blocking beta 7 per conversation with beltzner on irc.

	Marco Bonardo [:mak]
	Comment 3 • 14 years ago

Comment on attachment 486069 [details] [diff] [review]
v1.0

>diff --git a/browser/fuel/src/fuelApplication.js b/browser/fuel/src/fuelApplication.js
>--- a/browser/fuel/src/fuelApplication.js
>+++ b/browser/fuel/src/fuelApplication.js
>@@ -394,7 +394,7 @@ Bookmark.prototype = {
>   onEndUpdateBatch : function bm_oeub() {
>   },
> 
>-  onItemAdded : function bm_oia(aId, aFolder, aIndex) {
>+  onItemAdded : function bm_oia(aId, aFolder, aIndex, aURI) {

what you called aURI is aItemType, looks like we missed itemType addition here


>     // bookmark object doesn't exist at this point
>   },
> 
>@@ -547,7 +547,7 @@ BookmarkFolder.prototype = {
>   onEndUpdateBatch : function bmf_oeub() {
>   },
> 
>-  onItemAdded : function bmf_oia(aId, aFolder, aIndex) {
>+  onItemAdded : function bmf_oia(aId, aFolder, aIndex, aURI) {

ditto


>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

> nsNavHistoryFolderResultNode::OnItemAdded(PRInt64 aItemId,
>                                           PRInt64 aParentFolder,
>                                           PRInt32 aIndex,
>-                                          PRUint16 aItemType)
>+                                          PRUint16 aItemType,
>+                                          nsIURI* aURI)
> {

a bit deeper in this method we do GetBookmarkURI on aItemId, could you please fix it to use aURI?


>@@ -4158,7 +4160,7 @@ nsNavHistoryFolderResultNode::OnItemMove
>     if (aOldParent == mItemId)
>       OnItemRemoved(aItemId, aOldParent, aOldIndex, aItemType);
>     if (aNewParent == mItemId)
>-      OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType);
>+      OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType, nsnull);

this is wrong, if itemType is bookmark we should pass uri, sadly we don't know it and this will cause us to add a call to GetBookmarkURI.


>diff --git a/toolkit/components/places/tests/bookmarks/test_onBeforeItemRemoved_observer.js b/toolkit/components/places/tests/bookmarks/test_onBeforeItemRemoved_observer.js
>--- a/toolkit/components/places/tests/bookmarks/test_onBeforeItemRemoved_observer.js
>+++ b/toolkit/components/places/tests/bookmarks/test_onBeforeItemRemoved_observer.js
>@@ -63,7 +63,7 @@ Observer.prototype =
>   },
>   onEndUpdateBatch: function() {
>   },
>-  onItemAdded: function(id, folder, index) {
>+  onItemAdded: function(id, folder, index, uri) {

here it is itemType again

>diff --git a/toolkit/components/places/tests/unit/test_452777.js b/toolkit/components/places/tests/unit/test_452777.js
>--- a/toolkit/components/places/tests/unit/test_452777.js
>+++ b/toolkit/components/places/tests/unit/test_452777.js
>@@ -59,7 +59,7 @@ function run_test()
> 
>   // Now check to make sure it gets added with the right id
>   bs.addObserver({
>-    onItemAdded: function(aItemId, aFolder, aIndex)
>+    onItemAdded: function(aItemId, aFolder, aIndex, aURI)

and here too

	Shawn Wilsher :sdwilsh Assignee
	Comment 4 • 14 years ago

(In reply to comment #3)
> >@@ -4158,7 +4160,7 @@ nsNavHistoryFolderResultNode::OnItemMove
> >     if (aOldParent == mItemId)
> >       OnItemRemoved(aItemId, aOldParent, aOldIndex, aItemType);
> >     if (aNewParent == mItemId)
> >-      OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType);
> >+      OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType, nsnull);
> 
> this is wrong, if itemType is bookmark we should pass uri, sadly we don't know
> it and this will cause us to add a call to GetBookmarkURI.
Except that it doesn't matter in this case because it's just calling this class's method, which doesn't care about the URI anyway.  Passing null is fine here.

	Marco Bonardo [:mak]
	Comment 5 • 14 years ago

nsNavHistoryFolderResultNode::onItemAdded cares about uri, indeed see my getBookmarkURI comment above :)

donìt let be fooled by folder in the name, a folder cares about uris inside it

	Shawn Wilsher :sdwilsh Assignee
	Comment 6 • 14 years ago

Luckily that was all easy to fix :)

	Shawn Wilsher :sdwilsh Assignee
	Comment 7 • 14 years ago

Try server results here: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d10719614598

	Shawn Wilsher :sdwilsh Assignee
	Comment 8 • 14 years ago

Last version was slightly wrong...

	Marco Bonardo [:mak]
	Comment 9 • 14 years ago

Comment on attachment 486123 [details] [diff] [review]
v1.2

>diff --git a/toolkit/components/places/public/nsINavBookmarksService.idl b/toolkit/components/places/public/nsINavBookmarksService.idl

>@@ -3851,7 +3851,7 @@ nsNavHistoryFolderResultNode::OnItemAdde

>+    if (aNewParent == mItemId) {
>+      nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>+      NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>+
>+      nsCOMPtr<nsIURI> itemURI;
>+      if (aItemType == nsINavBookmarksService::TYPE_BOOKMARK) {
>+        nsresult rv =
>+          bookmarks->GetBookmarkURI(aItemId, getter_AddRefs(itemURI));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+      OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType, itemURI);
>+    }

Looks like we don't need to getBookmarksService() out of the TYPE_BOOKMARK check
This is trivial and can be fixed on push though.

	Shawn Wilsher :sdwilsh Assignee
	Comment 10 • 14 years ago

Per comments.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 11 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/3a1b694e8824 (please change status as appropriate)

	Shawn Wilsher :sdwilsh Assignee
	Comment 12 • 14 years ago

Thanks Chris!

	Shawn Wilsher :sdwilsh Assignee
	Comment 13 • 14 years ago

This really did land for beta 7...

	Serge Gautherie (:sgautherie)
	Comment 14 • 14 years ago

Comment on attachment 486188 [details] [diff] [review]
v1.3

>diff --git a/browser/components/places/tests/browser/browser_library_views_liveupdate.js b/browser/components/places/tests/browser/browser_library_views_liveupdate.js
>-  onItemAdded: function PSB_onItemAdded(aItemId, aFolderId, aIndex) {
>+  onItemAdded: function PSB_onItemAdded(aItemId, aFolderId, aIndex, aURI) {

Missing 'type' param...

	Shawn Wilsher :sdwilsh Assignee
	Comment 16 • 14 years ago

Comment on attachment 511169 [details] [diff] [review]
(Bv1) Correctly fix browser_library_views_liveupdate.js

Please file a new bug for this.

	Serge Gautherie (:sgautherie)
	Comment 17 • 14 years ago

(In reply to comment #16)
> Please file a new bug for this.

I filed bug 634245.