826421 - Remove Places onBeforeDeleteURI and onBeforeItemRemoved notifications in Sync

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Marco Bonardo [:mak]]

ah and we notify one between onDeleteVisits or onDeleteURI, not both (so you won't get deleteVisits and then deleteURI).

Comment 3

[Richard Newman [:rnewman]]

(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 :)

Comment 4

[Marco Bonardo [:mak]]

(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.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

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.

Comment 6

[Richard Newman [:rnewman]]

Try is green.

Comment 7

[Richard Newman [:rnewman]]

And confirmed that this change fixes Bug 601237.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Richard Newman [:rnewman]]

> 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!

Comment 10

[Richard Newman [:rnewman]]

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.

Comment 11

[Marco Bonardo [:mak]]

places branch is dead from a long time, don't worry :)

Comment 12

[Richard Newman [:rnewman]]

So Greg can use s-c for FHR:

https://hg.mozilla.org/integration/mozilla-inbound/rev/012ce2bf7607

Comment 13

[Marco Bonardo [:mak]]

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.

Comment 14

[Richard Newman [:rnewman]]

(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.

Comment 15

[Richard Newman [:rnewman]]

Follow-up:

https://hg.mozilla.org/services/services-central/rev/712bd4b20e47

Comment 16

[Marco Bonardo [:mak]]

(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.

Comment 17

[Richard Newman [:rnewman]]

I filed Bug 826969; there might be others.

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/012ce2bf7607

Comment 19

[Tracy Walker [:tracy]]

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?

Comment 20

[Richard Newman [:rnewman]]

(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.