1423167 - Move attribute-related methods to Element so they're not virtual

Assignee

Description

•

7 years ago

Followup to bug 1422529

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Assignee: nobody → emilio

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 1

•

7 years ago

Really sorry for the size of the patch here...

Comment hidden (mozreview-request)

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

Code Review Bot [:reviewbot]

Comment 3

•

7 years ago

mozreview-review

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211120


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/xul/XULDocument.cpp:2155
(Diff revision 1)
> -            nsCOMPtr<nsIContent> element = aElements.SafeObjectAt(i);
> +            nsCOMPtr<Element> element = aElements.SafeObjectAt(i);
>              if (!element) {
>                   continue;
>              }
>  
> -            rv = element->SetAttr(kNameSpaceID_None, attr, value, PR_TRUE);
> +            rv = element->SetAttr(kNameSpaceID_None, attr, value, true);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

::: dom/xul/templates/nsXULContentBuilder.cpp:713
(Diff revision 1)
>  
>                  if (isGenerationElement)
> -                    rv = InsertSortedNode(aRealNode, realKid, aChild, aNotify);
> +                    rv = InsertSortedNode(aRealElement, realKid, aChild, aNotify);
>  
>                  if (NS_FAILED(rv)) {
> -                    rv = aRealNode->AppendChildTo(realKid, aNotify);
> +                    rv = aRealElement->AppendChildTo(realKid, aNotify);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment hidden (mozreview-request)

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

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

Code Review Bot [:reviewbot]

Comment 5

•

7 years ago

mozreview-review

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211204


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/xul/templates/nsXULContentBuilder.cpp:713
(Diff revision 2)
>  
>                  if (isGenerationElement)
> -                    rv = InsertSortedNode(aRealNode, realKid, aChild, aNotify);
> +                    rv = InsertSortedNode(aRealElement, realKid, aChild, aNotify);
>  
>                  if (NS_FAILED(rv)) {
> -                    rv = aRealNode->AppendChildTo(realKid, aNotify);
> +                    rv = aRealElement->AppendChildTo(realKid, aNotify);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Boris Zbarsky [:bzbarsky]

Comment 6

•

7 years ago

mozreview-review

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211168

r=me with the various minor issues fixed and the FindChildByTag bit sorted out.  This is great!

::: accessible/xul/XULSliderAccessible.h:46
(Diff revision 1)
>  
>  protected:
>    /**
>     * Return anonymous slider element.
>     */
> -  nsIContent* GetSliderElement() const;
> +  mozilla::dom::Element* GetSliderElement() const;

You're inside "namespace mozilla" here, no?  So just dom::Element.

::: accessible/xul/XULSliderAccessible.h:55
(Diff revision 1)
>  
>    double GetSliderAttr(nsAtom *aName) const;
>    bool SetSliderAttr(nsAtom *aName, double aValue);
>  
>  private:
> -  mutable nsCOMPtr<nsIContent> mSliderNode;
> +  mutable RefPtr<mozilla::dom::Element> mSliderNode;

mSliderElement, maybe?

::: accessible/xul/XULSliderAccessible.h:55
(Diff revision 1)
>  
>    double GetSliderAttr(nsAtom *aName) const;
>    bool SetSliderAttr(nsAtom *aName, double aValue);
>  
>  private:
> -  mutable nsCOMPtr<nsIContent> mSliderNode;
> +  mutable RefPtr<mozilla::dom::Element> mSliderNode;

And here.

::: accessible/xul/XULSliderAccessible.cpp:43
(Diff revision 1)
>  XULSliderAccessible::NativeInteractiveState() const
>   {
>    if (NativelyUnavailable())
>      return states::UNAVAILABLE;
>  
> -  nsIContent* sliderElm = GetSliderElement();
> +  Element* sliderElm = GetSliderElement();

dom::Element, I'd think.

::: accessible/xul/XULSliderAccessible.cpp:86
(Diff revision 1)
>  XULSliderAccessible::DoAction(uint8_t aIndex)
>  {
>    if (aIndex != 0)
>      return false;
>  
> -  nsIContent* sliderElm = GetSliderElement();
> +  Element* sliderElm = GetSliderElement();

dom::Element

::: accessible/xul/XULSliderAccessible.cpp:166
(Diff revision 1)
>  XULSliderAccessible::SetSliderAttr(nsAtom* aName, const nsAString& aValue)
>  {
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> -  nsIContent* sliderElm = GetSliderElement();
> +  if (Element* sliderElm = GetSliderElement())

dom::Element

::: dom/base/Element.h:834
(Diff revision 1)
> +   *        caller with the given principal is directly responsible for the
> +   *        attribute change.
> +   * @param aNotify specifies how whether or not the document should be
> +   *        notified of the attribute change.
> +   */
> +  virtual nsresult SetAttr(int32_t aNameSpaceID, nsAtom* aName,

So...  The only overrides of this that are subclasses of Element are SVGAElement::SetAttr and SVGStyleElement::SetAttr.  It looks like those got skipped in bug 1365092 because of some unrelated SVG problems.  Do you mind filing a followup to move the work there into AfterSetAttr and devirtualize this function?

::: dom/base/Element.h:847
(Diff revision 1)
> +   * @param aNameSpaceID the namespace id of the attribute
> +   * @param aAttr the name of the attribute to unset
> +   * @param aNotify specifies whether or not the document should be
> +   * notified of the attribute change
> +   */
> +  virtual nsresult UnsetAttr(int32_t aNameSpaceID,

There are a few more overrides of UnsetAttr, all SVG, but again they should probably be able to transition to Before/AfterSett and then we can devirtualize here.  A separate bug on doing that, please?  nsSVGElement::UnsetAttr might get complicated...

::: dom/base/FragmentOrElement.cpp:405
(Diff revision 1)
>  
>  nsAtom*
>  nsIContent::GetLang() const
>  {
>    for (const auto* content = this; content; content = content->GetParent()) {
> -    if (!content->GetAttrCount() || !content->IsElement()) {
> +    if (!content->IsElement()) {

You want to keep the fast-path continue if GetAttrCount() is 0, no?  Just do it after the IsElement() check.

::: dom/base/FragmentOrElement.cpp:2239
(Diff revision 1)
>    for (i = 0; i < count; ++i) {
>      const nsAttrName* name = mAttrsAndChildren.AttrNameAt(i);
>      const nsAttrValue* value = mAttrsAndChildren.AttrAt(i);
>      nsAutoString valStr;
>      value->ToString(valStr);
> -    rv = aDst->SetAttr(name->NamespaceID(), name->LocalName(),
> +    if (aDst->IsElement()) {

You should be able to assert this.  CopyInnerTo is passed an object of the same concrete class as "this", which is known to be an element in this case, because mAttrsAndChildren.AttrCount() was nonzero.

We can also move this entire method from FragmentOrElement to Element and give DocumentFragment a CopyInnerTo that just does the `aDst->mAttrsAndChildren.EnsureCapacityToClone(mAttrsAndChildren, aPreallocateChildren)` bit.  Followup would be fine for that.  And once bug 651120 is fixed, we could even remove that EnsureCapacityToClone call...

::: dom/svg/SVGAElement.cpp:120
(Diff revision 1)
>  }
>  
>  void
>  SVGAElement::SetDownload(const nsAString & aDownload, ErrorResult& rv)
>  {
> -  rv = SetAttr(kNameSpaceID_None, nsGkAtoms::download, aDownload, true);
> +  rv = Element::SetAttr(kNameSpaceID_None, nsGkAtoms::download, aDownload, true);

I'd prefer if we left the "using Element::SetAttr" in the header and then didn't need to unnecessarily qualify here.

And then we can remove the "using" when we remove the SetAttr override in general here.

::: dom/svg/SVGStyleElement.cpp:206
(Diff revision 1)
>  }
>  
>  void
>  SVGStyleElement::SetXmlspace(const nsAString & aXmlspace, ErrorResult& rv)
>  {
> -  rv = SetAttr(kNameSpaceID_XML, nsGkAtoms::space, aXmlspace, true);
> +  rv = Element::SetAttr(kNameSpaceID_XML, nsGkAtoms::space, aXmlspace, true);

Again, leave "using Element::SetAttr" in the header.

::: dom/svg/SVGStyleElement.cpp:218
(Diff revision 1)
>  }
>  
>  void
>  SVGStyleElement::SetMedia(const nsAString& aMedia, ErrorResult& rv)
>  {
> -  rv = SetAttr(kNameSpaceID_None, nsGkAtoms::media, aMedia, true);
> +  rv = Element::SetAttr(kNameSpaceID_None, nsGkAtoms::media, aMedia, true);

And here.

::: dom/svg/SVGStyleElement.cpp:242
(Diff revision 1)
>  }
>  
>  void
>  SVGStyleElement::SetType(const nsAString& aType, ErrorResult& rv)
>  {
> -  rv = SetAttr(kNameSpaceID_None, nsGkAtoms::type, aType, true);
> +  rv = Element::SetAttr(kNameSpaceID_None, nsGkAtoms::type, aType, true);

And here.

::: dom/xbl/nsXBLPrototypeBinding.cpp:488
(Diff revision 1)
>    if (!copyParent)
>      return nullptr;
>  
> -  return copyParent->GetChildAt(templParent->IndexOf(aTemplChild));
> +  nsIContent* child = copyParent->GetChildAt(templParent->IndexOf(aTemplChild));
> +  if (child) {
> +    MOZ_RELEASE_ASSERT(child->IsElement());

This may not hold, due to the XXX comment at the start of this method...

What you could probably do is just return null if !child->IsElement().

::: dom/xbl/nsXBLPrototypeBinding.cpp:1375
(Diff revision 1)
> -      content->AppendChildTo(child, false);
> +      element->AppendChildTo(child, false);
>      }
>    }
>  
> +  nsCOMPtr<nsIContent> content = element.forget();
>    content.swap(*aContent);

We should figure out why *aContent might not be null here.... and if it can't, we can |element.forget(aContent)| I'd think.  Followup is fine here.

::: dom/xul/XULDocument.cpp:3855
(Diff revision 1)
>          // We don't want to swap IDs, they should be the same.
>          if (name->Equals(nsGkAtoms::id))
>              continue;
>  
>          // In certain cases merging command or observes is unsafe, so don't.
> -        if (!aNotify) {
> +        if (!aNotify && aTargetElement->IsElement()) {

No need for the IsElement()/AsElement() dance here; aTargetElement is an Element already.

::: dom/xul/XULDocument.cpp:3891
(Diff revision 1)
>              if (NS_FAILED(rv)) return rv;
> -
>              return NS_OK;
>          }
>  
> -        rv = aTargetNode->SetAttr(nameSpaceID, attr, prefix, value, aNotify);
> +        if (aTargetElement->IsElement()) {

No need for this if.

::: dom/xul/XULDocument.cpp:3892
(Diff revision 1)
> -
>              return NS_OK;
>          }
>  
> -        rv = aTargetNode->SetAttr(nameSpaceID, attr, prefix, value, aNotify);
> -        if (!NS_FAILED(rv) && !aNotify)
> +        if (aTargetElement->IsElement()) {
> +            rv = aTargetElement->AsElement()->SetAttr(nameSpaceID, attr, prefix, value, aNotify);

Or the IsElement() here.

::: dom/xul/XULDocument.cpp:3950
(Diff revision 1)
> -            if (parentID &&
> +            if (parentID && aTargetElement->GetID() == parentID) {
> -                aTargetNode->AttrValueIs(kNameSpaceID_None, nsGkAtoms::id,
> -                                         nsDependentAtomString(parentID),
> -                                         eCaseMatters)) {
>                  // The element matches. "Go Deep!"
> -                rv = Merge(elementInDocument, currContent, aNotify);
> +                rv = Merge(elementInDocument, currContent->AsElement(), aNotify);

Might be worth documenting that currContent must be an element, because elementInDocument is only non-null when currContent is an element whose id matches that of elementInDocument?

::: dom/xul/templates/nsXULContentBuilder.cpp:668
(Diff revision 1)
>              // template to incrementally build content.
>              mTemplateMap.Put(realKid, tmplKid);
>  
> -            rv = CopyAttributesToElement(tmplKid, realKid, aChild, false);
> +            if (tmplKid->IsElement()) {
> +                rv = CopyAttributesToElement(tmplKid->AsElement(),
> +                                             realKid->AsElement(), aChild,

realKid is already an Element; no need for this AsElement().

Furthermore, I suspect we can prove by examination that any time realKid is non-null (which we are here), tmplKid is in fact an element, so I bet we can remove the tmplKid->IsElement() check and add a comment.  But please verify that.

::: dom/xul/templates/nsXULContentBuilder.cpp:676
(Diff revision 1)
> +            }
>  
>              // Add any persistent attributes
>              if (isGenerationElement) {
>                  rv = AddPersistentAttributes(tmplKid->AsElement(), aChild,
> -                                             realKid);
> +                                             realKid->AsElement());

realKid is an Element already.

::: dom/xul/templates/nsXULContentBuilder.cpp:1591
(Diff revision 1)
>                                    nsTemplateRule* aNewMatchRule,
>                                    void *aContext)
>  
>  {
>      nsresult rv;
> -    nsIContent* content = static_cast<nsIContent*>(aContext);
> +    auto* element = static_cast<Element*>(aContext);

How about we just make aContext be Element*?  Why isn't it?

::: dom/xul/templates/nsXULContentUtils.cpp:136
(Diff revision 1)
>      for (nsIContent* child = aElement->GetFirstChild();
>           child;
>           child = child->GetNextSibling()) {
>  
>          if (child->NodeInfo()->Equals(aTag, aNameSpaceID)) {
> -            NS_ADDREF(*aResult = child);
> +            NS_ADDREF(*aResult = child->AsElement());

How do we know the child is an element?  It's not obvious to me that this is guaranteed when we land here from nsXULContentBuilder::EnsureElementHasGenericChild.

::: dom/xul/templates/nsXULTemplateBuilder.cpp:1917
(Diff revision 1)
>                  }
>              }
>  
>              hasQuerySet = true;
>  
> -            rv = CompileTemplate(rulenode, aQuerySet, true, aPriority, aCanUseTemplate);
> +            rv = CompileTemplate(rulenode->AsElement(), aQuerySet, true,

Document why rulenode is known to be an Element here (because its namespace is XUL)?

::: dom/xul/templates/nsXULTemplateBuilder.cpp:1962
(Diff revision 1)
>                          if (NS_FAILED(rv))
>                              return rv;
>                      }
>  
>                      if (aQuerySet->mCompiledQuery) {
> -                        rv = CompileExtendedQuery(rulenode, action, memberVariable,
> +                        rv = CompileExtendedQuery(rulenode->AsElement(),

Document why this is known to be an Element.

::: dom/xul/templates/nsXULTemplateBuilder.cpp:2010
(Diff revision 1)
>                                                             getter_AddRefs(aQuerySet->mCompiledQuery));
>                          if (NS_FAILED(rv))
>                              return rv;
>  
>                          if (aQuerySet->mCompiledQuery) {
> -                            rv = CompileExtendedQuery(rulenode, action, memberVariable,
> +                            rv = CompileExtendedQuery(rulenode->AsElement(),

Document why we know it's an Element.

::: dom/xul/templates/nsXULTemplateBuilder.cpp:2036
(Diff revision 1)
>                      }
>                  }
>  
>                  hasQuerySet = true;
>  
> -                rv = CompileSimpleQuery(rulenode, aQuerySet, aCanUseTemplate);
> +                rv = CompileSimpleQuery(rulenode->AsElement(), aQuerySet,

Document why known Element.

::: layout/forms/nsComboboxControlFrame.h:287
(Diff revision 1)
>    nsPoint GetCSSTransformTranslation();
>  
>  protected:
>    nsFrameList              mPopupFrames;             // additional named child list
>    nsCOMPtr<nsIContent>     mDisplayContent;          // Anonymous content used to display the current selection
> -  nsCOMPtr<nsIContent>     mButtonContent;           // Anonymous content for the button
> +  RefPtr<Element>     mButtonContent;                // Anonymous content for the button

Please fix the indent (line up the mButtonContent bit  with the other member names).

::: layout/forms/nsFileControlFrame.cpp:152
(Diff revision 1)
>                                                   kNameSpaceID_XUL,
>                                                   nsIDOMNode::ELEMENT_NODE);
> -  NS_TrustedNewXULElement(getter_AddRefs(mTextContent), nodeInfo.forget());
> +  {
> +    nsCOMPtr<nsIContent> content;
> +    NS_TrustedNewXULElement(getter_AddRefs(content), nodeInfo.forget());
> +    mTextContent =

You could probably do:

    mTextContent = content.forget().downcast<Element>()
    
but that would skip the IsElement() assert and isn't much nicer, I guess...

::: layout/generic/nsGfxScrollFrame.cpp:4578
(Diff revision 1)
>    if (canHaveHorizontal) {
>      RefPtr<NodeInfo> ni = nodeInfo;
> -    NS_TrustedNewXULElement(getter_AddRefs(mHScrollbarContent), ni.forget());
> +    {
> +      nsCOMPtr<nsIContent> content;
> +      NS_TrustedNewXULElement(getter_AddRefs(content), ni.forget());
> +      mHScrollbarContent =

OK, this is the second time we're seeing this ugly pattern.

How about we make the outparam of  NS_TrustedNewXULElement an Element**?

Or maybe even just use "new nsXULElement(ni.forget())" directly, since that's all  NS_TrustedNewXULElement does?

::: layout/xul/nsSplitterFrame.cpp:703
(Diff revision 1)
>          nsRect r(childBox->GetRect());
>          r.Inflate(margin);
>  
>          // We need to check for hidden attribute too, since treecols with
>          // the hidden="true" attribute are not really hidden, just collapsed
> -        if (!content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::fixed,
> +        if (content->IsElement() &&

Shouldn't this be !content->IsElement() || and then the AttrValueIs checks?

::: layout/xul/nsXULTooltipListener.cpp:596
(Diff revision 1)
>    if (!tooltipText.IsEmpty()) {
>      // specifying tooltiptext means we will always use the default tooltip
>      nsIRootBox* rootBox = nsIRootBox::GetRootBox(document->GetShell());
>      NS_ENSURE_STATE(rootBox);
> -    *aTooltip = rootBox->GetDefaultTooltip();
> -    if (*aTooltip) {
> +    if (Element* tooltip = rootBox->GetDefaultTooltip()) {
> +      tooltip->SetAttr(kNameSpaceID_None, nsGkAtoms::label, tooltipText, true);

You want to hold a strong ref to "tooltip" across the notifying SetAttr call (like the old ordering, where the addref came before SetAttr).

Probably just make "tooltip" a RefPtr and then .forget() into aTooltip, to make this harder to screw up.

Attachment #8934607 - Flags: review?(bzbarsky) → review+

Comment hidden (mozreview-request)

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

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

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Blocks: 1423490

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Blocks: 1423492

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 8

•

7 years ago

mozreview-review-reply

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211168

> We should figure out why *aContent might not be null here.... and if it can't, we can |element.forget(aContent)| I'd think.  Followup is fine here.

It's set to null at the beginning of the function, so just did that.

> No need for the IsElement()/AsElement() dance here; aTargetElement is an Element already.

Whoops, it didn't use to be :)

> How do we know the child is an element?  It's not obvious to me that this is guaranteed when we land here from nsXULContentBuilder::EnsureElementHasGenericChild.

The EnsureElementHasGenericChild thing is a mess... I don't know how it works at all. In particular, if a text-node ever arrives there, then the thing just doesn't make sense.

 * If there's another text-node, we'll arrive in `FindChildByTag`, return it, and see it already existed, then fine, but...
 * If there's not, we'll create an element with the text-node's tag, which is pretty busted I think.

So I just avoided returning non-elements in `FindChildByTag` explicitly, and handled the non-element case in the caller of `EnsureElementHasGenericChild`.

> OK, this is the second time we're seeing this ugly pattern.
> 
> How about we make the outparam of  NS_TrustedNewXULElement an Element**?
> 
> Or maybe even just use "new nsXULElement(ni.forget())" directly, since that's all  NS_TrustedNewXULElement does?

Yeah, I just did that, much nicer.

> Shouldn't this be !content->IsElement() || and then the AttrValueIs checks?

Yup, pretty much, nice catch :)

Comment hidden (mozreview-request)

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

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

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 10

•

7 years ago

mozreview-review-reply

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211168

> How about we just make aContext be Element*?  Why isn't it?

It wasn't because I saw the `void*` and the classes overriding the method, and by then I had already ran out of mental energy :).

But I just did that.

Pulsebot

Comment 11

•

7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a27e933a27b6
Move most attribute-related methods from nsIContent to Element. r=bz

Code Review Bot [:reviewbot]

Comment 12

•

7 years ago

mozreview-review

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211340


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/xul/templates/nsXULContentBuilder.cpp:720
(Diff revision 3)
>  
>                  if (isGenerationElement)
> -                    rv = InsertSortedNode(aRealNode, realKid, aChild, aNotify);
> +                    rv = InsertSortedNode(aRealElement, realKid, aChild, aNotify);
>  
>                  if (NS_FAILED(rv)) {
> -                    rv = aRealNode->AppendChildTo(realKid, aNotify);
> +                    rv = aRealElement->AppendChildTo(realKid, aNotify);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Code Review Bot [:reviewbot]

Comment 13

•

7 years ago

mozreview-review

Comment on attachment 8934607 [details]
Bug 1423167: Move most attribute-related methods from nsIContent to Element.

https://reviewboard.mozilla.org/r/205486/#review211344


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/xul/templates/nsXULContentBuilder.cpp:720
(Diff revision 4)
>  
>                  if (isGenerationElement)
> -                    rv = InsertSortedNode(aRealNode, realKid, aChild, aNotify);
> +                    rv = InsertSortedNode(aRealElement, realKid, aChild, aNotify);
>  
>                  if (NS_FAILED(rv)) {
> -                    rv = aRealNode->AppendChildTo(realKid, aNotify);
> +                    rv = aRealElement->AppendChildTo(realKid, aNotify);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Pulsebot

Comment 14

•

7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a83339d160f1
Address missed review comment and fix last-minute typo. r=me

Raul Gurzau (:RaulG)

Comment 15

•

7 years ago

Backed out 2 changesets (bug 1423167) for build bustage on OSX and Windows on a CLOSED TREE\

Backout: https://hg.mozilla.org/integration/autoland/rev/70b8084a6d191bfde4a6f52be1bbdebd76105712

Pushes with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a83339d160f144645852d80a4fda04c48f7119bb

Flags: needinfo?(emilio)

Pulsebot

Comment 16

•

7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ab98667390
Move most attribute-related methods from nsIContent to Element. r=bz

Pulsebot

Comment 17

•

7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f75a23d6aa9
Fix osx-only bustage. r=me

Dorel Luca [:dluca]

Comment 18

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/d7ab98667390
https://hg.mozilla.org/mozilla-central/rev/0f75a23d6aa9

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox59: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla59

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Blocks: 1423990

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Flags: needinfo?(emilio)