1072833 - Livemark containers should be represent as query nodes rather than folder nodes

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P3)

Description

[Asaf Romano (gone)]

Livemarks are semantically queries: their contents are live, temporary and read-only to the user. However, they're internally treated as folders, having the read-only annotation. Read-only folders being roughly the same as queries, the current setup works generally fine. However, the read-only concept is about to be removed (bug 1068671), and that forces us to introduce some fresh hacks to both queries code and UI code. Those being (at least): (1) keeping excludeReadOnlyFolders having the semantics of something like excludeLivemarks); (2) Adding a synchronous getItemAnnotation call to isCommandEnabled for container nodes.

If we find a way to make livemark nodes show up as queries, matching their semantics, we can avoid these hacks. Moreover, some of the ui glitches we have with livemarks (e.g. that they initially show up with the folder icon).

Comment 1

[Marco Bonardo [:mak]]

While brainstorming over this, I had this insane idea of a query like place:feed=feeduri&uri=siteuri that would also completely remove the annotations need and the hacks we use around to understand if a node is a livemark.
Hard to tell how hard it would be to support such a change in Sync. Likely we could remove the old "folder" and insert a new "uri" record, though from that point on, no idea what happens :\

Comment 2

[Marco Bonardo [:mak]]

Kit, any quick thoughts about how hard would it be such a change on the Sync side?

Comment 3

[Lina Butler [:lina]]

Fairly easy, I think! We sync livemarks as their own record type, so we would only need to change `PlacesSyncUtils`. The record format would remain the same: `{ type: "livemark", feedUri: "...", siteUri: "...", title: "..." }`.

Old versions of Firefox would store the livemark internally as a folder; new versions would store it as a query. Assuming the site and feed URIs stay the same, we wouldn't need to re-sync the livemark when we remove the folder and replace it with a query.

We'd need to change https://searchfox.org/mozilla-central/rev/c9214daa09e3eb8246b1448e469df1652a140825/toolkit/components/places/PlacesSyncUtils.jsm#1407 to distinguish between livemark queries and other queries (which we sync as `type: "query"`), but this is all backward-compatible.

Comment 4

[Marco Bonardo [:mak]]

I made a wip patch during the week end, and it sounds like it would be a good change. Though, there is a lot of code to touch before it could be ready, so I cannot give a strict ETA, but I'll bring it on as a side project and I did a good chunk of work already.

The advantages:
- not having to manage special read-only folders (the whole concept may disappear if we fix this and bug 1310295)
- it's trivial to identify a livemark node in views, currently we have to roundtrip to the livemarks service that is async
- it removes 2 annotations. Along with the other changes like bug 1402890, we'd be left with very few annotations cases, that may allow us to more easily move them to a new async API.
- it largely simplifies the annotations handling code around, reducing I/O and views overhead

Comment 5

[Marco Bonardo [:mak]]

Unassigning until we know the future of livemarks.

Comment 6

[Marco Bonardo [:mak]]

Wontfixing because live bookmarks are going away - See Bug 1477667.