Issue 1906 - Fix broken Android build

Issue 1906 - Fix broken Android build

[René Jeschke, 2015-01-29 14:22:22]

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
File Makefile (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
Makefile:5: ANDROID_PARAMETERS += arm_neon=0 armv7=0 arm_fpu=off vfp3=off
arm_float_abi=default
The v8 version we upgraded to needs this value to be defined now.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
Makefile:38: $(ANDROID_NDK_ROOT)/ndk-build -C build installed_modules \
As we already require $ANDROID_NDK_ROOT to be defined, I removed the need to
either have the NDK on the PATH or to symlink 'ndk-build' into '~/bin' (which
now fails anyway).

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/liba...
File libadblockplus.gyp (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/liba...
libadblockplus.gyp:52:
'android_arm.release/obj.target/tools/gyp/libv8_base.arm.a',
We are currently hardcoding 'arm' builds anyway, so I just changed the library
reference here. The new v8 seems to have changed the name.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
File test/Notification.cpp (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
test/Notification.cpp:41: std::tr1::shared_ptr<FilterEngine>(new
FilterEngine(jsEngine));
std::tr1::make_shared<T>(...) is not available on the gcc-toolchain version used
by the Android build.

First I thought about just 'polyfilling' std::tr1::make_shared, but the guarding
#ifdefs would have needed a bit of research, so I just replaced all make_shared
calls with direct shared_ptr instantiations.

[Felix Dahlke, 2015-01-29 16:49:25]

Looks great, I bet that took some digging :P

Only some nits from my end.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
File Makefile (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
Makefile:5: ANDROID_PARAMETERS += arm_neon=0 armv7=0 arm_fpu=off vfp3=off
arm_float_abi=default
On 2015/01/29 14:22:22, René Jeschke wrote:
> The v8 version we upgraded to needs this value to be defined now.

Can you add another += line for that, for 80 column goodness? :)

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
Makefile:38: $(ANDROID_NDK_ROOT)/ndk-build -C build installed_modules \
On 2015/01/29 14:22:22, René Jeschke wrote:
> As we already require $ANDROID_NDK_ROOT to be defined, I removed the need to
> either have the NDK on the PATH or to symlink 'ndk-build' into '~/bin' (which
> now fails anyway).
> 

Yeah, I've got multiple NDK versions here, we really shouldn't rely on it being
on the PATH.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
File test/Notification.cpp (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
test/Notification.cpp:41: std::tr1::shared_ptr<FilterEngine>(new
FilterEngine(jsEngine));
On 2015/01/29 14:22:22, René Jeschke wrote:
> std::tr1::make_shared<T>(...) is not available on the gcc-toolchain version
used
> by the Android build.
> 
> First I thought about just 'polyfilling' std::tr1::make_shared, but the
guarding
> #ifdefs would have needed a bit of research, so I just replaced all
make_shared
> calls with direct shared_ptr instantiations.

We can actually go for the shorter:

filterEngine.reset(new FilterEngine(jsEngine));

Same below.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
test/Notification.cpp:85: std::tr1::shared_ptr<LazyFileSystem>(new
LazyFileSystem()));
Next line indent is generally 2 spaces, apparent at some places in this file.
Also below.

[Felix Dahlke, 2015-01-29 16:58:40]

Forgot to mention, but this is not trivial enough to be a Noissue. Can you
create an issue for it?

[René Jeschke, 2015-01-29 17:07:53]

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
File Makefile (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/Make...
Makefile:5: ANDROID_PARAMETERS += arm_neon=0 armv7=0 arm_fpu=off vfp3=off
arm_float_abi=default
On 2015/01/29 16:49:25, Felix H. Dahlke wrote:
> On 2015/01/29 14:22:22, René Jeschke wrote:
> > The v8 version we upgraded to needs this value to be defined now.
> 
> Can you add another += line for that, for 80 column goodness? :)

Done.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
File test/Notification.cpp (right):

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
test/Notification.cpp:41: std::tr1::shared_ptr<FilterEngine>(new
FilterEngine(jsEngine));
On 2015/01/29 16:49:25, Felix H. Dahlke wrote:
> On 2015/01/29 14:22:22, René Jeschke wrote:
> > std::tr1::make_shared<T>(...) is not available on the gcc-toolchain version
> used
> > by the Android build.
> > 
> > First I thought about just 'polyfilling' std::tr1::make_shared, but the
> guarding
> > #ifdefs would have needed a bit of research, so I just replaced all
> make_shared
> > calls with direct shared_ptr instantiations.
> 
> We can actually go for the shorter:
> 
> filterEngine.reset(new FilterEngine(jsEngine));
> 
> Same below.

Done.

http://codereview.adblockplus.org/5708936187478016/diff/5629499534213120/test...
test/Notification.cpp:85: std::tr1::shared_ptr<LazyFileSystem>(new
LazyFileSystem()));
On 2015/01/29 16:49:25, Felix H. Dahlke wrote:
> Next line indent is generally 2 spaces, apparent at some places in this file.
> Also below.

Done.

[Felix Dahlke, 2015-01-29 17:27:36]

LGTM!