Closed Bug 607309 Opened 9 years ago Closed 9 years ago

Modify nsINavBookmarkObserver::onItemAdded to include the URI

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #486069 - Flags: review?(mak77)
This is blocking beta 7 per conversation with beltzner on irc.
blocking2.0: --- → beta7+
Whiteboard: [needs review mak]
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
Attachment #486069 - Flags: review?(mak77) → review-
(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.
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
Attached patch v1.1 (obsolete) — Splinter Review
Luckily that was all easy to fix :)
Attachment #486069 - Attachment is obsolete: true
Attachment #486118 - Flags: review?(mak77)
Attached patch v1.2 (obsolete) — Splinter Review
Last version was slightly wrong...
Attachment #486118 - Attachment is obsolete: true
Attachment #486123 - Flags: review?(mak77)
Attachment #486118 - Flags: review?(mak77)
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.
Attachment #486123 - Flags: review?(mak77) → review+
Whiteboard: [needs review mak]
Whiteboard: [needs superreview]
Attachment #486123 - Flags: superreview?(mconnor)
Attachment #486123 - Flags: superreview?(mconnor) → superreview+
Attached patch v1.3Splinter Review
Per comments.
Attachment #486123 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs superreview] → [can land]
Thanks Chris!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b7
Blocks: FF2SM
Target Milestone: mozilla2.0b7 → mozilla2.0b8
This really did land for beta 7...
Target Milestone: mozilla2.0b8 → mozilla2.0b7
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...
No longer blocks: FF2SM
Comment on attachment 511169 [details] [diff] [review]
(Bv1) Correctly fix browser_library_views_liveupdate.js

Please file a new bug for this.
Attachment #511169 - Attachment is obsolete: true
Attachment #511169 - Flags: review?(mak77)
Depends on: 634245
(In reply to comment #16)
> Please file a new bug for this.

I filed bug 634245.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.