Make the web-animations engine use the passed in blink::WebFrameTime values.

Make the web-animations engine use the passed in blink::WebFrameTime values.

BUG=346230

[mithro-old, 2014-07-07 05:11:44]

Make sure you look at patchset-1 (rather than patchset-2 which includes the base
patch needed to get the try-bots to run).

[eseidel, 2014-07-07 16:31:50]

not lgtm.  the -1 magic numbers everywhere make this unreadable.  I gave up a
few files in, sorry.

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationClock.cpp (right):

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClock.cpp:65: m_frameTime =
blink::WebFrameTime(-1, -1, -1, -1);
Either make it default with an .isEmpty() accessor to test for that case, or
hide this behind a .clear() or similar.

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClock.h:45: : m_frameTime(-1, -1, -1, -1)
Can this be a default constructor on WebFrameTime instead?

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationClockTest.cpp (right):

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClockTest.cpp:50:
animationClock.updateTime(blink::WebFrameTime(-1, -1, 0, 0));
This is extremely opaque. Can we have a helper?

WebFrameTime::fromSeconds(0)
WebFrameTime(SecondsOnly, 0) or some such?

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClockTest.cpp:72:
animationClock.updateTime(blink::WebFrameTime(-1, -1, 100, 200));
Or if these are only used in unittests at least a helper for this file.  This is
not readable to my eyes.

[mithro-old, 2014-07-07 17:47:16]

Please look at the 3th patchset -->
https://codereview.chromium.org/369793003/#ps60001 , not the 4th (which contains
the needed https://codereview.chromium.org/321373003/ to allow it to run on the
try bots).

On 2014/07/07 16:31:50, eseidel wrote:
> not lgtm.  the -1 magic numbers everywhere make this unreadable.  I gave up a
> few files in, sorry.

It looks like you reviewed the files which really matter. Almost all the other
files are about plumbing the WebFrameTime object around (rather than the double)
value.

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationClock.cpp (right):

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClock.cpp:65: m_frameTime =
blink::WebFrameTime(-1, -1, -1, -1);
On 2014/07/07 16:31:50, eseidel wrote:
> Either make it default with an .isEmpty() accessor to test for that case, or
> hide this behind a .clear() or similar.

I've attempted a isNull / createNull pair on the WebFrameTime object but not
really sure that is a good idea. Open to better suggestions.

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClock.h:45: : m_frameTime(-1, -1, -1, -1)
On 2014/07/07 16:31:50, eseidel wrote:
> Can this be a default constructor on WebFrameTime instead?

Open to better suggestions on how to deal with the below.

The only place which should really be constructing WebFrameTime objects is the
Chrome code which calls into Blink so don't want to really encourage people to
make these objects. (There is already a possible issue in
Source/core/svg/graphics/SVGImageChromeClient.cpp which I have another
experimental patch to fix).

As this is a member variable C++ doesn't let us have this value as being "unset"
so we need some way to mark it as being unset (and using a pointer seems bad).

I can't use (0, 0, 0, 0) - the most logical choice as a many unit tests depend
on the first valid time being t=0.0; I ended up choosing (-1, -1, -1, -1)
because that seemed like the next logical choice.

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
File Source/core/animation/AnimationClockTest.cpp (right):

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClockTest.cpp:50:
animationClock.updateTime(blink::WebFrameTime(-1, -1, 0, 0));
On 2014/07/07 16:31:50, eseidel wrote:
> This is extremely opaque. Can we have a helper?
> 
> WebFrameTime::fromSeconds(0)
> WebFrameTime(SecondsOnly, 0) or some such?

As you said below, this type of creation only happens in unit tests because
AnimationClock doesn't use the first two fields (where the rest of Blink
does/will).

I created the helper function on this class.

https://codereview.chromium.org/369793003/diff/40001/Source/core/animation/An...
Source/core/animation/AnimationClockTest.cpp:72:
animationClock.updateTime(blink::WebFrameTime(-1, -1, 100, 200));
On 2014/07/07 16:31:50, eseidel wrote:
> Or if these are only used in unittests at least a helper for this file.  This
is
> not readable to my eyes.

The only code which should be creating blink::WebFrameTime objects is up in
Chrome land which calls this code. For the unit tests I created helper function
for this. 

The first two fields of WebFrameTime are not used by the AnimationClock.

[eseidel, 2014-07-07 19:31:16]

https://codereview.chromium.org/369793003/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/369793003/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationClock.h:45: :
m_frameTime(blink::WebFrameTime::CreateNull())
Blink API is in Blink style, so CreateNull() is wrong.

[mithro-old, 2014-07-08 04:28:30]

Is that any better? 

I'm 100% happy to do what ever you think is the best solution.

https://codereview.chromium.org/369793003/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/369793003/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationClock.h:45: :
m_frameTime(blink::WebFrameTime::CreateNull())
On 2014/07/07 19:31:16, eseidel wrote:
> Blink API is in Blink style, so CreateNull() is wrong.

I had a quick re-read of the Blink Coding Style to see if there was any clear
direction there but wasn't able to see anything?

I was thinking of using a static constant on the WebFrameTime class to track an
Undefined value, the couple of APIs in public/platform which have things like
this are;

public/platform/WebCryptoAlgorithm.h
90:    static const ParamsTypeOrUndefined Undefined = -1;
public/platform/WebURLLoaderClient.h
74:    static const int64_t kUnknownEncodedDataLength = -1;
public/platform/Platform.h
305:    static const size_t noDecodedImageByteLimit = static_cast<size_t>(-1);

I thought following the Undefined pattern in
public/platform/WebCryptoAlgorithm.h might be the way to go but annoyingly you
can't have a "static const double Undefined = -1;" before C++0x. I've tried to
do the same with a static function.

How does that look?

[eseidel, 2014-07-08 04:34:31]

Sorry, I shoudl have been more clear.  CreateNull() was only wrong in casing. 
My understanding is all of the blink:: namespace is blink style which means
functions start with a lower case letter (unlike chromium).

[mithro-old, 2014-07-08 04:40:58]

On 2014/07/08 04:34:31, eseidel wrote:
> Sorry, I shoudl have been more clear.  CreateNull() was only wrong in casing. 
> My understanding is all of the blink:: namespace is blink style which means
> functions start with a lower case letter (unlike chromium).

Is having an explicit static function for creating a "null" WebFrameTime is
better than just an empty constructor? I personally think it's clearer about the
intention of what you are doing, but happy with either.

Does the Undefined() method seem okay / better than just using -1 everywhere?
What else needs to be fixed in this CL?

[eseidel, 2014-07-08 04:50:56]

I'm not opposed to this change.  In fact, I strongly support the concept.  I
think there are some style nits we could still pick, but those are better done
by someone in your same timezone.

lgtm from me (to remove my not lgtm), but please get one of the blink owners in
your timezone to walk through this patch with you.

The goal of removing the -1 is to make the code more self explainitory.  I would
have killed all of them, even the ones in the test files.  Those ones probably
just want a 2-param version of teh constructor or something which returns one of
these.

As discussed before, it's best to only commit the code we're actually using now
and leave the later parts to be added later.

blink:: needs to die, but that's not your fault, we need to rename all of
"namesapce core" to "namespace blink" and get rid all teh silly "blink::" usage
everywhere.

It reads funny to ahve a struct with methods.  I personally would have written 
"class foo { public:" instead since that's equivalent.

I wouldn't worry about the -1 vs. -1.0, the comment about it seems like
premature optimization.  I can't imagine that int->double conversion is showing
up on any benchmark.

You're correct that Undefined() as a method should be undefined() per blink
style.   If it were a constant (or a type or an enum) it could be Undefined. 
Why not just make it a real constant?  Does it need to be a method?  I guess if
this file doesn't have a .cpp file its not really worth creating one just for
that... 

Sorry for holding up this (rather trivial) change.

[eseidel, 2014-07-08 04:51:19]

bah.  silly regexp.  lgtm per above.

[jamesr, 2014-07-08 04:59:38]

This appears to use a different value for the animation engine and the value
passed to requestAnimationFrame.  If so, not lgtm.  The spec for rAF requires
that these be the same so that authors can sync their animations up with the
engine's animations exactly and it precisely defines the meaning of the
parameter passed to rAF callbacks.  If we prefer to use this value than what we
pass to rAF, then we'll need to expose this to authors in some way so that they
can stay in sync.

[jamesr, 2014-07-08 05:00:35]

https://codereview.chromium.org/369793003/diff/160001/Source/core/animation/A...
File Source/core/animation/AnimationClock.cpp (right):

https://codereview.chromium.org/369793003/diff/160001/Source/core/animation/A...
Source/core/animation/AnimationClock.cpp:62: return
m_frameTime.displayFrameTime;
in particular this value...

https://codereview.chromium.org/369793003/diff/160001/Source/core/dom/Scripte...
File Source/core/dom/ScriptedAnimationController.cpp (right):

https://codereview.chromium.org/369793003/diff/160001/Source/core/dom/Scripte...
Source/core/dom/ScriptedAnimationController.cpp:148: double highResNowMs =
1000.0 *
m_document->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(frameTime.lastFrameTime);
... and this value do not appear to be the same to me.

[mithro-old, 2014-07-08 05:31:28]

On 2014/07/08 04:59:38, jamesr wrote:
> This appears to use a different value for the animation engine and the value
> passed to requestAnimationFrame.  If so, not lgtm.  The spec for rAF requires
> that these be the same so that authors can sync their animations up with the
> engine's animations exactly and it precisely defines the meaning of the
> parameter passed to rAF callbacks.  If we prefer to use this value than what
we
> pass to rAF, then we'll need to expose this to authors in some way so that
they
> can stay in sync.

Hi James,

The lastFrameTime and the renderDisplayTime is currently the same value and
hence this change has no user facing effect at the moment. 

For more context on why we want animations to eventually use a different time
and the relationship with Javascript RAF, Nat Duca is a good person to talk too.

The web-animations javascript API will expose a document.timeline.currentTime()
which provides the time used inside the internal animation engine, allowing
javascript animations to exactly synchronize with CSS animations/transitions.

Tim

[jamesr, 2014-07-08 05:33:22]

That's all fine, but changing behavior away from what the spec currently says
requires at the very least proposing your change to the spec and having the
opportunity to discuss the pros and cons with other vendors + potential
compatability impact.  Neither of those have happened on any public forum from
what I can tell.

[mithro-old, 2014-07-08 05:37:09]

On 2014/07/08 05:33:22, jamesr wrote:
> That's all fine, but changing behavior away from what the spec currently says
> requires at the very least proposing your change to the spec and having the
> opportunity to discuss the pros and cons with other vendors + potential
> compatability impact.  Neither of those have happened on any public forum from
> what I can tell.

We are not proposing changing any behaviour away from what is currently
specified. 

Tim

[jamesr, 2014-07-08 05:38:00]

On 2014/07/08 05:37:09, mithro wrote:
> On 2014/07/08 05:33:22, jamesr wrote:
> > That's all fine, but changing behavior away from what the spec currently
says
> > requires at the very least proposing your change to the spec and having the
> > opportunity to discuss the pros and cons with other vendors + potential
> > compatability impact.  Neither of those have happened on any public forum
from
> > what I can tell.
> 
> We are not proposing changing any behaviour away from what is currently
> specified. 
> 
> Tim

Then what does this patch accomplish?

[mithro-old, 2014-07-08 05:42:28]

On 2014/07/08 05:38:00, jamesr wrote:
> On 2014/07/08 05:37:09, mithro wrote:
> > On 2014/07/08 05:33:22, jamesr wrote:
> > > That's all fine, but changing behavior away from what the spec currently
> says
> > > requires at the very least proposing your change to the spec and having
the
> > > opportunity to discuss the pros and cons with other vendors + potential
> > > compatability impact.  Neither of those have happened on any public forum
> from
> > > what I can tell.
> > 
> > We are not proposing changing any behaviour away from what is currently
> > specified. 
> > 
> > Tim
> 
> Then what does this patch accomplish?

1) It tells the web-animation system what the next frame time because the
compositor scheduler is more likely to know this information than blink.
2) Lays foundation for web animation to render to display time rather than last
frame time. According to the specification authors, the web animations timebase
has yet to be decided.

Tim

[mithro-old, 2014-07-08 23:59:17]

Hi James,

Lets defer more discussion on this CL until after no meeting week when we can
schedule a VC meeting to discuss this issue further. What time is good for you
next week?

Tim