Closed Bug 755758 Opened 8 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set

Tracking

(seamonkey2.9 wontfix, seamonkey2.10 wontfix, seamonkey2.11 wontfix, seamonkey2.12 wontfix, seamonkey2.15 fixed)

RESOLVED FIXED
seamonkey2.15
Tracking Status
seamonkey2.9 --- wontfix
seamonkey2.10 --- wontfix
seamonkey2.11 --- wontfix
seamonkey2.12 --- wontfix
seamonkey2.15 --- fixed

People

(Reporter: sgautherie, Assigned: ewong)

References

Details

Attachments

(2 files, 7 obsolete files)

27.95 KB, patch
ewong
: review+
Details | Diff | Splinter Review
820 bytes, patch
neil
: review+
Details | Diff | Splinter Review
No description provided.
No longer blocks: 732027
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #641401 - Flags: review?(neil)
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?
Attachment #641401 - Flags: feedback?(mak77)
(In reply to neil@parkwaycc.co.uk 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.
Attachment #641401 - Flags: feedback?(mak77)
(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.
(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 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.
Attachment #641401 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk 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 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.
Attachment #648951 - Flags: review?(neil) → review-
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 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.
Attachment #649160 - Flags: review?(neil) → review-
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.
Attachment #651250 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk 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.
(In reply to neil@parkwaycc.co.uk 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 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 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.
Attachment #655262 - Flags: review?(neil) → review+
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.
Depends on: 794279
Blocks: 794279
No longer depends on: 794279
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/8a26f31333b1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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"
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?
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.
Attached patch Fix broken testSplinter Review
ewong: You forgot to increment the loop counter (copy&paste error?) :-)
Attachment #668954 - Flags: review?(neil)
Attachment #668954 - Attachment description: Patch → Fix broken test
Attachment #668954 - Flags: review?(neil) → review+
Settings flags to make tracking of fixed bugs easier.
Target Milestone: --- → seamonkey2.15
You need to log in before you can comment on or make changes to this bug.