633266 - nsINavHistoryObserver: also pass in GUID whenever we pass in a URI

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Philipp von Weitershausen [:philikon]]

Sync needs to determine the GUID of history items as they're visited and before they're deleted. In the first case it's ok not to know the GUID right away, but since Places would probably know it already, it seems we can avoid some I/O here.

In the latter case, however, the observer is our last chance to find out which item should be deleted. That means we have to block the observer right now to ensure we find out the GUID, before we let Places nuke the item. Not cool.

I propose adding a 'aGUID' parameter to all nsINavHistoryObserver methods that take a 'aURI' argument, namely

  onBeforeDeleteURI
  onDeleteURI
  onDeleteVisits
  onPageChanged
  onTitleChanged
  onVisit

Comment 1

[Marco Bonardo [:mak]]

I'm probably going to prioritize bug 431274 over this, so if anybody wants to work on this one, feel free to steal it, should not be incredibly hard to do, maybe boring :)

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to comment #1)
> I'm probably going to prioritize bug 431274 over this, so if anybody wants
> to work on this one, feel free to steal it, should not be incredibly hard to
> do, maybe boring :)

I'll give this a go.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #0)
>   onPageChanged
>   onTitleChanged

We don't need those two in Sync and the code will be a lot simpler if I leave these out, so that's what I'm going to do.

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Tests

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): IDL changes

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Implementation

Comment 7

[Philipp von Weitershausen [:philikon]]

Try build for v1 patches: http://tbpl.mozilla.org/?tree=Try&rev=9e4eb518ffed

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 536199 [details] [diff] [review]
Part 2 (v1): IDL changes

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

r=me with the following addressed.
and please file a follow-up to add guids to the missing notifications as well, lower prio but it should be done sometimes.

::: toolkit/components/places/nsINavHistoryService.idl
@@ +873,2 @@
>     */
>    void onTitleChanged(in nsIURI aURI, in AString aPageTitle);

while here, may you put each agument on its own line please?

@@ +921,3 @@
>     */
>    const unsigned long ATTRIBUTE_FAVICON = 3; // favicon updated, aString = favicon annotation URI
>    void onPageChanged(in nsIURI aURI, in unsigned long aWhat, in AString aValue);

ditto for arguments on their own lines

OT: OMG this thing sucks, a magic number attribute without any documentation, and just one case? I highly suspect this thing should be fixed somehow in future, either onPageFaviconChange, or merge onTitleChanged in it, like we do in onItemChanged.

::: toolkit/components/places/nsPIPlacesHistoryListenersNotifier.idl
@@ +67,5 @@
>     */
> +  void notifyOnPageExpired(in nsIURI aURI,
> +                           in PRTime aVisitTime,
> +                           in boolean aWholeEntry,
> +                           in ACString aGUID);

looks like you didn't change the uuid here

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 536200 [details] [diff] [review]
Part 3 (v1): Implementation

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

I'm worried because I didn't find anything really broken here! As I thought this patch was boring :p

::: toolkit/components/places/nsNavHistory.cpp
@@ +1860,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>      NS_ASSERTION(hasResult, "hasResult is false but the call succeeded?");
>      pageId = getIdStmt->AsInt64(0);
> +    rv = getIdStmt->GetUTF8String(5, guid);
> +    NS_ENSURE_SUCCESS(rv, rv);

Add a comment above mDBGetURLPageInfo definition about the fact it doesn't stick to kGetInfoIndex_* constants (it was till now)

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 536200 [details] [diff] [review]
Part 3 (v1): Implementation

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

::: toolkit/components/places/nsNavHistory.cpp
@@ +2091,2 @@
>  {
>    PRUint32 added = 0;

I forgot, may you assert here !aGUID.IsEmpty()

@@ +5428,2 @@
>  {
>    // Invalidate the cached value for whether there's history or not.

I forgot, may you assert here !aGUID.IsEmpty()

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 536198 [details] [diff] [review]
Part 1 (v1): Tests

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

Seems fine as well, once comments are addressed. I was mostly worried that we may notify an empty guid, but with the assertion I asked in previous comments I'll be happier :)

I'd like if this could land on Places branch first (there are other changes to nsNavHistory.cpp there, so it may need a merge, but should be easy)

::: toolkit/components/places/tests/unit/test_async_history_api.js
@@ +126,4 @@
>    {
>      do_log_info("onVisit(" + aURI.spec + ", " + aVisitId + ", " + aTime +
>                  ", " + aSessionId + ", " + aReferringId + ", " +
> +                aTransitionType + ")"); 

add guid to this do_log_info?

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +41,5 @@
> +  query.executeStep();
> +  let guid = query.getUTF8String(0);
> +  query.finalize();
> +  return guid;
> +}

do_check_guid_for_uri may do everything you need without adding another new helper

@@ +59,5 @@
> +  });
> +  let testuri = NetUtil.newURI("http://firefox.com/");
> +  let testtime = Date.now() * 1000;
> +  PlacesUtils.history.addVisit(
> +    testuri, testtime, null, Ci.nsINavHistoryService.TRANSITION_TYPED, false, 0);

since you do the same in all tests, I suspect a add_visit helper may make sense

::: toolkit/components/places/tests/unit/test_onBeforeDeleteURI_observer.js
@@ +72,3 @@
>    {
>      this.removedURI = aURI;
> +    this.removedGUID = aGUID;

add do_check_guid_for_uri(aURI, aGUID) here please.

Comment 12

[Philipp von Weitershausen [:philikon]]

Thanks for the review, Marco!

(In reply to comment #11)
> Seems fine as well, once comments are addressed. I was mostly worried that
> we may notify an empty guid, but with the assertion I asked in previous
> comments I'll be happier :)

I've incorporated your comments above and will upload new patches shortly.

> I'd like if this could land on Places branch first (there are other changes
> to nsNavHistory.cpp there, so it may need a merge, but should be easy)

Absolutely. Once I get sr+, I can land it there. We now have TPS runs for Places as well, so we can even land the related tracker change (bug 660753) there.

> do_check_guid_for_uri may do everything you need without adding another new
> helper

w00t, I missed that one! I do still need a getGUID helper for the onDeleteURI case because by the time that's called, the record is gone so I need to get the GUID ahead of time. But I think I'll just factor this out of do_check_guid_for_uri because it seems like a generic enough helper.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1 dupe): Tests

Incorporate Marco's review comments (comment 11). Just asking for review again for factoring do_get_guid_for_uri out of do_check_guid_for_uri.

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): Tests

Bah, this is actually the v2 patch.

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): IDL changes

Incorporate Marco's review comments (comment 8). Just needs superreview at this point.

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2): Implementation

Incorporate Marco's review comments (comment 9 and comment 10). The asserts for !GUID.IsEmpty() did catch one case where we were notifying empty GUIDs from updatePlaces. I fixed this in DoDatabaseInserts by making sure we also do a FetchPageInfo when the given GUID. Asking for review for this change.

Comment 17

[Philipp von Weitershausen [:philikon]]

Try build on top of Places tip: http://tbpl.mozilla.org/?tree=Try&rev=f15dd783b20a

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to comment #16)
> I fixed this in DoDatabaseInserts by making sure we also
> do a FetchPageInfo when the given GUID.

...when the given GUID is empty.

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 537432 [details] [diff] [review]
Part 1 (v2): Tests

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

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +73,5 @@
> +
> +    run_next_test();
> +  });
> +  let [testuri] = add_visit();
> +  PlacesUtils.history.removePage(testuri);

removePage is part of nsIBrowserHistory, so here you should use PlacesUtils.bhistory.removePage(

@@ +86,5 @@
> +    run_next_test();
> +  });
> +  let [testuri] = add_visit();
> +  let testguid = do_get_guid_for_uri(testuri);
> +  PlacesUtils.history.removePage(testuri);

ditto

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 537434 [details] [diff] [review]
Part 3 (v2): Implementation

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

You're free to land in Places, or I can do that if you wish, just let me know.

::: toolkit/components/places/nsNavHistory.cpp
@@ +6945,5 @@
>    } else {
>      // Insert the new place entry
>      rv = InternalAddNewPage(aURI, aTitle, hidden == 1,
>                              aTransitionType == TRANSITION_TYPED, 0,
> +                            PR_FALSE, &placeId, guid);

even if we don't (yet) use guid here, I'd like if you could also update the if (alreadyVisited) branch here, to assign to guid, the query knows about it. I would like to avoid error-prone paths where one may think guid is correctly assigned.

Comment 21

[Philipp von Weitershausen [:philikon]]

Landed in Places with Marco's review comments addressed:

Part 1: http://hg.mozilla.org/projects/places/rev/78012c46e991
Part 2: http://hg.mozilla.org/projects/places/rev/c8b4de691036
Part 3: http://hg.mozilla.org/projects/places/rev/5cb0cca95254

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/78012c46e991
http://hg.mozilla.org/mozilla-central/rev/c8b4de691036
http://hg.mozilla.org/mozilla-central/rev/5cb0cca95254