Closed Bug 298264 Opened 19 years ago Closed 19 years ago

Change NS_ENSURE_SUCCESS()s in nsHTMLTokenizer to if-returns

Categories

(Core :: DOM: HTML Parser, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(1 file)

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.
Attached patch patch v1Splinter Review
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).
Attachment #186833 - Flags: superreview?(brendan)
Attachment #186833 - Flags: review?(jst)
Status: NEW → ASSIGNED
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
Attachment #186833 - Flags: superreview?(brendan) → superreview+
Comment on attachment 186833 [details] [diff] [review]
patch v1

r=jst
Attachment #186833 - Flags: review?(jst) → review+
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.
Attachment #186833 - Flags: approval1.8b3?
Comment on attachment 186833 [details] [diff] [review]
patch v1

Reducing DEBUG noise is good.

/be
Attachment #186833 - Flags: approval1.8b3? → approval1.8b3+
Fix checked in (with an additional comment).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: