1039200 - Add Migration for new tag tables

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Althaf Hameez [:ahameez]]

From mak on IRC

>  we could land a patch that introduces the new tags table and migrate tags 
>  it should also keep the table contents on par. On the next version of  
>  Firefox we might drop tags from the old table... this is to allow users to 
>  downgrade  without losing tags

>so far a very simple relational tags schema
>one table with "tag", one table with the relation
>"id, tag" and "tagid, place_id"
>I think the other time with Mano we discussed about current limitation of 
>tags to link with bookmarks and we'd like to go back linking with places 
>instead
>since with foreign_count we don't need anymore an entry in moz_bookmarks to 
>not throw away a page

Comment 1

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_migration_new_tags.diff part 1 v1

First part of the patch which involves creating the new tables and migrating the old data.

I'm not too happy with the way I'm testing whether the data has been migrated to moz_tags_relation. I just kept it in to show that it works, but I'd prefer any ideas on how to make it more reliable than having hard coded places id.

Comment 2

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_migration_new_tags.diff part 1 v1

Woops forgot to add the sqlite file.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8459411 [details] [diff] [review]
bug1039200_migration_new_tags.diff part 1 v1

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

I think the test might be fine so far, you might keep an array of results and walk it instead of repeating the same code inline, but that's not that much important.

::: toolkit/components/places/Database.cpp
@@ +1984,5 @@
> +  }
> +
> +  // Create moz_tags_relation table
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT * FROM moz_tags_relation"

You can use TableExists for these 2.

@@ +1992,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  // Migrate old tags to new tags database
> +  // First get id and title of tags by checking moz_bookmarks with parent = 4

Unfortunately we can't rely on tags folder being 4, we must join with moz_bookmarks_roots and get the tags root folder id properly.

Moreover, update code must be able to run multiple times even in future, so here you must ensure this code would be able to run (and do nothing) even after we will have removed tags from moz_bookmarks. We might remove tags from moz_bookmarks_roots at a certain point as well as remove the tags root completely.

@@ +1996,5 @@
> +  // First get id and title of tags by checking moz_bookmarks with parent = 4
> +  // For each obtained id find the corresponding row in moz_bookmarks having the id
> +  // as it's parent. The fk of this row is the corresponding places_id
> +
> +  //Select bookmark id and title of tags from moz_bookmarks

missing space after //

@@ +1998,5 @@
> +  // as it's parent. The fk of this row is the corresponding places_id
> +
> +  //Select bookmark id and title of tags from moz_bookmarks
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT id, title FROM moz_bookmarks WHERE parent = 4"

I think you might simplify this code quite  bit.

First let's see the schema

moz_tags is ok.

I'd change moz_tags_relation to:

CREATE TABLE moz_tags_relation (
  tag_id INTEGER,
  place_id INTEGER,
  PRIMARY KEY (tag_id, place_id)
) WITHOUT ROWID

Unfortunately we also need the reverse index to get all tags for a page:

CREATE INDEX moz_tags_relation_placetagindex ON moz_tags_relation(place_id, tag_id)

we are not going to use foreign_keys cause they are per connection and Places has
foreign consumers that are unlikly to use them, so their protection would not
help us. I'd go for static (non-temp) triggers:
if a tag is removed, all entries for that tag in moz_tags_relation should be removed
if a page is removed, all entries for that page in moz_tags_relation should be removed

Then, to copy over tags:

INSERT INTO moz_tags (tag)
SELECT title FROM moz_bookmarks WHERE parent = (SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags') AND type = 2

and finally to copy over the relations

INSERT INTO moz_tags_relation (tag_id, place_id)
SELECT (SELECT id FROM moz_tags WHERE tag = t.title), b.fk
FROM moz_bookmarks b
JOIN moz_bookmarks t ON t.id = b.parent AND t.parent = (SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags')

Another problem you will shortly hit, is how to handle foreign_count. we want to increase/decrease it with new tags as well, but we must ensure when an old tag is removed we always also remove the new version of it, otherwise foreign_count will prevent the page from being expired.

There are edge cases but I don't think we can handle them until we move onto a new tagging API using only the new table.

Comment 4

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_migration_new_tags.diff part 1 v2

Here is the updated version of the patch.
I've left a block comment in there to remind me to fix what I am about to ask now.
> even after we will have removed tags from moz_bookmarks.
> We might remove tags from moz_bookmarks_roots at a certain point as well as 
> remove the tags root completely.

Should I just check if tags exists in moz_bookmark_roots before continuing with the rest of the migration? Or am I missing something else here.

------------------------------------------------------------------------------
Other questions on nsTaggingService.js

1) How do i deal with identical tags with different casings. e.g. (test, Test tEst) - currently we update to the latest one. Therefore if all 3 were entered only tEst would be stored.

2) Is there a helper method to get the id of a URI from moz_places?

3) Im thinking that the new API for now should have the following:

_createTag(aTagName)
_getTagIdForTag(aTagName) 
_removeTag(aTagName) 

tagURI(aURI, aTags) 
untagURI(aURI, aTags)
getURIsForTag(aTagName) 
getTagsForURI(aURI, aCount)
get allTags
get hasTags


Thanks

Comment 5

[Marco Bonardo [:mak]]

(In reply to Althaf Hameez [:ahameez] from comment #4)
> > even after we will have removed tags from moz_bookmarks.
> > We might remove tags from moz_bookmarks_roots at a certain point as well as 
> > remove the tags root completely.
> 
> Should I just check if tags exists in moz_bookmark_roots before continuing
> with the rest of the migration? Or am I missing something else here.

Rethinking about that, I think we should not care about that here, it's something for the future bug when that will actually happen. Sorry for confusion.

> Other questions on nsTaggingService.js
> 
> 1) How do i deal with identical tags with different casings. e.g. (test,
> Test tEst) - currently we update to the latest one. Therefore if all 3 were
> entered only tEst would be stored.

we should keep the current behavior, so update casing to the last used one.

> 2) Is there a helper method to get the id of a URI from moz_places?

it depends what you need it for: a test? internal code? a query? off-hand I don't think, we are trying to remove any references to database "ids" in the external API and moving towards GUIDs.

> 3) Im thinking that the new API for now should have the following:
> 
> _createTag(aTagName)
> _getTagIdForTag(aTagName) 
> _removeTag(aTagName) 
> 
> tagURI(aURI, aTags) 
> untagURI(aURI, aTags)

I think should just be:

{promise} tag(aURI, aTags);
{promise} unTag(aURI, aTags);

we want to (and must) provide a new API, the names you suggested are the same as the old API. IT would be very confusing if we'd blindly convert the old sync api to be async, we could introduce lots of subtle bugs and break add-ons. Better to introduce a parallel API and deprecate the old one.

> getURIsForTag(aTagName) 
> getTagsForURI(aURI, aCount)
> get allTags
> get hasTags

{promise} taggedURIs(aTags)
returns uris that have all of the provided tags
{promise} tags([optional] aURI)
returns tags for a given uri in alphabetic order, if no uri is provided returns list of all tags in alphabetic order

let's not provide an hasTags, there is a single use and it's not worth it, we will provide an internal alternative to that (a bug should be filed for that please)

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8466750 [details] [diff] [review]
bug1039200_migration_new_tags.diff part 1 v2

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

I'd say it looks good so far
still need to hook up code to keep the new tables up-to-date...

the new API might even go to a follow-up bug.

::: toolkit/components/places/Database.cpp
@@ +2007,5 @@
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_tags (tag) "
> +    "SELECT title FROM moz_bookmarks WHERE parent = ( "
> +    "SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags') "
> +    "AND type = 2"));

indent the inner query please

@@ +2010,5 @@
> +    "SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags') "
> +    "AND type = 2"));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  //Create the new relations for the tags

whitespace after //

@@ +2013,5 @@
> +
> +  //Create the new relations for the tags
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_tags_relation (tag_id, place_id) "
> +    "SELECT (SELECT id FROM moz_tags WHERE tag = t.title), b.fk FROM moz_bookmarks b "

FROM moz_bookmarks on a new line

@@ +2015,5 @@
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_tags_relation (tag_id, place_id) "
> +    "SELECT (SELECT id FROM moz_tags WHERE tag = t.title), b.fk FROM moz_bookmarks b "
> +    "JOIN moz_bookmarks t ON t.id = b.parent AND t.parent = ( "
> +    "SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags')"));

indent more this subquery

@@ +2018,5 @@
> +    "JOIN moz_bookmarks t ON t.id = b.parent AND t.parent = ( "
> +    "SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags')"));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  //Create the reverse index for moz_tags_relation

whitespace after //

Comment 7

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_migration_new_tags.diff part1 v3

Updated per comments

Comment 8

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_updating_nsTaggingService.diff part2 v1

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8494533 [details] [diff] [review]
bug1039200_migration_new_tags.diff part1 v3

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

::: toolkit/components/places/Database.cpp
@@ +1997,5 @@
> +  }
> +
> +  // Migrate old tags to new tags database
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_tags (tag) "

just in case, please use INSERT OR IGNORE,since these can re-run on downgrade+upgrade

@@ +2005,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Create the new relations for the tags
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_tags_relation (tag_id, place_id) "

ditto

::: toolkit/components/places/nsPlacesIndexes.h
@@ +126,5 @@
> + */
> +
> +#define CREATE_IDX_MOZ_TAGS_RELATION_PLACETAG \
> +  CREATE_PLACES_IDX( \
> +    "placetagindex", "moz_tags_relation", "place_id, tag_id", "UNIQUE" \

I think we don't need to define this unique, cause we already have a primary key on tag_id, place_id that should ensure unicity also for the reverse.

::: toolkit/components/places/tests/migration/test_current_from_v24.js
@@ +31,5 @@
> +  yield OS.File.copy("places_v24_old_tags.sqlite", placesPath);
> +
> +  let conn = yield PlacesUtils.promiseDBConnection();
> +  dump_table("moz_tags");
> +  dump_table("moz_tags_relation");

did you intend to leave these or is it just debug leftover?

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8494534 [details] [diff] [review]
bug1039200_updating_nsTaggingService.diff part2 v1

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

So, I think we will change the implementation a little bit here, similarly to what we are doing for bookmarks or history, so provide a Tagging.jsm module, put the new code and its documentation there (no changes to nsITaggingService) and proxify PlacesUtils.tagging

For details please check bug 1068007 and bug 1069973.
In this case should just be matter of moving the new code to the module and use the module in nsTaggingService.js

I'm sorry if this requires a bit more changes than expected, please let me know for any issue.

::: toolkit/components/places/nsITaggingService.idl
@@ +30,5 @@
>     *        array can be either a tag name (non-empty string) or a concrete
>     *        itemId of a tag container.
>     */
>    void tagURI(in nsIURI aURI, in nsIVariant aTags);
> +  jsval tag(in nsIURI aURI, in nsIVariant aTags);

tag should allow both a url string or an nsIURI, in the former case we should still check it through .newURI

@@ +44,5 @@
>     *        url.  Each element within the array can be either a tag name
>     *        (non-empty string) or a concrete itemId of a tag container.
>     */
>    void untagURI(in nsIURI aURI, in nsIVariant aTags);
> +  jsval untag(in nsIURI aURI, in nsIVariant aTags);

ditto

@@ +56,5 @@
>     */
>    nsIVariant getURIsForTag(in AString aTag);
>  
>    /**
> +   * Retrieves all URLs tagged with the given tag(s)

with (all of) the given tag(s)

@@ +81,5 @@
> +  * @param [optional] aURI
> +  *         a URL
> +  * @returns array of tags
> +  */
> +  jsval tags([optional] in nsIURI aURI);

I fear the difference between tag() and tags() is too suble... 

let's revert back to tagsForURI name, and as well it should allow both url string or nsIURI

we will use .tags for a list of all of the tags in future.

::: toolkit/components/places/nsTaggingService.js
@@ +62,5 @@
> +                        .DBConnection;
> +    let conn = yield Sqlite.wrapStorageConnection({ connection: db });
> +    let stmt = "INSERT INTO moz_tags (tag) VALUES (:tag)";
> +    yield conn.executeCached(stmt, { tag: aTagName });
> +    yield conn.close();

In the module you should define the wrapper only once, something like this, in the global, should work:

let db = null;
function* promiseDB() {
  if (db)
    return db;
  db = yield Sqlite.wrapStorageConnection(PlacesUtils.history.DBConnection);
  try {
    Sqlite.shutdown.addBlocker("Places Tagging.jsm wrapper closing",
                               db.close.bind(db));
  } catch (ex) {
    // It's too late to block shutdown, just close the connection.
    yield db.close();
  }
}

@@ +96,5 @@
> +    let conn = yield Sqlite.wrapStorageConnection({ connection: db });
> +    let stmt = "INSERT OR IGNORE INTO moz_places (url, rev_host, hidden, frecency, guid) " +
> +               "VALUES (:page_url, :rev_host, :hidden, :frecency, GENERATE_GUID()) ";
> +    let rev_url = aURI.host.split("").reverse().join("");
> +    rev_url += '.';

you can inline the + '.' above.

@@ +98,5 @@
> +               "VALUES (:page_url, :rev_host, :hidden, :frecency, GENERATE_GUID()) ";
> +    let rev_url = aURI.host.split("").reverse().join("");
> +    rev_url += '.';
> +
> +    let params = { page_url: aURI.spec, rev_host: rev_url, hidden: 1, frecency: -1 };

just in case, if the scheme is 'place', frecency should be set to 0.

@@ +194,5 @@
> +  tag: Task.async(function* TS_tag(aURI, aTags)
> +  {
> +      if (!aURI || !aTags || !Array.isArray(aTags)) {
> +        throw Cr.NS_ERROR_INVALID_ARG;
> +      }

So, I know I said differently, but I think for input we should throw synchronously.

So tag is a normal function and after checking input validity, you can return Task.spawn.

for input you should also check the array doesn't contain empty tags, or stuff that is not strings. that if it's not an array it's a string. that aURI is either a string or instanceof Ci.nsIURI

@@ +203,5 @@
> +      let conn = yield Sqlite.wrapStorageConnection({ connection: db });
> +      yield this._insertURIIntoPlaces(aURI);
> +      let stmt = "INSERT OR IGNORE INTO moz_tags_relation (tag_id, place_id) VALUES(" +
> +                   "(SELECT id FROM moz_tags WHERE tag = :tag), " +
> +                   "(SELECT id FROM moz_places WHERE url = :url)) ";

I don't think it's worth optimizing this by defining the string our of the loop

@@ +213,5 @@
> +          yield this._ncreateTag(tag);
> +        } else {
> +          let tagName = rows[0].getResultByName("tag");
> +          if (tagName != tag){ // if tags don't match update db tag to match newer casing
> +            yield this._updateTag(tagName, tag);

I wonder if INSERT OR REPLACE would do all of this for you...

@@ +300,5 @@
> +  untag: Task.async(function* TS_untag(aURI, aTags){
> +
> +    if (!aURI || !aTags || !Array.isArray(aTags)) {
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }

ditto

@@ +308,5 @@
> +                        .DBConnection;
> +    let conn = yield Sqlite.wrapStorageConnection({ connection: db });
> +    let stmt = "DELETE FROM moz_tags_relation WHERE " +
> +                 "tag_id = (SELECT id from moz_tags WHERE tag = :tag) AND " +
> +                 "place_id = (SELECT id from moz_places WHERE url = :url)";

you should also remove a tag if it becomes unused.

I think you might do that directly in the AFTER DELETE trigger.

@@ +389,5 @@
> +  //nsITaggingService
> +  taggedURIs: Task.async(function* TS_taggedURIs(aTags) {
> +    if (aTags && !Array.isArray(aTags)) {
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }

ditto

I think this should also allow a single tag, that internally you could transform into an array. also in the other methods.

@@ +405,5 @@
> +      if (i < (aTags.length - 1)){
> +        sqlStmt += ", ";
> +      }
> +      tagParams["tag" + i] = aTags[i];
> +    }

aTags.forEach(v => tagParams[`tag${v}`] = v);
let bindings = Object.keys(params).map(v => ':' + v).join(", ");
sqlStmt = `SELECT...
           ...
           WHERE tag IN (${bindings})
           ...`;

@@ +435,5 @@
> +    }
> +    if (rows.length != 0) {
> +      for (row of rows) {
> +        tags.push(row.getResultByName("tag"));
> +      }

here as well I think you can
return rows.map(row => row.getResultByName("tag"));

Comment 11

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_migration_new_tags.diff part1 v4

Updated with comments

Comment 12

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_updating_nsTaggingService.diff part v2

After a long delay.

The only comment that I couldn't incorporate was the use of INSERT OR REPLACE.

Secondly the tests are a bit unstructured, but they are there to show that it works. I'll update that on the next update. 
Finally a question on checking whether a URI is of Ci.nsIURI / attempting to make a URI otherwise. Is it better to pull it out into it's own function similar to e.g.

function check_or_make_URI(url) {
  if (!(url instanceof Ci.nsIURI)){
      try {
        aURI = NetUtil.newURI(url);
      } catch (ex) {
        return null;
      }
    }
  return aURI;
}

and then therefore we can do check_or_make_uri first and then check whether it's null / not and if it is null, throw Cr.NS_ERROR_INVALID_ARG

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8496412 [details] [diff] [review]
bug1039200_migration_new_tags.diff part1 v4

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

::: toolkit/components/places/Database.cpp
@@ +1976,5 @@
> +nsresult
> +Database::MigrateV25Up()
> +{
> +
> +  // Create moz_tags table if it does not exist

nit: the newline above can be removed.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8505519 [details] [diff] [review]
bug1039200_updating_nsTaggingService.diff part v2

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

So, we are moving all of the new APIs to URL (https://developer.mozilla.org/en-US/docs/Web/API/URL)
You can import URL in modules and xpcshell through Cu.importGlobalProperties(["URL"]);

This means you might have to do some URL to URI or href dance.

We will also need a third patch that calls the new APIs from the old ones, so we keep the new table updated.

Unfortunately once we will start using the new API, the old one will return outdated data, I'm not sure I have a solution for that yet, but there are also other complications around we'll have to solve in future (tag autocomplete, tag notifications, tag queries...). let's take issues one at a time.

::: toolkit/components/places/PlacesUtils.jsm
@@ +1842,5 @@
>                                     "nsIAnnotationService");
>  
> +XPCOMUtils.defineLazyGetter(PlacesUtils, "tagging", () => {
> +  let bm = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]
> +             .getService(Ci.nsINavBookmarksService);

this should point to "@mozilla.org/browser/tagging-service;1", "nsITaggingService", not bookmarks.

::: toolkit/components/places/Tagging.jsm
@@ +5,5 @@
> + "use strict";
> +
> +/**
> +
> + * This module provides an asynchronous API for managing tags

empty newline above

@@ +13,5 @@
> +
> + const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +
> +
> + Cu.import("resource://gre/modules/XPCOMUtils.jsm");

double newline

@@ +42,5 @@
> +    // It's too late to block shutdown, just close the connection.
> +    yield db.close();
> +  }
> +  return db;
> +}

let's copy the code from Bug 834545 for DBConnPromised

@@ +52,5 @@
> +   *
> +   * @param aTagName
> +   *        the name for the new tag.
> +   */
> +   _ncreateTag: function* TS__ncreateTag(aTagName) {

we should not expose internal methods or properties to consumers, the module scope allows us to easily hide internal representation.
So, please move all private properties or methods to the global module scope.

You also don't need anymore to give both a name and a label to object methods, we had to do this in the past due to limitations in js stack walking, today those are fixed, so "name: function ()" is enough

So I'd move this in the global scope as

let maybeCreateTag = Task.async(function*(tag) {
  ...
});

@@ +54,5 @@
> +   *        the name for the new tag.
> +   */
> +   _ncreateTag: function* TS__ncreateTag(aTagName) {
> +    let conn = yield promiseDB();
> +    let stmt = "INSERT OR REPLACE INTO moz_tags (tag) VALUES (:tag)";

why OR REPLACE rather than OR IGNORE?

@@ +55,5 @@
> +   */
> +   _ncreateTag: function* TS__ncreateTag(aTagName) {
> +    let conn = yield promiseDB();
> +    let stmt = "INSERT OR REPLACE INTO moz_tags (tag) VALUES (:tag)";
> +    yield conn.executeCached(stmt, { tag: aTagName });

nit: I'm fine if you want to inline everything in the executeCached calls, like
yield conn.executeCached(
  `INSERT OR REPLACE INTO moz_tags (tag)
   VALUES (:tag)
  `, { tag: aTagName});

@@ +68,5 @@
> +   *        the new name of the tag
> +   */
> +   _updateTag: function* TS__updateTag(aOldTag, aNewTag) {
> +    let conn = yield promiseDB();
> +    let stmt = "UPDATE moz_tags SET tag = :new_tag WHERE tag =:old_tag";

the above comments can be repeated for the other private methods, so I'm not going to repeat them.

missing whitespace after =

@@ +74,5 @@
> +    yield conn.executeCached(stmt, params);
> +  },
> +
> +  /**
> +   * Inserts a URI that does not exist in moz_places

well, actually the OR IGNORE means we "insert an URL in moz_places, if needed."

@@ +77,5 @@
> +  /**
> +   * Inserts a URI that does not exist in moz_places
> +   *
> +   * @param aURI
> +   *        the URI to be inserted

all URI should become URL

@@ +84,5 @@
> +    let conn = yield promiseDB();
> +    let stmt = "INSERT OR IGNORE INTO moz_places (url, rev_host, hidden, frecency, guid) " +
> +    "VALUES (:page_url, :rev_host, :hidden, :frecency, GENERATE_GUID()) ";
> +    let rev_url = aURI.host.split("").reverse().join("") + '.';
> +    let frecency = aURI.scheme === 'place' ? 0 : -1;

URL has .protocol

@@ +86,5 @@
> +    "VALUES (:page_url, :rev_host, :hidden, :frecency, GENERATE_GUID()) ";
> +    let rev_url = aURI.host.split("").reverse().join("") + '.';
> +    let frecency = aURI.scheme === 'place' ? 0 : -1;
> +
> +    let params = { page_url: aURI.spec, rev_host: rev_url, hidden: 1, frecency: frecency };

and .href

@@ +96,5 @@
> +   * persist. Tags in aTags which are already set for the given URL are
> +   * ignored. Tags which have different casing are updated to the latest casing
> +   *
> +   * @param aURI
> +   *        the URL to tag (either nsIURI or string).

this should be either URL, nsIURI or href.

@@ +99,5 @@
> +   * @param aURI
> +   *        the URL to tag (either nsIURI or string).
> +   * @param aTags
> +   *        A single tag or an array of tags to set for the given URL.
> +   */

@return {Promise} resolved when tagging is complete.
@throws if input is invalid.

@@ +101,5 @@
> +   * @param aTags
> +   *        A single tag or an array of tags to set for the given URL.
> +   */
> +  tag: function TS_tag(aURI, aTags) {
> +    if (!aURI || !aTags || ((!Array.isArray(aTags)) && !(typeof aTags === 'string'))) {

you need to implement a better input validation algorithm, the patch in bug 1068009 has a more complicated validator, but you don't need to go that fancy.

Things we need to check here:
uri is non-null and either a string (that we try convert internally to a URL), a URL or a nsIURI (that we convert internally to a URL)
tags must be non-null, not an empty string, if an array not an empty array, not an array containing empty strings or nulls.

I guess a couple helpers could easily be reused across the various APIs here.

@@ +102,5 @@
> +   *        A single tag or an array of tags to set for the given URL.
> +   */
> +  tag: function TS_tag(aURI, aTags) {
> +    if (!aURI || !aTags || ((!Array.isArray(aTags)) && !(typeof aTags === 'string'))) {
> +      throw Cr.NS_ERROR_INVALID_ARG;

This is not crossing XPCOM, so we will throw new Error("nice error description");

@@ +113,5 @@
> +      }
> +    }
> +    return Task.spawn(function* () {
> +      if (!Array.isArray(aTags))
> +        aTags = [aTags];

Ideally your validator could convert the input automatically and always return an array

@@ +117,5 @@
> +        aTags = [aTags];
> +
> +      let url = aURI.spec;
> +      let conn = yield promiseDB();
> +      yield this._insertURIIntoPlaces(aURI);

you should use conn.executeTransaction to wrap all the db writes here

@@ +120,5 @@
> +      let conn = yield promiseDB();
> +      yield this._insertURIIntoPlaces(aURI);
> +      for (let tag of aTags) {
> +        let rows = yield conn.executeCached("SELECT tag FROM moz_tags " +
> +          "WHERE UPPER(tag) = UPPER(:t_tag) ", { t_tag: tag });

use string template

@@ +121,5 @@
> +      yield this._insertURIIntoPlaces(aURI);
> +      for (let tag of aTags) {
> +        let rows = yield conn.executeCached("SELECT tag FROM moz_tags " +
> +          "WHERE UPPER(tag) = UPPER(:t_tag) ", { t_tag: tag });
> +          if (rows.length == 0){ // if no tag exists, create a new tag

move the comment to its own line

@@ +125,5 @@
> +          if (rows.length == 0){ // if no tag exists, create a new tag
> +            yield this._ncreateTag(tag);
> +          } else {
> +            let tagName = rows[0].getResultByName("tag");
> +            if (tagName != tag){ // if tags don't match update db tag to match newer casing

ditto

@@ +132,5 @@
> +        }
> +        let params = { tag: tag, url: url};
> +        let stmt = "INSERT OR IGNORE INTO moz_tags_relation (tag_id, place_id) VALUES(" +
> +          "(SELECT id FROM moz_tags WHERE tag = :tag), " +
> +          "(SELECT id FROM moz_places WHERE url = :url)) ";

string template

@@ +135,5 @@
> +          "(SELECT id FROM moz_tags WHERE tag = :tag), " +
> +          "(SELECT id FROM moz_places WHERE url = :url)) ";
> +        yield conn.executeCached(stmt, params);
> +      }
> +    }.bind(this));

not needed once you move methods to the global scope

@@ +145,5 @@
> +   *
> +   * @param aURI
> +   *        the URL to tag (either nsIURI or string).
> +   * @param aTags
> +   *        A single tag or an array of tags to untag

ditto for @throws and @return

@@ +149,5 @@
> +   *        A single tag or an array of tags to untag
> +   */
> +  untag: function TS_untag(aURI, aTags){
> +    if (!aURI || !aTags || ((!Array.isArray(aTags)) && !(typeof aTags === 'string'))) {
> +      throw Cr.NS_ERROR_INVALID_ARG;

ditto for validation

@@ +171,5 @@
> +      for (let tag of aTags) {
> +        let params = { url: url, tag: tag};
> +        yield conn.executeCached(stmt, params);
> +      }
> +    }.bind(this));

ditto

@@ +178,5 @@
> +  /**
> +   * Retrieves all URLs tagged with all of the given tag(s)
> +   * @param aTags
> +   *        A single tag or array of tags.
> +   * @returns array of URIs

@return (no s) and it axtually @return {Promise} and @resolves to an array of URLs
also @throws

@@ +180,5 @@
> +   * @param aTags
> +   *        A single tag or array of tags.
> +   * @returns array of URIs
> +  */
> +  taggedURIs: function TS_taggedURIs(aTags) {

becomes taggedURLs

@@ +183,5 @@
> +  */
> +  taggedURIs: function TS_taggedURIs(aTags) {
> +    if (!aTags || ((!Array.isArray(aTags)) && !(typeof aTags === 'string'))) {
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }

ditto for validation

@@ +215,5 @@
> +   * Retrieves all tags set for the given URI.
> +   *
> +   * @param aURI
> +   *        the URL to tag (either nsIURI or string).
> +   * @returns array of tags

ditto for @return, @throws and @resolve

@@ +217,5 @@
> +   * @param aURI
> +   *        the URL to tag (either nsIURI or string).
> +   * @returns array of tags
> +   */
> +  tagsForURI: function TS_tags(aURI) {

for URL (and ditto for validation)

@@ +231,5 @@
> +      let url = aURI.spec;
> +      let tags = [];
> +      let rows = [];
> +
> +      if (!aURI) { // if no URI provided, retrieve all tags

move comment to its own line

@@ +242,5 @@
> +      }
> +      if (rows.length != 0) {
> +        return rows.map(row => row.getResultByName("tag"));
> +      }
> +      return tags;

tags is not really useful... you could just

return rows.map(row => row.getResultByName("tag"))
if rows is empty should return an empty array too...

::: toolkit/components/places/tests/unit/test_1039200_new_tags_table.js
@@ +10,5 @@
> +function run_test() {
> +  run_next_test();
> +}
> +
> +add_task(function* tagging_test() {

you should also add tests for invalid input for each API, using Assert.throws (see the tests in bug 1068009 for examples)

Comment 15

[Marco Bonardo [:mak]]

(In reply to Althaf Hameez [:ahameez] from comment #12)
> Finally a question on checking whether a URI is of Ci.nsIURI / attempting to
> make a URI otherwise. Is it better to pull it out into it's own function

yes, validator functions are nicer in this case cause we need them in multiple APIs.

Comment 16

[Althaf Hameez [:ahameez]]

>tags must be non-null, not an empty string, if an array not an empty array, 
>not an array containing empty strings or nulls.

If a passed in tag array contains something like ["one", "two", null, "three"]
Do we strip out the null and process the rest of the valid tags, or do we reject the whole array completely?

Comment 17

[Marco Bonardo [:mak]]

we should reject, it's clearly a coding error by the caller, we should not hide it.

Comment 18

[Marco Bonardo [:mak]]

or better, we should throw immediately, not reject.

Comment 19

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_updating_nsTaggingService.diff v3

Updated :)

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8508741 [details] [diff] [review]
bug1039200_updating_nsTaggingService.diff v3

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

it looks good.

apart fixing the following comments, what's left to do is to define interaction between the new and the old API. I'm discussing this with Mano right now, hope we can get out with something meaningful in short time. But that should be for a part 3 patch

::: toolkit/components/places/PlacesUtils.jsm
@@ +1843,5 @@
>  
> +XPCOMUtils.defineLazyGetter(PlacesUtils, "tagging", () => {
> +  let bm = Cc["@mozilla.org/browser/tagging-service;1"]
> +             .getService(Ci.nsITaggingService);
> +  return Object.freeze(new Proxy(bm, {

s/bm/ts/ (as tagging-service)

::: toolkit/components/places/Tagging.jsm
@@ +12,5 @@
> +
> + const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +
> + Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> + Cu.import("resource://gre/modules/PlacesUtils.jsm");

please defineLazyModuleGetter this as well

@@ +27,5 @@
> +  "resource://gre/modules/Task.jsm");
> + XPCOMUtils.defineLazyModuleGetter(this, "Sqlite",
> +  "resource://gre/modules/Sqlite.jsm");
> +
> + const URL_LENGTH_MAX = 65536;

// Imposed to limit database size.
const DB_URL_LENGTH_MAX = 65536;

@@ +56,5 @@
> + *
> + * @param tag
> + *        the name for the new tag.
> + */
> +let createTag = Task.async(function*(tag) {

s/createTag/maybeCreateTag/

@@ +57,5 @@
> + * @param tag
> + *        the name for the new tag.
> + */
> +let createTag = Task.async(function*(tag) {
> +  let conn = yield DBConnPromised;

let's pass the connection handle as first argument, you should already have it and we can avoid one yield.

@@ +70,5 @@
> + *        the tag to be renamed
> + * @param aNewTag
> + *        the new name of the tag
> + */
> +let updateTag = Task.async(function*(aOldTag, aNewTag) {

s/updateTag/renameTag/

@@ +71,5 @@
> + * @param aNewTag
> + *        the new name of the tag
> + */
> +let updateTag = Task.async(function*(aOldTag, aNewTag) {
> +  let conn = yield DBConnPromised;

ditto for the handle

@@ +74,5 @@
> +let updateTag = Task.async(function*(aOldTag, aNewTag) {
> +  let conn = yield DBConnPromised;
> +  yield conn.executeCached(
> +    `UPDATE moz_tags SET tag = :new_tag WHERE tag = :old_tag `
> +    , { old_tag: aOldTag, new_tag: aNewTag });

nit:

`UPDATE moz_tags SET tag = :new_tag
 WHERE tag = :old_tag
`, { old_tag: aOldTag, new_tag: aNewTag });

@@ +78,5 @@
> +    , { old_tag: aOldTag, new_tag: aNewTag });
> +});
> +
> +/**
> + * Inserts a URL that into moz_places if needed

s/that//

@@ +83,5 @@
> + *
> + * @param aURL
> + *        the URL to be inserted
> + */
> +let insertURLIntoPlaces = Task.async(function*(aURL) {

s/insertURLIntoPlaces/maybeCreatePlaceEntry/

@@ +84,5 @@
> + * @param aURL
> + *        the URL to be inserted
> + */
> +let insertURLIntoPlaces = Task.async(function*(aURL) {
> +  let conn = yield DBConnPromised;

ditto for the handle

@@ +86,5 @@
> + */
> +let insertURLIntoPlaces = Task.async(function*(aURL) {
> +  let conn = yield DBConnPromised;
> +
> +  let rev_url = aURL.host.split("").reverse().join("") + '.';

copy the getReversedHost function from Bookmarks.jsm
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#965

@@ +98,5 @@
> +
> +/**
> + * Attempts to convert the given param to a URL
> + * Converts a nsIURI or a valid href string to URL
> + * Throws error if invalid

I'd say something like 
Validates a URL like value, trying to convert it into a URL object.

@param val
       URL object, href string or nsIURI.
@return an URL object for the input value
@throws if input is invalid

@@ +103,5 @@
> +*/
> +let validateURL = function(val) {
> +  if ((typeof(val) == "string" && val.length <= URL_LENGTH_MAX) ||
> +              (val instanceof Ci.nsIURI && val.spec.length <= URL_LENGTH_MAX) ||
> +              (val instanceof URL && val.href.length <= URL_LENGTH_MAX))

fix indentation please

@@ +109,5 @@
> +    if (typeof(val) === "string")
> +      return new URL(val);
> +    if (val instanceof Ci.nsIURI)
> +      return new URL(val.spec);
> +    return val;

At this point you might well just split the ifs...

if (typeof(val) == "string" && val.length <= URL_LENGTH_MAX)
  return new URL...
if (val instanceof Ci.nsIURI && val.spec.length <= URL_LENGTH_MAX)
  return new URL...
if (URL)
  return ...

throw

@@ +119,5 @@
> + * Validates a tag by checking whether the input is a string or an array of strings
> + * If it's a non-empty string it converts it returns an array with the string as the lone element
> + * otherwise it throws an error
> + * If it's an array of strings, checks each item to ensure it's a non-empty string
> + * If any item is invalid, it throws an error, otherwise returns the valid array

Validates a tag or a tags array.

@param tags
       a tag string or an array of tag strings.
@return an array of tag strings
@throws if a tag is not a string or is an empty string.

I'm wondering if this should also remove duplicates from the array before returning it, sounds like it might be worth it. should be easy with a return tags.filter((v, i, a) => a.indexOf(v) == i) (should keep only the first entry for each value), that behavior should be added to the javadoc.

@@ +121,5 @@
> + * otherwise it throws an error
> + * If it's an array of strings, checks each item to ensure it's a non-empty string
> + * If any item is invalid, it throws an error, otherwise returns the valid array
> +*/
> +let validateTags = function(aTags) {

nit: "a" prefix notation in js is not mandatory anymore. if you like you can keep it, or you can just drop it and name this tags. Just make the code module coherent regarding your choice.

@@ +125,5 @@
> +let validateTags = function(aTags) {
> +  if ((typeof aTags == 'string') && !/^\s*$/.test(aTags))
> +    return [aTags];
> +  let valid;
> +  if (Array.isArray(aTags) && aTags.length > 0){

whitespace before {

@@ +132,5 @@
> +        return true;
> +    });
> +  }
> +  if (valid) {
> +    return aTags

I'm not sure why you need to define valid outside like this and not just

if (Array.isArray && length > 0) {
if (tags.every(tag => typeof(tag) == "string" && !(/^\s*$/.test(tag))))
  return tags;
}

throw new Error...

@@ +141,5 @@
> +};
> +
> +let Tagging = Object.freeze({
> +
> +  /**

nit: no need for that newline

@@ +144,5 @@
> +
> +  /**
> +   * Tags a URL with the given set of tags. Current tags set for the URL
> +   * persist. Tags in aTags which are already set for the given URL are
> +   * ignored. Tags which have different casing are updated to the latest casing

s/latest casing/passed-in casing, if different./

@@ +147,5 @@
> +   * persist. Tags in aTags which are already set for the given URL are
> +   * ignored. Tags which have different casing are updated to the latest casing
> +   *
> +   * @param aURL
> +   *        the URL to tag (either URL, nsIURI or href string).

I'd replace parenthesis with a simple comma

@@ +171,5 @@
> +               // if no tag exists, create a new tag
> +              yield createTag(tag);
> +            } else {
> +              let tagName = rows[0].getResultByName("tag");
> +              // if tags don't match update db tag to match newer casing

ucfirst, add comma after match, end with a period

I'd probably also s/db tag/tag/

@@ +181,5 @@
> +          yield conn.executeCached(
> +            `INSERT OR IGNORE INTO moz_tags_relation (tag_id, place_id)
> +            VALUES( (SELECT id FROM moz_tags WHERE tag = :tag),
> +            (SELECT id FROM moz_places WHERE url = :url))
> +            `, { tag: tag, url: url.href});

please fix indentation so that apices are aligned, as well as the query, like
`query1
 query2
`, params

also, please add whitespace before }

@@ +192,5 @@
> +   * Removes tags from a URL. Tags from aTags which are not set for the
> +   * given URL are ignored.
> +   *
> +   * @param aURL
> +   *        the URL to tag (either URL, nsIURI or href string).

to untag

@@ +209,5 @@
> +      for (let tag of tags) {
> +        yield conn.executeCached(
> +          `DELETE FROM moz_tags_relation WHERE tag_id =
> +            (SELECT id from moz_tags WHERE tag = :tag) AND place_id =
> +            (SELECT id from moz_places WHERE url = :url)

please reindent as
DELETE FROM
WHERE tag_id =
  AND place_id =

@@ +210,5 @@
> +        yield conn.executeCached(
> +          `DELETE FROM moz_tags_relation WHERE tag_id =
> +            (SELECT id from moz_tags WHERE tag = :tag) AND place_id =
> +            (SELECT id from moz_places WHERE url = :url)
> +          `, { url: url.href, tag: tag});

please add whitespace before }

@@ +218,5 @@
> +
> +  /**
> +   * Retrieves all URLs tagged with all of the given tag(s)
> +   * @param aTags
> +   *        A single tag or array of tags.

or an array...

@@ +231,5 @@
> +      let conn = yield PlacesUtils.promiseDBConnection();
> +      let urls = [];
> +      let tagParams = {};
> +
> +      tags.forEach(v => tagParams[`tag${v}`] = v);

let tagParams = tags.reduce((p, c, i) => { p[`tag${i}`] = c; return p; }, {})

@@ +232,5 @@
> +      let urls = [];
> +      let tagParams = {};
> +
> +      tags.forEach(v => tagParams[`tag${v}`] = v);
> +      let bindings = Object.keys(tagParams).map(v => ':' + v).join(", ");

just .join() will be fine

@@ +242,5 @@
> +        GROUP BY url HAVING count(*) = ${Object.keys(tagParams).length}`;
> +
> +      let rows = yield conn.executeCached(sqlStmt, tagParams);
> +      if (rows.length != 0) {
> +        for (let row of rows){

the length check looks pointless

@@ +243,5 @@
> +
> +      let rows = yield conn.executeCached(sqlStmt, tagParams);
> +      if (rows.length != 0) {
> +        for (let row of rows){
> +          urls.push(row.getResultByName("url"));

and fwiw you could probably return rows.map(r => r.getResultByName("url"))

@@ +278,5 @@
> +        rows = yield conn.executeCached(
> +          `SELECT t.tag FROM moz_places h
> +           JOIN moz_tags_relation r ON place_id = h.id
> +           JOIN moz_tags t ON t.id = r.tag_id
> +           WHERE url = :url `, { url: url.href });

please align apices

@@ +283,5 @@
> +      }
> +      if (rows.length != 0) {
> +        return rows.map(row => row.getResultByName("tag"));
> +      }
> +      return tags;

ditto, you can just return rows.map in any case, if length is zero that will return an empty array, that is what you expect.

::: toolkit/components/places/tests/unit/test_1039200_new_tags_table.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.importGlobalProperties(["URL"]);

we are now doing this in head_common.js, so you can stop doing this, it will just work

@@ +19,5 @@
> +  yield promiseAddVisits(T_URI);
> +
> +  // Tagging
> +  yield PlacesUtils.tagging.tag(T_URI, ["one"]);  // single tag
> +  Assert.deepEqual((yield PlacesUtils.tagging.tagsForURL(T_URI)), ["one"]);

sounds like you might want an helper to short name for PlacesUtils.tagging.tagsForURL, like promiseURLTags

@@ +89,5 @@
> +  Assert.throws(() => PlacesUtils.tagging.tagsForURL(5), "5 is not a valid URL");
> +
> +  // null URI
> +  Assert.throws(() => PlacesUtils.tagging.tag(null, "one"), "Invalid URL provided");
> +  Assert.throws(() => PlacesUtils.tagging.untag(null, "one"), "Invalid URL provided");

just in case, please also test undefined

@@ +113,5 @@
> +
> +  // Array containing nulls
> +  Assert.throws(() => PlacesUtils.tagging.tag(T_URI, ["one", null, "two"]), "Invalid tag(s) provided");
> +  Assert.throws(() => PlacesUtils.tagging.untag(T_URI, ["one", null, "two"]), "Invalid tag(s) provided");
> +  Assert.throws(() => PlacesUtils.tagging.taggedURLs(["one", null, "two"]), "Invalid tag(s) provided");

please also test undefined and a number

Comment 21

[Marco Bonardo [:mak]]

This is the plan we built, so far. It's sort of crazy but it's the best path forward we found to support having 2 services doing the same thing on incompatible storage.

1. we need the new tags to stay up-to-date with the old ones.

For this we need to make the new API observe onItemAdded and onItemRemoved (to observe tag changes).
We must init the module in PlacesCategoriesStarter. on import the module will register a single bookmarks observer using PlacesUtils.addLazyBookmarksObserver. This way we don't initialize the bookmarks service too early.
Once it gets notifications it will send an onItemChanged "tags" notification through the API I'm going to add in bug 1081099.
For this to work correctly we must remove onItemChanged "tags" notification for tags from Bookmarks.jsm and nsNavBookmarks.cpp, so that the new tagging service is the only one sending that kind of notification.

2. the new API must invoke the old one internally. This way we keep the old one working, the old one will notify onItemAdded/Removed and the new one will intercept those and notify onItemChanged.

2b. at this point using the new API or the old one should bring same results. We can close this bug.

3. (new bug) we must convert tag queries (RESULTS_AS_TAG_QUERY and RESULTS_AS_TAG_CONTENTS) to use the new tables. ensure they properly update using the existing notifications.

4. (new bug) convert consumers to the new API.

5. (new bug) deprecate the old API

6. remove the old API

Comment 22

[Althaf Hameez [:ahameez]]

> copy the getReversedHost function from Bookmarks.jsm
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#965

Do you to use it here as a function as well, or just copy the way the method works?

Comment 23

[Marco Bonardo [:mak]]

2 possibilities:
1. just copy it to this module
2. move it to PlacesUtils and use in both places (likely preferred)

Comment 24

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_updating_nsTaggingService.diff v4

Moved the function to PlacesUtils

Comment 25

[Althaf Hameez [:ahameez]]

Attachment: bug1039200_updating_nsTaggingService.diff v4 updated

forgot to rename a variable in one place.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8511991 [details] [diff] [review]
bug1039200_updating_nsTaggingService.diff v4 updated

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

great job!

::: toolkit/components/places/Bookmarks.jsm
@@ -961,5 @@
> - * @param url
> - *        the URL to generate a rev host for.
> - * @return the reversed host string.
> - */
> -function getReversedHost(url) url.host.split("").reverse().join("") + ".";

could you please move the part of the patch that moves getReversedHost to a separate bug that we can land immediately?
Since this bug still needs a third part, better try to not bitrot pieces of it.

::: toolkit/components/places/PlacesUtils.jsm
@@ +387,5 @@
>    },
>  
>    /**
> +   * Reverse a host based on the moz_places algorithm, that is reverse the host
> +   * string and add a trialing period.  For example "google.com" becomes

heh, typo s/trialing/trailing/ (my fault)

::: toolkit/components/places/Tagging.jsm
@@ +58,5 @@
> + *
> + * @param tag
> + *        the name for the new tag.
> + */
> +let maybeCreateTag = Task.async(function*(conn, tag) {

nit: usually for anonymous functions ( like function () or function* () ) in Places we add a whitespace before the parenthesis, to cope with the lack of a name (it would be function name() {})

@@ +74,5 @@
> + */
> +let renameTag = Task.async(function*(conn, oldTag, newTag) {
> +  yield conn.executeCached(
> +    `UPDATE moz_tags SET tag = :new_tag
> +     WHERE tag = :old_tag `

move backtick to the next line, aligned with the opening one, and no whitespace here please

@@ +114,5 @@
> +  throw new Error("Invalid URL provided");
> +};
> +
> +/**
> + * Validates a tag or a tags array.

please add "In case of an array, this will also de-duplicate entries." on a second line

@@ +127,5 @@
> +    return [tags];
> +
> +  if (Array.isArray(tags) && tags.length > 0) {
> +    if (tags.every(tag => typeof(tag) == "string" && !(/^\s*$/.test(tag))))
> +      return tags.filter((v, i, a) => a.indexOf(v) == i);

brace the if and add a comment that we are removing duplicates here (just to clarify what's up to the reader)

@@ +233,5 @@
> +        JOIN moz_tags t ON t.id = r.tag_id
> +        WHERE tag IN ( ${bindings} )
> +        GROUP BY url HAVING count(*) = ${Object.keys(tagParams).length}`;
> +
> +      let rows = yield conn.executeCached(sqlStmt, tagParams);

nit: could you please inline the stmt here as usual?

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +142,5 @@
>  skip-if = os == "android"
>  [test_utils_backups_create.js]
>  [test_utils_getURLsForContainerNode.js]
>  [test_utils_setAnnotationsFor.js]
> +[test_1039200_new_tags_table.js]
\ No newline at end of file

please keep this list in alphabetical order

Comment 27

[Milind L (:milindl)]

Attachment: Bug 1039200 Part1 - Add migrations and associated test for tags and tags relation table,

This adds migration for schema v38. In this we add 2 tables, moz_tags and moz_tags_relation,
and get all the folders/tag-bookmarks over to them respectively.

Review commit: https://reviewboard.mozilla.org/r/145300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145300/

Comment 28

[Milind L (:milindl)]

Hi,

The code was changed a bit to conform to the current state of stuff.

Mostly for C++ it was just the fact that some tables like moz_bookmarks_root don't exist anymore and this is V38 not V25 of the schema.

For JS I rewrote most of the test using async/await, more like newer tests. There was a places_v37.sqlite file added in 977177 but it seemed not to be doing anything (I can't find it anywhere else, other tests seem not to be using it), so I just replaced it with one of my own containing some data required for testing.

I tested it locally, and it works.

Thanks!

Comment 29

[Marco Bonardo [:mak]]

note, this week we are in feature-freeze for the upcoming merge of Firefox 55 to Beta, so migrations can't land. This this may be delayed by a week or so.

Comment 30

[Marco Bonardo [:mak]]

Milind, regarding the API design for tags, atm we don't have a path forward that is clearly superior to the other one. So we need designing and comparison of pro/cons. Also see the discussion in bug 1225916.

The functionality we need is:
1. Get all the tags, for each tag have the count of how many bookmarks are tagged with it
2. add and remove tags
3. get a list of all the bookmarks in a tag
4. get a list of all the tags for a given bookmark

The above lists should be easy to undupe in case we want urls instead of bookmarks, but I assume that's trivial if we return an array of bookmark objects using .map().

The 2 approaches are:
1. keep the tagging API separated as it is now, just move it into a Tagging.jsm and make it async
2. merge the tagging API into Bookmarks.jsm, as:
  * .insert({ ..., addTags: []}); will throw for folders and separators or if removeTags is provided.
  * .update({ ..., addTags: [], removeTags[]});
  * .fetch({ ..., tags: [] }); the doubt here is whether we should do an AND or an OR of the tags, what's more common?
  * .fetchTags(guidOrUrlOrNothing), fetch list of tags for a bookmark or its url, if nothing is passed fetch list of all the tags.

The advantage of the separate API is that it clearly applies to urls, and it can be adapted more easily... on the other side, we'll keep letting to set tags through the bookmarking UI, thus we'll always need to make tricks to keep them working together.

Do you oversee any problems with merging the API with Bookmarks, from you unbiased point of view?
Do you think the above plan would work if you should implement something that uses tags?
Would you use more often ANDed or ORed tags?

I think it's worth looking at this from an external POV, I'm really biased from years of legacy code :)

Comment 31

[Marco Bonardo [:mak]]

For this specific patch, it's likely we'll enqueue it after bug 1352502, because that has an higher priority for the product. so it may need an unbitrot for schema versions.

Comment 32

[Milind L (:milindl)]

Initially I was strongly inclined towards the separate APIs...tagging seems to be something you can do for URLs in general, not just bookmarks, and that makes a lot of sense also. Reading through the "Places Developer Guide", I see that "[tagging] service is URI-centric, meaning that consumers can tag URIs directly, without first creating a bookmark" but at the same time "...integrates with and is internally dependent upon bookmarks".

It seems like the intention originally was indeed tagging URIs, but due to current state of affairs we are considering moving towards integration with bookmarks. It also seems to me as an end-user that FF strongly integrates bookmarks with tags (tagging seems to be offered readily when I make a bookmark but seems like I need to open the library etc to tag urls...and the url I tagged ends up in the bookmarks anyway)

> Also see the discussion in bug 1225916.
I get that tagging will stay close to bookmarks and will not become an substitute/complement at least for webextensions -> this supports linking bookmarks to tagging instead of separate api

> Do you oversee any problems with merging the API with Bookmarks, from you
> unbiased point of view?
I think we should be good...
-> imo the relation table should be from moz_bookmarks to tags, instead of moz_places
-> this seems like this is a good time to do it, since we'll only be dealing with internal consumers (if we land in 57). apparently, webext doesn't have this yet.

> Do you think the above plan would work if you should implement something
> that uses tags?
this is a tricky one, since I've never written an add-on. I guess it will not be very different than what we have right now, since bookmarks and tags are very closely linked anyway. There is a tagging service to hide some of the internals, but any tagged uri needs to be added to bookmarks to be used


> Would you use more often ANDed or ORed tags?
ANDing makes sense to me, as I am looking for one particular result usually, and thus need to narrow down my query.
maybe one can also look at the case when you have AND in fetch but need OR. In that case you need to use union of these sets, while in the other case need to make an intersection.
I feel like we may get a lot of results wastefully from DB before discarding them in the intersection operation if we use fetch with OR implementation to do AND - this may be a possible reason to use AND over OR.


> I think it's worth looking at this from an external POV

At the same time, this is the first thing I've worked on having more than a few developers or large real world impact, so I may easily miss something :)
 
In summary, I would say go with merging this with Bookmarks.jsm: 
a) current UI is almost a perfect fit for this kind of tagging - changing the UI to use uri tags properly may take a lot of time and effort (realistically speaking).
b) the WebExt api thread makes it seem that tags can exist just for bookmarks (atleast in WebExt api) - so the only way we could actually tag URIs would be through an internal consumers.
Possible issues:
a) will be hard to fix later if WebExt decides to introduce tagging for URIs...or in any case really where we need to tag URIs instead of bookmarks.

My opinion is that the current situation is so close to actually merging with bookmarks that it makes sense to do it.

Comment 33

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #32)
> I see that "[tagging] service is URI-centric, meaning that consumers can tag
> URIs directly, without first creating a bookmark" but at the same time
> "...integrates with and is internally dependent upon bookmarks".

Right, the original intent and the most obvious thing to do is to tag uris.
But things are not ideal:
1. the ui associates them with bookmarks, and, differently from keywords, there's no future plan to undo that
2. a plausible plan for tags as alternative for bookmarks never concretized
3. foreign_count in moz_places means if tags are not bookmarked we may get orphans (as we do for keywords...)

> It also seems to me as an end-user that FF strongly integrates
> bookmarks with tags (tagging seems to be offered readily when I make a
> bookmark but seems like I need to open the library etc to tag urls...and the
> url I tagged ends up in the bookmarks anyway)

Good point indeed.

> -> imo the relation table should be from moz_bookmarks to tags, instead of
> moz_places
> -> this seems like this is a good time to do it, since we'll only be dealing
> with internal consumers (if we land in 57). apparently, webext doesn't have
> this yet.

The problem with that is that we'd end up with different bookmarks having different tags and that wouldn't be a great experience for the user, imo.
I actually think we should still relate to place_id (fk in bookmarks).
That's the pain point, the API and the scheme won't relation in a perfect way.

> > Would you use more often ANDed or ORed tags?
> ANDing makes sense to me, as I am looking for one particular result usually,
> and thus need to narrow down my query.
> maybe one can also look at the case when you have AND in fetch but need OR.
> In that case you need to use union of these sets, while in the other case
> need to make an intersection.
> I feel like we may get a lot of results wastefully from DB before discarding
> them in the intersection operation if we use fetch with OR implementation to
> do AND - this may be a possible reason to use AND over OR.

Whatever thing we do, we leave the intersection or concatenation to the consumer...
The current query system in Places (nsINavHistoryQuery) uses AND, so in the end I think we should just be coherent with that.

> In summary, I would say go with merging this with Bookmarks.jsm: 

SGTM.

On the other side, the scheme is more efficient linking with places, than bookmarks, to avoid different bookmarks to the same url having different tags. I'd retain this property in the schema backend.

Comment 34

[Marco Bonardo [:mak]]

Comment on attachment 8873913 [details]
Bug 1039200 Part1 - Add migrations and associated test for tags and tags relation table,

I'm temporarily canceling this review, since we have Activity Stream changes in bug 1352502 that will likely land sooner and require an unbitrot.

But I think that at least now we have a plan for the API, the schema was already looking good, I don't know if the new API may cause us issues with this schema, and that's the next thing to figure out.

Comment 35

[Marco Bonardo [:mak]]

Milind, I granted you editbugs privilege so you can edit any details of bugs.

Comment 36

[Milind L (:milindl)]

Thanks, now I can help with changing details I'm confident about in bugs

I guess the idea for me now would be to CC myself the Activity Stream bug (maybe pick up smaller bugs), and then modify my work accordingly as that is landed.

> to avoid different bookmarks to the same url having different tags. I'd retain this property in the schema backend.

I had not thought of this, makes sense to me now. I couldn't think of any reasons someone might do that, but it is certainly possible, so we need to handle it.

Comment 37

[Milind L (:milindl)]

Comment on attachment 8873913 [details]
Bug 1039200 Part1 - Add migrations and associated test for tags and tags relation table,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145300/diff/1-2/

Comment 38

[Milind L (:milindl)]

Comment on attachment 8873913 [details]
Bug 1039200 Part1 - Add migrations and associated test for tags and tags relation table,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145300/diff/2-3/

Comment 39

[Milind L (:milindl)]

(In reply to Milind L (:milindl) from comment #38)
> Comment on attachment 8873913 [details]
> Bug 1039200 Part1 - Add migrations and associated test for tags and tags
> relation table,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/145300/diff/2-3/

Unbitrotted wrt migration 38.
I'v kept the schema same as previous patch, since we are also making places-id to tag-id relation at schema level, not bookmarks-id to tag-id.

MozReview shows that I have copied places_v37.sqlite to places_v39.sqlite, but they are different (probably the result of me making some errors with hg.)

About places_v37.sqlite, it doesn't seem to be used anywhere, and it is identical to places_v38.sqlite; what is it actually used for? Removing it from xpcshell.ini doesn't seem to make a difference, the tests still run successfully. It seems to have been added a while back in Bug 977177, any idea about that (not strictly on topic, but I was wondering).

In the API to implement, one of the issues would be that we will need to maintain tags in the old way too, isn't it? For example, people who chose to downgrade might need that. I could start on making a first-implementation this weekend.

Comment 40

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #39)
> About places_v37.sqlite, it doesn't seem to be used anywhere, and it is
> identical to places_v38.sqlite; what is it actually used for? Removing it
> from xpcshell.ini doesn't seem to make a difference, the tests still run
> successfully. It seems to have been added a while back in Bug 977177, any
> idea about that (not strictly on topic, but I was wondering).

We add one file per each migration, for future reference.

> In the API to implement, one of the issues would be that we will need to
> maintain tags in the old way too, isn't it? For example, people who chose to
> downgrade might need that. I could start on making a first-implementation
> this weekend.

Migration will have to migrate tags that may have been added during a downgrade/upgrade.
Regarding the API, the old API will have to act on the new tables, as well as the new one. The problem is that having 2 APIs, any internal cache will have to listen for changes made by the other one. That will be simplified only at the point where we can remove the old one, that depends on the time needed to convert consumers.

Comment 41

[Marco Bonardo [:mak]]

Ah, but note that we are not supposed to keep tags the old way for downgrading consumers, it will be fine to lose tags on downgrade (they will be catched again on upgrade). This is our usual path, after a downgrade the product works, but there may be temporary dataloss. we can relnote about that.

Comment 42

[Milind L (:milindl)]

Hi, I've started work on this. This is the series of steps I am thinking of:

1a. [done] Implement new API
1b. [done] Add tests for new API
1c. [in progress] Look for places where tags need to be changed/updated indirectly - this is usually because of the place-tag linking rather than bookmark-tag linking, so for example if bookmark url changes, I need to update the tags_relation database as per new place_id (it's actually more complicated since there might be multiple bookmarks for one url..). This also includes removing tags from tags table if no entry exists for it in tags_relation etc. This is probably the most time-taking thing in 1. 

I'll be able to give a patch for 1 shortly.

2a. Convert old API to new API calls internally (this means old bookmarks will not be changed or updated anymore). In all places, where old API is used, new API will be used (keeping new tags updated).
2b. Find places which rely on the old tagging service in a way not covered by 2a. I do not understand completely what you meant by internal caches (in case they are the internal tagFolderCache in old tagging API that would be handled in 2a).

3. Remove/change tagging notifications in Bookmarks.jsm. At this point notifications are sent from most Bookmarks.jsm methods for tagging also; which notifies the old API. but we don't need this anymore since new API does it inside Bookmarks.jsm itself, without a need for the old API being called.

By the end of 3, new tags table will be updated regardless of which tagging method is preferred by consumers.
Changes suggested after this is just stuff I've thought about for a bit. Other repurcussions might exist as well.

I don't have much knowledge of "browser/components/places/content/" but at the very least it depends on finding nodes which are tag containers, which is not something we will have. There might be more stuff here which needs to be fixed for the places view to work fine.

Similarly I also need to look at other consumers to see if any of my changes causes some issue. After converting all the old consumers, we can drop the old API as soon as we drop support for old add-ons I guess.

The above is what I am thinking of doing, please tell me about anyting that I have missed or understood incorrectly.
Thanks!

Comment 43

[Milind L (:milindl)]

> I don't have much knowledge of "browser/components/places/content/" but at
> the very least it depends on finding nodes which are tag containers, which
> is not something we will have. There might be more stuff here which needs to
> be fixed for the places view to work fine.

It looks like this might be related to converting tag queries (RESULTS_AS_TAG_QUERY and RESULTS_AS_TAG_CONTENTS) to use the new tables instead of looking at every consumer individually.

Comment 44

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #42)
> 2a. Convert old API to new API calls internally (this means old bookmarks
> will not be changed or updated anymore). In all places, where old API is
> used, new API will be used (keeping new tags updated).

How do you plan to do this, considered the old API is synchronous while the new one is async?
spinning the events loop?

> 2b. Find places which rely on the old tagging service in a way not covered
> by 2a. I do not understand completely what you meant by internal caches (in
> case they are the internal tagFolderCache in old tagging API that would be
> handled in 2a).

IIRC both APIs may have an internal cache with a set of all the existing tags (to avoid having to query every time to know if a tag exists).

> 3. Remove/change tagging notifications in Bookmarks.jsm. At this point
> notifications are sent from most Bookmarks.jsm methods for tagging also;
> which notifies the old API. but we don't need this anymore since new API
> does it inside Bookmarks.jsm itself, without a need for the old API being
> called.

Yes, but we'll still need to notify about tag changes, just in different points.

> By the end of 3, new tags table will be updated regardless of which tagging
> method is preferred by consumers.

Note that from 57 on we'd be free to remove the old tagging API, provided internal consumers are handled.

> It looks like this might be related to converting tag queries
> (RESULTS_AS_TAG_QUERY and RESULTS_AS_TAG_CONTENTS) to use the new tables
> instead of looking at every consumer individually.

Yes, there is another aspect that I don't recall if we already fixed completely that is replacing RESULTS_AS_TAG_CONTENTS queries with tags=list_of_tags queries. That is bug 1293445 that may indeed be a blocker for this work and should probably happen sooner. it also has some relations with Sync that should be clarified with Kit.

Comment 45

[Milind L (:milindl)]

Attachment: Bug 1039200 Part 2 - Add implementation of new tagging API with unit tests,

This change adds several new API and changes the current API to
add tagging functionality to Bookmarks.jsm.
The following API are added (with a brief overview of their function):

1. Insert({..addTags:[]})
   This will add these tags along with the bookmark it is inserting.
   If the tag doesn't exist in the moz_tags database, it is added there.
   If the tag has a different case in moz_tags database, it is updated there.

2. InsertTree(...)
   The `tags` property on the various children is read and tag-url relations
   are made. Tags are added/replaced as in 1. if they don't exist/case is different.

3. Update({...addTags:[], removeTags:[]})
   This will add and remove the tags specified in these arrays to the bookmark.
   This will also update the tags-url relations when the `url` property is
   specified in the update information.
   This will remove tags from moz_tables which do not have any url association.

4. Remove()
   If any bookmark is removed, this method removes all tags associated with it,
   as long as the url does not have another bookmark associated with it.
   Any folder removal is also handled similarly in terms of bookmarks inside them.
   This will remove tags from moz_tables which do not have any url association.

5. Fetch(tags: [])
   By specifying a tags array, bookmarks can be fetched which are tagged with
   all the specified tags.

6. FetchTags(guidOrUrlOrNothing)
   This will fetch all tags or tags for a bookmark guid or a url.
   Note that internally, it uses urls, becase of the tag-url schema.

Review commit: https://reviewboard.mozilla.org/r/156500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156500/

Comment 46

[Milind L (:milindl)]

Comment on attachment 8873913 [details]
Bug 1039200 Part1 - Add migrations and associated test for tags and tags relation table,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145300/diff/3-4/

Comment 47

[Milind L (:milindl)]

> How do you plan to do this, considered the old API is synchronous while the
> new one is async?
> spinning the events loop?

Spinning the event loop sounds like something to be avoided, is that true?
Considering that this would be temporary measure for one cycle (since we can remove old API in new cycle) would this be feasible? Or is there some better way for doing this (in the short run)?

> IIRC both APIs may have an internal cache with a set of all the existing
> tags (to avoid having to query every time to know if a tag exists).

Ok, most recent patch has a cache (a Map from tags -> tagId) which I use quite a lot. It's fetched from the new tables however.

> Yes, there is another aspect that I don't recall if we already fixed
> completely that is replacing RESULTS_AS_TAG_CONTENTS queries with
> tags=list_of_tags queries. That is bug 1293445 that may indeed be a blocker
> for this work and should probably happen sooner. it also has some relations
> with Sync that should be clarified with Kit.

I will look into this and see if I can tackle this.

> Created attachment 8885650 [details]
> Bug 1039200 Part 2 - Add implementation of new tagging API with unit tests,

I have attached the new implementation. This is one of the larger patches I have submitted, since there were some cases which were not so trivial (mostly because of the tags-places relation as opposed to the tags-bookmarks relation).
I also updated existing unit tests + added two more to test auto-removal of tags in moz_tags (when no linking is there) and one for fetchTags.

Comment 48

[Marco Bonardo [:mak]]

Comment on attachment 8873913 [details]
Bug 1039200 Part1 - Add migrations and associated test for tags and tags relation table,

https://reviewboard.mozilla.org/r/145300/#review167766

::: toolkit/components/places/Database.cpp:2363
(Diff revision 4)
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  // Migrate old tags to new tags database
> +  // DOUBT: is there a better way to do this? It was using moz_bookmarks_root, but
> +  // we don't have it any more. Is using the tags folder guid directly OK?

Yes, using the guids directly is fine, they are immutable and set to a given value exactly so that we don't need a roots table anymore.

::: toolkit/components/places/Database.cpp:2378
(Diff revision 4)
> +    "INSERT OR IGNORE INTO moz_tags_relation (tag_id, place_id) "
> +      "SELECT (SELECT id FROM moz_tags WHERE tag = t.title), b.fk "
> +      "FROM moz_bookmarks b "
> +      "JOIN moz_bookmarks t ON t.id = b.parent "
> +      "JOIN moz_bookmarks p ON p.id = t.parent AND p.guid = 'tags________'"));
> +  NS_ENSURE_SUCCESS(rv, rv);

We should also remove the old tags and their contents from moz_bookmarks

::: toolkit/components/places/tests/migration/test_current_from_v39.js:20
(Diff revision 4)
> +  // Make sure the tables don't exist here already.
> +  Assert.ok(!(await db.tableExists("moz_tags")));
> +  Assert.ok(!(await db.tableExists("moz_tags_relation")));
> +
> +  await db.close();
> +});

you should do some raw queries on the db to manually add old-style tags, and then upgrade it and check the migration worked properly

::: toolkit/components/places/tests/migration/xpcshell.ini:27
(Diff revision 4)
>    places_v34.sqlite
>    places_v35.sqlite
>    places_v36.sqlite
>    places_v37.sqlite
>    places_v38.sqlite
> +  places_v39.sqlite

it should probably not be a straight copy, you should use the last version from here with a build of your version (so it gets upgraded), VACUUM it, then attach it.

Comment 49

[Marco Bonardo [:mak]]

We may need to break up this into smaller chunks so things may start landing, otherwise this is gigantic effort and it will just keep breaking.
1. We need to change queries to stop relating to item ids, bug 1293445. Without this we can't proceed because we won't have anymore itemIds of tag folders. It's a show-stopper.
2. We could land migration and change the existing code to use the new tables. No new APIs. This should be feasible and will already expose quite some interesting challenges to notify about tag changes.
3. Land the async API and try to have both APIs (sync and async) working at the same time. It needs some coordination across them.
4. Convert consumers to only use the async API.
5. Remove the sync API.

2 and 3 could likely be done at the same time, but it's not strictly necessary and may delay changes in the end.

Comment 50

[Marco Bonardo [:mak]]

Comment on attachment 8885650 [details]
Bug 1039200 Part 2 - Add implementation of new tagging API with unit tests,

https://reviewboard.mozilla.org/r/156500/#review167782

I'm clearing the review for now, since we're not ready for this yet.

It looks like a very good starting point though, kudos!

Comment 51

[Milind L (:milindl)]

(In reply to Marco Bonardo [::mak] from comment #49)
> We may need to break up this into smaller chunks so things may start
> landing, otherwise this is gigantic effort and it will just keep breaking.
> 1. We need to change queries to stop relating to item ids, bug 1293445.
> Without this we can't proceed because we won't have anymore itemIds of tag
> folders. It's a show-stopper.
> 2. We could land migration and change the existing code to use the new
> tables. No new APIs. This should be feasible and will already expose quite
> some interesting challenges to notify about tag changes.
> 3. Land the async API and try to have both APIs (sync and async) working at
> the same time. It needs some coordination across them.
> 4. Convert consumers to only use the async API.
> 5. Remove the sync API.
> 
> 2 and 3 could likely be done at the same time, but it's not strictly
> necessary and may delay changes in the end.

I think I will then apply comments on migration patch and do 2. after that (then we can land maybe).

After that I will use my existing patch to do 3 (it has async API in a mostly workable state with unit tests as well, but it's a rather large patch), but I need to do the coordination bits.After this it should be possible to (hopefully) convert everything to async.

5 will be done in next cycle I think, is that correct?

About 1, I read about it a bit, but I couldn't get an idea of how to solve that bug. I will see it again, and ask if I need some assistance solving that. I'll try to maybe see older bugs associated with the nsINavHistoryResultNode interface.

Thanks!

Comment 52

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #51)
> About 1, I read about it a bit, but I couldn't get an idea of how to solve
> that bug.

It needs a migration to rewrite any url like "place:type=7&folder=itemid" (where 7 is RESULTS_AS_TAG_CONTENTS) to "place:tags=mytag" (where mytag is the tag name for itemId). I'm not sure how to coordinate this with Sync though, so it may need some feedback from Kit Cambridge or someone from the Sync team, it may be enough to change the sync delta of the bookmarks pointing to the given urls, but I'm not totally sure how Sync handled uris changes, especially on queries (maybe queries are not synced at all?! Don't recall.).
And then it should change RESULTS_AS_TAG_QUERY to return these tags=NNN folders instead of the old format
http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/nsNavHistory.cpp#1893

Once done, it's likely there may be some fixes to do for the UI to work as usual, these new queries aren't very well tested yet (thus is may require checking tests and eventually write some new test).

One problem that came to my mind, for example, is that the selection from tags queries should probably be independent from the casing of the tag in the query (so if the query is place:tags=MyTag, it should also return things tagged with "mytag").

Comment 53

[Lina Butler [:lina]]

I'm excited this is happening, separating tags and bookmarks should simplify tag sync. (We do sync all queries, so we need to think about how to handle this). We can probably make it work in a backward-compatible way, though.

I'm thinking we can rewrite outgoing queries to use the old "place:type=7&folder=1" syntax, so that versions older than 57, or whenever this patch lands, can understand them. The folder ID in the query doesn't matter, since we also include the tag name in the record: https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/PlacesSyncUtils.jsm#1683-1698.

We then change `updateTagQueryFolder` to rewrite incoming queries to "place:tags=..." (https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/PlacesSyncUtils.jsm#902-915) before storing them, and remove `getOrCreateTagFolder`.

Once 57 hits release, we can stop rewriting outgoing queries, and keep rewriting incoming ones. In the future, we can remove the rewriting entirely.

WDYT?

Comment 54

[Marco Bonardo [:mak]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #53)
> I'm thinking we can rewrite outgoing queries to use the old
> "place:type=7&folder=1" syntax, so that versions older than 57, or whenever
> this patch lands, can understand them. The folder ID in the query doesn't
> matter, since we also include the tag name in the record:
> https://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/
> PlacesSyncUtils.jsm#1683-1698.

how do we rewrite queries, if we move the tags to another table? We lose that information if we move the data. The only way would be to keep having duped data across the 2 tables, and that seems to complicate things quite a bit.

Note that here the problem is just queries that the user created by himself, the queries in the Library should not be synced. Considered a few users use tags, and out of those probably only a tiny percentage knows they can copy tag queries around, I wonder if we could just let these break and leave to those users the burden to re-create those few copies...

Comment 55

[Lina Butler [:lina]]

(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #54)
> (In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #53)
> > I'm thinking we can rewrite outgoing queries to use the old
> > "place:type=7&folder=1" syntax, so that versions older than 57, or whenever
> > this patch lands, can understand them. The folder ID in the query doesn't
> > matter, since we also include the tag name in the record:
> > https://searchfox.org/mozilla-central/rev/
> > 09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/
> > PlacesSyncUtils.jsm#1683-1698.
> 
> how do we rewrite queries, if we move the tags to another table? We lose
> that information if we move the data.

Oh, we don't actually need the outgoing query URL to reference a real folder, because Sync will rewrite it when it's downloaded. The outgoing URL can literally be a static string ("place:type=7&folder=1"), for every tag query. Old versions of Firefox will rewrite to point to the correct tag folder; new versions can rewrite to point directly to the tag.

It's pretty simple to handle, but I'd also be OK with breaking this because custom tag queries are so uncommon. (Sync also doesn't rewrite other `folder` queries, so those are already broken today).

Comment 56

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:mak, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 57

[Marco Bonardo [:mak]]

Fixing this will require some kind of coordination and planning because we must change both the data layout and the API (See bug 1448885), so it may be worth creating a project covering also Bug 1172828