Closed Bug 1454233 Opened 6 years ago Closed 6 years ago

Remove nsINode::eDocument.

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(3 files)

      No description provided.
Comment on attachment 8968038 [details]
Bug 1454233: Ensure some casts and GetParentElement do get inlined.

https://reviewboard.mozilla.org/r/236722/#review243826

::: commit-message-c36de:1
(Diff revision 1)
> +Bug 1454233: Ensure some casts and GetParentElement do get inlined. r?bz

s/do get/get/
Attachment #8968038 - Flags: review?(bzbarsky) → review+
Comment on attachment 8968039 [details]
Bug 1454233: Introduce nsINode::{IsDocument, AsDocument}.

https://reviewboard.mozilla.org/r/236724/#review243834

::: dom/base/nsINode.h:463
(Diff revision 1)
> +  /**
> +   * Returns true if this is a document node.
> +   */
> +  bool IsDocument() const
> +  {
> +    return !GetParentNode() && IsInUncomposedDoc();

Why is this better than:

  return NodeType() == DOCUMENT_NODE;
  
?  I guess it's a bit less of a pointer-chase (reads two members of "this" instead of having to go to the nodeinfo.  But it's a lot more confusing to read...
Attachment #8968039 - Flags: review?(bzbarsky) → review+
Comment on attachment 8968040 [details]
Bug 1454233: Remove nsINode::eDOCUMENT.

https://reviewboard.mozilla.org/r/236726/#review243850

::: dom/base/nsINode.cpp:1342
(Diff revision 1)
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (aIndex == 0) {
>      mFirstChild = aKid;
>    }
>  
> -  nsIContent* parent =
> +  nsIContent* parent = IsDocument() ? nullptr : AsContent();

This might be better to reverse to:

    nsIContent* parent = IsContent() ? AsContent() : nullptr;
    
perhaps; a bit faster, because it just checks the one bit.
Attachment #8968040 - Flags: review?(bzbarsky) → review+
Comment on attachment 8968039 [details]
Bug 1454233: Introduce nsINode::{IsDocument, AsDocument}.

https://reviewboard.mozilla.org/r/236724/#review243834

> Why is this better than:
> 
>   return NodeType() == DOCUMENT_NODE;
>   
> ?  I guess it's a bit less of a pointer-chase (reads two members of "this" instead of having to go to the nodeinfo.  But it's a lot more confusing to read...

Yeah, I did it because of that... I'll keep it for now, but if you feel strongly please comment and I can change it.
Comment on attachment 8968040 [details]
Bug 1454233: Remove nsINode::eDOCUMENT.

https://reviewboard.mozilla.org/r/236726/#review243850

> This might be better to reverse to:
> 
>     nsIContent* parent = IsContent() ? AsContent() : nullptr;
>     
> perhaps; a bit faster, because it just checks the one bit.

Agreed. Also a bit nicer to read imo.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f13c376f1e
Ensure some casts and GetParentElement get inlined. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d8f7cb36e3
Introduce nsINode::{IsDocument, AsDocument}. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/faf615fcb4c1
Remove nsINode::eDOCUMENT. r=bz
> Yeah, I did it because of that...

In that case, worth documenting so someone doesn't come along and "fix" it.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bec96422fe4
Add a comment about why IsDocument() is the way it is. r=me
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.