Split content_shell dependencies for layout tests.

content/content_tests.gypi

 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 {
   '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': [
         '../net/net.gyp:net_test_support',
         '../skia/skia.gyp:skia',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
         '../ui/accessibility/accessibility.gyp:ax_gen',
         '../ui/base/ui_base.gyp:ui_base',
         '../ui/events/events.gyp:dom4_keycode_converter',
@@ skipping 33 matching lines @@
         'public/test/mock_download_manager.cc',
         'public/test/mock_download_manager.h',
         'public/test/mock_notification_observer.cc',
         'public/test/mock_notification_observer.h',
         'public/test/mock_render_process_host.cc',
         'public/test/mock_render_process_host.h',
         'public/test/mock_render_thread.cc',
         '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',
         'public/test/render_widget_test.h',
         'public/test/sandbox_file_system_test_helper.cc',
         'public/test/sandbox_file_system_test_helper.h',
         'public/test/test_browser_context.cc',
         'public/test/test_browser_context.h',
         'public/test/test_browser_thread.cc',
         'public/test/test_browser_thread.h',
@@ skipping 48 matching lines @@
         'test/content_test_suite.cc',
         'test/content_test_suite.h',
         'test/mock_google_streaming_server.cc',
         'test/mock_google_streaming_server.h',
         'test/mock_keyboard.cc',
         'test/mock_keyboard.h',
         'test/mock_keyboard_driver_win.cc',
         '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',
         'test/net/url_request_abort_on_end_job.cc',
         'test/net/url_request_abort_on_end_job.h',
         'test/net/url_request_failed_job.cc',
         'test/net/url_request_failed_job.h',
         'test/net/url_request_mock_http_job.cc',
         'test/net/url_request_mock_http_job.h',
         'test/net/url_request_prepackaged_interceptor.cc',
         'test/net/url_request_prepackaged_interceptor.h',
         'test/net/url_request_slow_download_job.cc',
         'test/net/url_request_slow_download_job.h',
         'test/ppapi_unittest.cc',
         'test/ppapi_unittest.h',
         'test/test_backing_store.cc',
         'test/test_backing_store.h',
         'test/test_content_browser_client.cc',
         'test/test_content_browser_client.h',
         'test/test_content_client.cc',
         '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',
         'test/test_render_frame_host_factory.h',
         'test/test_render_view_host.cc',
         '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',
         'test/test_web_contents_view.h',
         'test/test_webkit_platform_support.cc',
         'test/test_webkit_platform_support.h',
         'test/web_gesture_curve_mock.cc',
         'test/web_gesture_curve_mock.h',
         'test/web_layer_tree_view_impl_for_testing.cc',
         'test/web_layer_tree_view_impl_for_testing.h',
@@ skipping 36 matching lines @@
             }],
           ],
           'dependencies': [
             'content.gyp:content_child',
             'content.gyp:content_common',
             'content.gyp:content_gpu',
             'content.gyp:content_ppapi_plugin',
             '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',
             '../ppapi/ppapi_internal.gyp:ppapi_proxy',
             '../ppapi/ppapi_internal.gyp:ppapi_shared',
             '../ppapi/ppapi_internal.gyp:ppapi_unittest_shared',
             '../third_party/WebKit/public/blink.gyp:blink',
             '../ui/surface/surface.gyp:surface',
             '../webkit/child/webkit_child.gyp:webkit_child',
             '../webkit/common/gpu/webkit_gpu.gyp:webkit_gpu',
@@ skipping 56 matching lines @@
           ],
         }],
         ['OS!="android" and OS!="ios"', {
           'dependencies': [
             '../third_party/libvpx/libvpx.gyp:libvpx',
           ],
         }],
         ['OS=="android"', {
           'dependencies': [
             '../ui/shell_dialogs/shell_dialogs.gyp:shell_dialogs',
-            'test_support_content_jni_headers',
           ],
         }],
       ],
     },
     {
       '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',
         '../skia/skia.gyp:skia',
         '../sql/sql.gyp:sql',
         '../sql/sql.gyp:test_support_sql',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
         '../third_party/re2/re2.gyp:re2',
         '../ui/accessibility/accessibility.gyp:accessibility',
@@ skipping 628 matching lines @@
           'type': '<(gtest_target_type)',
           'dependencies': [
             'content.gyp:content_common',
             'content.gyp:content_gpu',
             'content.gyp:content_plugin',
             'content.gyp:content_renderer',
             'content_browser_test_support',
             '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',
             '../ipc/ipc.gyp:test_support_ipc',
             '../media/media.gyp:media_test_support',
             '../media/media.gyp:shared_memory_support',
             '../net/net.gyp:net_test_support',
             '../ppapi/ppapi_internal.gyp:ppapi_host',
             '../ppapi/ppapi_internal.gyp:ppapi_ipc',
             '../ppapi/ppapi_internal.gyp:ppapi_proxy',
@@ skipping 607 matching lines @@
             'test_suite_name': 'video_decode_accelerator_unittest',
             'input_shlib_path': '<(SHARED_LIB_DIR)/<(SHARED_LIB_PREFIX)content_unittests<(SHARED_LIB_SUFFIX)',
           },
           'includes': [ '../build/apk_test.gypi' ],
         },
       ],
     }],
     ['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',
           ],
           'variables': {
             'jni_gen_package': 'content/public/test',
             'jni_generator_ptr_type': 'long',
           },
           'includes': [ '../build/jni_generator.gypi' ],
         },
@@ skipping 176 matching lines @@
                   'files': ['<(PRODUCT_DIR)/libnpapi_test_plugin.so'],
                 },
               ],
             }],
           ],
         },
       ],
     }],
   ],
 }