1331994 - Back out bug 1300374 for Firefox 51

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1300374 comment 22.  The PR aspects of this suck, but since Hixie is being obstinate and this is not really blocking anything yet in practice...

Comment 1

[Sylvestre Ledru [:Sylvestre]]

I thought we were going to write a patch to disable it. I am a bit concerned that we will disable non trivial patch in rc2 without pre release channel testing

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Back out the fix for bug 1300374 from Firefox 51 for now, while we wait for Acid3 to be updated to the spec changes

This is just a straight backout of the C++ parts, plus the failure annotations for the tests; seemed simpler than backing out the tests

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sylvestre,

The patch in question (bug 1300374) basically removed some null-checks.  Disabling it just means putting them back.

This is restoring the behavior Firefox 50 has, as well as the behavior Chrome currently has, so I think the risk is _extremely_ low...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8828022 [details] [diff] [review]
Back out the fix for bug 1300374 from Firefox 51 for now, while we wait for Acid3 to be updated to the spec changes

Review of attachment 8828022 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. The patch could've been a bit more minimal (just touching the two nsCSSRuleProcessor.cpp functions and the test lists), but this is ok too :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

And yeah, I agree that this is really low risk to take.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

OK, thanks. Please land that in m-r asap then :)
(or ping a sheriff)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8828022 [details] [diff] [review]
Back out the fix for bug 1300374 from Firefox 51 for now, while we wait for Acid3 to be updated to the spec changes

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1300374
[User impact if declined]: None for actual user experience.  The main impact
   would be bad press around the Acid3 stuff.
[Is this code covered by automated tests?]:  Yes.
[Has the fix been verified in Nightly?]:  No.  However, this is a straight
   backout which applied cleanly, and restores the Firefox 50 behavior (and the
   current Chrome behavior).
[Needs manual test from QE? If yes, steps to reproduce]: No.  I did check that
   this fixes the Acid3 issue we're worrying about.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: In my opinion, no.  See above about verification.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8828022 [details] [diff] [review]
Back out the fix for bug 1300374 from Firefox 51 for now, while we wait for Acid3 to be updated to the spec changes

51 is now on m-c
By the way, I guess this is impacting fennec too, right?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I assume you mean "m-r".  ;)

Pushed, hopefully to the right place: https://hg.mozilla.org/releases/mozilla-release/rev/1f694d0ba1e8cbfd7d87c27946c95ed326dd85bf

Yes, this affects fennec too.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

yeah, sorry, long day :)

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

I'll be starting the build for fennec and desktop RC2 later today.