Closed Bug 826421 Opened 7 years ago Closed 7 years ago

Remove Places onBeforeDeleteURI and onBeforeItemRemoved notifications in Sync

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla20

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Whiteboard: [sync:bookmarks][sync:history][sync:perf])

Attachments

(1 file)

We already fixed this for removal of bookmarks (albeit with incorrect logging):

  onItemRemoved: function BMT_onItemRemoved(itemId, parentId, index, type, uri,
                                            guid, parentGuid) {
    if (this._ignore(itemId, parentId, guid))
      return;

    this._log.trace("onBeforeItemRemoved: " + itemId);
  …
  onBeforeItemRemoved: function () {},


We don't correctly handle history deletion, in that we don't do anything when we receive these events:

  onDeleteVisits: function () {},
  onDeleteURI: function () {},


which I guess is the cause of Bug 601237!

This should be a nice trivial bug to fix.
just to clarify, onDeleteVisits is fired when some visits of a page have been removed, but the page is still in the database (cause it's a bookmark or has other visits). Actually onDeleteVisits sucks quite a bit since doesn't give enough info at all, the only meaningful info is that if the reported visit time is zero all visits have been removed.
ah and we notify one between onDeleteVisits or onDeleteURI, not both (so you won't get deleteVisits and then deleteURI).
(In reply to Marco Bonardo [:mak] from comment #1)
> just to clarify, onDeleteVisits is fired when some visits of a page have
> been removed, but the page is still in the database (cause it's a bookmark
> or has other visits). Actually onDeleteVisits sucks quite a bit since
> doesn't give enough info at all, the only meaningful info is that if the
> reported visit time is zero all visits have been removed.

We should really handle both of those as "track this record". The rest of Sync's code will do the right thing to upload changes. If onDeleteVisits doesn't provide the GUID, then we're screwed, of course :)
(In reply to Richard Newman [:rnewman] from comment #3)
> If onDeleteVisits
> doesn't provide the GUID, then we're screwed, of course :)

Looks like you're lucky today.
https://tbpl.mozilla.org/?tree=Try&rev=80a9839844a9

Apparently the observer interface has sprouted some new methods ("batching"), so I added those to avoid scary log output.

This seems to work. The stubs can be deleted as part of Bug 826409, when they're no longer called -- I left them in here to avoid yet more scary log output about missing methods.
Attachment #697693 - Flags: review?(mak77)
Try is green.
And confirmed that this change fixes Bug 601237.
Comment on attachment 697693 [details] [diff] [review]
Proposed patch. v1

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

Thanks, this looks good!

::: services/sync/modules/engines/history.js
@@ +396,4 @@
>    },
>  
> +  onDeleteVisits: function (uri, visitTime, guid, reason) {
> +    this.onDeleteAffectsGUID(uri, guid, reason, "onDeleteVisits", SCORE_INCREMENT_SMALL);

As I said, onDeleteVisits indicates that "some" visits have been removed.
Though, if visitTime === 0, then ALL of the visits have been removed, and the page is surviving in the database just cause it's a bookmark.
If this will just trigger a new sync of this entry fine, otherwise you may want to handle only visitTime === 0 notifications
Attachment #697693 - Flags: review?(mak77) → review+
> As I said, onDeleteVisits indicates that "some" visits have been removed.
> Though, if visitTime === 0, then ALL of the visits have been removed, and
> the page is surviving in the database just cause it's a bookmark.
> If this will just trigger a new sync of this entry fine, otherwise you may
> want to handle only visitTime === 0 notifications

Sync uploads visits, so it cares if some have been removed, as well as total deletion; we could simplify this whole listener setup as "this GUID changed" :).

Thanks for the speedy review!
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
https://hg.mozilla.org/services/services-central/rev/f1df8615c8f9

Feel free to pull that commit into inbound or places if you need it, mak. Otherwise I'll merge again this week.
Whiteboard: [sync:bookmarks][sync:history][sync:perf] → [sync:bookmarks][sync:history][sync:perf][fixed in services][qa+]
places branch is dead from a long time, don't worry :)
So Greg can use s-c for FHR:

https://hg.mozilla.org/integration/mozilla-inbound/rev/012ce2bf7607
Target Milestone: --- → mozilla20
actually I missed this, batching is part of nsINavHistoryResultObserver, while you implement nsINavHistoryObserver, so you should not need it.  Minor problem though, just in case you want to clean it up.
(In reply to Marco Bonardo [:mak] from comment #13)
> actually I missed this, batching is part of nsINavHistoryResultObserver,
> while you implement nsINavHistoryObserver, so you should not need it.  Minor
> problem though, just in case you want to clean it up.

Ah, so the logging I saw was from something else. Perhaps crank up console logging and take a look? I'll land a cleanup patch on s-c.
(In reply to Richard Newman [:rnewman] from comment #14)
> Ah, so the logging I saw was from something else. Perhaps crank up console
> logging and take a look? I'll land a cleanup patch on s-c.

it's likely, for example the new downloads view was lacking it and I just added it in inbound... if you still see that warning please file a bug and we'll check all implementations of nsINavHistoryResultObserver to find the lacking one.
I filed Bug 826969; there might be others.
https://hg.mozilla.org/mozilla-central/rev/012ce2bf7607
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sync:bookmarks][sync:history][sync:perf][fixed in services][qa+] → [sync:bookmarks][sync:history][sync:perf][qa+]
tricky one to verify with instant sync on history delete in action.  You have to select two history items and delete both with organize > Delete.

Will this land on mobile soon? Or does it require a new bug to be fixed here?
Status: RESOLVED → VERIFIED
Whiteboard: [sync:bookmarks][sync:history][sync:perf][qa+] → [sync:bookmarks][sync:history][sync:perf]
(In reply to Tracy Walker [:tracy] from comment #19)

> Will this land on mobile soon? Or does it require a new bug to be fixed here?

Mobile doesn't use this code.
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.