1059256 - More powerful tagging transactions

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

All of the following should work:

pesudo-code:

Tag( tag: foo, uri: single uri )
Tag( tag: foo, uris: [ multiple uris ] )
Tag( tags: ["foo", "bar"], uri: single uri )
Tag( tags: ["foo", "bar"], uris: [ multiple uris ] )

Untag( tag: foo, uri: single uri )
Untag( tag: foo, uris: [ multiple uris ] )
Untag( tags: ["foo", "bar"], uri: single uri )
Untag( tags: ["foo", "bar"], uris: [ multiple uris ] )

Untag( uri: single uri )  // remove all tags for this uri
Untag( uris: [ multiple uris ] )

Untag( tag: foo ) // remove all uris "foo"-tagged
Untag( tags: ["foo", "bar"] )

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please add this to the iteration? It's basically split off of bug 984903 and blocks it.

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 34.3

Comment 3

[Asaf Romano (gone)]

I gave up on:

Untag( tag: foo ) // remove all uris "foo"-tagged
Untag( tags: ["foo", "bar"] )

at least for now.

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8480370 [details] [diff] [review]
patch

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

not r+ cause it's using some sync methods when we have async alternatives.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +1242,5 @@
>  
>  /**
>   * Transaction for tagging a URI.
>   *
>   * Required Input Properties: uri, tags.

uris

@@ +1249,5 @@
> +PT.Tag.prototype = {
> +  execute: function* (aURIs, aTags) {
> +    let onUndo = [], onRedo = [];
> +    for (let uri of aURIs) {
> +      let currentURI = uri;

why not just for (let currentURI of aURIs) ?
you don't use uri anymore later...

@@ +1250,5 @@
> +  execute: function* (aURIs, aTags) {
> +    let onUndo = [], onRedo = [];
> +    for (let uri of aURIs) {
> +      let currentURI = uri;
> +      if (PlacesUtils.getMostRecentBookmarkForURI(currentURI) == -1) {

argh no, this is terribly synchronous... can you use asyncGetBookmarkIds?

@@ +1251,5 @@
> +    let onUndo = [], onRedo = [];
> +    for (let uri of aURIs) {
> +      let currentURI = uri;
> +      if (PlacesUtils.getMostRecentBookmarkForURI(currentURI) == -1) {
> +        // Tagging is only allowed for bookmarked URIs.

but we are going to change that. Please annotate that here pointing to bug 424160. We must be sure this code won't make assumption on the fact only bookmarks might be tagged (clearly we'll change it in future when it's the time)

@@ +1289,5 @@
>  
>  /**
>   * Transaction for removing tags from a URI.
>   *
>   * Required Input Properties: uri.

uris

@@ +1299,5 @@
> +PT.Untag.prototype = {
> +  execute: function* (aURIs, aTags) {
> +    let onUndo = [], onRedo = [];
> +    for (let uri of aURIs) {
> +      let currentURI = uri;

ditto

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +1080,5 @@
> +  let bm_info_a = { uri: NetUtil.newURI("http://bookmarked.uri")
> +                  , parentGUID: yield PlacesUtils.promiseItemGUID(root) };
> +  let bm_info_b = { uri: NetUtil.newURI("http://bookmarked2.uri")
> +                  , parentGUID: yield PlacesUtils.promiseItemGUID(root) };
> +  let unbookmarked_uri = NetUtil.newURI("http://un.bookmSarked.uri");

I guess bookmSarked is a typo, not that it matters...

@@ +1093,5 @@
> +    let tags = "tag" in aInfo ? [aInfo.tag] : aInfo.tags;
> +
> +    let tagWillAlsoBookmark = new Set();
> +    for (let uri of uris) {
> +      if (!PlacesUtils.bookmarks.isBookmarked(uri)) {

not that it matters much but I'd prefer if this would use an async method like asyncGetBookmarkIds, so in future we have less sync calls to convert

@@ +1101,5 @@
> +
> +    function ensureTagsSet() {
> +      for (let uri of uris) {
> +        ensureTagsForURI(uri, tags);
> +        Assert.notEqual(PlacesUtils.getMostRecentBookmarkForURI(uri), -1);

asyncGetBookmarkIds

@@ +1108,5 @@
> +    function ensureTagsUnset() {
> +      for (let uri of uris) {
> +        ensureTagsForURI(uri, []);
> +        if (tagWillAlsoBookmark.has(uri))
> +          Assert.equal(PlacesUtils.getMostRecentBookmarkForURI(uri), -1);

asyncGetBookmarkIds

@@ +1110,5 @@
> +        ensureTagsForURI(uri, []);
> +        if (tagWillAlsoBookmark.has(uri))
> +          Assert.equal(PlacesUtils.getMostRecentBookmarkForURI(uri), -1);
> +        else
> +          Assert.notEqual(PlacesUtils.getMostRecentBookmarkForURI(uri), -1);

asyncGetBookmarkIds

@@ +1168,5 @@
> +      uris = "uri" in aInfo ? [aInfo.uri] : aInfo.uris;
> +      tagsRemoved = "tag" in aInfo ? [aInfo.tag] : aInfo.tags;
> +    }
> +
> +    let preRemovealTags = new Map();

typo Removeal

@@ +1203,5 @@
> +  yield doTest(bm_info_a.uri);
> +  yield doTest(bm_info_b.uri);
> +  yield doTest([bm_info_a.uri, bm_info_b.uri]);
> +  yield doTest({ uris: [bm_info_a.uri, bm_info_b.uri], tags: ["A", "B"] });
> +  yield doTest({ uris: [bm_info_a.uri, bm_info_b.uri], tag: "B" });

might you also test removing a non existing tag?

Comment 6

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #5)

> > +    for (let uri of aURIs) {
> > +      let currentURI = uri;
> 
> why not just for (let currentURI of aURIs) ?
> you don't use uri anymore later...

Short answer: bug 449811. I explained this in details in bug 1059257 comment 7.

Comment 7

[Asaf Romano (gone)]

No QA can be done here. However, QA for bug 984903 "as a whole" would cover this too.

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/909d55ee5d7e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/909d55ee5d7e