276149 - Remove method |nsScanner::Eof()| (bloat) and fix error handling in nsScanner.cpp

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Hans-Andreas Engel]

The method |nsScanner::Eof()| is somewhat superfluous and it makes nsScanner.cpp
more difficult to read.  This method can be eliminated by basically replacing
its invocations with |nsScanner::FillBuffer()|.

Comment 1

[Hans-Andreas Engel]

This bug can be resolved as follows.

1) First, we note that |nsScanner::Eof()| is identical to 
|return mSlidingBuffer ? FillBuffer() : kEOF;|

   Let us consider the following methods of nsScanner: |GetChar()|, |Peek()|,
|SkipWhitespace()|, |ReadWhitespace()|, |ReadTagIdentifier()|,
|ReadEntityIdentifier()|, |ReadNumber()|, |ReadWhile()|, and |ReadUntil()| 
(some of these methods are overloaded).  Each method begins with invoking
|if(!mSlidingBuffer) { return kEOF; }|.  This implies that in the rest of the
method body, |mSlidingBuffer| is always set.  Thus, we can replace all calls to
|Eof()| with |FillBuffer()|.


2) The error code returned from Eof()/FillBuffer() is only checked if it is
|kEOF|.  However, the error code can result from |mInputStream->Read()| and
could be, say, NS_BASE_STREAM_CLOSED.  So a more general error detection should
be used, e.g., replacing |if (result == kEOF)| with |if (NS_FAILED(result))|. 
Further, the error code should be returned directly and not via a second
invocation of Eof()/FillBuffer().


3) |nsScanner::Eof()| is used in only two other locations, namely in   
|nsExpatDriver::ConsumeToken()| and in |nsParser::Parse()|.  In both cases we
can replace |Eof()| by |FillBuffer()|.


4) Remove |Eof()| from nsScanner.h and make |nsScanner::FillBuffer()| public 
(cf. point 3).

Comment 2

[Hans-Andreas Engel]

Attachment: Remove method |nsScanner::Eof()| and fix error handling in nsScanner.cpp

This patch also fixes Bug 181543 by adding a check for the return value of
|Peek()| in the fourth definition of |nsScanner::ReadUntil()|.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 169663 [details] [diff] [review]
Remove method |nsScanner::Eof()| and fix error handling in nsScanner.cpp

>Index: nsScanner.cpp
>@@ -1187,17 +1161,17 @@ nsresult nsScanner::ReadUntil(nsAString&
>   // If we are here, we didn't find any terminator in the string and
>   // current = mEndPosition
>   SetPosition(current);
>   AppendUnicodeTo(origin, current, aString);
>-  return Eof();
>+  return FillBuffer();
> }

This logic appears a bunch throughout the scanner functions and I'm not sure
it's right. I think that if FillBuffer() returns anything other than kEOF,
tokenizer functions will get very confused, as an NS_OK return value indicates
that the function successfully found the end condition (or hit a real EOF).
Shouldn't we get another OnDataAvailable call in the parser?

However, that's probably outside the scope of this bug, so r=mrbkap

Comment 4

[Hans-Andreas Engel]

Thank you for the fast reviewing!

> I think that if FillBuffer() returns anything other than kEOF, tokenizer
> functions will get very confused, as an NS_OK return value indicates
> that the function successfully found the end condition (or hit a real EOF).
> (...) However, that's probably outside the scope of this bug, so r=mrbkap

I absolutely agree, there might be some further problems, especially in
parser/htmlparser/src/nsHTMLTokens.cpp.

As you indicate, this issue is not related to this bug.  The proposed patch does
not change behavior there, since in the old version |Eof()| returned the same
values as |FillBuffer()| returns in the new version.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 169663 [details] [diff] [review]
Remove method |nsScanner::Eof()| and fix error handling in nsScanner.cpp

>@@ -1323,35 +1297,39 @@ nsresult nsScanner::ReadUntil(nsAString&
>   }
> 
>   nsScannerIterator origin, current;
> 
>   origin = mCurrentPosition;
>   current = origin;
> 
>   PRUnichar theChar;
>-  Peek(theChar);
>-  
>+  nsresult = Peek(theChar);

This can't be right -- isn't there a result missing after the nsresult?  Or if
it was declared earlier, then this should assign to result, not to a new
variable (love C++'s type name disambiguation) named nsresult.

Please fix that and I'll re-review.  Also calling on cc: list additions to
look, in case there's more to see that I'm missing.

Thanks for diving into this code, it isn't pretty, and it definitely needs
cleanup and ownership.

/be

Comment 6

[Hans-Andreas Engel]

Attachment: Remove method |nsScanner::Eof()| and fix error handling in nsScanner.cpp, v2

Fixed silly mistake, it was not even C++ magic ;)

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 169681 [details] [diff] [review]
Remove method |nsScanner::Eof()| and fix error handling in nsScanner.cpp, v2

sr=me, thanks.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Patch has been checked in, marking as FIXED.