Closed Bug 767130 Opened 8 years ago Closed 8 years ago

Create nsINode.cpp

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch Patch v1Splinter Review
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...
Attachment #635443 - Flags: review?(mounir)
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.
Attachment #635443 - Flags: review?(mounir)
Attached file nsINode.cpp
Attachment #635443 - Flags: review?(mounir)
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.
Attachment #635443 - Flags: review?(mounir) → review+
Attachment #635693 - Flags: review?(mounir)
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.
Attachment #635693 - Flags: review?(mounir)
Attachment #635693 - Flags: review?(bzbarsky)
Attachment #635693 - Flags: review+
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.
Attachment #635693 - Flags: review?(bzbarsky) → review-
Attachment #635693 - Attachment is obsolete: true
Attachment #635786 - Flags: review?(bzbarsky)
Comment on attachment 635786 [details] [diff] [review]
With more nsContentUtils

r=me
Attachment #635786 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/187571843979
https://hg.mozilla.org/mozilla-central/rev/0a14c3b24796
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.