Open Bug 1087580 Opened 10 years ago Updated 1 year ago

eraseEverything needs a bookmarks-cleared notification

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

People

(Reporter: mak, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [snt-scrubbed][places-performance])

eraseEverything currently notifies for each removed bookmark, it would be better to notify a global bookmarks removal instead.
Priority: -- → P3
Keywords: perf
It would also be better for Sync - although it would obvs require changes in Sync to support it.

But I guess that leads to the question of why it exists? Best I can tell it's only used by tests.
(In reply to Mark Hammond [:markh] from comment #1)
> But I guess that leads to the question of why it exists? Best I can tell
> it's only used by tests.

Don't forget the part of the Places code still has to be converted to the new API, then we'll see the actual usage.
One of the cases that comes to my mind is restoring a backup, then we'll likely invoke eraseEverything before restoring.
(In reply to Marco Bonardo [::mak] from comment #2)
> Don't forget the part of the Places code still has to be converted to the
> new API, then we'll see the actual usage.

Makes sense, thanks.

> One of the cases that comes to my mind is restoring a backup, then we'll
> likely invoke eraseEverything before restoring.

Interestingly, in that use-case I doubt we'd want to handle the erase and just the restore. We can probably handle that by ensuring it's all wrapped in a single batch and have Sync only take action on completion (ie, ignore erase if it also saw a restore)
No longer depends on: placesAsyncBookmarks
See Also: → 1316348
Depends on: 1340498
Depends on: 1473530
No longer depends on: 1340498
Severity: normal → S3
Summary: eraseEverything needs an onClearBookmarks notification → eraseEverything needs a bookmarks-cleared notification

This affects bookmarks restore (from JSON or HTML) and Sync. That's all the cases where we remove all the bookmarks before proceeding with the new additions.
Fixing this will require changes to Sync to support the new notification.
We could either:

  1. only send the new notification and don't send anymore the single removals
  2. send both the new notification followed by the single removals, in the same notifications array, so consumers can handle it differently

This choice should be coordinated with Sync.

Whiteboard: [snt-scrubbed][places-performance]
You need to log in before you can comment on or make changes to this bug.