586836 - Support Meego Network Managment

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

Meego does also use still Libconic like Maemo 5, but this will change in the future. Add support for the common qt network interface introduced in qt 4.7 in order to get networking done independent to the network library used by the system.

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: adds qt network manager

The patch adds the qt network.

it uses internal (right now) libconic, this will change but we must ensure not to get 2 network managers. Thats why libconic gets turned of in case of qt4.7 even if there is a lib for that

Comment 2

[Jeremias Bosch (:jbos)]

Attachment: Add qt network manager, Version 2

I needed to update the patch a bit due to some strange problems (not appearing connectivity dialogs, Segfaults). This was caused by not using a pointer in the connection method. 

These problems are not reproduceable after using the pointer. It looks like a timeing issue, a internal bug is filed and deeper investigation on the meego connectivity and qt-network side is needed. For now we can just use this solution, it basically makes no difference - just that the deletion of the object controlled by qt.

Comment 3

[Dariusz Filipski]

Adding myself to CC.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 466159 [details] [diff] [review]
Add qt network manager, Version 2

>Index: mozilla/config/autoconf.mk.in
>===================================================================
>--- mozilla.orig/config/autoconf.mk.in
>+++ mozilla/config/autoconf.mk.in
>@@ -642,6 +642,8 @@ MOZ_PLATFORM_MAEMO_CFLAGS	= @MOZ_PLATFOR
> MOZ_PLATFORM_MAEMO_LIBS 	= @MOZ_PLATFORM_MAEMO_LIBS@
> MOZ_MAEMO_LIBLOCATION 	= @MOZ_MAEMO_LIBLOCATION@
> 
>+MOZ_ENABLE_QTNETWORK = @MOZ_ENABLE_QTNETWORK@


I really do not want to add another #define for this.  if it is truely Qt networking, can't we just use the widget define?


>Index: mozilla/netwerk/base/src/nsAutodialQt.cpp
License header needs to be updated.  It also references Maemo but this looks like a new file.


>Index: mozilla/netwerk/base/src/nsAutodialQt.h
>===================================================================

same header problem.


>+
>+#ifndef NSAUTODIALQT
>+#define NSAUTODIALQT

typically nsAutodialQt_h__

>+++ mozilla/netwerk/system/qt/Makefile.in
header

>+LOCAL_INCLUDES += -I$(srcdir)/../../base/src
>\ No newline at end of file

and add a new line to the end of the file.

>Index: mozilla/netwerk/system/qt/nsQtNetworkLinkService.cpp
>===================================================================

Header is wrong. tri license for everything, please


>+static QNetworkConfigurationManager *networkConfig = 0;

prefix statics with s.  e.g.  sNetworkConfig


>+PRBool
>+nsQtNetworkManager::OpenConnectionSync()
>+{
>+    if (!networkConfig)
>+        return PR_FALSE;
>+
>+    //do not request when we are online...
>+    if (networkConfig->isOnline())
>+        return PR_TRUE;

Windows returns false and then returns an error to the higher up callers.  Not sure if it makes that much of a difference.

>+
>+    //Check that there is atleast one XUL window. If there isn't just return
>+    //without establishing connectivity
>+    nsresult rv;
>+    nsCOMPtr <nsIWindowMediator> windowMediator =
>+            do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
>+
>+    NS_ENSURE_SUCCESS(rv,PR_FALSE);
>+
>+    nsCOMPtr <nsISimpleEnumerator> windowEnumerator;
>+
>+    rv = windowMediator->GetXULWindowEnumerator(nsnull,
>+                                                getter_AddRefs(windowEnumerator));
>+
>+    NS_ENSURE_SUCCESS(rv,PR_FALSE);
>+
>+    PRBool moreElements = PR_FALSE;
>+    windowEnumerator->HasMoreElements(&moreElements);
>+    if (!moreElements) {
>+        return PR_FALSE;
>+    }


Why this check?  remove it.  Having a window should not be a requirement of setting up a network connection.

>+        foreach (QNetworkConfiguration cfg, networkConfig->allConfigurations())

not sure if this template works everwhere, but i am glad it does for you.

>+    }
>+    const bool canStartIAP = (networkConfig->capabilities()
>+                           & QNetworkConfigurationManager::CanStartAndStopInterfaces);
>+


why const, and why not just put this the if below?

>+    if (!canStartIAP)
>+    {



>+    QNetworkSession *session = new QNetworkSession(default_cfg);
>+    QObject::connect (session, SIGNAL(opened()),
>+                      session, SLOT(deleteLater()));
>+    QObject::connect (session, SIGNAL(error(QNetworkSession::SessionError)),
>+                      session, SLOT(deleteLater()));
>+    session->open();
>+    return session->waitForOpened(-1);

I am not sure that is exactly what we need.  Oleg should also take a look.

Comment 5

[Jeremias Bosch (:jbos)]

(In reply to comment #4)
> Comment on attachment 466159 [details] [diff] [review]
> Add qt network manager, Version 2
> 
> >Index: mozilla/config/autoconf.mk.in
> >===================================================================
> >--- mozilla.orig/config/autoconf.mk.in
> >+++ mozilla/config/autoconf.mk.in
> >@@ -642,6 +642,8 @@ MOZ_PLATFORM_MAEMO_CFLAGS	= @MOZ_PLATFOR
> > MOZ_PLATFORM_MAEMO_LIBS 	= @MOZ_PLATFORM_MAEMO_LIBS@
> > MOZ_MAEMO_LIBLOCATION 	= @MOZ_MAEMO_LIBLOCATION@
> > 
> >+MOZ_ENABLE_QTNETWORK = @MOZ_ENABLE_QTNETWORK@
> 
> 
> I really do not want to add another #define for this.  if it is truely Qt
> networking, can't we just use the widget define?
> 

Mhm i could make a qt version Lookup if bigger 4.7. I thought that way is just cleaner. But ok i see the point.

> >+PRBool
> >+nsQtNetworkManager::OpenConnectionSync()
> >+{
> >+    if (!networkConfig)
> >+        return PR_FALSE;
> >+
> >+    //do not request when we are online...
> >+    if (networkConfig->isOnline())
> >+        return PR_TRUE;
> 
> Windows returns false and then returns an error to the higher up callers.  Not
> sure if it makes that much of a difference.
>
In case we are online it returns a error? Mhm interessting, that would stop reconnecting basically ok i see the logic behind it, since it stops loops in cases there is network connection and a ip but still no server aviable (route / dns issue or such) 
> >+
> >+    //Check that there is atleast one XUL window. If there isn't just return
> >+    //without establishing connectivity
> >+    nsresult rv;
> >+    nsCOMPtr <nsIWindowMediator> windowMediator =
> >+            do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
> >+
> >+    NS_ENSURE_SUCCESS(rv,PR_FALSE);
> >+
> >+    nsCOMPtr <nsISimpleEnumerator> windowEnumerator;
> >+
> >+    rv = windowMediator->GetXULWindowEnumerator(nsnull,
> >+                                                getter_AddRefs(windowEnumerator));
> >+
> >+    NS_ENSURE_SUCCESS(rv,PR_FALSE);
> >+
> >+    PRBool moreElements = PR_FALSE;
> >+    windowEnumerator->HasMoreElements(&moreElements);
> >+    if (!moreElements) {
> >+        return PR_FALSE;
> >+    }
> 
> 
> Why this check?  remove it.  Having a window should not be a requirement of
> setting up a network connection.
> 
It is important for Meego Prestart / Faststart. Without it we request connection during startup of the device which we are not allowed to do. 
That is also a issue on libconic and this part is a plain copy - the patch for that seems to be nowhere in BMO. I wonder actually why and need to check.
But it is important. I will add a comment.

> >+        foreach (QNetworkConfiguration cfg, networkConfig->allConfigurations())
> 
> not sure if this template works everwhere, but i am glad it does for you.
> 
foreach is something you use in qt all the time. Its crossplatform.


> >+    QNetworkSession *session = new QNetworkSession(default_cfg);
> >+    QObject::connect (session, SIGNAL(opened()),
> >+                      session, SLOT(deleteLater()));
> >+    QObject::connect (session, SIGNAL(error(QNetworkSession::SessionError)),
> >+                      session, SLOT(deleteLater()));
> >+    session->open();
> >+    return session->waitForOpened(-1);
> 
> I am not sure that is exactly what we need.  Oleg should also take a look.

We keept libconic for a long while but ran into a lot of trouble which where not fixable without hacking around.  The proper way to fix those issues is to use qtnetwork, since this is the platform and lib independent way to do network in qt 4.7.
 And a important sidenode, libconic will be gone soon and be switched. Using Qt here keeps it working no matter if someone might decide to use now another api.

Comment 6

[Jeremias Bosch (:jbos)]

Attachment: Version 3

I kept the define since i don't know how to handle it correctly with the version number. fixed the licences. and other points.

Comment 7

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments, Version 4

Fixed face-2-face comments.

Comment 8

[Halton Huo]

(In reply to comment #7)
> Created attachment 468390 [details] [diff] [review]
> Fixed Comments, Version 4
> 
> Fixed face-2-face comments.

the version 4 patch will leads to compiler error as following: 

c++ -o nsQtNetworkManager.o -c   -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DIMPL_NS_NET -I/home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt/../../base/src -I/home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/home/halton/work/projects/mozilla/qtdebug/dist/include/nspr -I/home/halton/work/projects/mozilla/qtdebug/dist/include/nss     -DQT_SHARED -I/usr/local/Trolltech/Qt-4.7.0/include -I/usr/local/Trolltech/Qt-4.7.0/include/QtGui -I/usr/local/Trolltech/Qt-4.7.0/include/QtCore -I/usr/local/Trolltech/Qt-4.7.0/include/QtNetwork -I/usr/local/Trolltech/Qt-4.7.0/include/QtOpenGL      -fPIC  -frtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -pedantic -Wno-long-long -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsQtNetworkManager.pp /home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt/nsQtNetworkManager.cpp
/home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt/nsQtNetworkManager.cpp: In static member function ‘static PRBool nsQtNetworkManager::IsConnected()’:
/home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt/nsQtNetworkManager.cpp:147: error: ‘networkConfig’ was not declared in this scope
/home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt/nsQtNetworkManager.cpp: In static member function ‘static PRBool nsQtNetworkManager::GetLinkStatusKnown()’:
/home/halton/work/projects/mozilla/mozilla-central/netwerk/system/qt/nsQtNetworkManager.cpp:153: error: ‘networkConfig’ was not declared in this scope

s/networkConfig/sNetworkConfig in IsConnected() and GetLinkStatusKnown() will fix this issue.

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: Correct Version of the Patch

I mixed up the files somehow. ...

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 468636 [details] [diff] [review]
Correct Version of the Patch

Small style comments:
>+nsAutodial::DialDefault(const PRUnichar* hostName)

this is correct


>+nsQtNetworkLinkService::Observe(nsISupports *aSubject,
>+                                   const char *aTopic,
>+                                   const PRUnichar *aData)

This looks different, and IIRC wrong according to mozilla style
should be
>+                                   const PRUnichar* aData)

>+
>+static QNetworkConfigurationManager *sNetworkConfig = 0;
should be
>+static QNetworkConfigurationManager* sNetworkConfig = 0;


>+
>+    NS_ENSURE_SUCCESS(rv,PR_FALSE);
space is missing here      ^, and same copy pasted in other places


>+    if (!default_cfg.isValid())
>+    {

should be
>+    if (!default_cfg.isValid()) {


>+    //do use pointer here, it will be deleted after connected!
>+    //Creation on stack cause appearing issues and segfaults of the connectivity dialog
>+    QNetworkSession *session = new QNetworkSession(default_cfg);
                      ^
>+    QObject::connect (session, SIGNAL(opened()),
                      ^

>+void
>+nsQtNetworkManager::CloseConnection()
>+{
>+    //not impl
I  guess  we should have some WARNING here at least, if implementation expected here...
If implementation not expected to be here, then why do we need this function at all and who is calling this function?


>+    sNetworkConfig = new QNetworkConfigurationManager(0);
Just empty args is enough, because
Qt/qnetworkconfigmanager.h:84
   QNetworkConfigurationManager( QObject* parent = 0 ); -< Qt will initialize by default


>+    delete sNetworkConfig;
>+    sNetworkConfig = 0;
same here

>+}
>\ No newline at end of file
probably new line would be nice 




Patch is seems to be ok, but style a bit broken, please fix it.

Comment 11

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments Version 5

Fixed

Comment 12

[Jeremias Bosch (:jbos)]

Attachment: Enable Network on Maemo in General

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 468734 [details] [diff] [review]
Fixed Comments Version 5


>+MOZ_ENABLE_QTNETWORK = @MOZ_ENABLE_QTNETWORK@
>+

still don't like this.

Maybe it would be better to simply add this library to the Qt libraries set under the section "QT support".  We could also disable libconic there too?


>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@

nit:  align equal chars.  see tabs?

>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+MODULE	       = necko
>+LIBRARY_NAME   = neckosystem_s


same

>+LOCAL_INCLUDES += -I$(srcdir)/../../base/src
>\ No newline at end of file

add new line.
>+NS_IMETHODIMP
>+nsQtNetworkLinkService::Observe(nsISupports* aSubject,
>+                                   const char* aTopic,
>+                                   const PRUnichar* aData)

nit: align.

>+{
>+  if (!strcmp(aTopic, "xpcom-shutdown"))
>+    Shutdown();
>+
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsQtNetworkLinkService::Init(void)
>+{
>+  nsCOMPtr<nsIObserverService> observerService =
>+    mozilla::services::GetObserverService();
>+  if (!observerService)
>+    return NS_ERROR_FAILURE;
>+
>+  nsresult rv = observerService->AddObserver(this, "xpcom-shutdown", PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!nsQtNetworkManager::Startup())
>+    return NS_ERROR_FAILURE;

use NS_ENSURE_SUCCESS, or return NS_ERROR_FAILURE.  Just not both.

>+
>+    //Check that there is atleast one XUL window. If there isn't just return
>+    //without establishing connectivity

again.  I do not care to have this code in necko.  We should chase down what is opening a network connection before a window is created -- that is the bug, and this is a bandaide.

>+    NS_WARNING ("nsQtNetworkManager::CloseConnection() Not implemented by QtNetwork.");

Nit: No space between NS_WARNING and the first (

>+}
>+
>+PRBool
>+nsQtNetworkManager::IsConnected()
>+{
>+    NS_ASSERTION (sNetworkConfig, "Network not initialized");


Nit: No space between NS_ASSERTION and the first (


>+nsQtNetworkManager::Shutdown()
>+{
>+    delete sNetworkConfig;
>+    sNetworkConfig = 0;

lets use nsnull instead of zero.

Getting really really close!!

Comment 14

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments Version 6

Its still a define in it, i dont know how to handle it different. If you do, please tell me. The rest should be adressed and fixed.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 470556 [details] [diff] [review]
Fixed Comments Version 6

Can't you just use QT_VERSION and add the QTNETWORK libraries to the LIBS where we handle Qt stuff in configure in.

Comment 16

[Oleg Romashin (:romaxa)]

IIUC we need to define _QTNETWORK variable and use it in order to play with that variable in Makefiles....

Comment 17

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/fdf35ea63a85

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 468851 [details] [diff] [review]
Enable Network on Maemo in General

pushed:
http://hg.mozilla.org/mobile-browser/rev/2d5c5f90f18b