We have IHistory, but this isn't exposed to JS, nor is it really the right API for JS. Need to know exactly what Sync wants to sync before I can design the API for this, but will be trivial once I know that.

Right now Sync uses addVisit, isVisited, setPageTitle, removeAllPages, removePage of nsINavHistoryService. It also rolls a couple of custom queries which are already executed async.

(In reply to comment #1) > Right now Sync uses addVisit, isVisited, setPageTitle, removeAllPages, > removePage of nsINavHistoryService. It also rolls a couple of custom queries > which are already executed async. Merr. Let's keep this bug scoped to adding a visit. In Sync's case, it knows the title when it wants to add the visit, yeah?

(In reply to comment #2) > Merr. Let's keep this bug scoped to adding a visit. Ok, I wasn't sure what exactly the scope of the bug was. > In Sync's case, it knows the title when it wants to add the visit, yeah? Yes.

FWIW, it should be pretty trivial for Sync to go ahead and work around not having an async is visited API. Adding a visit is a lot more complicated, so you shouldn't try to do that on your own.

OK, the API I am proposing we go with is this: void addVisit(in nsIURI aURI, in PRTime aTime, in nsIURI aReferringURI, in long aTransitionType, in boolean aIsRedirect, in long long aSessionID, in AString aTitle, [optional] in nsIPlacesIdCallback aCallback); nsIPlacesIdCallback is spec'd in bug 519514 comment 21. I believe this would be sufficient for Sync, yeah?

Ideally one could want to set the title just once per page, and not once per visit. The title is bound to the page, running 2 queries on each visit to set the same title over and over seems worse at first glance. Maybe having something that one can pass uri, title and multiple visits details would be more performant (creates the page if needed, sets the title if needed, adds visits)

(In reply to comment #4) > FWIW, it should be pretty trivial for Sync to go ahead and work around not > having an async is visited API. We already do that. (In reply to comment #7) > Maybe having something that one can pass uri, title and multiple visits details > would be more performant (creates the page if needed, sets the title if needed, > adds visits) That would be ideal for Sync. In fact, it would be grand if the API would take a page title and a list of history visits, ignoring those that already exist.

(In reply to comment #7) > Ideally one could want to set the title just once per page, and not once per > visit. The title is bound to the page, running 2 queries on each visit to set > the same title over and over seems worse at first glance. Except we could see if we need to update the title pretty easily when we check moz_places to see if we know about the URI yet. Grabbing the title then is trivial, and then we can not write if it hasn't changed (much like how History.cpp handles it now). > Maybe having something that one can pass uri, title and multiple visits details > would be more performant (creates the page if needed, sets the title if needed, > adds visits) Although this makes me realize an interesting point. There's no asyncBatchAPI, which is likely something that sync needs. Maybe we need the method I mentioned above, plus another method which takes an array of all those arguments (one array of an object with all those arguments). On an unrelated note (but important for this bug), how does Sync handle session ids?

(In reply to comment #9) > Except we could see if we need to update the title pretty easily when we check > moz_places to see if we know about the URI yet. Grabbing the title then is > trivial, and then we can not write if it hasn't changed (much like how > History.cpp handles it now). Indeed what I'd like to avoid is that for each visit we go looking up if the place exists. If we are going to add multiple visits for a single page (that could be the most common op for Sync), it's a completely useless work..

(In reply to comment #10) > Indeed what I'd like to avoid is that for each visit we go looking up if the > place exists. If we are going to add multiple visits for a single page (that > could be the most common op for Sync), it's a completely useless work.. Er, how could we not? We can't really assume it's already been visited in the past, especially in the case of sync.

the point is that Sync does not need a "add a bunch of visits" interface, it needs a "add a bunch of visits for this page". The first visit addition for a certain page can do the check on "exists" and "title", but next visits additions for the same page ideally don't need that check at all. This should be done either at an interface level with a addPageWithVisits interface or by providing a batch interface and every addition inside batches could cache page status in a local hash so that next additions know if they can skip the checks.

(In reply to comment #12) > the point is that Sync does not need a "add a bunch of visits" interface, it > needs a "add a bunch of visits for this page". While that may be the end result, I'm not sure that's how sync will get all it's data right away. Sync team? > The first visit addition for a certain page can do the check on "exists" and > "title", but next visits additions for the same page ideally don't need that > check at all. Agreed. > This should be done either at an interface level with a addPageWithVisits > interface or by providing a batch interface and every addition inside batches > could cache page status in a local hash so that next additions know if they can > skip the checks. Batch interface that is async is real easy to make into a footgun (in fact, I'd posit it's hard to do without making it a footgun). addPageWithVisits sounds promising, but I'm not sure if it's super usable for Sync.

(In reply to comment #13) > (In reply to comment #12) > > the point is that Sync does not need a "add a bunch of visits" interface, it > > needs a "add a bunch of visits for this page". > While that may be the end result, I'm not sure that's how sync will get all > it's data right away. Sync team? We'd need a "add a bunch of visits for this page" API. Each engine processes records asynchronously as it downloads them. A history record contains: histUri: uri of the page title: title of the page visits: array of objects: date: datetime of the visit type: transition type of the visit See https://wiki.mozilla.org/Labs/Weave/Developer/BrowserObjects#history Right now for each incoming record we * fetch visits for the URI in question from local places DB, * individually add visits from the record that don't yet exist in local places DB, * set the page title See http://mxr.mozilla.org/services-central/source/fx-sync/services/sync/modules/engines/history.js#256 It would be very cool if there was an API call that would do all of that at once, at least one that would let us add all the visits in one go.

(In reply to comment #14) > It would be very cool if there was an API call that would do all of that at > once, at least one that would let us add all the visits in one go. So, what you really want is mak's addVisitsForPage then, yeah?

(In reply to comment #15) > So, what you really want is mak's addVisitsForPage then, yeah? Aye. Sorry if comment 8 wasn't clear.

Is this needed to improve Sync performance in Fennec?

doubt it, visits addition through the docshell is already async and we are working to improve it in bug 599969 currently. This is to add multiple visits in a bunch (import, migration, Sync, ...) through the backend.

ops sorry, I missed the Sync piece, this should improve sync performance on desktop and mobile, yes.

Most importantly, it's a necessary aspect for preventing bug 606062 (really bug 563538). I don't think lock contention hangs are "performance" bugs, at lesat when it's stuff like this. It'll likely also improve responsiveness during sync if we're not doing it on the main thread, but I think this is a blocker for the first piece alone. (It's hard to really tell what'll be left over if we deal with the lock contention problems.)

The spec is now ready to start implementing.

Not having specific APIs for 1) add a place 2) update a place 3) adding or updating a visit but squeezing them into one API is a bit conflated?

I don't see a use-case for why we'd want to do any of that intelligence in code external to Places. This is specifically a bulk API, designed for the Sync use-case of adding a bunch of data at once. If we separate those out, we have to do a lot more querying to figure that out, instead of just handing it to Places and letting that be an internal detail.

(In reply to comment #22) > but squeezing them into one API is a bit conflated? Right. It's not designed to be easy to implement. It's designed to be easy to use from the consumers standpoint and do as little disk I/O as possible. Minimizing disk I/O is the motivating factor here, is it not?

shawn, I don't really care how hard it is in terms of implementation details, and that is not what my comment was about in any way. my concern was that you are group 3-4 different ideas into one API. Sounds like this is some special interface between Sync and Places and not meant as a general API. If that is the case, I care less.

It is designed specifically with Sync in mind, but can easily be used as a general API.

Adding a place without specifying visit or bookmarks is globally a bad idea. Updating a place is fine only regarding title, and I think the proposed api covers that. Updating a visit is bad too, unless you mean deleting a visit. This api does not have stuff for deleting visits, true, for now we'll still rely on the old api that is half sync, half async. If the old API implements these things I don't care to reimplement them, we should kill those APIs too, soon or later.

The public interfaces for this. Going to hold off on review until I implement some more of this just to make sure it doesn't need to change.

We don't need to worry about other tests that define run_next_test because they redefine it successfully. Going to need to write more async tests, so we might as well move it up to head_common now.

Forgot to qrefresh...

Note: I'm trying very hard to not introduce a third, completely different-but-trying-to-do-the-same-thing code path for adding visits, so the next few parts are going to be refactorings.

This adds two methods onto VisitData. One is a new constructor that will get the spec and reversed hostname for us automatically. The other sets the transition type and then hidden and typed based on the transition type. Makes the logic a bit simpler when we go ahead and reuse this struct for the new API.

More code sharing!

Comment on attachment 498895 [details] [diff] [review] Part 3 - stub out mozIAsyncHistory methods v1.0 I think this is going to change a bit since I need to switch to the JS API due to bug 621005.

wip of the actual implementation. This doesn't yet pass all the tests that are in this diff.

I don't expect these to change anymore, so I'm going to request r and sr now.

Comment on attachment 499876 [details] [diff] [review] Part 1 - public interfaces v1.1 Upon further inspection, the idl docs are not good enough yet. Will request r and sr when they are satisfactory to me.

More work. I now have all tests that check for bad data being passed in (before we do any work) passing. I believe I have full coverage on those situations as well.

We need to pass both of these pieces of data around with the new updatePlaces method, so adding them to the VisitData struct.

Spent most of the day refactoring this wip so we have more code re-use and updatePlaces was easier to follow along with. updatePlaces now collects all the data it needs to add visits, and I could even implement a naive version that doesn't have callback support that the Sync team could start playing with tomorrow.

Simple refactoring to make my next step a little more clear.

The deeper I get into the implementation of this, and the harder I look at what data Sync has and how the server sends it (https://wiki.mozilla.org/Labs/Weave/Developer/BrowserObjects#history), I have found a few issues with Sync and Places. I've filed two bugs on Sync (bug 623667 and bug 623665) about these issues. Fixing those issues would mean we'd want a different API for Sync than what I'm implementing in this bug, I think (this stuff is complicated, and I'm not finding some of these issues until I implement something because they are subtle). I'm going to move forward with implementing this because I don't think Sync will be fixing either of those two bugs before we ship. Keep in mind that we may want to throw this away in the future.

Upon further thought, the API is likely fine as-is regardless of the possible Sync changes.

This has a TODO that I'd like to address in the next part to keep the diffs smaller.

Comment on attachment 499052 [details] [diff] [review] Part 6 - move inserting/updating moz_place entries to History v1.0 >+ /** >+ * Updates an entry in moz_places with the data in aVisitData. >+ * >+ * @param aVisitData >+ * The visit data to use to populate the existing row in moz_places. nit: s/populate/update/ r=me

(In reply to comment #48) > This has a TODO that I'd like to address in the next part to keep the diffs > smaller. Actually, let's do this in a follow-up bug. We probably want it for mobile, but we can get the API done and working first. Filed bug 623969 for it.

Updated TODO comment to include the bug #

Comment on attachment 498874 [details] [diff] [review] Part 2 - refactor tests v1.1 >diff --git a/toolkit/components/places/tests/head_common.js b/toolkit/components/places/tests/head_common.js >+ >+/** >+ * Runs the next test in the gTests array. gTests should be defined in each >+ * test file. >+ */ >+let gTestIndex = 0; // The index of the currently running test. >+function run_next_test() >+{ ... >+} this part can be removed since I already implemented it in bug 615992 (will most likely land it tomorrow) Based on those changes, in each test where you are replacing run_next_test with the shared version, you should either not call do_test_pending or call do_test_finished in the last test. >diff --git a/toolkit/components/places/tests/migration/test_v11_from_v10.js b/toolkit/components/places/tests/migration/test_v11_from_v10.js ditto >diff --git a/toolkit/components/places/tests/migration/test_v11_from_v10_migrated_from_v11.js b/toolkit/components/places/tests/migration/test_v11_from_v10_migrated_from_v11.js ditto >diff --git a/toolkit/components/places/tests/unit/test_sql_guid_functions.js b/toolkit/components/places/tests/unit/test_sql_guid_functions.js ditto not doing so would cause tests to timeout. Thus, r+ with the above fixed (at that point the patch will be pretty much small, mostly code removals).

Comment on attachment 499878 [details] [diff] [review] Part 3 - stub out mozIAsyncHistory methods v1.1 (checked in) >diff --git a/toolkit/components/places/tests/unit/test_async_history_api.js b/toolkit/components/places/tests/unit/test_async_history_api.js >+function run_test() >+{ >+ do_test_pending(); >+ run_next_test(); >+} for the previous comment reasoning, do_test_pending is useless and timing out here. This part 3 is a bit out of context here, should probably be numbered later since it's unimplemented, there is not much to review as it is.

(In reply to comment #53) > This part 3 is a bit out of context here, should probably be numbered later > since it's unimplemented, there is not much to review as it is. The test is actually important here; my first implementation didn't implement QI properly and that is what it's testing.

Comment on attachment 499011 [details] [diff] [review] Part 4 - refactor hidden flag logic v1.0 >diff --git a/toolkit/components/places/src/Helpers.h b/toolkit/components/places/src/Helpers.h >+/** >+ * Determines if a visit should be marked as hidden given its transition type >+ * and whether or not it was a redirect. >+ * >+ * @param aIsRedirect >+ * True if this visit was due to a redirect, false otherwise. While transition_type == some_redirect_type indicates if a visit comes FROM a redirect, isRedirect usually indicates if a visit IS a redirect. The comment looks confusing saying "due to a redirect". Indeed there is no reason to hide a redirect destination, while there is to hide a redirect source. the old comment in history was largely too verbose but was almost well explaining the fact. nit: javadoc is missing a @return the changes look good but the fact the 2 checks are different between sync and async is a bit surprising, btw we discussed it on irc and doesn't look a big deal.

Comment on attachment 499025 [details] [diff] [review] Part 5 - refactor VisitData v1.0 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+ VisitData(nsIURI* aURI) >+ : placeId(0) >+ , visitId(0) >+ , sessionId(0) >+ , hidden(false) >+ , typed(false) >+ , transitionType(-1) >+ , visitTime(0) I'd like us to use a stronger base value for hidden (a page should be considered hidden till we say the opposite) > PRInt64 placeId; > PRInt64 visitId; > PRInt64 sessionId; >@@ -205,7 +236,7 @@ struct VisitData { > nsString revHost; > bool hidden; > bool typed; >- PRInt32 transitionType; >+ PRUint32 transitionType; I don't like the fact we assign -1 as default value for a uint, even if it can be handled correctly.

Comment on attachment 499066 [details] [diff] [review] Part 7 - pull FetchPageInfo onto History v1.0 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+ bool exists = mHistory->FetchPageInfo(mPlace); >+ if (!exists) { >+ // We have no record of this page, so there is no need to do any further >+ // work. >+ return NS_OK; > } > >- NS_ASSERTION(placeId > 0, "We somehow have an invalid place id here!"); >+ NS_ASSERTION(mPlace.placeId > 0, >+ "We somehow have an invalid place id here!"); could make sense to put the assertion directly inside FetchPageInfo

Comment on attachment 502044 [details] [diff] [review] Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp > private: >- InsertVisitedURI(mozIStorageConnection* aConnection, >- VisitData& aPlace) >+ InsertVisitedURIs(mozIStorageConnection* aConnection, >+ nsTArray<VisitData>& aPlace) rename aPlace to aPlaces? >+ nsNavHistory* navHistory = nsNavHistory::GetHistoryService(); >+ NS_ABORT_IF_FALSE(navHistory, "Could not get nsNavHistory?!"); this means we crash in opt builds in such a case? > mozIStorageConnection* mDBConn; > >- VisitData mPlace; >- VisitData mReferrer; >+ nsTArray<VisitData> mPlaces; >+ nsTArray<VisitData, nsTArrayInfallibleAllocator> mReferrers; Why do you want only referrers to be infallible, just because they are far less?

(In reply to comment #55) > While transition_type == some_redirect_type indicates if a visit comes FROM a > redirect, isRedirect usually indicates if a visit IS a redirect. The comment > looks confusing saying "due to a redirect". Indeed there is no reason to hide a > redirect destination, while there is to hide a redirect source. the old comment > in history was largely too verbose but was almost well explaining the fact. While that comment was useful, it didn't seem relevant to the place it was at (nor was I really sure where to put it).

(In reply to comment #57) > could make sense to put the assertion directly inside FetchPageInfo Had it here because we used to do different things based on if it was found or not. We could probably do away with the assertion, to be honest.

(In reply to comment #58) > Why do you want only referrers to be infallible, just because they are far > less? Well, mPlaces would never do allocations, whereas mReferrers does. However, nsTArray is now infallible on trunk, so I've removed that.

Addresses review comments.

Addresses review comments.

Addresses review comments.

Addresses review comments.

Addresses review comments.

Addresses review comments.

Addresses review comments.

Addresses review comments.

Comment on attachment 502850 [details] [diff] [review] Part 2 - refactor tests v1.2 (checked in) https://hg.mozilla.org/projects/places/rev/ae99d2b2efaa

Comment on attachment 502854 [details] [diff] [review] Part 4 - refactor hidden flag logic v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/725a035ad3e4

Comment on attachment 502855 [details] [diff] [review] Part 5 - refactor VisitData v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/705a34ab2091

Comment on attachment 502858 [details] [diff] [review] Part 6 - move inserting/updating moz_place entries to History v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/1e0f15394795

Comment on attachment 502861 [details] [diff] [review] Part 7 - pull FetchPageInfo onto History v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/90ba73a93180

Comment on attachment 502863 [details] [diff] [review] Part 8 - add guid and referrerSpec to VisitData v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/ea72cd89fcdd

Comment on attachment 502867 [details] [diff] [review] Part 9 - Move reffer loading logic into a helper method v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/c64ae2a271f6

Comment on attachment 502868 [details] [diff] [review] Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/de15ef2a9d44

I need to do this in our new method too, so moving all this out into a helper makes sense.

Part 10 had a bug in it which I just caught with a test. Since I already landed it, Part 12 has been created :)

More work in progress. This is actually broken up in my queue into multiple parts, but not ready to attach some of them yet until I'm sure they are correct. All the tests that I've written so far now pass, but I got a bunch more to write.

Comment on attachment 503004 [details] [diff] [review] Part 12 - Fix issues with multiple visits of the same URI (checked in) >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >- bool IsSamePlaceAs(const VisitData& aOther) const >+ bool IsSamePlaceAs(VisitData& aOther) > { >- return spec.Equals(aOther.spec); >+ if (!spec.Equals(aOther.spec)) { >+ return false; >+ } >+ >+ aOther.placeId = placeId; >+ aOther.guid = guid; >+ return true; do we care about hidden and typed in this fake-cloning or are them always overwritten?

(In reply to comment #81) > do we care about hidden and typed in this fake-cloning or are them always > overwritten? transition type is required for visits, and both of those are set in that case. I think we are OK in that situation.

Comment on attachment 502968 [details] [diff] [review] Part 11 - refactor embed visit adding (checked in) https://hg.mozilla.org/projects/places/rev/b9369c68197b

Comment on attachment 503004 [details] [diff] [review] Part 12 - Fix issues with multiple visits of the same URI (checked in) https://hg.mozilla.org/projects/places/rev/99bc471608e1

In another part, I'll have us update it. That proves to be a bit more difficult to do cleanly, however.

Like part 13, but for the guid.

There are only two known things left to do with this before it is 100% functional (as far as I can tell) for Sync to use: 1) title changes to things that already exist in moz_places do not work 2) guid changes to things that already exist in moz_places do not work I spent the day writing a bunch of tests, and found some minor issues (part 13 - part 15), but things have been going pretty smoothly. There are, however, a bunch more tests I can write and I may find minor issues still. The plus side is that with each test I write, the risk with this code goes down.

Fairly certain that these are stable at this point.

Comment on attachment 503364 [details] [diff] [review] Part 1 - public interfaces v1.2 >diff --git a/toolkit/components/places/public/mozIAsyncHistory.idl b/toolkit/components/places/public/mozIAsyncHistory.idl >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/places/public/mozIAsyncHistory.idl >@@ -0,0 +1,153 @@ >... >+[scriptable, uuid(1a3b1260-4bdb-45d0-a306-dc377dd9baa4)] >+interface mozIVisitInfo : nsISupports >+{ >+ /** >+ * The local id of the visit. >+ */ >+ readonly attribute long long visitId; >+ >+ /** >+ * The time the visit occurred. >+ */ >+ readonly attribute PRTime date; It *might* be good to name this visitDate but I am fine the way it is. For example, if you need to add another date attribute in the future. >+ >+ /** >+ * The transition type used to get to this visit. One of the TRANSITION_TYPE >+ * constants on nsINavHistory. >+ */ >+ readonly attribute unsigned long transitionType; >+ >+ /** >+ * The referring URI of this visit. This may be null. >+ */ >+ readonly attribute nsIURI referrer; nsISHEntry.idl uses referrerURI and it might be a good thing here too. Leave it up to you. >... >+[scriptable, uuid(f79ca67c-7e57-4511-a400-ea31001c762f)] >+interface mozIAsyncHistory : nsISupports >+{ >+ /** >+ * Adds a set of visits for one or more mozIPlaceInfo objects, or updates each >+ * mozIPlaceInfo's title or guid. >+ * >+ * @param aPlaceInfo >+ * The mozIPlaceInfo object[s] containing the information to store or >+ * update. This can be a single object, or an array of objects. >+ * @param [optional] aCallback >+ * Callback to be notified for each visit addition. >+ * >+ * @throws NS_ERROR_INVALID_ARG >+ * - Passing in NULL for aPlaceInfo. >+ * - Not providing at least one valid guid, placeId, or uri for any >+ * mozIPlaceInfo object. s/any mozIPlaceInfo object/all mozIPlaceInfo object[s]/ or something similar >+ * - Providing a non-array object for visits (visits can be undefined >+ * for title or guid changes, however). nit: this comment could be made a bit clearer. >+ * - Not providing a date and transtionType for each mozIVisitInfo. >+ * - Providing an invalid transitionType for a mozIVisitInfo. >+ */ >+ [implicit_jscontext] >+ void updatePlaces(in jsval aPlaceInfo, >+ [optional] in mozIVisitInfoCallback aCallback); >+ >+};

Comment on attachment 503364 [details] [diff] [review] Part 1 - public interfaces v1.2 >diff --git a/toolkit/components/places/public/mozIAsyncHistory.idl b/toolkit/components/places/public/mozIAsyncHistory.idl >+[scriptable, uuid(1a3b1260-4bdb-45d0-a306-dc377dd9baa4)] >+interface mozIVisitInfo : nsISupports >+{ >+ /** >+ * The local id of the visit. >+ */ >+ readonly attribute long long visitId; >+ I don't undestand what "local" is meant to say here... Maybe "internal"? I'm not sure what's the best english term here. >+ /** >+ * The time the visit occurred. >+ */ >+ readonly attribute PRTime date; I second suggestion from rs, plus this is more a datetime than just a date. visitTime sounds better to me. >+ /** >+ * The transition type used to get to this visit. One of the TRANSITION_TYPE >+ * constants on nsINavHistory. >+ */ >+ readonly attribute unsigned long transitionType; add a @see nsINavHistory.idl >+ /** >+ * The referring URI of this visit. This may be null. >+ */ >+ readonly attribute nsIURI referrer; I second rs suggestion for referrerURI, as well. >+ /** >+ * The sessionId of this visit. >+ */ add @see nsINavHistory.idl >+[scriptable, uuid(ad83e137-c92a-4b7b-b67e-0a318811f91e)] >+interface mozIPlaceInfo : nsISupports >+{ >+ /** >+ * The local id of the place. >+ */ >+ readonly attribute long long placeId; ditto for "local" >+[scriptable,function, uuid(3b97ca3c-5ea8-424f-b429-797477c52302)] >+interface mozIVisitInfoCallback : nsISupports >+{ >+ /** >+ * Called when one of the visit methods has added a visit. >+ * >+ * @param aResultCode >+ * nsresult of the change indicating success or failure reason. >+ * @param aPlaceInfo >+ * The information that was being entered into the database. >+ */ >+ void onComplete(in nsresult aResultCode, >+ in mozIPlaceInfo aPlaceInfo); It's unclear from the comment if this is called every time a single "visit" is added or a "place with multiple visits" is added... From the comment looks like it's the former, is that correct? >+[scriptable, uuid(f79ca67c-7e57-4511-a400-ea31001c762f)] >+interface mozIAsyncHistory : nsISupports >+{ >+ /** >+ * Adds a set of visits for one or more mozIPlaceInfo objects, or updates each >+ * mozIPlaceInfo's title or guid. This can get some more wording probably >+ * @throws NS_ERROR_INVALID_ARG >+ * - Passing in NULL for aPlaceInfo. >+ * - Not providing at least one valid guid, placeId, or uri for any >+ * mozIPlaceInfo object. >+ * - Providing a non-array object for visits (visits can be undefined >+ * for title or guid changes, however). >+ * - Not providing a date and transtionType for each mozIVisitInfo. typo: transtionType

Comment on attachment 503309 [details] [diff] [review] Part 13 - Set page title if we have it v1.0 clearing requests per IRC discussion

As discussed on irc, plus more correct w.r.t. title length.

(In reply to comment #91) > It's unclear from the comment if this is called every time a single "visit" is > added or a "place with multiple visits" is added... > From the comment looks like it's the former, is that correct? Called for every single visit added (former).

Addresses review and sr comments.

Comment on attachment 503593 [details] [diff] [review] Part 1 - public interfaces v1.3 (checked in) https://hg.mozilla.org/projects/places/rev/44797377722c

Comment on attachment 499878 [details] [diff] [review] Part 3 - stub out mozIAsyncHistory methods v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/b1388b6e2119

This could probably be split into two parts, however how I originally started on it made sense to do it as one. We need the place id and the guid if we need to notify a callback method, so get that information when we insert a new place (we'll have it already in the case that we are just updating the place). We also need the visit id after inserting a visit, but for that specific visit. We used to be fine getting the most recent visit because history was always current (except if the the user changed their clock). We know the visit time of the visit we just added, so query on that to get the right visit (although, we could possibly also end up getting the wrong visit if we have multiple visits at the same time, but I'm not sure I care about that edge case).

Comment on attachment 503620 [details] [diff] [review] Part 17 - Create mozI[Place|Visit]Info objects and use them v1.0 Asking Luke to review the jsapi usage here.

With this, the Sync team can start implementing on top of this. This provides all the API functionality that they need, AFAIK. There are still a few things broken here, and this patch probably needs to be cleaned up a bit. The following things still need to be done, but probably with parts after this: - Dispatch begin and end batch notifications (+ tests) - Tests to make sure we dispatch proper visit notifications for our current notification API - Tests for error cases (like adding a visit with a guid already in the database). Of course, anything there about implementing tests could uncover bugs, but I don't think that is likely at this point (I've been known to be wrong in the past, however). I'd also like to scale back the API for Firefox 4.0 to just what the Sync team needs. That means we cut (for now) the ability to change the title or guid without adding visits. This won't be a problem for Sync, and can be added later without changing the API signature, but looks to be a lot of work for little gain right this late in our shipping cycle. If robstrong is OK with this, I'll put up another patch changing the IDL accordingly. Lastly, I'd like to mark nsINavHistory::addVisit as [deprecated]. Objections?

Comment on attachment 503571 [details] [diff] [review] Part 13 - Set page title if we have it v2.0 (checked in) https://hg.mozilla.org/projects/places/rev/793fd0ed8c3f

Comment on attachment 503572 [details] [diff] [review] Part 14 - Set guid when provided v2.0 (checked in) https://hg.mozilla.org/projects/places/rev/30555fb614e1

Comment on attachment 503357 [details] [diff] [review] Part 15 - only get session id if needed v1.0 (checked in) https://hg.mozilla.org/projects/places/rev/f2eab9a3d674

Comment on attachment 503616 [details] [diff] [review] Part 16 - Handle a mozIVisitInfoCallback v1.0 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >--- a/toolkit/components/places/src/History.cpp >+++ b/toolkit/components/places/src/History.cpp >@@ -328,6 +328,38 @@ private: > }; > > /** >+ * Notifies a callback object about completion. >+ */ >+class NotifyCompletion : public nsRunnable >+{ >+public: >+ NotifyCompletion(mozIVisitInfoCallback* aCallback, >+ VisitData& aPlace, can we make this const? >+ NS_IMETHOD Run() >+ { >+ NS_PRECONDITION(NS_IsMainThread(), >+ "This should be called on the main thread"); >+ >+ // TODO build a mozIPlaceInfo object for the visit. I guess this is something that will be done in a next part, if it's not so you should annotate a bug #. >+private: >+ mozIVisitInfoCallback* mCallback; I guess if we should instead hold a strong reference to the callback, this runnable should keep it alive no? >+ >+ // We AddRef on the main thread, and release it when we are destroyed. >+ NS_IF_ADDREF(mCallback); >+ } >+ >+ virtual ~InsertVisitedURIs() >+ { >+ if (mCallback) { >+ nsCOMPtr<nsIThread> mainThread = do_GetMainThread(); >+ (void)NS_ProxyRelease(mainThread, mCallback); ah, we do that here... Now please double check that the notify runnable cannot live out this runnable. It looks like that could happen at first glance. >@@ -668,6 +746,8 @@ private: > nsTArray<VisitData> mPlaces; > nsTArray<VisitData> mReferrers; > >+ mozIVisitInfoCallback* mCallback; add a comment regarding how/where the ownership for this is handled, for added clarity. >+ if (aCallback) { >+ // NotifyCompletion does not hold a strong reference to the callback, so we >+ // have to manage it. >+ NS_ADDREF(aCallback); Looks like we don't do this in the code above... is not possible to manage a strong reference to the callback in the notify runnable and proxyrelease it? I'm a bit worried by having external management of ownership here. r=me with the ownership hell clarified/answered

(In reply to comment #108) > can we make this const? Signs point to yes. > I guess this is something that will be done in a next part, if it's not so you > should annotate a bug #. Done in part 17...I probably should have annotated it, but meh. > I guess if we should instead hold a strong reference to the callback, this > runnable should keep it alive no? We cannot addref or release it off of the main thread, which we'd do if we had a strong reference. > ah, we do that here... Now please double check that the notify runnable cannot > live out this runnable. It looks like that could happen at first glance. While it could outlive it, it'll be on the main thread and use the pointer before the proxied release runs on the main thread (all events are serialized), so we should be fine. > add a comment regarding how/where the ownership for this is handled, for added > clarity. Sure. > >+ if (aCallback) { > >+ // NotifyCompletion does not hold a strong reference to the callback, so we > >+ // have to manage it. > >+ NS_ADDREF(aCallback); > > Looks like we don't do this in the code above... We manage it by addreffing it, no? > is not possible to manage a strong reference to the callback in the notify > runnable and proxyrelease it? Sadly, no because of different thread issues.

(In reply to comment #109) > While it could outlive it, it'll be on the main thread and use the pointer > before the proxied release runs on the main thread (all events are serialized), > so we should be fine. Although, we didn't force the proxy to happen (pass PR_TRUE as the third argument), which would be safer and I've done that now.

Addresses review comments. I added a bunch of comments to explain the ownership model a bit better. It sucks, but there's a good way to deal with this short of making a proxy object that handles the ownership of mCallback (which I suppose I could do if we really want to).

Comment on attachment 503890 [details] [diff] [review] Part 16 - Handle a mozIVisitInfoCallback v1.1 (checked in) http://hg.mozilla.org/projects/places/rev/ed509b5771d3

Comment on attachment 503699 [details] [diff] [review] Part 19 - Get all the data that we need v1.0 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >@@ -555,14 +564,34 @@ private: >- nsCOMPtr<mozIStorageStatement> stmt = >- mHistory->syncStatements.GetCachedStatement( >+ nsCOMPtr<mozIStorageStatement> stmt; >+ // If we have a visitTime, we want information on that specific visit. >+ if (_place.visitTime) { >+ stmt = mHistory->syncStatements.GetCachedStatement( >+ "SELECT id, session, visit_date " >+ "FROM moz_historyvisits " >+ "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " >+ "AND visit_date = :visit_date " a simple JOIN seems to produce a slightly shorter explain here, performances are about the same though since timers don't have enough resolution to differentiate, so I'll leave it up to you. > // Now that it should be in the database, we need to obtain the id of the >- // place we just added. >- bool visited = FetchVisitInfo(_place); >- if (visited) { >- NS_NOTREACHED("Not visited after adding a visit!"); >- } >+ // visit we just added. >+ (void)FetchVisitInfo(_place); I guess you removed this additional check because we would fail before? otherwise doesn't look a bad check to leave in

(In reply to comment #113) > > // Now that it should be in the database, we need to obtain the id of the > >- // place we just added. > >- bool visited = FetchVisitInfo(_place); > >- if (visited) { > >- NS_NOTREACHED("Not visited after adding a visit!"); > >- } > >+ // visit we just added. > >+ (void)FetchVisitInfo(_place); > > I guess you removed this additional check because we would fail before? > otherwise doesn't look a bad check to leave in It was actually completely bogus and would always return false because we didn't pass in a second argument. We probably shouldn't have added it in the first place :)

Comment on attachment 503620 [details] [diff] [review] Part 17 - Create mozI[Place|Visit]Info objects and use them v1.0 JSAPI usage looks good.

Comment on attachment 503620 [details] [diff] [review] Part 17 - Create mozI[Place|Visit]Info objects and use them v1.0 I think you already have tests for this part, just have to split them out to parts? We need some comprehensive test for each property. >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >@@ -348,8 +350,26 @@ public: > NS_PRECONDITION(NS_IsMainThread(), > "This should be called on the main thread"); > >- // TODO build a mozIPlaceInfo object for the visit. >- (void)mCallback->OnComplete(mResult, nsnull); >+ nsCOMPtr<nsIURI> referrer; IIRC, in the same file we use referrer for a placeInfo, I'd suggest to go for referrerURI here to avoid confusion. >+ // We do not notify about the frecency of the place. >+ nsCOMPtr<PlaceInfo> place = >+ new PlaceInfo(mPlace.placeId, mPlace.guid, uri.forget(), mPlace.title, >+ -1, visits); re-evaluating all of this code, I noticed we keep using COMPtr for stuff not being a real com interface, we should either define a interface or use refptr, please check the history.cpp code as a whole, eventually make into a new part. >diff --git a/toolkit/components/places/src/PlaceInfo.cpp b/toolkit/components/places/src/PlaceInfo.cpp >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. nit: "the Moz..." >+#include "PlaceInfo.h" >+#include "VisitInfo.h" >+#include "nsIURI.h" >+#include "nsContentUtils.h" >+ >+PlaceInfo::PlaceInfo(PRInt64 aId, >+ const nsCString& aGUID, >+ already_AddRefed<nsIURI> aURI, >+ const nsString& aTitle, >+ PRInt64 aFrecency, >+ const VisitsArray& aVisits) >+: mId(aId) >+, mGUID(aGUID) >+, mURI(aURI) >+, mTitle(aTitle) >+, mFrecency(aFrecency) >+, mVisits(aVisits) >+{ what about adding a precondition on mURI? >+NS_IMETHODIMP >+PlaceInfo::GetVisits(JSContext* aContext, >+ jsval* _visits) >+{ >+ // TODO when we use this in situations that have more than one visit here, we >+ // will likely want to make this cache the value. next part or followup? (file it and annotate bug # if followup please) >diff --git a/toolkit/components/places/src/PlaceInfo.h b/toolkit/components/places/src/PlaceInfo.h >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. nit: "the Moz..." >+#include "mozIAsyncHistory.h" >+#include "nsString.h" >+#include "nsAutoPtr.h" >+#include "nsCOMArray.h" COMArray.h? >diff --git a/toolkit/components/places/src/VisitInfo.cpp b/toolkit/components/places/src/VisitInfo.cpp >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. nit: "the Moz..." >diff --git a/toolkit/components/places/src/VisitInfo.h b/toolkit/components/places/src/VisitInfo.h >+ * The Initial Developer of the Original Code is >+ * Mozilla Foundation. nit: "the Moz..."

(In reply to comment #116) > I think you already have tests for this part, just have to split them out to > parts? > We need some comprehensive test for each property. Yup, it's in the wip which is about to become part 20. > IIRC, in the same file we use referrer for a placeInfo, I'd suggest to go for > referrerURI here to avoid confusion. We actually use referrerSpec, aReferrer, or mReferrer, but I can change it :) > next part or followup? (file it and annotate bug # if followup please) I don't think it makes sense until we write a querying API that also gives these back. I can file the follow-up though I guess.

(In reply to comment #116) > re-evaluating all of this code, I noticed we keep using COMPtr for stuff not > being a real com interface, we should either define a interface or use refptr, > please check the history.cpp code as a whole, eventually make into a new part. I introduced it here; everything else was fine. Fixed now!

Per review comments.

Per review comments.

Finally, the wip is cleaned up and ready for review. This should be everything Sync needs.

Comment on attachment 503991 [details] [diff] [review] Part 17 - Create mozI[Place|Visit]Info objects and use them v1.1 (checked in) https://hg.mozilla.org/projects/places/rev/63512a53e63f

Comment on attachment 503688 [details] [diff] [review] Part 18 - Update changes to the guid/title v1.0 https://hg.mozilla.org/projects/places/rev/5ca06e1e6eb1

Comment on attachment 503992 [details] [diff] [review] Part 19 - Get all the data that we need v1.1 (checked-in) https://hg.mozilla.org/projects/places/rev/5ca06e1e6eb1

Comment on attachment 503992 [details] [diff] [review] Part 19 - Get all the data that we need v1.1 (checked-in) Actually, this one is https://hg.mozilla.org/projects/places/rev/7408fd331dd0

Will starting working on the stuff outlined in comment 104 on Tuesday, but what's in the bug should be good enough for the Sync team.

Comment on attachment 503993 [details] [diff] [review] Part 20 - actual API implementation + tests v1.0 >+GetStringFromJSObject(JSContext* aCtx, >+ JSObject* aObject, >+ const char* aProperty, >+ nsDependentString& _string) >+{ >+ jsval val; >+ JSBool rc = JS_GetProperty(aCtx, aObject, aProperty, &val); >+ if (!rc || JSVAL_IS_VOID(val) || !JSVAL_IS_STRING(val)) { >+ _string.SetIsVoid(PR_TRUE); >+ return; >+ } >+ size_t length; >+ const jschar* chars = >+ JS_GetStringCharsZAndLength(aCtx, JSVAL_TO_STRING(val), &length); >+ if (!chars) { >+ _string.SetIsVoid(PR_TRUE); >+ return; >+ } >+ _string.Assign(static_cast<const PRUnichar*>(chars), length); So one hazard here is that a dependent string is being passed out that depends on the lifetime of a JSString* which is not passed out. This is a general hazard of conservative gc when pointing into the chars of a string (see bug 602274). Now, the string is rooted by its property on aObject which is rooted for the duration of the UpdatePlaces. However, if this property were to be deleted (say, from a getter called for one of the JSAPI calls later on in UpdatePlaces), then the string could be gc'd out from under the dependent string. Maybe this isn't a concern since this is chrome code we control and surely noone would do such a thing... But for piece of mind and to be bulletproof, I would do something to either root that string or (if this code isn't hot) copy the chars. To do the former, since this is called in a loop, you could keep an AutoValueVector.

(In reply to comment #127) > So one hazard here is that a dependent string is being passed out that depends > on the lifetime of a JSString* which is not passed out. This is a general > hazard of conservative gc when pointing into the chars of a string (see bug > 602274). Now, the string is rooted by its property on aObject which is rooted > for the duration of the UpdatePlaces. However, if this property were to be > deleted (say, from a getter called for one of the JSAPI calls later on in > UpdatePlaces), then the string could be gc'd out from under the dependent > string. Maybe this isn't a concern since this is chrome code we control and > surely noone would do such a thing... But for piece of mind and to be > bulletproof, I would do something to either root that string or (if this code > isn't hot) copy the chars. To do the former, since this is called in a loop, > you could keep an AutoValueVector. I'll just use an nsString instead, which does the copy I believe. We currently assign this dependent string to an nsString already, so the copy happens, just not soon enough to be bulletproof.

Comment on attachment 503993 [details] [diff] [review] Part 20 - actual API implementation + tests v1.0 Thanks for this patch, Shawn! I've started using this, but I noticed that the callback passed into updatePlaces is called for each visit that's added. This isn't really not what we want. It makes the API very difficult to use because you need to manually keep track of how many places and how many visits you add before you know that all your data has been applied. I realise that the implementation right now is according to the spec, so this is very much an oversight on my part in the spec. I don't remember specifying this when we wrote the API, in fact I remember conversations with sdwilsh where we explicitly discussed just one invocation of the callback. So, hrm, this is definitely my bad for not double checking the spec. The question now is: can we change it? We really only ever want to be called back once per updatePlaces() invocation. P.S.: The spec is a bit ambiguous here: * @param [optional] aCallback * Callback to be notified for each visit addition, title change, or * guid change. If more than one operation is done for a given place, * only one callback will be given (i.e. title change and add visit). The first and second sentence seem to contract each other here.

(In reply to comment #129) > The first and second sentence seem to contract each other here. s/contract/contradict/ :)

Having temporarily worked around the multiple callback problem, I have noticed that changes (to, say, the title) made through updatePlaces() don't trigger nsINavHistoryObserver::onTitleChanged(). Doesn't look like this would be intended behaviour.

(In reply to comment #129) > Thanks for this patch, Shawn! I've started using this, but I noticed that the > callback passed into updatePlaces is called for each visit that's added. This > isn't really not what we want. It makes the API very difficult to use because > you need to manually keep track of how many places and how many visits you add > before you know that all your data has been applied. > > I realise that the implementation right now is according to the spec, so this > is very much an oversight on my part in the spec. I don't remember specifying > this when we wrote the API, in fact I remember conversations with sdwilsh where > we explicitly discussed just one invocation of the callback. So, hrm, this is > definitely my bad for not double checking the spec. The question now is: can we > change it? We really only ever want to be called back once per updatePlaces() > invocation. I don't think this will be too terrible to change. I am going to need to figure out a way to cache the visits object now though... > The first and second sentence seem to contract each other here. I'll clean this up too. (In reply to comment #131) > Having temporarily worked around the multiple callback problem, I have noticed > that changes (to, say, the title) made through updatePlaces() don't trigger > nsINavHistoryObserver::onTitleChanged(). Doesn't look like this would be > intended behaviour. This is an oversight, although I haven't done any work to make sure these things work, that was coming up (comment 104). Getting onTitleChanged right means more disk I/O though, sadly. We have some wins, and I do my best to minimize disk I/O in the normal page navigation case.

backed out part 19 to investigate a make check permaorange: http://hg.mozilla.org/projects/places/rev/fe5e4cdebb66 http://hg.mozilla.org/projects/places/rev/f9974b90d869 backed out part 18 for the same reason, and ended up being culprit: http://hg.mozilla.org/projects/places/rev/9cbbbeec5c7b http://hg.mozilla.org/projects/places/rev/7385eecc39f7

failure was: http://tinderbox.mozilla.org/showlog.cgi?log=Places/1295376668.1295378629.25199.gz TEST-INFO | (../../../../../../toolkit/components/places/tests/cpp/test_IHistory.cpp) | Running test_visituri_inserts. TEST-INFO | (../../../../../../toolkit/components/places/tests/cpp/test_IHistory.cpp) | Running test_visituri_updates. command timed out: 300 seconds without output, killing pid 15820 process killed by signal 9 program finished with exit code -1 elapsedTime=392.193718 TinderboxPrint: check<br/>5186/0 buildbot.slave.commands.TimeoutError: command timed out: 300 seconds without output, killing pid 15820

This fixes the lack of a title changed notification, and includes a test.

Comment on attachment 504882 [details] [diff] [review] Part 21 - track and notify about title changes v1.0 except that I forgot a case...

This now makes sure we don't notify if no title is provided (I don't think Sync would hit this case, but nevertheless, I didn't want to spuriously notify).

(In reply to comment #132) > I don't think this will be too terrible to change. I am going to need to > figure out a way to cache the visits object now though... So I've been thinking about this. We don't actually need the VisitInfo objects in the callback. So if this turns out to be too much of a hassle, let's just scrap the second argument to the callback altogether and leave it for a follow-up which may or may not get implemented at all.

(In reply to comment #137) > Created attachment 504894 [details] [diff] [review] > Part 21 - track and notify about title changes v1.1 Thank you, Shawn! I can confirm we now get notifications for onTitleChanged(). > This now makes sure we don't notify if no title is provided (I don't think Sync > would hit this case, but nevertheless, I didn't want to spuriously notify). Well, some Sync records can have title = null. You're going to ignore such titles, right?

After applying Part 21 I'm still hitting some issues running our unit tests. It seems that some visits are added twice. As part of our unit test suite we add a record with two visits. The history engine ends up making this call: asyncHistory.updatePlaces({ uri: Utils.makeURI("http://getfirefox.com/"), title: "Hol Dir Firefox!", visits: [{visitDate: 1295309002831000, transitionType: 1}, {visitDate: 1295405816456000, transitionType: 2}] }); We get called back twice (for now at least) with these VisitInfo objects: {"placeId":1, "guid":"ok8Hx3F0VFMu", "uri":"<nsURI http://getfirefox.com/>", "title":"Hol Dir Firefox!", "frecency":-1, "visits":[{"visitId":1, "visitDate":1295309002831000, "transitionType":1, "referrerURI":null, "sessionId":0}]} {"placeId":1, "guid":"ok8Hx3F0VFMu", "uri":"<nsURI http://getfirefox.com/>", "title":"Hol Dir Firefox!", "frecency":-1, "visits":[{"visitId":3, "visitDate":1295405816456000, "transitionType":2, "referrerURI":null, "sessionId":2}]} Notice how the visitId is 1 in the first callback and 3 in the second. The sessionId also jumps a number. This in itself isn't a problem if we didn't have three visits in the database: [{"date":1295405816456000,"type":2}, {"date":1295309002831000,"type":1}, {"date":1295309002831000,"type":1}] This is the result of this query: SELECT visit_type type, visit_date date FROM moz_historyvisits WHERE place_id = (SELECT id FROM moz_places WHERE url = "http://getfirefox.com/") ORDER BY date DESC LIMIT 10; Pretty sure that this is one visit too many ;)

(In reply to comment #139) > Well, some Sync records can have title = null. You're going to ignore such > titles, right? If we already have a title stored, I think the code will change it, but we do not have any tests for this, so I'm not too sure. (In reply to comment #140) > After applying Part 21 I'm still hitting some issues running our unit tests. It > seems that some visits are added twice. As part of our unit test suite we add a > record with two visits. The history engine ends up making this call: I'll make a reduced testcase tomorrow and investigate it.

(In reply to comment #141) > (In reply to comment #140) > > After applying Part 21 I'm still hitting some issues running our unit tests. It > > seems that some visits are added twice. As part of our unit test suite we add a > > record with two visits. The history engine ends up making this call: > > I'll make a reduced testcase tomorrow and investigate it. I just tried to do that and couldn't reproduce it. Then I took another look at our tests and realized that at the very top we add a visit to the page: historySvc.addPageWithDetails(Utils.makeURI("http://getfirefox.com/"), "Get Firefox!", 1295309002831000); And then we simulate a typical sync record update call as shown in comment 140. The expectation here is that this first visit doesn't get duplicated. I guess you're going by visitId and since we don't pass that in, you recognize the 1295309002831000 visit we pass into updatePlaces() as a separate one. This makes a certain amount of sense, although I think visits with the same timestamp and transitionType should be recognized as dupes. Does that make sense? If it's too much of a hassle, we can filter out the dupes on our end like we do now.

(In reply to comment #142) > Does that make sense? If it's too much of a hassle, we can filter out the dupes > on our end like we do now. I don't think we want to filter out duplicates because it's quite possible to add the same visit if you have two tabs open doing the same thing. If Sync cares about duplicates, it's going to have to filter.

Adds a test for the case mentioned in comment 139, and fixes the code so the test passes.

(In reply to comment #143) > I don't think we want to filter out duplicates because it's quite possible to > add the same visit if you have two tabs open doing the same thing. If Sync > cares about duplicates, it's going to have to filter. Fair enough.

Summarizing IRC discussion I had with sdwilsh just now: * updatePlaces() will call the callback once for each place that's passed in. * callers can listen to onEndUpdateBatch() to find out when the operation has ended.

Question: right now updatePlaces() requires a non-empty visits array. That seems rather inconvenient if you just want to update the title for the page. I'm not sure if this case can happen, I'm just wondering... I have also run into slightly more severe problem: updatePlaces({ uri: Utils.makeURI("javascript:''"), title: "javascript:''", visits: [{"visitDate":1295462007130000,"transitionType":4}] }) fails with this: ###!!! ASSERTION: Must pass a non-empty array!: 'aPlaces.Length() > 0', file /Users/philipp/dev/mozilla/places/toolkit/components/places/src/History.cpp, line 599 Since javascript: URLs are ignored by places you probably don't want to even get as far as InsertVisitedURIs::Start().

(In reply to comment #147) > Question: right now updatePlaces() requires a non-empty visits array. That > seems rather inconvenient if you just want to update the title for the page. > I'm not sure if this case can happen, I'm just wondering... In the future we can certainly support that, but it turns out to be a lot of work. I'd like to punt on that capability until post-Firefox 4 unless Sync absolutely needs it. (see comment 104) > I have also run into slightly more severe problem: Will fix this in a bit.

(In reply to comment #146) > * callers can listen to onEndUpdateBatch() to find out when the operation has > ended. Marco points out that this would be expensive for small batches. In Sync we won't need it so long as updatePlaces(places, cb) guarantees to call cb exactly N times where N == places.length.