If Unfiled Bookmarks is moved to Bookmarks Menu, unfiled bookmarks are invisible until browser is restarted

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
minor
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Tom, Assigned: dietrich)

Tracking

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

If the Unfiled Bookmarks folder is moved into the Bookmarks Menu folder, bookmarks in the Unfiled folder cannot be viewed via the Bookmarks Menu until the browser has been restarted.

Reproducible: Always

Steps to Reproduce:
1. Make sure "Unfiled Bookmarks" is under "All Bookmarks" (in the default place), and start the browser.
2. Use the star button to make an unfiled bookmark
3. Move the "Unfiled Bookmarks" folder into the "Bookmarks Menu" folder
Actual Results:  
Try to use the bookmarks menu to view what is in the Unfiled Bookmarks folder - the Unfiled Bookmarks folder appears empty.

Expected Results:  
You should be able to view what is in the "Unfiled Bookmarks" folder by using the Bookmarks menu.

Once the browser has been restarted, bookmarks appear correctly when viewing the unfiled bookmarks folder in this way, even if you add new ones, so there is no longer a problem.

The problem can be reproduced by moving the Unfiled Bookmarks folder back into "All Bookmarks", and then back in to "Bookmarks Menu" again.

If it is not moved again the problem does not seem to re-occur.
Component: Bookmarks → Places
QA Contact: bookmarks → places
Version: unspecified → Trunk
(Assignee)

Comment 1

10 years ago
(In reply to comment #0)
> 3. Move the "Unfiled Bookmarks" folder into the "Bookmarks Menu" folder

Hrm, here's the bug. You should not be able to do this. You should be able to copy, but not move it.

I can accomplish this by clicking on All Bookmarks in the organizer, then drag-and-dropping Unfiled on the Menu folder in the right-side pane.

Drivers: Requesting blocking. The special folders should not be able to moved, only copied. And maybe not even copied, since the action would not result in what the user expected.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
OS: Windows Vista → All
Priority: -- → P2
Hardware: PC → All
(Reporter)

Comment 2

10 years ago
Hm, thanks for the reply. Glad I was able to find an actual bug!

However, may I suggest that it be made so that you can do this, as it makes things much easier if you're able to quickly get to bookmarks via the bookmarks menu which you may have quickly saved as unfiled.

The full bookmark manager is great for filing them later, but I would not want to have to keep accessing that in order to find things that I've bookmarked with the star button and just not got around to filing yet. That is why I moved it to the Bookmarks Menu folder in the first place, as it's much quicker to get to them that way.

As it can already be moved there and does work properly (if the browser is restarted after it is moved), I think it would be more beneficial to allow that to happen if that is where the user wants it. Obviously better if you don't need to restart the browser before they show up properly though.

Suggestion: Allow the special folder to be moved so it can be accessed from the bookmarks menu if the user wants it to be there, but agree that copying (not moving) is a bad idea due to the action perhaps not resulting in what was expected.

Comment 3

10 years ago
I was just about to report the same bug. Actually, the same behavior happens when moving the Unfiled Bookmarks item to the Bookmarks Toolbar. I guess that different people trying to move the Unfilled Bookmarks item means it's a feature we want, so please don't block it :)

(In reply to comment #0)
> Drivers: Requesting blocking. The special folders should not be able to moved,
> only copied. And maybe not even copied, since the action would not result in
> what the user expected.
Blocking - either this should be immovable, or movable and working. :)
Flags: blocking-firefox3? → blocking-firefox3+
(Reporter)

Comment 5

10 years ago
If anything is done, ideally movable and working in my opinion for the reasons described above.

Comment 6

10 years ago
Do people really need/want to "move the root", or is putting a places:folder=n query there sufficiently functional? Or do those two amount to the same thing?

Comment 7

10 years ago
If a places:folder=n query is implemented, it would make much more sense than "moving the whole root" and would be more consistent.
(Assignee)

Updated

10 years ago
Assignee: nobody → dietrich

Updated

10 years ago
Whiteboard: [needs status update]
(Assignee)

Comment 8

10 years ago
Created attachment 313500 [details] [diff] [review]
conservative fix

This fix disallows dragging of any of the "special" roots, tag containers and the All Bookmarks folder. It does allow dnd reordering of tag containers within the Tags folder.

The change fixes this bug, bug 426805 and bug 426846.

There are a number of useful dnd interactions possible in the Library. Until these interactions are spec'd, it's better to conservatively disable dnd of these folders. Primum non nocere!

Regarding comment #6 and #7: drag-to-shortcut for roots sounds like it might be workable, please file an enhancement bug for that.
Attachment #313500 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [needs status update] → [has patch][needs review mano]
(Assignee)

Updated

10 years ago
Target Milestone: --- → Firefox 3
(Reporter)

Comment 9

10 years ago
Nice work on the fix. Was hoping that we would still be able to move it into the bookmarks menu folder, it really does make the unfiled bookmarks a lot easier to access, but since there's a number of bugs with dnd of these folders I suppose this way makes sense for now. Hopefully this will be considered for future tweaks/changes.
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> Nice work on the fix. Was hoping that we would still be able to move it into
> the bookmarks menu folder, it really does make the unfiled bookmarks a lot
> easier to access, but since there's a number of bugs with dnd of these folders
> I suppose this way makes sense for now. Hopefully this will be considered for
> future tweaks/changes.
> 

Please file an enhancement request for this.

See bug 423617 for changing the default location of newly starred+unfiled bookmarks, which might address your concern without needing to re-arrange the special folders.

Comment 11

10 years ago
A similar problem (but with copying, not moving):

1. Create a new profile, since it is dangerous in a way.
2. Go to Library.
3. Copy “All Bookmarks”.
4. Paste it into “Unfiled Bookmarks” or another subfolder of “All Bookmarks”.

Now you have an infinite recursive tree you cannot just delete or cut.

Comment 12

10 years ago
(In reply to comment #11)
> A similar problem (but with copying, not moving):
Confirmed as bug 426805 (also causes a temporary hang).
(Assignee)

Updated

10 years ago
Blocks: 426846
Comment on attachment 313500 [details] [diff] [review]
conservative fix

please set "roots" and isTagContainer outside of the loop.

r=mano.
Attachment #313500 - Flags: review?(mano) → review+
(Assignee)

Comment 14

10 years ago
Created attachment 314163 [details] [diff] [review]
fix v2

There were several scenarios not covered in the previous patch. Here's a more complete fix:

- address copy
- fix for items w/o concreteId property (eg: All Bookmarks)
- disallow for all immediate children of the left pane folder
Attachment #313500 - Attachment is obsolete: true
Attachment #314163 - Flags: review?(mano)
Comment on attachment 314163 [details] [diff] [review]
fix v2

For reasons given on #places.
Attachment #314163 - Flags: review?(mano) → review-
(Assignee)

Comment 16

10 years ago
Created attachment 314249 [details] [diff] [review]
fix v3

implementing a test for the fix we talked about over irc exposed a bug and a reproduce-able crash scenario.

the bug is that when a query result contains a folder shortcut, and that folder shortcut is deleted, the notification sends the item id, which doesn't match any in the result node (since it needs to match on the query item id).

the crash occurs if we assert when not finding the node. changing that to warn stopped the crashing. i'll file a followup to figure out why asserting there causes xpcshell to crash (stack below).

changes in the new patch:

- remove hasCopyableSelection, etc
- add support to force copying of shortcuts as folders
- fix FindChildById() to support folder shortcuts
- add a test that flexes forcing copy for shortcuts, as well as making sure we still properly handle non-folder-shortcut queries (i'll rename this on check-in)
- s/NS_NOTREACHED/NS_WARNING/ to stop the crashing during the test

ftr, i still do not like that we're allowing copy of special folders. it will come to no good.

###!!! ASSERTION: Removing item we don't have: 'node', file /Users/dietrich/moz/trunk/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 3371
nsPrintfCString::~nsPrintfCString()+0x00022485 [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libplaces.dylib +0x0005C6C3]
nsPrintfCString::~nsPrintfCString()+0x000212EE [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libplaces.dylib +0x0005B52C]
nsXPCOMCycleCollectionParticipant::nsXPCOMCycleCollectionParticipant()+0x00012B00 [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libplaces.dylib +0x00075472]
NS_InvokeByIndex_P+0x00000063 [/Users/dietrich/moz/trunk/build-dbg/xpcom/build/libxpcom_core.dylib +0x0009EFFB]
nsSupportsArray::AppendElement(nsISupports*)+0x0000540E [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libxpconnect.dylib +0x0004EAEE]
nsCRT::free(unsigned short*)+0x00004FD9 [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libxpconnect.dylib +0x000590D9]
js_Invoke+0x00000A21 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x00077383]
JS_CompareValues+0x0000F2A4 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x0006A348]
js_Invoke+0x00001324 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x00077C86]
JS_ExecuteScript+0x00000083 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x000172A1]
start+0x000017A0 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x00002D58]
start+0x00001AD7 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x0000308F]
start+0x00002131 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000036E9]
start+0x00003343 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000048FB]
start+0x000000FC [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000016B4]
start+0x00000029 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000015E1]
Attachment #314163 - Attachment is obsolete: true
Attachment #314249 - Flags: review?(mano)
(Assignee)

Comment 17

10 years ago
i filed bug 427793 for the xpcshell crash. the same conditions do not exhibit a crash when running the browser, so ignore the NS_WARNING change in this patch.
Status: NEW → ASSIGNED
Comment on attachment 314249 [details] [diff] [review]
fix v3

>-    
>-      // The following loop disallows the dropping of a folder on itself or
>-      // on any of its descendants.
>+
>       if (dragged.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER ||
>           /^place:/.test(dragged.uri)) {
>+
>+        var id = dragged.concreteId || dragged.id;

This seems very wrong over here.

>+  /**
>+   * Determines if a container can be moved or copied.
>+   * 
>+   * @param aContainerId
>+   *        The bookmark folder id.
>+   * @param aInsertionPoint
>+   *        The insertion point of the drop target.

The second parameter seems to be optional, document that.

>+   */
>+  canMoveOrCopyContainer:
>+  function PCDH_canMoveOrCopyContainer(aContainerId, aInsertionPoint) {
>+    // Disallow copying and dragging of roots
>+    var roots = [PlacesUtils.placesRootId, PlacesUtils.bookmarksMenuFolderId,
>+                 PlacesUtils.tagsFolderId, PlacesUtils.unfiledBookmarksFolderId,
>+                 PlacesUtils.toolbarFolderId];

nit: const.

>+    if (roots.indexOf(aContainerId) != -1)
>+      return false;
>+
>+    var parentId = PlacesUtils.bookmarks.getFolderIdForItem(aContainerId)
>+

I wonder if it'd be better to pass the dragged node to this method rather than itemIds.

>+    // Disallow dragging immediate children of the left pane folder
>+    // and the All Bookmarks folder.
>+    if (parentId == PlacesUIUtils.leftPaneFolderId ||
>+        parentId == PlacesUIUtils.allBookmarksFolderId)
>+      return false;
>+
>+    // Disallow dragging tag containers unless they're being moved
>+    // in the Tags folder.
>+    if (aInsertionPoint && parentId == PlacesUtils.tagsFolderId &&
>+        aInsertionPoint.itemId != PlacesUtils.tagsFolderId)
>+      return false;
>+

hrm, shouldn't we return false in this case if there's no insertion point?



>+
>+  // check folder node (no longer shortcut)
>+  var queryNode = root.getChild(root.childCount-2);
>+  do_check_eq(queryNode.type, queryNode.RESULT_TYPE_FOLDER);
>+  queryNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+  queryNode.containerOpen = true;
>+  do_check_eq(queryNode.childCount, 1);
>+  var child = queryNode.getChild(0);
>+  do_check_true(bookmarkURI.equals(uri(child.uri)));
>+  queryNode.containerOpen = false;
>+
>+  var queryNode2 = root.getChild(root.childCount-1);
>+  do_check_eq(queryNode2.type, queryNode2.RESULT_TYPE_QUERY);
>+  queryNode2.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+  queryNode2.containerOpen = true;
>+  do_check_eq(queryNode2.childCount, 0);
>+  queryNode2.containerOpen = false;
>+
>+  root.containerOpen = false;

you don't need to close containers if they're gced anyway...
Attachment #314249 - Flags: review?(mano) → review-
(Assignee)

Comment 19

10 years ago
Created attachment 315573 [details] [diff] [review]
wip patch

Updated to fix comments, as well as to address a number of scenarios not covered by the previous patch.

One problem I found is that setting the drag action is not persistent: It's set to COPY in tree.xml.startDrag(), but at any point during the drag, if you get the current drag session and dump dragAction, it's set to MOVE. I took a tour through nsDragAndDrop.js, and it's setting it properly, though I stopped tracing when I hit the OS-specific widget code.
Attachment #314249 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano] → [has wip patch]
(Assignee)

Comment 20

10 years ago
Created attachment 316098 [details] [diff] [review]
fix

two problems i found while fixing this that i'll file followups for:

- during drags, getting the current drag session does *not* reflect the dragAction set when the drag began

- even if cmd_cut is not enabled, and returns false, cuts via kb shortcut are still allowed (!)
Attachment #315573 - Attachment is obsolete: true
Attachment #316098 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [has wip patch] → [has patch][needs review mano]
(Assignee)

Updated

10 years ago
Attachment #316098 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano] → [has patch]
> - even if cmd_cut is not enabled, and returns false, cuts via kb shortcut are
still allowed (!)

yes this is a problem in tag containers too, probably valid also for copy(?)
(Assignee)

Comment 22

10 years ago
Created attachment 316320 [details] [diff] [review]
fix v2

- fixed irc comments
- fixed insertion point not being used
Attachment #316098 - Attachment is obsolete: true
Attachment #316320 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch] → [has patch][needs review mano]
             if (PlacesUtils.nodeIsReadOnly(parent) ||
-                PlacesUtils.nodeIsTagQuery(parent)) {
+                PlacesUtils.nodeIsTagQuery(parent) ||
+                !PlacesControllerDragHelper.canMoveContainerNode(node)) {
+              // XXX DOES NOTHING! dragAction doesn't persist
               dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
               break;

i'm not sure that this does nothing at all, IIRC when i put the check for nodeIsTagQuery here, it was correctly forcing a copy op, while that was a move op before.
+    // Disallow moving tag containers unless they're being moved
+    // within the Tags folder.
+    if (aParentId == PlacesUtils.tagsFolderId) {
+      if (aTargetId == -1 || aTargetId != PlacesUtils.tagsFolderId)
+        return false;
+    }

this will not work, tag containers are no more children of the tags folder, they are generated by a RESULTS_AS_TAG_QUERY query. or better, will work only for old profiles until we upgrade their left pane folder
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano] → [has patch]
(Assignee)

Updated

10 years ago
Attachment #316320 - Flags: review?(mano)
(Assignee)

Comment 25

10 years ago
Created attachment 316767 [details] [diff] [review]
wip

still building out browser tests
Attachment #316320 - Attachment is obsolete: true

Updated

10 years ago
Whiteboard: [has patch] → [needs new patch]
(Assignee)

Comment 26

10 years ago
Comment on attachment 316767 [details] [diff] [review]
wip

this is ready for review, i'm just working on browser chrome tests for this still.
Attachment #316767 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [needs new patch] → [has patch][needs review mano]
(Assignee)

Comment 27

10 years ago
Comment on attachment 316767 [details] [diff] [review]
wip


>+    LOG("canMoveContainerNode() for " + aNode.title + ": " +
>+      "id:" + aNode.itemId + ", " +
>+      "parentId:" + aNode.parent.itemId + ", " +
>+      "concreteId:" + concreteId + ", " +
>+      "parentIdOfConcreteId:" + parentIdOfConcreteId);
>+

i'll remove this in the final patch
Comment on attachment 316767 [details] [diff] [review]
wip


>? toolkit/components/places/tests/bookmarks/test_moveitem.js
>Index: browser/components/places/content/controller.js
>===================================================================

>+    // Get parent id if necessary
>+    if (!aParentId || aParentId == -1)

make that aParentId === undefined

>+      aParentId = PlacesUtils.bookmarks.getFolderIdForItem(aId);
>+
>+    // Disallow moving immediate children of the left pane folder
>+    // and the All Bookmarks folder.
>+    if (aParentId == PlacesUIUtils.leftPaneFolderId ||
>+        aParentId == PlacesUIUtils.allBookmarksFolderId)
>+      return false;
>+
>+    // Disallow moving tag containers unless they're being moved
>+    // within the Tags folder.
>+    if (aParentId == PlacesUtils.tagsFolderId) {
>+      if (aTargetId == -1 || aTargetId != PlacesUtils.tagsFolderId)

The later check would catch both cases.

>+    //var copy = session.dragAction & Ci.nsIDragService.DRAGDROP_ACTION_COPY;
>+    var copy = false;

hrm?

>Index: browser/components/places/content/tree.xml
>===================================================================

> 
>             // If this node is part of a readonly container (e.g. a livemark) it 
>             // cannot be moved, only copied, so we must change the action used
>             // by the drag session.
>             if (PlacesUtils.nodeIsReadOnly(parent) ||
>-                PlacesUtils.nodeIsTagQuery(parent)) {
>+                PlacesUtils.nodeIsTagQuery(parent) ||
>+                !PlacesControllerDragHelper.canMoveContainerNode(node)) {
>+              // XXX DOES NOTHING! dragAction doesn't persist
>               dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;

hrm, when and why doesn't it persist?



>Index: toolkit/components/places/src/utils.js
>===================================================================

>           annos = self.getAnnotationsForItem(id).filter(function(anno) {
>             // XXX should whitelist this instead, w/ a pref for
>             // backup/restore of non-whitelisted annos
>             // XXX causes JSON encoding errors, so utf-8 encode
>             //anno.value = unescape(encodeURIComponent(anno.value));
>-            if (anno.name == "livemark/feedURI")
>+            if (anno.name == LMANNO_FEEDURI)
>               aJSNode.livemark = 1;
>+            if (anno.name == READ_ONLY_ANNO && aResolveShortcuts)
>+              return false;

comment here...

>+      var concreteId = PlacesUtils.getConcreteItemId(aPlacesNode);
>+      if (aJSNode.id != -1 && (PlacesUtils.nodeIsQuery(aPlacesNode) ||
>+          (concreteId != aPlacesNode.itemId && !aResolveShortcuts))) {

You could simply if it's a shortcut.
Attachment #316767 - Flags: review?(mano) → review-
(Assignee)

Comment 29

10 years ago
> >+                !PlacesControllerDragHelper.canMoveContainerNode(node)) {
> >+              // XXX DOES NOTHING! dragAction doesn't persist
> >               dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
> 
> hrm, when and why doesn't it persist?

in the treebinding, in onDragStart(), we set the drag action to copy if the dragged node cannot be moved. however, *during* the drag, controller.canDrop() gets the drag action from the current session, which returns DRAGDROP_ACTION_MOVE. i traced it through nsDragAndDrop.js, up into mac widgets code, iirc.

> >+      var concreteId = PlacesUtils.getConcreteItemId(aPlacesNode);
> >+      if (aJSNode.id != -1 && (PlacesUtils.nodeIsQuery(aPlacesNode) ||
> >+          (concreteId != aPlacesNode.itemId && !aResolveShortcuts))) {
> 
> You could simply if it's a shortcut.
> 

hm, actually folder shortcuts should be removable even if they're a shortcut to a "special" folder. the only case in which they should not be removable is if the shortcut is a child of a read-only container. this a more consistent and simpler policy, resulting in less special-casing in the code. changes for this in the new patch:

- make left folder root read-only
- don't check concrete id of folder shortcuts at all in canMoveContainerNode()
- check if parent is read-only in canMoveContainerNode()
(Assignee)

Comment 30

10 years ago
Created attachment 317743 [details] [diff] [review]
fix + tests

comments addressed, plus browser tests.
Attachment #316767 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #317743 - Flags: review?(mano)
(Assignee)

Comment 31

10 years ago
Created attachment 317764 [details] [diff] [review]
bump left pane version

same patch, but also bumps left pane version number, so that existing users get the change to make the left pane root folder read-only.
Attachment #317743 - Attachment is obsolete: true
Attachment #317743 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Attachment #317764 - Flags: review?(mano)
Comment on attachment 317743 [details] [diff] [review]
fix + tests

aTargetId is no longer used, please remove that from the function, from its header and from all of its callers.

r=mano with that fixed, plus reving the left-pane version.
Attachment #317743 - Attachment is obsolete: false
(Assignee)

Comment 33

10 years ago
Comment on attachment 317764 [details] [diff] [review]
bump left pane version

carrying over review from comment #32
Attachment #317764 - Flags: review?(mano) → review+
(Assignee)

Comment 34

10 years ago
Comment on attachment 317764 [details] [diff] [review]
bump left pane version

drivers: has tests, fixes this and 3 other blockers.
Attachment #317764 - Flags: approval1.9?
Comment on attachment 317764 [details] [diff] [review]
bump left pane version

a=mconnor on behalf of 1.9 drivers
Attachment #317764 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 36

10 years ago
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.231; previous revision: 1.230
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.130; previous revision: 1.129
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.114; previous revision: 1.113
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.139; previous revision: 1.138
done
Checking in browser/components/places/tests/browser/Makefile.in;
/cvsroot/mozilla/browser/components/places/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/components/places/tests/browser/browser_423515.js,v
done
Checking in browser/components/places/tests/browser/browser_423515.js;
/cvsroot/mozilla/browser/components/places/tests/browser/browser_423515.js,v  <--  browser_423515.js
initial revision: 1.1
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_423515_forceCopyShortcuts.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_423515_forceCopyShortcuts.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_423515_forceCopyShortcuts.js,v  <--  test_423515_forceCopyShortcuts.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][needs review mano]
(Assignee)

Updated

10 years ago
Blocks: 428701

Updated

10 years ago
Depends on: 430948

Comment 37

10 years ago
Sorry to spam this bug. But I'm seeing that I cannot drag and drop anymore an icon on personal toolbar.

Here is the error I got :

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/controller.js :: PCDH_canMoveContainer :: line 1387"  data: no]
(Assignee)

Comment 38

10 years ago
(In reply to comment #37)
> Sorry to spam this bug. But I'm seeing that I cannot drag and drop anymore an
> icon on personal toolbar.
> 
> Here is the error I got :
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80070057 (NS_ERROR_ILLEGAL_VALUE)
> [nsINavBookmarksService.getFolderIdForItem]"  nsresult: "0x80070057
> (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> chrome://browser/content/places/controller.js :: PCDH_canMoveContainer :: line
> 1387"  data: no]
> 

bug 430948
verified with: 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre

flicker bug 431140
Status: RESOLVED → VERIFIED
(Assignee)

Updated

10 years ago
Depends on: 431332
(Assignee)

Comment 40

10 years ago
(In reply to comment #20)
> Created an attachment (id=316098) [details]
> fix
> 
> two problems i found while fixing this that i'll file followups for:
> 
> - during drags, getting the current drag session does *not* reflect the
> dragAction set when the drag began

bug 431699

> 
> - even if cmd_cut is not enabled, and returns false, cuts via kb shortcut are
> still allowed (!)
> 

bug 431671 (thanks marco)
Depends on: 462545
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
The test for this bug fails with the HTML5 parser (bug 45664). Is the execution of the test supposed to load an HTML document?
You need to log in before you can comment on or make changes to this bug.