Closed Bug 316582 Opened 14 years ago Closed 7 years ago

Clean up style in parser/htmlparser

Categories

(Core :: DOM: HTML Parser, defect, P4, minor)

defect

Tracking

()

RESOLVED WONTFIX
mozilla1.9alpha1

People

(Reporter: mrbkap, Unassigned)

References

Details

Attachments

(2 files, 12 obsolete files)

7.02 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
277.99 KB, patch
jst
: review+
Details | Diff | Splinter Review
I'm going through parser/htmlparser and cleaning up the style. I've sort of finished CNavDTD.
Attached patch CNavDTD changes, v1 (obsolete) — Splinter Review
Here's the full patch for safekeeping, diff -w up soon.
Attached patch CNavDTD changes, v1, diff -w (obsolete) — Splinter Review
This changes the bracing style for elses, adds spaces where needed (after if, while, commas, etc) and even fixes up some comments to be mildly useful. I was moving javadoc comments to the header at one point, but ran out of steam and stopped ~70% or so through.
Attachment #203153 - Flags: review?(jst)
Priority: -- → P4
Attached patch CNavDTD changes, v2 (obsolete) — Splinter Review
This one passes the parser regression tests!
Attachment #203152 - Attachment is obsolete: true
Attachment #203153 - Attachment is obsolete: true
Attachment #203153 - Flags: review?(jst)
Attached patch CNavDTD changes, v2, diff -w (obsolete) — Splinter Review
Attachment #203324 - Flags: review?(jst)
Attached patch htmltokenizer changes, v1 (obsolete) — Splinter Review
The htmlparser changes are much smaller. If a diff -w would be significantly easier to review, please tell me.
Attachment #203325 - Flags: review?(bugmail)
Attached patch htmltokenizer changes, v1 -w (obsolete) — Splinter Review
Attachment #203426 - Flags: review?(bugmail)
Attachment #203325 - Attachment description: htmlparser changes, v1 → htmltokenizer changes, v1
Attachment #203325 - Flags: review?(bugmail)
Comment on attachment 203324 [details] [diff] [review]
CNavDTD changes, v2, diff -w

rs=jst
Attachment #203324 - Flags: superreview+
Attachment #203324 - Flags: review?(jst)
Attachment #203324 - Flags: review+
Comment on attachment 203324 [details] [diff] [review]
CNavDTD changes, v2, diff -w

This has been checked in.
Attachment #203324 - Attachment is obsolete: true
Attachment #203323 - Attachment is obsolete: true
Comment on attachment 203426 [details] [diff] [review]
htmltokenizer changes, v1 -w

>Index: parser/htmlparser/src/nsHTMLTokenizer.cpp
>@@ -73,54 +73,50 @@ NS_IMPL_ISUPPORTS1(nsHTMLTokenizer, nsIT
>  */
> nsHTMLTokenizer::nsHTMLTokenizer(PRInt32 aParseMode,
>                                  eParserDocType aDocType,
>                                  eParserCommands aCommand,
>                                  PRUint16 aFlags) :
>   nsITokenizer(), mTokenDeque(0), mFlags(aFlags)
> {
>   if (aParseMode==eDTDMode_full_standards ||
>       aParseMode==eDTDMode_almost_standards) {
>     mFlags |= NS_IPARSER_FLAG_STRICT_MODE;
>-  }
>-  else if (aParseMode==eDTDMode_quirks)  {
>+  } else if (aParseMode == eDTDMode_quirks)  {

I actually much prefer the old style. It makes the 'else' line up nicely with the 'if'. It's the style used in most of the content code. Feel free to go either way though.

>-CToken* nsHTMLTokenizer::PeekToken()
>+CToken*
>+nsHTMLTokenizer::PeekToken()
> {
>   return (CToken*)mTokenDeque.PeekFront();
> }

A bunch of these methods should be inlined. Especially if they're non-virtual.

>-static PRInt32 FindLastIndexOfTag(eHTMLTags aTag,nsDeque &aTagStack)
>+static
>+PRInt32 FindLastIndexOfTag(eHTMLTags aTag, nsDeque &aTagStack)

Put |PRInt32| on the same line as |static| or on a line of its own.

> nsresult nsHTMLTokenizer::ScanDocStructure(PRBool aFinalChunk)
...
>-      if(eToken_start == theType) {
>-        if(eFormUnknown == theToken->GetContainerInfo()) {
>+      if (eToken_start == theType  &&
>+          eFormUnknown == theToken->GetContainerInfo()) {

Extra space before |&&|. And put the |eFormUnknown| on the right side of the |==|.

>+nsHTMLTokenizer::ConsumeAttributes(PRUnichar aChar,
...
>-      // Much as I hate to do this, here's some special case code.
>-      // This handles the case of empty-tags in XML. Our last
>-      // attribute token will come through with a text value of ""
>-      // and a textkey of "/". We should destroy it.
>       if(NS_SUCCEEDED(result)) {
>-        PRBool isUsableAttr = PR_TRUE;
>-        const nsSubstring& key=theToken->GetKey();
>-        const nsAString& text=theToken->GetValue();
>-
>-        if(!key.IsEmpty() && kForwardSlash==key.First() && text.IsEmpty()) {
>-          if(!(mFlags & NS_IPARSER_FLAG_VIEW_SOURCE)) {
>-            // We only care about these in view-source.
>-            isUsableAttr = PR_FALSE;
>-          }
>-        }
>-        if(isUsableAttr) {

I assume all this cruft isn't needed any more?

>           ++theAttrCount;
>           AddToken((CToken*&)theToken,result,&mTokenDeque,theAllocator);
>-        }
>-        else {
>-          IF_FREE(theToken, mTokenAllocator);
>-        }
>-      }
>-      else {
>+      } else {
>         IF_FREE(theToken, mTokenAllocator);
>-        // Bad attributes are not a reason to set empty.
>+        // Bad attribute returns shouldn't propagate out.
>         if(NS_ERROR_HTMLPARSER_BADATTRIBUTE==result) {
>           result=NS_OK;
>-        } else {
>-          aToken->SetEmpty(PR_TRUE);

And this?

with that explained, r=me
Attachment #203426 - Flags: review?(bugmail) → review+
(In reply to comment #9)
> >-  else if (aParseMode==eDTDMode_quirks)  {
> >+  } else if (aParseMode == eDTDMode_quirks)  {
> 
> I actually much prefer the old style. It makes the 'else' line up nicely with
> the 'if'. It's the style used in most of the content code. Feel free to go
> either way though.

I think I've been infected by jst's DOM style and Brendan's JS engine style. I've been transitioning towards this sort of style for a bit now and think I want to continue moving this way.

> A bunch of these methods should be inlined. Especially if they're non-virtual.

I'll do that in a followup patch.

> I assume all this cruft isn't needed any more?

Yes, this is no longer needed because we no longer allow / in the attribute key, so we'd never hit this code anymore, except in view-source, where we handle it correctly.

> >+        // Bad attribute returns shouldn't propagate out.
> >         if(NS_ERROR_HTMLPARSER_BADATTRIBUTE==result) {
> >           result=NS_OK;
> >-        } else {
> >-          aToken->SetEmpty(PR_TRUE);
> 
> And this?

The SetEmpty here never really made sense to me, and it turns out to be a no-op, since we'll end up propagating the error return value, which will cause us to assume that we've hit EOF for this chunk and discard this attribute token and all of other tokens we've queued up in this round. This simply cleans up that logic.
Attachment #203426 - Flags: superreview?(dveditz)
Comment on attachment 203426 [details] [diff] [review]
htmltokenizer changes, v1 -w

>+nsHTMLTokenizer::ConsumeTag(PRUnichar aChar,
...
>-          PRBool isXML=(mFlags & NS_IPARSER_FLAG_XML);
>+          PRBool isXML = mFlags & NS_IPARSER_FLAG_XML;

I like the parens here, but I haven't examined the rest of the
file to verify the predominant style. Looks like you're doing
this in several places though, so obviously a personal style choice.

sr=dveditz
Attachment #203426 - Flags: superreview?(dveditz) → superreview+
Comment on attachment 203426 [details] [diff] [review]
htmltokenizer changes, v1 -w

I just checked these changes in.
Attachment #203426 - Attachment is obsolete: true
Attachment #203325 - Attachment is obsolete: true
Attached patch nsParser changes, v1 (obsolete) — Splinter Review
Attachment #206141 - Flags: superreview?(peterv)
Attachment #206141 - Flags: review?(peterv)
Attached patch nsParser changes, v1 -w (obsolete) — Splinter Review
Attachment #206142 - Flags: superreview?(peterv)
Attachment #206142 - Flags: review?(peterv)
Comment on attachment 206141 [details] [diff] [review]
nsParser changes, v1

This isn't exactly reviewable ;-)
Attachment #206141 - Flags: superreview?(peterv)
Attachment #206141 - Flags: review?(peterv)
Attached patch nsHTMLTokens changes, v1 -w (obsolete) — Splinter Review
Attachment #206349 - Flags: superreview?(jst)
Attachment #206349 - Flags: review?(jst)
Attachment #206402 - Flags: superreview?(jst)
Attachment #206402 - Flags: review?(mrbkap)
Comment on attachment 206402 [details] [diff] [review]
CParserContext and small nsParser changes

Good catch in FindSuitableDTD!
Attachment #206402 - Flags: review?(mrbkap) → review+
Comment on attachment 206142 [details] [diff] [review]
nsParser changes, v1 -w

> Index: parser/htmlparser/src/nsParser.cpp
> ===================================================================

> +nsParser::~nsParser()

>    if (mFlags & NS_PARSER_FLAG_PENDING_CONTINUE_EVENT) {
> -    NS_ASSERTION(mEventQueue != nsnull,"Event queue is null"); 
> +    NS_ASSERTION(mEventQueue != nsnull,"Event queue is null");

Add space after comma


> -    else {
> +    } else {

Buerk!


> +NS_IMETHODIMP_(nsDTDMode) nsParser::GetParseMode()

NS_IMETHODIMP_(nsDTDMode)
nsParser::GetParseMode()


>  nsParser::Parse(nsIInputStream* aStream,
>                  const nsACString& aMimeType,
>                  PRBool aVerifyEnabled,
>                  void* aKey,
>                  nsDTDMode aMode)

> +  } else{

Space after the else

>  nsParser::Parse(const nsAString& aSourceBuffer,
>                  void* aKey,
>                  const nsACString& aMimeType,
>                  PRBool aVerifyEnabled,
>                  PRBool aLastCall,
>                  nsDTDMode aMode)

> +    CParserContext* pc = nsnull;
> +
> +    if(!mParserContext || mParserContext->mKey != aKey) {

Space after if


> +      // Start fix bug 40143
> +      if (pc->mMultipart) {
> +        pc->mStreamListenerState = eOnDataAvail;
> +        if (pc->mScanner)
> +          pc->mScanner->SetIncremental(PR_TRUE);

Add braces?

> +      } else {
> +        pc->mStreamListenerState = eOnStop;
> +        if (pc->mScanner)
> +          pc->mScanner->SetIncremental(PR_FALSE);

Add braces?



> + *  This routine is called to cause the parser to continue parsing it's

its

> + *  (the pop and free) contexts until 1) it get's blocked again; 2) it runs

gets

> +nsParser::ResumeParse(PRBool allowIteration, PRBool aIsFinalChunk,
> +                      PRBool aCanInterrupt)
> +{
> +  nsresult result = NS_OK;
>  
> -  if((mFlags & NS_PARSER_FLAG_PARSER_ENABLED) && 
> +  if ((mFlags & NS_PARSER_FLAG_PARSER_ENABLED) &&
>       mInternalState != NS_ERROR_HTMLPARSER_STOPPARSING) {

Line this up with the first parens


> +        // TODO: Nuke these else-after-returns.

Why not now, they look straightforward.

> +        if (NS_ERROR_HTMLPARSER_BLOCK == result) {
>            if (mParserContext->mDTD) {
>              mParserContext->mDTD->WillInterruptParse(mSink);
>            }
> -          
> +
>            BlockParser();
>            return NS_OK;
> -        }
> -        
> -        else if (NS_ERROR_HTMLPARSER_STOPPARSING==result) {
> +        } else if (NS_ERROR_HTMLPARSER_STOPPARSING == result) {
>            // Note: Parser Terminate() calls DidBuildModel.
> -          if(mInternalState!=NS_ERROR_HTMLPARSER_STOPPARSING) {
> +          if (mInternalState != NS_ERROR_HTMLPARSER_STOPPARSING) {
>              DidBuildModel(mStreamStatus);
>              mInternalState = result;
>            }
> -          return NS_OK;
> -        }
> -                  
> -        else if(((NS_OK==result) && (theTokenizerResult==kEOF)) || (result==NS_ERROR_HTMLPARSER_INTERRUPTED)){
> -
> -          PRBool theContextIsStringBased=PRBool(CParserContext::eCTString==mParserContext->mContextType);
> -          if( (eOnStop==mParserContext->mStreamListenerState) || 
> -              (!mParserContext->mMultipart) || theContextIsStringBased) {
> -
> -            if(!mParserContext->mPrevContext) {
> -              if(eOnStop==mParserContext->mStreamListenerState) {
>  
> -                DidBuildModel(mStreamStatus);          
> +          return NS_OK;
> +        } else if ((NS_OK == result && theTokenizerResult == kEOF) ||
> +                   result == NS_ERROR_HTMLPARSER_INTERRUPTED) {
> +          PRBool theContextIsStringBased =
> +            CParserContext::eCTString == mParserContext->mContextType;
> +
> +          if (eOnStop==mParserContext->mStreamListenerState ||

Spaces around == and inverse the operands?

> +              !mParserContext->mMultipart || theContextIsStringBased) {
> +            if (!mParserContext->mPrevContext) {
> +              if (eOnStop == mParserContext->mStreamListenerState) {

Inverse the operands?


> +DetectByteOrderMark(const unsigned char* aBytes, PRInt32 aLen, nsCString& oCharset, PRInt32& oCharsetSource) {

Wrap?


> -  while (theContext) {
> -    if (theContext->mRequest != request && theContext->mPrevContext)
> -      theContext = theContext->mPrevContext;
> -    else break;
> +  while (theContext && theContext->mRequest != request) {
> +    theContext = theContext->mPrevContext;
>    }
>  
> -  if (theContext && theContext->mRequest == request) {
> -
> +  if (theContext) {

Ooooh, and I liked this piece of code so much! Now it'll be easy to figure out what it does :-(.
Attachment #206142 - Flags: superreview?(peterv)
Attachment #206142 - Flags: superreview+
Attachment #206142 - Flags: review?(peterv)
Attachment #206142 - Flags: review+
Comment on attachment 206142 [details] [diff] [review]
nsParser changes, v1 -w

Checked into trunk.
Attachment #206142 - Attachment is obsolete: true
Attachment #206141 - Attachment is obsolete: true
this appears to have caused:
Bug 321344
Bug 321345
(In reply to comment #20)
> > -  while (theContext) {
> > -    if (theContext->mRequest != request && theContext->mPrevContext)
> > -      theContext = theContext->mPrevContext;
> > -    else break;
> > +  while (theContext && theContext->mRequest != request) {
> > +    theContext = theContext->mPrevContext;
> >    }
> >  
> > -  if (theContext && theContext->mRequest == request) {
> > -
> > +  if (theContext) {
> 
> Ooooh, and I liked this piece of code so much! Now it'll be easy to figure out
> what it does :-(.
> 

I'm guessing this is what caused the fallout.  Recompiling Firefox to test my theory...
No longer blocks: 321344, 321345
Depends on: 321344
Attached patch Fix for previous patch. (obsolete) — Splinter Review
Addresses a probable issue with nsParser::ResumeParse; probable cause for bug 321344.
Attachment #206717 - Flags: superreview?
Attachment #206717 - Flags: review?(mrbkap)
Attachment #206717 - Flags: review?(mrbkap) → review?
Comment on attachment 206717 [details] [diff] [review]
Fix for previous patch.

This the code as written is fine (the check for theContext->mPrevContext was weird and unnecessary). Your comment says your patch addresses nsParser::ResumeParse, does it look anything like my patch in bug 321344?
Attachment #206717 - Flags: review? → review-
(In reply to comment #25)
> (From update of attachment 206717 [details] [diff] [review] [edit])
> This the code as written is fine (the check for theContext->mPrevContext was
> weird and unnecessary). Your comment says your patch addresses
> nsParser::ResumeParse, does it look anything like my patch in bug 321344?
> 

Well, I was assuming that was related to the problem as it was one of the only significant changes in the patch that caused the problem.  Oh well, I tried. :/
Attachment #206717 - Attachment is obsolete: true
Attachment #206717 - Flags: superreview?
Comment on attachment 206402 [details] [diff] [review]
CParserContext and small nsParser changes

sr=jst
Attachment #206402 - Flags: superreview?(jst) → superreview+
Comment on attachment 206349 [details] [diff] [review]
nsHTMLTokens changes, v1 -w

r+sr=jst
Attachment #206349 - Flags: superreview?(jst)
Attachment #206349 - Flags: superreview+
Attachment #206349 - Flags: review?(jst)
Attachment #206349 - Flags: review+
Comment on attachment 206348 [details] [diff] [review]
nsHTMLTokens changes, v1

I checked this in.
Attachment #206348 - Attachment is obsolete: true
Attachment #206349 - Attachment is obsolete: true
I generated this with ../cleanup parser/htmlparser/{public,src}/* using the script:

for i in "$@"
do
    echo $i
    sed -e '{s/\s$//}' -i $i
done

it is diff -w clean. I think we should do this now, since people have started nuking trailing whitespace in files they touch.
Attachment #349518 - Flags: superreview?(jst)
Attachment #349518 - Flags: review?(jst)
Please pretend that the sed script was actually:
    sed -e '{s/\s*$//}' -i $i
I don't think it's worth re-uploading the patch.
Comment on attachment 349518 [details] [diff] [review]
Remove trailing whitespace

Ship it!
Attachment #349518 - Flags: superreview?(jst)
Attachment #349518 - Flags: superreview+
Attachment #349518 - Flags: review?(jst)
Attachment #349518 - Flags: review+
Attachment #349518 - Flags: approval1.9.1?
Comment on attachment 349518 [details] [diff] [review]
Remove trailing whitespace

I'd like to get this in at some point.
Attachment #349518 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 349518 [details] [diff] [review]
Remove trailing whitespace

a191=beltzner
One entry-one-exit is nice an all, but this is nicer.
Attachment #356593 - Flags: superreview?(jst)
Attachment #356593 - Flags: review?(jst)
Attachment #356593 - Flags: superreview?(jst)
Attachment #356593 - Flags: superreview+
Attachment #356593 - Flags: review?(jst)
Attachment #356593 - Flags: review+
Comment on attachment 349518 [details] [diff] [review]
Remove trailing whitespace

I don't think it's worth landing this on the 1.9.1 branch anymore since it hasn't even landed on trunk yet.
Attachment #349518 - Flags: approval1.9.1+
Assignee: mrbkap → nobody
No need for this anymore, really.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.