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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: jhk)

Tracking

({perf})

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

See bug 763283 comment 13 and 15.
Blocks: 763283
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.
(Assignee)

Comment 2

5 years ago
Created attachment 633569 [details] [diff] [review]
Patch(v1)
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+
(Assignee)

Comment 5

5 years ago
Created attachment 634158 [details] [diff] [review]
Patch(v2)

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.