ui: Remove gfx::Size::ClampToNonNegative, prevent negative sizes always.

ui: Remove gfx::Size::ClampToNonNegative, prevent negative sizes always.

This was added with the intention of using Size as a vector, replacing use of
IntSize. Since we have Vector2d now, negative sizes should not exist, so clamp
them in set_width/set_height and the constructor.

Not covered by tests, as we can't test DCHECKs.

TBR=sky
BUG=160158
Relanding: https://codereview.chromium.org/11365160/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167255

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175535

[danakj, 2012-11-10 06:23:09]

Apparently there's already some aura unit tests that create negative sizes.

ash_unittests:
[ RUN      ] ToplevelWindowEventHandlerTest.Caption
[7000:5772:1109/221237:1509886:FATAL:size_base_impl.h(19)] Check failed: !(width
< 0).

aura_unittests:
[ RUN      ] RootWindowTest.TouchEventsOutsideBounds
[6544:6700:1109/221221:1494364:FATAL:size_base_impl.h(19)] Check failed: !(width
< 0)

views_unittests:
[ RUN      ] NativeWidgetAuraTest.DontCaptureOnGesture
[4212:5616:1109/221219:1492336:FATAL:size_base_impl.h(19)] Check failed: !(width
< 0). 

Is this DCHECK still something you want to have sky? I'll look into these tests.

[danakj, 2012-11-11 20:03:56]

@sadrul can you please review ui/base/gestures/gesture_sequence.cc

[danakj, 2012-11-11 23:35:49]

@jeremya can you please review the chrome/browser changes?

[jeremya, 2012-11-11 23:40:09]

lgtm :)

[sadrul, 2012-11-12 01:28:01]

http://codereview.chromium.org/11410024/diff/9001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/11410024/diff/9001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:588: } else {
You can set bounding_box_ to empty when point_count_ == 0 (so you don't need to
use empty_box below)

[sky, 2012-11-12 04:21:46]

https://codereview.chromium.org/11410024/diff/9001/ui/gfx/size_base_impl.h
File ui/gfx/size_base_impl.h (right):

https://codereview.chromium.org/11410024/diff/9001/ui/gfx/size_base_impl.h#ne...
ui/gfx/size_base_impl.h:19: DCHECK(!(width < 0));
DCHECK_GE for all of these.

[danakj, 2012-11-12 05:45:56]

PTAL sadrul, thanks.

http://codereview.chromium.org/11410024/diff/9001/ui/base/gestures/gesture_se...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/11410024/diff/9001/ui/base/gestures/gesture_se...
ui/base/gestures/gesture_sequence.cc:588: } else {
On 2012/11/12 01:28:02, sadrul wrote:
> You can set bounding_box_ to empty when point_count_ == 0 (so you don't need
to
> use empty_box below)

Done.

[danakj, 2012-11-12 05:52:57]

https://codereview.chromium.org/11410024/diff/9001/ui/gfx/size_base_impl.h
File ui/gfx/size_base_impl.h (right):

https://codereview.chromium.org/11410024/diff/9001/ui/gfx/size_base_impl.h#ne...
ui/gfx/size_base_impl.h:19: DCHECK(!(width < 0));
On 2012/11/12 04:21:46, sky wrote:
> DCHECK_GE for all of these.

I did that at first, but then the DCHECK() fires for NaN values, which I didn't
know if we want - we had unit tests verifying that they work, so those were
DCHECKing. Opinion?

[sadrul, 2012-11-12 06:03:20]

gestures LGTM

[danakj, 2012-11-12 16:53:05]

OK I discussed with vollick, and we're reasonably certain NaN shouldn't really
happen in practice, so dchecking sounds quite reasonable, it should be
considered a bug.

[danakj, 2012-11-12 17:49:06]

And, it turns out there is a unit test in cc/ that verifies hit testing with a
certain matrix that causes NaNs. So, I am going to revert the last change and
not DCHECK in that scenario.

[commit-bot: I haz the power, 2012-11-12 17:50:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11410024/7002

[commit-bot: I haz the power, 2012-11-12 20:27:27]

Retried try job too often for step(s) browser_tests

[danakj, 2013-01-07 23:28:25]

I'm going to land this thing again, and we'll see what new fallout has occured
in the meantime. Fixed one dcheck in unittests already, from
ash/system/user/tray_user.cc.

[commit-bot: I haz the power, 2013-01-08 01:04:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11410024/16027

[commit-bot: I haz the power, 2013-01-08 16:01:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11410024/16027

[commit-bot: I haz the power, 2013-01-08 16:02:08]

Change committed as 175535

[Nico, 2013-01-20 01:10:20]

This CL added 25kB of binary size:
http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?...

Is this expected for a CL that's removing code? Is more code that used to be in
a regular function now in a template function?

[danakj, 2013-01-20 01:58:25]

On 2013/01/20 01:10:20, Nico wrote:
> This CL added 25kB of binary size:
>
http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?...
> 
> Is this expected for a CL that's removing code? Is more code that used to be
in
> a regular function now in a template function?

No.. this does not make sense. The set_width() and set_height() methods along
with the constructor from SizeBase all became non-inline (and equally
templated), with implementations moving to size.cc and size_f.cc.

Did something change in the system generating those numbers?

[Nico, 2013-01-20 03:33:36]

I don't think so.

[danakj, 2013-01-20 03:44:28]

sky@ or piman@ any ideas? I don't really understand how it could happen.

On Sat, Jan 19, 2013 at 10:33 PM, <thakis@chromium.org> wrote:

> I don't think so.
>
>
https://chromiumcodereview.**appspot.com/11410024/<https://chromiumcodereview...
>

[piman, 2013-01-20 06:14:51]

On Sat, Jan 19, 2013 at 5:58 PM, <danakj@chromium.org> wrote:

> On 2013/01/20 01:10:20, Nico wrote:
>
>> This CL added 25kB of binary size:
>>
>
> http://build.chromium.org/f/**chromium/perf/linux-release-**
>
64/sizes/report.html?history=**150&rev=-1<http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?history=150&rev=-1>
>
>  Is this expected for a CL that's removing code? Is more code that used to
>> be
>>
> in
>
>> a regular function now in a template function?
>>
>
> No.. this does not make sense. The set_width() and set_height() methods
> along
> with the constructor from SizeBase all became non-inline (and equally
> templated), with implementations moving to size.cc and size_f.cc.
>

I suspect the DCHECKs in set_width/set_height (being templates, I strongly
suspect they *are* inline). Those add code even in release (but not
official builds).


> Did something change in the system generating those numbers?
>
>
https://chromiumcodereview.**appspot.com/11410024/<https://chromiumcodereview...
>

[danakj, 2013-01-20 17:55:40]

On 2013/01/20 06:14:51, piman wrote:
> I suspect the DCHECKs in set_width/set_height (being templates, I strongly
> suspect they *are* inline). Those add code even in release (but not
> official builds).

Hm, well, they are in size_base_impl.h which is only included in size.cc and
size_f.cc, which is why I don't think they're inline. Everyone else only has
access to the definitions.

[sky, 2013-01-22 16:50:04]

Maybe back it all out and land in small chunks to see which one caused the
increase?

[danakj, 2013-01-22 17:39:51]

Ya, I guess so. Will do.

On Tue, Jan 22, 2013 at 11:50 AM, <sky@chromium.org> wrote:

> Maybe back it all out and land in small chunks to see which one caused the
> increase?
>
>
https://chromiumcodereview.**appspot.com/11410024/<https://chromiumcodereview...
>