298264 - Change NS_ENSURE_SUCCESS()s in nsHTMLTokenizer to if-returns

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

There are currently a couple of NS_ENSURE_SUCCESS()s in nsHTMLTokenizer.cpp that
unnecessarily warn and clog up my debug console when loading long pages. In
particular, the offending functions are ConsumeStartTag and ConsumeEndTag, which
warn when consuming the token text fails (which happens for |<foo|).

I'm going to attach a patch to change these warnings into simple if-returns,
since they're not really necessary, and less DEBUG clutter is better.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

While looking at my patch, it occured to me that one of those
NS_ENSURE_SUCCESS()s was a no-op since we explicitly checked for success two
lines earlier (and such an early return would potentially cause crashes and
other bad stuff anyway).

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 186833 [details] [diff] [review]
patch v1

>@@ -973,22 +971,27 @@ nsresult nsHTMLTokenizer::ConsumeEndTag(
>   nsTokenAllocator* theAllocator=this->GetTokenAllocator();
>   aToken=theAllocator->CreateTokenOfType(eToken_end,eHTMLTag_unknown);
>   // Remember this for later in case you have to unwind...
>   PRInt32 theDequeSize=mTokenDeque.GetSize();
>   nsresult result=NS_OK;
>   
>   if(aToken) {
>     // Tell the new token to finish consuming text...
>     result= aToken->Consume(aChar,aScanner,mFlags);
>     AddToken(aToken,result,&mTokenDeque,theAllocator);
>-    NS_ENSURE_SUCCESS(result, result);
>-      
>+    if (NS_FAILED(result)) {
>+      // Note that this early-return here is safe because we have not yet
>+      // added any of our tokens to the queue, so we don't need to fall
>+      // through.

May as well be explicit, and say "is safe because AddToken has not added..."
here -- unless that would be misleading in some sense I haven't grokked.  sr=me
anyway.

/be

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 186833 [details] [diff] [review]
patch v1

r=jst

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 186833 [details] [diff] [review]
patch v1

This is a super-safe fix to reduce DEBUG-only clutter. I'm not sure if it's the
kind of thing to go in for 1.8b3, though.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 186833 [details] [diff] [review]
patch v1

Reducing DEBUG noise is good.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in (with an additional comment).