700296 - Kill dynamic containers, now.

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

note to self: remember to update the schema version in head_common.js!

Comment 4

[Dietrich Ayala (:dietrich)]

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.

Comment 5

[Marco Bonardo [:mak]]

(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.

Comment 6

[Marco Bonardo [:mak]]

Attachment: Sync patch v1.0

Splitting the path to easy review.

Richard, any ETA on this review? it's blocking next refactorings.

Comment 7

[Marco Bonardo [:mak]]

Attachment: Interfaces patch v1.0

These are the interfaces changes, mostly removals.
I doubt anyone ever used them, btw.

Comment 8

[Marco Bonardo [:mak]]

Attachment: Places patch v1.0

This is the already reviewed part.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 574637 [details] [diff] [review]
Sync patch v1.0

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

This looks safe to me.

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/177eedec0f11
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be8546cdee2
https://hg.mozilla.org/integration/mozilla-inbound/rev/457b6bafbf3a

I will post about the change in m.d.extensions, as soon as I'm sure it sticks.

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/177eedec0f11
https://hg.mozilla.org/mozilla-central/rev/4be8546cdee2
https://hg.mozilla.org/mozilla-central/rev/457b6bafbf3a

Comment 12

[Marco Bonardo [:mak]]

posted about the removal to moz.dev.extensions