David Baron :dbaron: Reporter
	Description • 17 years ago

Borders on elements taller than 2^15 (32768) pixels are drawn backwards.

Steps to reproduce: load attached testcase

Expected results: green border around edges of (very tall) page

Actual results: top border at top of page, bottom border at bottom of page, side borders point off the edge of the page instead of connecting the top and bottom borders.

Works correctly in: Linux nightly 2007-04-30-04-trunk
Broken in: Linux nightly 2007-05-01-04-trunk
Presumably regression from bug 368247.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Assignee
	Comment 2 • 17 years ago

Ok, condition the border rects -- this might cause "swim" for really large dashed/dotted borders though, but I think I know how to fix that as well in a later patch.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Assignee
	Comment 3 • 17 years ago

Whoops, forgot to do the same thing to PaintOutline.  This also fixes bug 366752.

	David Baron :dbaron: Reporter
	Comment 4 • 17 years ago

Comment on attachment 264530 [details] [diff] [review]
handle overflow, v2

>+    // rounding to within cairo's max dimensions
>+    // returns false if the rect is entirely out of bounds; true otherwise.

I don't think that's the behavior you want; if you need to condition things like backgrounds, you'll want to finish the conditioning rather than short-circuiting and returning false.  I think this should return void, and should condition all 4 edges in.

>+#define CAIRO_COORD_MAX (16384.0)
>+#define CAIRO_COORD_MIN (-16384.0)

Sure the max isn't 16383?  You could even be safe and do -16383 to 16382, unless you know otherwise.

>-#undef DEBUG_NEW_BORDERS
>+#define DEBUG_NEW_BORDERS

You don't want that.

Maybe put it in DEBUG_vlad so you don't keep getting it in diffs?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Assignee
	Comment 5 • 17 years ago

(In reply to comment #4)
> (From update of attachment 264530 [details] [diff] [review])
> >+    // rounding to within cairo's max dimensions
> >+    // returns false if the rect is entirely out of bounds; true otherwise.
> 
> I don't think that's the behavior you want; if you need to condition things
> like backgrounds, you'll want to finish the conditioning rather than
> short-circuiting and returning false.  I think this should return void, and
> should condition all 4 edges in.

Well, if you're conditioning a rect that's entirely out of bounds, then you can't do anything to make anything appear in that rect -- so I'm hoping that callers can just bail out early if it returns FALSE (because the rect is totally outside of, and doesn't cover any of, the 16-bit coordinate space).

The only conditioning I can return in that case is (0,0,0,0), no?

> >+#define CAIRO_COORD_MAX (16384.0)
> >+#define CAIRO_COORD_MIN (-16384.0)
> 
> Sure the max isn't 16383?  You could even be safe and do -16383 to 16382,
> unless you know otherwise.

16384 is what we're currently using as the upper bound in the old/current conditioning code; I can do -16383..16382 -- I think as long as it's over 16k it should be ok.
 
> >-#undef DEBUG_NEW_BORDERS
> >+#define DEBUG_NEW_BORDERS
> 
> You don't want that.
> 
> Maybe put it in DEBUG_vlad so you don't keep getting it in diffs?

Argh, yes; I don't have it in a DEBUG_vladimir chunk because I don't keep it on all that often.  I just keep forgetting to flip it before generating patches.

	David Baron :dbaron: Reporter
	Comment 6 • 17 years ago

Er, sorry, was mixing up the cases where the two sides of the rect were out of bounds on opposite sides or out of bounds on the same side.  It's probably worth distinguishing them more clearly in the comment.

Still, you probably should condition the rects that are out of bounds on the same side to be 0-width rects at the edge of the bounds, so they don't accidentally show up in the middle of the screen if they happen to be the right position mod 32768.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Assignee
	Comment 7 • 17 years ago

Updated patch.  The rects are always conditioned now, and true/false is returned based on whether the rect is zero-sized in either axis.

	David Baron :dbaron: Reporter
	Comment 8 • 17 years ago

Comment on attachment 265026 [details] [diff] [review]
handle overflow, v3

>+/* Clamp r to (-16384,-16384) (16384,16384);

You should fix this comment to match the code.

>+    return (size.height != 0.0 && size.width != 0.0);

If this is what you're returning, why not just have the callers who care check rect.IsEmpty() ?

(I'd note that this returns false when conditioning a rect that's already empty, even if it wasn't offscreen.  You could fix that by setting a variable to true at the beginning and setting it to false in the 4 places it becomes empty.  But why bother if the callers just want to check IsEmpty?)

I'd also note the order of condition is a little odd.  There are three parts (call them A, B, and C) for both x and y coordinates.  You're doing Ax Ay Bx Cx By Cy; it seems like picking a major/minor sort might be more readable.

r=dbaron either way

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Assignee
	Comment 9 • 17 years ago

Checked in with suggested fixes.  (Returns void, comment says to use IsEmpty).