1423990 - Move the last few attribute-related methods from nsIContent to Element.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

I didn't do it in bug 1423167 because they were a bit harder, and would have made that patch gigantic, but this should be doable now.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

Review commit: https://reviewboard.mozilla.org/r/206348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206348/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206348/diff/1-2/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206348/diff/2-3/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

There's still some OSX-only stuff and some accessibility orange which I don't know if it's because of this patch or other one I had on the stack. Cancelling review for now, will revise tomorrow.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206348/diff/3-4/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review213548

r- for the nsContentList.cpp and nsContentSink.cpp bits, at least...

::: accessible/base/ARIAMap.cpp:1378
(Diff revision 4)
>  bool
>  aria::HasDefinedARIAHidden(nsIContent* aContent)
>  {
>    return aContent &&
>      nsAccUtils::HasDefinedARIAToken(aContent, nsGkAtoms::aria_hidden) &&
> -    !aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::aria_hidden,
> +    (!aContent->IsElement() ||

Not sure you need the IsElement() check here.  HasDefinedARIAToken always returns false for non-elements, right?

I guess this might be a bit safer...

::: accessible/base/nsAccUtils.cpp:144
(Diff revision 4)
>    while (ancestor) {
>  
>      // container-relevant attribute
>      if (relevant.IsEmpty() &&
>          HasDefinedARIAToken(ancestor, nsGkAtoms::aria_relevant) &&
> -        ancestor->GetAttr(kNameSpaceID_None, nsGkAtoms::aria_relevant, relevant))
> +        ancestor->IsElement() &&

Again, HasDefinedARIAToken returns false if !IsElement(), so no need for this check afaict.

::: accessible/base/nsAccUtils.cpp:155
(Diff revision 4)
>      if (live.IsEmpty()) {
>        const nsRoleMapEntry* role = nullptr;
>        if (ancestor->IsElement()) {
>          role = aria::GetRoleMap(ancestor->AsElement());
>        }
> -      if (HasDefinedARIAToken(ancestor, nsGkAtoms::aria_live)) {
> +      if (ancestor->IsElement() &&

No need for the check here, since HasDefinedARIAToken returns false on non-elements.  But if we do want to keep checking, maybe reuse the existing check we have just above?

::: accessible/base/nsAccUtils.cpp:182
(Diff revision 4)
>      }
>  
>      // container-busy attribute
>      if (busy.IsEmpty() &&
>          HasDefinedARIAToken(ancestor, nsGkAtoms::aria_busy) &&
> -        ancestor->GetAttr(kNameSpaceID_None, nsGkAtoms::aria_busy, busy))
> +        ancestor->IsElement() &&

Again, no need given HasDefinedARIAToken returned true.

::: accessible/base/nsAccUtils.cpp:203
(Diff revision 4)
>    NS_ASSERTION(aContent, "aContent is null in call to HasDefinedARIAToken!");
>  
> -  if (!aContent->HasAttr(kNameSpaceID_None, aAtom) ||
> -      aContent->AttrValueIs(kNameSpaceID_None, aAtom,
> +  if (!aContent->IsElement())
> +    return false;
> +
> +  if (!aContent->AsElement()->HasAttr(kNameSpaceID_None, aAtom) ||

Might be cleaner to just store aContent->AsElement() in a local.

::: accessible/base/nsAccessibilityService.cpp:216
(Diff revision 4)
>  {
> -  if (aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
> +  if (!aContent->IsElement()) {
> +    return nullptr;
> +  }
> +
> +  if (aContent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,

Again, would be nice to store aContent->AsElement() in a local.

::: accessible/generic/ARIAGridAccessible.cpp:646
(Diff revision 4)
>      return;
>  
>    nsIContent *rowContent = row->GetContent();
>    if (nsAccUtils::HasDefinedARIAToken(rowContent,
>                                        nsGkAtoms::aria_selected) &&
> -      !rowContent->AttrValueIs(kNameSpaceID_None,
> +      (!rowContent->IsElement() ||

HasDefinedARIAToken tested true, so you know it's an element.

::: accessible/generic/Accessible.cpp:491
(Diff revision 4)
>  }
>  
>  bool
>  Accessible::NativelyUnavailable() const
>  {
> +  if (!mContent->IsElement())

Could move this to after the IsHTMLElement() check, so in the common case that it is we don't do an extra branch here.

::: accessible/generic/Accessible.cpp:836
(Diff revision 4)
>    nsAutoString ancestorTitle;
>    while (parent) {
>      if (parent->IsXULElement(nsGkAtoms::toolbaritem) &&
> -        parent->GetAttr(kNameSpaceID_None, nsGkAtoms::title, ancestorTitle)) {
> +        parent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::title, ancestorTitle)) {
>        // Before returning this, check if the element itself has a tooltip:
> -      if (aElm->GetAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext, aName)) {
> +      if (aElm->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext, aName)) {

What guarantees that aElm is an Element?

::: accessible/generic/Accessible.cpp:1466
(Diff revision 4)
>        // For simplicity, any existing pressed attribute except "" or "undefined"
>        // indicates a toggle.
>        return roles::TOGGLE_BUTTON;
>      }
>  
> -    if (mContent->AttrValueIs(kNameSpaceID_None,
> +    if (mContent->AsElement()->AttrValueIs(kNameSpaceID_None,

Are we guaranteed that mContent is an Element here?  Why?  Worth a comment if we are for some reason.

::: accessible/xul/XULMenuAccessible.cpp:148
(Diff revision 4)
>  
>  void
>  XULMenuitemAccessible::Description(nsString& aDescription)
>  {
> -  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::description,
> +  mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::description,
>                      aDescription);

Fix indent?

::: accessible/xul/XULMenuAccessible.cpp:161
(Diff revision 4)
>  
>    // We do not use nsCoreUtils::GetAccesskeyFor() because accesskeys for
>    // menu are't registered by EventStateManager.
>    nsAutoString accesskey;
> -  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accesskey,
> +  mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::accesskey,
>                      accesskey);

Fix indent?

::: dom/base/Element.h:805
(Diff revision 4)
>    inline bool AttrValueIs(int32_t aNameSpaceID, nsAtom* aName,
>                            const nsAString& aValue,
>                            nsCaseTreatment aCaseSensitive) const;
> -  inline bool AttrValueIs(int32_t aNameSpaceID, nsAtom* aName,
> +
> +  /**
> +   * Test whether this content node's given attribute has the given value.  If

s/content node/Element/

::: dom/base/Element.h:824
(Diff revision 4)
> +  enum {
> +    ATTR_MISSING = -1,
> +    ATTR_VALUE_NO_MATCH = -2
> +  };
> +  /**
> +   * Check whether this content node's given attribute has one of a given

s/content node/Element/

::: dom/base/nsContentList.cpp:563
(Diff revision 4)
>      // XXX Should this pass eIgnoreCase?
>      if (content &&
>          ((content->IsHTMLElement() &&
> -          content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
> +          content->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
> -                               name, eCaseMatters)) ||
> +                                            name, eCaseMatters)) ||
> -         content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::id,
> +         content->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::id,

I don't think `content` is generally guaranteed to be an Element here...  Need to check it.

::: dom/base/nsContentSink.cpp:800
(Diff revision 4)
>                 aElement->NodeType() == nsIDOMNode::PROCESSING_INSTRUCTION_NODE,
>                 "We only expect processing instructions here");
>  
>    nsAutoString integrity;
>    if (aElement) {
> -    aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);
> +    aElement->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);

This doesn't look right, and I'm surprised it passes try.  Per the assert above, aElement is a PI, not an element, if non-null!  So AsElement() should be asserting fatally.  How is this passing try?

I filed bug 1426234 on this code not making sense.  For your part, I think you should just comment that GetAttr line out until the SRI people can figure out what they are actually trying to do.  Since right now we never make "integrity" nonempty, commenting it out will preserve behavior.

::: dom/base/nsContentSink.cpp:835
(Diff revision 4)
>  
>  nsresult
>  nsContentSink::ProcessMETATag(nsIContent* aContent)
>  {
>    NS_ASSERTION(aContent, "missing meta-element");
> +  MOZ_ASSERT(aContent->IsElement());

Might be nice to just store the Element* in a temporary here too.

::: dom/base/nsObjectLoadingContent.cpp:1597
(Diff revision 4)
>    /// Codebase
>    ///
>  
>    nsAutoString codebaseStr;
>    nsCOMPtr<nsIURI> docBaseURI = thisContent->GetBaseURI();
> -  bool hasCodebase = thisContent->HasAttr(kNameSpaceID_None, nsGkAtoms::codebase);
> +  thisContent->AsElement()->GetAttr(

Could use a thisElement temporary here to avoid lots of AsElement() sprinkled about this method.

::: dom/base/nsTextNode.cpp:245
(Diff revision 4)
>    nsresult rv = nsTextNode::BindToTree(aDocument, aParent,
>                                         aBindingParent, aCompileEventHandlers);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    NS_ASSERTION(!mGrandparent, "We were already bound!");
> -  mGrandparent = aParent->GetParent();
> +  mGrandparent = aParent->GetParent()->AsElement();

How is this safe?  What if our grandparent is a document fragment?

::: dom/html/HTMLImageElement.cpp:1223
(Diff revision 4)
>    if (!src->MatchesCurrentMedia()) {
>      return false;
>    }
>  
>    nsAutoString type;
> -  if (aSourceNode->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type) &&
> +  if (aSourceNode->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type) &&

Could just use "src" here instead.

::: dom/html/HTMLImageElement.cpp:1232
(Diff revision 4)
>  
>    return true;
>  }
>  
>  bool
>  HTMLImageElement::TryCreateResponsiveSelector(nsIContent *aSourceNode)

Both callers are explicitly passing candidateSource->AsContent(), but know that candidateSource is an Element.  Maybe change this method to take `Element* aSourceElement` and have them pass candidateSource->AsElement()?  Then you don't need the IsElement() check in this method, or the AsElement() bits in it.

::: dom/xbl/nsXBLWindowKeyHandler.cpp:207
(Diff revision 4)
>        // Check whether the key element has empty value at key/char attribute.
>        // Such element is used by localizers for alternative shortcut key
>        // definition on the locale. See bug 426501.
>        nsAutoString valKey, valCharCode, valKeyCode;
>        bool attrExists =
> -        key->GetAttr(kNameSpaceID_None, nsGkAtoms::key, valKey) ||
> +        key->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::key, valKey) ||

Might be nice to store key->AsElement() in a temporary here.

::: dom/xul/XULDocument.cpp:873
(Diff revision 4)
>  
>          // Look for an <observes> element beneath the listener. This
>          // ought to have an |element| attribute that refers to
>          // aBroadcaster, and an |attribute| element that tells us what
>          // attriubtes we're listening for.
> -        if (!child->NodeInfo()->Equals(nsGkAtoms::observes, kNameSpaceID_XUL))
> +        if (!child->IsAnyOfXULElements(nsGkAtoms::observes))

IsXULElement(nsGkAtoms::observes), if there's only one type we care about.

::: dom/xul/XULDocument.cpp:4264
(Diff revision 4)
>  
>      nsAutoString posStr;
>      bool wasInserted = false;
>  
>      // insert after an element of a given id
> -    aChild->GetAttr(kNameSpaceID_None, nsGkAtoms::insertafter, posStr);
> +    if (aChild->IsElement())

Curlies around if body, please.

::: dom/xul/XULDocument.cpp:4269
(Diff revision 4)
> -    aChild->GetAttr(kNameSpaceID_None, nsGkAtoms::insertafter, posStr);
> -    bool isInsertAfter = true;
> +    if (aChild->IsElement())
> +        aChild->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::insertafter, posStr);
>  
> +    bool isInsertAfter = true;
>      if (posStr.IsEmpty()) {
> -        aChild->GetAttr(kNameSpaceID_None, nsGkAtoms::insertbefore, posStr);
> +        if (aChild->IsElement())

Curlies around if body.

::: dom/xul/nsXULElement.cpp:975
(Diff revision 4)
>      nsIDocument* doc = GetComposedDoc();
>      if (doc && !aOldValue.IsEmpty()) {
>          nsIPresShell *shell = doc->GetShell();
>  
>          if (shell) {
> -            nsIContent *content = this;
> +            Element* element= this;

Space before `=`.

::: dom/xul/templates/nsXULTemplateQueryProcessorRDF.cpp:1110
(Diff revision 4)
>      // should use to test for containment.
>      nsresult rv;
>  
>      mContainmentProperties.Clear();
>  
> -    nsCOMPtr<nsIContent> content = do_QueryInterface(aRootNode);
> +    nsCOMPtr<Element> content = do_QueryInterface(aRootNode);

Are we guaranteed this QI will succeed?

::: dom/xul/templates/nsXULTemplateQueryProcessorRDF.cpp:1232
(Diff revision 4)
>                                                    TestNode** aResult)
>  {
>      nsresult rv = NS_OK;
>  
>      if (aTag == nsGkAtoms::triple) {
> -        rv = CompileTripleCondition(aQuery, aCondition, aParentNode, aResult);
> +        rv = CompileTripleCondition(aQuery, aCondition->AsElement(), aParentNode, aResult);

Are we guaranteed aCondition is an Element?  Documen why.

::: dom/xul/templates/nsXULTemplateQueryProcessorRDF.cpp:1234
(Diff revision 4)
>      nsresult rv = NS_OK;
>  
>      if (aTag == nsGkAtoms::triple) {
> -        rv = CompileTripleCondition(aQuery, aCondition, aParentNode, aResult);
> -    }
> -    else if (aTag == nsGkAtoms::member) {
> +        rv = CompileTripleCondition(aQuery, aCondition->AsElement(), aParentNode, aResult);
> +    } else if (aTag == nsGkAtoms::member) {
> +        rv = CompileMemberCondition(aQuery, aCondition->AsElement(), aParentNode, aResult);

And here.

::: dom/xul/templates/nsXULTemplateQueryProcessorXML.cpp:234
(Diff revision 4)
>                                               nsAtom* aMemberVariable,
>                                               nsISupports** _retval)
>  {
>      *_retval = nullptr;
>  
> -    nsCOMPtr<nsIContent> content = do_QueryInterface(aQueryNode);
> +    nsCOMPtr<Element> content = do_QueryInterface(aQueryNode);

How do we know this QI succeeds?  Document.

::: layout/base/nsCSSFrameConstructor.cpp:1857
(Diff revision 4)
>        if (aParentContent->IsHTMLElement() &&
>            aParentContent->NodeInfo()->Equals(nsGkAtoms::input)) {

While you're here, how about just: `aParentContent->IsHTMLElement(nsGkAtoms::input)`?

::: layout/forms/nsFileControlFrame.cpp:263
(Diff revision 4)
>  
>  
>    nsCOMPtr<nsIContent> content = mFrame->GetContent();
> -  bool supportsMultiple = content && content->HasAttr(kNameSpaceID_None, nsGkAtoms::multiple);
> +  bool supportsMultiple =
> +    content &&
> +    content->IsElement() &&

So why do you check here, but assume element in nsCheckboxRadioFrame::RegUnRegAccessKey?

::: layout/generic/BlockReflowInput.cpp:843
(Diff revision 4)
>  
>        if(prevFrame) {
>          //get the frame type
>          if (prevFrame->IsTableWrapperFrame()) {
>            //see if it has "align="
> -          // IE makes a difference between align and he float property
> +          // IE makes a difference between align and he float property.

s/he/the/ while you're changing this line?

::: layout/generic/BlockReflowInput.cpp:850
(Diff revision 4)
> +          // We're interested only if previous frame is align=left IE messes
> +          // things up when "right" (overlapping frames).
>            nsIContent* content = prevFrame->GetContent();
> -          if (content) {
> -            // we're interested only if previous frame is align=left
> -            // IE messes things up when "right" (overlapping frames)
> +          if (content &&
> +              content->IsElement() &&
> +              content->AsElement()->AttrValueIs(kNameSpaceID_None,

I wonder whether this thing is still needed.  Maybe file a followup bug to investigate it?

In particular, it's looking at "align" on random table-styled elements, which is a bit fishy.

::: layout/mathml/nsMathMLmactionFrame.cpp:45
(Diff revision 4)
>  static int32_t
>  GetActionType(nsIContent* aContent)
>  {
>    nsAutoString value;
>  
> -  if (aContent) {
> +  if (aContent && aContent->IsElement()) {

This is changing behavior.  We used to return ACTION_TYPE_NONE for non-element content.  Now we will return ACTION_TYPE_UNKNOWN.

Probably better to push the IsElement() check into the inner "if" here instead and preserve behavior.

::: layout/xul/nsBoxFrame.cpp:307
(Diff revision 4)
>  nsBoxFrame::GetInitialHAlignment(nsBoxFrame::Halignment& aHalign)
>  {
> -  if (!GetContent())
> +  if (!GetContent() || !GetContent()->IsElement())
>      return false;
>  
> +  Element* element = mContent->AsElement();

GetContent() like the surrounding code?

::: layout/xul/nsLeafBoxFrame.cpp:86
(Diff revision 4)
>    return rv;
>  }
>  
>  void nsLeafBoxFrame::UpdateMouseThrough()
>  {
> -  if (mContent) {
> +  if (mContent && mContent->IsElement()) {

I really wish I understood when you're checking (as here) and when you're assuming an element (like in lots of the other frame classes)....

::: layout/xul/nsMenuPopupFrame.cpp:300
(Diff revision 4)
>      mMouseTransparent = GetStateBits() & NS_FRAME_MOUSE_THROUGH_ALWAYS;
>      widgetData.mMouseTransparent = mMouseTransparent;
>    }
>  
>    nsAutoString title;
> -  if (mContent && widgetData.mNoAutoHide) {
> +  if (mContent && mContent->IsElement() && widgetData.mNoAutoHide) {

OK, so above in the !mInContentShell case you assumed IsElement(), but here you're checking it?  Please just be consistent...

::: layout/xul/nsScrollbarButtonFrame.cpp:118
(Diff revision 4)
> -  static nsIContent::AttrValuesArray strings[] = { &nsGkAtoms::increment,
> +  static Element::AttrValuesArray strings[] = { &nsGkAtoms::increment,
>                                                     &nsGkAtoms::decrement,
>                                                     nullptr };

Fix the indent, please.

::: layout/xul/nsTextBoxFrame.cpp:241
(Diff revision 4)
>      if (aAttribute == nullptr || aAttribute == nsGkAtoms::crop) {
> -        static nsIContent::AttrValuesArray strings[] =
> +        static Element::AttrValuesArray strings[] =
>            {&nsGkAtoms::left, &nsGkAtoms::start, &nsGkAtoms::center,
>             &nsGkAtoms::right, &nsGkAtoms::end, &nsGkAtoms::none, nullptr};
>          CroppingStyle cropType;
> -        switch (mContent->FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::crop,
> +        switch (mContent->AsElement()->FindAttrValueIn(kNameSpaceID_None,

Again, this is assuming IsElement(), while nsTextBoxFrame::UpdateAccesskey checked it...

::: layout/xul/nsTextBoxFrame.cpp:1221
(Diff revision 4)
>  
>      // XXXjag a side-effect is that we filter out anonymous <label>s
>      // in e.g. <menu>, <menuitem>, <button>. These <label>s inherit
>      // |accesskey| and would otherwise register themselves, overwriting
>      // the content we really meant to be registered.
> -    if (!mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::control))
> +    if (!mContent->IsElement() ||

And this checks wile RecomputeTitle() didn't.

::: layout/xul/nsXULLabelFrame.cpp:32
(Diff revision 4)
>  // in nsBoxFrame and nsTextBoxFrame
>  nsresult
>  nsXULLabelFrame::RegUnregAccessKey(bool aDoReg)
>  {
>    // if we have no content, we can't do anything
> -  if (!mContent)
> +  if (!mContent || !mContent->IsElement())

We used to return NS_OK for !IsElement().  Though I expect in practice we can't get here when !IsElement() anyway...

::: layout/xul/nsXULPopupManager.cpp:1550
(Diff revision 4)
> -    else {
>        // Now check if we need to fire the popuppositioned event. If not, call
>        // ShowPopupCallback directly.
>  
>        // The popuppositioned event only fires on arrow panels for now.
> -      if (popup->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
> +      if (popup->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,

Why do we know popup->IsElement()?  Why don't we know it earlier in this method?

::: layout/xul/tree/nsTreeContentView.cpp:199
(Diff revision 4)
>    if (row->IsSeparator())
>      realRow = row->mContent;
>    else
>      realRow = nsTreeUtils::GetImmediateChild(row->mContent, nsGkAtoms::treerow);
>  
> -  if (realRow) {
> +  if (realRow && realRow->IsElement()) {

Else should probably truncate aProperties....

::: widget/cocoa/nsMenuItemX.mm:111
(Diff revision 4)
>      NSString *newCocoaLabelString = nsMenuUtilsX::GetTruncatedCocoaLabel(aLabel);
>      mNativeMenuItem = [[NSMenuItem alloc] initWithTitle:newCocoaLabelString action:nil keyEquivalent:@""];
>  
>      [mNativeMenuItem setEnabled:(BOOL)isEnabled];
>  
> -    SetChecked(mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::checked,
> +    SetChecked(mContent->Iselement() &&

IsElement()

::: widget/gtk/nsNativeThemeGTK.cpp:362
(Diff revision 4)
>  
>          if (aWidgetType == NS_THEME_CHECKMENUITEM ||
>              aWidgetType == NS_THEME_RADIOMENUITEM) {
>            *aWidgetFlags = 0;
>            if (aFrame && aFrame->GetContent()) {
> -            *aWidgetFlags = aFrame->GetContent()->
> +            *aWidgetFlags = aFrame->GetContent()->AsElement()->

How do you know aFrame->GetContent()->IsElement()?

::: widget/nsNativeTheme.cpp:161
(Diff revision 4)
>    if (!aFrame)
>      return defaultValue;
>  
> +  nsIContent* content = aFrame->GetContent();
> +  if (!content || !content->IsElement())
> +    return false;

This doesn't make sense.  You mean "return defaultValue"?

::: widget/nsNativeTheme.cpp:423
(Diff revision 4)
>       &nsGkAtoms::scrollbarUpBottom, &nsGkAtoms::scrollbarUpTop,
>       nullptr};
>  
> -  switch (aFrame->GetContent()->FindAttrValueIn(kNameSpaceID_None,
> +  nsIContent* content = aFrame->GetContent();
> +  if (!content || !content->IsElement()) {
> +    return false;

return 0, right?

::: widget/nsNativeTheme.cpp:499
(Diff revision 4)
>  {
>    if (!aFrame)
>      return false;
>  
>    nsAutoString classStr;
> -  aFrame->GetContent()->GetAttr(kNameSpaceID_None, nsGkAtoms::_class, classStr);
> +  if (aFrame->GetContent()->IsElement()) {

Not very consistent about whether you null-check GetContent() on frames.  In practice, I suspect that the null-checks like that which this patch adds are not needed.

::: widget/nsNativeTheme.cpp:504
(Diff revision 4)
> -  aFrame->GetContent()->GetAttr(kNameSpaceID_None, nsGkAtoms::_class, classStr);
> +  if (aFrame->GetContent()->IsElement()) {
> +    aFrame->GetContent()->AsElement()->GetAttr(kNameSpaceID_None,
> +                                               nsGkAtoms::_class,
> +                                               classStr);
> +  }
> +  // FIXME: This looks bogus, shouldn't this be looking at GetClasses()?

Yes, looks pretty bogus to me.

::: widget/nsNativeTheme.cpp:525
(Diff revision 4)
>  }
>  
>  bool
>  nsNativeTheme::IsHorizontal(nsIFrame* aFrame)
>  {
> -  if (!aFrame)
> +  if (!aFrame || !aFrame->GetContent()->IsElement())

We used to return true in the !IsElement() case here.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review214432

::: accessible/base/ARIAMap.cpp:1378
(Diff revision 4)
>  bool
>  aria::HasDefinedARIAHidden(nsIContent* aContent)
>  {
>    return aContent &&
>      nsAccUtils::HasDefinedARIAToken(aContent, nsGkAtoms::aria_hidden) &&
> -    !aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::aria_hidden,
> +    (!aContent->IsElement() ||

Yeah, I changed HasDefinedAriaToken after these.

::: dom/base/nsContentList.cpp:563
(Diff revision 4)
>      // XXX Should this pass eIgnoreCase?
>      if (content &&
>          ((content->IsHTMLElement() &&
> -          content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
> +          content->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
> -                               name, eCaseMatters)) ||
> +                                            name, eCaseMatters)) ||
> -         content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::id,
> +         content->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::id,

There's a `content->IsHTMLElement()` check right before this condition.

::: dom/base/nsContentSink.cpp:800
(Diff revision 4)
>                 aElement->NodeType() == nsIDOMNode::PROCESSING_INSTRUCTION_NODE,
>                 "We only expect processing instructions here");
>  
>    nsAutoString integrity;
>    if (aElement) {
> -    aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);
> +    aElement->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity, integrity);

Huh, it definitely passes try, maybe it's dead code?

::: dom/base/nsTextNode.cpp:245
(Diff revision 4)
>    nsresult rv = nsTextNode::BindToTree(aDocument, aParent,
>                                         aBindingParent, aCompileEventHandlers);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    NS_ASSERTION(!mGrandparent, "We were already bound!");
> -  mGrandparent = aParent->GetParent();
> +  mGrandparent = aParent->GetParent()->AsElement();

This is an `nsAttrTextNode`, which we create for pseudo-elements, so the parent is a ::before / ::after pseudo-element, and the parent of that needs to be an element.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review214432

> There's a `content->IsHTMLElement()` check right before this condition.

Err, nvm, I don't know how to read, will fix.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review214432

> Huh, it definitely passes try, maybe it's dead code?

You're right, it is.  nsContentSink::ProcessStyleLink is only called with a non-null aElement from nsXMLContentSink::ProcessStyleLink, which is only called from nsXMLContentSink::HandleProcessingInstruction, which early-returns for the CSS cases.

I filed bug 1426432 on this.

> This is an `nsAttrTextNode`, which we create for pseudo-elements, so the parent is a ::before / ::after pseudo-element, and the parent of that needs to be an element.

Ah, I missed that this is for the attr text node.  Good, good.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review214432

> You're right, it is.  nsContentSink::ProcessStyleLink is only called with a non-null aElement from nsXMLContentSink::ProcessStyleLink, which is only called from nsXMLContentSink::HandleProcessingInstruction, which early-returns for the CSS cases.
> 
> I filed bug 1426432 on this.

Actually, I do have a testcase that fails this (but is not in our test suite):

    <?xml-stylesheet type="text/css;foo" href="data:text/css,* { color: green }"?>
    <html xmlns="http://www.w3.org/1999/xhtml">text</html>

I am going to fix that up in bug 1426432.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review213548

> I don't think `content` is generally guaranteed to be an Element here...  Need to check it.

It looks to me that nsContentList only matches elements from reading the code, something I'm missing?

> Are we guaranteed aCondition is an Element?  Documen why.

Well, it's testing its name atom, and I don't think we can create a `triple` named node without it being an element. But not worth the hassle since this code probably goes away soon, so I just checked `IsElement()` too.

> How do we know this QI succeeds?  Document.

Well, we don't, same way about knowing whether the `nsIContent` QI succeeds... I guess we just trust this API. But I think this is part of the code that's going away soon, so I'll keep nsIContent + IsElement I guess.

> I wonder whether this thing is still needed.  Maybe file a followup bug to investigate it?
> 
> In particular, it's looking at "align" on random table-styled elements, which is a bit fishy.

Filed bug 1426747.

> This is changing behavior.  We used to return ACTION_TYPE_NONE for non-element content.  Now we will return ACTION_TYPE_UNKNOWN.
> 
> Probably better to push the IsElement() check into the inner "if" here instead and preserve behavior.

I'm pretty sure this is always a `<mathml:maction>` element, but yeah, not the purpose of this patch.

> I really wish I understood when you're checking (as here) and when you're assuming an element (like in lots of the other frame classes)....

Yeah, this was done kinda conservatively when stomping with zillion compiler errors, but there's no reason this should check...

> Again, this is assuming IsElement(), while nsTextBoxFrame::UpdateAccesskey checked it...

Yeah, this was called from AttributeChanged, but yeah, will fix consistency in various places.

> How do you know aFrame->GetContent()->IsElement()?

This is one of the checkbox / radio widget items, so they should be elements. Anyway doesn't hurt to check, wish all this code asserted a bit more stuff.

> Not very consistent about whether you null-check GetContent() on frames.  In practice, I suspect that the null-checks like that which this patch adds are not needed.

Yeah, I mostly kept the existing null-checks, which weren't very consistent already. I'll remove the ones added.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review213548

> It looks to me that nsContentList only matches elements from reading the code, something I'm missing?

Hmm.  So I think you're right for nsContentList.  OK, this is fine, with a comment.  (Confusingly, mElements lives on a base class, and there it's _not_ guaranteed to contain elements.)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206348/diff/4-5/

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935454 [details]
Bug 1423990: Move the last few attribute-related methods outside of nsIContent.

https://reviewboard.mozilla.org/r/206348/#review215076

r=me base on some spot-checking and skimming the interdiff, but there's so much noise in the mozreview interdiffs that I can't guarantee catching everything...  All the things I did check look good, though.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e43f568b3e9a
Move the last few attribute-related methods outside of nsIContent. r=bz

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8b0e9c18f2f
Backout changeset e43f568b3e9a because some OSX-only code still doesn't build. r=me

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fceda645f5e3
Move the last few attribute-related methods outside of nsIContent. r=bz

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/fceda645f5e3

Comment 19

[Jorg K (CEST = GMT+2)]

I'm seeing
dom/base/nsDocumentEncoder.cpp(1412): error C2039: 'GetAttr': is not a member of 'nsIContent'
on a comm-central compile:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=0e55b426edf3b577214e746e146df3ead5335f9e&selectedJob=153362556

That's inside |#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)|.

Could you kindly fix this as well?

Comment 20

[Jorg K (CEST = GMT+2)]

Richard, if you read this, please try:

if (selContent->GetAttr(kNameSpaceID_None, nsGkAtoms::style, styleVal) &&
    styleVal.Find(NS_LITERAL_STRING("pre-wrap")) != kNotFound) {

replaced by

if (selContent->IsElement() &&
    selContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::style, styleVal) &&
    styleVal.Find(NS_LITERAL_STRING("pre-wrap")) != kNotFound) {

Maybe attach a patch and ask for review unless Emilio gets here first.

Comment 21

[Richard Marti (:Paenglab)]

I'm trying it...

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1423990-follow-up.patch

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks, if it works, please request review by Emilio and :bz, see who is quicker.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1423990-follow-up.patch

(fixed typo)

Comment 25

[Richard Marti (:Paenglab)]

Comment on attachment 8938864 [details] [diff] [review]
1423990-follow-up.patch

Worked

Comment 26

[Richard Marti (:Paenglab)]

Thanks Olli!

Sheriff, please can you change the reviewer to smaug when you check-in the patch?

Comment 27

[Jorg K (CEST = GMT+2)]

And please land this directly onto M-C since it's a bustage fix for Thunderbird. The code isn't compiled for Firefox.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ee9a4618a978
Follow-up for conditionally compiled code for Thunderbird. r=smaug