nsHTMLContentSink.cpp is a mess

RESOLVED FIXED in mozilla1.2beta

Status

()

Core
DOM
RESOLVED FIXED
16 years ago
a year ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
mozilla1.2beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
I can't take this any more, I need some consistency in this old old unreadable
code, so I started hacking. Now I'll get to be the proud owner of most of the
content sink code, oh well...

Patch coming up...
(Assignee)

Comment 1

16 years ago
Created attachment 99011 [details] [diff] [review]
diff
(Assignee)

Comment 2

16 years ago
Created attachment 99012 [details] [diff] [review]
diff-w, this one's for the reviewers.
Comment on attachment 99012 [details] [diff] [review]
diff-w, this one's for the reviewers.

>Index: content/html/document/src/nsHTMLContentSink.cpp

@@ -199,15 +200,28 @@
>+// Lower the value for mNotificationInterval and
>+// mMaxTokenProcessingTime

This comment will fit well under 80 chars on one line.	Move it up?  :-)

@@ -758,27 +988,21 @@
+  nsCOMPtr<nsIFormControl> formControl(do_QueryInterface(aForm));

Use aContent, not aForm.


@@ -1332,8 +1609,7 @@
-  if ((2 == mStackPos) &&
-      (mSink->mBody == mStack[1].mContent)) {
+  if ((2 == mStackPos) && (mSink->mBody == mStack[1].mContent)) {

Since you've changed a few other instances of this earlier on, mStackPos == 2
for consistency's sake.


@@ -1554,8 +1839,8 @@
-      break;

+    break;

Err, what's up with this?  Leave the indent alone in this switch.  You leave
all the others alone... (Yes I see you are moving the cases too from readingn
the non diff -w but don't do that, unless you touch every other switch in the
file which I beg you to not do since I think it decreases readability...)


@@ -1741,37 +2020,32 @@
+  if ((!mSink->mBody) && mSink->mHead) {

Lose the extra parens.


@@ -1810,16 +2081,20 @@
   if (0 != mStackPos) {

mStackPos != 0

Dude, if you are going to change some of these as you did earlier in the file,
change them all.  I'm not going to list any more of these (there are a few more
of these in the following hunks of patch), but please deal with this  :-)


@@ -2240,16 +2509,17 @@
-  NS_PRECONDITION(nsnull != aDoc, "null ptr");
-  NS_PRECONDITION(nsnull != aURL, "null ptr");
-  NS_PRECONDITION(nsnull != aContainer, "null ptr");
-  if ((nsnull == aDoc) || (nsnull == aURL) || (nsnull == aContainer)) {
+  if (!aDoc || !aURL || !aContainer) {
+    NS_ERROR("Null ptr!");

You're slightly changing what that does in debug builds.  With NS_PRECONDITION,
at least the user will be able to tell which variable was null.  Here they will
have to do some extra hacking.	Being that this is debug though, I don't think
it matters and if the developer can't figure out which of the three is null,
well then I guess there are larger problems.  The convenience is kind of nice
though....


@@ -2440,15 +2716,19 @@
+      // Don't return the error result, just reset flag which
+      // indicates that it can interrupt parsing. If
+      // AddDummyParserRequests fails it should not affect
       // WillBuildModel.

Nit: Move the if down to the next line.  Your comment will still be under the
80 char limit, but having the If next to it's condition makes the comment
easier to follow, IMO.


@@ -2771,35 +3076,46 @@

-  NS_PRECONDITION(mCurrentContext != nsnull && aPosition > -1, "non-existing
context");
+  NS_PRECONDITION(nsnull && aPosition > -1, "non-existing context");

I don't think that's what you want :-)


@@ -2771,35 +3076,46 @@
-  if(mCurrentContext->mText != nsnull)
+
+  if (mCurrentContext->mText) {
     delete [] mCurrentContext->mText;
+  }

Remove the if.


Gonna take a break for now and revisit this again in a short while.  For my
reference, I should pick back up at
@@ -2811,23 +3127,23 @@
Comment on attachment 99012 [details] [diff] [review]
diff-w, this one's for the reviewers.

@@ -3032,8 +3374,7 @@

       // XXX: Would it be better to use |parent->ChildCount() - 1| so that
       //      we don't cause notifications for the <head> element and it's
       //      children?

You missed fixing this one.


@@ -3381,14 +3758,15 @@
-  nsresult rv = NS_OK;
   MOZ_TIMER_DEBUGLOG(("Start: nsHTMLContentSink::AddDocTypeDecl()\n"));
   MOZ_TIMER_START(mWatch);

+  nsresult rv = NS_OK;
   nsCOMPtr<nsIDOMDocument> doc(do_QueryInterface(mHTMLDocument));

-  if (!doc)
+  if (!doc) {
     return NS_OK;
+  }

Moving nsresult rv = NS_OK seems useless as you have it.  Either remove it if
it's not used at all, or move it down to where it actually is used.

@@ -3490,18 +3870,14 @@
+	   // We didn't find an end quote, then we just make sure we
+	   // cut of the '>' on the end of the doctype declaration

s/then we/so/
s/of/off/

+	 // If we found an closing quote nor a '>' we truncate systemId
+	 // at that length.

s/an/a/
s/nor/or/

@@ -3513,31 +3889,26 @@
+  // Cut out "DOCTYPE" or "<!DOCTYPE" from the name.

Nit: how about re-arranging those so they actually match the code?

+  // Check if name contains whitespace chars, if it does and the first
+  // char is not a quote, we make set the name to contain the
+  // characters before the whitespace

Add a period after "chars", and capitalize "if": no run-on sentences please :-)
 Also kill "make" which is useless here.


@@ -3768,11 +4140,23 @@

+  PRUint32 flag;
+
+  if (mHTMLDocument && mHTMLDocument->IsWriting()) {
+    flag = nsIElementObserver::IS_DOCUMENT_WRITE;
+  } else {
+    flag = 0;
+  }

Just init your variable and remove the else.


@@ -3881,10 +4270,11 @@

	     nsIScrollableView* sview = nsnull;
+	     rootView->QueryInterface(NS_GET_IID(nsIScrollableView),
+				      (void**)&sview);

copy ctor with do_QI would be great here.


@@ -4123,20 +4545,21 @@

     parent=mCurrentContext->mStack[mCurrentContext->mStackPos-1].mContent;

Fix the spacing around the assignment operator while you're at it (as if you
aren't getting enough cvsblame...)

Same issue around here: HTMLContentSink::ProcessMETATag() @@ -4524,46 +4987,45
@@


@@ -4173,7 +4596,8 @@
   PRUnichar* start = (PRUnichar*)(const PRUnichar*)stringList.get();

Is the double cast really needed here?


+HTMLContentSink::ProcessHTTPHeaders(nsIChannel* aChannel)
+{
   NS_ASSERTION(aChannel,"can't process http headers without a channel");

Space after comma.


+HTMLContentSink::ProcessHeaderData()

+  // XXX necko isn't going to process headers coming in from the
+  // parser

Did you set your script on uber-anal mode or something?  Fit this all on one
line.  You know you want to...


@@ -4686,10 +5171,12 @@
+    nsCOMPtr<nsICodebasePrincipal> originalCodebase =
+      do_QueryInterface(originalPrincipal, &rv);
     if (NS_FAILED(rv)) {

Check the object, not rv.

@@ -4965,8 +5467,7 @@

+    if (mParser && aWasPending){

Needs a space before brace.

@@ -5057,7 +5559,7 @@
-  // Create a text node holding the content
+  // Create a text node holding the content.
   // First, get the text content of the script tag

Okay you really did set it on uber-anal mode, but it's not anal enough.  Or
something.  Period after the second sentence too if you're going to be doing
that sort of thing.
(Assignee)

Comment 5

16 years ago
Thanks for the feedback, all the issues and nits mentioned about have been
fixed, except the comments on the formatting of comments in the code. The
formatting I use is what emacs does by default. It formats comments to ~70
columns so that they're easier to read, so 80 columns is not what counts wrt
comment formatting, and some of the whitespace issues you mentioned were due to
the fact that you're reading a diff -w, the cases you pointed out were fixed in
the new version of the file.

As for the double cast on nsAFlatString::get(), I don't think it's needed any
more, I bet it's left-overs from a world where there was a protected PRUnichar*
operator on some of our string classes, or some such. I replaced those two casts
with a NS_CONST_CAST(PRUnichar *, str.get());

New diff's coming up.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2beta
(Assignee)

Comment 6

16 years ago
Created attachment 99188 [details] [diff] [review]
Updated diff.
Attachment #99011 - Attachment is obsolete: true
Attachment #99012 - Attachment is obsolete: true
(Assignee)

Comment 7

16 years ago
Created attachment 99189 [details] [diff] [review]
Updated diff -w
Comment on attachment 99189 [details] [diff] [review]
Updated diff -w

Some more comments:

-HTMLContentSink::GetPref(PRInt32 aTag,PRBool& aPref) {
+HTMLContentSink::GetPref(PRInt32 aTag, PRBool& aPref) {

Wanna move the opening brace down?

@@ -4965,11 +5482,10 @@
+    if (mParser && aWasPending){

Space before brace.

@@ -4991,19 +5507,19 @@
+  if (mParser && mParser->IsParserEnabled() && aWasPending){

Here too.

@@ -5051,32 +5568,32 @@
+    nsCOMPtr<nsIDOMText> tc;
+    rv = text->QueryInterface(NS_GET_IID(nsIDOMText),
+			       (void**)getter_AddRefs(tc));
     if (NS_OK == rv) {

nsCOMPtr<nsIDOMText> tc(do_QueryInterface(text));
if (tc)

Also note that you're changing the order of the operations in this block.  I
don't think it matters one way or the other, but please double check.
(Assignee)

Comment 9

16 years ago
Fixed. The tree's still in flames (but turning green) from me being stupid with
a last minute change, but it's in!
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.