755758 - Port |Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entitities| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Comment 1

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v1)

Comment 2

[[e-mail]]

Comment on attachment 641401 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v1)

>+  get _copyableAnnotations() [
>+    this.DESCRIPTION_ANNO,
>+    this.LOAD_IN_SIDEBAR_ANNO,
>+    PlacesUtils.POST_DATA_ANNO,
>+    PlacesUtils.READ_ONLY_ANNO,
>+  ],
This creates a new array each time, doesn't it? I'd prefer _copyableAnnotations: [ .. ], instead.

>+   _getURIItemCopyTransaction:
What's the different between a URI item and a bookmark item? Since this ends up creating a bookmark, I'd rather it was called _getBookmarkItemCopyTransaction.

>+     if (aData.dateAdded) {
>+       transactions.push(
>+         new PlacesEditItemDateAddedTransaction(null, aData.dateAdded)
>+       );
>+     }
[Consistency nit: Extra {}s weren't added anywhere else. See below for example.]

[Existing "bug" noticed due to code churn?]
>         if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
>+          if (node.livemark && node.annos)
>+            transactions.push(
>+              PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
>+            );
>           else
>+            transactions.push(
>+              PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
>+            );
>         }
_getFolderCopyTransaction special-cases being called on a livemark, so I don't see the need to special-case it again here.
What's the difference between a folder with two annotations and a livemark?

Comment 3

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #2)
> >+  get _copyableAnnotations() [
> >+    this.DESCRIPTION_ANNO,
> >+    this.LOAD_IN_SIDEBAR_ANNO,
> >+    PlacesUtils.POST_DATA_ANNO,
> >+    PlacesUtils.READ_ONLY_ANNO,
> >+  ],
> This creates a new array each time, doesn't it? I'd prefer
> _copyableAnnotations: [ .. ], instead.

the getter contents are unmodifiable, while your suggestion is not, though we may define a const array in the module scope and return it here.

> >+   _getURIItemCopyTransaction:
> What's the different between a URI item and a bookmark item? Since this ends
> up creating a bookmark, I'd rather it was called
> _getBookmarkItemCopyTransaction.

the point is that it may copy "either a bookmark or an history item" TO a bookmark folder (the destination is always a bookmark folder, since others are readonly, the source may be anything), as the javadoc says. URIItem just means "bookmark or history item".

> >+     if (aData.dateAdded) {
> >+       transactions.push(
> >+         new PlacesEditItemDateAddedTransaction(null, aData.dateAdded)
> >+       );
> >+     }
> [Consistency nit: Extra {}s weren't added anywhere else. See below for
> example.]

not sure what you mean, but this is multiline so should be braced (according to the guide also onelines, but we're not strict on that). Ideally we should fix the other places, but... whatever.

> [Existing "bug" noticed due to code churn?]
> >         if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
> >+          if (node.livemark && node.annos)
> >+            transactions.push(
> >+              PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
> >+            );
> >           else
> >+            transactions.push(
> >+              PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
> >+            );
> >         }
> _getFolderCopyTransaction special-cases being called on a livemark, so I
> don't see the need to special-case it again here.
> What's the difference between a folder with two annotations and a livemark?

good catch, indeed this if/else can be removed.

Comment 4

[[e-mail]]

(In reply to Marco Bonardo from comment #3)
> (In reply to comment #2)
> > >+  get _copyableAnnotations() [
> > >+    this.DESCRIPTION_ANNO,
> > >+    this.LOAD_IN_SIDEBAR_ANNO,
> > >+    PlacesUtils.POST_DATA_ANNO,
> > >+    PlacesUtils.READ_ONLY_ANNO,
> > >+  ],
> > This creates a new array each time, doesn't it? I'd prefer
> > _copyableAnnotations: [ .. ], instead.
> the getter contents are unmodifiable, while your suggestion is not, though
> we may define a const array in the module scope and return it here.
I thought the _ meant that the array was private to the module anyway...

> the point is that it may copy "either a bookmark or an history item" TO a
> bookmark folder (the destination is always a bookmark folder, since others
> are readonly, the source may be anything), as the javadoc says. URIItem just
> means "bookmark or history item".
Thanks for explaining it so well.

Comment 5

[[e-mail]]

(In reply to comment #2)
> (From update of attachment 641401 [details] [diff] [review])
> >+  get _copyableAnnotations() [
> >+    this.DESCRIPTION_ANNO,
> >+    this.LOAD_IN_SIDEBAR_ANNO,
> >+    PlacesUtils.POST_DATA_ANNO,
> >+    PlacesUtils.READ_ONLY_ANNO,
> >+  ],
> This creates a new array each time, doesn't it?
It refers to this, which you can't easily do otherwise. Sigh...

Comment 6

[[e-mail]]

Comment on attachment 641401 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v1)

OK, so as previously discussed:
>+     if (aData.dateAdded) {
>+       transactions.push(
>+         new PlacesEditItemDateAddedTransaction(null, aData.dateAdded)
>+       );
>+     }
Remove the {}s

>+     if (aData.lastModified) {
>+       transactions.push(
>+         new PlacesEditItemLastModifiedTransaction(null, aData.lastModified)
>+       );
>+     }
Remove the {}s

>         if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
>+          if (node.livemark && node.annos)
>+            transactions.push(
>+              PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
>+            );
>           else
index);
>+            transactions.push(
>+              PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
>+            );
>         }
>         else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
  transactions.push(
    PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
  );
else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)

Bonus points for making _copyableAnnotations a lazy getter.

Comment 7

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #4)
> (In reply to Marco Bonardo from comment #3)
> > (In reply to comment #2)
> > > >+  get _copyableAnnotations() [
> > > >+    this.DESCRIPTION_ANNO,
> > > >+    this.LOAD_IN_SIDEBAR_ANNO,
> > > >+    PlacesUtils.POST_DATA_ANNO,
> > > >+    PlacesUtils.READ_ONLY_ANNO,
> > > >+  ],
> > > This creates a new array each time, doesn't it? I'd prefer
> > > _copyableAnnotations: [ .. ], instead.
> > the getter contents are unmodifiable, while your suggestion is not, though
> > we may define a const array in the module scope and return it here.
> I thought the _ meant that the array was private to the module anyway...

yes, though that's a suggestion that unfortunately doesn't enforce anything on add-ons... not that I think we are active clever in other parts...

Comment 8

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v2)

Comment 9

[[e-mail]]

Comment on attachment 648951 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v2)

>+  XPCOMUtils.defineLazyGetter(PlacesUIUtils, "_copyableAnnotations", function() {
>+    return [this.DESCRIPTION_ANNO,
>+            this.LOAD_IN_SIDEBAR_ANNO,
>+            PlacesUtils.POST_DATA_ANNO,
>+            PlacesUtils.READ_ONLY_ANNO]
>+  }); 
[Nit: trailing whitespace on that last line]
This needs to go near the end of the file with the other lazy getters.

>+        if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
>+          transactions.push(
>+            PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
>+          );
Unfortunately you deleted the wrong line, you were supposed to delete this line and keep the _getFolderCopyTransaction line.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v3)

Comment 11

[[e-mail]]

Comment on attachment 649160 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v3)

>   _getFolderCopyTransaction:
>   function PUIU__getFolderCopyTransaction(aData, aContainer, aIndex) {
Something here must be wrong because I can't seem to copy a folder but I don't see any exceptions in the Error Console...

Comment 12

[[e-mail]]

Comment on attachment 649160 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v3)

>+        if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
>+          transactions.push(
>+            PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
>+          );
>+        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
>+          transaction.push(new PlacesCreateSeparatorTransaction(-1, index));
>+        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
>+          transaction.push(
>+            PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index)
>+          );
These all need to say transactions.push but only the first one is right.
Also, _getBookmarkItemCopyTransaction needs to get renamed back to _getURIItemCopyTransaction (sorry to confuse you there).

+      if (aContainer == PlacesUtils.tagsFolderId) { // Copying a tag folder.
+        let transactions = [];
+        if (aData.children) {
+          aData.children.forEach(function(aChild) {
+            transactions.push(
+              new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
+                                          [aData.title])          
+            );
+          });
         }
+        return this.ptm.aggregateTransactions("addTags", transactions);
+      }
 
+      if (aData.livemark && aData.annos)  //Copying a livemark.
+        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
     }
 
-    // tag folders use tag transactions
-    if (aContainer == PlacesUtils.tagsFolderId) {
-      var txns = [];
-      if (aData.children) {
-        aData.children.forEach(function(aChild) {
-          txns.push(
-            new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
-                                        [aData.title])          
-          );
-        }, this);
-      }
-      return this.ptm.aggregateTransactions("addTags", txns);
There's something seriously wrong here, this shouldn't have been reindented. Also the last line doesn't seem to have been updated.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v4)

Comment 14

[[e-mail]]

Comment on attachment 651250 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v4)

Did you miss this because my quoting went wrong last time?
>+      if (aContainer == PlacesUtils.tagsFolderId) { // Copying a tag folder.
>+        let transactions = [];
>+        if (aData.children) {
>+          aData.children.forEach(function(aChild) {
>+            transactions.push(
>+              new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
>+                                          [aData.title])          
>+            );
>+          });
>         }
>-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
>-          txn = new PlacesCreateSeparatorTransaction(-1, index);
>-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
>-          txn = PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index);
>+        return this.ptm.aggregateTransactions("addTags", transactions);
>+      }
> 
>-        if (txn)
>-          childItemsTransactions.push(txn);
>-        else
>-          throw("Unexpected item under a bookmarks folder");
>-      }
>-      return childItemsTransactions;
>+      if (aData.livemark && aData.annos)  //Copying a livemark.
>+        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
>     }
> 
>-    // tag folders use tag transactions
>-    if (aContainer == PlacesUtils.tagsFolderId) {
>-      var txns = [];
>-      if (aData.children) {
>-        aData.children.forEach(function(aChild) {
>-          txns.push(
>-            new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
>-                                        [aData.title])          
>-          );
>-        }, this);
>-      }
>-      return this.ptm.aggregateTransactions("addTags", txns);
Somehow the tag folders and livemark copying code got indented and added into the previous block of code, which is incorrect. Also, the last line that I quoted above neeeds to be updated as per bug 629620.

Comment 15

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #14)
> Comment on attachment 651250 [details] [diff] [review]
> Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since
> they are new entities| to SeaMonkey (v4)
> 
> Did you miss this because my quoting went wrong last time?

Mainly because the {} placement is confusing me.  I'm  having some
difficulties in checking which code belongs to which block.

Comment 16

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #14)

> >-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
> >-          txn = new PlacesCreateSeparatorTransaction(-1, index);
> >-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
> >-          txn = PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index);
> >+        return this.ptm.aggregateTransactions("addTags", transactions);
> >+      }
> > 
> >-        if (txn)
> >-          childItemsTransactions.push(txn);
> >-        else
> >-          throw("Unexpected item under a bookmarks folder");
> >-      }
> >-      return childItemsTransactions;
> >+      if (aData.livemark && aData.annos)  //Copying a livemark.
> >+        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
> >     }
> > 
> >-    // tag folders use tag transactions
> >-    if (aContainer == PlacesUtils.tagsFolderId) {
> >-      var txns = [];
> >-      if (aData.children) {
> >-        aData.children.forEach(function(aChild) {
> >-          txns.push(
> >-            new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
> >-                                        [aData.title])          
> >-          );
> >-        }, this);
> >-      }
> >-      return this.ptm.aggregateTransactions("addTags", txns);
> Somehow the tag folders and livemark copying code got indented and added
> into the previous block of code, which is incorrect. Also, the last line
> that I quoted above neeeds to be updated as per bug 629620.

Do you mean :

  return this.ptm.aggregateTransactions("addTags", transactions); ?

If so, isn't it in the above code? i.e:

> >-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
> >-          txn = PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index);
> >+        return this.ptm.aggregateTransactions("addTags", transactions);
> >+      }

or am I misunderstanding?

Comment 17

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v5)

Comment 18

[[e-mail]]

Comment on attachment 655262 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v5)

This is looking much better although I think I may have spotted a typo.

Comment 19

[[e-mail]]

Comment on attachment 655262 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v5)

>+      return new PlacesAggregatedTransactions("addTags", transactions);
Typo: PlacesAggregatedTransaction
r=me with that fixed.

>     }
>+    if (aData.livemark && aData.annos)  //Copying a livemark.
Nit: could put a blank line before this if.

Comment 20

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entities| to SeaMonkey (v6)

Comment 21

[[e-mail]]

Comment on attachment 656844 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entities| to SeaMonkey (v6)

Whoops, only just noticed this:

>+    if (aData.livemark && aData.annos)  //Copying a livemark.
>       return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
>     }
Mismatched } - I assume you meant to write
if (aData.livemark && aData.annos) { // Copying a livemark.
Or you could delete the } but you would still want to fix up the spaces.

Comment 22

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v7)

Comment 23

[Edmund Wong (:ewong)]

Attachment: Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v8)

Added space after //.

Comment 24

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/8a26f31333b1

Comment 25

[Frank Wein [:mcsmurf]]

I think this bug fix/test has to do with the recent test problem on SeaMonkey build bot. See http://tbpl.drapostles.org/?tree=SeaMonkey Win debug XPCShelltest debug:
"TEST-INFO | e:\builds\slave\test\build\xpcshell\tests\suite\common\places\tests\unit\test_PUIU_makeTransaction.js | running test ...

command timed out: 1200 seconds without output, attempting to kill
SIGKILL failed to kill process
using fake rc=-1
program finished with exit code -1"

Comment 26

[Frank Wein [:mcsmurf]]

ewong: Do you have time to look at this test problem? If not, we should back out your patch and/or fix the test. This patch isn't critical for the next release as far as I see this?

Comment 27

[Frank Wein [:mcsmurf]]

ewong: To help you a bit (not sure if you own Windows), the test gets stuck here:

 waitForBookmarkNotification("onItemAdded", function(aData) (line 122)

The tests before this line in this test run fine.

Comment 28

[Frank Wein [:mcsmurf]]

Attachment: Fix broken test

ewong: You forgot to increment the loop counter (copy&paste error?) :-)

Comment 29

[Frank Wein [:mcsmurf]]

Comment on attachment 668954 [details] [diff] [review]
Fix broken test

Checked in: https://hg.mozilla.org/comm-central/rev/0d68cf5ac488

Comment 30

[Frank Wein [:mcsmurf]]

Settings flags to make tracking of fixed bugs easier.