Move TimingFunction.h to platform/

Move TimingFunction.h to platform/

BUG=297477

[pilgrim_google, 2013-11-12 15:14:43]

This doesn't work. I think it's on the right track and it's close to working,
but I'm stuck. It moves TimingFunction and TimingFunctionTestHelperTest to
platform/animation/ , which requires a bunch of changes to
blink_platform_tests.gyp because the test itself depends on
RuntimeEnabledFeatures.h. It compiles but fails to link because of an undefined
operator from TimingFunction.h but I don't see where that is defined anywhere so
I'm stuck. Help?

[Steve Block, 2013-11-12 22:39:20]

+mithro, who wrote the tests for TimingFunction

> It compiles but fails to link because of an undefined
> operator from TimingFunction.h
TimingFunction doesn't have any operators. Do you mean the operators in
TimingFunctionTestHelper.h/cpp?

https://codereview.chromium.org/65303006/diff/1/Source/platform/blink_platfor...
File Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/65303006/diff/1/Source/platform/blink_platfor...
Source/platform/blink_platform.gypi:127:
'animation/TimingFunctionTestHelper.cpp',
TimingFunctionTestHelper.h/cpp are a bunch of utilities for testing
TimingFunction, so shouldn't they be in platform_test_files?

[mithro-old, 2013-11-12 23:37:10]

This might be a stupid question, but why are/should TimingFunction be in
platform/animation rather than core/animation? I'm afraid I don't know the
difference between the two directories.

On 2013/11/12 22:39:20, Steve Block wrote:
> +mithro, who wrote the tests for TimingFunction
> 
> > It compiles but fails to link because of an undefined
> > operator from TimingFunction.h
> TimingFunction doesn't have any operators. Do you mean the operators in
> TimingFunctionTestHelper.h/cpp?

operator== and operator!= were removed from TimingFunction.h by Steve in
https://codereview.chromium.org/60033004/

I added them back as test helper in https://codereview.chromium.org/60323005/.
After I added them back, they should have only been used in tests and should
never have ended up in production code.

>
https://codereview.chromium.org/65303006/diff/1/Source/platform/blink_platfor...
> File Source/platform/blink_platform.gypi (right):
> 
>
https://codereview.chromium.org/65303006/diff/1/Source/platform/blink_platfor...
> Source/platform/blink_platform.gypi:127:
> 'animation/TimingFunctionTestHelper.cpp',
> TimingFunctionTestHelper.h/cpp are a bunch of utilities for testing
> TimingFunction, so shouldn't they be in platform_test_files?

I concur with Steve.

---

BTW If you send the compiler output with the compile failure, I can probably
give you more hints from there.

Tim

[pilgrim_google, 2013-11-13 14:28:24]

On 2013/11/12 22:39:20, Steve Block wrote:
>
https://codereview.chromium.org/65303006/diff/1/Source/platform/blink_platfor...
> Source/platform/blink_platform.gypi:127:
> 'animation/TimingFunctionTestHelper.cpp',
> TimingFunctionTestHelper.h/cpp are a bunch of utilities for testing
> TimingFunction, so shouldn't they be in platform_test_files?

This was the correct solution. Sorry for all the noise.

[pilgrim_google, 2013-11-13 15:54:29]

On 2013/11/13 14:28:24, pilgrim_google wrote:
> On 2013/11/12 22:39:20, Steve Block wrote:
> >
>
https://codereview.chromium.org/65303006/diff/1/Source/platform/blink_platfor...
> > Source/platform/blink_platform.gypi:127:
> > 'animation/TimingFunctionTestHelper.cpp',
> > TimingFunctionTestHelper.h/cpp are a bunch of utilities for testing
> > TimingFunction, so shouldn't they be in platform_test_files?
> 
> This was the correct solution. Sorry for all the noise.

Or not. Any thoughts on these compilation/linkage errors?

http://build.chromium.org/p/tryserver.chromium/builders/mac_blink_rel/builds/...

Relevant excerpt:

Undefined symbols for architecture i386:
  "WebCore::operator==(WebCore::TimingFunction const&, WebCore::TimingFunction
const&)", referenced from:
      testing::AssertionResult
testing::internal::CmpHelperEQ<WebCore::TimingFunction,
WebCore::TimingFunction>(char const*, char const*, WebCore::TimingFunction
const&, WebCore::TimingFunction const&) in
webkit_unit_tests.CompositorAnimationsTimingFunctionReverserTest.o
  "WebCore::operator==(WebCore::ChainedTimingFunction const&,
WebCore::TimingFunction const&)", referenced from:
      testing::AssertionResult
testing::internal::CmpHelperEQ<WebCore::ChainedTimingFunction,
WebCore::TimingFunction>(char const*, char const*,
WebCore::ChainedTimingFunction const&, WebCore::TimingFunction const&) in
webkit_unit_tests.CompositorAnimationsTimingFunctionReverserTest.o
  "WebCore::PrintTo(WebCore::ChainedTimingFunction const&, std::ostream*)",
referenced from:
      std::string
testing::PrintToString<WebCore::ChainedTimingFunction>(WebCore::ChainedTimingFunction
const&) in webkit_unit_tests.CompositorAnimationsTimingFunctionReverserTest.o
  "WebCore::PrintTo(WebCore::TimingFunction const&, std::ostream*)", referenced
from:
      std::string
testing::PrintToString<WebCore::TimingFunction>(WebCore::TimingFunction const&)
in webkit_unit_tests.CompositorAnimationsTimingFunctionReverserTest.o

[Steve Block, 2013-11-14 00:22:32]

You've moved TimingFunctionTestHelperTest.cpp and its utilities to the
blink_platform_unittests target. This target builds fine.

The target that fails is webkit_unit_tests. This fails because some of its test
cases (CompositorAnimationsTest.cpp and
CompositorAnimationsTimingFunctionReverserTest.cpp) need the TimingFunction test
utilities, but these utilities are no longer included in this target.

I don't know whether you should add the utilities to both targets, or if there's
a common target that exists (or should be created) for this purpose.

[pilgrim_google, 2013-11-25 15:07:51]

Adam, if you could re-review this and let us know if this is an acceptable
solution.

[abarth-chromium, 2013-11-25 18:09:32]

https://codereview.chromium.org/65303006/diff/240001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/65303006/diff/240001/Source/core/core.gypi#ne...
Source/core/core.gypi:3535:
'../platform/animation/TimingFunctionTestHelper.cpp',
Is the problem that some code still uses these helpers?  We can leave the tests
here until we're ready to move them all.

https://codereview.chromium.org/65303006/diff/240001/Source/platform/blink_pl...
File Source/platform/blink_platform_tests.gyp (right):

https://codereview.chromium.org/65303006/diff/240001/Source/platform/blink_pl...
Source/platform/blink_platform_tests.gyp:54:
'<(SHARED_INTERMEDIATE_DIR)/blink/RuntimeEnabledFeatures.h',
This isn't correct.  We should pull these in from
blink_platform.gyp:blink_platform

[pilgrim_google, 2013-11-26 16:33:31]

On 2013/11/25 18:09:32, abarth wrote:
> https://codereview.chromium.org/65303006/diff/240001/Source/core/core.gypi
> File Source/core/core.gypi (right):
> 
>
https://codereview.chromium.org/65303006/diff/240001/Source/core/core.gypi#ne...
> Source/core/core.gypi:3535:
> '../platform/animation/TimingFunctionTestHelper.cpp',
> Is the problem that some code still uses these helpers?  We can leave the
tests
> here until we're ready to move them all.

The other code that uses these helpers are outside core/platform/. For example,
core/animation/CompositorAnimationsTest.cpp depends directly on #include
"core/platform/animation/TimingFunctionTestHelper.h" I don't think we plan to
ever move this.

[abarth-chromium, 2013-11-26 19:39:27]

On 2013/11/26 16:33:31, pilgrim_google wrote:
> The other code that uses these helpers are outside core/platform/. For
example,
> core/animation/CompositorAnimationsTest.cpp depends directly on #include
> "core/platform/animation/TimingFunctionTestHelper.h" I don't think we plan to
> ever move this.

Maybe the right thing to do is to create a static_library that contains these
test helpers and then have both testing targets depend on the static library? 
Pulling the source files directly into two targets isn't a good approach.

[pilgrim_google, 2013-12-04 03:31:40]

Please re-review.

[abarth-chromium, 2013-12-04 05:45:26]

https://codereview.chromium.org/65303006/diff/300001/Source/platform/blink_pl...
File Source/platform/blink_platform.gyp (right):

https://codereview.chromium.org/65303006/diff/300001/Source/platform/blink_pl...
Source/platform/blink_platform.gyp:74:
'blink_platform_tests.gyp:blink_platform_unittests_support',
This is a strange dependency.  blink_platform is a production target.  We don't
usually have production targets depending on testing targets...

https://codereview.chromium.org/65303006/diff/300001/Source/platform/blink_pl...
File Source/platform/blink_platform_tests.gyp (right):

https://codereview.chromium.org/65303006/diff/300001/Source/platform/blink_pl...
Source/platform/blink_platform_tests.gyp:48:
'<(SHARED_INTERMEDIATE_DIR)/blink/RuntimeEnabledFeatures.h',
I see, the problem is that you've moved these files here.  We need to ship the
code in these files, so they need to be part of the production target, not part
of the testing target.

[pilgrim_google, 2013-12-06 15:21:24]

rebase

[pilgrim_google, 2013-12-06 15:21:25]

rebase

[pilgrim_google, 2013-12-06 15:21:26]

rebase

[pilgrim_google, 2013-12-06 15:21:27]

rebase

[pilgrim_google, 2013-12-06 15:24:19]

Gosh, I sure hope GAE's 500 errors didn't screw up the 20th version of this
patch.

Please re-review.

[abarth-chromium, 2013-12-07 21:55:01]

lgtm

[commit-bot: I haz the power, 2013-12-07 21:55:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/65303006/550001

[commit-bot: I haz the power, 2013-12-07 22:20:42]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[pilgrim_google, 2013-12-09 14:50:30]

On 2013/12/07 22:20:42, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

Linux build failure looks like flake, but Windows and Mac build failures are
real link errors that I don't understand and don't know how to fix. Help?

http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/16353

http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/15077

[abarth-chromium, 2013-12-09 17:33:34]

https://codereview.chromium.org/65303006/diff/550001/Source/web/web_tests.gyp
File Source/web/web_tests.gyp (right):

https://codereview.chromium.org/65303006/diff/550001/Source/web/web_tests.gyp...
Source/web/web_tests.gyp:86:
'../platform/blink_platform_tests.gyp:blink_platform_unittests_support',
So, the problem is that you've run into a piece of insanity in our build system.
 In the component build, we link the unit tests directly into webkit.dll because
we don't export all the proper symbols we need to test webkit.dll.  That means
that you need to link this target into webkit.dll in the component build. 
Specifically, it should go in the list of stuff around line 84 in web.gyp.

[commit-bot: I haz the power, 2013-12-09 19:16:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/65303006/590001

[commit-bot: I haz the power, 2013-12-09 21:33:00]

Retried try job too often on win_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...