Last Comment Bug 574264 - "Bookmark This Link" does not work
: "Bookmark This Link" does not work
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Robert Kaiser
:
Mentors:
Depends on:
Blocks: 562339
  Show dependency treegraph
 
Reported: 2010-06-23 22:55 PDT by doctor__j
Modified: 2010-07-05 06:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix the three left cases (3.28 KB, patch)
2010-06-28 04:23 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review

Description doctor__j 2010-06-23 22:55:33 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.9.3a5pre) Gecko/20100517 SeaMonkey/2.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.9.3a5pre) Gecko/20100517 SeaMonkey/2.1a2pre

After checking-in of bug 562339, the "Bookmark This Link" in the context menu failed to work.

Reproducible: Always
Comment 1 Tony Mechelynck [:tonymec] 2010-06-24 00:35:31 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a6pre) Gecko/20100623 Lightning/1.1a1pre SeaMonkey/2.1a2pre - Build ID: 20100623005748

Also on Linux
Comment 2 Jens Hatlak (:InvisibleSmiley) 2010-06-27 14:49:45 PDT
Hmm, cannot reproduce with a recent nightly (WinXP). Can you give steps to reproduce, including which site you tried to bookmark and any messages appearing in the Error Console (Tools/Web Development)?
Comment 3 Robert Kaiser 2010-06-27 15:59:32 PDT
I can't test, because I have the bug 498596 patches applied and it works fine with those. :)

Still, the patch in bug 562339 didn't change anything for this command, so unless you're using some add-on that messes with the context menu or bookmarks, I don't know how that could cause a problem for you.
Comment 4 Tony Mechelynck [:tonymec] 2010-06-27 22:06:00 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a6pre) Gecko/20100627 Lightning/1.1a1pre SeaMonkey/2.1a3pre - Build ID: 20100627005056

In reply to comment #2 (and I'm on Linux)
1. Click right "bug 498596" in comment #3
2. Click "Bookmark this link"
3. Click "Bookmarks" in the top menu
-- No new link at the bottom of the bookmarks menu
4. Click "Manage Bookmarks"
-- No new bookmark at the end of the Bookmark Ma
Comment 5 Tony Mechelynck [:tonymec] 2010-06-27 22:07:58 PDT
(sorry, submitted too soon)
...nager

Error Console says:

Error: gContextMenu.linkURL is not a function
Source File: chrome://navigator/content/navigator.xul
Line: 1
Comment 6 Philip Chee 2010-06-27 22:26:42 PDT
> Error: gContextMenu.linkURL is not a function
> Source File: chrome://navigator/content/navigator.xul
> Line: 1

KaiRo changed linkURL from a function to a property. But forgot to check all callers.
Comment 7 Tony Mechelynck [:tonymec] 2010-06-27 22:31:24 PDT
From inspection of https://bugzilla.mozilla.org/attachment.cgi?id=445604&action=diff

Before the patch, this.linkURL() is a function
After the patch, this.linkURL is a string variable and there exists a function named this.getLinkURL (defined at new line 1119) and also a function getLinkURI (defined at new line 1134).

I suspect that one (or more) occurence(s) of linkURL() used as a function was overlooked by the assignee of bug 562339 but I didn't search for it.
Comment 8 Tony Mechelynck [:tonymec] 2010-06-27 22:43:26 PDT
http://mxr.mozilla.org/seamonkey/source/suite/common/contentAreaContextOverlay.xul#97

97       <menuitem id="context-bookmarklink"
98                 label="&bookmarkLinkCmd.label;"
99                 accesskey="&bookmarkLinkCmd.accesskey;"
100                 oncommand="BookmarksUtils.addBookmark(gContextMenu.linkURL(),
101                                                       gContextMenu.linkText(), 
102                                                       undefined, false);"/>
Comment 10 Robert Kaiser 2010-06-28 04:07:45 PDT
Ah, right, that makes sense. Yes, I changed it from a function to a property to comply with the same API as Firefox. Sorry I didn't catch those, but should be easy to fix.

While we're at that, I see linkText() there, that might have changed to a getter as well...
Comment 11 Robert Kaiser 2010-06-28 04:14:02 PDT
(In reply to comment #10)
> While we're at that, I see linkText() there, that might have changed to a
> getter as well...

Ah, no, that is still a function. Well, the context menu is still somewhat messy :(
Comment 12 Robert Kaiser 2010-06-28 04:23:07 PDT
Created attachment 454478 [details] [diff] [review]
fix the three left cases

This patch fixes the three remaining cases of using this as a function.

Sorry for this. I honestly didn't check "Send Link", and in the bookmarking cases, you guys paid for me having the places bookmarks patch set around for so long, as what I did test was the bookmarking items _after_ applying that as well.
Comment 13 Robert Kaiser 2010-06-29 08:50:12 PDT
Fixed in http://hg.mozilla.org/comm-central/rev/f9e5d3df42ec
Comment 14 Tony Mechelynck [:tonymec] 2010-07-04 11:54:19 PDT
No more comments in almost a week, and I just checked the testcase in comment #4.
=> VERIFIED.

Robert, I suppose no tests (in-testsuite, in-litmus) are necessary?
Comment 15 Robert Kaiser 2010-07-05 06:45:18 PDT
(In reply to comment #14)
> Robert, I suppose no tests (in-testsuite, in-litmus) are necessary?

We currently have no tests at all for context menus actually working, but we have test coverage for showing the right items.

I have noted that I should add tests for the bookmarking items for places bookmarks, but I haven't come around to doing any tests there yet.

Note You need to log in before you can comment on or make changes to this bug.