Roll googletest to 1.8.0.

Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropriate //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
TBR=rkc

Review-Url: https://codereview.chromium.org/2779193002
Cr-Commit-Position: refs/heads/master@{#467833}
Committed: https://chromium.googlesource.com/chromium/src/+/b539fa1123a19e6499f58582da55ca5af9df94a4

[pwnall, 2017-04-05 00:47:23]

Patchset #1 (id:1) has been deleted

[pwnall, 2017-04-05 01:10:36]

Description was changed from

==========
Bump googletest to 1.8.0.

BUG=
==========

to

==========
Updated googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.
testing/gtest/include/gtest/ and testing/gmock/include/gmock now have
forwarding headers into the appropriate locations of third_party/googletest.

BUG=
==========

[pwnall, 2017-04-05 01:10:48]

Patchset #1 (id:20001) has been deleted

[pwnall, 2017-04-05 01:12:59]

Patchset #1 (id:40001) has been deleted

[pwnall, 2017-04-05 01:13:06]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 01:14:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 01:25:23]

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

[commit-bot: I haz the power, 2017-04-05 01:25:24]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pwnall, 2017-04-11 00:51:07]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[pwnall, 2017-04-11 00:51:22]

Patchset #3 (id:100001) has been deleted

[pwnall, 2017-04-11 00:51:29]

Patchset #2 (id:80001) has been deleted

[pwnall, 2017-04-11 00:51:41]

Patchset #2 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-04-11 00:52:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-04-11 00:52:52]

Description was changed from

==========
Updated googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.
testing/gtest/include/gtest/ and testing/gmock/include/gmock now have
forwarding headers into the appropriate locations of third_party/googletest.

BUG=
==========

to

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.
testing/gtest/include/gtest/ and testing/gmock/include/gmock now have
forwarding headers into the appropriate locations of third_party/googletest.

BUG=630705
==========

[commit-bot: I haz the power, 2017-04-11 01:04:14]

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

[commit-bot: I haz the power, 2017-04-11 01:04:15]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pwnall, 2017-04-12 09:37:21]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 09:37:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 09:49:05]

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

[commit-bot: I haz the power, 2017-04-12 09:49:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pwnall, 2017-04-12 23:40:04]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 23:40:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-04-12 23:41:14]

Patchset #2 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-04-13 00:27:16]

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

[commit-bot: I haz the power, 2017-04-13 00:27:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pwnall, 2017-04-13 01:53:56]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 01:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 04:17:05]

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

[commit-bot: I haz the power, 2017-04-13 04:17:06]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-04-26 20:42:51]

pwnall@chromium.org changed reviewers:
+ dpranke@chromium.org, thakis@chromium.org

[pwnall, 2017-04-26 20:42:51]

PTAL?

[Nico, 2017-04-26 20:46:19]

This looks basically good, but please add some words about the forwarding
headers being permanent for the foreseeable future to the CL description, the
new build file, the README.chromium.

Thanks!

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
File third_party/googletest/BUILD.gn (right):

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/BUILD.gn:40: source_set("gtest") {
please add a comment above this saying that people should depend on
testing/gtest instead

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/BUILD.gn:98: static_library("gmock") {
likewise

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
File third_party/googletest/OWNERS (right):

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/OWNERS:1: phajdan.jr@chromium.org
please add you and me

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
File third_party/googletest/README.chromium (right):

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/README.chromium:6: Security critical: no
Add some words here on how upstream is planning on moving gmock around some
more, and that this exists to make pulling from upstream easy, but that people
should use the forwarding headers for now instead of the headers here directly.

[pwnall, 2017-04-26 21:58:04]

Description was changed from

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.
testing/gtest/include/gtest/ and testing/gmock/include/gmock now have
forwarding headers into the appropriate locations of third_party/googletest.

BUG=630705
==========

to

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropraite //third_party/googletest targets.

BUG=630705
==========

[pwnall, 2017-04-26 22:35:03]

Patchset #5 (id:220001) has been deleted

[pwnall, 2017-04-26 22:38:00]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[pwnall, 2017-04-26 22:38:24]

thakis: Thank you very much for the quick response! PTAL?

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
File third_party/googletest/BUILD.gn (right):

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/BUILD.gn:40: source_set("gtest") {
On 2017/04/26 20:46:18, Nico wrote:
> please add a comment above this saying that people should depend on
> testing/gtest instead

Done.

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/BUILD.gn:98: static_library("gmock") {
On 2017/04/26 20:46:18, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
File third_party/googletest/OWNERS (right):

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/OWNERS:1: phajdan.jr@chromium.org
On 2017/04/26 20:46:19, Nico wrote:
> please add you and me

Done.

I'm not sure about the ordering. I borrowed the original list from
testing/OWNERS. Does this look reasonable?

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
File third_party/googletest/README.chromium (right):

https://codereview.chromium.org/2779193002/diff/200001/third_party/googletest...
third_party/googletest/README.chromium:6: Security critical: no
On 2017/04/26 20:46:19, Nico wrote:
> Add some words here on how upstream is planning on moving gmock around some
> more, and that this exists to make pulling from upstream easy, but that people
> should use the forwarding headers for now instead of the headers here
directly.

Done.

[commit-bot: I haz the power, 2017-04-26 22:38:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2017-04-26 23:42:55]

I defer to thakis@ here. If you need an extra pair of eyes on the build files,
let me know.

[pwnall, 2017-04-26 23:49:13]

On 2017/04/26 23:42:55, Dirk Pranke wrote:
> I defer to thakis@ here. If you need an extra pair of eyes on the build files,
> let me know.

Thank you for the quick response!

I will need an rs LGTM for testing/, though. Should I create
testing/{gtest,gmock}/OWNERS and add thakis@ and myself there, to smooth out
future reviews?

[Nico, 2017-04-26 23:57:44]

Description was changed from

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropraite //third_party/googletest targets.

BUG=630705
==========

to

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropraite //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
==========

[Nico, 2017-04-26 23:59:08]

lgtm from my side.

Thanks much for working on this!

[Nico, 2017-04-26 23:59:22]

Description was changed from

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropraite //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
==========

to

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropriate //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
==========

[Dirk Pranke, 2017-04-27 00:00:26]

On 2017/04/26 23:49:13, pwnall wrote:
> On 2017/04/26 23:42:55, Dirk Pranke wrote:
> > I defer to thakis@ here. If you need an extra pair of eyes on the build
files,
> > let me know.
> 
> Thank you for the quick response!
> 
> I will need an rs LGTM for testing/, though. Should I create
> testing/{gtest,gmock}/OWNERS and add thakis@ and myself there, to smooth out
> future reviews?

oh, sorry, I was getting tools/ confused w/ testing/ and thinking that thakis
was already an owner.

rs- LGTM. Feel free to add OWNERS files, that's always helpful.

[pwnall, 2017-04-27 00:51:48]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 00:52:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-04-27 01:18:16]

pwnall@chromium.org changed reviewers:
+ agl@chromium.org, mcchou@chromium.org

[pwnall, 2017-04-27 01:18:17]

agl@chromium.org: Please review changes in
net/socket/sequenced_socket_data_unittest.cc

mcchou@chromium.org: Please review changes in
device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h

[commit-bot: I haz the power, 2017-04-27 02:40:15]

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

[commit-bot: I haz the power, 2017-04-27 02:40:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pwnall, 2017-04-27 07:05:11]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 07:05:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 07:06:55]

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

[commit-bot: I haz the power, 2017-04-27 07:06:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pwnall, 2017-04-27 08:17:28]

Patchset #7 (id:280001) has been deleted

[pwnall, 2017-04-27 08:17:39]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 08:17:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 08:29:24]

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

[commit-bot: I haz the power, 2017-04-27 08:29:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pwnall, 2017-04-27 08:50:21]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[pwnall, 2017-04-27 08:50:32]

Patchset #7 (id:300001) has been deleted

[commit-bot: I haz the power, 2017-04-27 08:50:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-04-27 09:27:26]

Patchset #7 (id:320001) has been deleted

[pwnall, 2017-04-27 09:27:32]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 09:27:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 12:03:24]

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

[commit-bot: I haz the power, 2017-04-27 12:03:25]

Dry run: This issue passed the CQ dry run.

[agl, 2017-04-27 14:03:27]

lgtm

[pwnall, 2017-04-27 23:46:56]

pwnall@chromium.org changed reviewers:
+ rkc@chromium.org
- mcchou@chromium.org

[pwnall, 2017-04-27 23:46:57]

rkc: Can you please review the changes in
device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h?

[Nico, 2017-04-28 00:46:44]

I think it's fine to TBR=rkc this.

https://codereview.chromium.org/2779193002/diff/340001/device/bluetooth/dbus/...
File device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h
(right):

https://codereview.chromium.org/2779193002/diff/340001/device/bluetooth/dbus/...
device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h:17:
#include "testing/gtest/include/gtest/gtest_prod.h"  // nogncheck
Can you explain why this is needed?

[pwnall, 2017-04-28 00:59:00]

thakis, dpranke, agl: Thank you very much for the quick reviews!

https://codereview.chromium.org/2779193002/diff/340001/device/bluetooth/dbus/...
File device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h
(right):

https://codereview.chromium.org/2779193002/diff/340001/device/bluetooth/dbus/...
device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h:17:
#include "testing/gtest/include/gtest/gtest_prod.h"  // nogncheck
On 2017/04/28 00:46:44, Nico wrote:
> Can you explain why this is needed?

This appeases "gn check".

Longer version: gn's deps check (completely separate from checkdeps.py, which
uses DEPS) uses a target's "deps" in BUILD.gn, and ignores files that are not in
the repository. For this reason, #including gtest_prod did not trigger "gn
check" while testing/gtest and testing/gmock were brought in via DEPS.

I think that the correct long-term fix is to add a gtest_prod target to GN,
which would only contain the gtest_prod header file. The GN targets with files
who #include gtest_prod would have to add gtest_prod to their deps.

One of the files #includes gtest_prod is base/gtest_prod_util.h (also patched in
this CL), so //base would need this dependency. I expect that this CL will be
tough to land as-is, so I used nogncheck instead of building up the gtest_prod
target to avoid growing the CL even further, and making it more difficult to
land.

Does this seem reasonable?

[Nico, 2017-04-28 01:11:29]

Landing this is fine. I'd be surprised if gn ignored files not in src
though, Dirk, do you know if that's true?

On Apr 27, 2017 8:59 PM, <pwnall@chromium.org> wrote:

> thakis, dpranke, agl: Thank you very much for the quick reviews!
>
>
> https://codereview.chromium.org/2779193002/diff/340001/
> device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h
> File
> device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h
> (right):
>
> https://codereview.chromium.org/2779193002/diff/340001/
> device/bluetooth/dbus/fake_bluetooth_media_endpoint_
> service_provider.h#newcode17
> device/bluetooth/dbus/fake_bluetooth_media_endpoint_service_provider.h:17:
> #include "testing/gtest/include/gtest/gtest_prod.h" // nogncheck
> On 2017/04/28 00:46:44, Nico wrote:
> > Can you explain why this is needed?
>
> This appeases "gn check".
>
> Longer version: gn's deps check (completely separate from checkdeps.py,
> which uses DEPS) uses a target's "deps" in BUILD.gn, and ignores files
> that are not in the repository. For this reason, #including gtest_prod
> did not trigger "gn check" while testing/gtest and testing/gmock were
> brought in via DEPS.
>
> I think that the correct long-term fix is to add a gtest_prod target to
> GN, which would only contain the gtest_prod header file. The GN targets
> with files who #include gtest_prod would have to add gtest_prod to their
> deps.
>
> One of the files #includes gtest_prod is base/gtest_prod_util.h (also
> patched in this CL), so //base would need this dependency. I expect that
> this CL will be tough to land as-is, so I used nogncheck instead of
> building up the gtest_prod target to avoid growing the CL even further,
> and making it more difficult to land.
>
> Does this seem reasonable?
>
> https://codereview.chromium.org/2779193002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pwnall, 2017-04-28 01:51:43]

Description was changed from

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropriate //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
==========

to

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropriate //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
TBR=rkc
==========

[pwnall, 2017-04-28 01:52:30]

On 2017/04/28 01:11:29, Nico wrote:
> Landing this is fine. I'd be surprised if gn ignored files not in src
> though, Dirk, do you know if that's true?

On second thought, I can't find the reference for that claim.
https://cs.chromium.org/chromium/src/tools/gn/command_check.cc?q=gncheck+pack...
is the closest thing I can think of, but it doesn't state what I said above. I
should have taken better notes.

[pwnall, 2017-04-28 01:52:47]

The CQ bit was checked by pwnall@chromium.org

[pwnall, 2017-04-28 01:52:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2779193002/#ps340001
(title: "Rebase + trial of GN tweaks needed for v8.")

[commit-bot: I haz the power, 2017-04-28 01:54:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 02:07:34]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1493344367576260,
"parent_rev": "a3cfe1428faf4f5bd556e5d9aa4924a7d0fb02f4", "commit_rev":
"b539fa1123a19e6499f58582da55ca5af9df94a4"}

[commit-bot: I haz the power, 2017-04-28 02:07:47]

Description was changed from

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropriate //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
TBR=rkc
==========

to

==========
Roll googletest to 1.8.0.

GoogleTest (gtest) and GoogleMock (gmock) are now hosted into the same
googletest repository. In order to cope with this, the googletest
repository is now sourced at third_party/googletest.

The file/directory layout of Google Test is not yet considered stable.
To minimize disruption while Google Test stabilizes, Chromium code will
be insulated from third_party/googletest.

* testing/gtest/include/gtest/ and testing/gmock/include/gmock have
  been populated with headers that forward into the appropriate locations of
  third_party/googletest

* testing/BUILD.gn has been populated with the targets
  //testing/gtest(:gtest_main) and //testing/gmock(:gmock_main), which
  depend on the appropriate //third_party/googletest targets.

All Chromium code should keep depending on the targets and headers
in testing/{gtest,gmock} for now.

BUG=630705
TBR=rkc

Review-Url: https://codereview.chromium.org/2779193002
Cr-Commit-Position: refs/heads/master@{#467833}
Committed:
https://chromium.googlesource.com/chromium/src/+/b539fa1123a19e6499f58582da55...
==========

[commit-bot: I haz the power, 2017-04-28 02:07:49]

Committed patchset #7 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/b539fa1123a19e6499f58582da55...

[skobes, 2017-04-28 02:30:19]

skobes@chromium.org changed reviewers:
+ skobes@chromium.org

[skobes, 2017-04-28 02:30:20]

This may have broken the Mac compile:

https://build.chromium.org/p/chromium/builders/Mac/builds/26741

FAILED: ced_unittests ced_unittests.unstripped 
export DEVELOPER_DIR=/b/c/b/mac/src/build/mac_files/Xcode.app; 
TOOL_VERSION=1487140455 ../../build/toolchain/mac/linker_driver.py
../../third_party/llvm-build/Release+Asserts/bin/clang++  -stdlib=libc++ -arch
x86_64 -Wcrl,unstripped,/b/c/b/mac/src/out/Release -Werror -Wl,-dead_strip
-isysroot
/b/c/b/mac/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-mmacosx-version-min=10.9 -Wl,-ObjC -Wl,-pie -Wcrl,strip,-x,-S -o
"./ced_unittests" -Wl,-filelist,"./ced_unittests.rsp"  
Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_NSAutoreleasePool", referenced from:
      objc-class-ref in platform_test_mac.o
  "_OBJC_CLASS_$_NSString", referenced from:
      objc-class-ref in gtest_mac.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Traceback (most recent call last):
  File "../../build/toolchain/mac/linker_driver.py", line 229, in <module>
    Main(sys.argv)
  File "../../build/toolchain/mac/linker_driver.py", line 79, in Main
    subprocess.check_call(compiler_driver_args)
  File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py",
line 540, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command
'['../../third_party/llvm-build/Release+Asserts/bin/clang++', '-stdlib=libc++',
'-arch', 'x86_64', '-Werror', '-Wl,-dead_strip', '-isysroot',
'/b/c/b/mac/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk',
'-mmacosx-version-min=10.9', '-Wl,-ObjC', '-Wl,-pie', '-o', './ced_unittests',
'-Wl,-filelist,./ced_unittests.rsp']' returned non-zero exit status 1

[findit-for-me, 2017-04-28 02:31:15]

A revert of this CL (patchset #7 id:340001) has been created in
https://codereview.chromium.org/2847043002/ by
findit-for-me@appspot.gserviceaccount.com.

The reason for reverting is: 
Findit(https://goo.gl/kROfz5) identified CL at revision 467833 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....

[pwnall, 2017-04-28 02:46:53]

On 2017/04/28 02:30:20, skobes wrote:
> This may have broken the Mac compile:
> 
> https://build.chromium.org/p/chromium/builders/Mac/builds/26741
> 
> FAILED: ced_unittests ced_unittests.unstripped 
> export DEVELOPER_DIR=/b/c/b/mac/src/build/mac_files/Xcode.app; 
> TOOL_VERSION=1487140455 ../../build/toolchain/mac/linker_driver.py
> ../../third_party/llvm-build/Release+Asserts/bin/clang++  -stdlib=libc++ -arch
> x86_64 -Wcrl,unstripped,/b/c/b/mac/src/out/Release -Werror -Wl,-dead_strip
> -isysroot
>
/b/c/b/mac/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
> -mmacosx-version-min=10.9 -Wl,-ObjC -Wl,-pie -Wcrl,strip,-x,-S -o
> "./ced_unittests" -Wl,-filelist,"./ced_unittests.rsp"  
> Undefined symbols for architecture x86_64:
>   "_OBJC_CLASS_$_NSAutoreleasePool", referenced from:
>       objc-class-ref in platform_test_mac.o
>   "_OBJC_CLASS_$_NSString", referenced from:
>       objc-class-ref in gtest_mac.o
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> Traceback (most recent call last):
>   File "../../build/toolchain/mac/linker_driver.py", line 229, in <module>
>     Main(sys.argv)
>   File "../../build/toolchain/mac/linker_driver.py", line 79, in Main
>     subprocess.check_call(compiler_driver_args)
>   File
>
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py",
> line 540, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command
> '['../../third_party/llvm-build/Release+Asserts/bin/clang++',
'-stdlib=libc++',
> '-arch', 'x86_64', '-Werror', '-Wl,-dead_strip', '-isysroot',
>
'/b/c/b/mac/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk',
> '-mmacosx-version-min=10.9', '-Wl,-ObjC', '-Wl,-pie', '-o', './ced_unittests',
> '-Wl,-filelist,./ced_unittests.rsp']' returned non-zero exit status 1

Yup, sorry. I think I was speaking to myself on #chromium on IRC.

I got the same error on a local Mac and the error went away after reverting
this, so I CQed the revert.

[Nico, 2017-04-28 20:56:23]

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
File build/secondary/testing/gtest/BUILD.gn (right):

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
build/secondary/testing/gtest/BUILD.gn:8: # and will be removed as soon as the
projects switch off of it.
I think we should either just remove this or make it forward to the new
locations. v8 and pdfium can then merge this change the next time they pull
chromium. Having this here might confuse gn and it won't pick up the new
BUILD.gn.

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
build/secondary/testing/gtest/BUILD.gn:94: if ((is_mac || is_ios) &&
gtest_include_objc_support) {
I think this bit is missing from the new BUILD.gn file and it's probably needed.
../*test_mac.* should probably move to third_party/gtest too.

[pwnall, 2017-04-29 00:16:26]

thakis: Thank you in advance for putting up with my questions.

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
File build/secondary/testing/gtest/BUILD.gn (right):

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
build/secondary/testing/gtest/BUILD.gn:8: # and will be removed as soon as the
projects switch off of it.
On 2017/04/28 20:56:23, Nico wrote:
> I think we should either just remove this or make it forward to the new
> locations. v8 and pdfium can then merge this change the next time they pull
> chromium. Having this here might confuse gn and it won't pick up the new
> BUILD.gn.

If we remove build/secondary, v8 and pdifum can't merge Chromium until they land
a variation of this Google Test migration CL. I was afraid that hard consistency
would be a lot more difficult to achieve than eventual consistency.

FWIW, according to the bots and local results, GN seems to use build/secondary
as a fallback. So, while humans and other tools might get confused, GN doesn't
seem to.

I don't really know how often V8 or PDFium update build/, so I don't know if
this is an actual problem. Do you still think we should remove build/secondary
in this CL?

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
build/secondary/testing/gtest/BUILD.gn:94: if ((is_mac || is_ios) &&
gtest_include_objc_support) {
On 2017/04/28 20:56:23, Nico wrote:
> I think this bit is missing from the new BUILD.gn file and it's probably
needed.
> ../*test_mac.* should probably move to third_party/gtest too.

The Mac bits are in testing/gtest/BUILD.gn --
https://codereview.chromium.org/2852613002/diff/40001/testing/gtest/BUILD.gn

I thought it'd be desirable to have third_party/googletest match upstream as
much as possible. I think I was wrong, because we're not shy about adding files
to other third_party projects, like leveldb. Moving *test_mac* to googletest
would let me remove a hack that I have to introduce now, so I really like it.
The hack is at http://crrev.com/2852613002#msg11

OTOH, gtest_mac.h is #included in a lot of places. Would you be OK with moving
it in a separate CL? -- https://cs.chromium.org/search/?q="testing/gtest_mac.h"

[Nico, 2017-05-01 17:52:54]

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
File build/secondary/testing/gtest/BUILD.gn (right):

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
build/secondary/testing/gtest/BUILD.gn:8: # and will be removed as soon as the
projects switch off of it.
On 2017/04/29 00:16:26, pwnall wrote:
> On 2017/04/28 20:56:23, Nico wrote:
> > I think we should either just remove this or make it forward to the new
> > locations. v8 and pdfium can then merge this change the next time they pull
> > chromium. Having this here might confuse gn and it won't pick up the new
> > BUILD.gn.
> 
> If we remove build/secondary, v8 and pdifum can't merge Chromium until they
land
> a variation of this Google Test migration CL. I was afraid that hard
consistency
> would be a lot more difficult to achieve than eventual consistency.
> 
> FWIW, according to the bots and local results, GN seems to use build/secondary
> as a fallback. So, while humans and other tools might get confused, GN doesn't
> seem to.
> 
> I don't really know how often V8 or PDFium update build/, so I don't know if
> this is an actual problem. Do you still think we should remove build/secondary
> in this CL?

Ok, let's do this separately.

This kind of raises the question: How are pdfium etc use this at all? They'll
need third_party/googletest/BUILD.gn, so I suppose we'll have to ask infra to
mirror out third_party/googletest to https://chromium.googlesource.com/ and then
those projects will have to deps in both that repo and the actual gtest repo?

https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
build/secondary/testing/gtest/BUILD.gn:94: if ((is_mac || is_ios) &&
gtest_include_objc_support) {
On 2017/04/29 00:16:26, pwnall wrote:
> On 2017/04/28 20:56:23, Nico wrote:
> > I think this bit is missing from the new BUILD.gn file and it's probably
> needed.
> > ../*test_mac.* should probably move to third_party/gtest too.
> 
> The Mac bits are in testing/gtest/BUILD.gn --
> https://codereview.chromium.org/2852613002/diff/40001/testing/gtest/BUILD.gn
> 
> I thought it'd be desirable to have third_party/googletest match upstream as
> much as possible. I think I was wrong, because we're not shy about adding
files
> to other third_party projects, like leveldb.

I think in general we try to keep third-party libs unchanged, leveldb is a bit
of an exception (though there are others). But since this isn't really changing
gtest but just adding a file to it, maybe it's better. (The plan was originally
to add gtest_mac to upstream gtest, but upstream didn't like it.)

> Moving *test_mac* to googletest
> would let me remove a hack that I have to introduce now, so I really like it.
> The hack is at http://crrev.com/2852613002#msg11
> 
> OTOH, gtest_mac.h is #included in a lot of places. Would you be OK with moving
> it in a separate CL? --
https://cs.chromium.org/search/?q=%22testing/gtest_mac.h%22

Yes, let's wait with that if it's possible to fix the problem that caused the
revert without doing it. I thought maybe it's needed to fix that link error. (If
we have to move it, I'd suggest to add a forwarding header for it too, like for
all the other files in regular gtest.)

[pwnall, 2017-05-02 01:34:03]

On 2017/05/01 17:52:54, Nico wrote:
>
https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
> File build/secondary/testing/gtest/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
> build/secondary/testing/gtest/BUILD.gn:8: # and will be removed as soon as the
> projects switch off of it.
> On 2017/04/29 00:16:26, pwnall wrote:
> > On 2017/04/28 20:56:23, Nico wrote:
> > > I think we should either just remove this or make it forward to the new
> > > locations. v8 and pdfium can then merge this change the next time they
pull
> > > chromium. Having this here might confuse gn and it won't pick up the new
> > > BUILD.gn.
> > 
> > If we remove build/secondary, v8 and pdifum can't merge Chromium until they
> land
> > a variation of this Google Test migration CL. I was afraid that hard
> consistency
> > would be a lot more difficult to achieve than eventual consistency.
> > 
> > FWIW, according to the bots and local results, GN seems to use
build/secondary
> > as a fallback. So, while humans and other tools might get confused, GN
doesn't
> > seem to.
> > 
> > I don't really know how often V8 or PDFium update build/, so I don't know if
> > this is an actual problem. Do you still think we should remove
build/secondary
> > in this CL?
> 
> Ok, let's do this separately.
> 
> This kind of raises the question: How are pdfium etc use this at all? They'll
> need third_party/googletest/BUILD.gn, so I suppose we'll have to ask infra to
> mirror out third_party/googletest to https://chromium.googlesource.com/ and
then
> those projects will have to deps in both that repo and the actual gtest repo?
> 
>
https://codereview.chromium.org/2779193002/diff/340001/build/secondary/testin...
> build/secondary/testing/gtest/BUILD.gn:94: if ((is_mac || is_ios) &&
> gtest_include_objc_support) {
> On 2017/04/29 00:16:26, pwnall wrote:
> > On 2017/04/28 20:56:23, Nico wrote:
> > > I think this bit is missing from the new BUILD.gn file and it's probably
> > needed.
> > > ../*test_mac.* should probably move to third_party/gtest too.
> > 
> > The Mac bits are in testing/gtest/BUILD.gn --
> > https://codereview.chromium.org/2852613002/diff/40001/testing/gtest/BUILD.gn
> > 
> > I thought it'd be desirable to have third_party/googletest match upstream as
> > much as possible. I think I was wrong, because we're not shy about adding
> files
> > to other third_party projects, like leveldb.
> 
> I think in general we try to keep third-party libs unchanged, leveldb is a bit
> of an exception (though there are others). But since this isn't really
changing
> gtest but just adding a file to it, maybe it's better. (The plan was
originally
> to add gtest_mac to upstream gtest, but upstream didn't like it.)
> 
> > Moving *test_mac* to googletest
> > would let me remove a hack that I have to introduce now, so I really like
it.
> > The hack is at http://crrev.com/2852613002#msg11
> > 
> > OTOH, gtest_mac.h is #included in a lot of places. Would you be OK with
moving
> > it in a separate CL? --
> https://cs.chromium.org/search/?q=%22testing/gtest_mac.h%22
> 
> Yes, let's wait with that if it's possible to fix the problem that caused the
> revert without doing it. I thought maybe it's needed to fix that link error.
(If
> we have to move it, I'd suggest to add a forwarding header for it too, like
for
> all the other files in regular gtest.)

I think https://crrev.com/2852613002/ is a minimally risky patch, and I think
that the easiest path forward is to land that patch, then move files afterwards.
Moving should be a smaller CL, and should have a smaller impact if it gets
reverted.

If you agree, can you please LGTM the CL? Based on the comments in this CL, I'd
imagine it's fine to TBR everyone else, after you take a look.

[Nico, 2017-05-02 20:39:51]

done

On Mon, May 1, 2017 at 9:34 PM, <pwnall@chromium.org> wrote:

> On 2017/05/01 17:52:54, Nico wrote:
> >
> https://codereview.chromium.org/2779193002/diff/340001/
> build/secondary/testing/gtest/BUILD.gn
> > File build/secondary/testing/gtest/BUILD.gn (right):
> >
> >
> https://codereview.chromium.org/2779193002/diff/340001/
> build/secondary/testing/gtest/BUILD.gn#newcode8
> > build/secondary/testing/gtest/BUILD.gn:8: # and will be removed as soon
> as the
> > projects switch off of it.
> > On 2017/04/29 00:16:26, pwnall wrote:
> > > On 2017/04/28 20:56:23, Nico wrote:
> > > > I think we should either just remove this or make it forward to the
> new
> > > > locations. v8 and pdfium can then merge this change the next time
> they
> pull
> > > > chromium. Having this here might confuse gn and it won't pick up the
> new
> > > > BUILD.gn.
> > >
> > > If we remove build/secondary, v8 and pdifum can't merge Chromium until
> they
> > land
> > > a variation of this Google Test migration CL. I was afraid that hard
> > consistency
> > > would be a lot more difficult to achieve than eventual consistency.
> > >
> > > FWIW, according to the bots and local results, GN seems to use
> build/secondary
> > > as a fallback. So, while humans and other tools might get confused, GN
> doesn't
> > > seem to.
> > >
> > > I don't really know how often V8 or PDFium update build/, so I don't
> know if
> > > this is an actual problem. Do you still think we should remove
> build/secondary
> > > in this CL?
> >
> > Ok, let's do this separately.
> >
> > This kind of raises the question: How are pdfium etc use this at all?
> They'll
> > need third_party/googletest/BUILD.gn, so I suppose we'll have to ask
> infra to
> > mirror out third_party/googletest to https://chromium.googlesource.com/
> and
> then
> > those projects will have to deps in both that repo and the actual gtest
> repo?
> >
> >
> https://codereview.chromium.org/2779193002/diff/340001/
> build/secondary/testing/gtest/BUILD.gn#newcode94
> > build/secondary/testing/gtest/BUILD.gn:94: if ((is_mac || is_ios) &&
> > gtest_include_objc_support) {
> > On 2017/04/29 00:16:26, pwnall wrote:
> > > On 2017/04/28 20:56:23, Nico wrote:
> > > > I think this bit is missing from the new BUILD.gn file and it's
> probably
> > > needed.
> > > > ../*test_mac.* should probably move to third_party/gtest too.
> > >
> > > The Mac bits are in testing/gtest/BUILD.gn --
> > > https://codereview.chromium.org/2852613002/diff/40001/
> testing/gtest/BUILD.gn
> > >
> > > I thought it'd be desirable to have third_party/googletest match
> upstream as
> > > much as possible. I think I was wrong, because we're not shy about
> adding
> > files
> > > to other third_party projects, like leveldb.
> >
> > I think in general we try to keep third-party libs unchanged, leveldb is
> a bit
> > of an exception (though there are others). But since this isn't really
> changing
> > gtest but just adding a file to it, maybe it's better. (The plan was
> originally
> > to add gtest_mac to upstream gtest, but upstream didn't like it.)
> >
> > > Moving *test_mac* to googletest
> > > would let me remove a hack that I have to introduce now, so I really
> like
> it.
> > > The hack is at http://crrev.com/2852613002#msg11
> > >
> > > OTOH, gtest_mac.h is #included in a lot of places. Would you be OK with
> moving
> > > it in a separate CL? --
> > https://cs.chromium.org/search/?q=%22testing/gtest_mac.h%22
> >
> > Yes, let's wait with that if it's possible to fix the problem that
> caused the
> > revert without doing it. I thought maybe it's needed to fix that link
> error.
> (If
> > we have to move it, I'd suggest to add a forwarding header for it too,
> like
> for
> > all the other files in regular gtest.)
>
> I think https://crrev.com/2852613002/ is a minimally risky patch, and I
> think
> that the easiest path forward is to land that patch, then move files
> afterwards.
> Moving should be a smaller CL, and should have a smaller impact if it gets
> reverted.
>
> If you agree, can you please LGTM the CL? Based on the comments in this
> CL, I'd
> imagine it's fine to TBR everyone else, after you take a look.
>
> https://codereview.chromium.org/2779193002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.