-- 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.
Created attachment 298823 [details] Patch
Created attachment 298829 [details] Patch
Created attachment 298832 [details] Patch
Created attachment 298838 [details] Patch
Created attachment 299102 [details] Patch
Created attachment 299109 [details] Patch
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?
Created attachment 299110 [details] Patch
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.
Created attachment 299147 [details] Patch
Created attachment 299150 [details] Patch
Created attachment 299151 [details] Patch
Created attachment 299154 [details] Patch
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?
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.
This patch does not apply anymore. The code has changed a lot. So resolve it as INVALID.
Actually, using std::optional<> would be nice rather than keeping two sets of instance variables. Re-opening this and rephrasing.
Re-opening to fix.
Might be even better to use Markable here.
Actually, we should go beyond tracking which properties are set and also improve how we track which properties are filled.
Pull request: https://github.com/WebKit/WebKit/pull/13750