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

RESOLVED FIXED in mozilla7

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

({dev-doc-complete})

unspecified
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status2.0 wontfix)

Details

(Whiteboard: [places-next-wanted][fixed in places])

Attachments

(3 attachments, 4 obsolete attachments)

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

Updated

6 years ago
status2.0: --- → wontfix

Updated

6 years ago
Whiteboard: [places-next-wanted]

Updated

6 years ago
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.
Created attachment 536198 [details] [diff] [review]
Part 1 (v1): Tests
Attachment #536198 - Flags: review?(mak77)
Created attachment 536199 [details] [diff] [review]
Part 2 (v1): IDL changes
Attachment #536199 - Flags: superreview?(robert.bugzilla)
Attachment #536199 - Flags: review?(mak77)
Created attachment 536200 [details] [diff] [review]
Part 3 (v1): Implementation
Attachment #536200 - Flags: review?(mak77)
Try build for v1 patches: http://tbpl.mozilla.org/?tree=Try&rev=9e4eb518ffed
(Assignee)

Updated

6 years ago
Blocks: 660753
(Assignee)

Updated

6 years ago
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.
Created attachment 537431 [details] [diff] [review]
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.
Attachment #536198 - Attachment is obsolete: true
Attachment #537431 - Flags: review?(mak77)
Created attachment 537432 [details] [diff] [review]
Part 1 (v2): Tests

Bah, this is actually the v2 patch.
Attachment #537431 - Attachment is obsolete: true
Attachment #537432 - Flags: review?(mak77)
Attachment #537431 - Flags: review?(mak77)
(Assignee)

Updated

6 years ago
Attachment #537431 - Attachment description: Part 1 (v2): Tests → Part 1 (v1 dupe): Tests
Created attachment 537433 [details] [diff] [review]
Part 2 (v2): IDL changes

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)
Created attachment 537434 [details] [diff] [review]
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.
Attachment #536200 - Attachment is obsolete: true
Attachment #537434 - Flags: review?(mak77)
Try build on top of Places tip: http://tbpl.mozilla.org/?tree=Try&rev=f15dd783b20a
(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]
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.