Last Comment Bug 767130 - Create nsINode.cpp
: Create nsINode.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 13:48 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-06-23 01:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (215.88 KB, patch)
2012-06-21 13:48 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
Details | Diff | Splinter Review
nsINode.cpp (62.70 KB, text/plain)
2012-06-22 01:13 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details
Turn shared static functions into member functions (11.08 KB, patch)
2012-06-22 04:41 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
bzbarsky: review-
Details | Diff | Splinter Review
With more nsContentUtils (11.03 KB, patch)
2012-06-22 09:35 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-06-21 13:48:00 PDT
Created attachment 635443 [details] [diff] [review]
Patch v1

nsGenericElement.cpp is too damn big. This patch splits the implementation of nsINode::* into a file of it own, cutting a 6676-line file into one of 4738 and one of 2077.

Would appreciate a quick review, because this is going to bitrot fast...
Comment 1 Mounir Lamouri (:mounir) 2012-06-21 14:24:49 PDT
Comment on attachment 635443 [details] [diff] [review]
Patch v1

Review of attachment 635443 [details] [diff] [review]:
-----------------------------------------------------------------

For the ease of the review, could you attach the nsINode.cpp file?
Re-ask for the review when done.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-06-22 01:13:58 PDT
Created attachment 635660 [details]
nsINode.cpp
Comment 3 Mounir Lamouri (:mounir) 2012-06-22 03:50:09 PDT
Comment on attachment 635443 [details] [diff] [review]
Patch v1

Review of attachment 635443 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like those methods are duplicated in nsINode.cpp and nsGenericElement.cpp:
static nsIEditor* GetHTMLEditor(nsPresContext* aPresContext)
static bool UnoptimizableCCNode(nsINode* aNode)

Please, make GetHTMLEditor a static nsINode methods so you can keep it in nsINode.cpp and still use it in nsGenericElement.cpp.
For UnoptimizableCCNode, I think you can make it a member of nsINode given that it is actually taking a nsINode argument.

I'm assuming you haven't done any change except copy-pasting the code. Let me know if this is not the case.

Regarding the includes, I can't really check all of them but it seems that you don't need the ones inside #ifdef MOZ_XUL and #ifdef DEBUG.

For the rest of the code, I'm assuming you haven't done any change except copy-pasting. Let me know if this is not the case.

r=me based on that and with the requested changes applied.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-06-22 04:41:24 PDT
Created attachment 635693 [details] [diff] [review]
Turn shared static functions into member functions
Comment 5 Mounir Lamouri (:mounir) 2012-06-22 04:51:51 PDT
Comment on attachment 635693 [details] [diff] [review]
Turn shared static functions into member functions

Review of attachment 635693 [details] [diff] [review]:
-----------------------------------------------------------------

Hmmm, I'm not a big fan of adding that method to nsPresShell so I would be more comfortable if we could get a layout peer's opinion.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-06-22 09:15:47 PDT
Comment on attachment 635693 [details] [diff] [review]
Turn shared static functions into member functions

Yeah, I'm not a huge fan of it either: it's basically an abuse that uses the prescontext just for its container... which you can get perfectly well off an document too.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-06-22 09:35:27 PDT
Created attachment 635786 [details] [diff] [review]
With more nsContentUtils
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-06-22 09:41:39 PDT
Comment on attachment 635786 [details] [diff] [review]
With more nsContentUtils

r=me

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