[MacViews] Get unit_tests linking.

[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
excluded in a 'mac_views_browser==1' condition.

BUG=569463
Committed: https://crrev.com/51a0287adf6e0f28c9b31a1387311f20e47a0737
Cr-Commit-Position: refs/heads/master@{#365949}

[jackhou1, 2015-12-16 23:36:29]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-12-16 23:36:29]

tapted, PTAL

[tapted, 2015-12-17 01:49:19]

https://codereview.chromium.org/1529723002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/1529723002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:604: if (is_mac && !mac_views_browser) {
can this just be an `else` clause of `if (toolkit_views && (!is_mac ||
mac_views_browser))` on line 515?  (i.e. and still rely on the
platform-stripping for non-mac). This feels appropriate since we'd never want to
include both file groups on any platform.

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1152: # Mac sources, except when
mac_views_browser==1
nit: maybe something like

# Tests corresponding to the files in chrome_browser_ui_cocoa_sources.
# Built on Mac, except when mac_views_browser==1.

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2109:
'browser/ui/cocoa/extensions/browser_action_test_util_mac.mm',
I think gyp still prefers 'add everything and remove`. It's confusing doing both
adds and removes here - there doesn't seem to be an underlying reason why
find_bar_host_unittest_util_views.cc is removed but
browser_action_test_util_mac.mm is added.

Then you can have a positive condition (mac_views_browser==1) and an `else` to
(kinda) match gyp a bit closer. (except removing instead of adding).

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2114:
'browser/ui/views/toolbar/browser_action_test_util_views.cc',
how was this getting removed before?

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2614: ['mac_views_browser!=1', {
There's a fragment below:

            ['OS != "mac"', {
              'sources': [ '<@(chrome_unit_tests_views_non_mac_sources)' ],
            }],

but in chrome_browser_ui.gypi, the condition (for the bit you've added here)
goes:

            ['mac_views_browser==1', {
              'sources': [
                '<@(chrome_browser_ui_views_mac_experimental_sources)',
                '<@(chrome_browser_ui_views_non_mac_sources)',
                '<@(chrome_browser_ui_views_extensions_non_mac_sources)',
              ],
            }, {
              'sources': [ '<@(chrome_browser_ui_cocoa_sources)' ],
            }],


So, can we add chrome_unit_tests_views_non_mac_sources to the mac_views_browser
build yet, so that these are closer?

Otherwise (e.g. if they don't compile yet, which is likely), I think it should
go something like

            ['mac_views_browser==1', {
              // TODO(tapted): Add chrome_unit_tests_views_non_mac_sources here.
            }, {
              'sources': [ '<@(chrome_unit_tests_cocoa_sources)' ],
            }],

This makes it look more like the corresponding non-test conditions.

https://codereview.chromium.org/1529723002/diff/40001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1529723002/diff/40001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:1948: if (!mac_views_browser) {
Same dealie here. I.e. 

    if (mac_views_browser) {
      // TODO(tapted): Add chrome_unit_tests_views_non_mac_sources/
    } else {
sources += cocoastuff
    }

[jackhou1, 2015-12-17 04:03:50]

https://codereview.chromium.org/1529723002/diff/40001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/1529723002/diff/40001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:604: if (is_mac && !mac_views_browser) {
On 2015/12/17 01:49:19, tapted wrote:
> can this just be an `else` clause of `if (toolkit_views && (!is_mac ||
> mac_views_browser))` on line 515?  (i.e. and still rely on the
> platform-stripping for non-mac). This feels appropriate since we'd never want
to
> include both file groups on any platform.

Done.

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1152: # Mac sources, except when
mac_views_browser==1
On 2015/12/17 01:49:19, tapted wrote:
> nit: maybe something like
> 
> # Tests corresponding to the files in chrome_browser_ui_cocoa_sources.
> # Built on Mac, except when mac_views_browser==1.

Done.

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2109:
'browser/ui/cocoa/extensions/browser_action_test_util_mac.mm',
On 2015/12/17 01:49:19, tapted wrote:
> I think gyp still prefers 'add everything and remove`. It's confusing doing
both
> adds and removes here - there doesn't seem to be an underlying reason why
> find_bar_host_unittest_util_views.cc is removed but
> browser_action_test_util_mac.mm is added.
> 
> Then you can have a positive condition (mac_views_browser==1) and an `else` to
> (kinda) match gyp a bit closer. (except removing instead of adding).

Done.

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2114:
'browser/ui/views/toolbar/browser_action_test_util_views.cc',
On 2015/12/17 01:49:19, tapted wrote:
> how was this getting removed before?

You mean find_bar_host_unittest_util_views.cc? I'm not sure actually, but it's
explicitly added in GN, and with my changes it fails to link if it's not
removed.

https://codereview.chromium.org/1529723002/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2614: ['mac_views_browser!=1', {
On 2015/12/17 01:49:19, tapted wrote:
> There's a fragment below:
> 
>             ['OS != "mac"', {
>               'sources': [ '<@(chrome_unit_tests_views_non_mac_sources)' ],
>             }],
> 
> but in chrome_browser_ui.gypi, the condition (for the bit you've added here)
> goes:
> 
>             ['mac_views_browser==1', {
>               'sources': [
>                 '<@(chrome_browser_ui_views_mac_experimental_sources)',
>                 '<@(chrome_browser_ui_views_non_mac_sources)',
>                 '<@(chrome_browser_ui_views_extensions_non_mac_sources)',
>               ],
>             }, {
>               'sources': [ '<@(chrome_browser_ui_cocoa_sources)' ],
>             }],
> 
> 
> So, can we add chrome_unit_tests_views_non_mac_sources to the
mac_views_browser
> build yet, so that these are closer?
> 
> Otherwise (e.g. if they don't compile yet, which is likely), I think it should
> go something like
> 
>             ['mac_views_browser==1', {
>               // TODO(tapted): Add chrome_unit_tests_views_non_mac_sources
here.
>             }, {
>               'sources': [ '<@(chrome_unit_tests_cocoa_sources)' ],
>             }],
> 
> This makes it look more like the corresponding non-test conditions.

Done.

https://codereview.chromium.org/1529723002/diff/40001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1529723002/diff/40001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:1948: if (!mac_views_browser) {
On 2015/12/17 01:49:19, tapted wrote:
> Same dealie here. I.e. 
> 
>     if (mac_views_browser) {
>       // TODO(tapted): Add chrome_unit_tests_views_non_mac_sources/
>     } else {
> sources += cocoastuff
>     }

Done.

[tapted, 2015-12-17 04:21:06]

lgtm - cl description just needs a minor tweak

[jackhou1, 2015-12-17 04:58:03]

Description was changed from

==========
[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
moved behind a 'mac_views_browser!=1' condition.

BUG=569463
==========

to

==========
[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
excluded in a 'mac_views_browser==1' condition.

BUG=569463
==========

[jackhou1, 2015-12-17 05:01:37]

jackhou@chromium.org changed reviewers:
+ thestig@chromium.org

[jackhou1, 2015-12-17 05:01:37]

thestig, please review for OWNERS

[Lei Zhang, 2015-12-17 07:01:04]

lgtm

[jackhou1, 2015-12-17 21:59:22]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-12-17 22:01:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1529723002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1529723002/80001

[commit-bot: I haz the power, 2015-12-17 22:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 22:12:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jackhou1, 2015-12-17 22:38:13]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-12-17 22:38:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1529723002/#ps100001
(title: "Sync and rebase")

[commit-bot: I haz the power, 2015-12-17 22:40:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1529723002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1529723002/100001

[commit-bot: I haz the power, 2015-12-18 00:31:35]

Description was changed from

==========
[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
excluded in a 'mac_views_browser==1' condition.

BUG=569463
==========

to

==========
[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
excluded in a 'mac_views_browser==1' condition.

BUG=569463
==========

[commit-bot: I haz the power, 2015-12-18 00:31:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-18 00:32:27]

Description was changed from

==========
[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
excluded in a 'mac_views_browser==1' condition.

BUG=569463
==========

to

==========
[MacViews] Get unit_tests linking.

This puts unit tests in browser/ui/cocoa/ into a new GYP variable
chrome_unit_tests_cocoa_sources which is only included in non-
mac_views_browser builds.

Parts of test_support_common that depend on the Cocoa browser are also
excluded in a 'mac_views_browser==1' condition.

BUG=569463

Committed: https://crrev.com/51a0287adf6e0f28c9b31a1387311f20e47a0737
Cr-Commit-Position: refs/heads/master@{#365949}
==========

[commit-bot: I haz the power, 2015-12-18 00:32:28]

Patchset 6 (id:??) landed as
https://crrev.com/51a0287adf6e0f28c9b31a1387311f20e47a0737
Cr-Commit-Position: refs/heads/master@{#365949}