Closed Bug 1434575 Opened 2 years ago Closed Last year

Review and possibly remove dead code in the processing of HTMLLinkElement in nsMsgCompose.cpp and nsMsgSend.cpp

Categories

(MailNews Core :: Composition, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file)

While working on bug 1433193 (see bug 1433193 comment #6) I noticed that the HTMLLinkElement processing in nsMsgCompose.cpp and nsMsgSend.cpp
https://dxr.mozilla.org/comm-central/search?q=HTMLLinkElement+file%3AnsMsgCompose.cpp&redirect=false
https://dxr.mozilla.org/comm-central/search?q=HTMLLinkElement+file%3AnsMsgSend.cpp&redirect=false
is dead code since the objects processed there are all retrieved via HTMLEditor::GetEmbeddedObjects() which returns four things: Images ("img"), Anchors ("a"), "embed" and body elements with background:
https://dxr.mozilla.org/mozilla-central/rev/5454ed95c82a956009db2f4b04d008ec8753e61e/editor/libeditor/HTMLEditor.cpp#3007
There are no links returned.

So we have two options:
1) Change GetEmbeddedObjects() to also return link elements or 2) remove the dead processing from mailnews.

Since the TB HTML editor, to my knowledge, doesn't allow insertion of links (and the menu item "Insert > Link" inserts an anchor, see personal note below), we would only do this for people using add-ons or fancy templates.
I've tested adding a link to the header section of an e-mail, like
  <link rel="stylesheet" type="text/css" href="http://www.jorgk.com/jorgk.css">
and that gets honoured and shipped out. That must be another code path, most likely were the obtain the HTML of the body of the message from the editor.

So I'm NI'ing a few people here to find out which option we should use.

Personal note:
For a long time I've been confused about links since anchors are typically called "links" by the general public, like: "click on the link at the bottom of the page", etc. The real HTML link element is used in the header of a document to tie in other documents like style sheets or alternate versions, but there are other uses in the wild, like |<link rel='dns-prefetch' href=...|, |<link rel='shortlink' href=...| or even |<link rel='https://api.w.org/' href=...| (all seen at https://html.com/tags/link/.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(alta88)
Flags: needinfo?(acelists)
I don't know what the goals of this code are, so not going to be much help here...
Flags: needinfo?(bzbarsky)
Can you find the history of that code if we were processing links in the past?
It seems if the code rewrites/handles attributes that can be URLs, then <link> would be a candidate. But it depends if any thing useful can be loaded via link (apart from css), e.g. some image or other object.
Flags: needinfo?(acelists) → needinfo?(richard.marti)
(In reply to :aceman from comment #2)
> But it depends if any thing useful can be
> loaded via link (apart from css), e.g. some image or other object.
That's why I was hoping for some input from Boris. Sorry to trouble you again Boris, can you imagine any use of a link element in the body of an e-mail? Since M-C code doesn't return them, should we just remove the dead code in mailnews?
Flags: needinfo?(bzbarsky)
I checked the code and the one Jörg changed was imported from CVS. So very old code.
Flags: needinfo?(richard.marti)
> can you imagine any use of a link element in the body of an e-mail?

Imagine, sure.  Simple testcase:

  data:text/html,<style>link { display: block; } link::before { content: "click me"}</style><body><link href="http://example.com"></body>

Now whether anyone would do that, who knows.

Of course <link rel="stylesheet"> could go in the <body> as well, and often does on websites.
Flags: needinfo?(bzbarsky)
This dead code may be a remnant of the old netscape composer. Anyway, imo, if there is neither current usage nor anyone asking for the feature, there isn't a reason to implement it.
Flags: needinfo?(alta88)
Yeah I'd remove it.
Flags: needinfo?(mkmelin+mozilla)
With bug 1478546 now landed, GetEmbeddedObjects() is under C-C control and we know it doesn't return HTMLLinkElement. So as per comment #8, just remove the dead code.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9007715 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9007715 [details] [diff] [review]
1434575-remove-link-processing.patch

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

Sure, r=mkmelin
Attachment #9007715 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3c055eb08eb5
remove dead HTMLLinkElement processing since GetEmbeddedObjects() doesn't return them. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.