606966 - Need an async history visit API exposed to JS

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Philipp von Weitershausen [:philikon]]

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

Comment 2

[Shawn Wilsher :sdwilsh]

(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?

Comment 3

[Philipp von Weitershausen [:philikon]]

(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.

Comment 4

[Shawn Wilsher :sdwilsh]

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.

Comment 5

[Shawn Wilsher :sdwilsh]

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?

Comment 7

[Marco Bonardo [:mak]]

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)

Comment 8

[Philipp von Weitershausen [:philikon]]

(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.

Comment 9

[Shawn Wilsher :sdwilsh]

(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?

Comment 10

[Marco Bonardo [:mak]]

(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..

Comment 11

[Shawn Wilsher :sdwilsh]

(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.

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Shawn Wilsher :sdwilsh]

(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.

Comment 14

[Philipp von Weitershausen [:philikon]]

(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.

Comment 15

[Shawn Wilsher :sdwilsh]

(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?

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to comment #15)
> So, what you really want is mak's addVisitsForPage then, yeah?

Aye. Sorry if comment 8 wasn't clear.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this needed to improve Sync performance in Fennec?

Comment 18

[Marco Bonardo [:mak]]

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.

Comment 19

[Marco Bonardo [:mak]]

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

Comment 20

[Mike Connor [:mconnor]]

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.)

Comment 21

[Mike Connor [:mconnor]]

The spec is now ready to start implementing.

Comment 22

[Doug Turner (:dougt)]

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?

Comment 23

[Mike Connor [:mconnor]]

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.

Comment 24

[Shawn Wilsher :sdwilsh]

(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?

Comment 25

[Doug Turner (:dougt)]

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.

Comment 26

[Shawn Wilsher :sdwilsh]

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

Comment 27

[Marco Bonardo [:mak]]

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.

Comment 28

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - public interfaces v1.0

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.

Comment 29

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - refactor tests

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.

Comment 30

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - refactor tests v1.1

Forgot to qrefresh...

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - stub out mozIAsyncHistory methods v1.0

Comment 32

[Shawn Wilsher :sdwilsh]

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.

Comment 33

[Shawn Wilsher :sdwilsh]

Attachment: Part 4 - refactor hidden flag logic v1.0

Comment 34

[Shawn Wilsher :sdwilsh]

Attachment: Part 5 - refactor VisitData v1.0

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.

Comment 35

[Shawn Wilsher :sdwilsh]

Attachment: Part 6 - move inserting/updating moz_place entries to History v1.0

Comment 36

[Shawn Wilsher :sdwilsh]

Attachment: Part 7 - pull FetchPageInfo onto History v1.0

More code sharing!

Comment 37

[Shawn Wilsher :sdwilsh]

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.

Comment 38

[Shawn Wilsher :sdwilsh]

Attachment: wip

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

Comment 39

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - public interfaces v1.1

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

Comment 40

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - stub out mozIAsyncHistory methods v1.1 (checked in)

Comment 41

[Shawn Wilsher :sdwilsh]

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.

Comment 42

[Shawn Wilsher :sdwilsh]

Attachment: wip

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.

Comment 43

[Shawn Wilsher :sdwilsh]

Attachment: Part 8 - add guid and referrerSpec to VisitData

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

Comment 44

[Shawn Wilsher :sdwilsh]

Attachment: wip

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.

Comment 45

[Shawn Wilsher :sdwilsh]

Attachment: Part 9 - Move referrer loading logic into a helper method

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

Comment 46

[Shawn Wilsher :sdwilsh]

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.

Comment 47

[Shawn Wilsher :sdwilsh]

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

Comment 48

[Shawn Wilsher :sdwilsh]

Attachment: Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs

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

Comment 49

[Dietrich Ayala (:dietrich)]

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

Comment 50

[Shawn Wilsher :sdwilsh]

(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.

Comment 51

[Shawn Wilsher :sdwilsh]

Attachment: Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs

Updated TODO comment to include the bug #

Comment 52

[Marco Bonardo [:mak]]

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 53

[Marco Bonardo [:mak]]

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.

Comment 54

[Shawn Wilsher :sdwilsh]

(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 55

[Marco Bonardo [:mak]]

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 56

[Marco Bonardo [:mak]]

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 57

[Marco Bonardo [:mak]]

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 58

[Marco Bonardo [:mak]]

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?

Comment 59

[Shawn Wilsher :sdwilsh]

(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).

Comment 60

[Shawn Wilsher :sdwilsh]

(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.

Comment 61

[Shawn Wilsher :sdwilsh]

(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.

Comment 62

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - refactor tests v1.2 (checked in)

Addresses review comments.

Comment 63

[Shawn Wilsher :sdwilsh]

Attachment: Part 4 - refactor hidden flag logic v1.1 (checked in)

Addresses review comments.

Comment 64

[Shawn Wilsher :sdwilsh]

Attachment: Part 5 - refactor VisitData v1.1 (checked in)

Addresses review comments.

Comment 65

[Shawn Wilsher :sdwilsh]

Attachment: Part 6 - move inserting/updating moz_place entries to History v1.1 (checked in)

Addresses review comments.

Comment 66

[Shawn Wilsher :sdwilsh]

Attachment: Part 7 - pull FetchPageInfo onto History v1.1 (checked in)

Addresses review comments.

Comment 67

[Shawn Wilsher :sdwilsh]

Attachment: Part 8 - add guid and referrerSpec to VisitData v1.1 (checked in)

Addresses review comments.

Comment 68

[Shawn Wilsher :sdwilsh]

Attachment: Part 9 - Move reffer loading logic into a helper method v1.1 (checked in)

Addresses review comments.

Comment 69

[Shawn Wilsher :sdwilsh]

Attachment: Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs v1.1 (checked in)

Addresses review comments.

Comment 70

[Shawn Wilsher :sdwilsh]

Comment on attachment 502850 [details] [diff] [review]
Part 2 - refactor tests v1.2 (checked in)

https://hg.mozilla.org/projects/places/rev/ae99d2b2efaa

Comment 71

[Shawn Wilsher :sdwilsh]

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 72

[Shawn Wilsher :sdwilsh]

Comment on attachment 502855 [details] [diff] [review]
Part 5 - refactor VisitData v1.1 (checked in)

https://hg.mozilla.org/projects/places/rev/705a34ab2091

Comment 73

[Shawn Wilsher :sdwilsh]

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 74

[Shawn Wilsher :sdwilsh]

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 75

[Shawn Wilsher :sdwilsh]

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 76

[Shawn Wilsher :sdwilsh]

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 77

[Shawn Wilsher :sdwilsh]

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

Comment 78

[Shawn Wilsher :sdwilsh]

Attachment: Part 11 - refactor embed visit adding (checked in)

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

Comment 79

[Shawn Wilsher :sdwilsh]

Attachment: Part 12 - Fix issues with multiple visits of the same URI (checked in)

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

Comment 80

[Shawn Wilsher :sdwilsh]

Attachment: wip

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 81

[Marco Bonardo [:mak]]

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?

Comment 82

[Shawn Wilsher :sdwilsh]

(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 83

[Shawn Wilsher :sdwilsh]

Comment on attachment 502968 [details] [diff] [review]
Part 11 - refactor embed visit adding (checked in)

https://hg.mozilla.org/projects/places/rev/b9369c68197b

Comment 84

[Shawn Wilsher :sdwilsh]

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

Comment 85

[Shawn Wilsher :sdwilsh]

Attachment: Part 13 - Set page title if we have it v1.0

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

Comment 86

[Shawn Wilsher :sdwilsh]

Attachment: Part 14 - Set guid when provided v1.0

Like part 13, but for the guid.

Comment 87

[Shawn Wilsher :sdwilsh]

Attachment: Part 15 - only get session id if needed v1.0 (checked in)

Comment 88

[Shawn Wilsher :sdwilsh]

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.

Comment 89

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - public interfaces v1.2

Fairly certain that these are stable at this point.

Comment 90

[Robert Strong (they/them - no direct email)]

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 91

[Marco Bonardo [:mak]]

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 92

[Marco Bonardo [:mak]]

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

clearing requests per IRC discussion

Comment 93

[Shawn Wilsher :sdwilsh]

Attachment: Part 13 - Set page title if we have it v2.0 (checked in)

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

Comment 94

[Shawn Wilsher :sdwilsh]

Attachment: Part 14 - Set guid when provided v2.0 (checked in)

Comment 95

[Shawn Wilsher :sdwilsh]

(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).

Comment 96

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - public interfaces v1.3 (checked in)

Addresses review and sr comments.

Comment 97

[Shawn Wilsher :sdwilsh]

Comment on attachment 503593 [details] [diff] [review]
Part 1 - public interfaces v1.3 (checked in)

https://hg.mozilla.org/projects/places/rev/44797377722c

Comment 98

[Shawn Wilsher :sdwilsh]

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

Comment 99

[Shawn Wilsher :sdwilsh]

Attachment: Part 16 - Handle a mozIVisitInfoCallback v1.0

Comment 100

[Shawn Wilsher :sdwilsh]

Attachment: Part 17 - Create mozI[Place|Visit]Info objects and use them v1.0

Comment 101

[Shawn Wilsher :sdwilsh]

Attachment: Part 18 - Update changes to the guid/title v1.0

Comment 102

[Shawn Wilsher :sdwilsh]

Attachment: Part 19 - Get all the data that we need v1.0

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 103

[Shawn Wilsher :sdwilsh]

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.

Comment 104

[Shawn Wilsher :sdwilsh]

Attachment: wip

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 105

[Shawn Wilsher :sdwilsh]

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 106

[Shawn Wilsher :sdwilsh]

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 107

[Shawn Wilsher :sdwilsh]

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 108

[Marco Bonardo [:mak]]

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

Comment 109

[Shawn Wilsher :sdwilsh]

(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.

Comment 110

[Shawn Wilsher :sdwilsh]

(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.

Comment 111

[Shawn Wilsher :sdwilsh]

Attachment: Part 16 - Handle a mozIVisitInfoCallback v1.1 (checked in)

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 112

[Shawn Wilsher :sdwilsh]

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 113

[Marco Bonardo [:mak]]

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

Comment 114

[Shawn Wilsher :sdwilsh]

(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 115

[Luke Wagner [:luke]]

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 116

[Marco Bonardo [:mak]]

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..."

Comment 117

[Shawn Wilsher :sdwilsh]

(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.

Comment 118

[Shawn Wilsher :sdwilsh]

(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!

Comment 119

[Shawn Wilsher :sdwilsh]

Attachment: Part 17 - Create mozI[Place|Visit]Info objects and use them v1.1 (checked in)

Per review comments.

Comment 120

[Shawn Wilsher :sdwilsh]

Attachment: Part 19 - Get all the data that we need v1.1 (checked-in)

Per review comments.

Comment 121

[Shawn Wilsher :sdwilsh]

Attachment: Part 20 - actual API implementation + tests v1.0

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

Comment 122

[Shawn Wilsher :sdwilsh]

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 123

[Shawn Wilsher :sdwilsh]

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 124

[Shawn Wilsher :sdwilsh]

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 125

[Shawn Wilsher :sdwilsh]

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

Comment 126

[Shawn Wilsher :sdwilsh]

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 127

[Luke Wagner [:luke]]

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.

Comment 128

[Shawn Wilsher :sdwilsh]

(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 129

[Philipp von Weitershausen [:philikon]]

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.

Comment 130

[Philipp von Weitershausen [:philikon]]

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

s/contract/contradict/ :)

Comment 131

[Philipp von Weitershausen [:philikon]]

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.

Comment 132

[Shawn Wilsher :sdwilsh]

(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.

Comment 133

[Marco Bonardo [:mak]]

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

Comment 134

[Marco Bonardo [:mak]]

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

Comment 135

[Shawn Wilsher :sdwilsh]

Attachment: Part 21 - track and notify about title changes v1.0

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

Comment 136

[Shawn Wilsher :sdwilsh]

Comment on attachment 504882 [details] [diff] [review]
Part 21 - track and notify about title changes v1.0

except that I forgot a case...

Comment 137

[Shawn Wilsher :sdwilsh]

Attachment: Part 21 - track and notify about title changes v1.1

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).

Comment 138

[Philipp von Weitershausen [:philikon]]

(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.

Comment 139

[Philipp von Weitershausen [:philikon]]

(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?

Comment 140

[Philipp von Weitershausen [:philikon]]

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 ;)

Comment 141

[Shawn Wilsher :sdwilsh]

(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.

Comment 142

[Philipp von Weitershausen [:philikon]]

(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.

Comment 143

[Shawn Wilsher :sdwilsh]

(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.

Comment 144

[Shawn Wilsher :sdwilsh]

Attachment: Part 21 - track and notify about title changes v1.2

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

Comment 145

[Philipp von Weitershausen [:philikon]]

(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.

Comment 146

[Philipp von Weitershausen [:philikon]]

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.

Comment 147

[Philipp von Weitershausen [:philikon]]

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().

Comment 148

[Shawn Wilsher :sdwilsh]

(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.

Comment 149

[Philipp von Weitershausen [:philikon]]

(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.

Comment 150

[Philipp von Weitershausen [:philikon]]

(In reply to comment #148)
> (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)

Ok, no problem.

> > I have also run into slightly more severe problem:
> 
> Will fix this in a bit.

Actually, I wasn't using v1.2 of part 21 yet. That seems to have improved the situation a bit. I now only get:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/philipp/dev/mozilla/places/toolkit/components/places/src/nsNavHistory.cpp, line 5079

Comment 151

[Shawn Wilsher :sdwilsh]

(In reply to comment #150)
> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file
> /Users/philipp/dev/mozilla/places/toolkit/components/places/src/nsNavHistory.cpp,
> line 5079
Eh, you are still calling SetPageTitle somewhere then!

Comment 152

[Marco Bonardo [:mak]]

Code part, tests following (there are a lot)

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

>+/**
>+ * Obtains an nsIURI from the "uri" property of a JSObject.
>+ *
>+ * @param aCtx
>+ *        The JSContext for aObject.
>+ * @param aObject
>+ *        The JSObject to get the URI from.
>+ * @param aProperty
>+ *        The name of the property to get the URI from.
>+ * @returns the URI if it exists.

nit: as usual it's the opposite, @return without the s :)

>+already_AddRefed<nsIURI>
>+GetURIFromJSObject(JSContext* aCtx,
>+                   JSObject* aObject,
>+                   const char* aProperty)
>+{
>+  jsval uriVal;
>+  JSBool rc = JS_GetProperty(aCtx, aObject, aProperty, &uriVal);
>+  NS_ENSURE_TRUE(rc, nsnull);
>+  if (!JSVAL_IS_PRIMITIVE(uriVal)) {
>+    nsCOMPtr<nsIXPConnectWrappedNative> wrappedObj;
>+    nsresult rv = nsContentUtils::XPConnect()->GetWrappedNativeOfJSObject(
>+      aCtx,
>+      JSVAL_TO_OBJECT(uriVal),
>+      getter_AddRefs(wrappedObj)
>+    );
>+    NS_ENSURE_SUCCESS(rv, nsnull);
>+    nsCOMPtr<nsIURI> uri = do_QueryWrappedNative(wrappedObj);

is this supposed to always success since the previous call succeeded, or should we NS_ENSURE_TRUE uri for added safety?
Also, looks like this is the only helper that directly returns its output, some return a nsresult, some void... I think it's worth to unify them to a common behavior.

>+void
>+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);

who is freeing this char buffer? Maybe you want your string to Adopt this buffer? I don't know the JS API but doesn't look like anything here is taking ownership of it

>+nsresult
>+GetJSObjectFromArray(JSContext* aCtx,
>+                     JSObject* aArray,
>+                     jsuint aIndex,
>+                     JSObject** _rooter)
>+{
>+  NS_PRECONDITION(JS_IsArrayObject(aCtx, aArray),
>+                  "Must provide an object that is an array!");

A precondition looks a weak check here for opt builds... We can receive any sort of js stuff in these public APIs

>@@ -1604,8 +1739,140 @@ History::UpdatePlaces(const jsval& aPlac

>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  JSObject* infos;
>+  if (JS_IsArrayObject(aCtx, JSVAL_TO_OBJECT(aPlaceInfos))) {
>+    infos = JSVAL_TO_OBJECT(aPlaceInfos);

ah, you do the check here, the precondition is ok then, maybe you could add a note to the javadoc that the caller is expected to check the passed in param is a object by itself.

>+  else {
>+    // Build a temporary array to store this one item so our code below can
>+    // just loop.

nit: s/our/the/

>+  jsuint infosLength;
>+  (void)JS_GetArrayLength(aCtx, infos, &infosLength);

you can probably avoid this call when you build the array by yourself for just 1 entry. Init to 1 and call this only if you get an array?


>+    // If we have no id or guid, we must have visits.
>+    if (!(placeId > 0 || isValidGUID)) {

I think this is the second time I see this check, maybe earlier you could pack up a hasId bool and use it in both places rather than bringing on isValidGUID

>+      // If the visit is an embed visit, we do not actually add it to the
>+      // database.
>+      if (transitionType == nsINavHistoryService::TRANSITION_EMBED) {
>+        StoreAndNotifyEmbedVisit(data, aCallback);
>+        visitData.RemoveElementAt(visitData.Length() - 1);
>+        continue;

Ah, I thought this was handled in InsertVisitedURIs, does it not handle this anymore?

Comment 153

[Shawn Wilsher :sdwilsh]

Attachment: Part 22 - Dispatch errors for URIs that fail canAddURI v1.0 (checked in)

We need to let consumers know that we failed to add a visit for something that canAddURI would return false for.  I also updated test_isvisited.js since it was slightly out of date.

Comment 154

[Marco Bonardo [:mak]]

Comment on attachment 503993 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.0

>diff --git a/toolkit/components/places/tests/head_common.js b/toolkit/components/places/tests/head_common.js

>+function do_check_guid_for_uri(aURI,
>+                               aGUID)
> {
>   let stmt = DBConn().createStatement(
>     "SELECT guid "
>@@ -619,6 +622,10 @@ function do_check_guid_for_uri(aURI)
>   stmt.params.url = aURI.spec;
>   do_check_true(stmt.executeStep());
>   do_check_valid_places_guid(stmt.row.guid);
>+  if (aGUID) {
>+    do_check_valid_places_guid(aGUID);
>+    do_check_eq(stmt.row.guid, aGUID);
>+  }

hm, these checks associate the failure to head_common.js or whatever head is involved...
I guess if we should make this just do_log_info the checks results and return true/false so that the test itself is blamed, or walk up the stack
btw, this was already bogus, so you can ignore me

>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 VisitInfo(aTransitionType,
>+                   aVisitTime)
>+{
>+  this.transitionType =
>+    aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
>+  this.visitDate = aVisitTime || Date.now() * 1000;
>+}

for the love of randomoranges, please do a global let gNow = Date.now() * 1000 and gNow++ here

>+function do_check_title_for_uri(aURI,
>+                                aTitle)
>+{
>+  let stmt = DBConn().createStatement(
>+    "SELECT title " +
>+    "FROM moz_places " +
>+    "WHERE url = :url "
>+  );
>+  stmt.params.url = aURI.spec;
>+  do_check_true(stmt.executeStep());
>+  do_check_eq(stmt.row.title, aTitle);
>+  stmt.finalize();
>+}

what's exactly the point of this when we have PlacesUtils.history.getPageTitle(uri); ?

>+function test_invalid_uri_throws()
>+{
>+  let place = {
>+    // XXX do we want to allow strings?  Would not be hard to support...

XXXs get lost easily, if you think it's worth it file a follow-up, I think it's better if we get proper URIs, there is no perf gain too since we have to make it later.

>+function test_no_places_throws()
>+{
>+  // First test passing in nothing.
>+  try {
>+    gHistory.updatePlaces();
>+    do_throw("Should have thrown!");
>+  }
>+  catch (e) {
>+    do_check_eq(e.result, Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS);
>+  }
>+
>+  // Then test passing in null.

what about undefined, a empty object, a empty array, could be worth adding more common errors

>+function test_invalid_id_throws()
>+{
>+  // First check invalid id "0".

>+  // Now check negative id.

Looks like missing "positive id that is not in the database."
  
>+function test_invalid_guid_throws()

missing "valid guid that is not in the database"

also, what happens if I pass a uri a place id and a guid but they are not referred to the same entry?
like I have placeid1-uri1 and placeid1-uri2 and I pass in placeid2-uri1... who wins, who is updated? (same with guid, and a mixture of the three)
I'm not sure the interface is cristal clear about this, I could write a broken code that passes in values not referring to the same entry, and I don't know what I'm updating.


>+function test_add_visit()
>+{
>+  const VISIT_TIME = Date.now() * 1000;

I thought if you don't pass in a visit time to VisitInfo now is used already...


>+    // mozIVisitInfo::date

>+    // mozIVisitInfo::transitionType
>
>+    // mozIVisitInfo::sessionId

>+    // mozIPlaceInfo::title

these could be made helpers in head_common.js, I'd find them useful


>+function test_referrer_saved()
>+{

I don't recall if there is a test checking what happens by passing in a bad valie for referrer
  
>+    let stmt = DBConn().createStatement(
>+      "SELECT COUNT(1) AS count " +
>+      "FROM moz_historyvisits " +
>+      "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " +
>+      "AND from_visit = ( " +
>+        "SELECT id " +
>+        "FROM moz_historyvisits " +
>+        "WHERE place_id = (SELECT id FROM moz_places WHERE url = :referrer) " +
>+      ") "
>+    );

doesn't matter in this case, but from_visit and referrer are not exactly the same thing.
especially in the case of redirects, all visits have the same referrer that is the first page, but from_visit is a chain, like A -> B -> C, from_visit is A for B and B for C, but referrer for B and C is A.
just to be clear :)

>+function test_sessionId_saved()

test for a broken sessionId? (like a string)

>+    let stmt = DBConn().createStatement(
>+      "SELECT COUNT(1) AS count " +
>+      "FROM moz_historyvisits " +
>+      "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " +
>+      "AND session = :session_id "
>+    );

ditto for a head_common util

r=me BUT the above doubts regardin mixed "identificators" needs to be addressed and clarified at the API level.

Comment 155

[Philipp von Weitershausen [:philikon]]

(In reply to comment #151)
> (In reply to comment #150)
> > WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file
> > /Users/philipp/dev/mozilla/places/toolkit/components/places/src/nsNavHistory.cpp,
> > line 5079
> Eh, you are still calling SetPageTitle somewhere then!

Not on Gecko 2.0 we're not! Seriously weird. Ah well, now we don't even bother calling updatePlaces() anymore in this place because canAddURI() returns false as it should. So ignore me :)

Comment 156

[Shawn Wilsher :sdwilsh]

(In reply to comment #154)
> hm, these checks associate the failure to head_common.js or whatever head is
> involved...
> I guess if we should make this just do_log_info the checks results and return
> true/false so that the test itself is blamed, or walk up the stack
> btw, this was already bogus, so you can ignore me
Naw, this is easy to fix by just passing in Components.stack.caller into the do_check_* methods.

> for the love of randomoranges, please do a global let gNow = Date.now() * 1000
> and gNow++ here
None of our tests actually depend on the visit date though (or if they do, I actually define one).

> what's exactly the point of this when we have
> PlacesUtils.history.getPageTitle(uri); ?
future proofing, really.  I'd really like us to get rid of that method since it is synchronous.

> Looks like missing "positive id that is not in the database."
It could be in the database; we don't know at call time.  We just make sure it's valid in the "could be possible" case.

> missing "valid guid that is not in the database"
At which point we'd try to insert it, so that's OK.

> also, what happens if I pass a uri a place id and a guid but they are not
> referred to the same entry?
You should get a constraint error, but we have no test for this (yet!)

> like I have placeid1-uri1 and placeid1-uri2 and I pass in placeid2-uri1... who
> wins, who is updated? (same with guid, and a mixture of the three)
> I'm not sure the interface is cristal clear about this, I could write a broken
> code that passes in values not referring to the same entry, and I don't know
> what I'm updating.
Will update and add more tests.

> I thought if you don't pass in a visit time to VisitInfo now is used already...
I wanted these to all to have the the same visit time here.

> these could be made helpers in head_common.js, I'd find them useful
I didn't do that because this is currently the only test file that uses them.  If we find a need for them in the future elsewhere, I think we should do that then.  OK?

> I don't recall if there is a test checking what happens by passing in a bad
> valie for referrer
We don't.  I'll need to add one.

> doesn't matter in this case, but from_visit and referrer are not exactly the
> same thing.
> especially in the case of redirects, all visits have the same referrer that is
> the first page, but from_visit is a chain, like A -> B -> C, from_visit is A
> for B and B for C, but referrer for B and C is A.
> just to be clear :)
Oy, we need tests for this too...
And better tests in the case where we drop the referrer because it isn't recent enough.

> test for a broken sessionId? (like a string)
Will make.

> ditto for a head_common util
Again, this was the only place we seem to care about it, so I wasn't going to bother (that, and we are going to get rid of it maybe in firefox 5?)

Comment 157

[Shawn Wilsher :sdwilsh]

To be clear, I now have six tests in my todo list to write based on that last comment.

Comment 158

[Marco Bonardo [:mak]]

(In reply to comment #156)
> > Looks like missing "positive id that is not in the database."
> It could be in the database; we don't know at call time.  We just make sure
> it's valid in the "could be possible" case.

My point is that if a user passes in a invalid (valid but not in the db) place id we should NOT do anything with it. I think we could want a test that checks we DO NOT write this value neither to a new row nor updating a old row.

> > missing "valid guid that is not in the database"
> At which point we'd try to insert it, so that's OK.

So if I got this correctly, we insert a new entry with this guid. Or, if there is already another entry for the same uri but with a different guid, we update that entry's guid?

> I wanted these to all to have the the same visit time here.

uri and visit time are nice primary keys to identify a visit without the need of a fake id, unfortunately we never forced visit time uniquity :(