Fix skia build when not on armv7.

Fix skia build when not on armv7.

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=165400

[Stephen White, 2012-10-29 20:58:19]

https://codereview.chromium.org/11342016/diff/1/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/11342016/diff/1/skia/skia.gyp#newcode754
skia/skia.gyp:754: '../third_party/skia/src/opts/opts_check_arm.cpp',
Not your fault, but this thing is a growing mess.  I would prefer that all of
the armv7-specific files on line 725 were made conditional on armv7 (and the
_none.cpp files were added in a new 'target_arch == "arm" and armv7 == 0 clause
below it), rather than adding the _arm files there and then excluding them here.
 Could you take care of that?

[Paweł Hajdan Jr., 2012-10-29 21:21:01]

PTAL

[Stephen White, 2012-10-29 22:16:35]

https://codereview.chromium.org/11342016/diff/3/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/11342016/diff/3/skia/skia.gyp#newcode750
skia/skia.gyp:750: '../third_party/skia/src/opts/opts_check_arm.cpp',
No, this doesn't seem right for armv7.  You're excluding the files here, but
never adding them above.

Let me try adding a patch to show you what I mean.

[Stephen White, 2012-10-29 22:31:21]

On 2012/10/29 22:16:35, Stephen White wrote:
> https://codereview.chromium.org/11342016/diff/3/skia/skia.gyp
> File skia/skia.gyp (right):
> 
> https://codereview.chromium.org/11342016/diff/3/skia/skia.gyp#newcode750
> skia/skia.gyp:750: '../third_party/skia/src/opts/opts_check_arm.cpp',
> No, this doesn't seem right for armv7.  You're excluding the files here, but
> never adding them above.
> 
> Let me try adding a patch to show you what I mean.

Here's a draft patch (untested).

diff --git a/skia/skia.gyp b/skia/skia.gyp
index 747427c..8e7dfac 100644
--- a/skia/skia.gyp
+++ b/skia/skia.gyp
@@ -721,6 +721,15 @@
           'cflags': [
             '-fomit-frame-pointer',
           ],
+        }],
+        [ 'target_arch == arm' and 'armv7 == 0', {
+          'sources': [
+            '../third_party/skia/src/opts/SkBitmapProcState_opts_none.cpp',
+            '../third_party/skia/src/opts/SkBlitRow_opts_none.cpp',
+            '../third_party/skia/src/opts/SkBlitRow_opts_none.h',
+          ],
+        }],
+        [ 'armv7 == 1', {
           'sources': [
             '../third_party/skia/src/opts/SkBitmapProcState_opts_arm.cpp',
             '../third_party/skia/src/opts/SkBlitRow_opts_arm.cpp',
@@ -744,14 +753,6 @@
             '../third_party/skia/src/opts/SkBlitRow_opts_arm_neon.cpp',
         ],
         }],
-        [ 'target_arch == "arm" and armv7 != 1', {
-          'sources': [
-            '../third_party/skia/src/opts/SkBlitRow_opts_none.cpp',
-          ],
-          'sources!': [
-            '../third_party/skia/src/opts/SkBlitRow_opts_arm.cpp',
-          ],
-        }],
       ],
     },
     # For the same lame reasons as what is done for skia_opts, we have to

Note that I actually turned off SkBitmapProcState_opts_arm.cpp for non-armv7
too, just for symmetry.  If in fact it does work, you can turn it back on.

[Stephen White, 2012-10-29 22:34:49]

On 2012/10/29 22:31:21, Stephen White wrote:
> On 2012/10/29 22:16:35, Stephen White wrote:
> > https://codereview.chromium.org/11342016/diff/3/skia/skia.gyp
> > File skia/skia.gyp (right):
> > 
> > https://codereview.chromium.org/11342016/diff/3/skia/skia.gyp#newcode750
> > skia/skia.gyp:750: '../third_party/skia/src/opts/opts_check_arm.cpp',
> > No, this doesn't seem right for armv7.  You're excluding the files here, but
> > never adding them above.
> > 
> > Let me try adding a patch to show you what I mean.
> 
> Here's a draft patch (untested).
> 
> diff --git a/skia/skia.gyp b/skia/skia.gyp
> index 747427c..8e7dfac 100644
> --- a/skia/skia.gyp
> +++ b/skia/skia.gyp
> @@ -721,6 +721,15 @@
>            'cflags': [
>              '-fomit-frame-pointer',
>            ],
> +        }],
> +        [ 'target_arch == arm' and 'armv7 == 0', {

Whoops, should be:

> +        [ 'target_arch == "arm" and armv7 == 0', {

[Paweł Hajdan Jr., 2012-10-29 23:17:19]

Is this more like what you want? Is there any trybot I could test this on?

[Stephen White, 2012-10-30 00:49:05]

On 2012/10/29 23:17:19, Paweł Hajdan Jr. wrote:
> Is this more like what you want?

Yes, LGTM thanks.

>  Is there any trybot I could test this on?

I think the android bots should take care of armv7 and higher.  I don't think we
have any non-armv7 bots, but I could be wrong.

[Paweł Hajdan Jr., 2012-10-30 17:18:26]

Does anyone else want to comment on this change?

I'd like to commit within say a day if there is no reply.

[commit-bot: I haz the power, 2012-10-31 18:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/11342016/10002

[commit-bot: I haz the power, 2012-11-01 00:43:48]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[epoger, 2012-11-01 14:12:03]

Note that this change will conflict with
https://codereview.chromium.org/11292003 ('[MIPS] Add build support in Skia for
MIPS.').

I fear the incentives associated with such a warning, but: whoever commits last
will have to manually merge the changes.