User-Agent: Opera/9.64 (Macintosh; Intel Mac OS X; U; de) Presto/2.1.1 Build Identifier: The vast majority of maemo applications try to initiate the device's internet connection if they need it and it's currently disable. Fennec doesn't do that yet but it should. Reproducible: Always Steps to Reproduce: 1. Disconnect any internet connection on your Maemo device 2. Start Fennec 3a. Open the extension manager 3b. Try to access a website Actual Results: 3a => "Fennec couldn't retrieve add-ons" 3b => "Page load error" Expected Results: Fennec should initiate the device's internet connection

Code from maemo microb that does this? https://garage.maemo.org/svn/browser/mozilla/trunk/microb-eal/src/gmozillaconnectivity.c

This patch makes use of the build-in autodial function of Fennec! It handles the mode quite different than the windows versions do, since it switch fennec in its offline mode when the user abort the connection process. The Connection Library of Maemo Diablo does work asynchronously, that made it necessary to add an Maemo Network Manager which synchronize this process. You need to have the Library: libconic installed to get this compiling. Libconic is installed by default in Maemo Diablo.

Patch works for me, but I am not able to set review+ here and have it stick:-)

patch looks great ! dougt could be your reviewer i guess. a few nit comments: +ifdef MOZ_PLATFORM_HILDON + LIBS += -lconic + OS_INCLUDES += $(GLIB_CFLAGS) + LOCAL_INCLUDES += -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/conic you could probably add an entry in configure.in and have LIBCONIC_CFLAGS | _LIBS here. i am wondering why is this required: ifdef MOZ_ENABLE_DBUS +ifndef MOZ_PLATFORM_HILDON tier_toolkit_dirs += toolkit/system/dbus +endif there could be possible followup bugs: * listen to connection failures and pause downloads when connection failures.

Comment on attachment 387156 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo >diff --git a/netwerk/base/src/nsIOService.cpp b/netwerk/base/src/nsIOService.cpp >+#if defined( MOZ_PLATFORM_HILDON ) >+ if (mSocketTransportService) >+ mSocketTransportService->SetAutodialEnabled(true); >+#endif PR_TRUE instead of true ? However, maybe we should be using the preference to set this value. PrefsChanged is called in Init too and reads "network.autodial-helper.enabled" from preferences. We should just add that to mobile.js in the Fennec code and not force it here. >diff --git a/netwerk/base/src/nsMaemoNetworkManager.cpp b/netwerk/base/src/nsMaemoNetworkManager.cpp >+ void notifyNetworkLinkObservers() Use leading capital for method names A few things with the entire file: - No need for space padding inside parens - The convention is to use an "a" prefix for method arguments: "event" -> "aEvent" >diff --git a/netwerk/base/src/nsMaemoNetworkManager.h b/netwerk/base/src/nsMaemoNetworkManager.h >+class nsMaemoNetworkManager >+{ >+public: >+ static nsMaemoNetworkManager* GetInstance(); >+ ~nsMaemoNetworkManager(); >+ >+ PRBool openConnectionSync(); >+ void closeConnection(); >+ >+ PRBool isConnected(); >+ PRBool isConnecting(); >+ >+private: >+ nsMaemoNetworkManager(); >+ nsMaemoNetworkManagerPrivate* const d; >+}; Use Leading Caps for method names: OpenConnectionSync(), CloseConnection(), IsConnected(), IsConnecting() >diff --git a/netwerk/system/maemo/Makefile.in b/netwerk/system/maemo/Makefile.in >+REQUIRES = xpcom \ >+ string \ >+ pref \ >+ $(NULL) I don't think you need to use REQUIRES anymore >+ifdef MOZ_PLATFORM_HILDON >+ CPPSRCS += nsMaemoNetworkLinkService.cpp >+endif Is this #ifdef needed here? This file should only be built if MOZ_PLATFORM_HILDON, right? >+ifdef MOZ_PLATFORM_HILDON >+LIBS += -lconic >+OS_INCLUDES += $(GLIB_CFLAGS) >+LOCAL_INCLUDES += -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/conic -I$(srcdir)/../../base/src >+endif Same here >diff --git a/netwerk/system/maemo/nsMaemoNetworkLinkService.cpp b/netwerk/system/maemo >+NS_IMETHODIMP >+nsMaemoNetworkLinkService::Observe(nsISupports *subject, >+ const char *topic, >+ const PRUnichar *data) >+{ >+ if (!strcmp( topic, "xpcom-shutdown" )) >+ { >+ Shutdown(); >+ } Use aSubject, aTopic and aData as arguments >+ nsCOMPtr<nsIObserverService> observerService = >+ do_GetService( "@mozilla.org/observer-service;1", &rv ); >+ NS_ENSURE_SUCCESS( rv, rv ); >+ >+ rv = observerService->AddObserver( this, "xpcom-shutdown", PR_FALSE ); >+ NS_ENSURE_SUCCESS( rv, rv ); No space padding needed in the parens >diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in > ifdef MOZ_PLATFORM_HILDON >-EXTRA_DSO_LDOPTS += $(LIBHILDONMIME_LIBS) >+EXTRA_DSO_LDOPTS += $(LIBHILDONMIME_LIBS) -lconic > endif Might be nice to get conic in a LIBCONIC_LIBS >diff --git a/toolkit/toolkit-tiers.mk b/toolkit/toolkit-tiers.mk > ifdef MOZ_ENABLE_DBUS >+ifndef MOZ_PLATFORM_HILDON > tier_toolkit_dirs += toolkit/system/dbus >+endif > endif Why is this needed? ----- Overall this looks very good. We should get Doug Turner and Ted Mielczarek (for the build & makefile parts) to review as well.

Thanks for the Reviews, I will change the things you mentioned and add the patch later. To your question: >>diff --git a/toolkit/toolkit-tiers.mk b/toolkit/toolkit-tiers.mk >> ifdef MOZ_ENABLE_DBUS >>+ifndef MOZ_PLATFORM_HILDON >> tier_toolkit_dirs += toolkit/system/dbus >>+endif >> endif > Why is this needed? The Problem is that in linux the network status is managed here (in /toolkit/dbus/nsNetworkManagerListener) and not in network/system as for every other platform. From the file /toolkit/dbus/nsDBusService.h > [..] > * Currently the only daemon we communicate with is NetworkManager. We listen > * for NetworkManager state changes; we set nsIOService's offline status to > * FALSE when NetworkManager reports NM_STATE_CONNECTED, and to TRUE otherwise. > * We also solicit the current status from NetworkManager when this component > * gets loaded. > [..] Since the patch adds a NetworkManager for Linux/Maemo/HILDON-Environment, its wrong to have a second network manager here which interfere also with nsIOService and causing uncontrolled network states (i.e. some errors: setting fennec uncontrolled by the MaemoNetworkLinkService to offline-mode, Fennec does not make use of autodial, Fennec does not make use of MaemoNetworkLinkService at all,...). I hope its clear now. ---

I fixed the issues you mentioned in your last comment. The second patch is for mobile.js to add the autodial preference!

As mentioned in comment #5 i added the autodial preference to mobile.js and not force it in nsIOService.cpp anymore.

Comment on attachment 387449 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo >+++ b/netwerk/base/src/nsMaemoNetworkManager.cpp Still some "( " and " )" in here >+++ b/netwerk/system/maemo/Makefile.in >\ No newline at end of file Very good. I'll set for DougT to review.

Thanks for the + Mark. Changed the things from comment #9.

Comment on attachment 387455 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo Ted - could you look at the build parts?

Comment on attachment 387455 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo nsAutodailMaemo -> nsAutodialMaemo Can we use the tri license on these files, please? Space between the #includes and the start of the implementation: +#include "nsThreadUtils.h" +nsAutodial::nsAutodial() style nit. i like 2 spaces. I think that is most common in /netwerk/ no need to have comments that state the obvious: + // ctor + // dtor Lets use a more descriptive member name: + nsMaemoNetworkManagerPrivate* const d; +nsMaemoNetworkManager::nsMaemoNetworkManager(): d(new nsMaemoNetworkManagerPrivate()) Can we move much/all of this initialization functionality into an init() method so that we can handle error cases? Also, lets test for |d| before using it. lets fix the above, and I can take another look.

Comment on attachment 387455 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo diff --git a/netwerk/base/src/nsAutodailMaemo.cpp b/netwerk/base/src/nsAutodailMaemo.cpp I think it's spelled "dial". :) +ifdef MOZ_PLATFORM_HILDON + LIBS += $(LIBCONIC_LIBS) + OS_INCLUDES += $(GLIB_CFLAGS) + LOCAL_INCLUDES += $(LIBCONIC_CFLAGS) Don't use actual tabs here, just two spaces is fine. r=me on the build parts with those nits addressed.

I changed the things you come up with. Just left this things be: + // ctor + // dtor Because its the same in nsAutodialWin/CE its consistent that way. I do not think that this is necessary: "lets test for |d| before using it." because |d| is nsMaemoNetworkManagerPrivate* const d

ping

the file name is still wrong. why does "nsMaemoNetworkManagerPrivate* const pimpl" get you around a failed malloc/new? if you OOM, you're going to crash, right? Also, change "pimpl" to something else like "mImpl". Frankly, i am not sure why you can't just roll this class into nsMaemoNetworkManager.

PImpl is a well known technique, see: http://en.wikipedia.org/wiki/Pimpl In Qt this pointer is always called d (or d_ptr), but you did not like that, so we moved on to another well known name:-) Why use that here? It improves binary compatibility (we have no IDL for that in this case) and hides all the private implementation details from the view of a developer using the interface defined by nsMaemoNetworkManager. The pointer is const (not the data pointed to), so only one test is required when the pointer is initialized. It can never change afterwards. This is why testing for != 0 does not make sense when *using* the d/PImpl pointer. There should be no need to move this initialization into a init method. New should throw an exception on OOM, so the issue is noticed. Doing the initialization outside the initializer list further stops us from using a const pointer to the non-const data, introducing all kinds of issues with changing the pointer accidentally.

I am familiar with the technique, mostly was just unhappy about the variable name. the var name is not used in mozilla (for better or worse): http://mxr.mozilla.org/mozilla-central/search?string=pimpl mImpl would be a better choice, imo. +#include "nsAutodailMaemo.h" update the file name I will try the patch out tomorrow, and review the dbus bits and pieces. Looks good so far!

Sorry, i missed somehow the header file... don't know how that happened, but anyway Tobias mentioned everything already, i removed the init Function since it does not make any sense.

Comment on attachment 388430 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo got this when compiling your patch on a clean scratchbox. c++ -o nsMaemoNetworkManager.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DIMPL_NS_NET -I/usr/include/conic -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/home/dougt/builds/mobile/mozilla-central/netwerk/base/src -I. -I../../../dist/include -I../../../dist/include/nsprpub -I/home/dougt/builds/mobile/mobilebase/xulrunner/dist/include/nspr -I/home/dougt/builds/mobile/mobilebase/xulrunner/dist/include/nss -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions -finline-limit=50 -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsMaemoNetworkManager.pp /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp In file included from ../../../dist/include/nsCOMArray.h:42, from ../../../dist/include/nsCategoryCache.h:48, from /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsIOService.h:58, from /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp:38: ../../../dist/include/nsVoidArray.h: In member function 'void nsAutoVoidArray::ResetToAutoBuffer()': ../../../dist/include/nsVoidArray.h:193: warning: cast from 'char*' to 'nsVoidArray::Impl*' increases required alignment of target type /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp: In member function 'void nsMaemoNetworkManagerPrivate::ConnectionEventCallback(ConIcConnection*, ConIcConnectionEvent*)': /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp:84: error: 'CON_IC_STATUS_NETWORK_UP' was not declared in this scope /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp: At global scope: /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp:48: warning: 'void _g_statistics_event(ConIcConnection*, ConIcStatisticsEvent*, void*)' declared 'static' but never defined make[7]: *** [nsMaemoNetworkManager.o] Error 1 make[7]: Leaving directory `/home/dougt/builds/mobile/mobilebase/xulrunner/netwerk/base/src' make[6]: *** [libs] Error 2

I did a | fakeroot apt-get install libconic0-dev | into my scrtahcbox, just to be sure I had the conic files. I still get the same error as Doug: /nsMaemoNetworkManager.cpp:84: error: 'CON_IC_STATUS_NETWORK_UP' was not declared in this scope I am building using cs2007q3 in a CHINOOK-ARMEL sbox.

Im not able to build plain Mozilla Central at the moment, so i Need to sort this out Today (some sqlite3 Error). Than i can see whats going on with the Patch. Using Chinook for à diablo based Patch Could Be the issue, but i will See .

The Patch is definitely compiling here on an Diablo-Arm Target. It have to be your old CHINOOK-ARML sbox. I cant test it with the newest mozilla central see #504761

Update. With the Patch from Bug 503570 or the most recent mozilla central it works again, i can verify that the patch is compiling and working as it supposed to be.

(In reply to comment #25) > Update. > > With the Patch from Bug 503570 or the most recent mozilla central it works > again, i can verify that the patch is compiling and working as it supposed to > be. jeremias, if you have not tried it yet, please try setting up a build rootstrap based on the following steps: https://wiki.mozilla.org/Mobile/Build/cs2007q3 i believe most of the mozilla guys are still stuck on using 4.0.1 SDK (chinook) , and not 4.1.x (not diablo SDK) ... and then those libconic problems can be reproducible for you too (?)

> jeremias, if you have not tried it yet, please try setting up a build rootstrap > based on the following steps: > https://wiki.mozilla.org/Mobile/Build/cs2007q3 > > i believe most of the mozilla guys are still stuck on using 4.0.1 SDK (chinook) > , and not 4.1.x (not diablo SDK) ... and then those libconic problems can be > reproducible for you too (?) This patch is for Diablo, not for a previous SDK Version. So it does not work with Chinook. Unfortunately there is no way to distinguish Chinook from Diablo in fennec/mozilla so what we do about it. Support for Chinook won't be made by this patch, the solution would be to not compile it in a Chinook Version of fennec - when there is a way to distinguish it... I think they should really update then this Description and there environment.

vote for not blocking1.0

why can we not just dlopen() some functions here and get this landed?

Hello, For the case that you really want to get that landed in 1.0. I recently just got some old documentation in hand, regarding to that documentation the only change in libconic which should bothering the patch is the NETWORK_UP state and that is not really used by the patch-implementation anyway yet. So I removed the state from the patch and it should now work now also in CHINOOK. When you get these patch landed you should also look into Bug 495674, its just a very small extension of this patch and of course it works fine. -JB

Sorry i meant Bug 458211 -JB

I successfully built Fennec with the latest patch using the Chinook SDK. However, when attempting to test using the steps in comment 0, I saw some errors in the console: fennec[1423]: GLIB CRITICAL ** ConIc - con_ic_connection_disconnect_by_id: assertion 'connection != NULL && id != NULL' failed fennec[1423]: GLIB DEBUG ConIc - con_ic_connection_send_event(0x40263b20, (null), (null), 1) fennec[1423]: GLIB DEBUG ConIc - con_ic_connection_send_event(0x40263b20, (null), (null), 1) The internet connection was not started.

Comment on attachment 398593 [details] [diff] [review] Patch to add use of connectivity to fennec for maemo lots of problems. r-

mobile.js needs to add both: pref("network.autodial-helper.enabled", true); pref("network.manage-offline-status", true);

Comment on attachment 406770 [details] [diff] [review] conn patch for maemo n900 (v3) the call to CloseConnection() from Shutdown must be removed. We shouldn't attempt to close the internet connection simply because mozilla exits. The OS will cleanup our connection per the docs.

Comment on attachment 406770 [details] [diff] [review] conn patch for maemo n900 (v3) +++ b/configure.in Thu Oct 15 11:41:41 2009 -0700 + PKG_CHECK_MODULES(LIBCONIC,conic) add a space after the comma +++ b/netwerk/base/src/Makefile.in Thu Oct 15 11:41:41 2009 -0700 + OS_INCLUDES += $(GLIB_CFLAGS) + LOCAL_INCLUDES += $(LIBCONIC_CFLAGS) why not put both CFLAGS into OS_INCLUDES? +++ b/netwerk/build/nsNetModule.cpp Thu Oct 15 11:41:41 2009 -0700 +#include "nsMaemoNetworkLinkService.h" +NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsMaemoNetworkLinkService, Init) The define is called PLATFORM_HILDON... shouldn't the classes/directories also be called Hildon instead of Maemo then? +++ b/toolkit/toolkit-tiers.mk Thu Oct 15 11:41:41 2009 -0700 ifdef MOZ_ENABLE_DBUS +ifndef MOZ_PLATFORM_HILDON tier_toolkit_dirs += toolkit/system/dbus why? +++ b/netwerk/base/src/nsAutodialMaemo.cpp Thu Oct 15 15:43:45 2009 -0700 same Maemo/Hildon comment +++ b/netwerk/base/src/nsAutodialMaemo.h Thu Oct 15 11:56:06 2009 -0700 +#include "nsCOMPtr.h" no need for that +#include "nscore.h" I don't think you need this either +public: + + nsAutodial(); + virtual ~nsAutodial(); remove the empty line. also no need to make the constructor virtual. +++ b/netwerk/base/src/nsMaemoNetworkManager.cpp Fri Oct 16 13:43:19 2009 -0700 Same Maemo/Hildon comment, also - shouldn't this file be in netwerk/system? + * Jeremias Bosch <jeremias.bosch@gmail.com> You're using a rather inconsistent indentation for his name in the various files :) +static void connection_event_callback(ConIcConnection *aConnection, + ConIcConnectionEvent *aEvent, + gpointer aUser_data); incorrect indentation on the last two lines But it doesn't look like you need the forward declaration anyway + nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1"); line too long, please break at 80 characters instead of forcing the io service to go offline, shouldn't you broadcast a NS_NETWORK_LINK_TOPIC notification? + if (!gConnectionMutex) + return; this can't happen, right? why are you using a glib mutex instead of mozilla::Mutex? The latter also provides a MutexAutoLock class which ensures that you can't forget an unlock call. It would be kinda nice if you could document which thread the various functions are called on. If I got this right: connection_event_callback - main thread only (via the glib event loop) nsMaemoNetworkManager::OpenConnectionSync - socket transport thread - nsMaemoNetworkManager::CloseConnection - main thread +nsMaemoNetworkManager::IsConnected() - socket transport thread +nsMaemoNetworkManager::IsConnecting() - not called +nsMaemoNetworkManager::Startup() - main thread +nsMaemoNetworkManager::Shutdown() - main thread is that correct? +nsMaemoNetworkManager::OpenConnectionSync() + if (!con_ic_connection_connect(gConnection, CON_IC_CONNECT_FLAG_NONE)) + g_error("openConnectionSync: Error while connecting. %p \n", (void*) PR_GetCurrentThread()); line too long. shouldn't you also return false? + g_cond_wait(gConnectionCond, gConnectionMutex); please follow the API docs: It is important to use the g_cond_wait() and g_cond_timed_wait() functions only inside a loop, which checks for the condition to be true as it is not guaranteed that the waiting thread will find it fulfilled, even if the signaling thread left the condition in that state. This is because another thread can have altered the condition, before the waiting thread got the chance to be woken up, even if the condition itself is protected by a GMutex, like above. +nsMaemoNetworkManager::CloseConnection() why don't you have to lock the mutex here? + g_type_init(); shouldn't you also call g_thread_init()? + g_signal_connect(G_OBJECT(gConnection), + "connection-event", + G_CALLBACK(connection_event_callback), + nsnull); incorrect indentation +nsMaemoNetworkManager::Shutdown() +{ + if (!gConnection) + return; Is this possible? +++ b/netwerk/system/maemo/nsMaemoNetworkLinkService.cpp Thu Oct 15 15:44:44 2009 -0700 +nsMaemoNetworkLinkService::GetLinkStatusKnown(PRBool *aIsKnown) isn't this a lie? if internalstate == InternalState_Invalid, you don't know the state. + if (!strcmp( aTopic, "xpcom-shutdown" )) no spaces after or before parentheses

thanks for the review. I am fixing the problems you found. What follows are answers and/or questions to some of your concerns: 1) regarding Maemo verses Hildon. Ideally there would have been a MAEMO #define since libconic is part of that. However, mozilla has blurred the line between what is hildon and what is maemo. for us, it is basically the same thing. 2) nscore.h is needed in the header because we use nsresult: /home/dougt/builds/mobile/mozilla-1.9.2/netwerk/base/src/nsAutodialMaemo.h:51: error: 'nsresult' does not name a type ... 3) shouldn't you also call g_thread_init()? no idea; not sure I needed to. everything seems to work without doing this. Since I am going to be converting over to Mozilla:mutex's, maybe this is less of a concern? 4) +nsMaemoNetworkManager::Shutdown() +{ +  if (!gConnection) +    return; Is this possible? sure -- if we fail to allocate a mutex, condvar, or if con_ic_connection_new fails.

I've copied dougt's v4 patch and changed the build configuration so that libconic detection and usage is independent of MOZ_PLATFORM_HILDON. libconic is autodetected and can be disabled by --disable-libconic.

(In reply to comment #40) > 3) shouldn't you also call g_thread_init()? > > no idea; not sure I needed to. everything seems to work without doing this. > Since I am going to be converting over to Mozilla:mutex's, maybe this is less > of a concern? The documentation for g_mutex_lock says that it does nothing if g_thread_init hasn't been called; so if you were still using GMutex, it would be better to make sure that threads are inited :)

the latest patches do not use g_mutex_lock. instead, i am using mozilla::monitor as you suggested.

Comment on attachment 407484 [details] [diff] [review] conn patch for maemo n900 (alternative build config) What was your opinion on my question whether nsMaemoNetworkManager.cpp should be in netwerk/system instead? +++ b/netwerk/base/src/nsMaemoNetworkManager.cpp + observerService->NotifyObservers(nsnull /* link service. no one uses it. */, So, that's really not a good reason to pass null here. But the documentation (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsINetworkLinkService.idl#69) doesn't specify what the subject of the notification is, so I'll let this pass... just keep in mind that there's a chance that this will introduce an incompatibility that might break extensions. + if (!con_ic_connection_connect(gConnection, + CON_IC_CONNECT_FLAG_NONE)) incorrect indentation on the second line + g_error("openConnectionSync: Error while connecting. %p \n", + (void*) PR_GetCurrentThread()); same for the second line here repeating my question from above: +nsMaemoNetworkManager::CloseConnection() why don't you have to lock the monitor here? + gMonitor = new Monitor("MaemoAutodialer"); + NS_ASSERTION(gMonitor, "Out of memory!"); personally I'd make this function return a bool indicating whether it succeeded +++ b/netwerk/base/src/nsNativeConnectionHelper.cpp + // On mobile platforms, instead of relying on the link service, we + // should ask the dialer directly. This allows the dialer to update + // link status more forcefully rather than passively watching link + // status changes. so.. the link status change gets broadcasted slowly on mobile? or why is this needed? +#if ! defined(MOZ_ENABLE_LIBCONIC) && ! defined(WINCE_WINDOWS_MOBILE) no spaces after ! sorry, didn't notice this before, but there's no point in specifying LIBS in netwerk/base/src/Makefile.in or netwerk/system/maemo/Makefile.in. As those directories only build static libraries, they never use $(LIBS). netwerk/build/Makefile.in probably needs it, though, at least if you ever want to build non-libxul builds (like, debug builds)

Comment on attachment 407329 [details] [diff] [review] conn patch for maemo n900 (v4) (I prefer the other patch)

this patch mostly works. I say mostly because sometimes I see that pressing on a link, does bring up the connection dialog, creates a new connection, and sets tryAgain to true in the nsSocketTransport2, BUT it doesn't complete the request. The browser just spins. christian, any ideas?

Comment on attachment 407654 [details] [diff] [review] conn patch for maemo n900 (v5 - trunk) dougt, this patch is not in the right place I *think*

Comment on attachment 407654 [details] [diff] [review] conn patch for maemo n900 (v5 - trunk) grrr.

Comment on attachment 407768 [details] [diff] [review] conn patch for maemo n900 (v5 - trunk) +++ b/netwerk/base/src/Makefile.in Wed Oct 21 11:34:33 2009 -0700 +ifdef MOZ_ENABLE_LIBCONIC + OS_INCLUDES += $(GLIB_CFLAGS) $(LIBCONIC_CFLAGS) +endif don't think you need this here anymore I'm confused by the diff: diff -r df08955601c5 netwerk/system/maemo/nsMaemoNetworkManager.cpp --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/netwerk/base/src/nsMaemoNetworkManager.cpp Wed Oct 21 13:17:10 2009 -0700 I assume it's really in system/maemo and just displayed weirdly here. + // as a service to our customers, we also manage offline support here. + // this is due to the fact that managed offline support and autodialers + // do not work well together. + nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1"); + if (!ioService) + return; + + // ioService->SetOffline(gInternalState != InternalState_Connected); I'm still skeptical about why this is needed, but one way or another you shouldn't have that line commented out. why does http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#853 not do the right thing? + //NotifyNetworkLinkObservers(); why did you comment out this line?

I updated the fennec patch like Doug mentioned it in #35.

[QUOTE] diff -r df08955601c5 netwerk/system/maemo/nsMaemoNetworkManager.cpp --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/netwerk/base/src/nsMaemoNetworkManager.cpp Wed Oct 21 13:17:10 2009 -0700 [/QUOTE] needs to be: diff -r df08955601c5 netwerk/system/maemo/nsMaemoNetworkManager.cpp --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/netwerk/system/maemo/nsMaemoNetworkManager.cpp Wed Oct 21 13:17:10 2009 -0700 otherwise it wont compile. ---- [QUOTE] + // as a service to our customers, we also manage offline support here. + // this is due to the fact that managed offline support and autodialers + // do not work well together. + nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1"); + if (!ioService) + return; + + // ioService->SetOffline(gInternalState != InternalState_Connected); I'm still skeptical about why this is needed, but one way or another you shouldn't have that line commented out. [/QUOTE] The point here is to set the browser into the offline mode every time when the connection gets lost. There are possibilities where no new connection can be established. [QUOTE] why does http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#853 not do the right thing? [/QUOTE] Because 'disconnecting' does not cause a NS_NETWORK_LINK_TOPIC. One example here: The user does not have a connection established when he starts fennec. In this case Fennec will request a connection through libconic. In some cases now (i.e. no known network available) the user will see a dialog. But in this dialog he have the possibility to cancel it which means there wasn't a connection established and there wont be a NS_NETWORK_LINK_TOPIC. The only information we will get is a disconnected event from libconic, which change our internal state to DISCONNECTED. So the call is now. "When we are not connected, we are offline." I hope that clarify that thingy a little.

The patch should not have a review+. When you move nsMaemoNetworkManager.cpp to system/maemo you have also to fix the includes in nsAutodialMaemo.cpp to ../system/maemo/nsMaemoNetworkManager.cpp I didn't want to do that (../system/maemo), thats why i placed nsMaemoNetworkManager.* in base.

Ok actually what I said in #55 was not completely true regarding to the newest patch. I missed the part that you actually use NS_NETWORK_LINK_*, but just removed the call of the complete function. So as Christian said, i also think we do not need this anymore this should be done automatically: + // as a service to our customers, we also manage offline support here. + // this is due to the fact that managed offline support and autodialers + // do not work well together. + nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1"); + if (!ioService) + return; + + // ioService->SetOffline(gInternalState != InternalState_Connected);

(changes) - Enables NotifyNetworkLinkObservers again. - Fix includes - Change path of nsMaemoNetworkManager to netwerk/system/maemo

@jeremias - I applied your patch and i can not establish a connection. I think this is because calling NotifyNetworkLinkObservers() will continue to put us into an offline state.

Comment on attachment 407991 [details] [diff] [review] conn patch for maemo n900 (v6 - trunk) you shouldn't include files using relative paths, you should add the file to an EXPORTS= directive in the makefile instead.

christian, please ignore the patches from jeremias; they are not working for me. I will put something up that works and doesn't have that mistake of using relative patch #includes. Also, we don't want to export that file, we can use LOCAL_INCLUDES like other makefiles do. (no one outside of necko needs this autodialer file)

@Doug, ok I'm back on Diablo where the patch was original created for. (It should definitely be compatible.) Well, I need to check what has changed in detail but unfortunately it seems totally broken function-wise. It never switch into the offline state, sometimes it keeps loading for ever, it mostly (80%) does not request a new connection when the connection was lost (esp. when there was no connection at startup) or fennec keeps asking for a new connection and the user can't cancel it.

This patch is based on a pre-version doug send to me. I fixed it finally up and tested it successfully on a Diablo Device. The cause for the most problems i saw before was, that in linux is in /toolkit/system/dbus a networkmanager. This one "overwrite" our libconic networkmanger in compreg.dat since maemo is a linux. For more information check Bug 312793. And before you may ask, we do not want to use plain dbus messages here, since libconic might changed the dbus messages internally in different versions within the past and upcoming future. And off course they provide us with a nice and stable interface. Doug and I come also to the point that setting the browser to offline as soon as the device loose its connection and setting it back online as soon as the connection returns is the most direct and best way to handle the offline mode. You also want to use the patches within the Bugs 524937 and 523296, they will provide you the ability to return back to online mode within the offline mode "error page", by clicking try again.

OK, so why should Maemo be the only platform that doesn't use NS_NETWORK_LINK_STATUS_TOPIC for the online/offline mode?

NS_NETWORK_LINK_STATUS_TOPIC stuff is just for managing offline status. The patch explictly sets it offline status and we don't set the pref that tells necko to monitor these notifications.

the last patch continues to not work on the n900. further investigation is required.

(In reply to comment #65) > NS_NETWORK_LINK_STATUS_TOPIC stuff is just for managing offline status. The > patch explictly sets it offline status and we don't set the pref that tells > necko to monitor these notifications. Well yeah, that was obvious from the patch. But why do it that way?

(In reply to comment #66) > the last patch continues to not work on the n900. further investigation is > required. It also doesn't seem to work on the n810

Hmm, at least for the n810 i'm sure it does work! Did you apply both patches? The One for xulrunner and the One for fennec? Do you Build them in à chinook or à diablo Environment? You might also need to reset compreg.dat in your profile.

no, I missed the fennec patch. Thanks for pointing that out.

@brad ok, so it works now, you should consider to take the offline page patches to get the best results. @christian The issues we saw before: - keeps loading and nerver open a page - no offline mode page displayed - continues asking for a connection are caused by the NS_NETWORK_LINK_STATUS_TOPIC stuff. In the code of WinCE you also wont find the NS_NETWORK_LINK_STATUS_TOPIC stuff, they actually not useing the offline-mode at all.

I tried the patch on a N900 and actually it does work fine! The only thing you will face is a grey page because of this Bug 520000 . The workaround is just to press reload in fennec.

Any chance to get this landed? Anything pending apart from 520000?

Would it be possible also re-read Maemo proxy settings from GConf when connection is changed?

(In reply to comment #71) > The issues we saw before: > - keeps loading and nerver open a page > - no offline mode page displayed > - continues asking for a connection > are caused by the NS_NETWORK_LINK_STATUS_TOPIC stuff. So how does this notification cause those issues? Basically it seems to me that using the topic is the right way to go about this, it's what all the desktop platforms use and it's not at all clear to me why mobile should be different.

Ok, a short update: This patch doesn't have a nice user experience because it relies on other things to be changed (for example bug 520000 which makes the patch looking like it wouldn't work and bug 523296 which would be needed to be able to leave offline mode at all again from the browser side). Technically it apparently works though. To 'fix' the user experience the proposal would be to get rid of the error page by triggering libconic explicitely to "dial out" if we would hit the error. So libconic and its UI would basically offer the "error handling" according to its configuration. If the user (or configuration) decides to stay online we want to keep the last displayed page also instead of showing an error which can only be left by clicking BACK.

Christian - After talking with all parties, we are ready for you to review attachment 408833 [details] [diff] [review]. It's not the ideal behavior, but it is good enough to Fennec 1.0 and we will be filing followup bugs to improve the behavior for future releases. The patch does require additional front-end work for Fennec and we will be working on that in bug 523296.

Just to clarify. We will be filing a followup bug(s) to improve the connection behavior. We should not ever show the offline error page, unless the autodial fails to find a connection.

Also, we should not be touching IOService::SetOffline directly. We should be using the notifications only.

Comment on attachment 408833 [details] [diff] [review] conn patch for maemo n900 (v6 - trunk) >diff -r c600af9cdd05 netwerk/base/src/Makefile.in >@@ -99,6 +99,11 @@ ifeq ($(MOZ_WIDGET_TOOLKIT),os2) >+ifdef MOZ_ENABLE_LIBCONIC >+ CPPSRCS += nsAutodialMaemo.cpp >+ CPPSRCS += nsNativeConnectionHelper.cpp >+ LOCAL_INCLUDES += -I$(srcdir)/../../system/maemo >+endif >+ifdef MOZ_ENABLE_LIBCONIC >+ OS_INCLUDES += $(GLIB_CFLAGS) $(LIBCONIC_CFLAGS) >+endif indentation seems quasi random >+ // On Windows and Maemo (libconic) we should first check from the OS from => with >+ // if autodial is enabled. If it is enabled then we are allowed if => to see if >@@ -1156,6 +1159,12 @@ static const nsModuleComponentInfo gNetM > NS_NETWORK_LINK_SERVICE_CONTRACTID, > nsNetworkLinkServiceConstructor > }, >+#elif defined(MOZ_ENABLE_LIBCONIC) >+ { NS_NETWORK_LINK_SERVICE_CLASSNAME, >+ NS_NETWORK_LINK_SERVICE_CID, >+ NS_NETWORK_LINK_SERVICE_CONTRACTID, >+ nsMaemoNetworkLinkServiceConstructor This is wrong. a CID is supposed to be tied to a single implementation. >+ // Get the autodial info from the OS and init this object with it. Call it any >+ // time to refresh the object's settings from the OS. That this isn't a public (.idl) method is odd given the "call it any time" statement. >+ nsresult Init(); >+nsMaemoNetworkLinkService::Init(void) >+{ >+ nsresult rv; >+ trailing whitespace >+static void NotifyNetworkLinkObservers() >+{ >+ nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1"); >+ if (!ioService) >+ return; >+ >+ ioService->SetOffline(gInternalState != InternalState_Connected); overidented >+ if (CON_IC_STATUS_CONNECTED == status) >+ gInternalState = InternalState_Connected; >+ else //when we are not connected, we are always disconnected >+ gInternalState = InternalState_Disconnected; use ?: notation? >+ gMonitor = new Monitor("MaemoAutodialer"); >+ NS_ASSERTION(gMonitor, "Out of memory!"); bogus assert, lose it. >+ if (!gMonitor) >+ return PR_FALSE; >+ // notify anyone waiting trailing whitespace

Comment on attachment 408833 [details] [diff] [review] conn patch for maemo n900 (v6 - trunk) Please also fix timeless's comments. +++ b/netwerk/base/src/nsAutodialMaemo.h Tue Oct 27 09:20:18 2009 -0700 + // Get the autodial info from the OS and init this object with it. Call it any + // time to refresh the object's settings from the OS. + nsresult Init(); That comment seems wrong, since the method doesn't do anything. +++ b/netwerk/system/maemo/Makefile.in Tue Oct 27 09:21:12 2009 -0700 +CPPSRCS += nsMaemoNetworkLinkService.cpp nsMaemoNetworkManager.cpp I think normal formatting would be: CPPSRCS += \ nsMaemoNetworkLinkService.cpp \ nsMaemoNetworkManager.cpp \ $(NULL) +OS_INCLUDES += $(GLIB_CFLAGS) +LOCAL_INCLUDES += $(LIBCONIC_CFLAGS) -I$(srcdir)/../../base/src As asked in comment 39, why not put both _CFLAGS in OS_INCLUDES? +++ b/netwerk/system/maemo/nsMaemoNetworkManager.cpp Tue Oct 27 11:58:38 2009 -0700 + // notify anyone waiting + MonitorAutoEnter mon(*gMonitor); + gInternalState = InternalState_Invalid; + mon.Notify(); + } + + delete gMonitor; But by the time you delete gMonitor, there could still be another thread waiting on the monitor. NSPR doesn't allow destroying monitors that are still potentially in use. Alternatively, if you are sure that there are no threads waiting on the monitor here, there's no point in calling Notify.

(In reply to comment #80) > >@@ -1156,6 +1159,12 @@ static const nsModuleComponentInfo gNetM > > NS_NETWORK_LINK_SERVICE_CONTRACTID, > > nsNetworkLinkServiceConstructor > > }, > >+#elif defined(MOZ_ENABLE_LIBCONIC) > >+ { NS_NETWORK_LINK_SERVICE_CLASSNAME, > >+ NS_NETWORK_LINK_SERVICE_CID, > >+ NS_NETWORK_LINK_SERVICE_CONTRACTID, > >+ nsMaemoNetworkLinkServiceConstructor > > This is wrong. a CID is supposed to be tied to a single implementation. > I made all changes except this one. Windows, Linux and now Maemo all use the same NS_NETWORK_LINK_SERVICE_XXX for their respective implementations. Since all are mutually exclusive, it works for now. I can file a bug to fix this up in general.

(In reply to comment #81) > (From update of attachment 408833 [details] [diff] [review]) > Please also fix timeless's comments. Done as noted above > + // Get the autodial info from the OS and init this object with it. Call it > any > + // time to refresh the object's settings from the OS. > + nsresult Init(); > > That comment seems wrong, since the method doesn't do anything. Removed it > +CPPSRCS += nsMaemoNetworkLinkService.cpp nsMaemoNetworkManager.cpp > > I think normal formatting would be: > CPPSRCS += \ > nsMaemoNetworkLinkService.cpp \ > nsMaemoNetworkManager.cpp \ > $(NULL) Changed > +OS_INCLUDES += $(GLIB_CFLAGS) > +LOCAL_INCLUDES += $(LIBCONIC_CFLAGS) -I$(srcdir)/../../base/src > > As asked in comment 39, why not put both _CFLAGS in OS_INCLUDES? No idea. I put LIBCONIC_CFLAGS in OS_INCLUDES > + // notify anyone waiting > + MonitorAutoEnter mon(*gMonitor); > + gInternalState = InternalState_Invalid; > + mon.Notify(); > + } > + > + delete gMonitor; > > But by the time you delete gMonitor, there could still be another thread > waiting on the monitor. NSPR doesn't allow destroying monitors that are still > potentially in use. > > Alternatively, if you are sure that there are no threads waiting on the monitor > here, there's no point in calling Notify. After talking on IRC about this, we decided to leak the monitor for now. I added a comment in the code making it explicit about why we did it.

Patch with comments addressed

Patch didn't break anything on try server and my build was functional in a local Fennec Maemo build.

pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/784cd37c4665 pushed to m-192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/eff5f946faf4