Closed Bug 763861 Opened 8 years ago Closed 8 years ago

Assign a flag in nsINode to say whether the node is content


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

Not set





(Reporter: ayg, Assigned: jhk)



(Keywords: perf, Whiteboard: [mentor=ehsan][lang=c++])


(1 file, 1 obsolete file)

Keywords: perf
Whiteboard: [mentor=ehsan][lang=c++]
In order to fix this bug, a new value should be added to the nsINode::BooleanFlag enum, called NodeIsContent, and it should be set in nsIContent's constructor (using SetBoolFlag).  Then, nsINode::IsContent should be modified to return the value of that flag using the GetBoolFlag method.  It's also a good idea for IsContent to be moved down in nsINode.h to near the rest of the methods there which call GetBoolFlag.
Attached patch Patch(v1) (obsolete) — Splinter Review
Attachment #633569 - Flags: feedback?(ehsan)
Comment on attachment 633569 [details] [diff] [review]

Review of attachment 633569 [details] [diff] [review]:

Except the mentioned error, the patch seems correct.

Feel free to ask a review after updating the patch.

Keeping the feedback flag to Ehsan in case of he has something to add.

::: content/base/public/nsINode.h
@@ +1277,5 @@
>      NodeMayHaveDOMMutationObserver,
>      // Guard value
> +    BooleanFlagCount,
> +    // Set if node is Content
> +    NodeIsContent

This should be added _before_ |BooleanFlagCount|.
Attachment #633569 - Flags: feedback+
Comment on attachment 633569 [details] [diff] [review]

Review of attachment 633569 [details] [diff] [review]:

Great work!  :-)

Please submit the next version of the patch which addresses these our comments for review!

::: content/base/public/nsIContent.h
@@ +64,5 @@
>      : nsINode(aNodeInfo)
>    {
>      NS_ASSERTION(mNodeInfo,
>                   "No nsINodeInfo passed to nsIContent, PREPARE TO CRASH!!!");
> +    SetNodeIsContent();

Please call SetBoolFlag(NodeIsContent); directly here.  It doesn't make sense for us to have a public SetNodeIsContent method which other people can call as well.
Attachment #633569 - Flags: feedback?(ehsan) → feedback+
Attached patch Patch(v2)Splinter Review
As per discussion on IRC. Moved SetIsNodeContent() to protected.
Attachment #633569 - Attachment is obsolete: true
Attachment #634158 - Flags: review?(ehsan)
Attachment #634158 - Flags: review?(ehsan) → review+
Thanks for your patch, Jignesh!  :-)
Assignee: nobody → jigneshhk1992
Target Milestone: --- → mozilla16
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.