Comment on attachment 759863 [details] [diff] [review] move SSRCS to moz.build (logic). Add SSRCS as a passthrough variable

Comment on attachment 760369 [details] [diff] [review] move SSRCS to moz.build (logic). rebase, r=ted carried forward.

Comment on attachment 760369 [details] [diff] [review] move SSRCS to moz.build (logic). Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b15bb52a236 committed changeset 134505:1b15bb52a236

gfx/skia - Triple negative Makefile.in conditional for INTEL_ARCHITECTURE should be inverted when memset.arm.S is assigned.

Comment on attachment 767365 [details] [diff] [review] move SSRCS to mozbuild (file batch #1) https://tbpl.mozilla.org/?tree=Try&rev=3f0bd084ae32 USE_ARM_NEON_GCC and USE_ARM_SIMD_GCC are both local defines. Test HAVE_ARM_NEON and HAVE_ARM_SIMD instead.

Comment on attachment 768535 [details] [diff] [review] move SSRCS to mozbuild (file batch #1). Try results: http://tbpl.mozilla.org/?tree=Try&rev=2f51dbe8607d Bg failure (x2) is known: bug 848604

Comment on attachment 768535 [details] [diff] [review] move SSRCS to mozbuild (file batch #1). >diff --git a/gfx/cairo/libpixman/src/moz.build b/gfx/cairo/libpixman/src/moz.build >--- a/gfx/cairo/libpixman/src/moz.build >+++ b/gfx/cairo/libpixman/src/moz.build >@@ -6,8 +6,19 @@ > > MODULE = 'libpixman' > > EXPORTS += [ > 'pixman-version.h', > 'pixman.h', > ] > >+if CONFIG['OS_ARCH'] != 'Darwin' and CONFIG['GNU_CC']: >+ if CONFIG['HAVE_ARM_NEON']: >+ SSRCS += [ >+ 'pixman-arm-neon-asm-bilinear.S', >+ 'pixman-arm-neon-asm.S', >+ ] >+ if CONFIG['HAVE_ARM_SIMD']: >+ SSRCS += [ >+ 'pixman-arm-simd-asm-scaled.S', >+ 'pixman-arm-simd-asm.S', >+ ] The Makefile.in also has a check for 'arm' in OS_TEST before setting these. Do we not need this because HAVE_ARM_NEON and HAVE_ARM_SIMD only get defined if we're on arm? (That would make sense, so I wonder why the Makefile has the check...) Also recommend moving this comment over from the Makefile.in: # Apple's arm assembler doesn't support the same syntax as # the standard GNU assembler, so use the C fallback paths for now. # This may be fixable if clang's ARM/iOS assembler improves into a # viable solution in the future. >diff --git a/gfx/skia/moz.build b/gfx/skia/moz.build >--- a/gfx/skia/moz.build >+++ b/gfx/skia/moz.build >@@ -469,10 +469,15 @@ CPP_SOURCES += [ > 'SkTypeface.cpp', > 'SkTypefaceCache.cpp', > 'SkUnPreMultiply.cpp', > 'SkUtils.cpp', > 'SkWriter32.cpp', > 'SkXfermode.cpp', > ] > >+if not CONFIG['INTEL_ARCHITECTURE'] and CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC'] == '1': >+ SSRCS += [ >+ 'memset.arm.S', >+ ] >+ You can just use CONFIG['GNU_CC'] rather than explicitly checking that it == '1'.

Comment on attachment 768535 [details] [diff] [review] move SSRCS to mozbuild (file batch #1). Try results: https://tbpl.mozilla.org/?tree=Try&rev=2f51dbe8607d "GAIA_APPDIRS is not defined": osx-1.6-opt, Fedora-opt

(In reply to Michael Shal [:mshal] from comment #12) > Comment on attachment 768535 [details] [diff] [review] > move SSRCS to mozbuild (file batch #1). > > >diff --git a/gfx/cairo/libpixman/src/moz.build b/gfx/cairo/libpixman/src/moz.build > >--- a/gfx/cairo/libpixman/src/moz.build > >+++ b/gfx/cairo/libpixman/src/moz.build > >@@ -6,8 +6,19 @@ > > > > MODULE = 'libpixman' > > > > EXPORTS += [ > > 'pixman-version.h', > > 'pixman.h', > > ] > > > >+if CONFIG['OS_ARCH'] != 'Darwin' and CONFIG['GNU_CC']: > >+ if CONFIG['HAVE_ARM_NEON']: > >+ SSRCS += [ > >+ 'pixman-arm-neon-asm-bilinear.S', > >+ 'pixman-arm-neon-asm.S', > >+ ] > >+ if CONFIG['HAVE_ARM_SIMD']: > >+ SSRCS += [ > >+ 'pixman-arm-simd-asm-scaled.S', > >+ 'pixman-arm-simd-asm.S', > >+ ] > > The Makefile.in also has a check for 'arm' in OS_TEST before setting these. > Do we not need this because HAVE_ARM_NEON and HAVE_ARM_SIMD only get defined > if we're on arm? (That would make sense, so I wonder why the Makefile has > the check...) [fyi] The OS_TEST check is not reliable according to bug 886689 http://bugzilla.mozilla.org/show_bug.cgi?id=886689 To answer your question yes, configure is explicitly testing CPU_ARCH == 'arm' so testing for it would be redundant. > Also recommend moving this comment over from the Makefile.in: > > # Apple's arm assembler doesn't support the same syntax as > # the standard GNU assembler, so use the C fallback paths for now. > # This may be fixable if clang's ARM/iOS assembler improves into a > # viable solution in the future. Will do > >diff --git a/gfx/skia/moz.build b/gfx/skia/moz.build > >--- a/gfx/skia/moz.build > >+++ b/gfx/skia/moz.build > >+if not CONFIG['INTEL_ARCHITECTURE'] and CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC'] == '1': > >+ SSRCS += [ > >+ 'memset.arm.S', > >+ ] > >+ > > You can just use CONFIG['GNU_CC'] rather than explicitly checking that it == > '1'. Literal translation from the conversion, will also crush this down to a boolean test.

Same patch as last time but added the apple comment block. Crushed if x==1 conditional to if x

Comment on attachment 769776 [details] [diff] [review] move SSRCS to mozbuild (file batch #1) Looks good to me now!

Comment on attachment 769776 [details] [diff] [review] move SSRCS to mozbuild (file batch #1) Inbound push https://hg.mozilla.org/integration/mozilla-inbound/rev/024f4fcbdfdb committed changeset 137061:024f4fcbdfdb

Comment on attachment 775958 [details] [diff] [review] Cleanup/final patch - remove DISABLED_SSRCS. Remove DISABLED_ tokens and SSRCS can be crossed off the list.

Comment on attachment 775958 [details] [diff] [review] Cleanup/final patch - remove DISABLED_SSRCS. Looks good!

Comment on attachment 775958 [details] [diff] [review] Cleanup/final patch - remove DISABLED_SSRCS. Inbound push http://hg.mozilla.org/integration/mozilla-inbound/rev/3d33faf4d81f