bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Need an async history visit API exposed to JS

RESOLVED FIXED in mozilla2.0b11

Status

()

Toolkit
Places
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

(Blocks: 5 bugs, {dev-doc-complete})

Trunk
mozilla2.0b11
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+, fennec2.0b5+)

Details

(Whiteboard: [hardblocker], URL)

Attachments

(26 attachments, 31 obsolete attachments)

3.22 KB, patch
mak
: review+
Details | Diff | Splinter Review
4.21 KB, patch
Details | Diff | Splinter Review
4.06 KB, patch
Details | Diff | Splinter Review
4.91 KB, patch
Details | Diff | Splinter Review
8.54 KB, patch
Details | Diff | Splinter Review
8.35 KB, patch
Details | Diff | Splinter Review
3.18 KB, patch
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
8.08 KB, patch
Details | Diff | Splinter Review
2.10 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.55 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.35 KB, patch
mak
: review+
Details | Diff | Splinter Review
2.01 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.10 KB, patch
mak
: review+
Details | Diff | Splinter Review
6.09 KB, patch
Details | Diff | Splinter Review
7.63 KB, patch
mak
: review+
Details | Diff | Splinter Review
8.19 KB, patch
Details | Diff | Splinter Review
15.18 KB, patch
Details | Diff | Splinter Review
2.83 KB, patch
Details | Diff | Splinter Review
9.75 KB, patch
mak
: review+
Details | Diff | Splinter Review
3.57 KB, patch
Details | Diff | Splinter Review
32.98 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.64 KB, patch
rstrong
: superreview+
Details | Diff | Splinter Review
8.20 KB, patch
mak
: review+
Details | Diff | Splinter Review
19.58 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.98 KB, patch
mak
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 2

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
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?

Updated

8 years ago
Duplicate of this bug: 606350

Updated

8 years ago
Blocks: 606353
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.
(Assignee)

Comment 9

8 years ago
(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..
(Assignee)

Comment 11

8 years ago
(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.
(Assignee)

Comment 13

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
(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.)
(Assignee)

Updated

8 years ago
blocking2.0: --- → betaN+
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.
(Assignee)

Comment 24

8 years ago
(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.
(Assignee)

Comment 26

8 years ago
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.
Keywords: dev-doc-needed
(Assignee)

Updated

8 years ago
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
(Assignee)

Comment 28

8 years ago
Created attachment 498752 [details] [diff] [review]
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.
(Assignee)

Comment 29

8 years ago
Created attachment 498867 [details] [diff] [review]
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.
Attachment #498867 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 30

8 years ago
Created attachment 498874 [details] [diff] [review]
Part 2 - refactor tests v1.1

Forgot to qrefresh...
Attachment #498867 - Attachment is obsolete: true
Attachment #498874 - Flags: review?(mak77)
Attachment #498867 - Flags: review?(mak77)
(Assignee)

Comment 31

8 years ago
Created attachment 498895 [details] [diff] [review]
Part 3 - stub out mozIAsyncHistory methods v1.0
Attachment #498895 - Flags: review?(mak77)
(Assignee)

Comment 32

8 years ago
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.
(Assignee)

Comment 33

8 years ago
Created attachment 499011 [details] [diff] [review]
Part 4 - refactor hidden flag logic v1.0
Attachment #499011 - Flags: review?(mak77)
(Assignee)

Comment 34

8 years ago
Created attachment 499025 [details] [diff] [review]
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.
Attachment #499025 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Attachment #499011 - Attachment description: Part 4 - refactor hidden flag logic → Part 4 - refactor hidden flag logic v1.0
(Assignee)

Updated

8 years ago
Attachment #498895 - Attachment description: Part 3 - stub out mozIAsyncHistory methods → Part 3 - stub out mozIAsyncHistory methods v1.0
(Assignee)

Comment 35

8 years ago
Created attachment 499052 [details] [diff] [review]
Part 6 - move inserting/updating moz_place entries to History v1.0
Attachment #499052 - Flags: review?(mak77)
(Assignee)

Comment 36

8 years ago
Created attachment 499066 [details] [diff] [review]
Part 7 - pull FetchPageInfo onto History v1.0

More code sharing!
Attachment #499066 - Flags: review?(mak77)
(Assignee)

Comment 37

8 years ago
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.
Attachment #498895 - Flags: review?(mak77)
(Assignee)

Comment 38

8 years ago
Created attachment 499875 [details] [diff] [review]
wip

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

Comment 39

8 years ago
Created attachment 499876 [details] [diff] [review]
Part 1 - public interfaces v1.1

I don't expect these to change anymore, so I'm going to request r and sr now.
Attachment #498752 - Attachment is obsolete: true
Attachment #499876 - Flags: superreview?
Attachment #499876 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Attachment #499876 - Flags: superreview? → superreview?(robert.bugzilla)
(Assignee)

Comment 40

8 years ago
Created attachment 499878 [details] [diff] [review]
Part 3 - stub out mozIAsyncHistory methods v1.1 (checked in)
Attachment #498895 - Attachment is obsolete: true
Attachment #499878 - Flags: review?(mak77)
(Assignee)

Comment 41

8 years ago
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.
Attachment #499876 - Flags: superreview?(robert.bugzilla)
Attachment #499876 - Flags: review?(mak77)
(Assignee)

Comment 42

8 years ago
Created attachment 500092 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Attachment #499875 - Attachment is obsolete: true
(Assignee)

Comment 43

8 years ago
Created attachment 500227 [details] [diff] [review]
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.
Attachment #500227 - Flags: review?(mak77)
(Assignee)

Comment 44

8 years ago
Created attachment 500231 [details] [diff] [review]
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.
Attachment #500092 - Attachment is obsolete: true
Whiteboard: [has patch][needs review mak]

Updated

8 years ago
tracking-fennec: --- → 2.0b4+
(Assignee)

Comment 45

8 years ago
Created attachment 501786 [details] [diff] [review]
Part 9 - Move referrer loading logic into a helper method

Simple refactoring to make my next step a little more clear.
Attachment #501786 - Flags: review?(dietrich)
(Assignee)

Comment 46

8 years ago
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.
(Assignee)

Comment 47

8 years ago
Upon further thought, the API is likely fine as-is regardless of the possible Sync changes.
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review mak] → [has patch][needs review mak][needs review dietrich]
(Assignee)

Updated

8 years ago
Attachment #499052 - Flags: review?(mak77) → review?(dietrich)
(Assignee)

Updated

8 years ago
Attachment #499066 - Flags: review?(mak77) → review?(dietrich)
(Assignee)

Comment 48

8 years ago
Created attachment 501845 [details] [diff] [review]
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.
Attachment #501845 - Flags: review?(mak77)
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
Attachment #499052 - Flags: review?(dietrich) → review+
Attachment #499066 - Flags: review?(dietrich) → review+
Attachment #501786 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][needs review mak][needs review dietrich] → [has patch][needs review mak]
(Assignee)

Updated

8 years ago
Attachment #499025 - Flags: review?(mak77) → review?(dietrich)
(Assignee)

Updated

8 years ago
Attachment #501786 - Attachment description: Part 9 - Move reffer loading logic into a helper method → Part 9 - Move referrer loading logic into a helper method
(Assignee)

Updated

8 years ago
Attachment #500227 - Flags: review?(mak77) → review?(dietrich)
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review mak] → [has patch][needs review mak][needs review dietrich]
(Assignee)

Comment 50

8 years ago
(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.
Blocks: 623969
(Assignee)

Comment 51

8 years ago
Created attachment 502044 [details] [diff] [review]
Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs

Updated TODO comment to include the bug #
Attachment #501845 - Attachment is obsolete: true
Attachment #502044 - Flags: review?(mak77)
Attachment #501845 - Flags: review?(mak77)
Attachment #499025 - Flags: review?(dietrich) → review+
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).
Attachment #498874 - Flags: review?(mak77) → review+
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.
Attachment #499878 - Flags: review?(mak77) → review+
(Assignee)

Comment 54

8 years ago
(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.
Attachment #499011 - Flags: review?(mak77) → review+
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

Updated

8 years ago
Attachment #500227 - Flags: review?(dietrich) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review mak][needs review dietrich] → [has patch][needs review 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?
Attachment #502044 - Flags: review?(mak77) → review+
Whiteboard: [has patch][needs review mak] → [has patch][needs review mak][hard blocker]
(Assignee)

Comment 59

8 years ago
(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).
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review mak][hard blocker] → [hard blocker]
(Assignee)

Comment 60

8 years ago
(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.
(Assignee)

Comment 61

8 years ago
(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.
(Assignee)

Comment 62

8 years ago
Created attachment 502850 [details] [diff] [review]
Part 2 - refactor tests v1.2 (checked in)

Addresses review comments.
Attachment #498874 - Attachment is obsolete: true
(Assignee)

Comment 63

8 years ago
Created attachment 502854 [details] [diff] [review]
Part 4 - refactor hidden flag logic v1.1 (checked in)

Addresses review comments.
Attachment #499011 - Attachment is obsolete: true
(Assignee)

Comment 64

8 years ago
Created attachment 502855 [details] [diff] [review]
Part 5 - refactor VisitData v1.1 (checked in)

Addresses review comments.
Attachment #499025 - Attachment is obsolete: true
(Assignee)

Comment 65

8 years ago
Created attachment 502858 [details] [diff] [review]
Part 6 - move inserting/updating moz_place entries to History v1.1 (checked in)

Addresses review comments.
Attachment #499052 - Attachment is obsolete: true
(Assignee)

Comment 66

8 years ago
Created attachment 502861 [details] [diff] [review]
Part 7 - pull FetchPageInfo onto History v1.1 (checked in)

Addresses review comments.
Attachment #499066 - Attachment is obsolete: true
(Assignee)

Comment 67

8 years ago
Created attachment 502863 [details] [diff] [review]
Part 8  - add guid and referrerSpec to VisitData v1.1 (checked in)

Addresses review comments.
Attachment #500227 - Attachment is obsolete: true
(Assignee)

Comment 68

8 years ago
Created attachment 502867 [details] [diff] [review]
Part 9 - Move reffer loading logic into a helper method v1.1 (checked in)

Addresses review comments.
Attachment #501786 - Attachment is obsolete: true
(Assignee)

Comment 69

8 years ago
Created attachment 502868 [details] [diff] [review]
Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs v1.1 (checked in)

Addresses review comments.
Attachment #502044 - Attachment is obsolete: true
(Assignee)

Comment 70

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

https://hg.mozilla.org/projects/places/rev/ae99d2b2efaa
Attachment #502850 - Attachment description: Part 2 - refactor tests v1.2 → Part 2 - refactor tests v1.2 (checked in)
(Assignee)

Comment 71

8 years ago
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
Attachment #502854 - Attachment description: Part 4 - refactor hidden flag logic v1.1 → Part 4 - refactor hidden flag logic v1.1 (checked in)
(Assignee)

Comment 72

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

https://hg.mozilla.org/projects/places/rev/705a34ab2091
Attachment #502855 - Attachment description: Part 5 - refactor VisitData v1.1 → Part 5 - refactor VisitData v1.1 (checked in)
(Assignee)

Comment 73

8 years ago
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
Attachment #502858 - Attachment description: Part 6 - move inserting/updating moz_place entries to History v1.1 → Part 6 - move inserting/updating moz_place entries to History v1.1 (checked in)
(Assignee)

Comment 74

8 years ago
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
Attachment #502861 - Attachment description: Part 7 - pull FetchPageInfo onto History v1.1 → Part 7 - pull FetchPageInfo onto History v1.1 (checked in)
(Assignee)

Comment 75

8 years ago
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
Attachment #502863 - Attachment description: Part 8 - add guid and referrerSpec to VisitData v1.1 → Part 8 - add guid and referrerSpec to VisitData v1.1 (checked in)
(Assignee)

Comment 76

8 years ago
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
Attachment #502867 - Attachment description: Part 9 - Move reffer loading logic into a helper method v1.1 → Part 9 - Move reffer loading logic into a helper method v1.1 (checked in)
(Assignee)

Comment 77

8 years ago
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
Attachment #502868 - Attachment description: Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs v1.1 → Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs v1.1 (checked in)
(Assignee)

Comment 78

8 years ago
Created attachment 502968 [details] [diff] [review]
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.
Attachment #502968 - Flags: review?(mak77)
(Assignee)

Comment 79

8 years ago
Created attachment 503004 [details] [diff] [review]
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 :)
Attachment #503004 - Flags: review?(mak77)
(Assignee)

Comment 80

8 years ago
Created attachment 503005 [details] [diff] [review]
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.
Attachment #500231 - Attachment is obsolete: true
Whiteboard: [hard blocker] → [hardblocker]

Updated

8 years ago
Attachment #502968 - Flags: review?(mak77) → review+
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?
Attachment #503004 - Flags: review?(mak77) → review+

Updated

8 years ago
Whiteboard: [hardblocker] → [hardblocker][part 1-10 fixed-in-places]
(Assignee)

Comment 82

8 years ago
(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.
Whiteboard: [hardblocker][part 1-10 fixed-in-places] → [hardblocker][part 2,4-10 fixed-in-places]
(Assignee)

Comment 83

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

https://hg.mozilla.org/projects/places/rev/b9369c68197b
Attachment #502968 - Attachment description: Part 11 - refactor embed visit adding → Part 11 - refactor embed visit adding (checked in)
(Assignee)

Comment 84

8 years ago
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
Attachment #503004 - Attachment description: Part 12 - Fix issues with multiple visits of the same URI → Part 12 - Fix issues with multiple visits of the same URI (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 2,4-10 fixed-in-places] → [hardblocker][part 2,4-12 fixed-in-places]
(Assignee)

Comment 85

8 years ago
Created attachment 503309 [details] [diff] [review]
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.
Attachment #503309 - Flags: review?(mak77)
(Assignee)

Comment 86

8 years ago
Created attachment 503330 [details] [diff] [review]
Part 14 - Set guid when provided v1.0

Like part 13, but for the guid.
Attachment #503330 - Flags: review?(mak77)
(Assignee)

Comment 87

8 years ago
Created attachment 503357 [details] [diff] [review]
Part 15 - only get session id if needed v1.0 (checked in)
Attachment #503357 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Attachment #503357 - Attachment description: Part 15 - only get session id if needed → Part 15 - only get session id if needed v1.0
(Assignee)

Comment 88

8 years ago
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.
(Assignee)

Comment 89

8 years ago
Created attachment 503364 [details] [diff] [review]
Part 1 - public interfaces v1.2

Fairly certain that these are stable at this point.
Attachment #499876 - Attachment is obsolete: true
Attachment #503364 - Flags: superreview?
Attachment #503364 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Attachment #503364 - Flags: superreview? → superreview?(robert.bugzilla)
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);
>+
>+};
Attachment #503364 - Flags: superreview?(robert.bugzilla) → superreview+

Updated

8 years ago
Attachment #503357 - Flags: review?(mak77) → review+
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
Attachment #503364 - Flags: review?(mak77) → review+
Comment on attachment 503309 [details] [diff] [review]
Part 13 - Set page title if we have it v1.0

clearing requests per IRC discussion
Attachment #503309 - Flags: review?(mak77)

Updated

8 years ago
Attachment #503330 - Flags: review?(mak77)
(Assignee)

Comment 93

8 years ago
Created attachment 503571 [details] [diff] [review]
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.
Attachment #503309 - Attachment is obsolete: true
Attachment #503571 - Flags: review?(mak77)
(Assignee)

Comment 94

8 years ago
Created attachment 503572 [details] [diff] [review]
Part 14 - Set guid when provided v2.0 (checked in)
Attachment #503330 - Attachment is obsolete: true
Attachment #503572 - Flags: review?(mak77)
(Assignee)

Comment 95

8 years ago
(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).
(Assignee)

Comment 96

8 years ago
Created attachment 503593 [details] [diff] [review]
Part 1 - public interfaces v1.3 (checked in)

Addresses review and sr comments.
Attachment #503364 - Attachment is obsolete: true
(Assignee)

Comment 97

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

https://hg.mozilla.org/projects/places/rev/44797377722c
Attachment #503593 - Attachment description: Part 1 - public interfaces v1.3 → Part 1 - public interfaces v1.3 (checked in)
(Assignee)

Comment 98

8 years ago
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
Attachment #499878 - Attachment description: Part 3 - stub out mozIAsyncHistory methods v1.1 → Part 3 - stub out mozIAsyncHistory methods v1.1 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 2,4-12 fixed-in-places] → [hardblocker][part 1-12 fixed-in-places]
(Assignee)

Comment 99

8 years ago
Created attachment 503616 [details] [diff] [review]
Part 16 - Handle a mozIVisitInfoCallback v1.0
Attachment #503616 - Flags: review?(mak77)
(Assignee)

Comment 100

8 years ago
Created attachment 503620 [details] [diff] [review]
Part 17 - Create mozI[Place|Visit]Info objects and use them v1.0
Attachment #503620 - Flags: review?(mak77)
(Assignee)

Comment 101

8 years ago
Created attachment 503688 [details] [diff] [review]
Part 18 - Update changes to the guid/title v1.0
Attachment #503688 - Flags: review?(mak77)
(Assignee)

Comment 102

8 years ago
Created attachment 503699 [details] [diff] [review]
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).
Attachment #503699 - Flags: review?(mak77)
(Assignee)

Comment 103

8 years ago
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.
Attachment #503620 - Flags: review?(lw)

Updated

8 years ago
Attachment #503571 - Flags: review?(mak77) → review+
(Assignee)

Comment 104

8 years ago
Created attachment 503711 [details] [diff] [review]
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?

Updated

8 years ago
Attachment #503572 - Flags: review?(mak77) → review+
(Assignee)

Comment 105

8 years ago
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
Attachment #503571 - Attachment description: Part 13 - Set page title if we have it v2.0 → Part 13 - Set page title if we have it v2.0 (checked in)
(Assignee)

Comment 106

8 years ago
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
Attachment #503572 - Attachment description: Part 14 - Set guid when provided v2.0 → Part 14 - Set guid when provided v2.0 (checked in)
(Assignee)

Comment 107

8 years ago
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
Attachment #503357 - Attachment description: Part 15 - only get session id if needed v1.0 → Part 15 - only get session id if needed v1.0 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-12 fixed-in-places] → [hardblocker][part 1-15 fixed-in-places]
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
Attachment #503616 - Flags: review?(mak77) → review+
(Assignee)

Comment 109

8 years ago
(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.
(Assignee)

Comment 110

8 years ago
(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.
(Assignee)

Comment 111

8 years ago
Created attachment 503890 [details] [diff] [review]
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).
(Assignee)

Comment 112

8 years ago
Comment on attachment 503890 [details] [diff] [review]
Part 16 - Handle a mozIVisitInfoCallback v1.1 (checked in)

http://hg.mozilla.org/projects/places/rev/ed509b5771d3
Attachment #503890 - Attachment description: Part 16 - Handle a mozIVisitInfoCallback v1.1 → Part 16 - Handle a mozIVisitInfoCallback v1.1 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-15 fixed-in-places] → [hardblocker][part 1-16 fixed-in-places]

Updated

8 years ago
Blocks: 625828

Updated

8 years ago
Attachment #503688 - Flags: review?(mak77) → review+
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
Attachment #503699 - Flags: review?(mak77) → review+
(Assignee)

Comment 114

8 years ago
(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.
Attachment #503620 - Flags: review?(lw) → review+
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..."
Attachment #503620 - Flags: review?(mak77) → review+
(Assignee)

Comment 117

8 years ago
(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.
(Assignee)

Comment 118

8 years ago
(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!
(Assignee)

Updated

8 years ago
Blocks: 625913
(Assignee)

Updated

8 years ago
Attachment #503005 - Attachment is obsolete: true
(Assignee)

Comment 119

8 years ago
Created attachment 503991 [details] [diff] [review]
Part 17 - Create mozI[Place|Visit]Info objects and use them v1.1 (checked in)

Per review comments.
Attachment #503620 - Attachment is obsolete: true
(Assignee)

Comment 120

8 years ago
Created attachment 503992 [details] [diff] [review]
Part 19 - Get all the data that we need v1.1 (checked-in)

Per review comments.
Attachment #503699 - Attachment is obsolete: true
(Assignee)

Comment 121

8 years ago
Created attachment 503993 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.0

Finally, the wip is cleaned up and ready for review.  This should be everything Sync needs.
Attachment #503711 - Attachment is obsolete: true
Attachment #503993 - Flags: review?(mak77)
Attachment #503993 - Flags: review?(lw)
(Assignee)

Comment 122

8 years ago
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
Attachment #503991 - Attachment description: Part 17 - Create mozI[Place|Visit]Info objects and use them v1.1 → Part 17 - Create mozI[Place|Visit]Info objects and use them v1.1 (checked in)
(Assignee)

Comment 123

8 years ago
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
Attachment #503688 - Attachment description: Part 18 - Update changes to the guid/title v1.0 → Part 18 - Update changes to the guid/title v1.0 (checked in)
(Assignee)

Comment 124

8 years ago
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
Attachment #503992 - Attachment description: Part 19 - Get all the data that we need v1.1 → Part 19 - Get all the data that we need v1.1 (checked in)
(Assignee)

Comment 125

8 years ago
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
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-16 fixed-in-places] → [hardblocker][part 1-19 fixed-in-places]
(Assignee)

Comment 126

8 years ago
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.
(Assignee)

Comment 128

8 years ago
(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.
Attachment #503993 - Flags: feedback-
(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.
(Assignee)

Comment 132

8 years ago
(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
Whiteboard: [hardblocker][part 1-19 fixed-in-places] → [hardblocker][part 1-17 fixed-in-places]
(Assignee)

Updated

8 years ago
Blocks: 626836

Updated

8 years ago
Attachment #503688 - Attachment description: Part 18 - Update changes to the guid/title v1.0 (checked in) → Part 18 - Update changes to the guid/title v1.0

Updated

8 years ago
Attachment #503992 - Attachment description: Part 19 - Get all the data that we need v1.1 (checked in) → Part 19 - Get all the data that we need v1.1
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
(Assignee)

Comment 135

8 years ago
Created attachment 504882 [details] [diff] [review]
Part 21 - track and notify about title changes v1.0

This fixes the lack of a title changed notification, and includes a test.
Attachment #504882 - Flags: review?(mak77)
(Assignee)

Comment 136

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

except that I forgot a case...
Attachment #504882 - Attachment is obsolete: true
Attachment #504882 - Flags: review?(mak77)
(Assignee)

Comment 137

8 years ago
Created attachment 504894 [details] [diff] [review]
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).
Attachment #504894 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Attachment #504894 - Attachment is patch: true
Attachment #504894 - Attachment mime type: application/octet-stream → text/plain
(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 ;)
(Assignee)

Comment 141

8 years ago
(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.
(Assignee)

Comment 143

8 years ago
(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.
(Assignee)

Comment 144

8 years ago
Created attachment 505081 [details] [diff] [review]
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.
Attachment #504894 - Attachment is obsolete: true
Attachment #505081 - Flags: review?(mak77)
Attachment #504894 - Flags: review?(mak77)
(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().
(Assignee)

Comment 148

8 years ago
(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.
(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
(Assignee)

Comment 151

8 years ago
(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!
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?
(Assignee)

Comment 153

8 years ago
Created attachment 505173 [details] [diff] [review]
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.
Attachment #505173 - Flags: review?(mak77)
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.
Attachment #503993 - Flags: review?(mak77) → review+
(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 :)
(Assignee)

Comment 156

8 years ago
(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?)
(Assignee)

Comment 157

8 years ago
To be clear, I now have six tests in my todo list to write based on that last comment.
(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 :(
(Assignee)

Comment 159

8 years ago
(In reply to comment #158)
> 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.
I'm fine with this, but for place ids only.

> 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?
Correct (there's even a test; test_guid_change_saved!)
Good news, I can reproduce the make check orange on Linux. I don't have a solution yet, but it's a first step.
bah, stupid error:

+  nsresult rv;
+  if (!aPlace.title.IsVoid()) {
+    rv = stmt->BindStringByName(NS_LITERAL_CSTRING("title"),
+                                StringHead(aPlace.title, TITLE_LENGTH_MAX));
+  }
   NS_ENSURE_SUCCESS(rv, rv);

rv is not initialized, the NS_ENSURE_SUCCESS check should be moved inside the if, then everything will work fine. I'll update part 18 and reland 18-19.

The interesting part is that here we just timeout and no error is returned...
Created attachment 505400 [details] [diff] [review]
Part 18 - Update changes to the guid/title v1.1 (checked-in)
Attachment #503688 - Attachment is obsolete: true

Updated

8 years ago
Attachment #505400 - Attachment description: Part 18 - Update changes to the guid/title v1.1 → Part 18 - Update changes to the guid/title v1.1 (checked-in)

Updated

8 years ago
Attachment #503992 - Attachment description: Part 19 - Get all the data that we need v1.1 → Part 19 - Get all the data that we need v1.1 (checked-in)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-17 fixed-in-places] → [hardblocker][part 1-19 fixed-in-places]
Taking the liberty of marking this 22-part patch "risky".

Don't let me meddle with a system that's working, but would these reviews go faster/better if you guys (Mak/Shawn) got on the phone? Can I buy one of you a plane ticket?
Whiteboard: [hardblocker][part 1-19 fixed-in-places] → [hardblocker][part 1-19 fixed-in-places][risky]
Comment on attachment 505081 [details] [diff] [review]
Part 21 - track and notify about title changes v1.2

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

>+struct PlaceChanges
>+{
>+  PlaceChanges()
>+  : title(false)
>+  {
>+  }
>+
>+  // TODO bug 626836 hook up hidden and typed change tracking too!
>+  bool title;
>+};

I don't like much a member called title that is a bool... I was expecting it to be a string indeed...
Maybe you could rename to titleChanged, updateTitle or rename the struct to PlaceChangedFields.... BUT actually...
Is there a specific reason we need this separate struct, rather than adding a titleChanged member to VisitData?
To me looks like this is being passed in where we already have a VisitData.
I'd largely prefer having just one structure to manage it all, and I don't see a showstopper to do so.

> void
> GetStringFromJSObject(JSContext* aCtx,

>+  // |null| in JS maps to the empty string.
>+  if (JSVAL_IS_NULL(val)) {
>+    _string.Truncate();
>+    return;
>+  }

ah, I thought a js null was a void string for us, how this differs from a empty ""? which js value do I pass in to get a null in the db?

>+  NotifyTitleObservers(const nsCString& aSpec,
>+                       const nsString& aTitle)
>+  : mSpec(aSpec)
>+  , mTitle(aTitle)
>+  {
>+    NS_PRECONDITION(!NS_IsMainThread(),
>+                    "This should not be called on the main thread");

nit: I think we don't care where this object is created, the important part is where Run() is executed.
The caller has already its own thread check too

>+  NS_IMETHOD Run()
>+  {
>+    NS_PRECONDITION(NS_IsMainThread(),
>+                    "This should be called on the main thread");
>+
>+    nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
>+    NS_ENSURE_TRUE(navHistory, NS_ERROR_OUT_OF_MEMORY);
>+    nsCOMPtr<nsIURI> uri;
>+    (void)NS_NewURI(getter_AddRefs(uri), mSpec);

is NS_newURI infallible now? if not I'd prefer you checking the error.

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

> /**
>+ * Generic nsINavHistoryObserver that doesn't implement anything, but provides
>+ * dummy methods to prevent errors about an object not having a certain method.
>+ */

ah, I love this! will consider it for head_common :)

>+TitleChangedObserver.prototype = ;
>+TitleChangedObserver.prototype.onTitleChanged = function(aURI,
>+                                                         aTitle) {

nit: I think using __proto__ is more readable as a classic inheritance, btw up to you.
TitleChangedObserver.prototype = {
  __proto__: NavHistoryObserver.prototype,
  onTitleChanged: function(aURI, aTitle)
  {
    ...
  }
}

So, I think I want you to merge the 2 structs, if it's not feasible please ping me.
Attachment #505081 - Flags: review?(mak77) → review-

Updated

8 years ago
Attachment #505173 - Flags: review?(mak77) → review+
(Assignee)

Comment 166

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

https://hg.mozilla.org/mozilla-central/rev/44797377722c
(Assignee)

Comment 167

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

https://hg.mozilla.org/mozilla-central/rev/ae99d2b2efaa
(Assignee)

Comment 168

8 years ago
Comment on attachment 499878 [details] [diff] [review]
Part 3 - stub out mozIAsyncHistory methods v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/b1388b6e2119
(Assignee)

Comment 169

8 years ago
Comment on attachment 502854 [details] [diff] [review]
Part 4 - refactor hidden flag logic v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/725a035ad3e4
(Assignee)

Comment 170

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

https://hg.mozilla.org/mozilla-central/rev/705a34ab2091
(Assignee)

Comment 171

8 years ago
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/mozilla-central/rev/1e0f15394795
(Assignee)

Comment 172

8 years ago
Comment on attachment 502861 [details] [diff] [review]
Part 7 - pull FetchPageInfo onto History v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/90ba73a93180
(Assignee)

Comment 173

8 years ago
Comment on attachment 502863 [details] [diff] [review]
Part 8  - add guid and referrerSpec to VisitData v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/ea72cd89fcdd
(Assignee)

Comment 174

8 years ago
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/mozilla-central/rev/c64ae2a271f6
(Assignee)

Comment 175

8 years ago
Comment on attachment 502868 [details] [diff] [review]
Part 10 - Refactor InsertVistiedURI to InsertVisitedURIs v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/de15ef2a9d44
(Assignee)

Comment 176

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

https://hg.mozilla.org/mozilla-central/rev/b9369c68197b
(Assignee)

Comment 177

8 years ago
Comment on attachment 503004 [details] [diff] [review]
Part 12 - Fix issues with multiple visits of the same URI (checked in)

https://hg.mozilla.org/mozilla-central/rev/99bc471608e1
(Assignee)

Comment 178

8 years ago
Comment on attachment 503571 [details] [diff] [review]
Part 13 - Set page title if we have it v2.0 (checked in)

https://hg.mozilla.org/mozilla-central/rev/793fd0ed8c3f
(Assignee)

Comment 179

8 years ago
Comment on attachment 503572 [details] [diff] [review]
Part 14 - Set guid when provided v2.0 (checked in)

https://hg.mozilla.org/mozilla-central/rev/30555fb614e1
(Assignee)

Comment 180

8 years ago
Comment on attachment 503357 [details] [diff] [review]
Part 15 - only get session id if needed v1.0 (checked in)

https://hg.mozilla.org/mozilla-central/rev/f2eab9a3d674
(Assignee)

Comment 181

8 years ago
Comment on attachment 503890 [details] [diff] [review]
Part 16 - Handle a mozIVisitInfoCallback v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/ed509b5771d3
(Assignee)

Comment 182

8 years ago
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/mozilla-central/rev/63512a53e63f
(Assignee)

Comment 183

8 years ago
Comment on attachment 505400 [details] [diff] [review]
Part 18 - Update changes to the guid/title v1.1 (checked-in)

https://hg.mozilla.org/mozilla-central/rev/1bb1afd756ca
(Assignee)

Comment 184

8 years ago
Comment on attachment 503992 [details] [diff] [review]
Part 19 - Get all the data that we need v1.1 (checked-in)

https://hg.mozilla.org/mozilla-central/rev/acf017729733
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 fixed-in-places][risky] → [hardblocker][part 1-19 checked in][risky]
(Assignee)

Comment 185

8 years ago
(In reply to comment #152)
> is this supposed to always success since the previous call succeeded, or should
> we NS_ENSURE_TRUE uri for added safety?
Well, it could fail in that the object isn't a wrapped nsIURI, but then the nsCOMPtr is null, and we'll return null.

> 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.
Well, per the coding style guid, I'm returning the actual result whenever possible.  Can't do that with strings (the one that returns void) because we should not return our string classes, and I can't do that with the integer because there is no way to signal bad results with an integer return variable.  I'd prefer to keep these the way they are.

> 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
It's a shared buffer; we don't actually own it, however it will be copied into the string when we assign it, so it's all good.

> A precondition looks a weak check here for opt builds... We can receive any
> sort of js stuff in these public APIs
Mostly there to make sure our own code does the check first, as you noticed.

> 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.
Added an @pre comment.

> 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
I do not quite understand your comment here.  You don't want me to use isValidGUID, but you want a bool for isValidId?  Or do you want me to consolidate the checks?

I didn't the the latter because they are checking for two very different things.  The first cases ensure that we have at least one of:
- uri
- valid place id
- guid
We need at least one of those so we can properly identify the entry we are adding visits for.

The second time we use them (where you commented), we make sure we have visits if we only have a uri.  This check will actually be obsoleted when I address comment 104.  I think it's probably best that I leave all the logic alone until I tighten up what the API can do (which I guess I'll do in part 22 so we can get it out of the way).

> Ah, I thought this was handled in InsertVisitedURIs, does it not handle this
> anymore?
It has never actually handled that because it needs to run on the main thread.  History::VisitURI also handles it itself (but I did refactor things in part 11 so that they would share more code together).

(In reply to comment #165)
> I don't like much a member called title that is a bool... I was expecting it to
> be a string indeed...
> Maybe you could rename to titleChanged, updateTitle or rename the struct to
> PlaceChangedFields.
Changed to titleChanged.

> Is there a specific reason we need this separate struct, rather than adding a
> titleChanged member to VisitData?
> To me looks like this is being passed in where we already have a VisitData.
> I'd largely prefer having just one structure to manage it all, and I don't see
> a showstopper to do so.
The problem with adding it to VisitData is that it means we have more stuff sitting on the heap being pushed around to different threads.  With this separate structure, we get to keep the data only in the background thread, and we get to keep it on the stack.  As we end up adding more things that we need to track changes on, it seemed to make sense to minimize things we need on the heap.

> ah, I thought a js null was a void string for us, how this differs from a empty
> ""? which js value do I pass in to get a null in the db?
Ack...we have a bug in how we handle titles.  I need to think about this more...

> nit: I think we don't care where this object is created, the important part is
> where Run() is executed.
> The caller has already its own thread check too
Yeah, it doesn't actually matter where this is ran...

> is NS_newURI infallible now? if not I'd prefer you checking the error.
It isn't, however we are creating a URI from a spec we got from an nsIURI.  It's clearly not going to be malformed, so this should be fine to ignore.

> ah, I love this! will consider it for head_common :)
yup, if we need to pull it out, we can do that :)

> nit: I think using __proto__ is more readable as a classic inheritance, btw up
> to you.
Ah, I couldn't figure out how to make that work, so I just went with the quick and dirty method.  I like your way better.
(Assignee)

Comment 186

8 years ago
Created attachment 505486 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.1 (checked in)

This address the review comments, with the exception of adding a bunch of tests, which I'd like to do in a later part if that is OK with you mak.

I also went ahead and undid the changes asuth added in https://hg.mozilla.org/projects/places/rev/79348d4f7586 since they are not needed with this change.
Attachment #503993 - Attachment is obsolete: true
Attachment #505486 - Flags: review?(lw)
Attachment #503993 - Flags: review?(lw)

Updated

8 years ago
Depends on: 627474
(Assignee)

Comment 187

8 years ago
Created attachment 505520 [details] [diff] [review]
Part 21 - track and notify about title changes v1.3

(In reply to comment #185)
> > ah, I thought a js null was a void string for us, how this differs from a empty
> > ""? which js value do I pass in to get a null in the db?
> Ack...we have a bug in how we handle titles.  I need to think about this
> more...
Turns out I just needed to make our binding logic match what nsNavHistory::SetPageTitle does.  If we have an empty (or a void) string, it will bind null to the value.  However, if we already have a title, we won't clear it out by not passing one in (test added in this version) because we'll load it in FetchPageInfo.

This should address your other review comments as well.
Attachment #505081 - Attachment is obsolete: true
Attachment #505520 - Flags: review?(mak77)
(Assignee)

Comment 188

8 years ago
(In reply to comment #185)
> The second time we use them (where you commented), we make sure we have visits
> if we only have a uri.  This check will actually be obsoleted when I address
> comment 104.  I think it's probably best that I leave all the logic alone until
> I tighten up what the API can do (which I guess I'll do in part 22 so we can
> get it out of the way).
And by part 22 here, I meant part 23.  Forgot I already had a part 22...
(Assignee)

Comment 189

8 years ago
Created attachment 505557 [details] [diff] [review]
Part 23 - Update API (with test changes) v1.0

This contains the API changes I talked about in comment 104.  Tweaked some code so that it follows the new rules, and made the test for this case much more extensive.

What's left to do from comment 104:
- Dispatch begin and end batch notifications (+ tests)
- Dispatch one callback per place instead of one callback per visit.  The Sync team really really wants this.
- A handful of tests that I'm tracking in a remember the milk list (can figure out how to post if people want to see it).
Attachment #505557 - Flags: superreview?(robert.bugzilla)
Attachment #505557 - Flags: review?(mak77)
(Assignee)

Comment 190

8 years ago
(In reply to comment #189)
> - Dispatch one callback per place instead of one callback per visit.  The Sync
> team really really wants this.
After discussing this in #Sync, this won't work.  What we are going to move forward with is modifying mozIVisitInfoCallback to look like this:
void onComplete(in nsresult aResultCode,
                in mozIPlaceInfo aPlaceInfo,
                in boolean aMoreResultsPending)
The idea is that aMoreResutlsPending will be true until the very last notification, at which point it will be false.  This satisfies Sync's use case, and makes it implementable.
Comment on attachment 505486 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.1 (checked in)

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

>+#include "nsContentUtils.h"

>+    nsresult rv = nsContentUtils::XPConnect()->GetWrappedNativeOfJSObject(
>+      aCtx,
>+      JSVAL_TO_OBJECT(uriVal),
>+      getter_AddRefs(wrappedObj)
>+    );

this looks a lot like the return of bug 627474 (libgklayout part)

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

I'm still unsure you should not rather use a common nsString and Adopt, does assign make a copy of the buffer? dependentstring does not own its buffer
Also, is the returned string assured to be null terminated?
(In reply to comment #185)
> > 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
> It's a shared buffer; we don't actually own it, however it will be copied into
> the string when we assign it, so it's all good.

ok, missed this comment, just be sure we get a null terminated string.

> > 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
> I do not quite understand your comment here.  You don't want me to use
> isValidGUID, but you want a bool for isValidId?  Or do you want me to
> consolidate the checks?

consolidate the checks earlier and use the consolidated var later.
I mean the fist case iirc did (A || B || C) the second case (D && (B || C)), I think consolidating B||C in hasValidId would be more readable. If I don't recall correctly nevermind.

> (In reply to comment #165)
> The problem with adding it to VisitData is that it means we have more stuff
> sitting on the heap being pushed around to different threads.  With this
> separate structure, we get to keep the data only in the background thread, and
> we get to keep it on the stack.  As we end up adding more things that we need
> to track changes on, it seemed to make sense to minimize things we need on the
> heap.

I think the cost here is really low, this is just a struct of bools and mergint to visitData will just add bools to a existing struct, the code clarity would gain quite a bit instead. I still think it's the best way to handle this for now. If in future we see this ends up being more than a list of bools then I'd agree with moving them to a separate struct.

> > is NS_newURI infallible now? if not I'd prefer you checking the error.
> It isn't, however we are creating a URI from a spec we got from an nsIURI. 
> It's clearly not going to be malformed, so this should be fine to ignore.

provided we don't run out of memory in the creation...
Comment on attachment 505557 [details] [diff] [review]
Part 23 - Update API (with test changes) v1.0

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

>    * @throws NS_ERROR_INVALID_ARG
>    *         - Passing in NULL for aPlaceInfo.
>    *         - Not providing at least one valid guid, placeId, or uri for all
>    *           mozIPlaceInfo object[s].

So, I still feel this ambiguous. If I pass those wrongly, there must be a priority or a error condition.
As I said previously, if I pass in a placeId and a URI and these don't point to the same row (because I'm drunk while I'm developing this add-on) I still have no idea what happens. If we throw this should state it as a error condition. If we don't throw I have really no idea what happens: will we change uri for that placeId (please NO)? or update entry based on the uri and ignore the placeid? or update entry based on placeId and ignore the uri?
This is practically trying to update a row of a database providing 3 different primary keys, each one is unique and can be used or omissed, moreover guid can also be changed by this same api (I hope placeid and uri cannot).
So it must be _crystal clear_ about what happens in the various combinations.
Comment on attachment 505486 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.1 (checked in)

(In reply to comment #186)
> Created attachment 505486 [details] [diff] [review]
> Part 20 - actual API implementation + tests v1.1

>+void
>+GetStringFromJSObject(JSContext* aCtx,
>+                      JSObject* aObject,
>+                      const char* aProperty,
>+                      nsDependentString& _string)

You suggested in comment 128 that you would change _string would be an nsString.  Assuming you do that, r+.
Attachment #505486 - Flags: review?(lw) → review+
(Assignee)

Comment 195

8 years ago
(In reply to comment #194)
> You suggested in comment 128 that you would change _string would be an
> nsString.  Assuming you do that, r+.
That's funny...I distinctly recall changing that...fixed now!

(In reply to comment #193)
> As I said previously, if I pass in a placeId and a URI and these don't point to
> the same row (because I'm drunk while I'm developing this add-on) I still have
> no idea what happens. If we throw this should state it as a error condition. If
> we don't throw I have really no idea what happens: will we change uri for that
> placeId (please NO)? or update entry based on the uri and ignore the placeid?
> or update entry based on placeId and ignore the uri?
> This is practically trying to update a row of a database providing 3 different
> primary keys, each one is unique and can be used or omissed, moreover guid can
> also be changed by this same api (I hope placeid and uri cannot).
> So it must be _crystal clear_ about what happens in the various combinations.
We would not throw, but we would raise an error to the callback.  I haven't specified this because I have zero test coverage for it.  When I add the tests, I'll be happy to add documentation about it, but until it's tested, it is unspecified.  I know what should happen, but I'm not sure that it will happen yet.

(In reply to comment #192)
> ok, missed this comment, just be sure we get a null terminated string.
It is null terminated, and should be an nsString (although, it wasn't in the patch oddly enough).

> consolidate the checks earlier and use the consolidated var later.
> I mean the fist case iirc did (A || B || C) the second case (D && (B || C)), I
> think consolidating B||C in hasValidId would be more readable. If I don't
> recall correctly nevermind.
The second check went away in part 23, so I suppose we can stop debating about it :)

> I think the cost here is really low, this is just a struct of bools and mergint
> to visitData will just add bools to a existing struct, the code clarity would
> gain quite a bit instead. I still think it's the best way to handle this for
> now. If in future we see this ends up being more than a list of bools then I'd
> agree with moving them to a separate struct.
It's low cost for one entry, sure.  As of now it is one byte, and with the changes in bug 626836, it would be a total of three bytes.  However, those three extra bytes would be aligned on a new cache line given our current VisitData struct, making it consume four bytes (it consumes four even now).  This may not seem like much for a single VisitData object, but it adds up.  Right now (if I recall correctly), Sync adds visits in batches of 50 per place, with up to 10 or 20 visits per place (I can't recall which, exactly, and nobody is around to ask).  That results in 500-1000 VisitData structs being created per batch, or 2000-4000 additional bytes on the main thread heap (because jemalloc has a different heap for each thread).  This ends up polluting the main thread's heap needlessly, and will contribute to fragmentation.  Maybe I'm being paranoid though.

> provided we don't run out of memory in the creation...
new is infallible, so if we did, the program would die anyway.

(In reply to comment #191)
> this looks a lot like the return of bug 627474 (libgklayout part)
I think this might be OK actually, but regardless, non-libxul builds are not tier1.  If we break them, someone can file a bug with a proper patch, but I'd rather not speculate on what will break it, and what would fix it since I don't intend to build non-libxul.  If ehsan wanted to comment though, that'd be fine.
(Assignee)

Comment 196

8 years ago
Comment on attachment 505486 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.1 (checked in)

http://hg.mozilla.org/projects/places/rev/c03b00587c5d
Attachment #505486 - Attachment description: Part 20 - actual API implementation + tests v1.1 → Part 20 - actual API implementation + tests v1.1 (checked in)
Comment on attachment 505557 [details] [diff] [review]
Part 23 - Update API (with test changes) v1.0

r+ with the condition that we clarify the ambiguous situation I presented in comment 193 before the final release.
Attachment #505557 - Flags: review?(mak77) → review+
> That results in 500-1000 VisitData structs being created
> per batch, or 2000-4000 additional bytes on the main thread heap (because
> jemalloc has a different heap for each thread).  This ends up polluting the
> main thread's heap needlessly, and will contribute to fragmentation.

Honestly, 4KB of short-lived heap space in the best case doesn't look much of a win; the fragmentation win is uncertain, since just having the VisitData structs is a fragmentation problem, making each of them them bigger by 4 bytes won't make a difference in percentage.
On the other side not having to bring on 2 structs to track the same thing is a clear code clarity win, and the code changed would be like half-sized (wherever we pass a place we have the changed data already).
Being really paranoid about this, we could use a bitfield and reduce those 4 bytes to one , with a .changes field and doing (places.changes & PLACES_TITLE_CHANGED), then we could track up to 8 changes with a single byte.
Whiteboard: [hardblocker][part 1-19 checked in][risky] → [hardblocker][part 1-19 checked in][part 20 fixed in places][risky]
(Assignee)

Comment 199

8 years ago
Test cases that I need written but have not done so yet (so, if you have spare cycles and want to write them, this is a great opportunity to help!):
- Test for dropping referrer when it isn't "recent enough"
- Test for frecency with updatePlaces
- Test for inserting a place with a duplicate guid, place id, and existing uri fails
- Test for inserting a place with a duplicate place id and existing uri fails
- Test for inserting a place with duplicate guid and existing uri fails
- Test for invalid referrer
- Test for invalid session id
- Test proper visit notifications dispatched

These need to be done on top of code living in the Places branch (http://hg.mozilla.org/projects/places/).  Some of these tests may pass; I haven't even had a chance to figure out the expected outcome yet.

If you want to steal some of these off my plate, file a new bug and mark it blocking this (it become a hard blocker!).
(Assignee)

Comment 200

8 years ago
Also, I should add, that if you are going to steal some tests, you want to patch this file:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_async_history_api.js
(Assignee)

Comment 201

8 years ago
Created attachment 506499 [details] [diff] [review]
Part 21 - track and notify about title changes v1.4

Updated per review comments.
Attachment #505520 - Attachment is obsolete: true
Attachment #506499 - Flags: review?(mak77)
Attachment #505520 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20 fixed in places][risky] → [hardblocker][part 1-19 checked in][part 20 fixed in places][risky][needs review mak][needs sr rs]
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20 fixed in places][risky][needs review mak][needs sr rs] → [hardblocker][part 1-19 checked in][part 20 fixed in places][risky][part 21 needs review mak][part 23 needs sr rs]
(Assignee)

Comment 202

8 years ago
Comment on attachment 505173 [details] [diff] [review]
Part 22 - Dispatch errors for URIs that fail canAddURI v1.0 (checked in)

http://hg.mozilla.org/projects/places/rev/af4e43e6d048
Attachment #505173 - Attachment description: Part 22 - Dispatch errors for URIs that fail canAddURI → Part 22 - Dispatch errors for URIs that fail canAddURI v1.0 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20 fixed in places][risky][part 21 needs review mak][part 23 needs sr rs] → [hardblocker][part 1-19 checked in][part 20,22 fixed-in-places][risky][part 21 needs review mak][part 23 needs sr rs]
(Assignee)

Comment 203

8 years ago
(In reply to comment #198)
> On the other side not having to bring on 2 structs to track the same thing is a
> clear code clarity win, and the code changed would be like half-sized (wherever
> we pass a place we have the changed data already).
I think that is subjective, fwiw.  I think it's clearer the way I implemented it ;)

> Being really paranoid about this, we could use a bitfield and reduce those 4
> bytes to one , with a .changes field and doing (places.changes &
> PLACES_TITLE_CHANGED), then we could track up to 8 changes with a single byte.
That wouldn't actually matter, because the struct will still be expanded by four bytes in total.

Regardless, I've changed it in the most recent patch to what you wanted.
(Assignee)

Comment 204

8 years ago
Created attachment 506541 [details] [diff] [review]
Part 23 - Update API (with test changes) v1.1 (checked in)

Not going to deprecate nsINavHistoryService::addVisit, and going to mark this new API as experimental.
Attachment #505557 - Attachment is obsolete: true
Attachment #506541 - Flags: superreview?(robert.bugzilla)
Attachment #505557 - Flags: superreview?(robert.bugzilla)
Comment on attachment 506499 [details] [diff] [review]
Part 21 - track and notify about title changes v1.4

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

>+  // We track if we change the title, but will add the current title to _place
>+  // if we do not have one set.
>+  nsAutoString title;
>+  rv = stmt->GetString(1, title);
>+  NS_ENSURE_SUCCESS(rv, true);
>+  _place.titleChanged = !(_place.title.Equals(title) ||
>+                          (_place.title.IsVoid() && title.IsVoid()));

nit: what about moving the comment before setting titleChanged?
Attachment #506499 - Flags: review?(mak77) → review+
(Assignee)

Comment 206

8 years ago
Created attachment 506583 [details] [diff] [review]
Part 24 - Dispatch observer topic when complete v1.0 (checked in)
Attachment #506583 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20,22 fixed-in-places][risky][part 21 needs review mak][part 23 needs sr rs] → [hardblocker][part 1-19 checked in][part 20,22 fixed-in-places][risky][part 24 needs review mak][part 23 needs sr rs]
(Assignee)

Comment 207

8 years ago
Comment on attachment 506499 [details] [diff] [review]
Part 21 - track and notify about title changes v1.4

http://hg.mozilla.org/projects/places/rev/3962f456fd9a
Attachment #506499 - Attachment description: Part 21 - track and notify about title changes v1.4 → Part 21 - track and notify about title changes v1.4 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20,22 fixed-in-places][risky][part 24 needs review mak][part 23 needs sr rs] → [hardblocker][part 1-19 checked in][part 20-22 fixed-in-places][risky][part 24 needs review mak][part 23 needs sr rs]
Comment on attachment 506583 [details] [diff] [review]
Part 24 - Dispatch observer topic when complete v1.0 (checked in)

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

+  else {
+    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
+    if (obs)
+      (void)obs->NotifyObservers(nsnull, mTopic, nsnull);
+  }

add a main-thread precondition here please


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

>+// Topic used to notify that work in mozIAsyncHistory::updatePlaces is done.
>+#define TOPIC_UPDATEPLACES_COMPLETE "places-updatePlaces-complete"

what about adding this topic to PlacesUtils.jsm and use the PlacesUtils constant in the test? we already have the other topics there.


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

>@@ -428,6 +429,50 @@ function test
>+  let observer = {
>+    observe: function(aSubject, aTopic, aData)
>+    {
>+      do_check_eq(aTopic, TOPIC_UPDATEPLACES_COMPLETE);
>+      do_check_eq(callbackCount, EXPECTED_COUNT);
>+      Services.obs.removeObserver(observer, TOPIC_UPDATEPLACES_COMPLETE);
>+      run_next_test();
>+    },
>+  };
>+  Services.obs.addObserver(observer, TOPIC_UPDATEPLACES_COMPLETE, false);

nit: you could inline a observing function rather than making a full object, would be more compact (-4 rows), that looks like a positive thing, considering size of this test file.
Attachment #506583 - Flags: review?(mak77) → review+
(Assignee)

Comment 209

8 years ago
(In reply to comment #208)
> what about adding this topic to PlacesUtils.jsm and use the PlacesUtils
> constant in the test? we already have the other topics there.
I didn't do this because this observer topic is only a stopgap for Firefox 4 and Sync.  I think we'll want to rework this API post-4.0, and I don't really want people to rely on this topic (apart from Sync).  OK to leave it as-is?

> nit: you could inline a observing function rather than making a full object,
> would be more compact (-4 rows), that looks like a positive thing, considering
> size of this test file.
didn't know we could do that!
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20-22 fixed-in-places][risky][part 24 needs review mak][part 23 needs sr rs] → [hardblocker][part 1-19 checked in][part 20-22 fixed-in-places][risky][part 23 needs sr rs]
(In reply to comment #209)
> (In reply to comment #208)
> > what about adding this topic to PlacesUtils.jsm and use the PlacesUtils
> > constant in the test? we already have the other topics there.
> I didn't do this because this observer topic is only a stopgap for Firefox 4
> and Sync.  I think we'll want to rework this API post-4.0, and I don't really
> want people to rely on this topic (apart from Sync).  OK to leave it as-is?

In this vision yes, I agree it's better to not spread it, if it's going to go away post FX4. leave it as it is.
(Assignee)

Comment 211

8 years ago
Comment on attachment 506583 [details] [diff] [review]
Part 24 - Dispatch observer topic when complete v1.0 (checked in)

http://hg.mozilla.org/projects/places/rev/8abbd360989e
Attachment #506583 - Attachment description: Part 24 - Dispatch observer topic when complete v1.0 → Part 24 - Dispatch observer topic when complete v1.0 (checked in)
(Assignee)

Comment 212

8 years ago
Once part 23 gets sr, I'll land it on Places branch, make sure everything is green still, and then merge to mozilla-central.  At that point, we'll have everything that Sync needs landed in mozilla-central.

Note, however, that we may still end up finding issues with this code as we write more test cases.
Whiteboard: [hardblocker][part 1-19 checked in][part 20-22 fixed-in-places][risky][part 23 needs sr rs] → [hardblocker][part 1-19 checked in][part 20-22,24 fixed-in-places][risky][part 23 needs sr rs]
Attachment #506541 - Flags: superreview?(robert.bugzilla) → superreview+
Whiteboard: [hardblocker][part 1-19 checked in][part 20-22,24 fixed-in-places][risky][part 23 needs sr rs] → [hardblocker][part 1-19 checked in][part 20-22,24 fixed-in-places][risky]
(Assignee)

Comment 213

8 years ago
Comment on attachment 506499 [details] [diff] [review]
Part 21 - track and notify about title changes v1.4

Bah...this had Moth failures because we don't have tests for IHistory::SetPageTitlte in test_IHistory.cpp.  New patch with more tests inbound...

http://hg.mozilla.org/projects/places/rev/5186a60fddfd
Attachment #506499 - Attachment description: Part 21 - track and notify about title changes v1.4 (checked in) → Part 21 - track and notify about title changes v1.4

Updated

8 years ago
tracking-fennec: 2.0b4+ → 2.0b5+
(Assignee)

Comment 214

8 years ago
Comment on attachment 506541 [details] [diff] [review]
Part 23 - Update API (with test changes) v1.1 (checked in)

http://hg.mozilla.org/projects/places/rev/61bc209bf1b4
Attachment #506541 - Attachment description: Part 23 - Update API (with test changes) v1.1 → Part 23 - Update API (with test changes) v1.1 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20-22,24 fixed-in-places][risky] → [hardblocker][part 1-19 checked in][part 20,22-24 fixed-in-places][risky]
(Assignee)

Comment 215

8 years ago
Created attachment 506873 [details] [diff] [review]
Part 21 - track and notify about title changes v1.5 (checked in)

The SetPageTitle class was slightly busted, which is why we had the test failures.  This fixes it.
Attachment #506499 - Attachment is obsolete: true
Attachment #506873 - Flags: review?(mak77)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20,22-24 fixed-in-places][risky] → [hardblocker][part 1-19 checked in][part 20,22-24 fixed-in-places][risky][part 21 needs review mak]

Updated

8 years ago
Depends on: 628805

Updated

8 years ago
Attachment #506873 - Flags: review?(mak77) → review+
(Assignee)

Comment 216

8 years ago
Comment on attachment 506873 [details] [diff] [review]
Part 21 - track and notify about title changes v1.5 (checked in)

http://hg.mozilla.org/projects/places/rev/2f9e83d546d3
Attachment #506873 - Attachment description: Part 21 - track and notify about title changes v1.5 → Part 21 - track and notify about title changes v1.5 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20,22-24 fixed-in-places][risky][part 21 needs review mak] → [hardblocker][part 1-19 checked in][part 20-24 fixed-in-places][risky]
(Assignee)

Comment 217

8 years ago
Still failing browser_privatebrowsing_placestitle.js, but it looks like it's just a failure in notifications.  The page's title is set correctly, we just don't seem to dispatch the right title in our observers.  Investigating...
(Assignee)

Comment 218

8 years ago
OK, so the failure looks to be because we also may notify about title changes when we add a visit now.  However, we shouldn't have to notify in this case, so digging deeper...
(Assignee)

Comment 219

8 years ago
Created attachment 507265 [details] [diff] [review]
Part 25 - only notify on title changes when they actually happen v1.0 (checked in)

What was going on was this:
History::VisitURI never set a title on the VisitData struct it gave to InsertVisitedURIs (which is the correct behavior).  However, when the second visit to a page came up, and we called into FetchPageInfo we would say that the title changed because what was in the VisitData struct (a void string) was different from what was in the database.  As a result, we would notify about title change (and actually clear the title in the database, which is not what we want to have happen).

This change makes it so that if we do not provide a title (VisitData.title is void), we know that we did not change the title.  An empty string will always indicate that we need to clear the title, and we handle that correctly with the changes in part 21.

On an unrelated note, I cannot believe I just spent three hours debugging this :(
Attachment #507265 - Flags: review?(mak77)

Updated

8 years ago
Attachment #507265 - Flags: review?(mak77) → review+
(Assignee)

Comment 220

8 years ago
Comment on attachment 505486 [details] [diff] [review]
Part 20 - actual API implementation + tests v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/c03b00587c5d
(Assignee)

Comment 221

8 years ago
Comment on attachment 506873 [details] [diff] [review]
Part 21 - track and notify about title changes v1.5 (checked in)

https://hg.mozilla.org/mozilla-central/rev/2f9e83d546d3
(Assignee)

Comment 222

8 years ago
Comment on attachment 505173 [details] [diff] [review]
Part 22 - Dispatch errors for URIs that fail canAddURI v1.0 (checked in)

https://hg.mozilla.org/mozilla-central/rev/af4e43e6d048
(Assignee)

Comment 223

8 years ago
Comment on attachment 506541 [details] [diff] [review]
Part 23 - Update API (with test changes) v1.1 (checked in)

https://hg.mozilla.org/mozilla-central/rev/61bc209bf1b4
(Assignee)

Comment 224

8 years ago
Comment on attachment 506583 [details] [diff] [review]
Part 24 - Dispatch observer topic when complete v1.0 (checked in)

https://hg.mozilla.org/mozilla-central/rev/8abbd360989e
(Assignee)

Comment 225

8 years ago
Comment on attachment 507265 [details] [diff] [review]
Part 25 - only notify on title changes when they actually happen v1.0 (checked in)

https://hg.mozilla.org/projects/places/rev/4f3ea17d645d
https://hg.mozilla.org/mozilla-central/rev/4f3ea17d645d
Attachment #507265 - Attachment description: Part 25 - only notify on title changes when they actually happen v1.0 → Part 25 - only notify on title changes when they actually happen v1.0 (checked in)
(Assignee)

Updated

8 years ago
Whiteboard: [hardblocker][part 1-19 checked in][part 20-24 fixed-in-places][risky] → [hardblocker][part 1-25 checked in][risky]
(Assignee)

Comment 226

8 years ago
As far as I can tell there are three things left to (not counting bug 628805 and bug 628865 which are softblockers for tests), there are three things left to do here:
- Test proper visit notifications dispatched
- Test for frecency with updatePlaces
- Better from_visit tests (see bug 606966 comment 154)

I'd like to push these off to into softblocker follow-ups and split the work up (I'd actually like mak to take the last item since he seems to best understand it).

Does this make sense to folks?  I'm fairly certain that all these tests are going to pass (with the exception of the last one, but that shouldn't impact Sync).  Any objections?
(Assignee)

Updated

8 years ago
Blocks: 629539
(Assignee)

Updated

8 years ago
Blocks: 629540
(Assignee)

Updated

8 years ago
Blocks: 629542
(Assignee)

Updated

8 years ago
Blocks: 628805, 628865
No longer depends on: 628805, 628865
Whiteboard: [hardblocker][part 1-25 checked in][risky] → [hardblocker]
(Assignee)

Comment 227

8 years ago
Per discussions on irc, moving the tests to a follow-up.  If the Sync team hits any issues, it'll be a hardblocker on me (or mak) to fix.  This bug is fixed (with only 227 comments)!
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11

Updated

8 years ago
Depends on: 629577
Keywords: dev-doc-needed → dev-doc-complete
Yeah sure, sorry! Will double check next time.
Blocks: 632625

Updated

7 years ago
Depends on: 647557
You need to log in before you can comment on or make changes to this bug.