[EnforceRange] doesn't enforce range of a short

[EnforceRange] doesn't enforce range of a short

Handle Web IDL short / unsigned short types as per the
specification:
- http://www.w3.org/TR/WebIDL/#es-short
- http://www.w3.org/TR/WebIDL/#es-unsigned-short

Specifically, we used to treat short / unsigned short as 32bit
integers, which was wrong. We now properly handle them as 16bit
integers.

This CL is based on WebKit r158521:
http://trac.webkit.org/changeset/158521

R=haraken
TEST=js/dom/webidl-type-mapping.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161248

[Inactive, 2013-11-04 00:23:46]

On 2013/11/03 23:52:42, Chris Dumez wrote:

There appears to be a bug. I will investigate it and reopen shortly.

[Inactive, 2013-11-04 00:45:58]

https://codereview.chromium.org/54903014/diff/100001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/54903014/diff/100001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:229: numberValue = numberValue < 0 ?
-floor(fabs(numberValue)) : floor(fabs(numberValue));
I actually found a long-standing bug where we were using abs(int) instead of
fabs(double) here, thus potentially loosing precision (covered by the new
tests).

[haraken, 2013-11-04 06:43:41]

LGTM. Thanks for fixing the inconsistent behavior.

[eseidel, 2013-11-04 07:13:27]

Is the issue that we didn't understand the "short" type, or that we just
mistakenly treated it as 32-bit?  I assume we have asserts, etc. in the IDL
parser to make sure we understand all types we encounter?

[eseidel, 2013-11-04 07:14:13]

https://codereview.chromium.org/54903014/diff/150001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (left):

https://codereview.chromium.org/54903014/diff/150001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:5372: return "toInt32($value,
$intConversion, ok)" if $type eq "long" or $type eq "short";
I see.  We just were mistakenly treating it 32bit.

Thanks for fixing.

[Inactive, 2013-11-04 12:51:13]

On 2013/11/04 07:14:13, eseidel wrote:
>
https://codereview.chromium.org/54903014/diff/150001/Source/bindings/scripts/...
> File Source/bindings/scripts/code_generator_v8.pm (left):
> 
>
https://codereview.chromium.org/54903014/diff/150001/Source/bindings/scripts/...
> Source/bindings/scripts/code_generator_v8.pm:5372: return "toInt32($value,
> $intConversion, ok)" if $type eq "long" or $type eq "short";
> I see.  We just were mistakenly treating it 32bit.
> 
> Thanks for fixing.

Yes, we were treating it as 32-bit which works in most cases. however, when
[EnforceRange] was used,
we mistakenly did not know for values that fit in an int but not in a short.
Also, without EnforceRange, the result can be different for values that don't
fit in a short due to the modulo 2**32 instead of 2**16.

[commit-bot: I haz the power, 2013-11-04 12:51:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/54903014/150001

[commit-bot: I haz the power, 2013-11-04 14:33:25]

Change committed as 161248

[Nils Barth (inactive), 2013-11-05 01:35:39]

Thanks so much Chris!

[Nils Barth (inactive), 2013-11-05 01:51:44]

On 2013/11/04 07:13:27, eseidel wrote:
> I assume we have asserts, etc. in the IDL parser
> to make sure we understand all types we encounter?

Technical detail:
the parser doesn't do *any* type-checking:
interface names are types, so any syntactically valid identifier
is potentially a type.

Type handling happens in the code generator: we handle built-in
types, and for other types we sometimes need to check that a type
is actually valid (namely when you need to  look up something in
an interface), but other times it just assumes it's a valid interface
and you'll get compile errors if you make a typo.

We mostly don't do semantic analysis, esp. verification - mistakes
usually yield obvious compile errors, and adding verification makes
the compiler *much* more complex (and probably slower) for little benefit,
though we do verify Extended Attributes, as these otherwise cause
silent errors (b/c inapplicable attributes are ignored) and have been a
source of bugs in the past.

Instead, we rely on the IDL being correct, which is verified by the
bindings actually working with the implementation.

We could add detailed verification in future, probably as a separate
test (e.g., presubmit) to avoid making the CG complex and slowing down
the compile, but for now this has been a low priority.