229575 - Move some more code into nsContentSink

Status: VERIFIED FIXED

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

Description

[Harshal Pradhan]

There is a little bit of stuff that is pretty much common in html and xml and
can easily be moved into nsContentSink.

Comment 1

[Harshal Pradhan]

Attachment: Fix

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.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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!

Comment 5

[Johnny Stenback (:jst)]

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

Comment 6

[Harshal Pradhan]

Attachment: Final patch for checkin

Updated for reviewers' comments.

Comment 7

[Harshal Pradhan]

Thanks for the reviews. Checked in. 

Linux mZdiff:-1420
Windows mZdiff:-650

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using LXR.