Make sure that begin time cannot be greater than

Make sure that begin time cannot be greater than
SMILTime::indefiniteValue unintentionally.

When blink::SVGSMILElement::resolveInterval creates a SMILTime begin
node for a SMILInterval, then it only checks if the value of begin is
indefinite or unresolved but misses the case if it is between these two
reference values.

This patch ensures that no SMILTime can be created above indefiniteValue.

R=dschulze@chromium.org,fs@opera.com
BUG=396044
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178825

[fs, 2014-07-22 16:29:37]

https://codereview.chromium.org/406263002/diff/1/Source/core/svg/animation/SM...
File Source/core/svg/animation/SMILTime.h (right):

https://codereview.chromium.org/406263002/diff/1/Source/core/svg/animation/SM...
Source/core/svg/animation/SMILTime.h:37: SMILTime(double time, bool checkRange =
true) : m_time(checkRange && (time > indefiniteValue) ? indefiniteValue : time)
{ ASSERT(!std::isnan(time)); }
I think I'd prefer if this was done at parse-time (parseClockValue) instead. 

I'm getting more and more convinced that using NaN and Inf for these would be
better... That's probably best left for another CL though.

[reni, 2014-07-23 13:42:35]

I've added the isFinite() check not just to offset parsing but also to the full
clock parsing (hh:mm:ss).
According to the standard
(http://www.w3.org/TR/SVG/animate.html#ClockValueSyntax), hours in full clock
format can have any digits and so their calculated time can be greater than
FLT_MAX. Although the current implementation assumes that hours could have only
two digits (is it a bug or feature?) I wanted to make sure we don't run into the
same problem later.

https://codereview.chromium.org/406263002/diff/1/Source/core/svg/animation/SM...
File Source/core/svg/animation/SMILTime.h (right):

https://codereview.chromium.org/406263002/diff/1/Source/core/svg/animation/SM...
Source/core/svg/animation/SMILTime.h:37: SMILTime(double time, bool checkRange =
true) : m_time(checkRange && (time > indefiniteValue) ? indefiniteValue : time)
{ ASSERT(!std::isnan(time)); }
On 2014/07/22 16:29:37, fs wrote:
> I think I'd prefer if this was done at parse-time (parseClockValue) instead. 
> 
> I'm getting more and more convinced that using NaN and Inf for these would be
> better... That's probably best left for another CL though.

Do you mean using NaN and Inf instead FLT_MAX and DBL_MAX would be the better
choice?

[fs, 2014-07-23 14:25:32]

LGTM

On 2014/07/23 13:42:35, reni wrote:
> I've added the isFinite() check not just to offset parsing but also to the
full
> clock parsing (hh:mm:ss).
> According to the standard
> (http://www.w3.org/TR/SVG/animate.html#ClockValueSyntax), hours in full clock
> format can have any digits and so their calculated time can be greater than
> FLT_MAX. Although the current implementation assumes that hours could have
only
> two digits (is it a bug or feature?) I wanted to make sure we don't run into
the
> same problem later.

Sounds like a bug. (But animations running after 99:59:59.999 are probably rare
;-)

>
https://codereview.chromium.org/406263002/diff/1/Source/core/svg/animation/SM...
> File Source/core/svg/animation/SMILTime.h (right):
> 
>
https://codereview.chromium.org/406263002/diff/1/Source/core/svg/animation/SM...
> Source/core/svg/animation/SMILTime.h:37: SMILTime(double time, bool checkRange
=
> true) : m_time(checkRange && (time > indefiniteValue) ? indefiniteValue :
time)
> { ASSERT(!std::isnan(time)); }
> On 2014/07/22 16:29:37, fs wrote:
> > I think I'd prefer if this was done at parse-time (parseClockValue) instead.

> > 
> > I'm getting more and more convinced that using NaN and Inf for these would
be
> > better... That's probably best left for another CL though.
> 
> Do you mean using NaN and Inf instead FLT_MAX and DBL_MAX would be the better
> choice?

Yes. NaN as "unresolved" and Inf as "indefinite" - should be a decent match
semantically (how Inf orders compared to finite values for instance). The only
downside is probably that NaN != NaN so you (still) need to check for
(non-)finiteness here and there - but then again that's require with the current
values as well...

[reni, 2014-07-24 06:59:30]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-07-24 07:00:26]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rhodovan.u-szeged@partner.samsung.com/...

[commit-bot: I haz the power, 2014-07-24 07:57:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18669)

[commit-bot: I haz the power, 2014-07-24 08:35:50]

Change committed as 178825