Last Comment Bug 711779 - Add Bookmark This Link to feeds' Website header link context menu
: Add Bookmark This Link to feeds' Website header link context menu
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.9
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-17 15:52 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-12-23 10:52 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch [Checkin: comment 3] (3.96 KB, patch)
2011-12-17 15:52 PST, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2011-12-17 15:52:07 PST
Created attachment 582604 [details] [diff] [review]
patch [Checkin: comment 3]

If you view a feed item in MailNews which contains a Website (Content-Base) header, you can either click it or context-click it. In the latter case, only Copy Link Location is offered currently.

This bug is about adding Bookmark This Link to the context menu, using the Subject header value as default bookmark title.

Note: If review takes longer, please consider giving f+ and allowing me to land the strings in advance (next Aurora merge is only two days ahead). Also, if SR is needed, please request it since I'm assuming this is a MOA case.

BTW: See bug 224433 comment 11 regarding the general context menu item's label.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-12-18 15:18:09 PST
Comment on attachment 582604 [details] [diff] [review]
patch [Checkin: comment 3]

Review of attachment 582604 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/mailnews/msgHdrViewOverlay.js
@@ +1707,5 @@
> +
> +    if (currentHeaderData && "content-base" in currentHeaderData)
> +    {
> +      var url = currentHeaderData["content-base"].headerValue;
> +      if (url != websiteAddressNode)

bah, make that websiteAddress (forgot to hg qrefresh before attaching the patch)
Comment 2 Karsten Düsterloh 2011-12-21 17:00:04 PST
Comment on attachment 582604 [details] [diff] [review]
patch [Checkin: comment 3]

Just formal nitpicking, apart from the bug you already noticed.

>+function BookmarkWebsite(websiteAddressNode)

The parameter should be named aWebsiteAddressNode.

>+    var websiteAddress = websiteAddressNode.getAttribute("value");
>+      var url = currentHeaderData["content-base"].headerValue;
>+      var title = currentHeaderData["subject"].headerValue;

Subscope variables should use let.


r/moa=me with that and the bug fixed.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-12-22 11:27:12 PST
Comment on attachment 582604 [details] [diff] [review]
patch [Checkin: comment 3]

http://hg.mozilla.org/comm-central/rev/f59488355a18
Comment 4 Tony Mechelynck [:tonymec] 2011-12-23 10:52:59 PST
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20111223 Firefox/12.0a1 SeaMonkey/2.9a1 ID:20111223003001

The "Website" link which appears in the headers of a feed item now has a "Bookmark This Link" menuitem in addition to "Copy Link Location" which I've always seen there.

Checked in all three of: preview pane, message tab, message window.

=> VERIFIED.

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