Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
620783002:
Devtools Animations: Basic animation inspection & control in Styles pane (Closed)
|
Created:
6 years, 2 months ago by samli Modified:
6 years, 2 months ago CC:
blink-reviews, shans, apavlov+blink_chromium.org, loislo+blink_chromium.org, aandrey+blink_chromium.org, caseq+blink_chromium.org, Steve Block, pfeldman+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, dstockwell, Timothy Loh, devtools-reviews_chromium.org, Eric Willigers, rjwright, lushnikov+blink_chromium.org, eustas+blink_chromium.org, paulirish+reviews_chromium.org, darktears, blink-reviews-animation_chromium.org, vsevik+blink_chromium.org, Mike Lawther (Google), sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionDevtools Animations: Basic animation inspection & control in Styles pane
Add a new hidden devtools experiment for animation inspection within the
inspector. A new inspector domain is created for Animations.
CSS Animations, CSS Transitions and Web Animations can be viewed in an
Animations pane when an animation is in effect and targeting the element
being viewed in the Elements pane. Each animation can be paused, played
and scrubbed based on time position. Animation properties are shown for
all animations.
BUG=419269
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183847
Patch Set 1 #
Total comments: 42
Patch Set 2 : Review changes for AnimationSection #
Total comments: 31
Patch Set 3 : Review changes #Patch Set 4 : Changed sequence number to string id #
Total comments: 24
Patch Set 5 : Animations now in a separate tab #Patch Set 6 : Rebase #Patch Set 7 : Remove keyframe view #
Total comments: 30
Patch Set 8 : Refactor animations into separate domain #Patch Set 9 : Update JSDOC #
Total comments: 8
Patch Set 10 : #Messages
Total messages: 28 (6 generated)
samli@chromium.org changed reviewers: + vsevik@chromium.org
awesome! Make sure to tell the team in SYD to enable the experiment once it lands! On 1 October 2014 16:10, <samli@chromium.org> wrote: > Reviewers: vsevik, > > Description: > Devtools Animations: Basic animation inspection & control in Styles pane > > Add a new hidden devtools experiment for animation inspection within the > inspector. > > CSS Animations, CSS Transitions and Web Animations can be viewed in the > Styles pane when an animation is in effect and targeting the element > being viewed in the Elements pane. Each animation can be paused, played > and scrubbed based on time position. Animation properties are shown for > all animations. For Web Animations the keyframe styles are also shown. > > BUG=419269 > > Please review this at https://codereview.chromium.org/620783002/ > > SVN Base: https://chromium.googlesource.com/chromium/blink.git@master > > Affected files (+713, -1 lines): > M Source/core/animation/KeyframeEffectModel.h > M Source/core/animation/StringKeyframe.h > M Source/core/inspector/InspectorCSSAgent.h > M Source/core/inspector/InspectorCSSAgent.cpp > M Source/core/inspector/InspectorDOMAgent.h > M Source/core/inspector/InspectorDOMAgent.cpp > M Source/devtools/devtools.gypi > A Source/devtools/front_end/elements/AnimationSection.js > M Source/devtools/front_end/elements/StylesSidebarPane.js > M Source/devtools/front_end/elements/module.json > M Source/devtools/front_end/main/Main.js > M Source/devtools/front_end/sdk/CSSStyleModel.js > M Source/devtools/front_end/sdk/DOMModel.js > M Source/devtools/protocol.json > > > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
apavlov@chromium.org changed reviewers: + apavlov@chromium.org
A quick scan across AnimationSection.js https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... File Source/devtools/front_end/elements/AnimationSection.js (right): https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:1: /** The [short 3-line] copyright notice is missing https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:7: var section = this; This should go just before the createCurrentTimeSlider(), since it's only used in nested functions https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:12: this.player = animationPlayer; We typically assign the constructor arguments first https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:20: function createCurrentTimeSlider() { Brace on the next line. I also think this should be a private method, not a nested function. This will allow the use of "this", and we'll get rid of "var section" https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:24: slider.type = "range"; It's best to glue these to the "var slider" declaration https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:37: slider.addEventListener('input', function (e) { DevTools use double quotes for string literals. This should also be a named function (since it will allow the proper annotation of |e|) https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:47: function createAnimationControls() { Brace on the next line. Ditto for this becoming a private method. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:51: var pausePlayButton = document.createElement("button"); Not sure, but you might want to take a look at WebInspector.StatusBarButton instead https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:52: if (section.player.paused()) { no braces around single-line blocks https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:54: } else { ditto https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:57: pausePlayButton.addEventListener("click", function () { Please use a named nested function https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:78: function updateAnimationPlayer(animationPlayer) { brace on the next line https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:79: if (animationPlayer) Is this missing braces around the next two lines or is the "requestAnimationFrame" call improperly indented? https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:84: function updateCurrentTime() { brace on the next line https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:85: section.player.getCurrentState(function (currentTime, isRunning) { Annotated named function, please https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:87: if (isRunning) { no braces around single-line blocks https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:95: get player() We do not use getters and setters in the new code. Please turn these into functions https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:99: set player(p) blank line above https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:109: 'paused': p.paused(), Double-quotes for string literals https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:124: var section = new WebInspector.ObjectPropertiesSection(obj, WebInspector.UIString("Animation Properties"), undefined, null, undefined, undefined); You only need the first two arguments here. Please remove the rest. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:127: }, extra comma
PTAL. Updated AnimationSection based on comments. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... File Source/devtools/front_end/elements/AnimationSection.js (right): https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:1: /** On 2014/10/01 07:39:36, apavlov wrote: > The [short 3-line] copyright notice is missing Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:7: var section = this; On 2014/10/01 07:39:35, apavlov wrote: > This should go just before the createCurrentTimeSlider(), since it's only used > in nested functions Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:12: this.player = animationPlayer; On 2014/10/01 07:39:36, apavlov wrote: > We typically assign the constructor arguments first Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:20: function createCurrentTimeSlider() { On 2014/10/01 07:39:35, apavlov wrote: > Brace on the next line. I also think this should be a private method, not a > nested function. This will allow the use of "this", and we'll get rid of "var > section" Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:24: slider.type = "range"; On 2014/10/01 07:39:35, apavlov wrote: > It's best to glue these to the "var slider" declaration Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:37: slider.addEventListener('input', function (e) { On 2014/10/01 07:39:35, apavlov wrote: > DevTools use double quotes for string literals. > > This should also be a named function (since it will allow the proper annotation > of |e|) Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:47: function createAnimationControls() { On 2014/10/01 07:39:35, apavlov wrote: > Brace on the next line. Ditto for this becoming a private method. Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:51: var pausePlayButton = document.createElement("button"); On 2014/10/01 07:39:35, apavlov wrote: > Not sure, but you might want to take a look at WebInspector.StatusBarButton > instead Ack. Going to look into this tomorrow. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:52: if (section.player.paused()) { On 2014/10/01 07:39:36, apavlov wrote: > no braces around single-line blocks Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:54: } else { On 2014/10/01 07:39:36, apavlov wrote: > ditto Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:57: pausePlayButton.addEventListener("click", function () { On 2014/10/01 07:39:35, apavlov wrote: > Please use a named nested function Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:78: function updateAnimationPlayer(animationPlayer) { On 2014/10/01 07:39:35, apavlov wrote: > brace on the next line Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:79: if (animationPlayer) On 2014/10/01 07:39:36, apavlov wrote: > Is this missing braces around the next two lines or is the > "requestAnimationFrame" call improperly indented? Acknowledged. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:84: function updateCurrentTime() { On 2014/10/01 07:39:35, apavlov wrote: > brace on the next line Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:85: section.player.getCurrentState(function (currentTime, isRunning) { On 2014/10/01 07:39:36, apavlov wrote: > Annotated named function, please Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:87: if (isRunning) { On 2014/10/01 07:39:35, apavlov wrote: > no braces around single-line blocks Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:95: get player() On 2014/10/01 07:39:36, apavlov wrote: > We do not use getters and setters in the new code. Please turn these into > functions Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:99: set player(p) On 2014/10/01 07:39:35, apavlov wrote: > blank line above Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:109: 'paused': p.paused(), On 2014/10/01 07:39:36, apavlov wrote: > Double-quotes for string literals Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:124: var section = new WebInspector.ObjectPropertiesSection(obj, WebInspector.UIString("Animation Properties"), undefined, null, undefined, undefined); On 2014/10/01 07:39:36, apavlov wrote: > You only need the first two arguments here. Please remove the rest. Done. https://codereview.chromium.org/620783002/diff/1/Source/devtools/front_end/el... Source/devtools/front_end/elements/AnimationSection.js:127: }, On 2014/10/01 07:39:35, apavlov wrote: > extra comma Done.
caseq@chromium.org changed reviewers: + caseq@chromium.org
https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:845: AnimationPlayer& player = *(m_domAgent->m_idToAnimationPlayer.get(sequenceNumber)); You can't expect client to provide a valid id, please check and return protocol error. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:856: if (i->get()->isStringKeyframe()) { nit: we prefer early bail-out, i.e: if (!i->get()->isStringKeyframe()) continue; https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:868: offset.append("%"); unused? https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1363: Element* element = toElement(node); This is a protocol method, what if protocol client supplied id of some node that isn't an element? https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1376: ASSERT(player); You shouldn't ASSERT() on client providing correct player id, ASSERT() is for invariants only. Please return a protocol error for invalid ids. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1384: ASSERT(player); ditto. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1392: ASSERT(player); ditto. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1402: *isRunning = !player->paused() && !player->finished(); isn't player->playing()? good for that? https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:88: friend class InspectorCSSAgent; Is this for access to the animation player map? Please expose a public method to look up a player by id instead (something similar to InspectorDOMAgent::assertNode would be handy perhaps). https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:147: virtual void pauseAnimationPlayer(ErrorString*, int sequenceNumber, RefPtr<TypeBuilder::DOM::AnimationPlayer>&) OVERRIDE; Here and below, int sequenceNumber -> const String& playerId https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:150: virtual void getStateAnimationPlayer(ErrorString*, int sequenceNumber, double* currentTime, bool* isRunning) OVERRIDE; getAnimationPalyerState() / setAnimaitonPlayerCurrentTime() perhaps? https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:2114: { "name": "sequenceNumber", "type": "number", "description": "<code>AnimationPlayer</code>'s sequenceNumber which can be used as an id." }, Can we just call it an id for consistency? Also, we're moving towards using strings as ids to get a bit more flexibility when we need composited ids (e.g. when we eventually need to identify objects in multiple renderers). I'd suggest introducing an AnimationPlayerId type which is defined to string (please see LayerId or ScriptIdentifier for an example). https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:2549: "name": "getStateAnimationPlayer", I wonder if we instead want to get events on animation player state change?
https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/sdk/CSSStyleModel.js (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/sdk/CSSStyleModel.js:271: function callback(userCallback, error, payloads) nit: no need to pass userCallback explicitly (outer one will be captured as part of closure) https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/sdk/DOMModel.js (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/sdk/DOMModel.js:2080: function mycallback(error, player) Here and below, please consider using InspectorBackend.wrapClientCallback() for reducing boilerplate.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
Maybe it's worth to create a separate Animation domain, especially considering further additions (I see FIXMEs about animation groups and custom effects)?
Changes based on caseq@ comments. @dgozman - what do you mean an animation domain? A separate pane? That's what we were considering before. Some of those FIXME will be a long time before they are introduced in the Web Animations API, if at all. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:845: AnimationPlayer& player = *(m_domAgent->m_idToAnimationPlayer.get(sequenceNumber)); On 2014/10/02 09:48:01, caseq wrote: > You can't expect client to provide a valid id, please check and return protocol > error. Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:856: if (i->get()->isStringKeyframe()) { On 2014/10/02 09:48:01, caseq wrote: > nit: we prefer early bail-out, i.e: > > if (!i->get()->isStringKeyframe()) > continue; Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:868: offset.append("%"); On 2014/10/02 09:48:01, caseq wrote: > unused? It's used below in the selector text. 873 This allows the keyframe offset to be displayed. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1363: Element* element = toElement(node); On 2014/10/02 09:48:01, caseq wrote: > This is a protocol method, what if protocol client supplied id of some node that > isn't an element? Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1376: ASSERT(player); On 2014/10/02 09:48:01, caseq wrote: > You shouldn't ASSERT() on client providing correct player id, ASSERT() is for > invariants only. Please return a protocol error for invalid ids. Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1384: ASSERT(player); On 2014/10/02 09:48:01, caseq wrote: > ditto. Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1392: ASSERT(player); On 2014/10/02 09:48:01, caseq wrote: > ditto. Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.cpp:1402: *isRunning = !player->paused() && !player->finished(); On 2014/10/02 09:48:01, caseq wrote: > isn't player->playing()? good for that? Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:88: friend class InspectorCSSAgent; On 2014/10/02 09:48:01, caseq wrote: > Is this for access to the animation player map? Please expose a public method to > look up a player by id instead (something similar to > InspectorDOMAgent::assertNode would be handy perhaps). Done. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:147: virtual void pauseAnimationPlayer(ErrorString*, int sequenceNumber, RefPtr<TypeBuilder::DOM::AnimationPlayer>&) OVERRIDE; On 2014/10/02 09:48:01, caseq wrote: > Here and below, int sequenceNumber -> const String& playerId Acknowledged. https://codereview.chromium.org/620783002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:150: virtual void getStateAnimationPlayer(ErrorString*, int sequenceNumber, double* currentTime, bool* isRunning) OVERRIDE; On 2014/10/02 09:48:01, caseq wrote: > getAnimationPalyerState() / setAnimaitonPlayerCurrentTime() perhaps? Done. https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/sdk/CSSStyleModel.js (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/sdk/CSSStyleModel.js:271: function callback(userCallback, error, payloads) On 2014/10/02 10:02:26, caseq wrote: > nit: no need to pass userCallback explicitly (outer one will be captured as part > of closure) Done. https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/sdk/DOMModel.js (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/sdk/DOMModel.js:2080: function mycallback(error, player) On 2014/10/02 10:02:26, caseq wrote: > Here and below, please consider using InspectorBackend.wrapClientCallback() for > reducing boilerplate. Done. https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:2114: { "name": "sequenceNumber", "type": "number", "description": "<code>AnimationPlayer</code>'s sequenceNumber which can be used as an id." }, On 2014/10/02 09:48:02, caseq wrote: > Can we just call it an id for consistency? Also, we're moving towards using > strings as ids to get a bit more flexibility when we need composited ids (e.g. > when we eventually need to identify objects in multiple renderers). I'd suggest > introducing an AnimationPlayerId type which is defined to string (please see > LayerId or ScriptIdentifier for an example). Ack. Will change to use a String id. I'll upload in next patchset. https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:2549: "name": "getStateAnimationPlayer", On 2014/10/02 09:48:02, caseq wrote: > I wonder if we instead want to get events on animation player state change? What would be the best way to get events, register a callback? Any good examples of that elsewhere? It's probably what we want to do for changes to the animation itself. Here I think polling on a requestAnimationFrame tick is sufficient since we simply want to get the current time which is always changing (this stops polling once the animation stops).
PTAL, incorporated all review feedback. https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/620783002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:2114: { "name": "sequenceNumber", "type": "number", "description": "<code>AnimationPlayer</code>'s sequenceNumber which can be used as an id." }, On 2014/10/03 06:05:05, samli wrote: > On 2014/10/02 09:48:02, caseq wrote: > > Can we just call it an id for consistency? Also, we're moving towards using > > strings as ids to get a bit more flexibility when we need composited ids (e.g. > > when we eventually need to identify objects in multiple renderers). I'd > suggest > > introducing an AnimationPlayerId type which is defined to string (please see > > LayerId or ScriptIdentifier for an example). > > Ack. Will change to use a String id. I'll upload in next patchset. Done now.
> @dgozman - what do you mean an animation domain? A separate pane? That's what we > were considering before. Some of those FIXME will be a long time before they are > introduced in the Web Animations API, if at all. I mean, separate domain in protocol.json and it's implementation as InspectorAnimationAgent. This doesn't require any changes on the UI side. If you plan to add more protocol methods (compared to this patch), I think it's worth to separate animations from DOM.
This generally looks pretty good, but I'm concerned about AnimationPlayer leakage. Can we perhaps make it a weak map (we already have something similar for DOM nodes, please have a look at InspectorNodeIds). > What would be the best way to get events, register a > callback? Any good examples of that elsewhere? > It's probably what we want to do for changes > to the animation itself. I think we can follow the way DOM agent handles DOM mutation events, i.e. we send the notification iff the client has previously requested the related object. So if, say, the client has requested the animation player for particular node (i.e. you have the player id in m_idToAnimationPlayer), you do emit the event (and if you need an event for animation player creation, this may perhaps be emitted based on whether it's target node is on he front-end, i.e. m_documentNodeToIdMap->contains(node)). > Here I think polling on a requestAnimationFrame tick is > sufficient since > we simply want to get the current time which is always > changing (this stops polling once the animation stops). Good point, I agree that polling is better in this case. https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationPlayer.h (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/An... Source/core/animation/AnimationPlayer.h:144: String id() const { return "s" + String::number(m_sequenceNumber); } Do we need the prefix and do we need this to happen in AnimationPlayer? I'd rather stringize the result of sequenceNumber() somewhere in the inspector code. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:221: AnimationPlayer* assertAnimationPlayer(ErrorString*, String id); nit: const String& id https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:289: WillBeHeapHashMap<String, RefPtrWillBeMember<AnimationPlayer> > m_idToAnimationPlayer; When do we release references to AnimationPlayers? https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/StylesSidebarPane.js:525: separatorElement.createTextChild(WebInspector.UIString("Animation") + " " + player.id()); Should we use name when available? https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/StylesSidebarPane.js:526: this._sectionsContainer.appendChild(separatorElement); nit: var separatorElement = this._sectionsContainer.createChild("div", "sidebar-separator"); https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/StylesSidebarPane.js:537: function keyframeStylesCallback(anchorElement, keyframeStyles) { style: { => next line
https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/Ke... File Source/core/animation/KeyframeEffectModel.h (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/Ke... Source/core/animation/KeyframeEffectModel.h:54: friend class InspectorCSSAgent; You should define an explicit API here that should be used by inspector instead. For instance you could add some methods with *ForInspector postfix to avoid them being used elsewhere. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:848: ASSERT(player->source()); Why do you ASSERT that source is not null? Looks like it is possible to create an animation player without source, so this ASSERT is not correct. I think what you want to do here is to check that all this conditions are held and return protocol error otherwise, but you should not crash otherwise. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:856: keyframeStyles = TypeBuilder::Array<TypeBuilder::CSS::CSSRule>::create(); This seems wrong. Not only you are abusing CSSRule for storing keyframes rules, you are also losing all the source data information (properties ranges in the original css file) while doing this. I think the right way would be to track source data for keyframes as part of our css support. In this case to implement this method you will need to 1. Get a css animation name by player id (you could use a map in CSSAnimation for that) 2. Extract CSSKeyframesRule using code similar to CSSAnimations::matchScopedKeyframesRule This would be in line with the way we support other CSS related features. However this seems quite complex, so I'd suggest that you moved keyframes-related code out of this patch and implement keyframes inspection support separately. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/elements/AnimationSection.js (right): https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/AnimationSection.js:36: requestAnimationFrame(this._updateCurrentTime.bind(this)); I think polling this on each animation frame creates too much load on the devtools connection. You could use WebInspector.Throttler to poll the time each 100ms and you could also use local time to emulate smooth slider position change between these requests if needed. Also I don't see how you exit from this time request loop, you should do this when the selected node is changed. We might want to change this in future but I think for now we should move all animation related stuff to a separate StylesSidebarPane tab and only poll animation state while this tab is selected. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/AnimationSection.js:125: setAnimationPlayer: function(p) This should be private (name should start with underscore) https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/AnimationSection.js:132: if (p instanceof WebInspector.DOMModel.AnimationPlayer) { Why do you need this check? It seems redundant
PTAL https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationPlayer.h (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/An... Source/core/animation/AnimationPlayer.h:144: String id() const { return "s" + String::number(m_sequenceNumber); } On 2014/10/03 12:43:55, caseq wrote: > Do we need the prefix and do we need this to happen in AnimationPlayer? I'd > rather stringize the result of sequenceNumber() somewhere in the inspector code. Done. https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/Ke... File Source/core/animation/KeyframeEffectModel.h (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/animation/Ke... Source/core/animation/KeyframeEffectModel.h:54: friend class InspectorCSSAgent; On 2014/10/06 16:00:54, vsevik wrote: > You should define an explicit API here that should be used by inspector instead. > For instance you could add some methods with *ForInspector postfix to avoid them > being used elsewhere. Done. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:848: ASSERT(player->source()); On 2014/10/06 16:00:54, vsevik wrote: > Why do you ASSERT that source is not null? Looks like it is possible to create > an animation player without source, so this ASSERT is not correct. I think what > you want to do here is to check that all this conditions are held and return > protocol error otherwise, but you should not crash otherwise. Done. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorCSSAgent.cpp:856: keyframeStyles = TypeBuilder::Array<TypeBuilder::CSS::CSSRule>::create(); On 2014/10/06 16:00:54, vsevik wrote: > This seems wrong. Not only you are abusing CSSRule for storing keyframes rules, > you are also losing all the source data information (properties ranges in the > original css file) while doing this. > I think the right way would be to track source data for keyframes as part of our > css support. In this case to implement this method you will need to > 1. Get a css animation name by player id (you could use a map in CSSAnimation > for that) > 2. Extract CSSKeyframesRule using code similar to > CSSAnimations::matchScopedKeyframesRule > > This would be in line with the way we support other CSS related features. > However this seems quite complex, so I'd suggest that you moved > keyframes-related code out of this patch and implement keyframes inspection > support separately. These keyframe styles are only shown for JS instantiated animations (via the Web Animations API). As a result, these animations don't have source data information. Web Anim uses StringKeyframe while CSS Animations uses AnimatableValueKeyframe, hence the early exit below. Implementing it the way you suggest would only work for CSS Animations and not Web Animations so I would prefer to leave this in this patch and implement CSS Animation keyframes separately since they are independent. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:221: AnimationPlayer* assertAnimationPlayer(ErrorString*, String id); On 2014/10/03 12:43:55, caseq wrote: > nit: const String& id Done. https://codereview.chromium.org/620783002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDOMAgent.h:289: WillBeHeapHashMap<String, RefPtrWillBeMember<AnimationPlayer> > m_idToAnimationPlayer; On 2014/10/03 12:43:55, caseq wrote: > When do we release references to AnimationPlayers? For now they are cleared on each new node selection. At a later stage this may need to change depending on what features we add. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/elements/AnimationSection.js (right): https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/AnimationSection.js:36: requestAnimationFrame(this._updateCurrentTime.bind(this)); On 2014/10/06 16:00:54, vsevik wrote: > I think polling this on each animation frame creates too much load on the > devtools connection. You could use WebInspector.Throttler to poll the time each > 100ms and you could also use local time to emulate smooth slider position change > between these requests if needed. > > Also I don't see how you exit from this time request loop, you should do this > when the selected node is changed. > > We might want to change this in future but I think for now we should move all > animation related stuff to a separate StylesSidebarPane tab and only poll > animation state while this tab is selected. Done. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/AnimationSection.js:125: setAnimationPlayer: function(p) On 2014/10/06 16:00:55, vsevik wrote: > This should be private (name should start with underscore) Done. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/AnimationSection.js:132: if (p instanceof WebInspector.DOMModel.AnimationPlayer) { On 2014/10/06 16:00:54, vsevik wrote: > Why do you need this check? It seems redundant Done. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/StylesSidebarPane.js:525: separatorElement.createTextChild(WebInspector.UIString("Animation") + " " + player.id()); On 2014/10/03 12:43:55, caseq wrote: > Should we use name when available? Done. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/StylesSidebarPane.js:526: this._sectionsContainer.appendChild(separatorElement); On 2014/10/03 12:43:55, caseq wrote: > nit: var separatorElement = this._sectionsContainer.createChild("div", > "sidebar-separator"); Done. https://codereview.chromium.org/620783002/diff/60001/Source/devtools/front_en... Source/devtools/front_end/elements/StylesSidebarPane.js:537: function keyframeStylesCallback(anchorElement, keyframeStyles) { On 2014/10/03 12:43:55, caseq wrote: > style: { => next line Done.
I have removed web anim keyframe view from this patch. PTAL.
https://codereview.chromium.org/620783002/diff/120001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/620783002/diff/120001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.h:266: static String playerId(AnimationPlayer& player) { return String::number(player.sequenceNumber()); } I don't think we should inline it. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/elements/AnimationsSidebarPane.js (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:50: if (this._selectedNode == node) { We use === https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:51: for (var i = 0; i < this._animationSections.length; ++i) { no need to use {} for one line loops / ifs https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:91: * @private we do not use @private annotations. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:123: } Please add a blank line after a funciton https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:174: this._pauseButton.title = WebInspector.UIString("Play animation"); this._pauseButton.title = paused ? WebInspector.UIString("Play animation") : ...; https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/sdk/DOMModel.js (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/sdk/DOMModel.js:2067: return new WebInspector.DOMModel.AnimationNode(this.target(), this._payload.animation); This could be done in constructor to avoid creation of duplicating AnimationNode objects. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/sdk/DOMModel.js:2111: callback(currentTime, isRunning); You might want to add console.error calls when error is not empty. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/sdk/DOMModel.js:2135: payload: function() I don't think this is needed https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2115: { "name": "paused", "type": "boolean", "description": "<code>AnimationPlayer</code>'s paused." }, s/paused/paused state/ https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2116: { "name": "finished", "type": "boolean", "description": "<code>AnimationPlayer</code>'s finished." }, ditto https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2117: { "name": "playbackRate", "type": "number", "description": "<code>AnimationPlayer</code>'s playbackRate." }, playback rate https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2118: { "name": "startTime", "type": "number", "description": "<code>AnimationPlayer</code>'s current time." }, start time https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2120: { "name": "animation", "$ref": "DOM.AnimationNode", "description": "<code>AnimationPlayer</code>'s source animation." } I think naming it source better matches the spec. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2120: { "name": "animation", "$ref": "DOM.AnimationNode", "description": "<code>AnimationPlayer</code>'s source animation." } source animation node
dstockwell@chromium.org changed reviewers: + dstockwell@chromium.org
https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2115: { "name": "paused", "type": "boolean", "description": "<code>AnimationPlayer</code>'s paused." }, Drive by: AnimationPlayer doesn't have paused/finished properties anymore, just playState.
Let's extract all animations related methods to a specific (hidden) Animation domain in protocol in move the related methods / classes on front-end to AnimationModel.js
I've refactored the code by introducing a new hidden Animation domain as suggested and moved relevant code. I've also addressed all other review comments. PTAL, particularly at InspectorAnimationAgent and InspectorController since there were some parts I was unsure about. eg. is it appropriate that the AnimationAgent is a deferred agent? https://codereview.chromium.org/620783002/diff/120001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/620783002/diff/120001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.h:266: static String playerId(AnimationPlayer& player) { return String::number(player.sequenceNumber()); } On 2014/10/14 09:08:08, vsevik wrote: > I don't think we should inline it. Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/elements/AnimationsSidebarPane.js (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:50: if (this._selectedNode == node) { On 2014/10/14 09:08:08, vsevik wrote: > We use === Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:51: for (var i = 0; i < this._animationSections.length; ++i) { On 2014/10/14 09:08:09, vsevik wrote: > no need to use {} for one line loops / ifs Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:91: * @private On 2014/10/14 09:08:08, vsevik wrote: > we do not use @private annotations. Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:123: } On 2014/10/14 09:08:08, vsevik wrote: > Please add a blank line after a funciton Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/elements/AnimationsSidebarPane.js:174: this._pauseButton.title = WebInspector.UIString("Play animation"); On 2014/10/14 09:08:09, vsevik wrote: > this._pauseButton.title = paused ? WebInspector.UIString("Play animation") : > ...; Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/sdk/DOMModel.js (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/sdk/DOMModel.js:2067: return new WebInspector.DOMModel.AnimationNode(this.target(), this._payload.animation); On 2014/10/14 09:08:09, vsevik wrote: > This could be done in constructor to avoid creation of duplicating AnimationNode > objects. Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/sdk/DOMModel.js:2111: callback(currentTime, isRunning); On 2014/10/14 09:08:09, vsevik wrote: > You might want to add console.error calls when error is not empty. Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/sdk/DOMModel.js:2135: payload: function() On 2014/10/14 09:08:09, vsevik wrote: > I don't think this is needed Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2115: { "name": "paused", "type": "boolean", "description": "<code>AnimationPlayer</code>'s paused." }, On 2014/10/14 09:08:09, vsevik wrote: > s/paused/paused state/ Acknowledged. Changed to playState instead. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2116: { "name": "finished", "type": "boolean", "description": "<code>AnimationPlayer</code>'s finished." }, On 2014/10/14 09:08:09, vsevik wrote: > ditto Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2117: { "name": "playbackRate", "type": "number", "description": "<code>AnimationPlayer</code>'s playbackRate." }, On 2014/10/14 09:08:09, vsevik wrote: > playback rate Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2118: { "name": "startTime", "type": "number", "description": "<code>AnimationPlayer</code>'s current time." }, On 2014/10/14 09:08:09, vsevik wrote: > start time Done. https://codereview.chromium.org/620783002/diff/120001/Source/devtools/protoco... Source/devtools/protocol.json:2120: { "name": "animation", "$ref": "DOM.AnimationNode", "description": "<code>AnimationPlayer</code>'s source animation." } On 2014/10/14 09:08:09, vsevik wrote: > source animation node Done.
lgtm with a few nits https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... File Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorAnimationAgent.cpp:4: // found in the LICENSE file. nit: I don't think lines 1 and 5 are necessary https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorAnimationAgent.cpp:35: Please remove extra line here https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... File Source/core/inspector/InspectorAnimationAgent.h (right): https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorAnimationAgent.h:4: * found in the LICENSE file. // Copyright 2014 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. https://codereview.chromium.org/620783002/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/sdk/AnimationModel.js (right): https://codereview.chromium.org/620783002/diff/160001/Source/devtools/front_e... Source/devtools/front_end/sdk/AnimationModel.js:38: } Please add an empty line before and after functions
https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... File Source/core/inspector/InspectorAnimationAgent.cpp (right): https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorAnimationAgent.cpp:4: // found in the LICENSE file. On 2014/10/16 08:13:52, vsevik wrote: > nit: I don't think lines 1 and 5 are necessary Done. https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorAnimationAgent.cpp:35: On 2014/10/16 08:13:52, vsevik wrote: > Please remove extra line here Done. https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... File Source/core/inspector/InspectorAnimationAgent.h (right): https://codereview.chromium.org/620783002/diff/160001/Source/core/inspector/I... Source/core/inspector/InspectorAnimationAgent.h:4: * found in the LICENSE file. On 2014/10/16 08:13:52, vsevik wrote: > // Copyright 2014 The Chromium Authors. All rights reserved. > // Use of this source code is governed by a BSD-style license that can be > // found in the LICENSE file. Done. https://codereview.chromium.org/620783002/diff/160001/Source/devtools/front_e... File Source/devtools/front_end/sdk/AnimationModel.js (right): https://codereview.chromium.org/620783002/diff/160001/Source/devtools/front_e... Source/devtools/front_end/sdk/AnimationModel.js:38: } On 2014/10/16 08:13:52, vsevik wrote: > Please add an empty line before and after functions Done.
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/620783002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as 183847 |
