Closed Bug 633266 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7
Tracking Status
status2.0 --- wontfix

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [places-next-wanted][fixed in places])

Attachments

(3 files, 4 obsolete files)

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
Whiteboard: [places-next-wanted]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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 :)
(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.
Assignee: mak77 → philipp
(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.
Attached patch Part 1 (v1): Tests (obsolete) — Splinter Review
Attachment #536198 - Flags: review?(mak77)
Attached patch Part 2 (v1): IDL changes (obsolete) — Splinter Review
Attachment #536199 - Flags: superreview?(robert.bugzilla)
Attachment #536199 - Flags: review?(mak77)
Attached patch Part 3 (v1): Implementation (obsolete) — Splinter Review
Attachment #536200 - Flags: review?(mak77)
Blocks: 660753
No longer blocks: 633062
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
Attachment #536199 - Flags: review?(mak77) → review+
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)
Attachment #536200 - Flags: review?(mak77) → review+
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 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.
Attachment #536198 - Flags: review?(mak77) → review+
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.
Attached patch Part 1 (v1 dupe): Tests (obsolete) — Splinter Review
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.
Attachment #536198 - Attachment is obsolete: true
Attachment #537431 - Flags: review?(mak77)
Bah, this is actually the v2 patch.
Attachment #537431 - Attachment is obsolete: true
Attachment #537432 - Flags: review?(mak77)
Attachment #537431 - Flags: review?(mak77)
Attachment #537431 - Attachment description: Part 1 (v2): Tests → Part 1 (v1 dupe): Tests
Incorporate Marco's review comments (comment 8). Just needs superreview at this point.
Attachment #536199 - Attachment is obsolete: true
Attachment #537433 - Flags: superreview?(robert.bugzilla)
Attachment #536199 - Flags: superreview?(robert.bugzilla)
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.
Attachment #536200 - Attachment is obsolete: true
Attachment #537434 - Flags: review?(mak77)
(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.
Attachment #537433 - Flags: superreview?(robert.bugzilla) → superreview+
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
Attachment #537432 - Flags: review?(mak77) → review+
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.
Attachment #537434 - Flags: review?(mak77) → review+
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
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed in places]
Depends on: 662806
http://hg.mozilla.org/mozilla-central/rev/78012c46e991
http://hg.mozilla.org/mozilla-central/rev/c8b4de691036
http://hg.mozilla.org/mozilla-central/rev/5cb0cca95254
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.