767130 - Create nsINode.cpp

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: 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)]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: nsINode.cpp

Comment 3

[Mounir Lamouri (: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.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Turn shared static functions into member functions

Comment 5

[Mounir Lamouri (: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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: With more nsContentUtils

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 635786 [details] [diff] [review]
With more nsContentUtils

r=me

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/187571843979
https://hg.mozilla.org/mozilla-central/rev/0a14c3b24796