393286 - Make nsCSSScanner::Read regularize newlines

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Eli Friedman]

Attachment: Patch

Hopefully straightforward patch; simplifies Read() and some of the other code that has to deal with newlines by fixing them immediately as they are read out of a string.  Also splits out an EnsureData method for reading more data into the buffer, because it is needed in multiple places.

If this patch is a bit too big, I can split out the change to split out EnsureData; it seems like a simple enough change, though.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277776 [details] [diff] [review]
Patch

>Index: nsCSSScanner.cpp
>+PRBool nsCSSScanner::EnsureData(nsresult& aErrorCode) >+  if (mInputStream) {
>+    mOffset = 0;

We used to set mOffset = 0 even if !mInputStream.  Seems safer to keep doing that to me...  Unless we have good reason to change it?

> PRInt32 nsCSSScanner::Read(nsresult& aErrorCode)
>+    if (mOffset == mCount && !EnsureData(aErrorCode)) {

Why duplicate the test from inside EnsureData?  Just to avoid the function call?  I guess that makes sense, since we'll always hit this code.

Rest of this looks great.  r+sr=bzbarsky

Comment 2

[Eli Friedman]

(In reply to comment #1)
> (From update of attachment 277776 [details] [diff] [review])
> >Index: nsCSSScanner.cpp
> >+PRBool nsCSSScanner::EnsureData(nsresult& aErrorCode) >+  if (mInputStream) {
> >+    mOffset = 0;
> 
> We used to set mOffset = 0 even if !mInputStream.  Seems safer to keep doing
> that to me...  Unless we have good reason to change it?

It seems more straightforward to me to do it this way, and it's perfectly safe: we never check the value of mOffset directly, only its relation to mCount.  I don't see the point to setting mCount and mOffset to zero.

> > PRInt32 nsCSSScanner::Read(nsresult& aErrorCode)
> >+    if (mOffset == mCount && !EnsureData(aErrorCode)) {
> 
> Why duplicate the test from inside EnsureData?  Just to avoid the function
> call?  I guess that makes sense, since we'll always hit this code.

I could do it either way... but yeah, it's cheap to check, and we want to avoid the function call on the common path.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 277776 [details] [diff] [review]
Patch

>+PRBool nsCSSScanner::EnsureData(nsresult& aErrorCode) {

Please put the brace on its own line.

>+    // There are four types of newlines in CSS: "\r", "\n", "\r\n", and "\f".

Wow, I hadn't even noticed the \f thing, but it's been there all along.

a1.9=dbaron

Comment 4

[Eli Friedman]

Checked in.