Closed Bug 1059256 Opened 6 years ago Closed 6 years ago

More powerful tagging transactions

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla35
Iteration:
35.2

People

(Reporter: mano, Assigned: mano)

References

Details

Attachments

(1 file, 1 obsolete file)

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"] )
Iteration: --- → 34.3
Points: --- → 5
Marco, can you please add this to the iteration? It's basically split off of bug 984903 and blocks it.
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
OS: Mac OS X → All
Hardware: x86 → All
Flags: qe-verify+
QA Contact: andrei.vaida
Summary: More powerfull tagging transactions → More powerful tagging transactions
Added to IT 34.3
Flags: needinfo?(mmucci) → firefox-backlog+
I gave up on:

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

at least for now.
Attached patch patch (obsolete) — Splinter Review
Attachment #8480370 - Flags: review?(mak77)
Iteration: 34.3 → 35.1
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?
Attachment #8480370 - Flags: review?(mak77) → feedback+
(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.
No QA can be done here. However, QA for bug 984903 "as a whole" would cover this too.
Flags: qe-verify+ → qe-verify-
Attached patch patchSplinter Review
Attachment #8480370 - Attachment is obsolete: true
Attachment #8489958 - Flags: review?(mak77)
Iteration: 35.1 → 35.2
Attachment #8489958 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/909d55ee5d7e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.