1418220 - Drop AnimationUtils::IsCoreAPIEnabled(ForCaller) and use nsContentUtils::AnimationsAPICoreEnabled instead

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Comment 1

[Brian Birtles (:birtles)]

Or perhaps nsDocument::IsWebAnimationsEnabled is better.

Comment 2

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=632416a60b34a13d9c5f31591a86e8495ac540ed

Comment 3

[Brian Birtles (:birtles)]

Attachment: Bug 1418220 - Drop AnimationUtils::IsCoreAPIEnabled(ForCaller) and use nsContentUtils::AnimationsAPICoreEnabled / nsDocument::IsWebAnimationsEnabled instead;

The difference between nsDocument::IsWebAnimationsEnabled and
nsContentUtils::AnimationsAPICoreEnabled is that the former checks the caller
type and treats the preference as set for system callers which is particularly
needed for enabling things like the getProperties() API for DevTools etc.

Generally in API-facing call sites we have a JS context / CallerType and so we
want to distinguish between system callers and non-system callers. However, for
a few internal uses--specifically filling-in missing keyframes--we don't care
about the caller type and always follow the pref setting.

That may or not be quite what we want, but this patch doesn't change that except
for one call site: KeyframeUtils::GetKeyframesFromObject. This patch changes
GetKeyframesFromObject from *not* checking the caller type to checking the
caller type. That seems to be the correct behavior here since this is called
from KeyframeEffect::SetKeyframes() (i.e. a JS API-facing call site) where we
*should* enable the full API when the caller is chrome code.

Review commit: https://reviewboard.mozilla.org/r/201156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201156/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8929983 [details]
Bug 1418220 - Drop AnimationUtils::IsCoreAPIEnabled(ForCaller) and use nsContentUtils::AnimationsAPICoreEnabled / nsDocument::IsWebAnimationsEnabled instead;

https://reviewboard.mozilla.org/r/201156/#review206248

Nice!

::: commit-message-dd08f:17
(Diff revision 1)
> +
> +That may or not be quite what we want, but this patch doesn't change that except
> +for one call site: KeyframeUtils::GetKeyframesFromObject. This patch changes
> +GetKeyframesFromObject from *not* checking the caller type to checking the
> +caller type. That seems to be the correct behavior here since this is called
> +from KeyframeEffect::SetKeyframes() (i.e. a JS API-facing call site) where we

It might be better to write arguments for the SetKeyframes() to make it clear that the function is only used in JS, i.e.

SetKeyframes(JSContext*, JS::Handle<JSObject*>, ErrorResult&)

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8929983 [details]
Bug 1418220 - Drop AnimationUtils::IsCoreAPIEnabled(ForCaller) and use nsContentUtils::AnimationsAPICoreEnabled / nsDocument::IsWebAnimationsEnabled instead;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201156/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b828f7a6c2bf
Drop AnimationUtils::IsCoreAPIEnabled(ForCaller) and use nsContentUtils::AnimationsAPICoreEnabled / nsDocument::IsWebAnimationsEnabled instead; r=hiro

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b828f7a6c2bf