Last Comment Bug 763861 - Assign a flag in nsINode to say whether the node is content
: Assign a flag in nsINode to say whether the node is content
Status: RESOLVED FIXED
[mentor=ehsan][lang=c++]
: perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: Jignesh Kakadiya [:jhk]
:
Mentors:
Depends on:
Blocks: 763283
  Show dependency treegraph
 
Reported: 2012-06-12 02:38 PDT by :Aryeh Gregor (working until September 2)
Modified: 2012-06-19 01:17 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch(v1) (3.22 KB, patch)
2012-06-15 10:06 PDT, Jignesh Kakadiya [:jhk]
ehsan: feedback+
mounir: feedback+
Details | Diff | Splinter Review
Patch(v2) (4.17 KB, patch)
2012-06-18 13:46 PDT, Jignesh Kakadiya [:jhk]
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2012-06-12 02:38:14 PDT
See bug 763283 comment 13 and 15.
Comment 1 :Ehsan Akhgari 2012-06-12 18:08:55 PDT
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.
Comment 2 Jignesh Kakadiya [:jhk] 2012-06-15 10:06:47 PDT
Created attachment 633569 [details] [diff] [review]
Patch(v1)
Comment 3 Mounir Lamouri (:mounir) 2012-06-18 06:00:03 PDT
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|.
Comment 4 :Ehsan Akhgari 2012-06-18 11:09:21 PDT
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.
Comment 5 Jignesh Kakadiya [:jhk] 2012-06-18 13:46:24 PDT
Created attachment 634158 [details] [diff] [review]
Patch(v2)

As per discussion on IRC. Moved SetIsNodeContent() to protected.
Comment 6 :Ehsan Akhgari 2012-06-18 20:48:23 PDT
Thanks for your patch, Jignesh!  :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e09d2f6098
Comment 7 Ed Morley [:emorley] 2012-06-19 01:17:55 PDT
https://hg.mozilla.org/mozilla-central/rev/87e09d2f6098

Note You need to log in before you can comment on or make changes to this bug.