465996 - Small objects that are scaled up don't get invalidated correctly

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Comment 1

[Daniel Holbert [:dholbert]]

STR: 
 1. Install GreaseMonkey ( https://addons.mozilla.org/en-US/firefox/addon/748 )
 2. Install the FakeSmile GreaseMonkey userscript ( http://leunen.d.free.fr/fakesmile/status.html )
 3. Load URL

EXPECTED RESULTS:
 - Animated circle moving across a box.

ACTUAL RESULTS:
 - As the tiny orange circle moves across the box, it leaves a persistent trail of visual artifacts.
 - If you force a repaint by clicking the Firefox toolbar, or clicking into a different window, the existing artifacts are cleared (but new ones keep appearing)

(Note: This issue was originally noted in bug 216462 comment 130, as a potential issue with the SMIL patch.  However, given that this reproduces with the FakeSmile userscript, I think it's a repaint issue in mozilla-central somewhere)

Comment 2

[Daniel Holbert [:dholbert]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre ID:20081120022145

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (requires FakeSmile)

Here's a reduced testcase from the URL.

I think the issue has something to do with the circle's small radius -- if I increase it from 0.05 to 0.08, I get no artifacts.  (And a value of "0.075" shows very tiny artifacts)

I don't think the large transform matters, though, other than to make the testcase more visible.  (I tried removing the transform, and I could still see teeny-tiny artifacts behind the teeny-tiny circle)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: Just SVG and JS (no FakeSmile needed)

This testcase shows the "smeared" artifacts using a fresh profile.  (No FakeSmile / Greasemonkey required.)

Comment 5

[Daniel Holbert [:dholbert]]

This regressed between these nightlies:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081103 Minefield/3.1b2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre

Comment 6

[Daniel Holbert [:dholbert]]

Pushlog for regression range: http://tinyurl.com/5jueta

Comment 7

[Robert Longson [:longsonr]]

Attachment: patch

Cairo seems to calculate the arc extents incorrectly. This applies to both stroke and path extents. Bug 459148 will remove the other Arc call in the SVG code.

canvas uses Arc and might well suffer from some effect due to this bug. Unfortunately canvas actually wants to draw an Arc at that point.

Can anyone get a regression range. It might help to report this to cairo so they can look into it.

Comment 8

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

Jeff, see comment #7.

Comment 9

[Robert Longson [:longsonr]]

Oh you did the regression range already :-) That shows that bug 460946 exposed the underlying cairo problem. Maybe cairo has always had this issue.

Comment 10

[Robert Longson [:longsonr]]

Carl,

cairo_path_extents and cairo_stroke_extents should return xmax - min == ymax - ymin as there is no scaling applied when we calculate the extents of the circular arc segment. The height of the extent is approximately 10% of the correct value in this case.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

cairo_arc is translated into curve_to before the extents are computed so I'm confused how the extents could be miscomputed...

Comment 12

[Jeff Muizelaar [:jrmuizel]]

It's also not necessarily desirable to switch to Ellipse() as it will only use a single Bezier curve instead of as many as needed to keep the error minimized as Arc() does.

Comment 13

[Robert Longson [:longsonr]]

extents are calculated in nsSVGPathGeometryFrame::UpdateCoveredRegion. The bounds from GetUserStrokExtent and GetUserPathExtent seem to be incorrect.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

What are the values being passed in path construction (for Arc() and Ellipse()) and what are the results that you expect to be getting?

Comment 15

[Robert Longson [:longsonr]]

In the testcase the circle has a radius of 0.05. This is what's passed into Arc. It's twice that for Ellipse as you pass in the circle's diameter. x and y vary during the test.

Looking at an example Arc...

x = 0.45, y = 0.45, r = 0.05

GetUserStrokeExtent
(x = 0.394, y = 0.445, width = 0.109, height = 0.0078)

GetUserPathExtent
(x = 0.398, y = 0.449, width = 0.101, height = 0.0000)

Comment 16

[Robert Longson [:longsonr]]

In _cairo_spline_decompose_into I think we're hitting the default tolerance limit which is 0.1. I don't think thebes provides a way to adjust the tolerance at the moment. I'm not sure anyway what we'd adjust the value to.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I've been able to reproduce something like with a small test app. However, I'm surprised that the error makes such a difference. In the test case we're drawing with a scale of 300, when I apply the scale before measuring the extents the error vanishes. Do we measure the extents with the scale factor applied?

Comment 18

[Robert Longson [:longsonr]]

We measure without the scale applied. Otherwise if you had a rotation rather than a scale there would be no way to get the correct extents. That's what bug 460946 fixed.

Comment 19

[Daniel Holbert [:dholbert]]

FWIW, see also related bug 463939 and its testcase (attachment 347196 [details]).  Its symptoms look similar to this bug, though I'm not sure if it has the same underlying root. (It definitely won't be fixed by this bug's patch, because it doesn't use any circles.)

Comment 20

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

I'll check in the patch, but can we have a test here too?

Comment 21

[Robert Longson [:longsonr]]

Attachment: patch with reviewed ellipse change from bug 459148

Comment 22

[Robert Longson [:longsonr]]

Without the ellipse change, ellipses would be affected too. The ellipse change was once part of bug 459148 but it makes more sense to land it as this bug.

Comment 23

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

Hmm, I just pushed the earlier patch to trunk as 1d84189da181.

Comment 24

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

So can you file that ellipse change as a separate patch in another bug? Thanks...

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: attempted reftest patch (doesn't work - passes on unpatched trunk)

(In reply to comment #20)
> I'll check in the patch, but can we have a test here too?

I don't think this bug is reftestable -- the bug is caused by us *not* repainting when we need to, but reftest can't catch that because it forces a full repaint when it takes a snapshot for comparison. (AFAIK)

I tried making a reftest just for the heck of it, but it's not good, because it passes on unpatched trunk right now (even though I can see the paint errors in the reftest window while the page laods), for the reason described above.  Here's the reftest, though, in case anyone can improve upon it.

Maybe this needs a litmus-test?

Comment 26

[Robert Longson [:longsonr]]

It is not reftestable but it is mochitestable. Create an svg file with a really small circle within a <g> that magnifies it and then get its bounds. See the mochitest for bug 460946 which I've yet to fix the rounding errors in but the method is there is sound.

Comment 27

[Robert Longson [:longsonr]]

Daniel are you planning to do a mochitest for this?

Comment 28

[Daniel Holbert [:dholbert]]

Well, I had been intending to make a mochitest based on the one for bug 460946, as mentioned in comment 26, but I couldn't find it.  I looked at the patches attached on the bug and at the pushed patch, and I searched mozilla-central for files including "460946" in the name, but I couldn't find anything.

Maybe I'll let you write it, if you've got that existing mochitest that you can just tweak a bit?  Or, if you point me at where to find that existing mochitest, I'd still be happy to write one.

Comment 29

[Robert Longson [:longsonr]]

Ooops wrong bug number. You want to look at bug 463934

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: mochitest (as patch)

Here's a mochitest.  I've confirmed that it fails pre-patch but passes post-patch. 

The failures pre-patch are:
  not ok - bounds top got 48, expected 23
  not ok - bounds bottom got 53, expected 77
  not ok - bounds height got 5, expected 54
which is consistent with the behavior of testcase 2. (The invalidated region there seems to be missing its top & bottom chunks.)

Robert (Longson), could you take a quick look and confirm that this is what you had in mind?

Comment 31

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #18)
> We measure without the scale applied. Otherwise if you had a rotation rather
> than a scale there would be no way to get the correct extents. That's what bug
> 460946 fixed.

This patch doesn't actually fix the bug, it just gets lucky and avoids triggering it. The ellipse code constructs the circle from four splines instead of the two splines that the arc code uses. Using four splines instead of two acts as more accurate approximation of a circle which causes more accurate extents to be computed.

I believe the correct way to solve this is to apply the transformation matrix before generating the path and then remove the transformation matrix before computing the extents so that the returned extents are in device space.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

Attachment: testcase 3: avoid using circle and use curves directly.

Here's a test case that reproduces the bug using a curved path instead of a circle.

Comment 33

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

(In reply to comment #31)
> I believe the correct way to solve this is to apply the transformation matrix
> before generating the path and then remove the transformation matrix before
> computing the extents so that the returned extents are in device space.

That doesn't work for stroke extents because cairo doesn't fix the stroke width until the extents are computed.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Good point. I don't see a good solution other than adding cairo_(path|stroke)_device_extents() api to cairo. As workarounds we could increase the tolerance, hoping that will help or we could extract the scale from the transformation matrix and compute scaled extents.

Comment 35

[Robert Longson [:longsonr]]

I think we should backout the patch (1d84189da181) then.

Comment 36

[Robert Longson [:longsonr]]

Comment on attachment 351277 [details] [diff] [review]
mochitest (as patch)

This is exactly what I had in mind.  Looks like my solution is flawed so we shouldn't land this though :-(

Comment 37

[Robert Longson [:longsonr]]

Comment on attachment 349263 [details] [diff] [review]
patch

Backed out.

Comment 38

[Damon Sicore (:damons)]

Progress on this?  Who is going to take this one?

Comment 39

[Robert Longson [:longsonr]]

Needs someone to implement the cairo api change per comment 34 I guess.

Comment 40

[Jeff Muizelaar [:jrmuizel]]

I'll try to get the cairo api change in soon...

Comment 41

[Robert Longson [:longsonr]]

You have to have reasonably unusual SVG to see this though. I.e. you have to define something really small, draw it scaled up and then change its location.

Comment 42

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

Assigning to Jeff temporarily to get the cairo patch done.

Comment 43

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

Jeff offers this code as the basis for estimate the stroke extents based on the path extents:

_cairo_stroke_style_max_distance_from_path (const cairo_stroke_style_t *style,
                                            const cairo_matrix_t *ctm,
                                            double *dx, double *dy)
{
    double style_expansion = 0.5;
    
    if (style->line_cap == CAIRO_LINE_CAP_SQUARE)
        style_expansion = M_SQRT1_2;
    
    if (style->line_join == CAIRO_LINE_JOIN_MITER &&
        style_expansion < style->miter_limit)
    {
        style_expansion = style->miter_limit;
    }

    style_expansion *= style->line_width;

    *dx = style_expansion * (fabs (ctm->xx) + fabs (ctm->xy));
    *dy = style_expansion * (fabs (ctm->yy) + fabs (ctm->yx));
}

We should be able to use this to work around the cairo API design deadlock.

Comment 44

[Jonathan Watt [:jwatt]]

Okay, that looks okay as a workaround for now. I'll implement that in SVG land for 1.9.1, but it would be good to have cairo API for getting the tight extents at some point if that operation isn't too expensive. Do we need a separate bug for that?

Comment 45

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #44)
> Okay, that looks okay as a workaround for now. I'll implement that in SVG land
> for 1.9.1, but it would be good to have cairo API for getting the tight extents
> at some point if that operation isn't too expensive. Do we need a separate bug
> for that?

Yeah, I think so. I've added bug 478152 to track it.

Comment 46

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

Just for my own comprehension, here's what happened.

Before bug 460946, we would compute the path's covered area by a) setting up the CTM b) generating the path c) getting cairo's user-space stroke extents and d) converting those to device space. This caused bug 460946 because in step c), cairo would compute the device-space stroke extents, convert them to the user-space bounding box of that device-space rectangle (potentially enlarging it a lot if there's rotation), and then in step d) we'd enlarge the box again when we convert it back to device space and take the bounding-box again.

So we fixed bug 460946 by a) setting the cairo matrix to identity, ignoring the SVG CTM b) generating the path c) getting cairo's user-space stroke extents d) converting those to SVG device space by applying the SVG CTM and taking the bounding box. Unfortunately that triggered this bug, because the SVG user-space coordinates we're using are out of the range that cairo can really deal with as device coordinates.

So what we have to do is basically revert bug 460946 and fix it differently. Ideally we would a) set up the CTM b) generate the path c) set the identity matrix d) get cairo's user-space stroke extents (which are now device space). But step d) breaks because the stroke width is not locked, and changing that behaviour or adding new API for it is not popular in some circles. We could also replace steps c) and d) with "c) get cairo's device-space stroke extents via a new cairo API" but apparently that new API is not popular in other circles.

So until one of those solutions becomes available, we should revert bug 460946 and fix it by a) setting up the CTM b) generating the path c) set the identity matrix d) get the fill extents and e) expanding the fill extents to a conservative estimate of the stroke extents, using the code in comment #43.

Comment 47

[Jeff Muizelaar [:jrmuizel]]

That matches my understanding, thanks for summarizing.

Comment 48

[Jonathan Watt [:jwatt]]

Attachment: patch

So here's what I have ATM. I've not quite fully got my head around the consequences of the change to nsSVGGlyphFrame yet, especially what I should be doing with the SetLineWidth(), if anything.

Comment 49

[Jonathan Watt [:jwatt]]

Hmm, I've at least forgotten the context.IdentityMatrix() in nsSVGGlyphFrame.

Comment 50

[Robert Longson [:longsonr]]

I wrote some mochitests in bug 463934 but they never landed. Although they will fail on non-windows they should be pretty close. If your results are way out you've done something wrong.

+  static gfxRect PathExtentsToMaxStrokeExtents(gfxRect aPathExtents,
+                                               nsSVGGeometryFrame* aElement);

const & for aPathExtents, also why is the second argument called aElement if it is a frame? aFrame or aGeometryFrame would be less confusing.

+   * exploit the fact that cairo does have API for getting the tight device

s/does have API/does have an API/

Comment 51

[Jonathan Watt [:jwatt]]

Attachment: updated wip

Thanks for the tests Robert, they showed up some issues with this approach (and a bug I've still to figure out).

The problem with reusing the logic from _cairo_stroke_style_max_distance_from_path is that we compute stroke bounds that are substantially bigger than the actual stroke bounds. Since the default value for the 'stroke-linejoin' property is 'miter', and the default value for 'stroke-miterlimit' is '4', we end up with:

  double dx = stroke-miterlimit * stroke-width * (fabs(a) + fabs(c));
  double dy = stroke-miterlimit * stroke-width * (fabs(d) + fabs(b));

Let's take the case of a shape that has a modest stroke of say 10% the widest dimension of the shape. Specifically, in the case of a 100 x 100 rect with a 10px stroke, the actual stroke bounds of the rect are 110 x 110, but we would compute it as 180 x 180. That's quite a lot larger.

The impact is not only that we'll invalidate much larger areas than we currently invalidate, but also that getBoundingClientRect() returns almost uselessly large "bounds".

Comment 52

[Jonathan Watt [:jwatt]]

I should have said that I was assuming that the identity matrix for the CTM, so (fabs(a) + fabs(c)) == (fabs(d) + fabs(b)) == 1. Things don't get better when you start using a non-identity transform though.

Comment 53

[Jonathan Watt [:jwatt]]

Attachment: patch using Jeff's suggestion

Okay, I figured out the bug. The problem was that the bounds for text was being calculated as if the transform was being ignored. That's because it was. CharacterIterator::SetupForDirectTextRun calls aContext->SetMatrix(mInitialMatrix), and mInitialMatrix is the identity matrix unless we set it. The stack looks like this:

  gfxContext::SetMatrix
  CharacterIterator::SetupForDirectTextRun
  CharacterIterator::SetupForDirectTextRunMetrics
  nsSVGGlyphFrame::AddBoundingBoxesToPath
  nsSVGGlyphFrame::UpdateCoveredRegion

I've added iter.SetInitialMatrix(tmpCtx) in nsSVGGlyphFrame::UpdateCoveredRegion to fix this.

Comment 54

[Robert Longson [:longsonr]]

In some cases the stroke can be so thin cairo returns no bounds as it would not draw the stroke even though one exists. In that case we have to use the fill extents. You need a testcase with a really thin stroke, something reduced from bug 437704 perhaps.

That's why the current logic does a union of fill/stroke extents.

Comment 55

[Jonathan Watt [:jwatt]]

Jeff, is there no way we could hack our version of cairo for now to expose some temporary API to get the real stroke extents until cairo folks agree how to do it right. I don't actually think that this approach is acceptable, given the oversized bounds problem I explained in comment 51.

Comment 56

[Jonathan Watt [:jwatt]]

Robert: ah, I see. I assumed it was to cover the dashing case explained inline in my comments on the patch. I wish people would add more comments documenting why non-obvious things are the way they are. ;-) Thanks for pointing that out.

Comment 57

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

The problem with adding cairo API locally without also having it upstream is that it means --enable-system-cairo won't work anymore.

I didn't think about how bad it would be to assume the worst about the miter limit... But I think we should take this patch to fix invalidation bugs and then file another bug about getting the correct stroke extents to fix getBoundingClientRect.

Comment 58

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

Comment on attachment 369246 [details] [diff] [review]
patch using Jeff's suggestion

+nsSVGUtils::PathExtentsToMaxStrokeExtents(gfxRect aExtents,

const gfxRect&

+  if (aExtents.Width() == 0 && aExtents.Height() == 0) {

aExtents.IsEmpty()

   }
-
+  
   // Add in markers

Don't add trailing whitespace.

We can have an invalidation reftest here.

Comment 59

[Jonathan Watt [:jwatt]]

I pushed http://hg.mozilla.org/mozilla-central/rev/6d229acfb6d5

(In reply to comment #58)
> +  if (aExtents.Width() == 0 && aExtents.Height() == 0) {
> 
> aExtents.IsEmpty()

It does not have this change. The problem is that <line> elements that are horizontal or vertical have zero height or width respectively for their path extents. Thus extents.IsEmpty() returns true, and if we used that they'd get zero stroke extents too, and as a result would not paint. I did try messing around with code to check for that case, but in the end I think it's just simpler to leave it as it is.

Comment 60

[Jonathan Watt [:jwatt]]

(In reply to comment #57)
> I think we should take this patch to fix invalidation bugs and
> then file another bug about getting the correct stroke extents to fix
> getBoundingClientRect.

A bug other than bug 478152?

Comment 61

[Robert Longson [:longsonr]]

Comment on attachment 351277 [details] [diff] [review]
mochitest (as patch)

Jonathan, you should be able to land this too if you think it's worth it.

Comment 62

[Robert Longson [:longsonr]]

Also I'm pretty sure bug 478152 covers Roc's suggestion.

Comment 63

[Jonathan Watt [:jwatt]]

The problem with the Mochitests is that the values are actually bad (much too big, due to the problem explained in comment 51), but since they're equally bad in both cases, the test passes. Really we should be able to compare against hard values, rather than using the reftest strategy (comparing one object to another) in mochitests. I think that once bug 478152 is fixed, we should do exactly that.

Comment 64

[Jonathan Watt [:jwatt]]

I filed bug 486114 for mochitests.

Comment 65

[Shawn Wilsher :sdwilsh]

This appears to have had a nice win on Tsvg at least on the mac.

http://graphs.mozilla.org/#show=794438,794456,794447&sel=1238329253,1238674853

Comment 66

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: adds M_SQRT_1_2 to the shunt

wince was busted by this push, because its math.h doesn't have M_SQRT_1_2.  Adding it to the shunt fixes that.

Comment 67

[Doug Turner (:dougt)]

Comment on attachment 370204 [details] [diff] [review]
adds M_SQRT_1_2 to the shunt

do you want to add a comment on what that value is? :-) 

something like:

/* 1 / sqrt(2) */

Comment 68

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/7552e5c484bc

Comment 69

[Shawn Wilsher :sdwilsh]

And a win on linux:
http://graphs.mozilla.org/#show=395143,395148,395172&sel=1238334226,1238679826

Comment 70

[Robert Longson [:longsonr]]

(In reply to comment #69)
> http://graphs.mozilla.org/#show=395143,395148,395172&sel=1238334226,1238679826

That's worrying as this should have increased the size of the covered regions we calculate and therefore reduced performance at the expense of correctness.

Comment 71

[Robert Longson [:longsonr]]

s/correctness/ensuring the regions are sufficiently sized not to get artifacts/

Comment 72

[Jonathan Watt [:jwatt]]

(In reply to comment #65)
> This appears to have had a nice win on Tsvg at least on the mac.

That's because we're kinda cheating by no longer asking cairo for stroke bounds. In the long run we're going to need to do that, so the perf improvement will only be temporary.

(In reply to comment #70)
> That's worrying as this should have increased the size of the covered regions
> we calculate and therefore reduced performance at the expense of correctness.

The problem is that the calculations to get the covered region (asking cairo to get fill and stroke extents) are not cheap. :-(

Comment 73

[Jonathan Watt [:jwatt]]

BTW, to be clear, the tests under Tsvg do not test invalidation. Computing covered regions is up front overhead we accept specifically to save cycles when doing invalidation. In pure invalidation tests I'm pretty confident we'd find that the change introduced to fix this bug would actually result in a perf regression.

Comment 74

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

It's perfectly OK to compute a loose estimate of the bounds for invalidation if that's an overall performance win. We should test it, but it's quite plausible to me that it would be a win even on invalidation-heavy pages. The overestimate of the stroke bounds is proportional to the stroke width, which I suspect that most content uses stroke widths that are reasonably small compared to the size of the path being stroked (or else they're not using mitred joins).

Comment 75

[[e-mail]]

(In reply to comment #66)
> wince was busted by this push, because its math.h doesn't have M_SQRT_1_2. 

VC7.1 doesn't have it by default, but you can enable it with _USE_MATH_DEFINES

Comment 76

[[e-mail]]

Attachment: VC7.1 bustage fix

Comment 77

[Robert Longson [:longsonr]]

(In reply to comment #76)
> Created an attachment (id=370591) [details]
> VC7.1 bustage fix

That line is in nsSVGUtils.h. Do we just need to move the nsSVGUtils.h #include in nsSVGUtils.cpp up to the top?

Comment 78

[[e-mail]]

Attachment: Better VC7.1 bustage fix

Thanks for pointing that out!

Comment 79

[Robert Longson [:longsonr]]

(In reply to comment #78)
> Created an attachment (id=370600) [details]
> Better VC7.1 bustage fix
> 

I think that's the wrong attachment. It's the same as the last one. 

Also you don't need an sr for something this simple (bug 485544 comment 4).

Comment 80

[[e-mail]]

Attachment: Correct VC7.1 bustage fix

Oops, I mistyped the diff command, and created a new file by mistake...

Comment 81

[Robert Longson [:longsonr]]

Comment on attachment 370602 [details] [diff] [review]
Correct VC7.1 bustage fix

Maybe change the comment in nsSVGUtils.cpp to
// include nsSVGUtils.h early to ensure that the definition of M_SQRT1_2 is picked up.

r=longsonr whether you update the comment or not.

Comment 82

[Jonathan Watt [:jwatt]]

(In reply to comment #79)
> Also you don't need an sr for something this simple (bug 485544 comment 4).

That's from http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/ff56bb7ba88dab52/b27d9b0565c382c3 rather than something I decided BTW. ;-)

Comment 83

[[e-mail]]

Comment on attachment 370602 [details] [diff] [review]
Correct VC7.1 bustage fix

I pushed this (with a slightly different comment) as changeset 212f7ae2a298 to mozilla-central claiming sr=roc since he had plussed the previous iteration.

Comment 84

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5fd4fe17ca1c (including WinCE and VC7.1 fixes).