Kill dynamic containers, now.

RESOLVED FIXED in mozilla11

Status

()

defect
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

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

unspecified
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [killthem])

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

8 years ago
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.
Assignee

Comment 1

8 years ago
Posted patch patch v1.0 (obsolete) — Splinter Review
Attachment #572474 - Flags: review?(dietrich)
Assignee

Comment 2

8 years ago
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)
Assignee

Comment 3

8 years ago
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+
Assignee

Comment 5

8 years ago
(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.
Assignee

Comment 6

8 years ago
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)
Assignee

Comment 7

8 years ago
These are the interfaces changes, mostly removals.
I doubt anyone ever used them, btw.
Attachment #574640 - Flags: superreview?(robert.bugzilla)
Assignee

Comment 8

8 years ago
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+
Assignee

Updated

8 years ago
Attachment #574640 - Flags: superreview?(robert.bugzilla) → superreview?(gavin.sharp)
Attachment #574640 - Flags: superreview?(gavin.sharp) → superreview+
Assignee

Updated

8 years ago
Flags: in-testsuite-
Assignee

Comment 12

8 years ago
posted about the removal to moz.dev.extensions
Keywords: dev-doc-needed
Assignee

Updated

8 years ago
Blocks: 371087
Blocks: 745410
You need to log in before you can comment on or make changes to this bug.