Move core/platform/animation files

Move core/platform/animation files

CSS/Style specific animation classes are moved to core/animation, the rest to platform/animation.

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

[rwlbuis, 2013-12-30 23:08:21]

I just had to move two enums out of RenderStyleConstants.h for this to work,
also renaming them slightly. I also wonder if this all belongs in platform/ or
it could be merged into core/animation...

[rwlbuis, 2013-12-31 16:20:34]

On 2013/12/30 23:40:37, eseidel wrote:

Nothing? :)

I now see that something similar was attempted before, though just a subset
move:

https://codereview.chromium.org/61753010/

We should choose one approach and close any obsolete patches, whatever we end up
choosing IMHO, otherwise this gets too confusing.

[Stephen Chennney, 2014-01-02 15:28:36]

I stand by my comment on the previous patch, which was apparently never followed
up on. That is, move CSS specific stuff into core/animation and move the rest
into platform.

I didn't look carefully at your patch, so I'm not sure if your patch does what I
would like, but I do prefer your move-it-all-at-once approach.

We'll close any overlapping patches as you suggest.

[rwlbuis, 2014-01-02 17:34:46]

On 2014/01/02 15:28:36, Stephen Chenney wrote:
> I stand by my comment on the previous patch, which was apparently never
followed
> up on. That is, move CSS specific stuff into core/animation and move the rest
> into platform.
> 
> I didn't look carefully at your patch, so I'm not sure if your patch does what
I
> would like, but I do prefer your move-it-all-at-once approach.
> 
> We'll close any overlapping patches as you suggest.

Sounds good! I went ahead and did that with the second patch. The third patch is
a slight variation where the TimingFunction unit tests goes into
platform/animation along with TimingFunction.h. It does have to export some
stuff that way since some unit tests in core/animation rely on it, but I do
think the unit test does belong alongside the actual class it is testing.

Let's see what the win_blink_rel trybot says, it should be the most sensitive to
export problems.

[Stephen Chennney, 2014-01-02 19:02:22]

On 2014/01/02 17:34:46, rwlbuis wrote:
> On 2014/01/02 15:28:36, Stephen Chenney wrote:
> > I stand by my comment on the previous patch, which was apparently never
> followed
> > up on. That is, move CSS specific stuff into core/animation and move the
rest
> > into platform.
> > 
> > I didn't look carefully at your patch, so I'm not sure if your patch does
what
> I
> > would like, but I do prefer your move-it-all-at-once approach.
> > 
> > We'll close any overlapping patches as you suggest.
> 
> Sounds good! I went ahead and did that with the second patch. The third patch
is
> a slight variation where the TimingFunction unit tests goes into
> platform/animation along with TimingFunction.h. It does have to export some
> stuff that way since some unit tests in core/animation rely on it, but I do
> think the unit test does belong alongside the actual class it is testing.
> 
> Let's see what the win_blink_rel trybot says, it should be the most sensitive
to
> export problems.

I'm happy with this approach and thanks for fixing the style issues too. There
are some export issues to fix, so I'll hold off on the lg.. for now.

[Stephen Chennney, 2014-01-02 19:02:43]

https://codereview.chromium.org/112463008/diff/460001/Source/platform/animati...
File Source/platform/animation/TimingFunctionTestHelper.h (right):

https://codereview.chromium.org/112463008/diff/460001/Source/platform/animati...
Source/platform/animation/TimingFunctionTestHelper.h:46: void PrintTo(const
LinearTimingFunction&, ::std::ostream*);
My guess is that you need PLATFORM_EXPORT for all of these.

[rwlbuis, 2014-01-02 22:25:17]

On 2014/01/02 19:02:43, Stephen Chenney wrote:
>
https://codereview.chromium.org/112463008/diff/460001/Source/platform/animati...
> File Source/platform/animation/TimingFunctionTestHelper.h (right):
> 
>
https://codereview.chromium.org/112463008/diff/460001/Source/platform/animati...
> Source/platform/animation/TimingFunctionTestHelper.h:46: void PrintTo(const
> LinearTimingFunction&, ::std::ostream*);
> My guess is that you need PLATFORM_EXPORT for all of these.

I tried with patch #4, alas the problem remains. However I noticed that patch #3
and #4 share the same problem according to win_blink. When running blink
platform unit tests it shows:

ASSERTION FAILED: RuntimeEnabledFeatures::webAnimationsCSSEnabled()

I was able to reproduce this on my linux Debug build. I am guessing this flag is
not enabled for the platform unit tests but is for the webkit unit tests. So it
seems patch #2 is the way to go after all...

[Stephen Chennney, 2014-01-06 20:29:57]

We've addressed the need for core or web features in unit tests by placing the
test code in platform but building it via the web_unit_tests set in web.gypi.
This is done for some image decoders, for example.

I would like the following:
- Put Timing.h and it's tests back in platform
- Create a set in blink_platform.gypi called platform_web_unittest_files or the
like
- Add Timing... and the other tests from platform currently in
web.gypi:web_unittest_files to this set
- Include platform_web_unittest_files in web.gyp and web_tests.gyp in the places
where core_unittest_files is already included

Does all that address the problems?

[rwlbuis, 2014-01-06 23:01:02]

On 2014/01/06 20:29:57, Stephen Chenney wrote:
> We've addressed the need for core or web features in unit tests by placing the
> test code in platform but building it via the web_unit_tests set in web.gypi.
> This is done for some image decoders, for example.
> 
> I would like the following:
> - Put Timing.h and it's tests back in platform
> - Create a set in blink_platform.gypi called platform_web_unittest_files or
the
> like
> - Add Timing... and the other tests from platform currently in
> web.gypi:web_unittest_files to this set
> - Include platform_web_unittest_files in web.gyp and web_tests.gyp in the
places
> where core_unittest_files is already included
> 
> Does all that address the problems?

That makes a lot of sense, I implemented this in Patch set 6. I verified that
the webkit_unit_tests work fine with the new added animation unit tests.

[dstockwell, 2014-01-06 23:13:17]

If CSSAnimationData/List are moving to core/animation, they should be in
core/animation/css.

I'm a little confused why CompositorAnimationsTest.cpp moved to
Source/platform/animation. CompositorAnimations.cpp is in core/animation.

[rwlbuis, 2014-01-07 16:48:37]

On 2014/01/06 23:13:17, dstockwell wrote:
> If CSSAnimationData/List are moving to core/animation, they should be in
> core/animation/css.

Sounds good, fixed with my latest patch.

> I'm a little confused why CompositorAnimationsTest.cpp moved to
> Source/platform/animation. CompositorAnimations.cpp is in core/animation.

I hope it is not needed, my latest patch keeps them where they were. I'll watch
the trybots for symbol export problems...

[Stephen Chennney, 2014-01-07 16:51:07]

We'll need a web owner review for the gyp changes. I hope I correctly channeled
the web owner's thoughts (and jamesr).

[abarth-chromium, 2014-01-07 17:35:22]

Source/web LGTM with some comments explaining that this pattern isn't a good one
to replicate.

https://codereview.chromium.org/112463008/diff/720001/Source/web/web.gyp
File Source/web/web.gyp (right):

https://codereview.chromium.org/112463008/diff/720001/Source/web/web.gyp#newc...
Source/web/web.gyp:120: '<@(platform_web_unittest_files)',
Can you add a FIXME about wanting to remove this line?  We'd much rather run
these unit tests outside webkit.dll.  I'd also add a comment to the definition
of this variable explaining that these are legacy unit tests and folks shouldn't
add more.

[Stephen Chennney, 2014-01-08 15:58:33]

LGTM from me too. But ...
(1) Please add the comment as Adam requests.
(2) Add the other ../platform/... tests that currently appear in web.gypi to
blink_platform.gypi so that everything is together and any subsequent fix will
address them all.
      '../platform/graphics/DeferredImageDecoderTest.cpp',
      '../platform/graphics/ImageDecodingStoreTest.cpp',
      '../platform/graphics/ImageFrameGeneratorTest.cpp',
      '../platform/graphics/test/MockImageDecoder.h',

[rwlbuis, 2014-01-08 16:55:43]

On 2014/01/08 15:58:33, Stephen Chenney wrote:
> LGTM from me too. But ...
> (1) Please add the comment as Adam requests.
> (2) Add the other ../platform/... tests that currently appear in web.gypi to
> blink_platform.gypi so that everything is together and any subsequent fix will
> address them all.
>       '../platform/graphics/DeferredImageDecoderTest.cpp',
>       '../platform/graphics/ImageDecodingStoreTest.cpp',
>       '../platform/graphics/ImageFrameGeneratorTest.cpp',
>       '../platform/graphics/test/MockImageDecoder.h',

Thanks, makes sense, I did all that in CL #8. Will commit soon :)

[commit-bot: I haz the power, 2014-01-08 19:04:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/112463008/860001

[commit-bot: I haz the power, 2014-01-08 20:40:56]

Change committed as 164710