633274 - nsINavBookmarkObserver: also pass in GUID whenever we pass in an item id

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Philipp von Weitershausen [:philikon]]

Like bug 633266, but for bookmarks.

tl;dr: I propose adding a 'aGUID' parameter to all nsINavBookmarkObserver methods that
take a 'aItemId' argument, namely:

  onBeforeItemRemoved
  onItemAdded
  onItemChanged
  onItemMoved
  onItemRemoved
  onItemVisited

Comment 1

[Philipp von Weitershausen [:philikon]]

Summarizing IRC discussion with mak: If onItemAdded, onItemRemoved, and onItemMoved would also pass in the folder GUIDs (in addition to the folder IDs), Sync would not have to do any extra queries to find those out in its observer. That would reduce the overall amount of queries at the expense of doing an extra JOIN.

Comment 2

[Marco Bonardo [:mak]]

Attachment: wip

Just backing up work, this refactors some of the bookmarks service methods, to share a common data structure more sanely.  The result allows to not increase queries traffic, instead, by fixing observers, we can reduce it.

TODO:
- add grandParent (internal) and guids for parents (external).
- figure out some missing info in results observers.
- fix js observers adding missing arguments
- performance optimizations to internal observers
- add tests (by modifying existing ones)
- file a bug to fix Sync observers

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> - file a bug to fix Sync observers

Way ahead of you: bug 633062 :) In fact, it depends on this bug.

Comment 4

[Marco Bonardo [:mak]]

yes, but this bug is only about bookmarks observers, and I'd like Sync bookmarks observers being fixed even if history observers won't be ready.
So my idea was to have a "fix Sync bookmarks observers" bug blocking bug 633062 and depending on this one, but you could manage that differently :)

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> yes, but this bug is only about bookmarks observers, and I'd like Sync
> bookmarks observers being fixed even if history observers won't be ready.

Good point!

> So my idea was to have a "fix Sync bookmarks observers" bug blocking bug 633062
> and depending on this one, but you could manage that differently :)

That works for me. Filed bug 654900.

Comment 6

[Marco Bonardo [:mak]]

nice, thanks!

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Passes all tests locally, no new queries have been added, killed 3 methods, removed some minor queries traffic. Unfortunately all of this ended up being related changes up to a 150KB patch :(
Unless I find that some new argument is not that useful or I need more, while updating all observers, this should be pretty much done.

TODO (in separate parts):
- mechanical changes to all observers in the codebase
- optimizations to internal observers
- tests
- buy a wine bottle for the reviewer

Comment 8

[Marco Bonardo [:mak]]

Also, build once on gcc to catch warnings.

Comment 9

[Marco Bonardo [:mak]]

Attachment: idl changes v1.0

Will need SR.

Comment 10

[Marco Bonardo [:mak]]

Attachment: Places changes v1.0

This is a big patch, I suggest starting from the header file to figure out the changes at an higher level. most of it is refactoring to have methods go through a common fetching method (similar to what we did for history and icons).
I made a dedicated test to check each returned value (attaching later), so that you can trust the test more than me :)

Comment 11

[Marco Bonardo [:mak]]

Attachment: Sync changes v0.9

looks like this is failing one test, so I have to check why before going to review.

Comment 12

[Marco Bonardo [:mak]]

Attachment: Places observers changes v1.0

changes and optimizations to existing Places observers

Comment 13

[Marco Bonardo [:mak]]

Attachment: Test v1.0

Comment 14

[Marco Bonardo [:mak]]

Attachment: Sync changes v1.0

The Sync test failure was actually the test in need of update, it was expecting an empty string title and an empty description annotation, rather than null and no annotation.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 531576 [details] [diff] [review]
Sync changes v1.0

This patch really belongs in bug 654900. I would also like to land it separately from your other changes, on services-central, so that the crossweave suit runs against the push and we can QA the Sync changes before they go to m-c.

>+    Svc.Annos.setItemAnnotation(mRoot, EXCLUDEBACKUP_ANNO, 1, 0,
>+                                Svc.Annos.EXPIRE_NEVER);

<3

>     switch (type) {
>     case this._bms.TYPE_BOOKMARK:
>       this._log.debug("  -> removing bookmark " + guid);
>-      this._ts.untagURI(this._bms.getBookmarkURI(itemId), null);

Why are you removing this?

>       case "title":
>-        val = val || "";
>         this._bms.setItemTitle(itemId, val);
>         break;
...
>       case "description":
>-        val = val || "";
>-        Svc.Annos.setItemAnnotation(itemId, DESCRIPTION_ANNO, val, 0,
>-                                    Svc.Annos.EXPIRE_NEVER);
>+        if (val) {
>+          Svc.Annos.setItemAnnotation(itemId, DESCRIPTION_ANNO, val, 0,
>+                                      Svc.Annos.EXPIRE_NEVER);
>+        } else {
>+          Svc.Annos.removeItemAnnotation(itemId, DESCRIPTION_ANNO);
>+        }

Glad we have tests that make sure we can pass a null title + description in! Otherwise I'd feel less comfortable about this change.

>@@ -1310,23 +1314,23 @@ BookmarksTracker.prototype = {
>   },
> 
>   _GUIDForId: function _GUIDForId(item_id) {
>     // Isn't indirection fun...
>     return Engines.get("bookmarks")._store.GUIDForId(item_id);
>   },

We should be able to get rid of this function now, as well as the already unused _idForGuid(). The _ignore helper will have to be changed a bit, I suppose.

>+  onBeforeItemRemoved: function BMT_onBeforeItemRemoved(itemId, type, parentId,
>+                                                        guid, parentGuid) {
>+    if (this._ignore(itemId, parentId))

We could also do this work in onItemRemoved() now since we no longer need the bookmark to still be around to get at its GUID and parent GUID. No idea if that's in some preferred over onBeforeItemRemoved() -- I could imagine that onItemRemoved() might be notified async.


Removing r? since we should really be doing this in bug 654900, and we need a new patch.

Comment 16

[Marco Bonardo [:mak]]

(In reply to comment #15)
> Comment on attachment 531576 [details] [diff] [review] [review]
> Sync changes v1.0
> 
> This patch really belongs in bug 654900.

there is much more to do than this patch to fix that bug, but I'm fine having multiple parts in that bug, will move it there.

> I would also like to land it
> separately from your other changes, on services-central

ok fine by me

> >     switch (type) {
> >     case this._bms.TYPE_BOOKMARK:
> >       this._log.debug("  -> removing bookmark " + guid);
> >-      this._ts.untagURI(this._bms.getBookmarkURI(itemId), null);
> 
> Why are you removing this?

removeItem invokes onItemRemoved and the tagging service takes care of removing tags. So no need to do it apart.

> >@@ -1310,23 +1314,23 @@ BookmarksTracker.prototype = {
> >   },
> > 
> >   _GUIDForId: function _GUIDForId(item_id) {
> >     // Isn't indirection fun...
> >     return Engines.get("bookmarks")._store.GUIDForId(item_id);
> >   },
> 
> We should be able to get rid of this function now, as well as the already
> unused _idForGuid(). The _ignore helper will have to be changed a bit, I
> suppose.

I'd prefer that being another part worked from someone in Sync team, I don't know Sync code THAT well :)

> >+  onBeforeItemRemoved: function BMT_onBeforeItemRemoved(itemId, type, parentId,
> >+                                                        guid, parentGuid) {
> >+    if (this._ignore(itemId, parentId))
> 
> We could also do this work in onItemRemoved() now since we no longer need
> the bookmark to still be around to get at its GUID and parent GUID. No idea
> if that's in some preferred over onBeforeItemRemoved() -- I could imagine
> that onItemRemoved() might be notified async.

Sorry, I don't completely understand the second part of your comment, clarify please?

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 531576 [details] [diff] [review] [review] [review]
> > Sync changes v1.0
> > 
> > This patch really belongs in bug 654900.
> 
> there is much more to do than this patch to fix that bug, but I'm fine
> having multiple parts in that bug, will move it there.

Really, what else would be needed for bug 654900? _GUIDForId is afaik the only thing that spins the event loop in BookmarkTracker.

> > >     switch (type) {
> > >     case this._bms.TYPE_BOOKMARK:
> > >       this._log.debug("  -> removing bookmark " + guid);
> > >-      this._ts.untagURI(this._bms.getBookmarkURI(itemId), null);
> > 
> > Why are you removing this?
> 
> removeItem invokes onItemRemoved and the tagging service takes care of
> removing tags. So no need to do it apart.

Ah ok. Good to know.

> > >@@ -1310,23 +1314,23 @@ BookmarksTracker.prototype = {
> > >   },
> > > 
> > >   _GUIDForId: function _GUIDForId(item_id) {
> > >     // Isn't indirection fun...
> > >     return Engines.get("bookmarks")._store.GUIDForId(item_id);
> > >   },
> > 
> > We should be able to get rid of this function now, as well as the already
> > unused _idForGuid(). The _ignore helper will have to be changed a bit, I
> > suppose.
> 
> I'd prefer that being another part worked from someone in Sync team, I don't
> know Sync code THAT well :)

That's fine, I'll be happy to do it. Thanks for already getting us most of the way! :)

> > >+  onBeforeItemRemoved: function BMT_onBeforeItemRemoved(itemId, type, parentId,
> > >+                                                        guid, parentGuid) {
> > >+    if (this._ignore(itemId, parentId))
> > 
> > We could also do this work in onItemRemoved() now since we no longer need
> > the bookmark to still be around to get at its GUID and parent GUID. No idea
> > if that's in some preferred over onBeforeItemRemoved() -- I could imagine
> > that onItemRemoved() might be notified async.
> 
> Sorry, I don't completely understand the second part of your comment,
> clarify please?

I just wanted to point out that there's no compelling reason anymore, at least not from Sync's point of view, to use onBeforeItemRemoved. We could use onItemRemoved which might perform better perhaps. I'm totally guessing here, which is why I'm asking you.

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> I just wanted to point out that there's no compelling reason anymore, at
> least not from Sync's point of view, to use onBeforeItemRemoved. We could
> use onItemRemoved which might perform better perhaps. I'm totally guessing
> here, which is why I'm asking you.

Well, there is no particular difference apart that one runs before the removal (and the db churn) and one runs after the removal (And db churn). I don't think either would make interesting differences in perf.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 531517 [details] [diff] [review]
idl changes v1.0

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

r=sdwilsh

::: toolkit/components/places/nsINavBookmarksService.idl
@@ +203,5 @@
>  
>    /**
> +   * Notifies that the item was visited.  Can be invoked only for TYPE_BOOKMARK
> +   * items.  The reported time is not guaranteed to be the most recent visit to
> +   * this bookmark.

This seems sorta odd, but I understand why it happens.  I think explaining why might actually be better here:
The reported time is the time of the visit that was added, which may be well in the past since the visit time can be specified.  This means that the visit the observer is told about may not be the most recent visit.

In fact, maybe that should be an @note.

Comment 20

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

Comment on attachment 531517 [details] [diff] [review]
idl changes v1.0

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

>+  void onItemRemoved(in long long aItemId,
>+                     in long long aParentId,
>+                     in long aIndex,
>+                     in unsigned short aItemType,
>+                     in nsIURI aURI,
>+                     in ACString aGUID,
>+                     in ACString aParentGUID);

Forgot to add a comment for aURI here.

sr=me

Comment 21

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

oh, and a nit: s/associated to/associated with/

I just talked with Shawn and he had another idea - introducing a new interface for here rather than changing the old one. This would have the benefit of allowing people to "switch to the new interface", potentially allowing us to remove/deprecate the old one at some point, thus avoiding an observer interface that sends out two types of IDs (it would also involve fewer changes for binary addons, though that's less of a concern).

I haven't thought through all of the implications to that implementation-wise, but it seems like it might be a better approach.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> that sends out two types of IDs (it would also involve fewer changes for binary
> addons, though that's less of a concern).
I /think/ it is fewer changes to our code too, but mak should say more.

Comment 23

[Marco Bonardo [:mak]]

Personally for now I'd stick with this. It's not just for the work put into, but: 

1. these changes are absolutely backwards compatible for js consumers (cpp just have to update some signature and recompile). It's easier for everyone. For example as said above Sync would want to do their part of changes after we land these changes, to easy QA work.

2. we are not yet ready to rely only on guids, particularly we still don't have methods to get any information by guid, so even if we add a new interface with only guids nobody could use it in a decent way.

I'm fine with defining a new interface based only on guids but it's a lot more work than just defining it.

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #22)
> I /think/ it is fewer changes to our code too, but mak should say more.

It would be the opposite, mostly would be the same changes, plus I should change the notifying macro to add a new family of listeners, and add code to maintain those listeners.

Comment 25

[Shawn Wilsher :sdwilsh]

Comment on attachment 531518 [details] [diff] [review]
Places changes v1.0

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

Given gavin's latest comment, I'm going to hold off reviewing this other stuff for the time being until we resolve that.

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +3276,5 @@
>    if (aItemType == nsINavBookmarksService::TYPE_BOOKMARK &&
>        (mLiveUpdate == QUERYUPDATE_SIMPLE ||  mLiveUpdate == QUERYUPDATE_TIME)) {
>      nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>      NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>      nsresult rv = bookmarks->GetBookmarkURI(aItemId, getter_AddRefs(mRemovingURI));

The fact that we get the URI in this methods tells me that we should consider adding that in the observer notifications too.  The benefit being that it's one less thing we have to lookup (especially when we get our async bookmarking api)

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #25)
> The fact that we get the URI in this methods tells me that we should
> consider adding that in the observer notifications too.  The benefit being
> that it's one less thing we have to lookup (especially when we get our async
> bookmarking api)

The big patch has only signature updates, observers updates are in the Places observers changes patch (and this is done)

Comment 27

[Marco Bonardo [:mak]]

Actually there is also a third reason to not make a new interface now, our internal observers base most of their work on simple ids still, so these changes allow us to greatly reduce queries traffic.
Having a second interface right now would mean dropping any performance win, since internal observers should stick to the old interface with missing information.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> 1. these changes are absolutely backwards compatible for js consumers (cpp
> just have to update some signature and recompile). It's easier for everyone.
> For example as said above Sync would want to do their part of changes after
> we land these changes, to easy QA work.
Doing a new interface doesn't make any of this untrue either.  The downside to the modification is that we have to live with the crappy parts of this observer API forever (unless we want to break the world).  If we do a new interface, we can keep the old one around for a while (say, 6 - 12 months), then drop the old crufty stuff.  Of course, we'd have to update all of our internal code first, which is something we may have to do anyway when we make things more async.

Additionally, having both APIs means Sync can still do their changes after our stuff lands.

> 2. we are not yet ready to rely only on guids, particularly we still don't
> have methods to get any information by guid, so even if we add a new
> interface with only guids nobody could use it in a decent way.
True, but sync doesn't care about ids anymore after this, right?  That means we do support some basic use cases with only guids, and we can add more as we go (like the async bookmark API can be guid only, etc).

> I'm fine with defining a new interface based only on guids but it's a lot
> more work than just defining it.
I think most of that work is already done, with the exception of mechanical changes though, right?  The big thing is modifying the observer macros, but you already pass in all the information you'd need to them.

(In reply to comment #26)
> The big patch has only signature updates, observers updates are in the
> Places observers changes patch (and this is done)
What I looked at was the signature changes, specifically in OnBeforeItemRemoved which doesn't get a URI, right?

(In reply to comment #27)
> Actually there is also a third reason to not make a new interface now, our
> internal observers base most of their work on simple ids still, so these
> changes allow us to greatly reduce queries traffic.
> Having a second interface right now would mean dropping any performance win,
> since internal observers should stick to the old interface with missing
> information.
Why do we need to change our internal observers though?  The idea is that the new interface is strictly additive (neither observer interface is in the inheritance chain of the other), so one should not impact the other.  Could you elaborate more on this cost please?

Comment 29

[Marco Bonardo [:mak]]

(In reply to comment #28)
> Doing a new interface doesn't make any of this untrue either.  The downside
> to the modification is that we have to live with the crappy parts of this
> observer API forever (unless we want to break the world). 

Indeed I think we should do a new interface, but not right now.

> > 2. we are not yet ready to rely only on guids, particularly we still don't
> > have methods to get any information by guid, so even if we add a new
> > interface with only guids nobody could use it in a decent way.
> True, but sync doesn't care about ids anymore after this, right?  That means
> we do support some basic use cases with only guids, and we can add more as
> we go (like the async bookmark API can be guid only, etc).

Sync uses both guid and id, because if it has to query our APIs it can only do with old ids.

> I think most of that work is already done, with the exception of mechanical
> changes though, right?  The big thing is modifying the observer macros, but
> you already pass in all the information you'd need to them.

Yes, adding a new interface won't be that hard with these changes, but nobody will be able to use it till we add methods to get information by guid. So it's something we can do apart WITH new async methods to get stuff by guid.

> (In reply to comment #26)
> > The big patch has only signature updates, observers updates are in the
> > Places observers changes patch (and this is done)
> What I looked at was the signature changes, specifically in
> OnBeforeItemRemoved which doesn't get a URI, right?

it doesn't need a URI because now onItemRemoved gets a URI. indeed in a new interface we could not need onBeforeItemRemoved at all. In the observers changes patch OnBeforeItemRemoved is a no-op now.

> (In reply to comment #27)
> Why do we need to change our internal observers though?  The idea is that
> the new interface is strictly additive (neither observer interface is in the
> inheritance chain of the other), so one should not impact the other.  Could
> you elaborate more on this cost please?

As it is now history results and tagging service are getting a nice perf boost, mostly due to not having to query the db for information (they get it for free from the notifications). If I make a new interface I cannot do these optimizations since then notifications would not have the needed data.
Practically a new interface means taking "Places observers changes v1.0" patch and throwing it away, see all the optimizations to queries traffic there.

Comment 30

[Philipp von Weitershausen [:philikon]]

(In reply to comment #29)
> (In reply to comment #28)
> > Doing a new interface doesn't make any of this untrue either.  The downside
> > to the modification is that we have to live with the crappy parts of this
> > observer API forever (unless we want to break the world). 
> 
> Indeed I think we should do a new interface, but not right now.

If we can do it right now without too much cost, I could try. But the extra cost would involve adding some GUID-based APIs. I wonder if that's worth the effort if we want to move to async APIs anyway.

> > > 2. we are not yet ready to rely only on guids, particularly we still don't
> > > have methods to get any information by guid, so even if we add a new
> > > interface with only guids nobody could use it in a decent way.
> > True, but sync doesn't care about ids anymore after this, right?  That means
> > we do support some basic use cases with only guids, and we can add more as
> > we go (like the async bookmark API can be guid only, etc).
> 
> Sync uses both guid and id, because if it has to query our APIs it can only
> do with old ids.

This is what we need to know in Sync:

a) whether an item is a livemark
b) whether an given item or the item's parent is the tag folder
c) whether an item is excluded from backup

(a) and (c) are annotations. We could add an API to retrieve them by GUID. For (b) we could add an API to retrieve the tag folder's GUID. Of course these would just piggyback off the existing implementations and be synchronous :/

Comment 31

[Marco Bonardo [:mak]]

Actually I'm not even sure if we should continue this craziness of having history/annotations/bookmarks observers rather than just a single Places notification API.
I think a new API deserves much more design time and discussion rather than rushing one mimicking the existing one.

Comment 32

[Shawn Wilsher :sdwilsh]

(In reply to comment #31)
> I think a new API deserves much more design time and discussion rather than
> rushing one mimicking the existing one.
I'm fine with this at this point.

Comment 33

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

OK, makes sense.

Comment 34

[Shawn Wilsher :sdwilsh]

Comment on attachment 531518 [details] [diff] [review]
Places changes v1.0

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

r=sdwilsh

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +148,5 @@
> +      rv = row->GetInt64(2, &mData.bookmark.parentId);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      // lastModified (3) should not be set for the use-cases of this getter.
> +      rv = row->GetUTF8String(4, mData.bookmark.parentGuid);
> +      NS_ENSURE_SUCCESS(rv, rv);

I didn't care before when there were only two columns, but now there are six.  Can we pull the magic numbers into named constants please?

@@ +946,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    for (PRUint32 i = 0; i < bookmarks.Length(); ++i) {
> +      NS_ASSERTION(bookmarks[i].id != *aNewBookmarkId,
> +                   "GetBookmarksForURI should not return any tag.");

I actually think we should start using MOZ_ASSERT (#include "mozilla/Util.h") since all our assertions really should be fatal.  This applies to the whole patch.  The downside is that it doesn't take a message, but we can use comments then.  If you could use that for any assertion you add in this patch, that would be awesome.

@@ +1579,4 @@
>      return NS_OK;
>  
> +  // Make sure aNewParent is not aFolder or a subfolder of aFolder.
> +  // TODO: make this performant, maybe with a nested tree.

File a bug on this please and reference it here.

@@ +1700,5 @@
>  }
>  
> +nsresult
> +nsNavBookmarks::FetchItemInfo(PRInt64 aItemId,
> +                              BookmarkData& _bookmark)

Please use named constants in this method instead of magic numbers please.  It makes it a lot easier to check that we are using the right index.

@@ +2456,5 @@
>  }
>  
> +nsresult
> +nsNavBookmarks::GetBookmarksForURI(nsIURI* aURI,
> +                                   nsTArray<BookmarkData>& aBookmarks)

ditto here about named constants.

::: toolkit/components/places/nsNavBookmarks.h
@@ +76,2 @@
>    struct ItemVisitData {
> +    mozilla::places::BookmarkData bookmark;

You shouldn't need to use mozilla::places here since it's inside that namespace already.

@@ +80,5 @@
>      PRTime time;
>    };
>  
>    struct ItemChangeData {
> +    mozilla::places::BookmarkData bookmark;

Same here.

@@ +203,5 @@
> +   * @param aBookmark
> +   *        BookmarkData to store the information.
> +   */
> +  nsresult FetchItemInfo(PRInt64 aItemId,
> +                         mozilla::places::BookmarkData& _bookmark);

We should create a typedef (private) on this class so we don't need the namespacing here too:
typedef mozilla::places::BookmarkData BookmarkData;

@@ +420,5 @@
>     */
>    mozIStorageStatement* GetStatement(const nsCOMPtr<mozIStorageStatement>& aStmt);
>  
>    nsCOMPtr<mozIStorageStatement> mDBGetChildren;
>    // kGetInfoIndex_* results + kGetChildrenIndex_* results

What was this comment for again?  I have no idea now :(

@@ +473,5 @@
>        nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>        NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
> +      mozilla::places::BookmarkData folder;
> +      nsresult rv = bookmarks->FetchItemInfo(mID, folder);
> +      // TODO: store the BookmarkData struct instead.

Can you file a follow-up for this and put the bug number in the comment.  That'd also be a good first bug.

Comment 35

[Shawn Wilsher :sdwilsh]

Comment on attachment 531521 [details] [diff] [review]
Places observers changes v1.0

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

Can you also file a bug to deprecate onBeforeItemRemoved since it isn't needed anymore.  We can reduce some observer traffic by getting rid of it, which will be nice.

r=sdwilsh

::: toolkit/components/places/tests/bookmarks/test_bookmarks.js
@@ +57,5 @@
>      this._itemAddedIndex = index;
>      this._itemAddedURI = uri;
>  
>      // Ensure that we've created a guid for this item.
> +    do_check_valid_places_guid(guid);

This doesn't test the same thing that the old code tested.  Namely, that we've written the guid out to disk.  You can check that we are passed in the right (and valid) guid too if you want, but I'd like to keep the old check too.

Comment 36

[Shawn Wilsher :sdwilsh]

Comment on attachment 531522 [details] [diff] [review]
Test v1.0

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

r=sdwilsh

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that each nsINavBookmarksObserver method gets the correct input.run_test() {}

I think the run_test() bit is spurious ;)

@@ +21,5 @@
> +  },
> +
> +  // nsINavBookmarkObserver
> +  onBeginUpdateBatch: function onBeginUpdateBatch()
> +    this.validate(arguments.callee.name, Array.prototype.slice.call(arguments)),

you can just pass arguments here; it has a length property, so you don't need to bother with slicing since that's all you use in validate.

@@ +38,5 @@
> +  onItemMoved: function onItemMoved()
> +    this.validate(arguments.callee.name, Array.prototype.slice.call(arguments)),
> +
> +  // nsISupports
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsINavBookmarkObserver])

nit: trailing comma here please

Comment 37

[Marco Bonardo [:mak]]

(In reply to comment #35)
> ::: toolkit/components/places/tests/bookmarks/test_bookmarks.js
> @@ +57,5 @@
> >      this._itemAddedIndex = index;
> >      this._itemAddedURI = uri;
> >  
> >      // Ensure that we've created a guid for this item.
> > +    do_check_valid_places_guid(guid);
> 
> This doesn't test the same thing that the old code tested.  Namely, that
> we've written the guid out to disk.  You can check that we are passed in the
> right (and valid) guid too if you want, but I'd like to keep the old check
> too.

Really I think we made that to read the guid (no API can do that), but what you say is a nice testing effect, I'll restore it.

Comment 38

[Marco Bonardo [:mak]]

Attachment: idl changes v1.1

Comment 39

[Marco Bonardo [:mak]]

Attachment: Places changes v1.1

Comment 40

[Marco Bonardo [:mak]]

Attachment: Places observers changes v1.1

Moved the Sync changes to their own bug.

Comment 41

[Marco Bonardo [:mak]]

Attachment: Test v1.1

Comment 42

[Marco Bonardo [:mak]]

Thanks everybody here for quick replies!

http://hg.mozilla.org/projects/places/rev/e274253e6345
http://hg.mozilla.org/projects/places/rev/1b154964463b
http://hg.mozilla.org/projects/places/rev/5f50e7bea677
http://hg.mozilla.org/projects/places/rev/b8af03410cc0

Comment 43

[Marco Bonardo [:mak]]

Attachment: intermittent crash fix v1.0

I've pushed an additional changeset to fix an intermittent crash and move on the branch. I was unable to reproduce it on Windows, but was able to reproduce on Linux. nsTArray (as documented in the header), while being updated may memmove its entries, so passing out internal addresses it not a good idea at all, it fails 1 time out of 3.
I'd like to get a postreview on the change.
http://hg.mozilla.org/projects/places/rev/1cca7563001a

Comment 44

[Shawn Wilsher :sdwilsh]

Comment on attachment 532593 [details] [diff] [review]
intermittent crash fix v1.0

r=sdwilsh

Comment 45

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/e274253e6345
http://hg.mozilla.org/mozilla-central/rev/1b154964463b
http://hg.mozilla.org/mozilla-central/rev/5f50e7bea677
http://hg.mozilla.org/mozilla-central/rev/b8af03410cc0
http://hg.mozilla.org/mozilla-central/rev/1cca7563001a

Comment 46

[Marco Bonardo [:mak]]

ehr, this is dev-doc-needed to update nsINavBookmarkObserver docs