Status

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jbos, Assigned: dougt)

Tracking

Trunk
All
MeeGo
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(fennec2.0b1+)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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.
Blocks: 583135
Posted patch adds qt network manager (obsolete) — Splinter Review
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
Attachment #465755 - Flags: review?(doug.turner)
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.
Attachment #465755 - Attachment is obsolete: true
Attachment #465755 - Flags: review?(doug.turner)
Attachment #466159 - Flags: review?(doug.turner)
tracking-fennec: --- → ?
Adding myself to CC.
tracking-fennec: ? → 2.0b1+
Assignee: nobody → doug.turner
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.
Attachment #466159 - Flags: review?(doug.turner) → review-
(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.
Posted patch Version 3 (obsolete) — Splinter Review
I kept the define since i don't know how to handle it correctly with the version number. fixed the licences. and other points.
Attachment #466159 - Attachment is obsolete: true
Attachment #467959 - Flags: review?(doug.turner)
Attachment #467959 - Flags: review?(romaxa)
Posted patch Fixed Comments, Version 4 (obsolete) — Splinter Review
Fixed face-2-face comments.
Attachment #467959 - Attachment is obsolete: true
Attachment #468390 - Flags: review?(romaxa)
Attachment #467959 - Flags: review?(romaxa)
Attachment #467959 - Flags: review?(doug.turner)
Attachment #468390 - Flags: review?(romaxa) → review?(doug.turner)
(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.
Posted patch Correct Version of the Patch (obsolete) — Splinter Review
I mixed up the files somehow. ...
Attachment #468390 - Attachment is obsolete: true
Attachment #468636 - Flags: review?(doug.turner)
Attachment #468390 - Flags: review?(doug.turner)
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.
Posted patch Fixed Comments Version 5 (obsolete) — Splinter Review
Fixed
Attachment #468636 - Attachment is obsolete: true
Attachment #468734 - Flags: review?(doug.turner)
Attachment #468636 - Flags: review?(doug.turner)
Attachment #468851 - Flags: review?(mark.finkle)
Attachment #468851 - Flags: review?(mark.finkle) → review+
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!!
Attachment #468734 - Flags: review?(doug.turner) → review-
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.
Attachment #468734 - Attachment is obsolete: true
Attachment #470556 - Flags: review?(doug.turner)
Attachment #468851 - Flags: review?(romaxa)
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.
Attachment #468851 - Flags: review?(romaxa)
IIUC we need to define _QTNETWORK variable and use it in order to play with that variable in Makefiles....
Attachment #470556 - Flags: review?(doug.turner) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/fdf35ea63a85
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 593142
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.