Closed
Bug 229575
Opened 21 years ago
Closed 21 years ago
Move some more code into nsContentSink
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
People
(Reporter: keeda, Assigned: keeda)
Details
Attachments
(1 file, 1 obsolete file)
33.63 KB,
patch
|
Details | Diff | Splinter Review |
There is a little bit of stuff that is pretty much common in html and xml and can easily be moved into nsContentSink.
Assignee | ||
Comment 1•21 years ago
|
||
The patch moves two things into the base class - Most of StartLayout() - Some <meta> handling code. Besides there are other minor cleanups - Eliminating an unused member in HTMLContentSink, an unused method etc. - Eliminating some redundant initializations and a pointless deathgrip in nsXMLContentSink. I had a look at unifying the <base> stuff. But html is just too quirky wrt base.
Assignee | ||
Updated•21 years ago
|
Attachment #138061 -
Flags: review?(bugmail)
I'm not really happy with the current state of affairs. The performance regression that the initial nsContentSink patch introduce is still an unsolved mystery and seems complealy unneccesary to me. I'd like to see at least some discussion about what we should do before going further.
ok, i talked with dbaron and jst on irc. We agreed that we can live with the perf-regression so i'll go ahead and start reviewing this.
Comment on attachment 138061 [details] [diff] [review] Fix >Index: html/document/src/nsHTMLContentSink.cpp >@@ -3753,115 +3741,31 @@ HTMLContentSink::StartLayout() ... >+ nsContentSink::StartLayout(mFrameset ? PR_TRUE : PR_FALSE); IMHO |!!mFrameset| looks nicer. Compleatly optional though. Really nice patch, this code is in a desperate need of this cleanup!
Attachment #138061 -
Flags: review?(bugmail) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #138061 -
Flags: superreview?(jst)
Comment 5•21 years ago
|
||
Comment on attachment 138061 [details] [diff] [review] Fix - In nsContentSink::StartLayout(): + PRUint32 i, ns = mDocument->GetNumberOfShells(); + for (i = 0; i < ns; i++) { ... + } ... + ns = mDocument->GetNumberOfShells(); I can't imagine how the number of shells could change between the two calls to mDocument->GetNumerOfShells() here, loose the latter one. + vm->GetRootView(rootView); + if (rootView) { + nsCOMPtr<nsIScrollableView> sview(do_QueryInterface(rootView)); + + if (sview) { do_QueryInterface() is null safe, loose the if (rootView) check. - In nsXMLContentSink::nsXMLContentSink(): { - - mDocument = nsnull; - mDocumentURL = nsnull; - mDocumentBaseURL = nsnull; - mParser = nsnull; mDocElement = nsnull; mText = nsnull; mTextLength = 0; mTextSize = 0; mConstrainSize = PR_TRUE; mInTitle = PR_FALSE; - mCSSLoader = nsnull; - mNeedToBlockParser = PR_FALSE; mPrettyPrintXML = PR_TRUE; mPrettyPrintHasSpecialRoot = PR_FALSE; mPrettyPrintHasFactoredElements = PR_FALSE; mHasProcessedBase = PR_FALSE; Wanna convert all that to member initializers? - In nsXMLContentSink::StartLayout(): if(docShellAsItem.get() == root.get()) { Loose the .get()'s while you're here :-) sr=jst
Attachment #138061 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 6•21 years ago
|
||
Updated for reviewers' comments.
Attachment #138061 -
Attachment is obsolete: true
Assignee | ||
Comment 7•21 years ago
|
||
Thanks for the reviews. Checked in. Linux mZdiff:-1420 Windows mZdiff:-650
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•