424160 - [meta] tags should not be bookmarks folders, move them to a separate table

Status: NEW

(Toolkit :: Places, task, P3)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i think we should asap move tags to a separate table. Ideally (imo) that should be  like this:

    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
        "CREATE TABLE moz_tags ("
        "id INTEGER PRIMARY KEY AUTOINCREMENT, "
        "tag TEXT UNIQUE, "
        "lastModified INTEGER)"));
    NS_ENSURE_SUCCESS(rv, rv);

    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
        "CREATE TABLE moz_tags_relation ("
        "tag_id INTEGER, "
        "item_id INTEGER)"));
    NS_ENSURE_SUCCESS(rv, rv);

why this, actual system cons:

- tags as folders are duplicating data into bookmarks table and that is not good, a good db impl should always avoid duplicating data

- data duplication can bring to problems, since you have to sync things

- data duplication makes table bigger, thus slowing down ALL queries and indexes onto that table. So you have 100 bookmarks, a user set up 10 tags per bookmark, you end up with a 1100 records table, without having so much useful data added. all those new records has to be skipped or searched by bookmarks queries

- actually we don't know which bookmark has generated a tag, our tags are url-centric, and when we modify a tagged bookmark we don't know what to sync and what to show

- tag queries are generally slower, if you want to generate a tag cloud you end up with some complex and slow query

as a pro the current system has that we can use bookmark functions to manage folders and roots

The new system would require specialized methods, they could be in nsNavBookmarks similar to keyword methods, so GetTagForURI, SetTagForURI, RemoveTagFromURI (or for  Item), or we could refactor TagsService to use queries and manage them by itself.
Cleanup could be done by triggers and expiration.
Migration is quite easy (i already have code to migrate old tags)

Changing from old to new system should not be too complex, that is because mainly where we use tags we use queries, so querying one table or the other should not make big differences. 
You can easily (and fast) get list of tags, number of bookmarks for a tag, bookmark ids for a tag, tags for a bookmark id. You don't have dupes, you don't duplicate data, you extract a list of REAL bookmark ids that you can edit in the UI and changes are system-wide, you're changing the real bookmark.

So even if this is a rewrite that should be done for 3.1, i fear that when 3.0 will be ready users will start using tags (and creating extensions on tags) and our migration work will be more difficult and complex (a regression there could cause dataloss in a release product).

Changing this could be done with some effort for RC1, being protected by the beta status, and that would:
- create a more solid base for future tag changes / extensions
- would make bookmark queries faster (when user will add more tags)
- would make tag queries faster
- would help solving problems with empty or wrong titles (bug 415113, 
- could help mitigating loops problems (bug 400448)
- would solve conflicting names (unique index on tag name)(bug 412655)
- setting/editing tags for a bunch of bookmarks would become changing some index in a simple relation table (bug 412002)
- autocomplete could be faster when querying tags (bug 415960)
- GROUP_CONCAT in awesomebar could be faster
- could help solving duplicated items (bug 402799)
- drag&drop into a tag becomes setting an index into a table
- implementing count would be fast (bug 399260)

So, ideas about this?

First answer would probably be "no, we are too late for taking this change", but that would require adding some method to nsNavBookmarks, revisiting tagginservice and some query. Biggest problem could be frontend moving between tags, but since we already have problems and we are moving to use queries for all tag containers in the frontend...

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

forgot, blocker bug 419731 could disappear if we are going to use real bookmarks instead of copies..

Comment 2

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, this is a first possible solution that can be extended to multiple tags. Unfortunately there's no clean way to bind an IN construct, and doing that manually could introduce SQL injection. so maybe better to use a for loop and manually add "where "
loop
  "tag = ? OR "
and then bind in another loop.

SELECT id, url FROM moz_places h
JOIN tags_relation r ON place_id = h.id
JOIN tags t ON t.tag_id = r.tag_id
WHERE tag IN ("one", "two")
GROUP BY url
HAVING count(*) = 2

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

please also note I'm adding a Sqlite.wrapStorageConnection in bug 1068620.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Hi althaf, do you have any news?

Comment 6

[0RUBn0]

Anyway I can help?
I'm making an addon that's stuck on this "bug". So I'm prepared to help.
I'm just a beginner at ECMA 6 and Firefox code, but I can code a lot and let someone review it.
Never helped source code of firefox itself though, so I would need some link on how to help (and some status of where you guys are at in this issue.)

Comment 7

[Althaf Hameez [:ahameez]]

Hey mak.
Sorry for the really late reply, and general disappearance, got just caught up in other things.
If ORUBn0 wants to take over, feel free to do so.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Althaf Hameez [:ahameez] from comment #7)
> Sorry for the really late reply, and general disappearance, got just caught
> up in other things.

I figured that :)

(In reply to 0RUBn0 from comment #6)
> Never helped source code of firefox itself though, so I would need some link
> on how to help (and some status of where you guys are at in this issue.)

you can likely start from https://developer.mozilla.org/en-US/docs/Introduction have a build system setup and ready, understand how to import and modify patches.

Then you could maybe start with bug 653816, where we can change and deprecate the  GetBookmarkIdsForURI method.
Then Althaf's patches in bug 1039200 should be unbitrotted, the pending feedback comments should be applied. From there on the plan is explained in  https://bugzilla.mozilla.org/show_bug.cgi?id=1039200#c21

Comment 9

[0RUBn0]

After some reading and addon-developing, including some native source diving, I think my expertise is much too low to help you with this, especially as it looks that this has all to be done in one go, seeing how often and quick bookmarks/tags code change in the current system already...
I'll think about it when I could find some time. I'm a teacher, so my "holidays" are over and it could take a year or more. Just saying. 
Sorry, because I would really have helped.

Comment 10

[David Lawrence [:dkl]]

Changing qe-verify? to qe-verify+.