Bug 167041 – Make tracking which Animation properties are set or filled more efficient

• Status: REOPENED
• Component: Animations

[Said Abou-Hallawa]

-- Shorten the enum names and use enum class instead.
-- Use std::optional for the members instead of tracking setting them variable by a set of booleans.

[Said Abou-Hallawa]

Created attachment 298823 [details]
Patch

[Said Abou-Hallawa]

Created attachment 298829 [details]
Patch

[Said Abou-Hallawa]

Created attachment 298832 [details]
Patch

[Said Abou-Hallawa]

Created attachment 298838 [details]
Patch

[Said Abou-Hallawa]

Created attachment 299102 [details]
Patch

[Said Abou-Hallawa]

Created attachment 299109 [details]
Patch

[Jon Lee]

Comment on attachment 299109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299109&action=review

> Source/WebCore/css/makeprop.pl:604
> +    $setterContent .= $indent . "list.animation(0).setterContent(Animation::Mode::All);\n";

Is this correct?

[Said Abou-Hallawa]

Created attachment 299110 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 299109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299109&action=review

>> Source/WebCore/css/makeprop.pl:604
>> +    $setterContent .= $indent . "list.animation(0).setterContent(Animation::Mode::All);\n";
> 
> Is this correct?

No it is not correct. I do not remember how it happened. It looks like it happened because of either a bad merge or a bad copy/paste.

[Said Abou-Hallawa]

Created attachment 299147 [details]
Patch

[Said Abou-Hallawa]

Created attachment 299150 [details]
Patch

[Said Abou-Hallawa]

Created attachment 299151 [details]
Patch

[Said Abou-Hallawa]

Created attachment 299154 [details]
Patch

[Darin Adler]

Comment on attachment 299154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299154&action=review

> Source/WebCore/platform/animation/Animation.cpp:32
> +    : m_mode(Mode::All)
> +    , m_isNone(false)
> +    , m_nameStyleScopeOrdinal(Style::ScopeOrdinal::Element)

Why not initialize these in the class definition/

> Source/WebCore/platform/animation/Animation.cpp:38
> +    , m_timingFunction(initialTimingFunction())
> +    , m_timingFunctionSet(false)

Could we instead use an optional or use RefPtr and have null mean not set? If keeping a separate boolean, could we initialize in the class definition?

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:530
> +        animationObject->setTimingFunction(*timingFunction.leakRef());

I don’t see how this could be correct. There’s a reason this function is named "leak". Are you sure tis is right?

[Maciej Stachowiak]

Comment on attachment 299154 [details]
Patch

This patch no longer applies (per EWS). I did not review whether it's a good idea, but r- for not applying any more.

[Said Abou-Hallawa]

This patch does not apply anymore. The code has changed a lot. So resolve it as INVALID.

[Antoine Quint]

Actually, using std::optional<> would be nice rather than keeping two sets of instance variables. Re-opening this and rephrasing.

[Antoine Quint]

Re-opening to fix.

[Antoine Quint]

Might be even better to use Markable here.

[Antoine Quint]

Actually, we should go beyond tracking which properties are set and also improve how we track which properties are filled.

[Antoine Quint]

Pull request: https://github.com/WebKit/WebKit/pull/13750