Bug 102159 – [chromium] Clamp negative sizes to zero when converting to gfx:: types

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Clamp contentsRect to not have negative sizes

[Dana Jansens]

Created attachment 174038 [details]
Patch

[Dana Jansens]

Created attachment 174040 [details]
Patch

Add bug to the changelog

[Dana Jansens]

Review ping

[James Robinson]

Comment on attachment 174040 [details]
Patch

Sorry, I thought I had already left feedback.  Would it make more sense to do this in the WebFoo -> gfx::Foo conversions in webkit/compositor_bindings?  I worry that trying to this here is going to be fragile to other rects/sizes that end up negative for who-knows-what reasons.

[Dana Jansens]

Sure, I'm good with that. I kind of like that new cases will come to attention easier. But then again it seems like negative sizes in WebCore aren't really a huge problem for us anyways and clamping seems reasonable.

[Dana Jansens]

Created attachment 174310 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

(In reply to comment #4)
> (From update of attachment 174040 [details])
> in webkit/compositor_bindings

do other WebKit API users have this restriction?  If it's a compositor restriction we should do it ^^

[Dana Jansens]

Ohh. Well as soon as you try to make a gfx::Rect out of an IntRect with a negative size, it's going to assert.

Anyone else who uses these methods to convert will suffer the same assertions. So unless you want to leave them with asserts but protect w_c_bindings, I think we should do it here.

[James Robinson]

I see.  If it's a restriction of the gfx:: types then the only question is how loud we want to be when things other than the compositor pass negative values in to here.  How long have the gfx:: types asserted?  If it's been a while, then I think we can be pretty confident that nobody else depends on any particular behavior here.

[Dana Jansens]

They asserted before we came along and started the gtfo thing. Then we removed the assertions to try match webcore behaviour. Now I am putting them back, and there's a few places that we've found trigger them. We've been without them since Fri Sep 28 - chromium a25e25b907d387f0b76a2b8acd522193e777e793

One is contentsRect from GraphicsLayerChromium. One was the thumbLength for scrollbars. And the others were simple things that had cropped up in the meantime.

[WebKit Review Bot]

Comment on attachment 174310 [details]
Patch

Attachment 174310 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14845244

[James Robinson]

Comment on attachment 174310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174310&action=review

OK, thanks for the history.  Seems reasonable except for the build failure.

> Source/Platform/chromium/public/WebFloatRect.h:111
> +        return gfx::RectF(x, y, std::max(0, width), std::max(0, height));

gonna need to use floating point literals or invoke the ::max() override you want with max<float>, i think

[Dana Jansens]

Created attachment 174313 [details]
Patch for landing

[Dana Jansens]

Cool, thanks!

[WebKit Review Bot]

Comment on attachment 174313 [details]
Patch for landing

Clearing flags on attachment: 174313

Committed r134725: <http://trac.webkit.org/changeset/134725>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.