[Development] Reviews needed before android integration in two weeks

[Thiago Macieira]

On terça-feira, 5 de fevereiro de 2013 11.33.29, Paul Olav Tvete wrote:
> M       config.tests/qpa/linuxfb/linuxfb.cpp
> M       config.tests/unix/arch.test

+2 up to here.

> M       config.tests/unix/compile.test

This one has superfluous changes. It sets a QMAKE variable that is not used 
anywhere.

> M       config.tests/unix/evdev/evdev.cpp
> M       config.tests/unix/getaddrinfo/getaddrinfotest.cpp

+2
And +ugh! on the getaddrinfotest.cpp. Recheck if it's necessary. Drop if it's 
not.

> A       config.tests/unix/ipc_android/ipc.cpp
> A       config.tests/unix/ipc_android/ipc_android.pro

Questionable copyright on the .cpp file. Looks like it's checking for 
linux/ashmem.h and some macros.

> M       config.tests/unix/makeabs

+1. It's about cross-compilation from windows to unix.

> M       configure

+1 on the msys part, but I can't approve.

-1 on the detection of android. It detects android via one single mkspec name, 
which is hardcoded. This should be done by a getQMakeConf call and extracting 
a variable set there, if necessary. Or by a compile test, like the ashmem 
above.

-1 on the unnecessary whitespace changes. Except that I can't find where they 
were introduced... might be an artifact of my diff.

-1 on this:
+if [ "$PLATFORM" = "win32-g++" ] || [ "$XPLATFORM" = "win32-g++" ] && [ 
"$CFG_MS_BITFIELDS" = "yes" ]; then
+    QMAKE_CONFIG="$QMAKE_CONFIG ms_bitfields"
+fi

The above enables ms_bitfields unconditionally for MinGW, regardless of cross-
compilation or anything else. You're missing parentheses.

-2 on the -openssl-source option. It's not used anywhere.

> A       lib/rules.xml

I'd rather you found a better place for this file.

> A       mkspecs/android-g++/qmake.conf

-1 on CONFIG += ashmem. It looks wrong.

-2 on QT = android. You can't have "android" there, period. That belongs to a 
library called QtAndroid, if and when that exists, and it MUST NOT be default.

If you need that for initialisation, figure out like qtmain / winmain does. 
Just don't touch the QT variable.

The file has some weird indentation.

-1 on setting -g on the release flags. This produces an unlinkable QtWebKit.

> A       mkspecs/android-g++/qplatformdefs.h
> D       mkspecs/common/android/qplatformdefs.h
> D       mkspecs/common/linux-android.conf

Please keep the common files.

> A       mkspecs/features/android.prf

Copyright header is out of style. Please use one in our format.

> A       mkspecs/features/java.prf

Ossi to judge if the mkpath should be there.

javac.depends looks like a horrible hack. In fact, while this is a nice 
feature, the whole file looks like a hack. qmake should be modified to learn how 
to compile Java instead.

> M       mkspecs/features/moc.prf

-1. The change looks wrong. It needs justification.

> A       mkspecs/features/qpa/android.prf

$$[QT_INSTALL_PREFIX]/src makes no sense. Please choose another path.

Also, doesn't qpa/android.prf conflict with android.prf?

> M       mkspecs/features/qpa/genericunixfontdatabase.prf

-1 on coding style.

> M       mkspecs/features/qt_functions.prf

-2 for reasons explained above. Do not modify the QT variable.

> M       mkspecs/features/qt_parts.prf

Changes look incorrect at first glance. Ossi to judge.

> M       mkspecs/features/resources.prf

As the comment says, proper investigation should be done. If it weren't for 
the comment, I'd have given +2.

> M       mkspecs/features/static.prf

There's no linux-mingw-*. I'd like to see this made more generic: add a 
variable to the qmake.conf instead. I can then add -static-intel for icc, for 
example.

> D       mkspecs/unsupported/linux-android-armeabi-g++/qmake.conf
> D       mkspecs/unsupported/linux-android-armeabi-g++/qplatformdefs.h
> D       mkspecs/unsupported/linux-android-armeabi-v7a-g++/qmake.conf
> D       mkspecs/unsupported/linux-android-armeabi-v7a-g++/qplatformdefs.h
> D       mkspecs/unsupported/linux-android-x86-g++/qmake.conf
> D       mkspecs/unsupported/linux-android-x86-g++/qplatformdefs.h

Ok, it's supported now. +2

> M       qmake/generators/makefile.cpp
> M       qmake/generators/unix/unixmake2.cpp
> M       qmake/generators/win32/mingw_make.cpp
> M       qmake/generators/win32/msvc_nmake.cpp

Part of the make-qmake-understand-Java, but incomplete. See above.

> M       qtbase.pro

Unnecessary change.

> A       src/android/...

I suggest the dir be renamed to src/androidmain, to match winmain and the old 
(and thankfully removed) s60main.

> M       src/corelib/arch/arch.pri
> A       src/corelib/arch/qatomic_android.h

-1. Looks superfluous. It's doing exactly what qbasicatomic.h is already doing.

Please justify.

> M       src/corelib/codecs/qiconvcodec.cpp

+2

> M       src/corelib/codecs/qiconvcodec_p.h

+1 but Q_OS_WIN && Q_CC_GNU.

> M       src/corelib/codecs/qtextcodec.cpp
> M       src/corelib/codecs/qtextcodec_p.h

This has been discussed before. Please leave the macro called 
Q_OS_LINUX_ANDROID.

>From this point on, any +2 does is modulo the macro name being changed back.

> M       src/corelib/global/qglobal.cpp
> M       src/corelib/global/qlogging.cpp

+2

> M       src/corelib/global/qsystemdetection.h

-1

Macro name. See above.

> M       src/corelib/io/qabstractfileengine_p.h
> M       src/corelib/io/qfilesystemengine_unix.cpp

+2

> M       src/corelib/io/qfsfileengine_unix.cpp

-1. You need to add a fallback.

> M       src/corelib/io/qtemporarydir.cpp

-1. It looks like Android has mkdtemp, it's just not defined in the headers.

> M       src/corelib/kernel/kernel.pri

0

> M       src/corelib/kernel/qeventdispatcher_unix.cpp
> M       src/corelib/kernel/qeventdispatcher_unix_p.h

This one needs a very good explanation.

> M       src/corelib/kernel/qsharedmemory.cpp

+2

> D       src/corelib/kernel/qsharedmemory_android.cpp

-2. Keep the name, especially since there's qsystemsemaphore_android.cpp.

> A       src/corelib/kernel/qsharedmemory_ashmem.cpp

-1: 
 * need to use qt_safe_{open,close}
 * why the strlcopy? Can't it pass the name parameter?
 * "static" missing in all functions
 * coding style
 * there's a QString creation between the failing system call and checking of 
   errno. That is a bad idea, since malloc() can modify errno. Refactor 
   setErrorString.
	-> additionally, merge it with the _unix implementation.
 * the pin / unpin functions seem to be wrongly used

> M       src/corelib/kernel/qsharedmemory_p.h

-1:
 * coding style (spaces in preprocessor macros)
 * the nested #ifdef look bad. You can do better.

> M       src/corelib/kernel/qsystemsemaphore.cpp

+2

> M       src/corelib/kernel/qsystemsemaphore_android.cpp

-1:
 * code duplication (setErrorString again)
 * coding style

> M       src/corelib/kernel/qsystemsemaphore_p.h

-1: coding style

> M       src/corelib/thread/qbasicatomic.h

-1. See above.

> A       src/corelib/tools/android/cpu-features.c
> A       src/corelib/tools/android/cpu-features.h
> M       src/corelib/tools/qsimd.cpp
> M       src/corelib/tools/tools.pri

Please explain why this is necessary. The checking in qsimd.cpp should be 
enough already.

Move new files to 3rdparty if still valid.

> M       src/gui/image/qimage_neon.cpp

-2. Looks wrong. qsimd_p.h already does that include and this won't compile on 
Android-x86.

> M       src/gui/kernel/qplatforminputcontext.cpp
> M       src/gui/kernel/qplatforminputcontext.h

-2. Superfluous and adds a warning.

> M       src/network/access/qnetworkaccessfilebackend.cpp
> M       src/network/access/qnetworkaccessmanager.cpp
> M       src/network/access/qnetworkreplyfileimpl.cpp

#ifdef missing.

> M       src/network/kernel/qhostinfo_unix.cpp

-2. Fix the configure test instead.

> M       src/network/kernel/qnetworkproxy_win.cpp

-2. The Android assets will never be run on Windows.

> M       src/network/ssl/qsslcertificate.cpp

+2

> M       src/network/ssl/qsslsocket_openssl.cpp

-1: coding style

> M       src/network/ssl/ssl.pri

-1: this unconditionally builds OpenSSL support in, even if the sources aren't 
present. The configure script changes required for this aren't correct.

> A       src/plugins/platforms/android/...

Everything with AOSP or Khronos copyright: move to 3rdparty.

asset file engine: I don't like it. I need to think some more about it.

> M       src/plugins/platforms/cocoa/qcocoamenu.h
> M       src/plugins/platforms/cocoa/qcocoamenuitem.h

-1: coding style

> M       src/plugins/platforms/platforms.pro

-1: superfluous commented out line

> M       src/src.pro

See above about the naming.

> M       src/widgets/graphicsview/qgraphicsitem.cpp
> M       src/widgets/itemviews/qitemdelegate.cpp
> M       src/widgets/kernel/qwidget.cpp

-1 on OS-based policy decisions. This should be in QPA.

> A       src/widgets/styles/json/json.cpp
> A       src/widgets/styles/json/json.g
> A       src/widgets/styles/json/json.h
> A       src/widgets/styles/json/json.pri
> A       src/widgets/styles/json/jsonparser.cpp

-1. Do not add.

You need to show that this is at least 5x faster than the one in QtCore before 
this begins to be acceptable.

I'm not the QtWidgets maintainer. If I were, I'd be giving a -2.

If somehow this is kept, it needs to be moved to 3rdparty.

> M       src/widgets/styles/qcommonstyle.cpp

-1: OS-based policy decisions.

> M       src/widgets/styles/qstylefactory.cpp
> M       src/widgets/styles/styles.pri

+1, except for json.

> M       src/widgets/widgets/qabstractbutton.cpp
> M       src/widgets/widgets/qcombobox.cpp

-1: OS-based policy decisions.

> M       src/widgets/widgets/qmenu.cpp

+2.

> M       src/widgets/widgets/qtextbrowser.cpp

#ifdef on assets

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/development/attachments/20130205/8318d86e/attachment.sig>