1454233 - Remove nsINode::eDocument.

Status: RESOLVED FIXED

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1454233: Ensure some casts and GetParentElement do get inlined.

GetParentElement was using AsElement before being defined. This is only fine
because AsElement wasn't actually getting inlined in this case.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1454233: Introduce nsINode::{IsDocument, AsDocument}.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1454233: Remove nsINode::eDOCUMENT.

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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/

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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 8

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Yeah, I did it because of that...

In that case, worth documenting so someone doesn't come along and "fix" it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bec96422fe4
Add a comment about why IsDocument() is the way it is. r=me

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/87f13c376f1e
https://hg.mozilla.org/mozilla-central/rev/44d8f7cb36e3
https://hg.mozilla.org/mozilla-central/rev/faf615fcb4c1
https://hg.mozilla.org/mozilla-central/rev/6bec96422fe4