Closed Bug 364018 Opened 18 years ago Closed 18 years ago

cannot drag and drop url from location bar or webpage to bookmarks toolbar.

Categories

(Firefox :: Bookmarks & History, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: fullmetaljacket.xp+bugmail, Assigned: moco)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

cannot drag and drop url (from address bar or webpage) to bookmark toolbar.

known bug or regressed from enabling places for history handling?

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a1) Gecko/20061215 Minefield/3.0a1 ID:2006121517 [cairo]

WFM
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a1) Gecko/20061212 Minefield/3.0a1 ID:2006121204 [cairo]
Summary: cannot drag and drop url (from address bar or webpage) to bookmark toolbar. → cannot drag and drop url from address bar or webpage to bookmark toolbar.
(In reply to comment #0)
> cannot drag and drop url (from address bar or webpage) to bookmark toolbar.

Confirmed
WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061215 Minefield/3.0a1 - Build ID: 2006121510 

Not working after
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061215 Minefield/3.0a1 - Build ID: 2006121511
Blocks: 355738
Keywords: regression
Summary: cannot drag and drop url from address bar or webpage to bookmark toolbar. → cannot drag and drop url from location bar or webpage to bookmark toolbar.
Latest trunk/Linux build has also this problem.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061217 Minefield/3.0a1
OS: Windows Vista → All
*** Bug 364311 has been marked as a duplicate of this bug. ***
*** Bug 364321 has been marked as a duplicate of this bug. ***
a regression from my history-on-places landing.

From http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarks.js#1905

var HISTDS  = RDF.GetDataSource("rdf:history");

that's not going to work anymore.  my apologies for not catching it when testing.
Status: NEW → ASSIGNED
looking at http://lxr.mozilla.org/seamonkey/search?string=rdf%3Ahistory, this is the only one I need to worry about.
Assignee: nobody → sspitzer
Status: ASSIGNED → NEW
Code for the wise: MOZ_PLACES #ifdefs in browser/components/bookmarks (is not that remotely innovative?).
working on the fix.

we need to execute a query on the nav history service (nsINavHistoryService) and use the title (from nsINavHistoryResultNode).

note, we need to do the same thing for createLivemark() method in the same file.
Status: NEW → ASSIGNED
Attached patch patch, draft 1 (obsolete) — Splinter Review
first draft of patch.  not ready for review yet.  still to do:

1)  test createLivemark() path
2)  make sure I didn't break non-places bookmarks
3)  double check query options (should it be RESULTS_AS_URI?)
4)  test the "no title for uri" code paths (with default name and without)
5)  investigate this error when dragging from places-history-sidebar to personal toolbar.

JavaScript error: chrome://global/content/nsDragAndDrop.js, line 22: aTransferDa
taSet has no properties

(I think this might have to do with the places controller.  I may spin this bug out.)
*** Bug 342357 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — Splinter Review
removed the unused createLivemark() method
Attachment #249128 - Attachment is obsolete: true
Attachment #249173 - Flags: review?
this patch fixes the regression and removes the unused code.

as for the js error, when dragging from the history-panel-on-places sidebar to bookmarks, I still see this error:

JavaScript error: chrome://global/content/nsDragAndDrop.js, line 22:
aTransferDataSet has no properties

I'm going to spin up a new bug about that, as i think the fix will be to the places controller.js, which we can look into after mano lands his monster patch (see #359462, pending review).

note, you do not see that js error when dnd links from bookmarks panel, pages, the url bar, etc.
> I'm going to spin up a new bug about that

see bug #364401.
Attachment #249173 - Flags: review? → review?(gavin.sharp)
Comment on attachment 249173 [details] [diff] [review]
patch

alternatively, mano could review this.
Attachment #249173 - Flags: review?(mano)
Comment on attachment 249173 [details] [diff] [review]
patch

>Index: browser/components/bookmarks/content/bookmarks.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v
>retrieving revision 1.127
>diff -u -8 -p -r1.127 bookmarks.js
>--- browser/components/bookmarks/content/bookmarks.js	10 Dec 2006 22:40:39 -0000	1.127
>+++ browser/components/bookmarks/content/bookmarks.js	19 Dec 2006 23:55:36 -0000
>@@ -1892,51 +1892,69 @@ var BookmarksUtils = {
>     var selection    = {};
>     selection.length = 1;
>     selection.item   = [aItem  ];
>     selection.parent = [aParent];
>     this.checkSelection(selection);
>     return selection;
>   },
> 
>-  createBookmark: function (aName, aURL, aCharSet, aDefaultName)
>+  getTitleForURLFromHistory: function(aURL, aDefaultName)
>   {
>-    if (!aName) {
>-      // look up in the history ds to retrieve the name
>-      var rSource = RDF.GetResource(aURL);
>-      var HISTDS  = RDF.GetDataSource("rdf:history");
>-      var nameArc = RDF.GetResource(gNC_NS+"Name");
>-      var rName   = HISTDS.GetTarget(rSource, nameArc, true);
>-      aName       = rName ? rName.QueryInterface(kRDFLITIID).Value : aDefaultName;
>-      if (!aName)
>-        aName = aURL;
>+    var title = null;
>+
>+#ifndef MOZ_PLACES
>+    // look up in the history ds to retrieve the name
>+    var rSource = RDF.GetResource(aURL);
>+    var HISTDS  = RDF.GetDataSource("rdf:history");
>+    var nameArc = RDF.GetResource(gNC_NS+"Name");
>+    var rName   = HISTDS.GetTarget(rSource, nameArc, true);
>+    title = rName ? rName.QueryInterface(kRDFLITIID).Value : aDefaultName;
>+#else
>+    var histsvc = 
>+      Components.classes["@mozilla.org/browser/nav-history-service;1"]
>+                .getService(Components.interfaces.nsINavHistoryService);
>+
>+    // query for the URL
>+    var options = histsvc.getNewQueryOptions();
>+    options.resultType = options.RESULTS_AS_URI;
>+    var query = histsvc.getNewQuery();
>+    query.uri = IOSVC.newURI(aURL, null, null);
>+    var result = histsvc.executeQuery(query, options);
>+    var root = result.root;
>+    root.containerOpen = true;
>+    var cc = root.childCount;
>+    for (var i=0; i < cc && !title; ++i) { 
>+      var node = root.getChild(i);
>+      title = node.title;
>     }
>+
>+    if (!title)
>+      title = aDefaultName;
>+#endif
>+
>+    if (!title)
>+      title = aURL;
>+   
>+    return title;
>+  },
>+

It would be cleaner to avoid the title variable and return early for each case.
*** Bug 364932 has been marked as a duplicate of this bug. ***
Comment on attachment 249173 [details] [diff] [review]
patch

see comment 15
Attachment #249173 - Flags: review?(mano)
Attachment #249173 - Flags: review?(gavin.sharp)
> It would be cleaner to avoid the title variable and return early for each case.

thanks for looking this over, asaf.  I'll clean it up, per your comment, and attach a new patch.
Attachment #249173 - Attachment is obsolete: true
Attachment #249793 - Flags: review?(mano)
Comment on attachment 249793 [details] [diff] [review]
updated patch, per asaf's comments

r=mano
Attachment #249793 - Flags: review?(mano) → review+
fixed.  thanks for the review, asaf.

Checking in browser/components/bookmarks/content/bookmarks.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v  <--  bookm
arks.js
new revision: 1.128; previous revision: 1.127
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20061227 Minefield/3.0a2pre ID:2006122714 [cairo]

verified fixed for :
D&D from location bar to bookmarks toolbar
D&D a link from a page to bookmarks toolbar

also fixes:
D&D from location bar to bookmarks menu
D&D a link from a page to bookmarks menu

I couldn't find a bug for the latter 2, if one doesn't exist the summary should be changed to:

cannot drag and drop url from location bar or webpage to Bookmarks Toolbar or Bookmarks menu.
Status: RESOLVED → VERIFIED
Summary: cannot drag and drop url from location bar or webpage to bookmark toolbar. → cannot drag and drop url from location bar or webpage to bookmarks toolbar.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: