339290 - unable to paste links into the organize bookmarks dialog

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

unable to paste links into the organize bookmarks dialog

I know this is a regression (but not sure since when) and I think there is a dup in bugzilla, but I couldn't find it (yet).

The fix is very simple, and I found this bug while working on bug #315690 (in an attempt to see if CloneResource() was the culprit of that bug.)

I'll attach the low risk fix.

Comment 1

[(not reading, please use [e-mail] instead)]

the fix (on my work machine) is something like this to bookmarks.js:

-// XXX: we should infer the best charset
-BookmarksUtils.createBookmark(null, items, null, name, null);
-items = [items];

+// XXX: we should infer the best charset
+var createdBookmarkResource = BookmarksUtils.createBookmark(null, items, null, name, null);
+items = [createdBookmarkResource];

the code expects items to be an array of RDF resources, and not an array of strings.

later, we will get a JS exception when we try to do items[0].Value (because we expect it to be a RDF literal.)

this fix solves that problem, I'll attach a proper diff once I get my tree building at home.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: oops, remove that bogus comment.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 223463 [details] [diff] [review]
oops, remove that bogus comment.

I think your analysis is correct, but I'm really not qualified to review this patch. I'm not sure who I would suggest as a reviewer, maybe mconnor or vlad?

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 223463 [details] [diff] [review]
oops, remove that bogus comment.

switching the review from gavin to mconnor

Comment 6

[(not reading, please use [e-mail] instead)]

fix checked into the MOZILLA_1_8_BRANCH branch

note to QA:  to test this, try right clicking on a link and pasting into the bookmark manager dialog.

note, if we have it (in your history) we will use the page title as the name of the bookmark.

Comment 7

[(not reading, please use [e-mail] instead)]

the trunk also seems to have this bug, but for a different reason.

the paste item is disabled (and note, when I do ctrl+v I get "###!!! ASSERTION: NOT IMPLEMENTED: '0', file c:/builds/trunk/mozilla/layout/base/nsDocumentViewer.cpp, line 2560" on my console)

I think it might be better to spin up another bug on this issue for places on the trunk.

Comment 8

[(not reading, please use [e-mail] instead)]

fyi, in order to keep trunk in sync with the 1.8 branch, I've landed this change on the trunk as well.

Comment 9

[Jesse Ruderman]

Was a bug filed on the similar issue with Places?

Comment 10

[(not reading, please use [e-mail] instead)]

> Was a bug filed on the similar issue with Places?

oops, I forgot to do that.  Thank you for the reminder.  See bug #341953

Comment 11

[(not reading, please use [e-mail] instead)]

oops, I never marked this as fixed (even though it was on the branch.)

the fix was cross committed to trunk (even though it uses places), but this is still an issue with places, see bug #341953