[chromium] Clamp contentsRect to not have negative sizes
Created attachment 174038 [details] Patch
Created attachment 174040 [details] Patch Add bug to the changelog
Review ping
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.
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.
Created attachment 174310 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
(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 ^^
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.
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.
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.
Comment on attachment 174310 [details] Patch Attachment 174310 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14845244
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
Created attachment 174313 [details] Patch for landing
Cool, thanks!
Comment on attachment 174313 [details] Patch for landing Clearing flags on attachment: 174313 Committed r134725: <http://trac.webkit.org/changeset/134725>
All reviewed patches have been landed. Closing bug.