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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: fullmetaljacket.xp+bugmail, Assigned: moco)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
3.45 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Updated•18 years ago
|
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
Updated•18 years ago
|
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.
Comment 2•18 years ago
|
||
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
Comment 3•18 years ago
|
||
*** Bug 364311 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 4•18 years ago
|
||
*** Bug 364321 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 5•18 years ago
|
||
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
Assignee | ||
Comment 6•18 years ago
|
||
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
Comment 7•18 years ago
|
||
Code for the wise: MOZ_PLACES #ifdefs in browser/components/bookmarks (is not that remotely innovative?).
Assignee | ||
Comment 8•18 years ago
|
||
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
Assignee | ||
Comment 9•18 years ago
|
||
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.)
Assignee | ||
Comment 10•18 years ago
|
||
*** Bug 342357 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 11•18 years ago
|
||
removed the unused createLivemark() method
Attachment #249128 -
Attachment is obsolete: true
Attachment #249173 -
Flags: review?
Assignee | ||
Comment 12•18 years ago
|
||
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.
Assignee | ||
Comment 13•18 years ago
|
||
> I'm going to spin up a new bug about that see bug #364401.
Assignee | ||
Updated•18 years ago
|
Attachment #249173 -
Flags: review? → review?(gavin.sharp)
Assignee | ||
Comment 14•18 years ago
|
||
Comment on attachment 249173 [details] [diff] [review] patch alternatively, mano could review this.
Attachment #249173 -
Flags: review?(mano)
Comment 15•18 years ago
|
||
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.
Comment 16•18 years ago
|
||
*** Bug 364932 has been marked as a duplicate of this bug. ***
Comment 17•18 years ago
|
||
Comment on attachment 249173 [details] [diff] [review] patch see comment 15
Attachment #249173 -
Flags: review?(mano)
Attachment #249173 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 18•18 years ago
|
||
> 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.
Assignee | ||
Comment 19•18 years ago
|
||
Attachment #249173 -
Attachment is obsolete: true
Attachment #249793 -
Flags: review?(mano)
Comment 20•18 years ago
|
||
Comment on attachment 249793 [details] [diff] [review] updated patch, per asaf's comments r=mano
Attachment #249793 -
Flags: review?(mano) → review+
Assignee | ||
Comment 21•18 years ago
|
||
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
Comment 22•18 years ago
|
||
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
Updated•18 years ago
|
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.
Description
•