Devtools Animations: Add method to fetch CSS keyframed animations

Devtools Animations: Add method to fetch CSS keyframed animations

This change adds a method in StyleResolver which checks the animation-name
property of an element and returns all CSSOM keyframes rule representations
for that element. This method can then be used in InspectorCSSAgent to propogate
to the front-end and show the keyframes there.

BUG=266276
Committed: https://crrev.com/981e28fe80483dc7edf3687205472c6bae61db79
Cr-Commit-Position: refs/heads/master@{#357041}

[samli, 2015-09-25 23:15:02]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-09-25 23:15:03]

PTAL.

WIP, pretty ugly code, but looking for general comments on structure. eg.
whether or not calling StyleResolver::styleForElement() is fine, or that will
have a large performance cost.

[samli, 2015-09-28 19:27:09]

PTAL.

[samli, 2015-09-29 00:47:09]

samli@chromium.org changed reviewers:
+ lushnikov@chromium.org

[samli, 2015-09-29 00:47:10]

+lushnikov

[samli, 2015-09-30 18:13:58]

PTAL. Split into its own method.

[samli, 2015-10-05 20:31:30]

ping

[lushnikov, 2015-10-06 23:01:34]

devtools lgtm

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:889: //
TODO(samli): Repeat for all pseudo elements.
why do you want this to be for pseudo elements?

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:1523: for
(unsigned j = 0; j < rule->length(); j++)
++j

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:3106: { "name": "name", "type":
"string", "description": "Animation name." },
nit: "name" -> "animationName"

[samli, 2015-10-06 23:31:45]

samli@chromium.org changed reviewers:
+ dstockwell@chromium.org

[samli, 2015-10-06 23:31:46]

+ dstockwell for core/css

[dstockwell, 2015-10-07 00:10:09]

dstockwell@chromium.org changed reviewers:
+ timloh@chromium.org

[samli, 2015-10-08 17:37:32]

ping timloh/dstockwell for core/css

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:889: //
TODO(samli): Repeat for all pseudo elements.
On 2015/10/06 at 23:01:34, lushnikov wrote:
> why do you want this to be for pseudo elements?

Pseudo elements can have CSS animations as well. Since we show pseudo elements
in the Styles pane, we should probably show any associated animations as well.

/ I guess we should support it when you click a pseudo element as well.

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:1523: for
(unsigned j = 0; j < rule->length(); j++)
On 2015/10/06 at 23:01:34, lushnikov wrote:
> ++j

Done.

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1371723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:3106: { "name": "name", "type":
"string", "description": "Animation name." },
On 2015/10/06 at 23:01:34, lushnikov wrote:
> nit: "name" -> "animationName"

Done.

[samli, 2015-10-12 18:11:06]

ping

[alancutter (OOO until 2018), 2015-10-15 00:11:52]

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

[alancutter (OOO until 2018), 2015-10-15 00:11:53]

https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:88:
WillBeHeapHashMap<const StringImpl*, CSSStyleSheet*> m_keyframesStyleSheetMap;
D:

https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:898:
RefPtrWillBeRawPtr<CSSKeyframesRule> keyframesRule =
findCSSKeyframesRule(element, animationName);
It seems like a lot of added complexity to find the existing wrappers. Can we
just create new CSSOM wrappers for the @keyframes rules?

[samli, 2015-10-15 00:16:07]

https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:898:
RefPtrWillBeRawPtr<CSSKeyframesRule> keyframesRule =
findCSSKeyframesRule(element, animationName);
On 2015/10/15 at 00:11:53, alancutter wrote:
> It seems like a lot of added complexity to find the existing wrappers. Can we
just create new CSSOM wrappers for the @keyframes rules?

Yes but with one caveat. I think when we later have the source ranges, devtools
won't be able to respond to CSSOM changes if we have a different wrapper.
lushnikov@, is that right?

Even so, its probably enough of an edge case that we could do that instead.

[alancutter (OOO until 2018), 2015-10-15 04:05:47]

On 2015/10/15 at 00:16:07, samli wrote:
>
https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:898:
RefPtrWillBeRawPtr<CSSKeyframesRule> keyframesRule =
findCSSKeyframesRule(element, animationName);
> On 2015/10/15 at 00:11:53, alancutter wrote:
> > It seems like a lot of added complexity to find the existing wrappers. Can
we just create new CSSOM wrappers for the @keyframes rules?
> 
> Yes but with one caveat. I think when we later have the source ranges,
devtools won't be able to respond to CSSOM changes if we have a different
wrapper. lushnikov@, is that right?
> 
> Even so, its probably enough of an edge case that we could do that instead.

I'm not sure how source ranges tie into this.
What do you mean by CSSOM changes? Do you mean persistence of the Javascript
wrapper object?

It does seem like enough of an edge case to me to leave out. There's no spec
behaviour to worry about here, we're free to do whatever we consider sensible.

[samli, 2015-10-17 00:43:57]

On 2015/10/15 at 04:05:47, alancutter wrote:
> On 2015/10/15 at 00:16:07, samli wrote:
> >
https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):
> > 
> >
https://codereview.chromium.org/1371723002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:898:
RefPtrWillBeRawPtr<CSSKeyframesRule> keyframesRule =
findCSSKeyframesRule(element, animationName);
> > On 2015/10/15 at 00:11:53, alancutter wrote:
> > > It seems like a lot of added complexity to find the existing wrappers. Can
we just create new CSSOM wrappers for the @keyframes rules?
> > 
> > Yes but with one caveat. I think when we later have the source ranges,
devtools won't be able to respond to CSSOM changes if we have a different
wrapper. lushnikov@, is that right?
> > 
> > Even so, its probably enough of an edge case that we could do that instead.
> 
> I'm not sure how source ranges tie into this.
> What do you mean by CSSOM changes? Do you mean persistence of the Javascript
wrapper object?
> 
> It does seem like enough of an edge case to me to leave out. There's no spec
behaviour to worry about here, we're free to do whatever we consider sensible.

Changed, PTAL.

[alancutter (OOO until 2018), 2015-10-27 23:57:30]

https://codereview.chromium.org/1371723002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/1371723002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:991: return
CSSKeyframesRule::create(keyframesRule, styleSheet);
I'm not happy with how stylesheets are associated with @keyframes in this patch,
let's just use nullptr for the stylesheet for now and tackle this aspect later
if we end up needing it.

[samli, 2015-10-28 20:57:37]

samli@chromium.org changed reviewers:
- dstockwell@chromium.org, pfeldman@chromium.org, timloh@chromium.org

[samli, 2015-10-28 20:57:37]

alancutter PTAL. Removed everything to do with the style sheet and moved
remaining logic to inspector.

[alancutter (OOO until 2018), 2015-10-28 23:41:38]

lgtm

[samli, 2015-10-28 23:57:39]

The CQ bit was checked by samli@chromium.org

[samli, 2015-10-28 23:57:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/1371723002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-10-28 23:58:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371723002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371723002/140001

[commit-bot: I haz the power, 2015-10-29 00:13:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-29 00:13:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[samli, 2015-10-29 00:41:02]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-10-29 00:41:02]

+pfeldman for core/inspector OWNERs

[pfeldman, 2015-10-29 01:21:52]

https://codereview.chromium.org/1371723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/1371723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:813:
RefPtrWillBeRawPtr<StaticCSSRuleList> keyframeRuleList =
StaticCSSRuleList::create();
Why do you populate this intermediate container instead of creating the
typebuilder structure right away?

[samli, 2015-10-29 01:37:35]

https://codereview.chromium.org/1371723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/1371723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:813:
RefPtrWillBeRawPtr<StaticCSSRuleList> keyframeRuleList =
StaticCSSRuleList::create();
On 2015/10/29 at 01:21:52, pfeldman wrote:
> Why do you populate this intermediate container instead of creating the
typebuilder structure right away?

Good point, was doing that this work in core/css before, but moved it here.
Fixed.

[pfeldman, 2015-10-29 22:27:36]

lgtm

[samli, 2015-10-30 02:36:34]

The CQ bit was checked by samli@chromium.org

[samli, 2015-10-30 02:36:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1371723002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-10-30 02:37:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371723002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371723002/160001

[commit-bot: I haz the power, 2015-10-30 03:59:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-30 04:00:10]

Patchset 9 (id:??) landed as
https://crrev.com/981e28fe80483dc7edf3687205472c6bae61db79
Cr-Commit-Position: refs/heads/master@{#357041}