enable android x86 build

This patch is to enable chromium x86 android build. 

The patch includes two parts:
1. gyp files modification to include x86 android build. 
2. add android config files for yasm which is copied from linux ones. 

BUG=125160
TEST=NONE

[Wei James, 2012-04-26 11:45:00]

This patch is to enable x86 android build. 

patch includes two parts:
1. gyp files modification to include x86 android build. 
2. add android config files for yasm which is copied from linux ones. 

This patch needs to work with three patches for third party components:
1. skia/src  http://codereview.appspot.com/6124050/
2. libvpx.gyp in libvpx: https://chromiumcodereview.appspot.com/10232002/
3. libjpeg.gyp in libjpeg_turbo:
https://chromiumcodereview.appspot.com/10234002/

[John Grabowski, 2012-04-26 21:53:07]

You will also need review by
- OWNER for base
- OWNER for skia
- someone with yasm-foo (not me)

https://chromiumcodereview.appspot.com/10198027/diff/1/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/1/build/common.gypi#newc...
build/common.gypi:805: 'platform_arch%': 'arm',
Might prefer a gyp style if/else (e.g. conditions have an else clause)

https://chromiumcodereview.appspot.com/10198027/diff/1/build/common.gypi#newc...
build/common.gypi:2379: '-Wl, --icf=safe', # Enable identical code folding to
reduce size.
>80

https://chromiumcodereview.appspot.com/10198027/diff/1/skia/skia.gyp
File skia/skia.gyp (right):

https://chromiumcodereview.appspot.com/10198027/diff/1/skia/skia.gyp#newcode1102
skia/skia.gyp:1102: [ 'OS != "android"', {
Add explicit comment this conditional is hit for x86 Android else it seems
misplaced

[Wei James, 2012-04-27 00:29:37]

thanks for the review. 

I have added reviewers in base/OWNER and skia/OWNER and owner of
yasm/source/config/linux/config.h. 

thanks for your guidance and I will fix the issue you mentioned and update the
patch. thanks

[Wei James, 2012-04-27 01:25:07]

patch updated to address john's review comments. 

update for the three third party components patch: 
1. libvpx one already landed. https://chromiumcodereview.appspot.com/10232002/
2. skia/src: tom think it is LGTM and will land it soon.
http://codereview.appspot.com/6124050/
3. libjpeg.gyp: LGTM from qinmin but need a LGTM from a full committer. 
https://chromiumcodereview.appspot.com/10234002/

[John Grabowski, 2012-04-27 01:29:16]

build/android and build/ lg
Please find an appropriate yasm reviewer
I will add a committer-ell-gee-tee-em once yasm is properly reviewed

[Wei James, 2012-04-27 01:49:20]

On 2012/04/27 01:29:16, John Grabowski wrote:
> build/android and build/ lg
> Please find an appropriate yasm reviewer
> I will add a committer-ell-gee-tee-em once yasm is properly reviewed

jrg, thanks for your help. 

I added Paweł Hajdan Jr(phajdan.jr@chromium.org) in the reviewer list because I
found he is the committer of yasm linux config files. 

is he appropriate for yasm review or I need to find another one? thanks

[Wei James, 2012-04-27 01:58:22]

qinmin, hbono, 

as this yasm code is used by libjpeg, could you help to review it? they are
copied from linux ones.

[vandebo (ex-Chrome), 2012-04-27 02:12:16]

LGTM for skia changes.

[Hironori Bono, 2012-04-27 02:38:38]

Greetings,

(+hclam)
I have added hclam for yasm changes.

Regards,

Hironori Bono

[Hironori Bono, 2012-04-27 02:41:54]

Greetings,

Oops, I forgot adding wez. Sorry.

Regards,

Hironori Bono

[Nico, 2012-04-27 05:29:22]

This looks fine, a few minor comments below. If 
third_party/yasm/README.chromium  needs updating (if you did something not in
there), please do so.

Please expand the changelist description a bit: Use sentence case, full
capitalization, and add a bit of detail (the first few bits of the contents of
your first message on the review thread would suffice).

Thanks!

http://codereview.chromium.org/10198027/diff/5001/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/10198027/diff/5001/build/android/envsetup.sh#n...
build/android/envsetup.sh:48: # If the TARGET_PRODUCT wasn't set, use 'full' by
default.
s/the //

http://codereview.chromium.org/10198027/diff/5001/build/android/envsetup.sh#n...
build/android/envsetup.sh:51: fi
You can remove this...

http://codereview.chromium.org/10198027/diff/5001/build/android/envsetup.sh#n...
build/android/envsetup.sh:55: case "${TARGET_PRODUCT}" in
...and say "${TARGET_PRODUCT-full}" here, that will use TARGET_PRODUCT if not
null and 'full' else.

http://codereview.chromium.org/10198027/diff/5001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10198027/diff/5001/build/common.gypi#newcode807
build/common.gypi:807: 'platform_arch%': 'x86',
Remove trailing %

http://codereview.chromium.org/10198027/diff/5001/build/common.gypi#newcode809
build/common.gypi:809: 'platform_arch%': 'arm',
Remove trailing %

http://codereview.chromium.org/10198027/diff/5001/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/10198027/diff/5001/skia/skia.gyp#newcode1102
skia/skia.gyp:1102: # Remove SSSE3 opt for x86 Android.
Change the comment to say why, not what (e.g. "Android doesn't support SSSE3
instructions." or whatever the reason is).

http://codereview.chromium.org/10198027/diff/5001/third_party/yasm/source/con...
File third_party/yasm/source/config/android/Makefile (right):

http://codereview.chromium.org/10198027/diff/5001/third_party/yasm/source/con...
third_party/yasm/source/config/android/Makefile:2: # Makefile.  Generated from
Makefile.in by configure.
Was this created by following  third_party/yasm/README.chromium ? (If you copied
and modified the linux makefile, use `git cp` instead, then rietveld will show
your changes)

http://codereview.chromium.org/10198027/diff/5001/third_party/yasm/source/con...
File third_party/yasm/source/config/android/config.h (right):

http://codereview.chromium.org/10198027/diff/5001/third_party/yasm/source/con...
third_party/yasm/source/config/android/config.h:2: /* config.h.in.  Generated
from configure.ac by autoheader.  */
Here too: If this was copied, please use `git cp` (or `svn cp`), so that that's
clear in the change history.

[Paweł Hajdan Jr., 2012-04-27 06:56:19]

https://chromiumcodereview.appspot.com/10198027/diff/5001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/base/base.gypi#newc...
base/base.gypi:682: 'sources/': [
nit: Why don't you just use 'sources!' instead?

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:806: ['target_arch == "ia32"', {
Wait, target_arch is our (Chromium) thing, so it should be possible to adjust it
to the Android naming scheme so that we don't need those confusing conversions.

I don't expect you to do it right now, but please file a bug and put a TODO here
linking to the bug.

It's important!

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:808: }, {
I don't think the assumption that non-ia32 is an arm will always hold. This
seems fragile.

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:2381: '-Wl, --icf=safe',
nit: Are you sure there should be a space between the comma and "--"? Even if
that works, no space seems safer.

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
File third_party/yasm/source/config/android/Makefile (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
third_party/yasm/source/config/android/Makefile:1: # Makefile.in generated by
automake 1.10.1 from Makefile.am.
We generally don't check in generated files. Shouldn't gyp be used for the yasm
build? What's going on here?

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
File third_party/yasm/source/config/android/config.h (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
third_party/yasm/source/config/android/config.h:2: /* config.h.in.  Generated
from configure.ac by autoheader.  */
On 2012/04/27 05:29:22, Nico wrote:
> Here too: If this was copied, please use `git cp` (or `svn cp`), so that
that's
> clear in the change history.

Yup. In this case checking in config.h is fine, just use "$VCS cp" as Nico said.

[Wei James, 2012-04-27 08:59:05]

thanks for your review. patch updated according to your comments. but I cannot
find "git cp" command.

https://chromiumcodereview.appspot.com/10198027/diff/5001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/base/base.gypi#newc...
base/base.gypi:682: 'sources/': [
On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> nit: Why don't you just use 'sources!' instead?

this code is just copied from above line#658. do you mean "sources"? because I
would like to include the files for target build just like linux host build. I
tested with both "sources" and "sources!", but it seems the files are not
included so there is undefined error when linking. I am not a gyp expert and
cannot find the root cause yet. 

I saw most of the sources in this file use "sources/" style. is it ok to keep it
to be consistent with others? thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/android/envse...
File build/android/envsetup.sh (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/android/envse...
build/android/envsetup.sh:48: # If the TARGET_PRODUCT wasn't set, use 'full' by
default.
On 2012/04/27 05:29:22, Nico wrote:
> s/the //
fixed. thanks.

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/android/envse...
build/android/envsetup.sh:55: case "${TARGET_PRODUCT}" in
On 2012/04/27 05:29:22, Nico wrote:
> ...and say "${TARGET_PRODUCT-full}" here, that will use TARGET_PRODUCT if not
> null and 'full' else.
fixed. thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:806: ['target_arch == "ia32"', {
On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> Wait, target_arch is our (Chromium) thing, so it should be possible to adjust
it
> to the Android naming scheme so that we don't need those confusing
conversions.
> 
> I don't expect you to do it right now, but please file a bug and put a TODO
here
> linking to the bug.
> 
> It's important!

fixed with target_arch. thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:807: 'platform_arch%': 'x86',
On 2012/04/27 05:29:22, Nico wrote:
> Remove trailing %

changed to target_arch and removed %. thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:808: }, {
On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> I don't think the assumption that non-ia32 is an arm will always hold. This
> seems fragile.

use target_arch and only change "ia32" to "x86" when target_arch == "ia32".
thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:809: 'platform_arch%': 'arm',
On 2012/04/27 05:29:22, Nico wrote:
> Remove trailing %

fixed. thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/build/common.gypi#n...
build/common.gypi:2381: '-Wl, --icf=safe',
On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> nit: Are you sure there should be a space between the comma and "--"? Even if
> that works, no space seems safer.
fixed. thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/skia/skia.gyp
File skia/skia.gyp (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/skia/skia.gyp#newco...
skia/skia.gyp:1102: # Remove SSSE3 opt for x86 Android.
On 2012/04/27 05:29:22, Nico wrote:
> Change the comment to say why, not what (e.g. "Android doesn't support SSSE3
> instructions." or whatever the reason is).
fixed. thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
File third_party/yasm/source/config/android/Makefile (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
third_party/yasm/source/config/android/Makefile:2: # Makefile.  Generated from
Makefile.in by configure.
On 2012/04/27 05:29:22, Nico wrote:
> Was this created by following  third_party/yasm/README.chromium ? (If you
copied
> and modified the linux makefile, use `git cp` instead, then rietveld will show
> your changes)
I am using git but there is not git cp command like svn. 
could you give me some suggestion for it? thanks

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
File third_party/yasm/source/config/android/config.h (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
third_party/yasm/source/config/android/config.h:2: /* config.h.in.  Generated
from configure.ac by autoheader.  */
On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> On 2012/04/27 05:29:22, Nico wrote:
> > Here too: If this was copied, please use `git cp` (or `svn cp`), so that
> that's
> > clear in the change history.
> 
> Yup. In this case checking in config.h is fine, just use "$VCS cp" as Nico
said.

cannot find "git cp" command. what should I do for it? thanks

[Paweł Hajdan Jr., 2012-04-27 09:40:06]

https://chromiumcodereview.appspot.com/10198027/diff/5001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/base/base.gypi#newc...
base/base.gypi:682: 'sources/': [
On 2012/04/27 08:59:05, Wei James wrote:
> On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> > nit: Why don't you just use 'sources!' instead?
> 
> this code is just copied from above line#658. do you mean "sources"? because I
> would like to include the files for target build just like linux host build. I
> tested with both "sources" and "sources!", but it seems the files are not
> included so there is undefined error when linking. I am not a gyp expert and
> cannot find the root cause yet. 

Ah right, it's "include" so just "sources" please.

> I saw most of the sources in this file use "sources/" style. is it ok to keep
it
> to be consistent with others? thanks 

It's simpler to use "sources" to include files and "sources!" to exclude them.
I'd rather convert the surrounding code (possibly in a separate CL) instead of
keeping the weird style (maybe someone just didn't know gyp well).

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
File third_party/yasm/source/config/android/config.h (right):

https://chromiumcodereview.appspot.com/10198027/diff/5001/third_party/yasm/so...
third_party/yasm/source/config/android/config.h:2: /* config.h.in.  Generated
from configure.ac by autoheader.  */
On 2012/04/27 08:59:05, Wei James wrote:
> On 2012/04/27 06:56:19, Paweł Hajdan Jr. wrote:
> > On 2012/04/27 05:29:22, Nico wrote:
> > > Here too: If this was copied, please use `git cp` (or `svn cp`), so that
> > that's
> > > clear in the change history.
> > 
> > Yup. In this case checking in config.h is fine, just use "$VCS cp" as Nico
> said.
> 
> cannot find "git cp" command. what should I do for it? thanks 

Indeed there is no "git cp", just ignore it (at least for me that's fine).

https://chromiumcodereview.appspot.com/10198027/diff/18001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/18001/build/common.gypi#...
build/common.gypi:813: 'target_arch': 'x86',
My comments that it is a hack still apply. Please file the bug and stick the
TODO.

[Wei James, 2012-04-27 10:00:28]

added a TODO to address chromium/android target_arch naming scheme issue.

https://chromiumcodereview.appspot.com/10198027/diff/18001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10198027/diff/18001/build/common.gypi#...
build/common.gypi:813: 'target_arch': 'x86',
On 2012/04/27 09:40:06, Paweł Hajdan Jr. wrote:
> My comments that it is a hack still apply. Please file the bug and stick the
> TODO.
fixed. thanks

[Mark Mentovai, 2012-04-27 15:05:41]

LGTM

[Wei James, 2012-04-27 16:12:28]

thanks you all for your review. 

can I check the commit bit now? thanks

[commit-bot: I haz the power, 2012-04-27 16:13:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wistoch@gmail.com/10198027/15004

[commit-bot: I haz the power, 2012-04-27 16:14:03]

Presubmit check for 10198027-15004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
wistoch@gmail.com is not in AUTHORS file. If you are a new contributor, please
visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 1.4s to calculate.

[Wei James, 2012-04-27 16:20:38]

anybody can help me on the AUTHORS issue? 

I am from Intel and the legal department has already put my name in CLA list
with the email address: james.wei@intel.com. 

what should I do to associate wistoch@gmail.com to james.wei@intel.com? 

thanks

[M-A Ruel, 2012-04-27 16:57:56]

On 2012/04/27 16:20:38, Wei James wrote:
> anybody can help me on the AUTHORS issue? 
> 
> I am from Intel and the legal department has already put my name in CLA list
> with the email address: mailto:james.wei@intel.com. 
> 
> what should I do to associate mailto:wistoch@gmail.com to james.wei@intel.com?

> 
> thanks

The Commit queue will use the Author credential to commit the CL on your behalf.
As such, the code review author email address is important.

Unless Intel is using Google Apps, which I presume doesn't, you can create a
Google Account for your email address james.wei@intel.com at
https://accounts.google.com/NewAccount. Then you can upload from there and the
CL will be properly committed. The CL would show up at
https://chromiumcodereview.appspot.com/user/james.wei@intel.com.

In practice, I can change the ownership of an issue but this is a manual
process. I need to have you log in to rietveld with the proper account first.

[Wei James(wistoch), 2012-04-27 23:24:23]

On 2012/04/27 16:57:56, Marc-Antoine Ruel wrote:
> On 2012/04/27 16:20:38, Wei James wrote:
> > anybody can help me on the AUTHORS issue? 
> > 
> > I am from Intel and the legal department has already put my name in CLA list
> > with the email address: mailto:james.wei@intel.com. 
> > 
> > what should I do to associate mailto:wistoch@gmail.com to
james.wei@intel.com?
> 
> > 
> > thanks
> 
> The Commit queue will use the Author credential to commit the CL on your
behalf.
> As such, the code review author email address is important.
> 
> Unless Intel is using Google Apps, which I presume doesn't, you can create a
> Google Account for your email address mailto:james.wei@intel.com at
> https://accounts.google.com/NewAccount. Then you can upload from there and the
> CL will be properly committed. The CL would show up at
> https://chromiumcodereview.appspot.com/user/james.wei%40intel.com.
> 
> In practice, I can change the ownership of an issue but this is a manual
> process. I need to have you log in to rietveld with the proper account first.

thanks. I have a Google Apps account with james.wei@intel.com. could you help me
to change the ownership of this issue? thanks 

BTW. the Intel Google Apps system is not convenient for me because I cannot
change the password(I can only use a web page to reset it and the password is
hard to remember).  Previously I have some colleagues work with @gmail.com
account and can associate the @intel.com account with the @gamil.com when they
contribute the first patch. 

can I do it too? thanks

[Wei James(wistoch), 2012-05-02 08:48:13]

a new issue cloned with correct owner:
https://chromiumcodereview.appspot.com/10271039/ 

could you kindly help to review the patch there? thanks

[erikwright (departed), 2012-05-03 18:34:09]

On 2012/05/02 08:48:13, James Wei wrote:
> a new issue cloned with correct owner:
> https://chromiumcodereview.appspot.com/10271039/ 
> 
> could you kindly help to review the patch there? thanks

I gather this CL can now be closed?