Split content_shell dependencies for layout tests.

content/content_tests.gypi

Index: content/content_tests.gypi
diff --git a/content/content_tests.gypi b/content/content_tests.gypi
index 2289ef4509c1f5c20210adbec1bb932cbdae8a10..1bec1dd371a8ef801ac4ce93fe69a28f3a68ff07 100644
--- a/content/content_tests.gypi
+++ b/content/content_tests.gypi
@@ -5,6 +5,40 @@
 {
   'targets': [
     {
+      'target_name': 'layouttest_support_content',
+      'type': 'static_library',
+      'conditions': [
+        ['OS=="android"', {
+          'dependencies': [
+            'layouttest_support_content_jni_headers',
+          ],
+        }],
+        ['OS!="ios"', {

[jam, 2014/04/03 17:40:20]

this whole target, since its not used on ios, should be inside an OS != "ios"
condition

[Primiano Tucci (use gerrit), 2014/04/03 19:13:44]

Hmm, if I nest the entire target inside a conditions section I get:
gyp: Dependency
'/src/chrome/src/content/content_shell_and_tests.gyp:layouttest_support_content#target'
not found while trying to load target
/src/chrome/src/content/content_shell_and_tests.gyp:content_shell_lib#target

[jam, 2014/04/03 19:39:33]

hmm, are these targets built on ios? they shouldn't be

[Primiano Tucci (use gerrit), 2014/04/03 19:52:59]

all.gyp is including content_shell_and_tests.gyp:* (which in turn includes this
file). Essentially, only a small part of test_support_content is intended to be
built for iOS (if you look at line 190, it has an horrible rule which excludes
all .cc files and then reincludes only a dozen on iOS ..['exclude',
'\\.(cc|mm)$']).

This layouttest target, however, is not required for iOS. But unfortunately is
implicitly included by all.gyp (and I don't feel adventuring in refactoring also
all.gyp at this point, at least not in this CL).
I agree with you that ideally the entire target should be masked by target!=ios,
but unfortunatelly GYP is not liking that (and I feel I reached the borders of
my GYP knowledge... any hints?)

[jam, 2014/04/03 20:08:29]

ugh, it's unfortunate that these other targets are built on ios. ok, that's not
your fault, so this is fine to workaround that for now. i agree fixing that is
outside the scope of this cl.

+          # layouttest_support_content is not supported nor required on iOS.
+          'dependencies': [
+            '../skia/skia.gyp:skia',
+            '../v8/tools/gyp/v8.gyp:v8',
+            '../webkit/common/webkit_common.gyp:webkit_common',
+          ],
+          'include_dirs': [
+            '..',
+          ],
+          'sources': [
+            'public/test/layouttest_support.h',
+            'public/test/nested_message_pump_android.cc',
+            'public/test/nested_message_pump_android.h',
+            'test/layouttest_support.cc',
+            'test/mock_webclipboard_impl.cc',
+            'test/mock_webclipboard_impl.h',
+            'test/test_media_stream_client.cc',
+            'test/test_media_stream_client.h',
+            'test/test_video_frame_provider.cc',
+            'test/test_video_frame_provider.h',
+          ],
+        }],
+      ],
+    },
+    {
       'target_name': 'test_support_content',
       'type': 'static_library',
       'dependencies': [
@@ -58,8 +92,6 @@
         'public/test/mock_render_thread.h',
         'public/test/mock_resource_context.cc',
         'public/test/mock_resource_context.h',
-        'public/test/nested_message_pump_android.cc',
-        'public/test/nested_message_pump_android.h',
         'public/test/render_view_test.cc',
         'public/test/render_view_test.h',
         'public/test/render_widget_test.cc',
@@ -128,8 +160,6 @@
         'test/mock_keyboard_driver_win.h',
         'test/mock_render_process.cc',
         'test/mock_render_process.h',
-        'test/mock_webclipboard_impl.cc',
-        'test/mock_webclipboard_impl.h',
         'test/mock_webframeclient.h',
         'test/mock_weburlloader.cc',
         'test/mock_weburlloader.h',
@@ -153,8 +183,6 @@
         'test/test_content_client.h',
         'test/test_context_provider_factory.cc',
         'test/test_context_provider_factory.h',
-        'test/test_media_stream_client.cc',
-        'test/test_media_stream_client.h',
         'test/test_render_frame_host.cc',
         'test/test_render_frame_host.h',
         'test/test_render_frame_host_factory.cc',
@@ -163,8 +191,6 @@
         'test/test_render_view_host.h',
         'test/test_render_view_host_factory.cc',
         'test/test_render_view_host_factory.h',
-        'test/test_video_frame_provider.cc',
-        'test/test_video_frame_provider.h',
         'test/test_web_contents.cc',
         'test/test_web_contents.h',
         'test/test_web_contents_view.cc',
@@ -221,6 +247,7 @@
             'content.gyp:content_renderer',
             'content.gyp:content_utility',
             'content.gyp:content_worker',
+            'layouttest_support_content',

[jam, 2014/04/03 17:40:20]

this is odd to see, why does test_support_content need this? if they're files
just for layout tests, only content_shell should need them

[Primiano Tucci (use gerrit), 2014/04/03 19:13:44]

See the attached drawing.
Essentially some of the 3 files being moved from test_support_content to
layouttest_support_content (e.g, mock_webclipboard_impl.cc) are needed by both
layouttests and contentshell itself. But they can't be recompiled twice and
linked twice. Hence the extra dependency here.

Furthermore, webkit_unit_tests need test_support_content (cause they depend on
mock_webclipboard_impl.cc) but they don't depend on content-shell. That's why I
need this (see the drawing)

[jam, 2014/04/03 19:39:33]

let's look at each of the cc files one by one

test/mock_webclipboard_impl.cc
-this is used by content_shell and the unit tests. so we can add that cc file in
the layout test target, and in this target

test_media_stream_client.cc
-TestMediaStreamClient is only used by layouttest_support.cc, so it should be
fine to keep in the layout test target only

test_video_frame_provider.cc
-TestVideoFrameProvider is only used by test_media_stream_client.cc, so it can
go to the layout test target

[Primiano Tucci (use gerrit), 2014/04/03 19:52:59]

Unfortunately we can't. If I include the .cc file both in layouttest and in this
target, content_shell_lib will fail to link because the symbols in
webclipboard.cc will end'up being defined twice.
It would work if I could add the mock_webclipboard file to both AND remove this
dependency on line 250.

However, the game here is more complicated because:
webkit_unit_tests (via webkit_test_runner.cc) depend on both:
 - test_support_content
 - layouttest_support

So, if I remove the dep on line 250, I'd break the functional dependency between
webkit_test_runner.cc and layouttest_support.cc
(Sorry I missed that in the drawing)

Agree for test_media_stream_client and tesT_video_frame_provider (but they are
already in the right place)

[jam, 2014/04/03 20:08:29]

huh? why would it be defined twice? content_shell_lib doesn't depend on
test_support_content?
why does webkit_unit_tests need layouttest_support? seems that
layouttest_support should only contain stuff that content_shell needs for
running layout tests

[Primiano Tucci (use gerrit), 2014/04/03 20:36:29]

Twice: in this target (test_support_content) and layouttest_support_content
(according to the suggestion in your comment)
Unfortunatelly there is yet another dependency between content_shell and
mock_webclipboard_impl.cc via shell_content_renderer_client.cc.
I dont' see other solutions then keeping also shell_content_renderer_client.cc
into layout_test... (hence keeping this dependency here between
test_support_content and layouttest_support_content)

             '../cc/cc_tests.gyp:cc_test_support',
             '../media/media.gyp:media',
             '../ppapi/ppapi_internal.gyp:ppapi_host',
@@ -297,7 +324,6 @@
         ['OS=="android"', {
           'dependencies': [
             '../ui/shell_dialogs/shell_dialogs.gyp:shell_dialogs',
-            'test_support_content_jni_headers',
           ],
         }],
       ],
@@ -306,10 +332,10 @@
       'target_name': 'content_unittests',
       'type': '<(gtest_target_type)',
       'dependencies': [
+        'browser/speech/proto/speech_proto.gyp:speech_proto',
         'content.gyp:content_browser',
         'content.gyp:content_common',
         'test_support_content',
-        'browser/speech/proto/speech_proto.gyp:speech_proto',
         '../base/base.gyp:test_support_base',
         '../crypto/crypto.gyp:crypto',
         '../net/net.gyp:net_test_support',
@@ -958,6 +984,7 @@
             'content_resources.gyp:content_resources',
             'content_shell_lib',
             'content_shell_pak',
+            'test_support_content',

[Primiano Tucci (use gerrit), 2014/04/03 16:44:28]

This is needed because previously content_unittests were relying on
content_shell to depend on test_support_content.

             '../base/base.gyp:test_support_base',
             '../gin/gin.gyp:gin',
             '../gpu/gpu.gyp:gpu',
@@ -1585,7 +1612,7 @@
     ['OS == "android"', {
       'targets': [
         {
-          'target_name': 'test_support_content_jni_headers',
+          'target_name': 'layouttest_support_content_jni_headers',

[jam, 2014/04/03 17:40:20]

why the rename? this is also used in content_browsertests right?

[Primiano Tucci (use gerrit), 2014/04/03 19:13:44]

Hmm, not really. THis is only needed by nested_message_pump_android.cc, which is
for layouttest_support_content

[jam, 2014/04/03 19:39:33]

hmmm, this s used by browser_tests afaik because they need a nested message loop
on Android to run the tests. NestedMessagePumpAndroid is used by
content_test_launcher.cc, which is in content_browsertests

[Primiano Tucci (use gerrit), 2014/04/03 19:52:59]

Ahh sorry, you're right here. NestedMessagePumpAndroid is used also by other
parties, not only layouttest. Will rename.

           'type': 'none',
           'sources': [
             'public/test/android/javatests/src/org/chromium/content/browser/test/NestedSystemMessageHandler.java',