Web Animations CSS: Split AnimatableNumber into AnimatableDouble and AnimatableLength

Web Animations CSS: Split AnimatableNumber into AnimatableDouble and AnimatableLength

AnimatableNumber currently does too many things. Splitting the class up
will allow us to subclass number functionality and make the overall
implementation cleaner.

BUG=257591
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158686

[dstockwell, 2013-09-30 22:01:57]

lgtm

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/css/...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/css/...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:83: case Relative:
Need a comment about why we don't expect this.

[alancutter (OOO until 2018), 2013-09-30 23:04:43]

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/css/...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/css/...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:83: case Relative:
On 2013/09/30 22:01:58, dstockwell wrote:
> Need a comment about why we don't expect this.

Done.

[Steve Block, 2013-09-30 23:21:30]

lgtm

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLength.h (right):

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
Source/core/animation/AnimatableLength.h:46: class AnimatableLength : public
AnimatableValue {
It would be good to retain the class-level comment, here and in
AnimatableDouble.

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
Source/core/animation/AnimatableLength.h:49: UnitTypeLength,
This seems like a bad name, now that the class is AnimatableLength. These are
all ways of specifying a length. Maybe UnitTypeFixed/Constant/Simple?

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
File Source/core/animation/AnimatableValue.h (right):

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
Source/core/animation/AnimatableValue.h:58: bool isVisibility() const { return
type() == TypeVisibility; }
These should be in alpha order

[Timothy Loh, 2013-09-30 23:27:59]

lgtm with tiny comments..

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableDouble.cpp (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableDouble.cpp:53: // Optimization for adding with
0.
You might want to add tests for taking the two branches here

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableDouble.h (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableDouble.h:36: #include "wtf/MathExtras.h"
Is this used here?

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableDoubleTest.cpp (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableDoubleTest.cpp:59: EXPECT_EQ(10,
AnimatableDouble::create(10)->toDouble());
I'm sure the class works fine, but maybe add a couple of tests here with
doubles, given that we're testing toDouble...

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableLength.h (left):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableLength.h:90: ASSERT(m_unitType !=
UnitTypeInvalid);
Why is this line removed?

[alancutter (OOO until 2018), 2013-10-01 09:32:24]

Thanks for the extra review.

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableDouble.cpp (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableDouble.cpp:53: // Optimization for adding with
0.
On 2013/09/30 23:27:59, Timothy Loh wrote:
> You might want to add tests for taking the two branches here

Done.

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableDouble.h (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableDouble.h:36: #include "wtf/MathExtras.h"
On 2013/09/30 23:27:59, Timothy Loh wrote:
> Is this used here?

Removed.

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableDoubleTest.cpp (right):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableDoubleTest.cpp:59: EXPECT_EQ(10,
AnimatableDouble::create(10)->toDouble());
On 2013/09/30 23:27:59, Timothy Loh wrote:
> I'm sure the class works fine, but maybe add a couple of tests here with
> doubles, given that we're testing toDouble...

Done.

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
File Source/core/animation/AnimatableLength.h (left):

https://codereview.chromium.org/25082007/diff/8001/Source/core/animation/Anim...
Source/core/animation/AnimatableLength.h:90: ASSERT(m_unitType !=
UnitTypeInvalid);
On 2013/09/30 23:27:59, Timothy Loh wrote:
> Why is this line removed?

Mistake. Line put back.

[commit-bot: I haz the power, 2013-10-01 09:33:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/25082007/23001

[commit-bot: I haz the power, 2013-10-01 09:33:29]

Failed to apply patch for Source/core/animation/AnimatableValue.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/animation/AnimatableValue.h
  Hunk #1 FAILED at 46.
  Hunk #2 FAILED at 58.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/animation/AnimatableValue.h.rej

Patch:       Source/core/animation/AnimatableValue.h
Index: Source/core/animation/AnimatableValue.h
diff --git a/Source/core/animation/AnimatableValue.h
b/Source/core/animation/AnimatableValue.h
index
c06e6bb314ec13ce2f360d49db7a131557fd5cb2..cfff495a618b7dd73490bfa5d6bb6eedabf514c4
100644
--- a/Source/core/animation/AnimatableValue.h
+++ b/Source/core/animation/AnimatableValue.h
@@ -46,11 +46,12 @@ public:
     // For noncommutative values read add(A, B) to mean the value A with B
composed onto it.
     static PassRefPtr<AnimatableValue> add(const AnimatableValue*, const
AnimatableValue*);
 
+    bool isDouble() const { return type() == TypeDouble; }
     bool isColor() const { return type() == TypeColor; }
     bool isImage() const { return type() == TypeImage; }
+    bool isLength() const { return type() == TypeLength; }
     bool isLengthBox() const { return type() == TypeLengthBox; }
     bool isLengthSize() const { return type() == TypeLengthSize; }
-    bool isNumber() const { return type() == TypeNumber; }
     bool isNeutral() const { return type() == TypeNeutral; }
     bool isTransform() const { return type() == TypeTransform; }
     bool isUnknown() const { return type() == TypeUnknown; }
@@ -58,12 +59,13 @@ public:
 
 protected:
     enum AnimatableType {
+        TypeDouble,
         TypeColor,
         TypeImage,
+        TypeLength,
         TypeLengthBox,
         TypeLengthSize,
         TypeNeutral,
-        TypeNumber,
         TypeTransform,
         TypeUnknown,
         TypeVisibility,

[alancutter (OOO until 2018), 2013-10-02 00:01:18]

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLength.h (right):

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
Source/core/animation/AnimatableLength.h:46: class AnimatableLength : public
AnimatableValue {
On 2013/09/30 23:21:31, Steve Block wrote:
> It would be good to retain the class-level comment, here and in
> AnimatableDouble.

Done.

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
Source/core/animation/AnimatableLength.h:49: UnitTypeLength,
On 2013/09/30 23:21:31, Steve Block wrote:
> This seems like a bad name, now that the class is AnimatableLength. These are
> all ways of specifying a length. Maybe UnitTypeFixed/Constant/Simple?

I agree, UnitTypeFixed sounds okay but in the context of zooming I don't think
it's suitable here (the value is yet to be zoomed unlike a Length). Instead I've
renamed it as UnitTypePixels.

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
File Source/core/animation/AnimatableValue.h (right):

https://codereview.chromium.org/25082007/diff/14001/Source/core/animation/Ani...
Source/core/animation/AnimatableValue.h:58: bool isVisibility() const { return
type() == TypeVisibility; }
On 2013/09/30 23:21:31, Steve Block wrote:
> These should be in alpha order

Done.

[commit-bot: I haz the power, 2013-10-02 00:02:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/25082007/32001

[commit-bot: I haz the power, 2013-10-02 04:57:10]

Change committed as 158686

[caseq, 2013-10-02 05:54:04]

On 2013/10/02 04:57:10, I haz the power (commit-bot) wrote:
> Change committed as 158686

Reverted as 158688 for breaking build. Sample stdio:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/19...

FAILED: g++ -MMD -MF
obj/third_party/WebKit/Source/core/css/resolver/webcore_remaining.AnimatedStyleBuilder.o.d
-DANGLE_DX11 -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD
-DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_NSS=1 -DUSE_X11=1
-DENABLE_ONE_CLICK_SIGNIN -DGTK_DISABLE_SINGLE_INCLUDES=1 -DUSE_XI2_MT=2
-DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS
-DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1
-DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
-DENABLE_GOOGLE_NOW=1 -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1
-DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DGL_GLEXT_PROTOTYPES
-DBLINK_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_CSS3_TEXT=0
-DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_REGIONS=1
-DENABLE_CUSTOM_SCHEME_HANDLER=0 -DENABLE_ENCRYPTED_MEDIA_V2=1
-DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_ICON_LOADING=0
-DENABLE_GDI_FONTS_ON_WINDOWS=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1
-DENABLE_CALENDAR_PICKER=1 -DENABLE_INPUT_SPEECH=1
-DENABLE_INPUT_MULTIPLE_FIELDS_UI=1 -DENABLE_LEGACY_NOTIFICATIONS=1
-DENABLE_MEDIA_CAPTURE=0 -DENABLE_NAVIGATOR_CONTENT_UTILS=1
-DENABLE_ORIENTATION_EVENTS=0 -DENABLE_WEB_AUDIO=1 -DWTF_USE_WEBAUDIO_FFMPEG=1
-DENABLE_OPENTYPE_VERTICAL=1 -DENABLE_DEFAULT_RENDER_THEME=1
-DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DSK_ENABLE_INST_COUNT=0
-DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_USE_POSIX_THREADS
-DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DCHROME_PNG_WRITE_SUPPORT
-DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../../third_party/khronos -I../../gpu -I../..
-I../../third_party/WebKit -I../../third_party/WebKit/Source -Igen/blink
-Igen/blink/bindings -I../../third_party/angle_dx11/include/GLSLANG
-I../../third_party/ffmpeg -I../../third_party/icu/source/i18n
-I../../third_party/icu/source/common -I../../third_party/skia/src/core
-I../../skia/config -I../../third_party/skia/include/config
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext
-I../../third_party/iccjpeg -I../../third_party/libpng
-I../../third_party/libwebp -I../../third_party/libxml/linux/include
-I../../third_party/libxml/src/include -I../../third_party/libxslt
-I../../third_party/npapi -I../../third_party/npapi/bindings
-I../../third_party/ots/include -I../../third_party/qcms/src
-I../../third_party/sqlite -I../../third_party/zlib -I../../v8/include
-I../../third_party/libjpeg_turbo -fstack-protector --param=ssp-buffer-size=4
-pthread -fno-exceptions -fno-strict-aliasing -Wno-unused-parameter
-Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -pthread
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include
-I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0
-I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/pixman-1
-I/usr/include/freetype2 -I/usr/include/libpng12 -fno-strict-aliasing
-Wno-format -Wno-unused-result -O2 -fno-ident -fdata-sections
-ffunction-sections -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wno-c++0x-compat -Wno-deprecated  -c
../../third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp -o
obj/third_party/WebKit/Source/core/css/resolver/webcore_remaining.AnimatedStyleBuilder.o
../../third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp: In
static member function 'static void
WebCore::AnimatedStyleBuilder::applyProperty(WebCore::CSSPropertyID,
WebCore::StyleResolverState&, const WebCore::AnimatableValue*)':
../../third_party/WebKit/Source/core/css/resolver/AnimatedStyleBuilder.cpp:182:72:
error: 'toAnimatableNumber' was not declared in this scope