Closed
Bug 316582
Opened 19 years ago
Closed 11 years ago
Clean up style in parser/htmlparser
Categories
(Core :: DOM: HTML Parser, defect, P4)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
WONTFIX
mozilla1.9alpha1
People
(Reporter: mrbkap, Unassigned)
References
Details
Attachments
(2 files, 12 obsolete files)
7.02 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
277.99 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
I'm going through parser/htmlparser and cleaning up the style. I've sort of finished CNavDTD.
Reporter | ||
Comment 1•19 years ago
|
||
Here's the full patch for safekeeping, diff -w up soon.
Reporter | ||
Comment 2•19 years ago
|
||
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)
Reporter | ||
Updated•19 years ago
|
Priority: -- → P4
Reporter | ||
Comment 3•19 years ago
|
||
This one passes the parser regression tests!
Attachment #203152 -
Attachment is obsolete: true
Attachment #203153 -
Attachment is obsolete: true
Attachment #203153 -
Flags: review?(jst)
Reporter | ||
Comment 4•19 years ago
|
||
Attachment #203324 -
Flags: review?(jst)
Reporter | ||
Comment 5•19 years ago
|
||
The htmlparser changes are much smaller. If a diff -w would be significantly easier to review, please tell me.
Attachment #203325 -
Flags: review?(bugmail)
Reporter | ||
Comment 6•19 years ago
|
||
Attachment #203426 -
Flags: review?(bugmail)
Reporter | ||
Updated•19 years ago
|
Attachment #203325 -
Attachment description: htmlparser changes, v1 → htmltokenizer changes, v1
Attachment #203325 -
Flags: review?(bugmail)
Comment 7•19 years ago
|
||
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+
Reporter | ||
Comment 8•19 years ago
|
||
Comment on attachment 203324 [details] [diff] [review] CNavDTD changes, v2, diff -w This has been checked in.
Attachment #203324 -
Attachment is obsolete: true
Reporter | ||
Updated•19 years ago
|
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+
Reporter | ||
Comment 10•19 years ago
|
||
(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.
Reporter | ||
Updated•19 years ago
|
Attachment #203426 -
Flags: superreview?(dveditz)
Comment 11•19 years ago
|
||
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+
Reporter | ||
Comment 12•19 years ago
|
||
Comment on attachment 203426 [details] [diff] [review] htmltokenizer changes, v1 -w I just checked these changes in.
Attachment #203426 -
Attachment is obsolete: true
Reporter | ||
Updated•19 years ago
|
Attachment #203325 -
Attachment is obsolete: true
Reporter | ||
Comment 13•19 years ago
|
||
Attachment #206141 -
Flags: superreview?(peterv)
Attachment #206141 -
Flags: review?(peterv)
Reporter | ||
Comment 14•19 years ago
|
||
Attachment #206142 -
Flags: superreview?(peterv)
Attachment #206142 -
Flags: review?(peterv)
Reporter | ||
Comment 15•19 years ago
|
||
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)
Reporter | ||
Comment 16•19 years ago
|
||
Reporter | ||
Comment 17•19 years ago
|
||
Attachment #206349 -
Flags: superreview?(jst)
Attachment #206349 -
Flags: review?(jst)
Comment 18•19 years ago
|
||
Attachment #206402 -
Flags: superreview?(jst)
Attachment #206402 -
Flags: review?(mrbkap)
Reporter | ||
Comment 19•19 years ago
|
||
Comment on attachment 206402 [details] [diff] [review] CParserContext and small nsParser changes Good catch in FindSuitableDTD!
Attachment #206402 -
Flags: review?(mrbkap) → review+
Comment 20•19 years ago
|
||
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+
Reporter | ||
Comment 21•19 years ago
|
||
Comment on attachment 206142 [details] [diff] [review] nsParser changes, v1 -w Checked into trunk.
Attachment #206142 -
Attachment is obsolete: true
Reporter | ||
Updated•19 years ago
|
Attachment #206141 -
Attachment is obsolete: true
Comment 22•19 years ago
|
||
this appears to have caused: Bug 321344 Bug 321345
Updated•19 years ago
|
Comment 23•19 years ago
|
||
(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...
Reporter | ||
Updated•19 years ago
|
Comment 24•19 years ago
|
||
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?
Reporter | ||
Comment 25•19 years ago
|
||
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-
Comment 26•19 years ago
|
||
(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. :/
Reporter | ||
Updated•19 years ago
|
Attachment #206717 -
Attachment is obsolete: true
Attachment #206717 -
Flags: superreview?
Comment 27•19 years ago
|
||
Comment on attachment 206402 [details] [diff] [review] CParserContext and small nsParser changes sr=jst
Attachment #206402 -
Flags: superreview?(jst) → superreview+
Comment 28•19 years ago
|
||
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+
Reporter | ||
Comment 29•19 years ago
|
||
Comment on attachment 206348 [details] [diff] [review] nsHTMLTokens changes, v1 I checked this in.
Attachment #206348 -
Attachment is obsolete: true
Reporter | ||
Updated•19 years ago
|
Attachment #206349 -
Attachment is obsolete: true
Reporter | ||
Comment 30•16 years ago
|
||
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)
Reporter | ||
Comment 31•16 years ago
|
||
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 32•16 years ago
|
||
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+
Reporter | ||
Updated•16 years ago
|
Attachment #349518 -
Flags: approval1.9.1?
Reporter | ||
Comment 33•16 years ago
|
||
Comment on attachment 349518 [details] [diff] [review] Remove trailing whitespace I'd like to get this in at some point.
Updated•16 years ago
|
Attachment #349518 -
Flags: approval1.9.1? → approval1.9.1+
Comment 34•16 years ago
|
||
Comment on attachment 349518 [details] [diff] [review] Remove trailing whitespace a191=beltzner
Reporter | ||
Comment 35•16 years ago
|
||
One entry-one-exit is nice an all, but this is nicer.
Attachment #356593 -
Flags: superreview?(jst)
Attachment #356593 -
Flags: review?(jst)
Updated•16 years ago
|
Attachment #356593 -
Flags: superreview?(jst)
Attachment #356593 -
Flags: superreview+
Attachment #356593 -
Flags: review?(jst)
Attachment #356593 -
Flags: review+
Reporter | ||
Comment 36•16 years ago
|
||
Comment on attachment 356593 [details] [diff] [review] Fix up the keygen handler function http://hg.mozilla.org/mozilla-central/rev/62ff51a9286c
Attachment #356593 -
Attachment is obsolete: true
Reporter | ||
Comment 37•15 years ago
|
||
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+
Reporter | ||
Updated•15 years ago
|
Assignee: mrbkap → nobody
Reporter | ||
Comment 38•11 years ago
|
||
No need for this anymore, really.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•