Closed Bug 700296 Opened 9 years ago Closed 9 years ago

Kill dynamic containers, now.

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [killthem])

Attachments

(3 files, 1 obsolete file)

They never worked, we tried efforts to rewrite them but never found the needed resources. They add complication and unused code paths, so I prefer just to kill them now, also because in the new schema I want to avoid their additional weight in the bookmarks table, and regardless, we need to make a new async bookmarks interface, complication may be an issue.

I have a slightly alternative interface-less idea, where bookmarks service keeps a temp table and you can just put your uri nodes there and we automatically pick them up when the folder is opened, there should just be a way for the external provider to notify the folder node to update its children, shouldn't be hard.
Btw, whatever we do it's better to restart from scratch here.

PS: plan to do this removal in the FF11 timeframe.
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #572474 - Flags: review?(dietrich)
Comment on attachment 572474 [details] [diff] [review]
patch v1.0

From what I remember Sync just ignores dynamic containers, I am removing the management here (see the services/ change) and I can't find further management...
Flagging rnewman to ensure I'm not missing some requirement on sync side.
Attachment #572474 - Flags: review?(rnewman)
note to self: remember to update the schema version in head_common.js!
Comment on attachment 572474 [details] [diff] [review]
patch v1.0

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

r=me, looks good. please over-communicate this to Jorge on add-ons team.
Attachment #572474 - Flags: review?(dietrich) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #4)

> r=me, looks good. please over-communicate this to Jorge on add-ons team.

I'll notify in addons newgroup, btw considered they NEVER worked and we never fixed them, I'd be really surprised to find someone using them. And if they do, they are likely breaking our views.
Attached patch Sync patch v1.0Splinter Review
Splitting the path to easy review.

Richard, any ETA on this review? it's blocking next refactorings.
Attachment #572474 - Attachment is obsolete: true
Attachment #572474 - Flags: review?(rnewman)
Attachment #574637 - Flags: review?(rnewman)
These are the interfaces changes, mostly removals.
I doubt anyone ever used them, btw.
Attachment #574640 - Flags: superreview?(robert.bugzilla)
This is the already reviewed part.
Comment on attachment 574637 [details] [diff] [review]
Sync patch v1.0

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

This looks safe to me.
Attachment #574637 - Flags: review?(rnewman) → review+
Attachment #574640 - Flags: superreview?(robert.bugzilla) → superreview?(gavin.sharp)
Attachment #574640 - Flags: superreview?(gavin.sharp) → superreview+
Flags: in-testsuite-
posted about the removal to moz.dev.extensions
Keywords: dev-doc-needed
Blocks: 371087
Blocks: 745410
You need to log in before you can comment on or make changes to this bug.