Closed Bug 763861 Opened 8 years ago Closed 8 years ago

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

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: ayg, Assigned: jhk)

References

Details

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

Attachments

(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]
Patch(v1)

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]
Patch(v1)

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!  :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e09d2f6098
Assignee: nobody → jigneshhk1992
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/87e09d2f6098
Status: NEW → RESOLVED
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.