[Development] Reviews needed before android integration in two weeks

[Eskil Abrahamsen Blomfeldt]

Thanks for the comments. I'll convert them all into tasks or add them as 
comments in Gerrit if the patches in question are already in there.


On 02/06/2013 03:04 AM, Thiago Macieira wrote:

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


Yes, that's a left-over. I'll remove it.

>> A       lib/rules.xml
> I'd rather you found a better place for this file.

I was planning on putting it into src/android and having it deployed 
into lib/rules.xml when you build for Android.

Would that be acceptable? I'd like to deploy it in lib/ because then the 
Qt Creator plugin doesn't need to look in two different places for the 
file for Qt 4 and Qt 5 builds.


> .
>> A       mkspecs/features/java.prf
> Ossi to judge if the mkpath should be there.

Ossi wrote the code ;)

>
> 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.

I disagree. The fewer hardcoded things we have in qmake, the better in 
my opinion. I do agree that the LINK override is hacky, but at least 
it's a working solution with the current system and the cleanest we 
could think of. If qmake is changed later to add a less hacky way to 
override the different compile steps, I'll be glad to adapt to it, but 
for now, I'd like to keep this build system and focus on more pressing 
things before the feature freeze :)

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

I don't know. I haven't seen any issues. But I don't know if 
qpa/android.prf is actually used for anything.

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

Ok, sounds good to me.

>
>> M       src/corelib/kernel/qeventdispatcher_unix.cpp
>> M       src/corelib/kernel/qeventdispatcher_unix_p.h
> This one needs a very good explanation.

Would you mind discussing this with Bogdan on

     https://codereview.qt-project.org/#change,46798

Apparently it was needed to fix a deadlock in the event loop on Android. 
It was suggested that we move the hacky implementation into the android 
plugin instead. Would that be an acceptable solution, at least for a 
first go?

>
> 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.

I've already reverted the changes in SSL.pri. I'll remove the enablers 
from configure as well.

> .
>
>> 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.

Yes, I'll remove these and rewrite to use Qt's json parser.

-- Eskil