154856 - Contextual "Bookmark This Link..." command doesn't work

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[sairuh (rarely reading bugmail)]

found using 2002.06.27.05 chimera bits.

1. go to any web page with links, such as http://mozilla.org/
2. bring up the context menu for a link, eg, the "Testing" link on the left side.
3. select "Bookmar This Link"

expected: the Bookmarks dlg should appear (or, at the very least, the page
should be automatically added to your bookmarks).

actual results: nothing happens. the link is not in the Bookmarks menu, nor is
it in the bookmarks tab of the Sidebar.

unimplemented feature?

Comment 1

[Greg K.]

Confirmed using Chimera/20020625.

Comment 2

[Brian Ryner (not reading)]

Taking, I've got a preliminary patch.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

Comment 4

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 89746 [details] [diff] [review]
patch

some comments:

+    if (aURL) {

you should check both aURL and aTitle, no? It won't crash, but it will create a
garbage title if a caller doesn't happen to specify a valid title.

+      PRUnichar* bookmarkURLStr = nsMemory::Alloc((length + 1) *
sizeof(PRUnichar));
+      [aURL getCharacters:bookmarkURLStr];
...

I think ben wrote a utility routine to go from a NSString to a nsString. You
should use that instead.

Other than that, looks good.

Comment 5

[Brian Ryner (not reading)]

> you should check both aURL and aTitle, no? It won't crash, but it will create a
> garbage title if a caller doesn't happen to specify a valid title.

Right, my assumption was that a caller wouldn't be doing that.

> I think ben wrote a utility routine to go from a NSString to a nsString. You
> should use that instead.

Ok.  I had to inline the utility function in StringUtils.h or else I would get
duplicate symbols (it was only used from one file previously).

Comment 6

[Brian Ryner (not reading)]

Attachment: patch #2

addressing pink's comments

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 89753 [details] [diff] [review]
patch #2

r=pink. not sure why all the code is in the header, maybe we should revisit it
being inline.

Comment 8

[Brian Ryner (not reading)]

Checked in.

Comment 9

[Winnie Lam]

Verified with 07-02 build on OS 10.1.5.  context menu "bookmark this link"
works.  Checked that the new bookmark appears both in the bookmarks menu and in
the sidebar.