457324 - More Tests for CSS Transform Support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[cmtalbert]

Attachment: Patch with tests - note it's got an image in it use hg import to apply

This bug adds more tests for the CSS Transform support, based off the testplan here: https://wiki.mozilla.org/QA/Firefox3.1/CSS_Transform_Testplan

Feel free to suggest more tests to add.

While writing these, I found two things that I believe are bugs:
* scale does not allow for percentage inputs.  I think this is a problem in the specification (since it states that scale takes type <number>).  But, I believe that most people are used to thinking of scale in terms of percent.  Do we know if this issue has been raised with the spec authors?

* When performing nearly infinite skews, I'm getting weird translation matrices.  I would assume that tan(Math.PI/2) would equal to NaN and therefore would cause us to use "none" as the matrix type.  Instead, for the rule: -moz-transform: skew(300grad, (Math.PI/2)rad), we get the following transformation matrix applied: matrix(1, -10000, 10000, 1, 0px, 0px);  Is this expected?  I've marked this and a similar issue in test_bug453293-skew.html as "todo" until we figure out where to go with those.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> * When performing nearly infinite skews, I'm getting weird translation
> matrices.
> ...
> matrix(1, -10000, 10000, 1, 0px, 0px)

IIRC, Keith capped certain matrix entries to prevent them from blowing up into ridiculously huge numbers (and NaNs).  Would that explain what you're seeing here?

Comment 2

[cmtalbert]

yeah, probably so.  I'll change the tests to expect that then.

Comment 3

[cmtalbert]

Attachment: Patch fixing the "infinite" case to use the upper bound of 10000 in the matrix

Tests for CSS Transform feature.  Daniel, do you know about whether the % scaling issue was brought up with the specification?  That isn't our bug, as we're just implementing what they have.  I'll have to look for the spec's mailing list...

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Daniel, do you know about whether the %
> scaling issue was brought up with the specification?

I don't know.  What does WebKit do with those sorts of situations?

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 342347 [details] [diff] [review]
Patch fixing the "infinite" case to use the upper bound of 10000 in the matrix

>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=435293">Mozilla Bug </a>

I haven't written many mochitests, but I think you're supposed to fill in the bug # there (s/Mozilla Bug /Mozilla Bug 435293/). (There are a few different places where you need that)

>--- a/layout/reftests/transform/reftest.list
>+++ b/layout/reftests/transform/reftest.list
>@@ -87,3 +87,17 @@
>+# ensure matrix 3d does not break us - should do nothing
>+== matrix3d.html matrix3d-ref.html

These (and the rest of the added reftests) should be named matrix3d-1.html and matrix3d-1-ref.html, to match the general reftest convention, which has so far been followed in the transform folder.

>+== scale.html scale-ref.html
>+== scale-1b.html scale-ref.html

These should be scale-1a.html, scale-1b.html, and scale-1-ref.html

>+          function animate() {
>            [snip]
>+              setTimeout(animate, 50);
>+            }
>+          }
>+          animate();
>+          iframe.setAttribute("class", "untransform");
>+          document.documentElement.className = "";
>+        }

Why do you have the "setTimeout(animate, 50)" there?  Aren't those going to end up being triggered _after_ you remove the reftest-wait class?  That seems bad, but maybe I'm missing something.

Comment 6

[cmtalbert]

Attachment: Addresses comments

Addresses Review comments.  Also, got rid of that 50ms timeout, which was unneeded.  I think it was a hold over from when I slowed the animate step down in testing to make sure it was working (before putting it into a full reftest).

Good catch, thanks.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 344277 [details] [diff] [review]
Addresses comments

In a few files, you have:
>+<html>
>+<head/>
>+<body>

The <head/> doesn't buy you anything there, and it's actually not valid HTML (though it would be valid XHTML).  The trailing "/" is ignored, so technically you've got a <body> within a <head> there.

I'd just get rid of these <head/> tags, since there's no reason to have them.

Related:
>+    <img src="square.png"/>

You have a trailing slash there in some files and not in others.  Again, the slash doesn't add anything, and it's ignored by HTML, so I'd just get rid of it. (or at least put a space before it to make it more HTML-friendly per guideline C2 at http://www.w3.org/TR/xhtml1/#guidelines)

>++ b/layout/reftests/transform/scale-1-ref.html
> [snip]
>+  <div style="background: green; width: 50px; height: 50px; margin-top: 33px; margin-left: 25px"
>+       id="test">

Why is there a margin-top and margin-left on the reference case, but not on the text cases?  Does that test actually pass?  It doesn't look like it should...  If for some reason you need a hard-coded margin there in the reference case, though, I'd include an HTML comment explaining why, 'cause it's not obvious.

> +++ b/layout/reftests/transform/scale-percent-1-ref.html
> [snip]
>+  <div style="background: green; width: 100px; height: 100px" id="test">

I noticed you've got a number of testcases where foo-1.html uses style declared in a <style> block but foo-1-ref.html has inline style. (e.g. in scalex-*, scaley-*, scale-*, scale-percent-*, maybe others)

Where possible & where it makes sense, I'd lean towards structuring the reference case & testcase as similarly as possible (in this case, having them both use a <style> block), so that the only difference between the files is the line(s) containing the specific style declaration that you're testing.  This makes it a lot easier to see & verify what's being tested, by inspection and by diffing the testcase vs the reference case.

i.e. I'd change scale-percent-1-ref.html to look exactly like scale-percent-1.html, minus the -moz-transform declaration and with the width/height changed.

>+++ b/layout/reftests/transform/scale-percent-1-ref.html
>+++ b/layout/reftests/transform/scale-percent-1.html

It looks like scale-percent-1.html and scale-percent-1-ref.html both have height/width=100px.  The testcase has scaling applied as well, so I don't think it'd currently pass.  Is that a typo?

Also, it looks like scale-percent-1.html uses 3 different scaling methods, in separate -moz-transform declarations -- scale(50%, 50%), scalex(50%), and scaley(50%).  I'm guessing that's a typo -- also, only the final declaration (scaley) would have any effect, right? (see related comments relating to "stresstest" below)

> +++ b/layout/reftests/transform/stresstest-1.html

I don't think this test does what you think it does, and I'd probably just get rid of it.

It looks like you're trying to test combinations of multiple -moz-transform effects, but really only one declaration is active at a time, by the rules of CSS, AFAIK / AFAICT.  This means:
    a) When you set the class to "transform", we only use the *last* declaration of that class -- namely, the "scale(2.0, 4.0);" -- and that overrides all earlier "-moz-transform" declarations.  I think everything above it is literally ignored by the CSS parser.
    b) In "animate()", when you set an element-specific MozTransform style on the element, that overrides all class transforms, because the element's style is the most specific.  So the "transform" and "untransform" classes have absolutely no effect from that point on.
    c) It looks like your "untransform" class tries to exactly undo each of the "transform" effects, but that's incorrect / unnecessary, becase
      * when you switch classes, the "untransform" style would *replace* the "transform" style, rather than "counteracting" it.
      * as mentioned in (a), only the final declaration in "untransform" would actually be applied, anyway -- everything else there is skipped.
      * as mentioned in (b), the element-specific style added in "animate()" takes precidence over any class-specific style, anyway, so actually, no "untransform" style gets to have any effect at all.

So really, the only things being tested here are:
  (1) that "scale(2.0, 4.0)" doesn't crash/hang anything (when we initially apply the "tranform" class)
  (2) that setting MozTransform= "rotate(45deg)" thru "rotate(360deg)" doesn't crash/hang anything
  (3) that "rotate(360deg)" ends up with a normal iframe
AFAIK, all other styles in the test are completely ignored by the CSS engine.


If you want to _actually_ stress-test the effects of stacking multiple transforms, you should be able to use one "-moz-transform: [long list of transform functions]" declaration, instead of a list of individual "-moz-transform: [function]" declarations.

To get the "untransform" effect that it looks like you were going for, you'd want to *append* the "untransform" effects to that transform-function-list. using JS.  Or you could probably use two nested elements, one with a list of transforms and one with a list of un-transforms.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> Why is there a margin-top and margin-left on the reference case, but not on the
> text cases?
er, "test cases"

Comment 9

[cmtalbert]

(In reply to comment #7)
> (From update of attachment 344277 [details] [diff] [review])
> In a few files, you have:
> >+<html>
> >+<head/>
> >+<body>
> 
Fixed this.  Sorry.
> Related:
> >+    <img src="square.png"/>
> 
> You have a trailing slash there in some files and not in others.  Again, the
> slash doesn't add anything, and it's ignored by HTML, so I'd just get rid of
> it. (or at least put a space before it to make it more HTML-friendly per
> guideline C2 at http://www.w3.org/TR/xhtml1/#guidelines)
Yeah, I just removed the image entirely, deciding it was really unnecessary 
> 
> >++ b/layout/reftests/transform/scale-1-ref.html
> > [snip]
> >+  <div style="background: green; width: 50px; height: 50px; margin-top: 33px; margin-left: 25px"
> >+       id="test">
> 
> Why is there a margin-top and margin-left on the reference case, but not on the
> text cases?  Does that test actually pass?  
Yes, it does, though it was an error to use margin there, as you note.  For some reason, the scale in the x direction is shrinking it toward the right edge, the scale in y is shrinking toward the bottom edge.  I can't figure out what the pattern here is or where it is calculating these values from.  I got the tests to pass by simple trial and error, but I can't explain where the 33px offset is coming from.  Do you think this is a bug, or is there something I'm missing?

> > +++ b/layout/reftests/transform/scale-percent-1-ref.html
> > [snip]
> 
> I noticed you've got a number of testcases where foo-1.html uses style declared
> in a <style> block but foo-1-ref.html has inline style. (e.g. in scalex-*,
> scaley-*, scale-*, scale-percent-*, maybe others)
> 
> Where possible & where it makes sense, I'd lean towards structuring the
> reference case & testcase as similarly as possible (in this case, having them
> both use a <style> block), so that the only difference between the files is the
> line(s) containing the specific style declaration that you're testing.  This
> makes it a lot easier to see & verify what's being tested, by inspection and by
> diffing the testcase vs the reference case.
Good suggestion.  I've changed these and I'll do that from now on.
 

> >+++ b/layout/reftests/transform/scale-percent-1-ref.html
> >+++ b/layout/reftests/transform/scale-percent-1.html
> 
> It looks like scale-percent-1.html and scale-percent-1-ref.html both have
> height/width=100px.  The testcase has scaling applied as well, so I don't think
> it'd currently pass.  Is that a typo?
Nope, scale transforms do not accept percent values, so this is a negative test, and it shouldn't do anything.  I added a comment to make that clearer.
> 
> Also, it looks like scale-percent-1.html uses 3 different scaling methods, in
> separate -moz-transform declarations -- scale(50%, 50%), scalex(50%), and
> scaley(50%).  I'm guessing that's a typo -- also, only the final declaration
> (scaley) would have any effect, right? (see related comments relating to
> "stresstest" below)
yeah, typo, fixed.

> > +++ b/layout/reftests/transform/stresstest-1.html
> > 
> It looks like you're trying to test combinations of multiple -moz-transform
> effects, but really only one declaration is active at a time, by the rules of
> CSS, AFAIK / AFAICT.  This means:
>     a) When you set the class to "transform", we only use the *last*
> declaration of that class -- namely, the "scale(2.0, 4.0);" -- and that
> overrides all earlier "-moz-transform" declarations.  I think everything above
> it is literally ignored by the CSS parser.
You're exactly right.  For some reason I thought that CSS Transforms were different.

> If you want to _actually_ stress-test the effects of stacking multiple
> transforms, you should be able to use one "-moz-transform: [long list of
> transform functions]" declaration, instead of a list of individual
> "-moz-transform: [function]" declarations.
> 
> To get the "untransform" effect that it looks like you were going for, you'd
> want to *append* the "untransform" effects to that transform-function-list.
> using JS.  Or you could probably use two nested elements, one with a list of
> transforms and one with a list of un-transforms.
I've adapted this test to do this now.

I also added a test to ensure you can't translate your way out of an iframe, and another simple selector test.

Patch forthcoming.

Comment 10

[cmtalbert]

Attachment: Patch rev3

Take 3.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 354909 [details] [diff] [review]
Patch rev3

>+++ b/layout/base/tests/Makefile.in

The changes to Makefile.in don't apply cleanly on up-to-date mozilla-central anymore (there's bitrot in the contextual lines -- probably due to my delay in reviewing this -- sorry :)).

>+++ b/layout/reftests/transform/reftest.list
>+== child-1a.html child-1-ref.html
>+== child-2a.html child-2-ref.html

These files (child-*) aren't included in the patch.  They either need to be added to the patch, or removed from reftest.list. (If you add them to the patch, I'd suggest also removing the "a" from "child-1a" and "child-2a", since there's no "b" version.)

>+++ b/layout/base/tests/test_bug435293-skew.html
>+    /* We'll use closer to the real numbers for this, but keeping them up here
>+       for documentation purposes
>+    #test4 {
>+      -moz-transform: skew(2(PI)rad, 45deg);
>+    }
>+    #test5 {
>+      -moz-transform: skew(PI/4rad, 150grad);
>+    }*/

This comment is a bit unclear -- I initially read it as "[In the future] we'll use...", like as a TODO.  I'd explicitly mention JS, to avoid any such confusion -- e.g. "The style for #test4 & #test5 is set via JS to get more exact values. I'm also noting their style inside in this comment, just for clarity."

You should probably also include a similar chunk for #test11 (at least declaring its style inside of a comment) to be consistent.

>+  is(style.getPropertyValue("-moz-transform"), "matrix(1, 0, 0.57735, 1, 0px, 0px)",
>+     "Test1: Skewx proper matrix is applied");
>+
>+  style = window.getComputedStyle(document.getElementById("test2"), "");
>+  is(style.getPropertyValue("-moz-transform"), "matrix(1, 1.73205, 0, 1, 0px, 0px)",
>+      "Test2: Skewy proper matrix is applied");

Are these decimal values (0.57735, 1.73205) exact, or are they just rounded to 5 decimal places?  And if they're rounded, then isn't there potential for precision errors here?

Same goes for a few other decimal values later on in that test.

>+</html>
>+

All three mochitests have an extra blank line at the end -- nix those.

>+++ b/layout/base/tests/test_bug435293-scale.html
[snip]
>+  is(style.getPropertyValue("-moz-transform"), "matrix(0.5, 0, 0, 1, 0px, 0px)",
[snip]
>+  is (style.getPropertyValue("-moz-transform"), "matrix(1, 0, 0, 0.5, 0px, 0px)",

Get rid of the space between "is" and "(", for consistency. That happens a number of times in the "scale" mochitest.

>+++ b/layout/reftests/transform/descendant-1-ref.html

This reference case has some inline style and some style-block style, whereas the testcase (descendant-1.html) has it all inside the style block.

I'd put the reference case's style all inside its style block, to match the testcase.  (per the  bit of comment 7 about structuring reference case & testcase similarly for easy diffing to verify what's being tested)

>+++ b/layout/reftests/transform/scale-1-ref.html
>+      position: absolute;
>+      top: 33px;
>+      left: 33px;

>+++ b/layout/reftests/transform/scalex-1-ref.html
>+      position: absolute;
>+      left: 33px;

>+++ b/layout/reftests/transform/scaley-1-ref.html
>+      position: absolute;
>+      top: 33px; 

(In reply to comment #9)
> I can't figure out
> what the pattern here is or where it is calculating these values from.  I got
> the tests to pass by simple trial and error, but I can't explain where the 33px
> offset is coming from.  Do you think this is a bug, or is there something I'm
> missing?

I looked into this a bit -- the "pseudo-margins" appear in the scaled versions because the transform is centered at the _center_ of the div, and so it leaves empty whitespace on all sides.  In this case, it leaves _exactly 25px_ of whitespace on each side (because we've shrunk by 50px in both dimensions).  However, the body of the document also has the default "margin: 8px".  So we end up with a _total_ of 33px of whitespace.

I'd fix this / clean this up as follows:
 - In all scale-*, scalex-*, scaley-* testcases: add "body { margin: 0px; }"
 - In scale-1-ref.html: Replace asbsolute positioning with a single "margin: 25px".  
 - In scalex-1-ref.html: Replace asbsolute positioning with a single "margin-left: 25px".
 - In scaley-1-ref.html: Replace asbsolute positioning with a single "margin-top: 25px".

>+++ b/layout/reftests/transform/scale-percent-1.html
>+  <!-- Percent Values are not allowed for scale, this doesn't do anything -->

s/Values/values/
s/this doesn't/so this shouldn't/

>+++ b/layout/reftests/transform/stresstest-1-ref.html
>+<html>
>+  <head/>

Get rid of this <head/>, per the first chunk of comment 7.

>+++ b/layout/reftests/transform/stresstest-1.html
>@@ -0,0 +1,32 @@
>+<html class="reftest-wait">
>+  <head>
>+    <style>
>+        .untransform {

You don't use the "untransform" style anymore.  You can nix the whole <style> block, in fact.

>+          style = style + " scale(0.5, 0.25) translatex(-50px) rotate(-45deg) translate(-50px, -50px) skewx(135deg);"

I'd add a comment above this line, saying something like:
// Here, we add transform functions to explicitly undo the effects of
// each already-applied transform.  This should leave us with an
// effectively-untransformed iframe.

Comment 12

[cmtalbert]

Attachment: Patch v4

(In reply to comment #11)
Took all your suggestions, just a few things to comment on.

> (From update of attachment 354909 [details] [diff] [review])
> >+++ b/layout/base/tests/Makefile.in
> 
> The changes to Makefile.in don't apply cleanly on up-to-date mozilla-central
> anymore (there's bitrot in the contextual lines -- probably due to my delay in
> reviewing this -- sorry :)).
No worries.
> Are these decimal values (0.57735, 1.73205) exact, or are they just rounded to
> 5 decimal places?  And if they're rounded, then isn't there potential for
> precision errors here?
> 
I imagine that they are rounded, but they are rounded by the underlying subsystem of the CSS Transform interface.  I pulled these numbers directly from the DOM Explorer when developing the tests, and have tested them on all three platforms.  They always report the same way.

> >+++ b/layout/reftests/transform/descendant-1-ref.html
> 
> This reference case has some inline style and some style-block style, whereas
> the testcase (descendant-1.html) has it all inside the style block.
> 
> I'd put the reference case's style all inside its style block, to match the
> testcase.  (per the  bit of comment 7 about structuring reference case &
> testcase similarly for easy diffing to verify what's being tested)
Doh! I thought I got all of those. Sorry.

> I looked into this a bit -- the "pseudo-margins" appear in the scaled versions
> because the transform is centered at the _center_ of the div, and so it leaves
> empty whitespace on all sides.  In this case, it leaves _exactly 25px_ of
> whitespace on each side (because we've shrunk by 50px in both dimensions). 
> However, the body of the document also has the default "margin: 8px".  So we
> end up with a _total_ of 33px of whitespace.
Ah, makes so much sense.  Thanks!  

Thanks for the reviews.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 357097 [details] [diff] [review]
Patch v4

>+++ b/layout/reftests/transform/iframe-1-ref.html
>+++ b/layout/reftests/transform/iframe-1.html
>+++ b/layout/reftests/transform/iframe-transform.html
>\ No newline at end of file

These three files each need a single newline added at the end.

>+++ b/layout/reftests/transform/iframe-transform.html
>+        -moz-transform: transform(500px, 500px);

I think you mean "transLATE(500px, 500px);" (not transFORM), right?

Also, that file abruptly ends in the middle of the style block, with no closing tags or body, so I'm not sure it tests anything right now (because of the lack of a transformed div).

You probably want to add (a) closing tags and (b) and a body that includes a div to be transformed.

(In reply to comment #12)
> > Are these decimal values (0.57735, 1.73205) exact, or are they just rounded
>
> I imagine that they are rounded, but they are rounded by the underlying
> subsystem of the CSS Transform interface.  I pulled these numbers directly from
> the DOM Explorer when developing the tests, and have tested them on all three
> platforms.  They always report the same way.

That sounds pretty brittle to me.... I don't think our tests should rely on the exact point at which we round off long decimal values.  If we increase our level of precision in the future, or if we find & fix a rounding bug in the transform code, then these tests would break & need to be re-generated (and that's bad).

I don't think I'd be comfortable with this sort of test unless we change its transforms such that they correspond to exact-decimal values that we can explicitly specify (e.g. "0.125").  If that's not possible (or too tricky), I think we should probably nix those few tests.

If you disagree, see what dbaron or roc think -- I'll gladly concede on this point, if either of them says that the rounded decimals are ok.

r=dholbert, with the above comments addressed.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> I don't think I'd be comfortable with this sort of test unless we change its
> transforms such that they correspond to exact-decimal values that we can
> explicitly specify (e.g. "0.125").

Actually, I'm dumb -- I don't think it's possible to get exact decimal values for non-90-degree-increment rotation & skew transforms, because the transform matrix will have cosine & sine values, which will involve square-roots even for "nice" angles like 30/45/60.

So, I think I may be ok with using these rounded decimal values after all, though I want to briefly ping dbaron about it when he logs in, to get his thoughts on this.

Comment 15

[Daniel Holbert [:dholbert]]

Just talked to dbaron on IRC, and he thinks the rounded decimal values are ok -- we just need to remember to fix this test if we ever change the precision level in the transform-matrix computed-style.

So, r=dholbert with the first half of comment 13 addressed. (iframe-*.html stuff)

Comment 16

[cmtalbert]

(In reply to comment #15)
> Just talked to dbaron on IRC, and he thinks the rounded decimal values are ok
> -- we just need to remember to fix this test if we ever change the precision
> level in the transform-matrix computed-style.
> 
> So, r=dholbert with the first half of comment 13 addressed. (iframe-*.html
> stuff)
Ha, just collided with your comment.  Here was my text:
(In reply to comment #13)
> (From update of attachment 357097 [details] [diff] [review])
> >+++ b/layout/reftests/transform/iframe-1-ref.html
> >+++ b/layout/reftests/transform/iframe-1.html
> >+++ b/layout/reftests/transform/iframe-transform.html
> >\ No newline at end of file
> 
> These three files each need a single newline added at the end.
> 
oops, sorry.


> >+++ b/layout/reftests/transform/iframe-transform.html
> >+        -moz-transform: transform(500px, 500px);
> 
> I think you mean "transLATE(500px, 500px);" (not transFORM), right?
> 
> Also, that file abruptly ends in the middle of the style block, with no closing
> tags or body, so I'm not sure it tests anything right now (because of the lack
> of a transformed div).
>
WTF.  I think I may have attached the wrong patch. Sorry, I'll correct this.

(In reply to comment #14)
> Actually, I'm dumb -- I don't think it's possible to get exact decimal values
> for non-90-degree-increment rotation & skew transforms, because the transform
> matrix will have cosine & sine values, which will involve square-roots even 
> for "nice" angles like 30/45/60.
You're not dumb, you're walking the same thought pattern I did when I first created these tests.  This was the only way I could figure out how to meaningfully test the skew transform.  I think the values are ok, although they will be brittle w.r.t. to changes in the transform code.  I look forward to hearing dbaron's thoughts on it.  And now I have. :-)

New patch in a few moments.

Comment 17

[cmtalbert]

Attachment: Patch addressing last comments

Final patch addressing comments, carrying dholbert's review forward.

Comment 18

[cmtalbert]

Checked in, though quite possibly without a proper check in comment.

changeset:   23813:ef6e2a5d9f5f
tag:         tip
user:        Clint Talbert <[e-mail]>
date:        Fri Jan 16 12:52:24 2009 -0600
summary:     imported patch bug435293-csstransform-tests

--> FIXED

Thanks dholbert for all your help with this.

Comment 19

[Daniel Holbert [:dholbert]]

Two follow-up notes here:
  #1: This bug's landing initially seemed to have caused leaks on Windows, so Clint filed follow-up bug 474016 and disabled some mochitests for now in the changeset http://hg.mozilla.org/mozilla-central/rev/786c7e01d148
(However, per bug 474016 comment 3 & bug 474016 comment 4, it looks like the leakage was just an unfortunately-timed sporadic problem, not caused by these mochitests after all.)

  #2: As mentioned in bug 474016 comment 1 and bug 474016 comment 4, there did end up being some rounding error in one of this bug's mochitests on Linux (e.g. in this log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232132249.1232135604.4872.gz ) I think Clint is going to address this by allowing a margin of error in the comparisons.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So is this ready to land on 1.9.1 or not?

Comment 21

[Daniel Holbert [:dholbert]]

AFAIK the rounding error from bug 474016 is still an issue, but this should be fine to land in the same state that it is currently on trunk, with that mochitest disabled.

Comment 22

[cmtalbert]

I'm going to check in the patch for 474016 this morning, after that we should be able to land on 1.9.1. (I don't think tests have the same bake requirement do they?)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to comment #22)
> (I don't think tests have the same bake requirement
> do they?)

Correct -- tests are generally fine to land on branches, as long as it's expected that they should pass.

I'd suggest holding off on the 1.9.1 landing until you've gotten a green Linux unittest cycle for mozilla-central+474016, just to make sure the rounding issue is fixed once and for all. :)

Comment 24

[cmtalbert]

(In reply to comment #23)
> (In reply to comment #22)
> > (I don't think tests have the same bake requirement
> > do they?)
> 
> Correct -- tests are generally fine to land on branches, as long as it's
> expected that they should pass.
> 
> I'd suggest holding off on the 1.9.1 landing until you've gotten a green Linux
> unittest cycle for mozilla-central+474016, just to make sure the rounding issue
> is fixed once and for all. :)
That's what I figured.  Landed 4c9e6fadfc7a on mozilla-1.9.1.