Bug 138206 – Remove create() factory function in FooAnimationValue classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 240647 [details]
Patch

[Gyuyoung Kim]

Created attachment 240719 [details]
Patch

[Gyuyoung Kim]

Created attachment 240724 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 240724 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.h:91
> +    FloatAnimationValue(double keyTime, float value, TimingFunction* timingFunction = nullptr)
> +        : AnimationValue(keyTime, timingFunction)
> +        , m_value(value)
>      {
> -        return adoptPtr(new FloatAnimationValue(keyTime, value, timingFunction));
>      }

Doesn't this change expose the copy constructor as a public member function?

[Gyuyoung Kim]

(In reply to comment #4)
> Comment on attachment 240724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240724&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:91
> > +    FloatAnimationValue(double keyTime, float value, TimingFunction* timingFunction = nullptr)
> > +        : AnimationValue(keyTime, timingFunction)
> > +        , m_value(value)
> >      {
> > -        return adoptPtr(new FloatAnimationValue(keyTime, value, timingFunction));
> >      }
> 
> Doesn't this change expose the copy constructor as a public member function?


When I try to create FloatAnimationValue through copy constructor, there is no build error. If you have concern about it, could you let me know it ?

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 240724 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240724&action=review
> > 
> > > Source/WebCore/platform/graphics/GraphicsLayer.h:91
> > > +    FloatAnimationValue(double keyTime, float value, TimingFunction* timingFunction = nullptr)
> > > +        : AnimationValue(keyTime, timingFunction)
> > > +        , m_value(value)
> > >      {
> > > -        return adoptPtr(new FloatAnimationValue(keyTime, value, timingFunction));
> > >      }
> > 
> > Doesn't this change expose the copy constructor as a public member function?
> 
> 
> When I try to create FloatAnimationValue through copy constructor, there is
> no build error.

I succeed to create FloatAnimationValue object using copy constructor as below,

 FloatAnimationValue* fValue = new FloatAnimationValue(key, keyframeStyle->opacity(), tf);
 FloatAnimationValue* copyValue(fValue);

It looks the change can support copy constructor.

[Gyuyoung Kim]

Created attachment 241266 [details]
Rebased

[Darin Adler]

Comment on attachment 241266 [details]
Rebased

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:160
> +    std::unique_ptr<AnimationValue> intialValue = std::make_unique<FloatAnimationValue>(0, 1);
> +    fadeKeyframes.insert(WTF::move(intialValue));

I think this would read better without a local variable:

    fadeKeyframes.insert(std::make_unique<FloatAnimationValue>(0, 1));

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:163
> +    std::unique_ptr<AnimationValue> finalValue = std::make_unique<FloatAnimationValue>(0.25, 0);
> +    fadeKeyframes.insert(WTF::move(finalValue));

I think this would read better without a local variable:

    fadeKeyframes.insert(std::make_unique<FloatAnimationValue>(0.25, 0));

[Gyuyoung Kim]

Created attachment 241275 [details]
Patch for landing

[Gyuyoung Kim]

(In reply to comment #8)
> Comment on attachment 241266 [details]
> Rebased
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241266&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:160
> > +    std::unique_ptr<AnimationValue> intialValue = std::make_unique<FloatAnimationValue>(0, 1);
> > +    fadeKeyframes.insert(WTF::move(intialValue));
> 
> I think this would read better without a local variable:
> 
>     fadeKeyframes.insert(std::make_unique<FloatAnimationValue>(0, 1));
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:163
> > +    std::unique_ptr<AnimationValue> finalValue = std::make_unique<FloatAnimationValue>(0.25, 0);
> > +    fadeKeyframes.insert(WTF::move(finalValue));
> 
> I think this would read better without a local variable:
> 
>     fadeKeyframes.insert(std::make_unique<FloatAnimationValue>(0.25, 0));

Thank you for review ! Fixed all.

[WebKit Commit Bot]

Comment on attachment 241275 [details]
Patch for landing

Clearing flags on attachment: 241275

Committed r175799: <http://trac.webkit.org/changeset/175799>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.