Closed
Bug 574264
Opened 14 years ago
Closed 14 years ago
"Bookmark This Link" does not work
Categories
(SeaMonkey :: Bookmarks & History, defect)
SeaMonkey
Bookmarks & History
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.1a3
People
(Reporter: doctor__j, Assigned: kairo)
References
Details
Attachments
(1 file)
3.28 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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
Component: General → Bookmarks
OS: Windows XP → All
QA Contact: general → bookmarks
Hardware: x86 → All
Version: unspecified → Trunk
Comment 2•14 years ago
|
||
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)?
Assignee | ||
Comment 3•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
> 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•14 years ago
|
||
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•14 years ago
|
||
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 9•14 years ago
|
||
Three instances of .linkUrl() http://mxr.mozilla.org/comm-central/search?string=.linkurl%28&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central
Assignee | ||
Comment 10•14 years ago
|
||
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...
Assignee | ||
Comment 11•14 years ago
|
||
(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 :(
Assignee | ||
Comment 12•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #454478 -
Flags: review?(neil) → review+
Assignee | ||
Comment 13•14 years ago
|
||
Fixed in http://hg.mozilla.org/comm-central/rev/f9e5d3df42ec
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
Comment 14•14 years ago
|
||
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?
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 15•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•