The default bug view has changed. See this FAQ.

Create nsINode.cpp

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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...
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)
(Assignee)

Comment 2

5 years ago
Created attachment 635660 [details]
nsINode.cpp
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 4

5 years ago
Created attachment 635693 [details] [diff] [review]
Turn shared static functions into member functions
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-
(Assignee)

Comment 7

5 years ago
Created attachment 635786 [details] [diff] [review]
With more nsContentUtils
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+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/187571843979
https://hg.mozilla.org/mozilla-central/rev/0a14c3b24796
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.