483971 - nsCSSScanner: cleanup of EatWhiteSpace and removal of EatNewline

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Alfred Kayser]

1. Fix EatWhitespace to eat all whitespace (using IsWhitespace) and remove return
value (is not used).
2. EatNewline is only used in one place, and can be inlined (and making Read logic simpler).

Comment 1

[Alfred Kayser]

Attachment: Patch to clean EatWhiteSpace, remove EatNewline and old ifdef 0 code

Comment 2

[Alfred Kayser]

Attachment: V2: hg diff -pU8 version from current build

Comment 3

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

Comment on attachment 368884 [details] [diff] [review]
V2: hg diff -pU8 version from current build

>+    if ((ch != ' ') && (ch != '\n') && (ch != '\t')) {
>+      Pushback(ch);
>+      break;
>+    }

I think you've introduced tabs here.

>+  // "Any character except a hexidecimal digit can be escaped to
>+  // remove its special meaning by putting a backslash in front"
>+  // -- CSS1 spec section 7.1
>+  if ((ch > 0) && (ch != '\n')) {
>+    aOutput.Append(ch);
>   }

You've made it so we no longer consume the character in this case.  Please fix that and add a regression test that would catch the mistake.

I think it looks ok with those two issues fixed.


What regression tests did you run before requesting review?  (for both the original patch and the revised patch)

Comment 4

[Alfred Kayser]

Attachment: V3: Do consume the escaped character!

Changes:
1. Remove the tabs.
2. Added 'ch = Read();' to consume the escaped character. Note, previous testing was not done correctly, causing to miss the introduced issue.

Tested using mochitest on /tests/layout:
Before: passed 30889, failed 0, todo 80
With faulty patch: passed 30858, failed 35!, todo 80.
With correct patch: passed 30889, failed 0, todo 80.

As the mochitests (when correctly run on correctly rebuild) clear show the impact of a faulty patch, I think that it is not needed to add specific tests for this. Even specifically: test_parse_rule.html does already include specific tests for escaping, including escaped newlines.

Comment 5

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

Comment on attachment 369990 [details] [diff] [review]
V3: Do consume the escaped character!

r=dbaron, but in the future please run appropriate tests before requesting review.  (For example, if you're changing the CSS parser, you should obviously be running at least the mochitests in layout/style.)

Comment 6

[Alfred Kayser]

dbaron, is a separate sr needed, or can you do this also?

P.s. I promise that I will do the tests on the correct build before requesting review.

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3c687f3d4ff4

Comment 8

[Boris Zbarsky [:bzbarsky]]

Backed out; the push this was part of seems to be causing random leak orange on Mac.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Looks like the random leak orange is still happening, so this can reland as desired...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Actually, scratch that.  This orange is on Linux, whereas the orange from the push this landed as part of was on Mac.  I recommend landing this separately from the other 4 patches that landed with it.

Comment 11

[Dão Gottwald [:dao]]

The leak exists on 1.9.1, so this didn't cause it.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/02f8e9df8612

Comment 13

[Alfred Kayser]

Thanks!