Closed
Bug 767130
Opened 12 years ago
Closed 12 years ago
Create nsINode.cpp
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: Ms2ger, Assigned: Ms2ger)
Details
Attachments
(3 files, 1 obsolete file)
215.88 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
62.70 KB,
text/plain
|
Details | |
11.03 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter 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 1•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #635443 -
Flags: review?(mounir)
Comment 3•12 years ago
|
||
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•12 years ago
|
||
Attachment #635693 -
Flags: review?(mounir)
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
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•12 years ago
|
||
Attachment #635693 -
Attachment is obsolete: true
Attachment #635786 -
Flags: review?(bzbarsky)
Comment 8•12 years ago
|
||
Comment on attachment 635786 [details] [diff] [review] With more nsContentUtils r=me
Attachment #635786 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/187571843979 https://hg.mozilla.org/mozilla-central/rev/0a14c3b24796
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•