568721 - Handle onClearHistory notifications correctly

Status: RESOLVED DUPLICATE of bug 578694

(Firefox :: Sync, defect, P2)

Description

[Matt Evans [:mevans]]

Steps:

1. go to History->Show All History
2. select durations with many entries > 30
3. select all pages but one and delete them
4. sync data into cloud
5. go to other instance and sync down history changes

Expected results:

The history on both computers show the deletion of the history

Actual results:

The deletion of the pages isn't synced on the other computer

Note:

Individual page deletions, do indeed work and are synced properly

Comment 1

[Tracy Walker [:tracy]]

is this because of how bug https://bugzilla.mozilla.org/show_bug.cgi?id=518945 handles history deletion?

Comment 2

[Mike Connor [:mconnor]]

Utterly unrelated, we don't even process the items.  Pretty sure it's that we don't get the same notifications from the backend.

Comment 3

[Ayan Shah[:ashah]]

Flagging in-testsuite for further investigation

Comment 4

[Mike Connor [:mconnor]]

So... is there a reason we don't get notifications here? IIRC it was >10...

Comment 6

[Shawn Wilsher :sdwilsh]

You won't be notified about individual entries being deleted, but you should be getting start and end notifications about the batch.

Comment 7

[Marco Bonardo [:mak]]

And that is a bug, just to be clear. We are not notifying single changes during batches, but the right behavior should be that UI listeners don't listen during batches. Any other listener will have an hard time finding what changed.

Comment 9

[Marco Bonardo [:mak]]

Actually, now we notify each change, on batch you may want to avoid doing some work or accumulating changes, but you are not losing anymore history removals.
The only situation we don't notify single removals is onClearHistory, in such a case you just get the onClearHistory notification.

Comment 10

[Gregory Szorc [:gps]]

According to Marco's last comment, this shouldn't be an issue any more?

Tempted to mark WORKSFORME, but I'd like verification.

Comment 11

[Tracy Walker [:tracy]]

so, manually deleting a dozen or so history items from the Library correctly syncs.  But I if I go to Tools > Clear Recent History and select Everything, the deletes don't sync.

Comment 12

[Richard Newman [:rnewman]]

We have a history observer that does nothing for batch starts and ends, and thus should get each individual item deletion, right?

My suspicion is that Places is calling onDeleteURI during batches, and we're only processing onBeforeDeleteURI, which is not called.

mak, could you confirm?

Comment 13

[Marco Bonardo [:mak]]

if you clear everything the only notification you get is onClearHistory.

Comment 14

[Richard Newman [:rnewman]]

Sync's only action in onClearHistory is to bump the score… but it doesn't track any of the changed IDs, nor receive any notifications for individual items, so it won't upload any deletions.

We could either:

* Query for every ID during onClearHistory and track them all, and allow sync to occur as normal
* Act as if we're generating a reset event for the history collection, and tell other clients to wipe themselves.

Which we do depends on whether we think of clearing history as something that should affect other clients' history that hasn't yet synced, or something that should only affect the records on this machine. (And efficiency, too; I'm not super excited about POSTing up 80,000 deleted records.)

Comment 15

[Richard Newman [:rnewman]]

Tidying up some metadata…

Bug 578694 describes my second course of action from Comment 14, so duping to that.