Closed Bug 482897 Opened 11 years ago Closed 11 years ago

AutoDialer support on Windows Mobile

Categories

(Core :: Networking, defect)

ARM
Windows Mobile 6 Standard
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: dougt, Assigned: dougt)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

when fennec starts a network load and there isn't already an existing data connection, we timeout.  As discussed, we should create a data connection using the connection manager api similar to what we do on other flavors of windows.

Basically there is a connection manager for windows mobile that allows you to very easily kick off a connection.  It also allows you to check to see if you are connected.

Previously, this use to work in the 1.8 code base.  However, when porting the 1.9 codebase, we ignored this area -- and now we are addressing it.

Currently, on windows 32, we support RAS autodialing.  The api on windows mobile is significantly different than RAS and the "rasdlg.h" header is completely broken in the SDK, I factored out the windows mobile code into a separate file.  I did the same also for the link monitoring code.

The following patch implements autodialing and link monitoring.
Attached patch patch v.1 (obsolete) — Splinter Review
WIP - I need to clean up the ws and license blocks.
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0a1-wm+
Comment on attachment 366990 [details] [diff] [review]
patch v.1

blassey, this is pretty close to what we want.  could you review?
Attachment #366990 - Flags: review?(bugmail)
Comment on attachment 366990 [details] [diff] [review]
patch v.1

>diff --git a/netwerk/base/src/nsAutodialWinCE.cpp b/netwerk/base/src/nsAutodialWinCE.cpp
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/base/src/nsAutodialWinCE.cpp
>+// pulled from the header so that we do not get multiple define errors during link
>+static const GUID ras_DestNetInternet =
>+        { 0x436ef144, 0xb4fb, 0x4863, { 0xa0, 0x41, 0x8f, 0x90, 0x5a, 0x62, 0xc5, 0x72 } };
>+

This looks fragile.  Is this because of the bad comment in the header?

>+nsresult nsRASAutodial::DialDefault(const PRUnichar* hostName)

nit, it looks like hostName isn't used. changing to "const PRUnichar* /*hostName*/" will avoid a compiler warning


>+  
>+  conn_info.cbSize      = sizeof(conn_info);
>+  conn_info.dwParams    = CONNMGR_PARAM_GUIDDESTNET;

CONNMGR_PARAM_GUIDDESTNET isn't listed as a valid param in the msdn docs I'm looking at.  What does it mean and why do we need it?


>+  HRESULT result = ConnMgrEstablishConnectionSync(&conn_info, 
>+                          &connectionHandle, 
>+                          60000,
>+                          &status);
>+  if (result != S_OK)
>+    return NS_ERROR_FAILURE;
>+
>+  ConnMgrReleaseConnection(connectionHandle, 0);

Why are you using 0 here? It indicates that the connection shouldn't be cached.  Perhaps this should be configurable. 

>diff --git a/netwerk/system/wince/nsNotifyAddrListener.cpp b/netwerk/system/wince/nsNotifyAddrListener.cpp
>new file mode 100644

I've got the same questions for this file.  Also, is there anyway not to repeat this code?
Attachment #366990 - Flags: review?(bugmail) → review-
thanks brad for the quick review:

> This looks fragile.  Is this because of the bad comment in the header?

Better than that!  #include <initguid.h> from two separate files, compile them into seperate objects, then link them together in a single DLL.  Compiler error. :-(

> nit, it looks like hostName isn't used. changing to "const PRUnichar*
/*hostName*/" will avoid a compiler warning

Fixed.

>CONNMGR_PARAM_GUIDDESTNET isn't listed as a valid param in the msdn docs I'm
looking at.  What does it mean and why do we need it?

http://msdn.microsoft.com/en-us/library/aa455861.aspx

> Why are you using 0 here? 

Because I do not want to manage the connection.  All mozilla should do is start the connection and let the system decide when it wants to close the connection.

>Also, is there anyway not to repeat this code?

The code is a bit different.  One does a sync connection on the socket thread that demands that the OS make a connection right away (that is the ConnMgrEstablishConnectionSync call in the AutoDialer).  The other makes a async call which doesn't create a connection, and it just does enough so that we can get status of the connection.

I can fix the nit, give the above answers and another r= from a necko peer or sr, do you have any further suggestions?
brad convinced me that the ConnMgrReleaseConnection cache value should be a pref.
(In reply to comment #4)
> http://msdn.microsoft.com/en-us/library/aa455861.aspx

wow...that makes is so much more clear!

> I can fix the nit, give the above answers and another r= from a necko peer or
> sr, do you have any further suggestions?

r=me with that nit fixed and the cache value configurable (and a sr or r from a necko peer)
Hardware: x86 → ARM
Attached patch patch v.2Splinter Review
brad, here is the nit fixed and a preferences for the default cache value.
Attachment #366990 - Attachment is obsolete: true
Attachment #367220 - Flags: review?(bugmail)
Attachment #367220 - Flags: review?(bugmail) → review+
Comment on attachment 367220 [details] [diff] [review]
patch v.2

jduell, can you review this change?
Attachment #367220 - Flags: review?(jduell.mcbugs)
Comment on attachment 367220 [details] [diff] [review]
patch v.2

Approved.  I don't know the WinCE APIs at all, so I'm taking lots of this patch of faith.

One question about the nal_DestNetInternet hack:  so you get multiple definition errors at linktime if you #include some WinCE header in two files?  Lovely.   I'm  trying to think of some hack to work around this.  I'm assuming you #include the header in a different file than this, and just cut-and-paste the value here.  Could you define some symbol in the other file, and/or provide a function for getting the value, or some other kluge that would at least keep us from hard-coding in values that might conceivable change in the header some day?

Otherwise looks fine.
Attachment #367220 - Flags: review?(jduell.mcbugs) → review+
regarding the DestNetInternet hack -- We probably could create a wince autodialing service that supports both places in necko that need to use this function, but i thought that was overkill.  I will add a better comment and welcome to wince. :-)
Attachment #367220 - Flags: approval1.9.1?
http://hg.mozilla.org/mozilla-central/rev/a47a52a6865f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 367220 [details] [diff] [review]
patch v.2

a191=beltzner
Attachment #367220 - Flags: approval1.9.1? → approval1.9.1+
You need to log in before you can comment on or make changes to this bug.