304739 - Tidy up hrefAndLinkNodeForClickEvent

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Ian Neal]

Neil pointed out to me on irc that hrefAndLinkNodeForClickEvent could be tied up
a bit:
should return { linkNode: ..., href: ... };
and just take event as its parameter
(or return null if linkNode is null)

Comment 1

[Ian Neal]

Attachment: First pass v0.1

This patch:
* Only passes event into hrefAndLinkNodeForClickEvent
* Returns href and linkNode from hrefAndLinkNodeForClickEvent

Not done the returning null as not 100% sure if that will be of use.

Comment 2

[Ian Neal]

From Neil via IRC:
a) I'd prefer if you return null if there is no linkNode
b) handleLinkClick would like the link element too
c) isPhishingURL doesn't work with XLinks, please make it accept an optionally
optional href

Comment 3

[Ian Neal]

Attachment: Second pass v0.1a

Changes since v0.1:
* hrefAndLinkNodeForClickEvent now returns null for both href and linkNode if
linkNode is null
* isPhishingURL takes optional argument for xlink's href
* handleLinkClick is passed linkNode

Comment 4

[Ian Neal]

Attachment: Tweaked patch v0.1b

Changes from v0.1a:
* hrefAndLinkNodeForClickEvent returns null rather than null for each array
entry
* changed messagePaneOnClick and contentAreaClick to cope with above

Comment 5

[[e-mail]]

Comment on attachment 192860 [details] [diff] [review]
Tweaked patch v0.1b

>+  var href = aHref ? aHref : aLinkNode.href;
Nit: could be aHref || aLinkNode.href

Comment 6

[Ian Neal]

Attachment: Un-nitted tweaked patch v0.1c (Checked in)

Changes from v0.1b:
* Fixed nit

Carrying forward r= and requesting sr

Comment 7

[Ian Neal]

Comment on attachment 192876 [details] [diff] [review]
Un-nitted tweaked patch v0.1c (Checked in)

Checking in
mailnews/base/resources/content/mailWindow.js;
new revision: 1.101; previous revision: 1.100
mailnews/base/resources/content/phishingDetector.js;
new revision: 1.2; previous revision: 1.1
xpfe/communicator/resources/content/contentAreaClick.js;
new revision: 1.51; previous revision: 1.50
done

Comment 8

[Ian Neal]

Reopening as we (SeaMonkey council) want this in the branch too

Comment 9

[Ian Neal]

Comment on attachment 192876 [details] [diff] [review]
Un-nitted tweaked patch v0.1c (Checked in)

Requesting a= for this suite only fairly low risk patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This checkin broke contentAreaClick() quite badly (and in fact broke every
single caller of hrefAndLinkNodeForClickEvent()).  I filed bug 305158 on that. 
Please make sure to not land this on branch without a fix for that bug.

Comment 11

[Ian Neal]

Comment on attachment 192876 [details] [diff] [review]
Un-nitted tweaked patch v0.1c (Checked in)

Request for a= also covers a request to land the bustage fix from bug 305158

Comment 12

[Ian Neal]

Attachment: Branch patch v0.1d (Checked in branch 1.8)

Checking in (1.8 branch)
mailnews/base/resources/content/mailWindow.js;
new revision: 1.100.2.1; previous revision: 1.100
mailnews/base/resources/content/phishingDetector.js;
new revision: 1.1.2.1; previous revision: 1.1
xpfe/communicator/resources/content/contentAreaClick.js;
new revision: 1.50.2.1; previous revision: 1.50
done

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using LXR for code verification.