Upstream: The Common.gypi change for Android.

Upstream: The Common.gypi change for Android.


BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102007

[michaelbai, 2011-09-19 20:06:45]

Please help to review this.

[Mark Mentovai, 2011-09-20 00:11:58]

http://codereview.chromium.org/7941007/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode576
build/common.gypi:576: 'android_build_type%': 0,
Document what 0 means. What are the other possibilities?

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode597
build/common.gypi:597: # In android, we build the gtest targets as a
shared_library
Please avoid writing “we” in comments. Line 603, 608, and 617-623 as well.

It’s also repetitive to say “Android” in each comment in this section. This is
the Android section. Throughout, lines 571-625.

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode598
build/common.gypi:598: 'gtest_target_type%': '<(gtest_target_type)',
This doesn’t seem right. Why not just say 'gtest_target_type%': 'shared_library'
then?

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode1119
build/common.gypi:1119: ['exclude', '(^|/)android_[^/]*\\.cc$'],
The correct pattern in Chromium is for these files to end in _android.cc. Then
the rule on line 1117 would exclude them. If there aren’t many occurrences, it
would be preferable to rename the files rather than introduce this additional
rule.

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode1765
build/common.gypi:1765: # by remoting.  Remoting is off, so we don't build it,
“We,” also on 1754, 1767, 1773, 1791-1794, 1798, 1804-1810, 1813, 1815, and
elsewhere.

[michaelbai, 2011-09-20 04:01:01]

PTAL

http://codereview.chromium.org/7941007/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode576
build/common.gypi:576: 'android_build_type%': 0,
This is the default build type.

On 2011/09/20 00:11:58, Mark Mentovai wrote:
> Document what 0 means. What are the other possibilities?

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode597
build/common.gypi:597: # In android, we build the gtest targets as a
shared_library
On 2011/09/20 00:11:58, Mark Mentovai wrote:
> Please avoid writing “we” in comments. Line 603, 608, and 617-623 as well.
> 
> It’s also repetitive to say “Android” in each comment in this section. This is
> the Android section. Throughout, lines 571-625.

Done.

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode598
build/common.gypi:598: 'gtest_target_type%': '<(gtest_target_type)',
We are in the transition of share_library,  gtest_target_type is defined as
environment variable. So we can switch quickly, I added a TODO.

On 2011/09/20 00:11:58, Mark Mentovai wrote:
> This doesn’t seem right. Why not just say 'gtest_target_type%':
'shared_library'
> then?

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode1119
build/common.gypi:1119: ['exclude', '(^|/)android_[^/]*\\.cc$'],
On 2011/09/20 00:11:58, Mark Mentovai wrote:
> The correct pattern in Chromium is for these files to end in _android.cc. Then
> the rule on line 1117 would exclude them. If there aren’t many occurrences, it
> would be preferable to rename the files rather than introduce this additional
> rule.

Done.

http://codereview.chromium.org/7941007/diff/1/build/common.gypi#newcode1765
build/common.gypi:1765: # by remoting.  Remoting is off, so we don't build it,
On 2011/09/20 00:11:58, Mark Mentovai wrote:
> “We,” also on 1754, 1767, 1773, 1791-1794, 1798, 1804-1810, 1813, 1815, and
> elsewhere.

Done.

[michaelbai, 2011-09-20 16:48:19]

PTAL

[Mark Mentovai, 2011-09-20 16:56:46]

LGTM as discussed privately.