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

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Jorg K (CEST = GMT+2)]

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/.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't know what the goals of this code are, so not going to be much help here...

Comment 2

[:aceman]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

(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?

Comment 4

[Richard Marti (:Paenglab)]

I checked the code and the one Jörg changed was imported from CVS. So very old code.

Comment 5

[Jorg K (CEST = GMT+2)]

What we should do is look at GetEmbeddedObjects(). I did a bit of digging and in 2003 that function already did NOT return any link elements:
https://searchfox.org/mozilla-central/commit/9371323b42c713f9a0cac6eb82a62f9dfd9a5c04
https://searchfox.org/mozilla-central/diff/9371323b42c713f9a0cac6eb82a62f9dfd9a5c04/editor/libeditor/html/nsHTMLEditor.cpp#3955

Also in 1999:
https://searchfox.org/mozilla-central/commit/6c385061f6c4ab2e45e6b25209279b6f1e313256
https://searchfox.org/mozilla-central/diff/6c385061f6c4ab2e45e6b25209279b6f1e313256/editor/libeditor/html/nsHTMLEditor.cpp#3213

So I really don't see why C-C is checking for link elements which weren't never returned since 1999.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[alta88]

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.

Comment 8

[Magnus Melin [:mkmelin]]

Yeah I'd remove it.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1434575-remove-link-processing.patch

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.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9007715 [details] [diff] [review]
1434575-remove-link-processing.patch

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

Sure, r=mkmelin

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3c055eb08eb5
remove dead HTMLLinkElement processing since GetEmbeddedObjects() doesn't return them. r=mkmelin