Closed Bug 1389650 Opened 7 years ago Closed 7 years ago

Remove nsIDOMHTMLAnchorElement

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1389650 - Change nsIDOMHTMLAnchorElement instanceof checks to getClassName;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1389650 - Remove nsIDOMHTMLAnchorElement;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Since it is no longer used (superceded by WebIDL and XPCOM extensions now deprecated), we can remove this XPCOM interface.
Is there any replacement of |nsCOMPtr<nsIDOMHTMLAnchorElement> anchor = do_QueryInterface(aNode);|?
Well, depends on the context, but for this bug I'm mostly doing:

nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
if (content->IsHTMLElement(nsGkAtoms::anchor)) ...

Then just using nsIContent->HasAttr/GetAttr after that for cleanup.

I'll upload my WIP patch here in a sec, still gotta fill out some missing XPCOM fields but it's mostly done.
Ok, WIP patch posted. This won't build because it's still missing quite a few fields we were picking up from XPCOM interface implementation. I'll take care of those on Monday.
Comment on attachment 8896521 [details]
Bug 1389650 - Change nsIDOMHTMLAnchorElement instanceof checks to getClassName;

https://reviewboard.mozilla.org/r/167774/#review173000

::: docshell/base/nsContextMenuInfo.cpp:68
(Diff revision 1)
>    aHRef.Truncate(0);
>  
> -  nsCOMPtr<nsIDOMElement> content(do_QueryInterface(mAssociatedLink));
> -  nsAutoString localName;
> -  if (content) {
> -    content->GetLocalName(localName);
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(mAssociatedLink);
> +  if (content->IsAnyOfHTMLElements(nsGkAtoms::anchor, nsGkAtoms::area, nsGkAtoms::link) &&
> +      content->HasAttr(kNameSpaceID_None, nsGkAtoms::href)) {
> +    content->GetAttr(kNameSpaceID_None, nsGkAtoms::href, aHRef);

I don't think this rewrite is appropreate because HTMLAnchorElement::GetHref is treated differently from GetAttr. (Making the URI absolute, for example.)

We should have something like nsIContent::AsAnchorElement.
Comment on attachment 8896521 [details]
Bug 1389650 - Change nsIDOMHTMLAnchorElement instanceof checks to getClassName;

https://reviewboard.mozilla.org/r/167774/#review173000

> I don't think this rewrite is appropreate because HTMLAnchorElement::GetHref is treated differently from GetAttr. (Making the URI absolute, for example.)
> 
> We should have something like nsIContent::AsAnchorElement.

Oh, yeah, you're totally correct on that, thanks for pointing that out! I made that change before making the edits to HTMLAnchorElement.h/cpp, so I wasn't aware of that when I did it. After the IsHTMLElement check passes, I think we should just be able to do a static_cast<HTMLAnchorElement*>(content) and can get GetHref from that.
See Also: → 1393419
Priority: -- → P2
I went with the ChromeUtils.getClassName solution for the chrome js 'cause it was the last viable thing posted in the dev-platform thread. If we're going to go ahead with .hasClassName, I'm fine to file a followup, and possible do the .hasClassName work myself since we're gonna see a lot of this kind of conversion in the XPCOM DOM element removal work.
Comment on attachment 8896521 [details]
Bug 1389650 - Change nsIDOMHTMLAnchorElement instanceof checks to getClassName;

https://reviewboard.mozilla.org/r/167774/#review187998

Hmm.  I guess ChromeUtils.getClassName already exists, so we're just reusing it...

I would really prefer it if this were split up into three patches:

1. The JS/JSM changes to stop using Ci.nsIDOMHTMLAnchorElement and replace with ChromeUtils.getClassName.
2. The FromContent infrastructure changes.
3. The rest of the changes.

That would not only make this simpler to review/understand, but also make bisection much simpler if there is any fallout (which I don't expect to happen, true).
Attachment #8896521 - Flags: review?(bzbarsky)
Comment on attachment 8896521 [details]
Bug 1389650 - Change nsIDOMHTMLAnchorElement instanceof checks to getClassName;

https://reviewboard.mozilla.org/r/167774/#review188020

r=me, but please file a comm-central bug to fix their uses too; they have some of this stuff in chat and editor.
Attachment #8896521 - Flags: review?(bzbarsky) → review+
Comment on attachment 8911285 [details]
Bug 1389650 - Remove nsIDOMHTMLAnchorElement;

https://reviewboard.mozilla.org/r/182780/#review188022

::: commit-message-d15f4:5
(Diff revision 1)
> +Bug 1389650 - Add FromContent helper macros for classes representing multiple tags; r=bz
> +
> +Currently, the FromContent helper macros only support classes that
> +represent a single tag. This would leave out classes like
> +HTMLAnchorElement, that presents 'anchor' and 'a'. This patch adds a

HTMLAnchorElement is always `<a>` as far as I can tell.  `<anchor>` gives you HTMLUnknownElement.

I'm going to mark this r- for the moment, because the basic premise here is wrong...
Attachment #8911285 - Flags: review?(bzbarsky) → review-
Comment on attachment 8911286 [details]
Bug 1389650 - Remove nsIDOMHTMLAnchorElement;

https://reviewboard.mozilla.org/r/182782/#review188024

::: docshell/base/nsContextMenuInfo.cpp:69
(Diff revision 1)
>  nsContextMenuInfo::GetAssociatedLink(nsAString& aHRef)
>  {
>    NS_ENSURE_STATE(mAssociatedLink);
>    aHRef.Truncate(0);
>  
> -  nsCOMPtr<nsIDOMElement> content(do_QueryInterface(mAssociatedLink));
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(mAssociatedLink);

Ick.  This is OK for now, but maybe file a followup to make `mAssociatedLink` an `Element*`?  ChromeContextMenuListener::HandleEvent should be able to provide one pretty easily here.  And then we wouldn't have to worry about whether it QIs to nsIContent (which not all  nsIDOMNodes do, but all current values of mAssociatedLink do, I think).

::: docshell/base/nsContextMenuInfo.cpp:71
(Diff revision 1)
> -    bool hasAttr;
> -    content->HasAttribute(NS_LITERAL_STRING("href"), &hasAttr);
> -    if (hasAttr) {
> -      linkContent = content;
> -      nsCOMPtr<nsIDOMHTMLAnchorElement> anchor(do_QueryInterface(linkContent));
> -      if (anchor) {
> +  if (anchor) {

This is not equivalent to the old code.  The old code used to only do this if there was an "href" attribute on the element...  That check needs to stay.

::: docshell/base/nsContextMenuInfo.cpp:94
(Diff revision 1)
> -        content->HasAttribute(NS_LITERAL_STRING("href"), &hasAttr);
> -        if (hasAttr) {
> -          linkContent = content;
> -          nsCOMPtr<nsIDOMHTMLAnchorElement> anchor(
> -            do_QueryInterface(linkContent));
> -          if (anchor) {
> +    if (anchor) {

Again, this lost the check for an href attr existing...

::: docshell/base/nsDocShell.cpp:14415
(Diff revision 1)
>  
>    nsAutoString target(aTargetSpec);
>  
>    // If this is an anchor element, grab its type property to use as a hint
>    nsAutoString typeHint;
> -  nsCOMPtr<nsIDOMHTMLAnchorElement> anchor(do_QueryInterface(aContent));
> +  if (IsElementAnchor(aContent) &&

This will include `<area>` elements.  That's not what the old code did; why do we want to do that?

::: docshell/base/nsDocShell.cpp:14416
(Diff revision 1)
>    nsAutoString target(aTargetSpec);
>  
>    // If this is an anchor element, grab its type property to use as a hint
>    nsAutoString typeHint;
> -  nsCOMPtr<nsIDOMHTMLAnchorElement> anchor(do_QueryInterface(aContent));
> -  if (anchor) {
> +  if (IsElementAnchor(aContent) &&
> +      aContent->HasAttr(kNameSpaceID_None, nsGkAtoms::type)) {

The old code didn't do this check, right?  I guess it's theoretically a performance optimization, maybe, but do we really need it?

::: dom/base/nsContentAreaDragDrop.cpp:509
(Diff revision 1)
>  
>      {
>        // set for linked images, and links
>        nsCOMPtr<nsIContent> linkNode;
>  
> -      if (area) {
> +      RefPtr<HTMLAreaElement> areaElem = HTMLAreaElement::FromContent(draggedNode);

Are we guaranteed that draggedNode is not null?  That's not obvious to me.

Probably safer to use FromContentOrNull, which would match the old code.

::: dom/base/nsContentAreaDragDrop.cpp:510
(Diff revision 1)
>      {
>        // set for linked images, and links
>        nsCOMPtr<nsIContent> linkNode;
>  
> -      if (area) {
> +      RefPtr<HTMLAreaElement> areaElem = HTMLAreaElement::FromContent(draggedNode);
> +      RefPtr<HTMLAnchorElement> anchor = HTMLAnchorElement::FromContent(draggedNode);

And here.  Note that you could probably move the FromContentOrNull calls into the if condition itself to avoid this useless-seeming set of "anchor" all the way up here.

::: dom/html/HTMLAnchorElement.h:40
(Diff revision 1)
>  
>    // CC
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(HTMLAnchorElement,
>                                             nsGenericHTMLElement)
>  
> +  NS_IMPL_FROMCONTENT_HTML_WITH_TAGS(HTMLAnchorElement, (a, anchor));

This is wrong, and I expect it to crash if actual elements with localname "anchor" end up in the system.

::: dom/html/HTMLAnchorElement.h:98
(Diff revision 1)
> -  // The XPCOM GetTarget is OK for us
> +  void GetTarget(nsAString& aValue);
>    void SetTarget(const nsAString& aValue, mozilla::ErrorResult& rv)
>    {
>      SetHTMLAttr(nsGkAtoms::target, aValue, rv);
>    }
> -  void GetDownload(DOMString& aValue)
> +  void GetDownload(nsAString& aValue)

Why this change?

::: dom/html/HTMLAnchorElement.h:106
(Diff revision 1)
>    }
>    void SetDownload(const nsAString& aValue, mozilla::ErrorResult& rv)
>    {
>      SetHTMLAttr(nsGkAtoms::download, aValue, rv);
>    }
> -  // The XPCOM GetPing is OK for us
> +  void GetPing(nsAString& aValue)

Why not DOMString?

::: dom/html/HTMLAnchorElement.h:114
(Diff revision 1)
> +  }
>    void SetPing(const nsAString& aValue, mozilla::ErrorResult& rv)
>    {
>      SetHTMLAttr(nsGkAtoms::ping, aValue, rv);
>    }
> -  void GetRel(DOMString& aValue)
> +  void GetRel(nsAString& aValue)

Why this change?

::: dom/html/HTMLAnchorElement.h:131
(Diff revision 1)
>    void GetReferrerPolicy(nsAString& aReferrer)
>    {
>      GetEnumAttr(nsGkAtoms::referrerpolicy, EmptyCString().get(), aReferrer);
>    }
>    nsDOMTokenList* RelList();
> -  void GetHreflang(DOMString& aValue)
> +  void GetHreflang(nsAString& aValue)

Why this change?

::: dom/html/HTMLAnchorElement.h:139
(Diff revision 1)
>    }
>    void SetHreflang(const nsAString& aValue, mozilla::ErrorResult& rv)
>    {
>      SetHTMLAttr(nsGkAtoms::hreflang, aValue, rv);
>    }
> -  void GetType(DOMString& aValue)
> +  void GetType(nsAString& aValue)

Why this change?

::: dom/html/HTMLAnchorElement.h:179
(Diff revision 1)
>    // Link::Link::SetSearch is OK for us
>  
>    // Link::Link::GetHash is OK for us
>    // Link::Link::SetHash is OK for us
>  
>    // The XPCOM URI decomposition attributes are fine for us

This comment is outdated (even before your patch) and should go away.  There's no more XPCOM here.

::: dom/html/HTMLAnchorElement.h:180
(Diff revision 1)
>  
>    // Link::Link::GetHash is OK for us
>    // Link::Link::SetHash is OK for us
>  
>    // The XPCOM URI decomposition attributes are fine for us
> -  void GetCoords(DOMString& aValue)
> +  void GetCoords(nsAString& aValue)

This is getting to be a pattern.  Why all these changes away from DOMString?

::: dom/html/HTMLAnchorElement.cpp:61
(Diff revision 1)
>  {
>    return HasAttr(kNameSpaceID_None, nsGkAtoms::href) ||
>           nsGenericHTMLElement::IsInteractiveHTMLContent(aIgnoreTabindex);
>  }
>  
> -NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED(HTMLAnchorElement,
> +NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED(HTMLAnchorElement)

This is crying out for a NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED0 that just takes the parent class name.  Followup for that, please?

And as long as we're there, in that followup you could switch HTMLOutputElement to NS_IMPL_QUERY_INTERFACE_CYCLE_COLLECTION_INHERITED, I think.

::: dom/html/HTMLAnchorElement.cpp:283
(Diff revision 1)
> -#undef IMPL_URI_PART
> -
> -NS_IMETHODIMP
>  HTMLAnchorElement::GetText(nsAString& aText)
>  {
> -  if(!nsContentUtils::GetNodeTextContent(this, true, aText, fallible)) {
> +  nsContentUtils::GetNodeTextContent(this, true, aText);

This doesn't seem right.  The old code used to silently return empty string on OOM, while the new code will crash.  Please either restore the old behavior or have it propagate the OOM out (mark .text as [Throws] in the webidl).

::: editor/libeditor/HTMLEditUtils.cpp:339
(Diff revision 1)
>  bool
>  HTMLEditUtils::IsLink(nsINode* aNode)
>  {
>    MOZ_ASSERT(aNode);
>  
> -  nsCOMPtr<nsIDOMHTMLAnchorElement> anchor = do_QueryInterface(aNode);
> +  RefPtr<HTMLAnchorElement> anchor = HTMLAnchorElement::FromContentOrNull(aNode->AsContent());

How do you know aNode is AsContent()?  This probably needs to check IsContent() or something.  In particular, aNode->AsContent() does NOT return null in the non-nsIContent case.  Instead it will just unsafely cast, after asserting IsContent().

::: editor/libeditor/HTMLEditor.cpp:2599
(Diff revision 1)
>    }
>  
> +
>    // Be sure we were given an anchor element
> -  nsCOMPtr<nsIDOMHTMLAnchorElement> anchor = do_QueryInterface(aAnchorElement);
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aAnchorElement);
> +  if (!content) {

Could just use FromContentOrNull, since that's hat you want anyway.
Attachment #8911286 - Flags: review?(bzbarsky) → review-
Attachment #8911286 - Attachment is obsolete: true
Comment on attachment 8911285 [details]
Bug 1389650 - Remove nsIDOMHTMLAnchorElement;

https://reviewboard.mozilla.org/r/182780/#review188128

::: docshell/base/nsContextMenuInfo.cpp:71
(Diff revision 2)
> -    bool hasAttr;
> -    content->HasAttribute(NS_LITERAL_STRING("href"), &hasAttr);
> -    if (hasAttr) {
> -      linkContent = content;
> -      nsCOMPtr<nsIDOMHTMLAnchorElement> anchor(do_QueryInterface(linkContent));
> -      if (anchor) {
> +  if (anchor) {

This is still not equivalent to the old code.  Why the change?

::: docshell/base/nsContextMenuInfo.cpp:94
(Diff revision 2)
> -        content->HasAttribute(NS_LITERAL_STRING("href"), &hasAttr);
> -        if (hasAttr) {
> -          linkContent = content;
> -          nsCOMPtr<nsIDOMHTMLAnchorElement> anchor(
> -            do_QueryInterface(linkContent));
> -          if (anchor) {
> +    if (anchor) {

And here.

::: dom/html/HTMLAnchorElement.h:126
(Diff revision 2)
>    }
>    void SetReferrerPolicy(const nsAString& aValue, mozilla::ErrorResult& rv)
>    {
>      SetHTMLAttr(nsGkAtoms::referrerpolicy, aValue, rv);
>    }
> -  void GetReferrerPolicy(nsAString& aReferrer)
> +  void GetReferrerPolicy(DOMString& aReferrer)

While you're here, maybe name the arg aPolicy?

::: dom/webidl/HTMLAnchorElement.webidl:34
(Diff revision 2)
>             [CEReactions, SetterThrows]
>             attribute DOMString hreflang;
>             [CEReactions, SetterThrows]
>             attribute DOMString type;
>  
> -           [CEReactions, SetterThrows]
> +           [CEReactions, SetterThrows, GetterThrows]

[CEReactions, Throws]
    
please.

::: editor/libeditor/HTMLEditUtils.cpp:350
(Diff revision 2)
> +    return false;
>    }
> +
> +  nsAutoString tmpText;
> +  anchor->GetHref(tmpText);
> +  if (tmpText.IsEmpty()) {

Could just:

  return !tmpText.IsEmpty();
  
but either way.
Attachment #8911285 - Flags: review?(bzbarsky) → review-
I reverted the nsContextMenuInfo code to the original logic, and tried to just change types and checks. As most of this code is from the CVS days, I did need to switch out nsIDOM* for nsIContent in order to get us the FromContent calls (would rather that than static_casts if I can), and also used that to clean up the element type checks. Hoping I'm not out of bounds on that assumption, but if so let me know and I'll continue fixing.
Comment on attachment 8911285 [details]
Bug 1389650 - Remove nsIDOMHTMLAnchorElement;

https://reviewboard.mozilla.org/r/182780/#review188628

r=me
Attachment #8911285 - Flags: review?(bzbarsky) → review+
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d5710ae6c0
Change nsIDOMHTMLAnchorElement instanceof checks to getClassName; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/26805294a547
Remove nsIDOMHTMLAnchorElement; r=bz
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3b3111d976
Change nsIDOMHTMLAnchorElement instanceof checks to getClassName; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba0ddc53833
Remove nsIDOMHTMLAnchorElement; r=bz
Flags: needinfo?(kyle)
Depends on: 1403516
Boris and Kyle, I'm trying to fix the bustage in bug 1403516, but when I include
mozilla/dom/HTMLAnchorElement.h
I get:
Cannot open include file: 'nsDOMTokenList.h': No such file or directory

So there appears to be something wrong in the M-C setup. Is this not "exported"?
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
Oh, the next one is:
Cannot open include file: 'nsStyleLinkElement.h': No such file or directory
Maybe the full message is more helpful:
10:42.60 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\mozilla/dom/HTMLAnchorElement.h(13): fatal error C1083: Cannot open include file: 'nsDOMTokenList.h': No such file or directory
0:58.30 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\mozilla/dom/HTMLLinkElement.h(14): fatal error C1083: Cannot open include file: 'nsStyleLinkElement.h': No such file or directory
BTW, will HTMLLinkElement::FromContent() and HTMLAnchorElement::FromContent() work? For body that's missing, so I had to hack it:
https://dxr.mozilla.org/comm-central/rev/eb81a4091d3cd18a78ba26caa8b62cd2945aa322/mailnews/compose/src/nsMsgSend.cpp#1289
I'll follow up in bug 1403516.
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
Kyle, did you ever file a comm-central bug on this?  They seem to be pretty confused about how to fix the JS side of this stuff... I'll set them straight, but a bug describing what they should do would have helped.
Sorry, totally forgot to file that. I'll get those filed for the other ones, and they can ni? me for that support too.
I've just removed use of nsIDOMHTMLImageElement in our C++ code, bug 1404734. There are some remnants in the SeaMonkey editor code as per bug 1403516 comment #31, but we'll take care of that.

Let us know before you remove nsIDOMNode ;-)
Oh, actually, I did file it, with instructions on what needed to be fixed:

https://bugzilla.mozilla.org/show_bug.cgi?id=1403067

I just didn't assign it so I guess it didn't get picked up? I'll dupe bug 1403067 against bug 1403516.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: