Expose Web Animations API to Web Workers

Expose Web Animations API to Web Workers
BUG=

[shans, 2014-08-25 11:53:59]

This is looking good. I've made a first pass through and commented on a few
things.

https://codereview.chromium.org/491053004/diff/1/Source/bindings/core/v8/Seri...
File Source/bindings/core/v8/SerializedScriptValue.cpp (left):

https://codereview.chromium.org/491053004/diff/1/Source/bindings/core/v8/Seri...
Source/bindings/core/v8/SerializedScriptValue.cpp:56: #include
"core/dom/ExceptionCode.h"
Revert the changes in this file (git checkout master
Source/bindings/core/v8/SerializedScriptValue.cpp)

https://codereview.chromium.org/491053004/diff/1/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/core.gypi#newcod...
Source/core/core.gypi:437: 
Remove empty lines from this file

https://codereview.chromium.org/491053004/diff/1/Source/core/frame/LocalDOMWi...
File Source/core/frame/LocalDOMWindow.h (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/frame/LocalDOMWi...
Source/core/frame/LocalDOMWindow.h:107: void passValues(MessageEvent);
passValues needs a more descriptive name.

https://codereview.chromium.org/491053004/diff/1/Source/core/page/Page.h
File Source/core/page/Page.h (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/page/Page.h#newc...
Source/core/page/Page.h:315: void add(DedicatedWorkerGlobalScope * instance)
add needs a more descriptive name.

https://codereview.chromium.org/491053004/diff/1/Source/core/page/PageAnimato...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/page/PageAnimato...
Source/core/page/PageAnimator.cpp:31: // sending monotonicAnimationStartTime
over to worker global scopes
'Send monotonic....' (full sentences, with capitalization and full stop at the
end, for comments).

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/Animatable...
File Source/core/wawwa/AnimatableElement.cpp (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/Animatable...
Source/core/wawwa/AnimatableElement.cpp:61: exit(42);
ASSERT(temp.isSafeToSendToAnotherThread() &&
action.isSafeToSendToAnotherThread());

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/Animatable...
Source/core/wawwa/AnimatableElement.cpp:116:
workerGlobalScope->thread()->workerObjectProxy().postTaskToMainExecutionContext(createCrossThreadTask(&ListOfElements::execute,
AllowCrossThreadAccess(page->m_listOfElements.get()), action, temp, holdVector,
ti));
This could be split onto a few lines for better readability.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/Animatable...
File Source/core/wawwa/AnimatableElement.idl (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/Animatable...
Source/core/wawwa/AnimatableElement.idl:42: // go/wawwa-design-doc
It's OK to link to documentation, but it should be externally viewable (i.e. in
chromium.org). That probably doesn't matter for now, but if you have a
chromium.org account I can show you how to copy the design doc across into that
org.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ListOfElem...
File Source/core/wawwa/ListOfElements.cpp (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ListOfElem...
Source/core/wawwa/ListOfElements.cpp:81: void ListOfElements::checkForReport()
checkForReport should have a more descriptive name.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ListOfElem...
File Source/core/wawwa/ListOfElements.h (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ListOfElem...
Source/core/wawwa/ListOfElements.h:57: static PassRefPtr<ListOfElements>
create() { return adoptRef(new ListOfElements()); }
We should stick to only Oilpan types - I think this is PassRefPtrWillBeRawPtr
(but Doug can help here too).

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ListOfElem...
File Source/core/wawwa/ListOfElements.idl (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ListOfElem...
Source/core/wawwa/ListOfElements.idl:33: interface ListOfElements {
If this isn't exposed to script it doesn't need to have an interface.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/MockAnimat...
File Source/core/wawwa/MockAnimationPlayer.cpp (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/MockAnimat...
Source/core/wawwa/MockAnimationPlayer.cpp:70: }
ASSERT(....);

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/MockAnimat...
Source/core/wawwa/MockAnimationPlayer.cpp:83: }
ASSERT(....);

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/MockAnimat...
Source/core/wawwa/MockAnimationPlayer.cpp:105: String action = "finish_element";
In C++, I think we can use an enum instead of Strings for the actions. This
might actually simplify things a bit.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ProxyKeyfr...
File Source/core/wawwa/ProxyKeyframe.h (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ProxyKeyfr...
Source/core/wawwa/ProxyKeyframe.h:1: #ifndef ProxyKeyframe_h
Add the copyright header at the top of new files.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ProxyPlaye...
File Source/core/wawwa/ProxyPlayer.cpp (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ProxyPlaye...
Source/core/wawwa/ProxyPlayer.cpp:104: }
with an enum, you'll be able to use a switch statement here.

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ProxyPlaye...
File Source/core/wawwa/ProxyPlayer.idl (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/wawwa/ProxyPlaye...
Source/core/wawwa/ProxyPlayer.idl:33: interface ProxyPlayer {
Is this exposed to script?

https://codereview.chromium.org/491053004/diff/1/Source/core/workers/Dedicate...
File Source/core/workers/DedicatedWorkerGlobalScope.h (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/workers/Dedicate...
Source/core/workers/DedicatedWorkerGlobalScope.h:67: private:
revert this change.

https://codereview.chromium.org/491053004/diff/1/Source/core/workers/Worker.idl
File Source/core/workers/Worker.idl (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/workers/Worker.i...
Source/core/workers/Worker.idl:39: [Custom, RaisesException] void
postMessage(SerializedScriptValue message, optional sequence<Transferable>
transfer);
revert this change.

https://codereview.chromium.org/491053004/diff/1/Source/core/workers/WorkerMe...
File Source/core/workers/WorkerMessagingProxy.h (right):

https://codereview.chromium.org/491053004/diff/1/Source/core/workers/WorkerMe...
Source/core/workers/WorkerMessagingProxy.h:83: WorkerGlobalScope* passObject();
passObject needs a more descriptive name.

[nainar1, 2014-08-29 00:33:44]

Fixed the issues stated.
Renamed files to reflect those in the polyfill.

[dstockwell, 2014-08-29 01:05:44]

Please update the description to indicate that this patch is not for landing.

https://codereview.chromium.org/491053004/diff/20001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.h:155: ExecutionContextClient* m_client;
Not sure why this is public. FIXME?

https://codereview.chromium.org/491053004/diff/20001/Source/core/page/Page.h
File Source/core/page/Page.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/page/Page.h#...
Source/core/page/Page.h:314: RefPtr<ProxyMap> m_ProxyMap;
This name is a bit vague, isn't this animation specific?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
File Source/core/wawwa/AnimatableElement.cpp (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.cpp:62: Timing ti =
TimingInput::convert(timingInputDictionary);
Don't use acronyms for local variable names.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.cpp:64: Vector<ProxyKeyframe> holdVector;
Try to avoid using types in variable names. 'holds' might be more appropriate,
but I'm still not sure what it is.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.cpp:68: for (size_t i = 0; i <
keyframeDictionaryVector.size(); i++) {
This logic should be shared with EffectInput.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
File Source/core/wawwa/AnimatableElement.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.h:67: AnimatableElement * clone();
Remove the space in 'AnimatableElement *'

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.h:68: PassRefPtrWillBeRawPtr<RemotePlayer>
animate(const Vector<Dictionary>& keyframeDictionaryVector, const Dictionary&
timingInputDictionary, PassRefPtr<DedicatedWorkerGlobalScope>, ExceptionState&);
keyframeDictionaryVector -> keyframes
timingInputDictionary -> timing or timingInput

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.h:71: String m_id;
Is this an ID or a string used as a selector?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
File Source/core/wawwa/AnimatableElement.idl (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.idl:33: Exposed=AnimatableElement,
What does this do?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyK...
File Source/core/wawwa/ProxyKeyframe.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyK...
Source/core/wawwa/ProxyKeyframe.h:40: enum ACTIONS {Animate, Pause, Play,
Cancel, Finish, Reverse};
Action, or ProxyPlayerAction.

Why is this in ProxyKeyframe?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
File Source/core/wawwa/ProxyMap.cpp (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
Source/core/wawwa/ProxyMap.cpp:44: RemotePlayerProxy *
ProxyMap::findDictRemotePlayerProxys(String val)
Needs a better name. What is val?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
Source/core/wawwa/ProxyMap.cpp:49: void ProxyMap::reportTime(String val, double
time)
Which time? Start, current time?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
File Source/core/wawwa/ProxyMap.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
Source/core/wawwa/ProxyMap.h:73: ProxyMap(ProxyMap const&);
const ProxyMap& ?

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
File Source/core/wawwa/RemotePlayer.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
Source/core/wawwa/RemotePlayer.h:64: void setStartTime(double val)
Trivial functions like this a better written on a single line.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
File Source/core/wawwa/RemotePlayer.idl (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
Source/core/wawwa/RemotePlayer.idl:44: /*
??

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
File Source/core/workers/Worker.cpp (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
Source/core/workers/Worker.cpp:152: void Worker::processRAF(double
monotonicAnimationStartTime)
RAF -> RequestAnimationFrame

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
File Source/core/workers/Worker.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
Source/core/workers/Worker.h:57: void postMessage(ExecutionContext*,
PassRefPtr<SerializedScriptValue > message, const MessagePortArray*,
ExceptionState&);
Revert this whitespace change.

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
Source/core/workers/Worker.h:67: WorkerGlobalScopeProxy* m_contextProxy; // The
proxy outlives the worker to perform thread shutdown.
why is this public now?

[nainar1, 2014-08-29 06:01:40]

Due to time constraints have responded with the needed actions or answers

https://codereview.chromium.org/491053004/diff/20001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.h:155: ExecutionContextClient* m_client;
There should be getters and setters for this

https://codereview.chromium.org/491053004/diff/20001/Source/core/page/Page.h
File Source/core/page/Page.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/page/Page.h#...
Source/core/page/Page.h:314: RefPtr<ProxyMap> m_ProxyMap;
No this is the overall ProxyMap with all RemotePlayerProxies attached to it

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
File Source/core/wawwa/AnimatableElement.cpp (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.cpp:68: for (size_t i = 0; i <
keyframeDictionaryVector.size(); i++) {
On 2014/08/29 01:05:43, dstockwell wrote:
> This logic should be shared with EffectInput.

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
File Source/core/wawwa/AnimatableElement.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.h:67: AnimatableElement * clone();
On 2014/08/29 01:05:43, dstockwell wrote:
> Remove the space in 'AnimatableElement *'

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.h:68: PassRefPtrWillBeRawPtr<RemotePlayer>
animate(const Vector<Dictionary>& keyframeDictionaryVector, const Dictionary&
timingInputDictionary, PassRefPtr<DedicatedWorkerGlobalScope>, ExceptionState&);
On 2014/08/29 01:05:43, dstockwell wrote:
> keyframeDictionaryVector -> keyframes
> timingInputDictionary -> timing or timingInput

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.h:71: String m_id;
String used as a selector - only supports IDs for now

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
File Source/core/wawwa/AnimatableElement.idl (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Animat...
Source/core/wawwa/AnimatableElement.idl:33: Exposed=AnimatableElement,
Exposes this class on the WorkerGlobalScope under this name

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyK...
File Source/core/wawwa/ProxyKeyframe.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyK...
Source/core/wawwa/ProxyKeyframe.h:40: enum ACTIONS {Animate, Pause, Play,
Cancel, Finish, Reverse};
Cause ProxyKeyframe - was already being imported everywhere

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
File Source/core/wawwa/ProxyMap.cpp (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
Source/core/wawwa/ProxyMap.cpp:44: RemotePlayerProxy *
ProxyMap::findDictRemotePlayerProxys(String val)
Thats the key you want to search on

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
Source/core/wawwa/ProxyMap.cpp:49: void ProxyMap::reportTime(String val, double
time)
startTime

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
File Source/core/wawwa/ProxyMap.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/ProxyM...
Source/core/wawwa/ProxyMap.h:73: ProxyMap(ProxyMap const&);
On 2014/08/29 01:05:44, dstockwell wrote:
> const ProxyMap& ?

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
File Source/core/wawwa/RemotePlayer.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
Source/core/wawwa/RemotePlayer.h:64: void setStartTime(double val)
On 2014/08/29 01:05:44, dstockwell wrote:
> Trivial functions like this a better written on a single line.

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
File Source/core/wawwa/RemotePlayer.idl (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/wawwa/Remote...
Source/core/wawwa/RemotePlayer.idl:44: /*
Should be removed

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
File Source/core/workers/Worker.cpp (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
Source/core/workers/Worker.cpp:152: void Worker::processRAF(double
monotonicAnimationStartTime)
On 2014/08/29 01:05:44, dstockwell wrote:
> RAF -> RequestAnimationFrame

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
File Source/core/workers/Worker.h (right):

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
Source/core/workers/Worker.h:57: void postMessage(ExecutionContext*,
PassRefPtr<SerializedScriptValue > message, const MessagePortArray*,
ExceptionState&);
On 2014/08/29 01:05:44, dstockwell wrote:
> Revert this whitespace change.

Acknowledged.

https://codereview.chromium.org/491053004/diff/20001/Source/core/workers/Work...
Source/core/workers/Worker.h:67: WorkerGlobalScopeProxy* m_contextProxy; // The
proxy outlives the worker to perform thread shutdown.
Should have getters and setters associated with this

[nainar, 2017-06-16 06:13:11]

nainar@chromium.org changed reviewers:
+ nainar@chromium.org

[nainar, 2017-06-16 06:13:12]

LOL LOL LOL