Re: [geometry] DOMRect: use of unrestricted doubles

On Mon, Oct 17, 2016 at 11:21 PM, Simon Fraser <smfr@me.com> wrote:

> On Oct 17, 2016, at 7:03 am, Rik Cabanier <cabanier@gmail.com> wrote:
>
> On Sun, Oct 16, 2016 at 11:15 PM, Rik Cabanier <cabanier@gmail.com> wrote:
>
>>
>>
>> On Sun, Oct 16, 2016 at 9:21 PM, Simon Fraser <smfr@me.com> wrote:
>>
>>> On Oct 16, 2016, at 9:34 AM, Rik Cabanier <cabanier@gmail.com> wrote:
>>>
>>> On Sat, Oct 15, 2016 at 2:47 PM, Simon Fraser <smfr@me.com> wrote:
>>>
>>>> https://drafts.fxtf.org/geometry/#DOMRect
>>>>
>>>> Using unrestricted doubles forces implementors to handle NaN and Inf
>>>> values for x, y, width and height, and to correctly propagate NaN and Inf
>>>> through steps used to calculate left, top, right, bottom (assuming IEEE
>>>> rules, though the spec does not make this explicit). This is non-trivial,
>>>> since std::min<> and  std::max<> do not follow the same NaN propagation
>>>> rules as Math.min()/Math.max(). Implementors have to add isnan() checks to
>>>> left, top, right, bottom implementations. Is the complexity worth it?
>>>>
>>>
>>> yes. We allowed this so NaN and Inf would signal when matrix
>>> calculations hit edge conditions.
>>> Instead of throwing or giving inaccurate result, it was decided to allow
>>> the values so authors can check for those. The alternative would be to
>>> throw and we feared that this would break a lot of code since people don't
>>> test with exceptions.
>>>
>>> See also the thread here: https://lists.mozilla.or
>>> g/pipermail/dev-platform/2014-June/005091.html
>>>
>>> When I did the implementation in Firefox, this actually made the code
>>> easier to implement since I didn't have to add a bunch of conditionals and
>>> could just rely on the FPU to do the correct thing.
>>>
>>>
>>> Well, Firefox does the wrong thing for DOMRect.left() when, for example.
>>> width is NaN (if you assume the spec follows JS Math rules).
>>>
>>
>> roc implemented DOMRect and it seems that it was a simple rename of
>> something that Firefox already had [1]. It likely doesn't follow the spec
>> to the letter.
>>
>> There's a test in the patch in https://bugs.webkit.org/sho
>>> w_bug.cgi?id=163464 (which needs to be converted to a web platform
>>> test).
>>>
>>
>> Why do you need to add the special case handling? If width or x is NaN,
>> shouldn't left always returns x?
>>
>
> Sorry, I meant to say NaN
> min(x, x+NaN) = min(x, NaN) = (x<NaN?x:NaN) = NaN
>
>
>> 1: https://bugzilla.mozilla.org/show_bug.cgi?id=916520
>> <https://bugzilla.mozilla.org/show_bug.cgi?id=916520>
>>
>
> Correct implementation of left, top, right and bottom require two NaN
> checks per call, which is an unfortunate side-effect of allowing NaN
> through the interface:
>
> https://trac.webkit.org/browser/trunk/Source/WebCore/
> dom/DOMRectReadOnly.h?rev=207438#L47
> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/
> MathExtras.h?rev=207438#L403
>

I see. You're interpreting min/max as math.min/max which don't follow
normal floating point rules. [1]
We should make the spec more clear on what these operators mean since we
didn't intend for this subtlety to happen.

I'd vote to put in pseudo code for min/max so you can get rid of the
special case handling on the c++ side

1: https://tc39.github.io/ecma262/#sec-math.min