1134280 - Remove nsINode::Tag()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Tag() is ancient and misleading. Better to use newer IsHTML/XUL/SVG etc.
When Tag() like functionality is needed, one can use node->NodeInfo()->NameAtom()
(assuming NodeInfo() was moved from nsIContent to nsINode. Why is it in nsIContent?).

Comment 1

[:Gijs (he/him)]

Out of curiosity / for posterity, can you explain why this is sec-sensitive?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: tag.patch

Here a big patch. In here, there are many assumption of namespaces, and that can break things. For this reason I propose this:

1. a basic patch that keeps Tag() and adds code in nsINode.* and move code from nsIContent.*. That will not break anything

2. several patch for big components (dom/html, dom/svg, etc...). We can land them separately and we can fix regressions.

3. A patch that removes Tag().

All of these steps will be done in this bug. What do you think?

Comment 4

[Robert Longson [:longsonr]]

there are a number of redundant >IsSVG() and ->IsHTML() calls that could should be removed as part of the patch.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Which cases do you mean Robert?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8567006 [details] [diff] [review]
tag.patch

Could you perhaps do the first review?

nsINode and nsIContent changes look ok to me.

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 8567006 [details] [diff] [review]
tag.patch

In general this patch shows that we need to introduce some method that takes a variable number of arguments (or a list or an array) rather than

>+    if (content->IsHTML(nsGkAtoms::script) ||
>+        content->IsHTML(nsGkAtoms::iframe) ||
>+        content->IsHTML(nsGkAtoms::frameset) ||
>+        content->IsHTML(nsGkAtoms::frame) ||
>+        content->IsHTML(nsGkAtoms::code) ||
>+        content->IsHTML(nsGkAtoms::noscript) ||
>+        content->IsHTML(nsGkAtoms::style)) {

or

>+  if (IsMathML(nsGkAtoms::maction_) ||
>+      IsMathML(nsGkAtoms::maligngroup_) ||
>+      IsMathML(nsGkAtoms::malignmark_) ||
>+      IsMathML(nsGkAtoms::menclose_) ||
>+      IsMathML(nsGkAtoms::merror_) ||
>+      IsMathML(nsGkAtoms::mfenced_) ||
>+      IsMathML(nsGkAtoms::mfrac_) ||
>+      IsMathML(nsGkAtoms::mover_) ||
>+      IsMathML(nsGkAtoms::mpadded_) ||
>+      IsMathML(nsGkAtoms::mphantom_) ||
>+      IsMathML(nsGkAtoms::mprescripts_) ||
>+      IsMathML(nsGkAtoms::mroot_) ||
>+      IsMathML(nsGkAtoms::msqrt_) ||
>+      IsMathML(nsGkAtoms::msub_) ||
>+      IsMathML(nsGkAtoms::msubsup_) ||
>+      IsMathML(nsGkAtoms::msup_) ||
>+      IsMathML(nsGkAtoms::mtd_) ||
>+      IsMathML(nsGkAtoms::mtr_) ||
>+      IsMathML(nsGkAtoms::munder_) ||
>+      IsMathML(nsGkAtoms::munderover_) ||
>+      IsMathML(nsGkAtoms::none)) {

To avoid checking the namespace large numbers of times.


>diff --git a/accessible/html/HTMLSelectAccessible.cpp b/accessible/html/HTMLSelectAccessible.cpp
>--- a/accessible/html/HTMLSelectAccessible.cpp
>+++ b/accessible/html/HTMLSelectAccessible.cpp
>@@ -125,19 +125,18 @@ HTMLSelectListAccessible::CacheChildren(
>   // to count all the <optgroup>s and <option>s as children because we want
>   // a flat tree under the Select List.
>   for (nsIContent* childContent = mContent->GetFirstChild(); childContent;
>        childContent = childContent->GetNextSibling()) {
>     if (!childContent->IsHTML()) {
>       continue;
>     }

don't need the 3 lines above any more.

>   }
> 
>@@ -949,28 +949,28 @@ HTMLTableAccessible::IsProbablyLayoutTab
>   if (caption && caption->Role() == roles::CAPTION && caption->HasChildren()) 
>     RETURN_LAYOUT_ANSWER(false, "Not empty caption -- legitimate table structures");
> 
>   for (nsIContent* childElm = mContent->GetFirstChild(); childElm;
>        childElm = childElm->GetNextSibling()) {
>     if (!childElm->IsHTML())
>       continue;

or here.

> 
>-    if (childElm->Tag() == nsGkAtoms::col ||
>-        childElm->Tag() == nsGkAtoms::colgroup ||
>-        childElm->Tag() == nsGkAtoms::tfoot ||
>-        childElm->Tag() == nsGkAtoms::thead) {
>+    if (childElm->IsHTML(nsGkAtoms::col) ||
>+        childElm->IsHTML(nsGkAtoms::colgroup) ||
>+        childElm->IsHTML(nsGkAtoms::tfoot) ||
>+        childElm->IsHTML(nsGkAtoms::thead)) {
>       RETURN_LAYOUT_ANSWER(false,
>                            "Has col, colgroup, tfoot or thead -- legitimate table structures");
>     }
> 
>-    if (childElm->Tag() == nsGkAtoms::tbody) {
>+    if (childElm->IsHTML(nsGkAtoms::tbody)) {
>       for (nsIContent* rowElm = childElm->GetFirstChild(); rowElm;
>            rowElm = rowElm->GetNextSibling()) {
>-        if (rowElm->IsHTML() && rowElm->Tag() == nsGkAtoms::tr) {
>+        if (rowElm->IsHTML() && rowElm->IsHTML(nsGkAtoms::tr)) {

remove the first IsHTML() call

>           for (nsIContent* cellElm = rowElm->GetFirstChild(); cellElm;
>                cellElm = cellElm->GetNextSibling()) {
>             if (cellElm->IsHTML()) {
> 
>               if (cellElm->NodeInfo()->Equals(nsGkAtoms::th)) {

should probably make the above 3 lines IsHTML(nsGkAtoms::th)

>                 RETURN_LAYOUT_ANSWER(false,
>                                      "Has th -- legitimate table structures");
>               }

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -1649,26 +1649,24 @@ nsDOMWindowUtils::GetTranslationNodes(ns
>   // We begin iteration with content->GetNextNode because we want to explictly
>   // skip the root tag from being a translation node.
>   nsIContent* content = root;
>   while ((limit > 0) && (content = content->GetNextNode(root))) {
>     if (!content->IsHTML()) {
>       continue;
>     }

could remove the above 3 lines.

> 
>-    nsIAtom* localName = content->Tag();
>-
>     // Skip elements that usually contain non-translatable text content.
>-    if (localName == nsGkAtoms::script ||
>-        localName == nsGkAtoms::iframe ||
>-        localName == nsGkAtoms::frameset ||
>-        localName == nsGkAtoms::frame ||
>-        localName == nsGkAtoms::code ||
>-        localName == nsGkAtoms::noscript ||
>-        localName == nsGkAtoms::style) {
>+    if (content->IsHTML(nsGkAtoms::script) ||
>+        content->IsHTML(nsGkAtoms::iframe) ||
>+        content->IsHTML(nsGkAtoms::frameset) ||
>+        content->IsHTML(nsGkAtoms::frame) ||
>+        content->IsHTML(nsGkAtoms::code) ||
>+        content->IsHTML(nsGkAtoms::noscript) ||
>+        content->IsHTML(nsGkAtoms::style)) {
>       continue;
>     }


>     // If we don't have an action hint and
>     // return won't submit the form, use "next".
>     if (context.mActionHint.IsEmpty() &&
>-        inputContent->Tag() == nsGkAtoms::input) {
>+        inputContent->IsHTML(nsGkAtoms::input)) {
>       bool willSubmit = false;
>       nsCOMPtr<nsIFormControl> control(do_QueryInterface(inputContent));
>       mozilla::dom::Element* formElement = control->GetFormElement();
>       nsCOMPtr<nsIForm> form;
>       if (control) {
>         // is this a form and does it have a default submit element?
>         if ((form = do_QueryInterface(formElement)) &&
>             form->GetDefaultSubmitElement()) {
>           willSubmit = true;
>         // is this an html form and does it only have a single text input element?
>-        } else if (formElement && formElement->Tag() == nsGkAtoms::form &&
>+        } else if (formElement && formElement->IsHTML(nsGkAtoms::form) &&
>                    formElement->IsHTML() &&

above line is redundant.

> SVGSVGElement*
> SVGContentUtils::GetOuterSVGElement(nsSVGElement *aSVGElement)
> {
>   nsIContent *element = nullptr;
>   nsIContent *ancestor = aSVGElement->GetFlattenedTreeParent();
> 
>   while (ancestor && ancestor->IsSVG() &&

remove ancestor->IsSVG()

> SVGContentUtils::EstablishesViewport(nsIContent *aContent)
> {
>   // Although SVG 1.1 states that <image> is an element that establishes a
>   // viewport, this is really only for the document it references, not
>   // for any child content, which is what this function is used for.
>   return aContent && aContent->IsSVG() &&

remove aContent->IsSVG()

>-           (aContent->Tag() == nsGkAtoms::svg ||
>-            aContent->Tag() == nsGkAtoms::foreignObject ||
>-            aContent->Tag() == nsGkAtoms::symbol);
>+           (aContent->IsSVG(nsGkAtoms::svg) ||
>+            aContent->IsSVG(nsGkAtoms::foreignObject) ||
>+            aContent->IsSVG(nsGkAtoms::symbol));
> }
> 
> /* virtual */ void
> nsHTMLStyleSheet::RulesMatching(ElementRuleProcessorData* aData)
> {
>   nsRuleWalker *ruleWalker = aData->mRuleWalker;
>   if (aData->mElement->IsHTML() && !ruleWalker->AuthorStyleDisabled()) {

not sure the above IsHTML() call is useful any more.

>-    nsIAtom* tag = aData->mElement->Tag();
> 
>     // if we have anchor colors, check if this is an anchor with an href
>-    if (tag == nsGkAtoms::a) {
>+    if (aData->mElement->IsHTML(nsGkAtoms::a)) {
>       if (mLinkRule || mVisitedRule || mActiveRule) {
>         EventStates state =
>           nsCSSRuleProcessor::GetContentStateForVisitedHandling(
>                                   aData->mElement,
>                                   aData->mTreeMatchContext,
>                                   aData->mTreeMatchContext.VisitedHandling(),
>                                   // If the node being matched is a link,
>                                   // it's the relevant link.
>@@ -279,20 +278,20 @@ nsHTMLStyleSheet::RulesMatching(ElementR
>         // No need to add to the active rule if it's not a link
>         if (mActiveRule && nsCSSRuleProcessor::IsLink(aData->mElement) &&
>             state.HasState(NS_EVENT_STATE_ACTIVE)) {
>           ruleWalker->Forward(mActiveRule);
>         }
>       } // end link/visited/active rules
>     } // end A tag
>     // add the rule to handle text-align for a <th>
>-    else if (tag == nsGkAtoms::th) {
>+    else if (aData->mElement->IsHTML(nsGkAtoms::th)) {
>       ruleWalker->Forward(mTableTHRule);
>     }
>-    else if (tag == nsGkAtoms::table) {
>+    else if (aData->mElement->IsHTML(nsGkAtoms::table)) {
>       if (aData->mTreeMatchContext.mCompatMode == eCompatibility_NavQuirks) {
>         ruleWalker->Forward(mTableQuirkColorRule);
>       }
>     }
>   } // end html element
> 
>   // just get the style rules from the content.  For SVG we do this even if
>   // author style is disabled, because SVG presentational hints aren't

Comment 8

[Robert Longson [:longsonr]]

Oh and it sure could do with being broken up into smaller parts.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIContent and nsINode

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 2 - no Tag()

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 3 - last step

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 4 - remove nsIContentUtils::IsHTMLBlock

Comment 13

[Robert Longson [:longsonr]]

Comment on attachment 8567702 [details] [diff] [review]
patch 2 - no Tag()

I can give feedback which I did on the first patch but I'm a peer for these changes.

Comment 14

[Andrea Marchesini [:baku]]

A feedback is ok! Then I can ask smaug for a second review. Thanks.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8567274 [details] [diff] [review]
patch 1 - nsIContent and nsINode

I don't understand the IsTag methods. Why are we mixing use of words
"Tag" and "Element".
Please use "Element" only.

>+  template<typename First, typename... Args>
>+  inline bool IsTag(First aFirst, Args... aArgs) const
>+  {
>+    return mNodeInfo->Equals(aFirst, kNameSpaceID_XHTML) || IsTag(aArgs...);
>+  }
Why special case html there? really odd. How could that work for SVG or XUL?

Comment 16

[Robert Longson [:longsonr]]

Comment on attachment 8567702 [details] [diff] [review]
patch 2 - no Tag()

>   // We don't do this for <body>, <html>, <window>, <dialog> etc. which
>   // correspond to the doc accessible and will be created in any case
>-  if (!newAcc && content->Tag() != nsGkAtoms::body && content->GetParent() &&
>+  if (!newAcc && content->NodeInfo()->NameAtom() != nsGkAtoms::body &&
>+      content->GetParent() &&

This doesn't seem right. Should be checking for a html body using your new IsHTMLElement method.

>-  if (rootElm && (rootElm->Tag() == nsGkAtoms::dialog ||
>-                  rootElm->Tag() == nsGkAtoms::wizard))
>+  if (rootElm && (rootElm->NodeInfo()->NameAtom() == nsGkAtoms::dialog ||
>+                  rootElm->NodeInfo()->NameAtom() == nsGkAtoms::wizard))

Should be IsXULElement perhaps?

> 
>   HWND hWnd = GetHWNDFor(accessible);
>   NS_ENSURE_TRUE(hWnd, NS_ERROR_FAILURE);
> 
>   nsAutoString tag;
>   nsAutoCString id;
>   nsIContent* cnt = accessible->GetContent();
>   if (cnt) {
>-    cnt->Tag()->ToString(tag);
>+    cnt->NodeInfo()->NameAtom()->ToString(tag);

Maybe create an inline ElementName? method on content which does this?

>diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp
>--- a/dom/base/FragmentOrElement.cpp
>+++ b/dom/base/FragmentOrElement.cpp
>@@ -726,20 +726,20 @@ nsIContent::PreHandleEvent(EventChainPre
>               anonOwnerRelated = FindChromeAccessOnlySubtreeOwner(anonOwnerRelated);
>             }
>             if (anonOwner == anonOwnerRelated) {
> #ifdef DEBUG_smaug
>               nsCOMPtr<nsIContent> originalTarget =
>                 do_QueryInterface(aVisitor.mEvent->originalTarget);
>               nsAutoString ot, ct, rt;
>               if (originalTarget) {
>-                originalTarget->Tag()->ToString(ot);
>+                originalTarget->NodeInfo()->NameInfo()->ToString(ot);

could use it here too.

>               }
>-              Tag()->ToString(ct);
>-              relatedTarget->Tag()->ToString(rt);
>+              NodeInfo()->NameAtom()->ToString(ct);
>+              relatedTarget->NodeInfo()->NameAtom()->ToString(rt);

and here.

> 
> bool 
> nsDocumentEncoder::IsTag(nsIContent* aContent, nsIAtom* aAtom)
> {
>-  return aContent && aContent->Tag() == aAtom;
>+  return aContent && aContent->NodeInfo()->NameAtom() == aAtom;
> }

This method seems dangerous as it's not checking the namespace.

>diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
>--- a/dom/base/nsFocusManager.cpp
>+++ b/dom/base/nsFocusManager.cpp
>@@ -79,17 +79,17 @@ PRLogModuleInfo* gFocusNavigationLog;
> 
> #define LOGFOCUS(args) PR_LOG(gFocusLog, 4, args)
> #define LOGFOCUSNAVIGATION(args) PR_LOG(gFocusNavigationLog, 4, args)
> 
> #define LOGTAG(log, format, content)                            \
>   {                                                             \
>     nsAutoCString tag(NS_LITERAL_CSTRING("(none)"));            \
>     if (content) {                                              \
>-      content->Tag()->ToUTF8String(tag);                        \
>+      content->NodeInfo()->NameAtom()->ToUTF8String(tag);       \

Another chance to use ElementName

> 
>   if (aNode->IsNodeOfType(nsINode::eCONTENT)) {
>-    nsIContent* content = static_cast<nsIContent*>(aNode);
>-    if (content->Tag() == nsGkAtoms::documentTypeNodeName) {
>+    if (aNode->NodeInfo()->NameAtom() == nsGkAtoms::documentTypeNodeName) {

Seems not to be checking a namespace.

>       return nullptr;
>     }
> 
>+    nsIContent* content = static_cast<nsIContent*>(aNode);
>+

Why is this required?

>     if (!mMaySpanAnonymousSubtrees) {
>       // If the node is in a shadow tree then the ShadowRoot is the root.
>       ShadowRoot* containingShadow = content->GetContainingShadow();
>       if (containingShadow) {
>         return containingShadow;
>       }
> 
>       // If the node has a binding parent, that should be the root.
>diff --git a/dom/base/nsStyleLinkElement.cpp b/dom/base/nsStyleLinkElement.cpp
>--- a/dom/base/nsStyleLinkElement.cpp
>+++ b/dom/base/nsStyleLinkElement.cpp
>@@ -408,17 +408,17 @@ nsStyleLinkElement::DoUpdateStyleSheet(n
>   bool doneLoading = false;
>   nsresult rv = NS_OK;
>   if (isInline) {
>     nsAutoString text;
>     if (!nsContentUtils::GetNodeTextContent(thisContent, false, text)) {
>       return NS_ERROR_OUT_OF_MEMORY;
>     }
> 
>-    MOZ_ASSERT(thisContent->Tag() != nsGkAtoms::link,
>+    MOZ_ASSERT(thisContent->NodeInfo()->NameAtom() != nsGkAtoms::link,
>                "<link> is not 'inline', and needs different CSP checks");

IsHTMLElement?


>diff --git a/dom/smil/nsSMILAnimationController.cpp b/dom/smil/nsSMILAnimationController.cpp
>--- a/dom/smil/nsSMILAnimationController.cpp
>+++ b/dom/smil/nsSMILAnimationController.cpp
>@@ -773,17 +773,17 @@ nsSMILAnimationController::GetTargetIden
>   if (!aAnimElem->GetTargetAttributeName(&attributeNamespaceID,
>                                          getter_AddRefs(attributeName)))
>     // Animation has no target attr -- skip it.
>     return false;
> 
>   // animateTransform can only animate transforms, conversely transforms
>   // can only be animated by animateTransform
>   if (IsTransformAttribute(attributeNamespaceID, attributeName) !=
>-      (aAnimElem->Tag() == nsGkAtoms::animateTransform))
>+      (aAnimElem->NodeInfo()->NameAtom() == nsGkAtoms::animateTransform))

IsSVGElement

> 
>-        if (childContent->Tag() == nsGkAtoms::documentTypeNodeName) {
>+        if (childContent->NodeInfo()->NameAtom() == nsGkAtoms::documentTypeNodeName) {

Is???Element maybe.

> nsresult
> txMozillaXMLOutput::startHTMLElement(nsIContent* aElement, bool aIsHTML)
> {
>     nsresult rv = NS_OK;
>-    nsIAtom *atom = aElement->Tag();
>+    nsIAtom *atom = aElement->NodeInfo()->NameAtom();
> 
>     if ((atom != nsGkAtoms::tr || !aIsHTML) &&

!IsHTMLElement(nsGkAtoms::tr)

> nsresult
> txMozillaXMLOutput::endHTMLElement(nsIContent* aElement)
> {
>-    nsIAtom *atom = aElement->Tag();
>+    nsIAtom *atom = aElement->NodeInfo()->NameAtom();
> 
>     if (mTableState == ADDED_TBODY) {
>         NS_ASSERTION(atom == nsGkAtoms::tbody,
>                      "Element flagged as added tbody isn't a tbody");

IsHTMLElement(nsGkAtoms::tbody)

> 
> XULDocument::BroadcasterHookup::~BroadcasterHookup()
> {
> #ifdef PR_LOGGING

>         nsAutoCString attributeC,broadcasteridC;
>         attributeC.AssignWithConversion(attribute);
>         broadcasteridC.AssignWithConversion(broadcasterID);
>         PR_LOG(gXULLog, PR_LOG_WARNING,
>                ("xul: broadcaster hookup failed <%s attribute='%s'> to %s",
>-                nsAtomCString(tag).get(),
>+                nsAtomCString(mObservesElement->NodeInfo()->NameAtom()).get(),

use the new ElementName?

>                ("xul: broadcaster hookup <%s attribute='%s'> to %s",
>-                nsAtomCString(content->Tag()).get(),
>+                nsAtomCString(content->NodeInfo()->NameAtom()).get(),

use the new ElementName?

> 
>diff --git a/dom/xul/templates/nsXULContentBuilder.cpp b/dom/xul/templates/nsXULContentBuilder.cpp
>--- a/dom/xul/templates/nsXULContentBuilder.cpp
>+++ b/dom/xul/templates/nsXULContentBuilder.cpp
>@@ -452,19 +452,19 @@ nsXULContentBuilder::BuildContentFromTem
>                ("nsXULContentBuilder::BuildContentFromTemplate (is unique: %d)",
>                aIsUnique));
> 
>         nsAutoString id;
>         aChild->GetId(id);
> 
>         PR_LOG(gXULTemplateLog, PR_LOG_ALWAYS,
>                ("Tags: [Template: %s  Resource: %s  Real: %s] for id %s",
>-                nsAtomCString(aTemplateNode->Tag()).get(), 
>-                nsAtomCString(aResourceNode->Tag()).get(),
>-                nsAtomCString(aRealNode->Tag()).get(), NS_ConvertUTF16toUTF8(id).get()));
>+                nsAtomCString(aTemplateNode->NodeInfo()->NameAtom()).get(),
>+                nsAtomCString(aResourceNode->NodeInfo()->NameAtom()).get(),
>+                nsAtomCString(aRealNode->NodeInfo()->NameAtom()).get(), NS_ConvertUTF16toUTF8(id).get()));

Use new ElementName



I seem to be making the same 2 or 3 comments again and again. Probably worth creating a new patch with the changes in and I'll feedback on that if you like.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIContent and nsINode

Comment 18

[Andrea Marchesini [:baku]]

> >+    cnt->NodeInfo()->NameAtom()->ToString(tag);
> 
> Maybe create an inline ElementName? method on content which does this?

It would be another Tag()...

> > bool 
> > nsDocumentEncoder::IsTag(nsIContent* aContent, nsIAtom* aAtom)
> > {
> >-  return aContent && aContent->Tag() == aAtom;
> >+  return aContent && aContent->NodeInfo()->NameAtom() == aAtom;
> > }
> 
> This method seems dangerous as it's not checking the namespace.

Right. Another patch.

> >   if (aNode->IsNodeOfType(nsINode::eCONTENT)) {
> >-    nsIContent* content = static_cast<nsIContent*>(aNode);
> >-    if (content->Tag() == nsGkAtoms::documentTypeNodeName) {
> >+    if (aNode->NodeInfo()->NameAtom() == nsGkAtoms::documentTypeNodeName) {
> 
> Seems not to be checking a namespace.

Right, but which one? :)

> >+    nsIContent* content = static_cast<nsIContent*>(aNode);
> >+
> 
> Why is this required?

Yes. I just moved it from 2 lines above.


> > 
> >-    MOZ_ASSERT(thisContent->Tag() != nsGkAtoms::link,
> >+    MOZ_ASSERT(thisContent->NodeInfo()->NameAtom() != nsGkAtoms::link,
> >                "<link> is not 'inline', and needs different CSP checks");
> 
> IsHTMLElement?

It could be XUL/SVG too, right?

> > 
> >-        if (childContent->Tag() == nsGkAtoms::documentTypeNodeName) {
> >+        if (childContent->NodeInfo()->NameAtom() == nsGkAtoms::documentTypeNodeName) {
> 
> Is???Element maybe.

I don't know which one :/

Comment 19

[Robert Longson [:longsonr]]

> > > 
> > >-    MOZ_ASSERT(thisContent->Tag() != nsGkAtoms::link,
> > >+    MOZ_ASSERT(thisContent->NodeInfo()->NameAtom() != nsGkAtoms::link,
> > >                "<link> is not 'inline', and needs different CSP checks");
> > 
> > IsHTMLElement?
> 
> It could be XUL/SVG too, right?

There's no link tag in SVG. The html link tag may work/be used in SVG though.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 5 - nsDocumentEncoder::IsTag

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8567872 [details] [diff] [review]
patch 1 - nsIContent and nsINode



>-  inline bool IsSVG() const
>+  template<typename First, typename... Args>
>+  inline bool IsAnyOfHTMLElements(First aFirst, Args... aArgs) const
>+  {
>+    return IsElementInternal(kNameSpaceID_XHTML, aFirst, aArgs...);
>+  }
>+
>+  inline bool IsSVGElement() const
>   {
>     return IsInNamespace(kNameSpaceID_SVG);
>   }
> 
>-  inline bool IsSVG(nsIAtom* aTag) const
>+  inline bool IsSVGElement(nsIAtom* aTag) const
>   {
>     return mNodeInfo->Equals(aTag, kNameSpaceID_SVG);
Might be nicer to use IsElementInternal() here.
Though, IsElementInternal in nsINode is a tad misleading, since it doesn't guarantee it is about Element.
Perhaps rename IsElementInternal to IsNodeInternal





>+++ b/dom/base/nsIDocument.h
>@@ -1297,43 +1297,23 @@ public:
>                                  nsAString& Standalone) = 0;
> 
>   /**
>    * Returns true if this is what HTML 5 calls an "HTML document" (for example
>    * regular HTML document with Content-Type "text/html", image documents and
>    * media documents).  Returns false for XHTML and any other documents parsed
>    * by the XML parser.
>    */
>-  bool IsHTML() const
>-  {
>-    return mType == eHTML;
>-  }
>-  bool IsHTMLOrXHTML() const
>-  {
>-    return mType == eHTML || mType == eXHTML;
>-  }
>-  bool IsXML() const
>-  {
>-    return !IsHTML();
>-  }
>-  bool IsSVG() const
>-  {
>-    return mType == eSVG;
>-  }
>-  bool IsXUL() const
>-  {
>-    return mType == eXUL;
>-  }
Why you remove all this?
We sure use IsHTML() at least. And 
-  bool isHTML = document->IsHTML();
+  bool isHTML = document->IsHTMLElement();
change in nsContentUtils.cpp doesn't make sense.



>+protected:
>+  // These 2 methods are useful for the recursive templates IsHTMLElement,
>+  // IsSVGElement, etc.
>+  inline bool IsElementInternal(int aNs) const
uint32_t
and I'd call it IsNodeInternal

>+  template<typename First, typename... Args>
>+  inline bool IsElementInternal(int aNs, First aFirst, Args... aArgs) const
ditto


>+  {
>+    return mNodeInfo->Equals(aFirst, aNs) || IsElementInternal(aNs, aArgs...);
>+  }
You could make this faster by comparing namespace only once and then just
mNodeInfo->Equals(aFirst);
So perhaps drop namespace usage from the method and...


>+
>+public:
>+  inline bool IsHTMLElement() const
>+  {
>+    return IsElement() && IsInNamespace(kNameSpaceID_XHTML);
>+  }
>+
>+  inline bool IsHTMLElement(nsIAtom* aTag) const
>+  {
>+    return IsElement() && mNodeInfo->Equals(aTag, kNameSpaceID_XHTML);
>+  }
... here do
return IsHTMLElement() && IsNodeInternal(aTag);
Same also in other similar methods

> 
>-    return content->IsHTML(nsGkAtoms::table) || content->IsHTML(nsGkAtoms::td)
>-      || content->IsHTML(nsGkAtoms::th);
>+    return content->IsHTMLElement(nsGkAtoms::table) ||
>+           content->IsHTMLElement(nsGkAtoms::td) ||
>+           content->IsHTMLElement(nsGkAtoms::th);
Why you don't use the IsAnyOf variant here?


> nsHTMLDocument::Close(ErrorResult& rv)
> {
>-  if (!IsHTML()) {
>+  if (!IsHTMLElement()) {
You very much change the behavior here.
I wonder if IsFoo() in nsIDocument should be renamed to IsFooDocument() to make it more clear.
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsIDocument* doc = GetComposedDoc();
>-  if (doc && doc->IsSVG()) {
>+  if (doc && doc->IsSVGElement()) {
changing behavior here.

> nsSVGFELightingElement::ComputeLightAttributes(nsSVGFilterInstance* aInstance)
> {
>   // find specified light
>   for (nsCOMPtr<nsIContent> child = nsINode::GetFirstChild();
>        child;
>        child = child->GetNextSibling()) {
>-    if (child->IsSVG(nsGkAtoms::feDistantLight) ||
>-        child->IsSVG(nsGkAtoms::fePointLight) ||
>-        child->IsSVG(nsGkAtoms::feSpotLight)) {
>+    if (child->IsSVGElement(nsGkAtoms::feDistantLight) ||
>+        child->IsSVGElement(nsGkAtoms::fePointLight) ||
>+        child->IsSVGElement(nsGkAtoms::feSpotLight)) {
Why not use the IsAnyOf method here?

> XPathEvaluator::CreateExpression(const nsAString& aExpression,
>                                  XPathNSResolver* aResolver, ErrorResult& aRv)
> {
>     nsCOMPtr<nsIDocument> doc = do_QueryReferent(mDocument);
>-    XPathEvaluatorParseContext pContext(aResolver, !(doc && doc->IsHTML()));
>+    XPathEvaluatorParseContext pContext(aResolver, !(doc && doc->IsHTMLElement()));
changing behavior here


> XPathEvaluator::CreateExpression(const nsAString& aExpression,
>                                  nsINode* aResolver, ErrorResult& aRv)
> {
>     nsCOMPtr<nsIDocument> doc = do_QueryReferent(mDocument);
>-    XPathEvaluatorParseContext pContext(aResolver, !(doc && doc->IsHTML()));
>+    XPathEvaluatorParseContext pContext(aResolver, !(doc && doc->IsHTMLElement()));
ditto

>+++ b/dom/xslt/xslt/txMozillaXSLTProcessor.cpp
>@@ -189,17 +189,17 @@ txToFragmentHandlerFactory::createHandle
>         {
>             txOutputFormat format;
>             format.merge(*aFormat);
>             nsCOMPtr<nsIDOMDocument> domdoc;
>             mFragment->GetOwnerDocument(getter_AddRefs(domdoc));
>             NS_ASSERTION(domdoc, "unable to get ownerdocument");
>             nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
> 
>-            if (doc && doc->IsHTML()) {
>+            if (doc && doc->IsHTMLElement()) {
and here

>-  if (aList->IsElement() && aList->AsElement()->IsHTML(aListType)) {
>+  if (aList->IsElement() && aList->AsElement()->IsHTMLElement(aListType)) {
You could drop IsElement() check here.

>   aNodeArray.RemoveObjectAt(0);
>   if (curNode->IsElement() &&
>-      (curNode->AsElement()->IsHTML(nsGkAtoms::div) ||
>-       curNode->AsElement()->IsHTML(nsGkAtoms::blockquote))) {
>+      curNode->AsElement()->IsAnyOfHTMLElements(nsGkAtoms::div,
>+                                                nsGkAtoms::blockquote)) {
You could drop IsElement() check here and just do curNode->IsAnyOf...

> nsHTMLEditUtils::IsNamedAnchor(nsINode* aNode)
> {
>   MOZ_ASSERT(aNode);
>-  if (!aNode->IsElement() || !aNode->AsElement()->IsHTML(nsGkAtoms::a)) {
>+  if (!aNode->IsElement() || !aNode->AsElement()->IsHTMLElement(nsGkAtoms::a)) {
You could drop IsElement() check and just do !aNode->IsHTMLElement(nsGkAtoms::a)

> IsBRElement(nsINode* aNode)
> {
>   return aNode->IsElement() &&
>-         aNode->AsElement()->IsHTML(nsGkAtoms::br);
>+         aNode->AsElement()->IsHTMLElement(nsGkAtoms::br);
aNode->isHTMLElement(nsGkAtoms::br); should be enough here, right?

>         if (contentString.Length()) {
>-          if (mDocument->IsHTML() && aParentContent->IsHTML()) {
>+          if (mDocument->IsHTMLElement() && aParentContent->IsHTMLElement()) {
>             ToLowerCase(contentString);
>           }
>           attrName = do_GetAtom(contentString);
>         }
>       }
>       else {
>-        if (mDocument->IsHTML() && aParentContent->IsHTML()) {
>+        if (mDocument->IsHTMLElement() && aParentContent->IsHTMLElement()) {
You're changing the behavior here. document isn't ever an element.

>     styleSet->PrependStyleSheet(nsStyleSet::eOverrideSheet, sheet);
>   }
> 
>-  if (!aDocument->IsSVG()) {
>+  if (!aDocument->IsSVGElement()) {
Again, behavior change

>   // We assume all images are visible in print, print preview, chrome, xul, and
>   // resource docs and don't keep track of them.
>   if (mPresContext->Type() == nsPresContext::eContext_PrintPreview ||
>       mPresContext->Type() == nsPresContext::eContext_Print ||
>       mPresContext->IsChrome() ||
>       mDocument->IsResourceDoc() ||
>-      mDocument->IsXUL()) {
>+      mDocument->IsXULElement()) {
ditto.


I think the same issue has happened often enough that
IsFoo should indeed be changed to IsFooDocument() in nsIDocument.



interdiff for the next version of the patch would be nice.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIContent and nsINode

interdiff coming.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: interdiff - patch 1

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8568082 [details] [diff] [review]
interdiff - patch 1

> #ifdef MOZ_XUL
>   if (aClone && !aParent && aNode->IsElement() &&
>-      aNode->AsElement()->IsXULElement()) {
>+      aNode->IsXULElement()) {
No need for the aNode->IsElement check

> NodeAllowsClickThrough(nsINode* aNode)
> {
>   while (aNode) {
>-    if (aNode->IsElement() && aNode->AsElement()->IsXULElement()) {
>+    if (aNode->IsElement() && aNode->IsXULElement()) {
No need for the IsElement() check

> static bool MatchItems(nsIContent* aContent, int32_t aNameSpaceID, 
>                        nsIAtom* aAtom, void* aData)
> {
>-  if (!(aContent->IsElement() && aContent->AsElement()->IsHTMLElement())) {
>+  if (!(aContent->IsElement() && aContent->IsHTMLElement())) {
No need for the IsElement() check.

>-  if (aList->IsElement() && aList->AsElement()->IsHTMLElement(aListType)) {
>+  if (aList->IsElement() && aList->IsHTMLElement(aListType)) {
No need for the IsElement() check.

> nsHTMLEditUtils::IsNamedAnchor(nsINode* aNode)
> {
>   MOZ_ASSERT(aNode);
>-  if (!aNode->IsElement() || !aNode->AsElement()->IsHTMLElement(nsGkAtoms::a)) {
>+  if (!aNode->IsElement() || !aNode->IsHTMLElement(nsGkAtoms::a)) {
No need for the IsElement() check


>-    if (aNode->IsElement() && aNode->AsElement()->IsHTMLElement(nsGkAtoms::span) &&
>+    if (aNode->IsElement() && aNode->IsHTMLElement(nsGkAtoms::span) &&
No need for the IsElement() check

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8568081 [details] [diff] [review]
patch 1 - nsIContent and nsINode

"Get the NodeInfo for this element"
s/element/node/
Or just drop the rather useless comment.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIContent and nsINode

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch 2 - no Tag()

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8568134 [details] [diff] [review]
patch 2 - no Tag()

> 
>   bool isHTML = content->IsHTMLElement();
>-  if (isHTML && content->Tag() == nsGkAtoms::map) {
>+  if (isHTML && content->IsHTMLElement(nsGkAtoms::map)) {
do we need isHTML anymore?

but surkov needs to go through the accessibility parts.



> nsGenericHTMLElement::IsLabelable() const
> {
>-  return Tag() == nsGkAtoms::progress ||
>-         Tag() == nsGkAtoms::meter;
>+  return IsAnyOfHTMLElements(nsGkAtoms::progress, nsGkAtoms::meter);
just came to my mind that, we might want to have IsHTMLElement/IsAnyOfHTMLElements overridden
also in nsGenericHTMLElement, in case we could get rid of the namespace check.
Perhaps do that in a followup?
>-  if (element && element->Tag() == nsGkAtoms::svg) {
>+  if (element && element->IsSVGElement(nsGkAtoms::svg)) {
>     return static_cast<SVGSVGElement*>(element);
Is the old code here possibly a security issue? If so, we might want to fix it separately on branches
(before landing all these changes).

>   nsIContent* targetContent = mSource.get();
>   if (!targetContent || !targetContent->IsSVGElement())
>     return nullptr;
> 
>   // make sure target is valid type for <use>
>   // QIable nsSVGGraphicsElement would eliminate enumerating all elements
>-  nsIAtom *tag = targetContent->Tag();
>-  if (tag != nsGkAtoms::svg &&
>-      tag != nsGkAtoms::symbol &&
>-      tag != nsGkAtoms::g &&
>-      tag != nsGkAtoms::path &&
>-      tag != nsGkAtoms::text &&
>-      tag != nsGkAtoms::rect &&
>-      tag != nsGkAtoms::circle &&
>-      tag != nsGkAtoms::ellipse &&
>-      tag != nsGkAtoms::line &&
>-      tag != nsGkAtoms::polyline &&
>-      tag != nsGkAtoms::polygon &&
>-      tag != nsGkAtoms::image &&
>-      tag != nsGkAtoms::use)
>+  if (!targetContent->IsAnyOfSVGElements(nsGkAtoms::svg,
>+                                         nsGkAtoms::symbol,
>+                                         nsGkAtoms::g,
>+                                         nsGkAtoms::path,
>+                                         nsGkAtoms::text,
>+                                         nsGkAtoms::rect,
>+                                         nsGkAtoms::circle,
>+                                         nsGkAtoms::ellipse,
>+                                         nsGkAtoms::line,
>+                                         nsGkAtoms::polyline,
>+                                         nsGkAtoms::polygon,
>+                                         nsGkAtoms::image,
>+                                         nsGkAtoms::use))
>     return nullptr;
You don't need !targetContent->IsSVGElement() anymore

>@@ -1044,17 +1044,17 @@ nsXULContentBuilder::CreateContainerCont
>     nsIContent* container = nullptr;
> 
>     int32_t querySetCount = mQuerySets.Length();
> 
>     for (int32_t r = 0; r < querySetCount; r++) {
>         nsTemplateQuerySet* queryset = mQuerySets[r];
> 
>         nsIAtom* tag = queryset->GetTag();
>-        if (tag && tag != aElement->Tag())
>+        if (tag && !aElement->IsXULElement(tag))
>             continue;
Not clear to my why we want IsXUL check here. In other words, might be safer to keep the old behavior.


> nsXULContentBuilder::IsOpen(nsIContent* aElement)
> {
>     // Determine if this is a <treeitem> or <menu> element
>     if (!aElement->IsXULElement())
>         return true;
> 
>     // XXXhyatt Use the XBL service to obtain a base tag.
>-    nsIAtom *tag = aElement->Tag();
>-    if (tag == nsGkAtoms::menu ||
>-        tag == nsGkAtoms::menubutton ||
>-        tag == nsGkAtoms::toolbarbutton ||
>-        tag == nsGkAtoms::button ||
>-        tag == nsGkAtoms::treeitem)
>+    if (aElement->IsAnyOfXULElements(nsGkAtoms::menu,
>+                                     nsGkAtoms::menubutton,
>+                                     nsGkAtoms::toolbarbutton,
>+                                     nsGkAtoms::button,
>+                                     nsGkAtoms::treeitem))
>         return aElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::open,
>                                      nsGkAtoms::_true, eCaseMatters);
>     return true;

You could drop 'if (!aElement->IsXULElement())'


>@@ -1442,17 +1441,17 @@ nsXULContentBuilder::HasGeneratedContent
> 
>     nsCOMPtr<nsIRDFResource> rootresource;
>     nsresult rv = mRootResult->GetResource(getter_AddRefs(rootresource));
>     if (NS_FAILED(rv))
>         return rv;
> 
>     // the root resource is always acceptable
>     if (aResource == rootresource) {
>-        if (!aTag || mRoot->Tag() == aTag)
>+        if (!aTag || mRoot->IsXULElement(aTag))
It is not clear to me we want IsXUL check here.


>             do {
>                 nsTemplateMatch* match;
>                 if (content == mRoot || mContentSupportMap.Get(content, &match)) {
>                     // If we've got a tag, check it to ensure we're consistent.
>-                    if (!aTag || content->Tag() == aTag) {
>+                    if (!aTag || content->IsXULElement(aTag)) {
ditto

>     nsTemplateMatch *newmatch = nullptr;
>     if (aNewResult) {
>         // only allow a result to be inserted into containers with a matching tag
>         nsIAtom* tag = aQuerySet->GetTag();
>-        if (aInsertionPoint && tag && tag != aInsertionPoint->Tag())
>+        if (aInsertionPoint && tag && !aInsertionPoint->IsXULElement(tag))
ditto


>-        if ((!aContainer || !tag || tag == aContainer->Tag()) &&
>+        if ((!aContainer || !tag || aContainer->IsXULElement(tag)) &&
ditto


Great stuff.

But I think this should land in smaller pieces so that regression handling would be a tad simpler.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 8568134 [details] [diff] [review]
patch 2 - no Tag()

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

r=me for a11y part

::: accessible/base/nsAccessibilityService.cpp
@@ +930,5 @@
>      newAcc->AsTextLeaf()->SetText(text);
>      return newAcc;
>    }
>  
>    bool isHTML = content->IsHTMLElement();

isHTML bit should be removed, should be ok if dealt in separate changeset

@@ +931,5 @@
>      return newAcc;
>    }
>  
>    bool isHTML = content->IsHTMLElement();
> +  if (isHTML && content->IsHTMLElement(nsGkAtoms::map)) {

no isHTML

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8567703 [details] [diff] [review]
patch 3 - last step

This is the last patch to land.

Comment 31

[Andrea Marchesini [:baku]]

> just came to my mind that, we might want to have
> IsHTMLElement/IsAnyOfHTMLElements overridden
> also in nsGenericHTMLElement, in case we could get rid of the namespace
> check.
> Perhaps do that in a followup?

additional patch.


> >     return static_cast<SVGSVGElement*>(element);
> Is the old code here possibly a security issue? If so, we might want to fix
> it separately on branches

No. because we have IsSVGElement() check before setting 'element'.
I'm planning to split this patch in small blocks.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: patch 6 - nsGenericHTMLElement

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8567703 [details] [diff] [review]
patch 3 - last step

update IID of nsINode

(I probably missed to say that also IID of nsIContent and nsIDocument should be updated too)

Comment 34

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=0d6aeba7ac30

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a8d0fcf2af

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/133ec7304f22
https://hg.mozilla.org/mozilla-central/rev/869306cc4c2f
https://hg.mozilla.org/mozilla-central/rev/b80d9dc2137e
https://hg.mozilla.org/mozilla-central/rev/87f0b3d98a7e
https://hg.mozilla.org/mozilla-central/rev/1699d6dd7ac8
https://hg.mozilla.org/mozilla-central/rev/478041cbd895
https://hg.mozilla.org/mozilla-central/rev/c6c310429562
https://hg.mozilla.org/mozilla-central/rev/ff2b7abcf9ff
https://hg.mozilla.org/mozilla-central/rev/513d2df01638
https://hg.mozilla.org/mozilla-central/rev/ec0d84308d7c
https://hg.mozilla.org/mozilla-central/rev/9e7aa092d6ec
https://hg.mozilla.org/mozilla-central/rev/70f156455f82
https://hg.mozilla.org/mozilla-central/rev/4d58f27af3fc
https://hg.mozilla.org/mozilla-central/rev/5f8986f5a560
https://hg.mozilla.org/mozilla-central/rev/a1815f880a88
https://hg.mozilla.org/mozilla-central/rev/2c119ba8d267
https://hg.mozilla.org/mozilla-central/rev/a94a92b42ef9
https://hg.mozilla.org/mozilla-central/rev/0d6aeba7ac30
https://hg.mozilla.org/mozilla-central/rev/b638717d445d
https://hg.mozilla.org/mozilla-central/rev/f43843d3fb0e