Closed Bug 393286 Opened 16 years ago Closed 16 years ago

Make nsCSSScanner::Read regularize newlines

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sharparrow1, Assigned: sharparrow1)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
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.
Attachment #277776 - Flags: review?(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
Attachment #277776 - Flags: superreview+
Attachment #277776 - Flags: review?(bzbarsky)
Attachment #277776 - Flags: review+
(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.
Attachment #277776 - Flags: approval1.9?
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
Attachment #277776 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.