Remove onBeforeDeleteURI and onBeforeItemRemoved notifications

RESOLVED FIXED in mozilla21

Status

()

Toolkit
Places
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla21
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
These notifications were added originally to allow Sync to collect information before a place is gone, but currently all of the information is provided already in onDeleteURI and onItemRemoved, so the Before notifications may likely go.

Though, the actual reason for the removal is that with off main-thread removals they complicate things too much, since we must collect places to remove off main-thread, notify onBefore on main-thread, proceed with removal off main-thread, notify removal on main-thread.
This adds lot of complication to properly synchronize all of the runnables, once we enter the "other" thread, we'd like to stay there until we're done.

We should verify if Sync and Places code can live without onBefore notifications, deprecate them, and finally remove them.
Depends on: 826421
This is ready for you, mak!
Whiteboard: [sync:bookmarks][sync:history]
(Assignee)

Updated

4 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Blocks: 834457
(Assignee)

Updated

4 years ago
Summary: Deprecate onBeforeDeleteURI and onBeforeItemRemoved notifications → Remove onBeforeDeleteURI and onBeforeItemRemoved notifications
(Assignee)

Comment 2

4 years ago
Created attachment 710373 [details] [diff] [review]
patch v1.0

it's a big patch, but basically just removals.

I'm now going to file a bug for Seamonkey, but it's not blocking since even if js consumers have these methods defined nothing bad happens.
Attachment #710373 - Flags: review?(mano)
(Assignee)

Updated

4 years ago
Blocks: 838333
Comment on attachment 710373 [details] [diff] [review]
patch v1.0

This one was easy.
Attachment #710373 - Flags: review?(mano) → review+
(Assignee)

Comment 4

4 years ago
Comment on attachment 710373 [details] [diff] [review]
patch v1.0

Asking SR for the removal of the 2 notifications
Attachment #710373 - Flags: superreview?(gavin.sharp)
(Assignee)

Updated

4 years ago
Keywords: addon-compat
CCing some of the authors of the potentially affected authors I could find:

https://mxr.mozilla.org/addons/source/242697/chrome/content/main.js#233
(https://addons.mozilla.org/en-US/firefox/addon/viewmarks/)
https://mxr.mozilla.org/addons/source/410818/chrome/content/bookmarks.js#21
(https://addons.mozilla.org/en-US/firefox/addon/booky/)
https://mxr.mozilla.org/addons/source/399944/chrome/content/browserOverlay.js#457
(https://addons.mozilla.org/en-US/firefox/addon/boom/)
https://mxr.mozilla.org/addons/source/392659/chrome/content/sortBookmarks.js#135
(https://addons.mozilla.org/en-US/firefox/addon/auto-sort-bookmarks/)
https://mxr.mozilla.org/addons/source/363782/chrome/content/tabdock-bookmarks.js#26
(https://addons.mozilla.org/en-US/firefox/addon/fdock/)
https://mxr.mozilla.org/addons/source/2499/modules/liveplaces.jsm#1375
(https://addons.mozilla.org/en-US/firefox/addon/liveclick/)
https://mxr.mozilla.org/addons/source/4578/modules/Storage.jsm#1496
(https://addons.mozilla.org/en-US/firefox/addon/brief/)
https://mxr.mozilla.org/addons/source/9071/chrome/content/sync/log.js#168
(https://addons.mozilla.org/en-US/firefox/addon/categorize/)
Attachment #710373 - Flags: superreview?(gavin.sharp) → superreview+
(Assignee)

Comment 6

4 years ago
Thanks, this has also been notified through mozilla.dev.extensions, on planet firefox and planet mozilla.
I also brefly discussed (right now) with TheOne who had an add-on using this notification and looks like he doesn't really need it, onItemRemoved covers most cases we thought about.
(Assignee)

Comment 7

4 years ago
for authors, notice onItemRemoved you get the parentId, this is valid and you can use it without any problem (items are notified before their containing folders), for example to check if an item is a tag.
onItemRemoved has also many new information compared to what it was when the Before version was introduced.
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 8

4 years ago
fixed a minor typo in test_placesTxn and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e913428a6628
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/e913428a6628
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.