465664 - Support for tel: and dialing on Windows Mobile

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Support for tel: links was added to Fennec for Maemo, but we also need support for Windows Mobile. The plan is to create a basic XPCOM component to access the platform phone functionality and then modify the tel: protocol handler added in bug 437949 to use the XPCOM component on Windows Mobile.

Code from Minimo could be used as a basis for the XPCOM component:
http://mxr.mozilla.org/mozilla1.8/source/minimo/components/phone/

Comment 1

[Nino D'Aversa (:ndaversa)]

Attachment: Adding tel: link support to Windows Mobile

This patch should be applied to the mobile directory. I was unsure if it needed to be in main source (mozilla-central) or in mobile.

If this is incorrect I can redo the patch.

Comment 2

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

Comment on attachment 351704 [details] [diff] [review]
Adding tel: link support to Windows Mobile

Just some drive by comments/questions:
* Code uses "The Original Code is Minimo." in the license headers. Should we change this to "Mozilla" or whatever we're using for Fennec?
* sendSMS is still included in the IDL/CPP. Should we remove it for now? I think we should.
* The code uses #ifdef WINCE in the impl CPP file. I think we should always assume this file is being built for WINCE and add the #ifdef to the Makefile.in that adds "phone" to DIRS.
* nsTelProtocolHandler.js uses a try/catch handler (like the existing code. I think this should be a #ifdef WINCE situation and not a try/catch.

Comment 3

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

OK, I know why the nsTelProtocolHandler.js file was not supporting the #ifdef WINCE preprocessing stuff in it. The Makefile.in uses:

EXTRA_COMPONENTS = nsTelProtocolHandler.js

Should be:

EXTRA_PP_COMPONENTS = nsTelProtocolHandler.js

Then the preprocessing will work.

See here: http://mxr.mozilla.org/mobile-browser/source/components/protocols/Makefile.in#47

Comment 4

[Nino D'Aversa (:ndaversa)]

Attachment: Adding tel: link support to Windows Mobile v2

As per Mark's suggestions I have changed the following:

* Code uses "The Original Code is Minimo." in the license headers. Should we
change this to "Mozilla" or whatever we're using for Fennec?

DONE!

* sendSMS is still included in the IDL/CPP. Should we remove it for now? I
think we should.

DONE!

* The code uses #ifdef WINCE in the impl CPP file. I think we should always
assume this file is being built for WINCE and add the #ifdef to the Makefile.in
that adds "phone" to DIRS.

DONE!

* nsTelProtocolHandler.js uses a try/catch handler (like the existing code. I
think this should be a #ifdef WINCE situation and not a try/catch.

DONE!

Comment 5

[Doug Turner (:dougt)]

Given that I wrote teh code for the project minimio, it probably should read something like "The Original Code the Minimo tel protocol handler", but I don't care if you want to call it something else

Comment 6

[Nino D'Aversa (:ndaversa)]

Attachment: Adding tel: link support to Windows Mobile v3

As per Doug's suggestion I have updated the patch:

Given that I wrote teh code for the project minimio, it probably should read
something like "The Original Code the Minimo tel protocol handler", but I don't
care if you want to call it something else

DONE!

Comment 7

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

Comment on attachment 351804 [details] [diff] [review]
Adding tel: link support to Windows Mobile v3

>diff -r cdff44cd6045 components/Makefile.in

>
>+ifdef WINCE
>+DIRS = protocols \
>+       phone \
>+	$(NULL)
>+else
> DIRS = protocols \
> 	$(NULL)
>+endif       	
>

Try this:

DIRS = protocols \
ifdef WINCE
       phone \
endif       	
       $(NULL)

>diff -r cdff44cd6045 components/phone/Makefile.in

>+REQUIRES	= \
>+		  xpcom \
>+		  string \
>+		  pref \
>+		  $(NULL)
>+


>diff -r cdff44cd6045 components/phone/nsPhoneSupport.cpp

>+#ifdef WINCE
>+#include "windows.h"
>+#include "phone.h"
>+#include "sms.h"
>+#endif

>+nsPhoneSupport::MakeCall(const PRUnichar *telephoneNumber, const PRUnichar *telephoneDescription)
>+{
>+#ifdef WINCE

...

>+#else
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+#endif

>diff -r cdff44cd6045 components/protocols/nsTelProtocolHandler.js

>     phoneNumber = phoneNumber.substring(phoneNumber.indexOf(":") + 1, phoneNumber.length);    
>     phoneNumber = encodeURI(phoneNumber);
>     
>     var ios = Components.classes[kIOSERVICE_CONTRACTID].getService(Ci.nsIIOService);

Why is this variable outside the #else? It is only used in the non-WINCE part


I promise to let Brad do the next review.

Comment 8

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

>+#ifdef WINCE
>+#include "windows.h"
>+#include "phone.h"
>+#include "sms.h"
>+#endif

>+nsPhoneSupport::MakeCall(const PRUnichar *telephoneNumber, const PRUnichar *telephoneDescription)
>+{
>+#ifdef WINCE

...

>+#else
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+#endif

Why are any of these #ifdef WINCE needed anumore? We are ensuring that the file is only built if WINCE is defined in the Makefile.in

Comment 9

[Mike Shaver (:shaver emeritus)]

(In reply to comment #7)
> (From update of attachment 351804 [details] [diff] [review])
> >diff -r cdff44cd6045 components/Makefile.in
> 
> >
> >+ifdef WINCE
> >+DIRS = protocols \
> >+       phone \
> >+	$(NULL)
> >+else
> > DIRS = protocols \
> > 	$(NULL)
> >+endif       	
> >
> 
> Try this:
> 
> DIRS = protocols \
> ifdef WINCE
>        phone \
> endif           
>        $(NULL)

I'd be surprised if that worked -- ifdef would be merged into that line, and not do what you want it to.

DIRS = protocols \
       $(NULL)

ifdef WINCE
DIRS += phone
endif

Comment 10

[Nino D'Aversa (:ndaversa)]

Attachment: Adding tel: link support to Windows Mobile v4

As per mfinkle and shaver's comments:

>DIRS = protocols \
>       $(NULL)
>
>ifdef WINCE
>DIRS += phone
>endif

DONE!


>Why is this variable outside the #else? It is only used in the non-WINCE part

DONE!

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 351843 [details] [diff] [review]
Adding tel: link support to Windows Mobile v4


>+#include "windows.h"
>
>+#include "phone.h"
>
>+#include "sms.h"

use <header.h> instead of "header.h" for system headers

>+nsPhoneSupport::MakeCall(const PRUnichar *telephoneNumber, const PRUnichar *telephoneDescription)
>
>+{
>
>+  long result = -1;
>
>+
>
>+  typedef LONG (*__PhoneMakeCall)(PHONEMAKECALLINFO *ppmci);
>
>+
>
>+  HMODULE hPhoneDLL = LoadLibrary(L"phone.dll");
>

this should be LoadLibraryW()
>-EXTRA_COMPONENTS = nsTelProtocolHandler.js
>
>+EXTRA_PP_COMPONENTS = nsTelProtocolHandler.js

why make this change?  to be honest I don't know the difference between the two
>+#ifdef WINCE
>
>+      var phoneInterface= Components.classes["@mozilla.org/phone/support;1"].createInstance(Ci.nsIPhoneSupport);
>
>+      phoneInterface.makeCall(phoneNumber,"");
>
>+      return NS_OK;

newChannel should return a channel.

>
>+#else
>
>     var ios = Components.classes[kIOSERVICE_CONTRACTID].getService(Ci.nsIIOService);
>
...

>     return channel;
>
>+#endif
>
>   }
>

I wouldn't ifdef out the rest of this, its a good fallback to try these equivalent protocols to see if we have a handler (like skype) installed.



Also, watch your line endings.  I suspect the reason this patch looks like its double spaced in the editor may be due to windows line endings.

Finally, you're using PhoneMakeCall(), there is also tapiRequestMakeCall().  I think the major difference is the latter pops up a dialing dialog and the former just makes the call.  We may want to use the latter for the tel uri handler so we don't dial the user's phone when they don't want it to be dialed (since that can be expensive in some situations). The former may be useful in other situations though, so it would be nice to expose them both to privileged javascript.

Comment 12

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

(In reply to comment #11)

> >-EXTRA_COMPONENTS = nsTelProtocolHandler.js
> >
> >+EXTRA_PP_COMPONENTS = nsTelProtocolHandler.js
> 
> why make this change?  to be honest I don't know the difference between the two

EXTRA_PP_COMPONENTS will preprocess the file first. Since the file now has a #ifdef WINCE, that is needed. (even if we remove the #else and do a fallback)

Comment 13

[Nino D'Aversa (:ndaversa)]

Attachment: Adding tel: link support to Windows Mobile v5

Brad thanks for the previous review, I have made the appropriate changes, let me know what you think...

>use <header.h> instead of "header.h" for system headers

DONE!

> this should be LoadLibraryW()

DONE!

> why make this change?  to be honest I don't know the difference between the two

DONE - the PP tells the file to be preprocessed so I can use the #ifdef WINCE, so its needed

> newChannel should return a channel.

DONE - I have created a BogusChannel as per mfinkle's recommendation. Returning a null channel causes certain ensure_success to fail, also just creating a channel like let channel = ios.newChannel("tel:"+phoneNumber, null, null);  creates an infinite loop, this BogusChannel should handle that problem, it was taken from the chatzilla-service.js.

>I wouldn't ifdef out the rest of this, its a good fallback to try these
>equivalent protocols to see if we have a handler (like skype) installed.

DONE

>Also, watch your line endings.  I suspect the reason this patch looks like its
>double spaced in the editor may be due to windows line endings.

DONE ? - I'm not sure about this, but I have saved with UNIX line endings and double checked it on my system, seems fine, give it another go (mfinkle tells me he has had similar issues, may be bugzilla related, but not sure).

>Finally, you're using PhoneMakeCall(), there is also tapiRequestMakeCall().  I
>think the major difference is the latter pops up a dialing dialog and the
>former just makes the call.  We may want to use the latter for the tel uri
>handler so we don't dial the user's phone when they don't want it to be dialed
>(since that can be expensive in some situations). The former may be useful in
>other situations though, so it would be nice to expose them both to privileged
>javascript.

DONE - instead of calling the two separate methods I have just used the callInfo.dwFlags, PMCF_PROMPTBEFORECALLING and PMCF_DEFAULT this is controlled by the PRBool aPrompt parameter I added.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 353779 [details] [diff] [review]
Adding tel: link support to Windows Mobile v5


>+MODULE_NAME     = mozphone
>
>+MODULE		    = mozphone
>
>+LIBRARY_NAME	= mozphone
>
>+

nit, fix spacing


>+#EXTRA_DSO_LDOPTS += $(MOZ_COMPONENT_LIBS)
>
if this line isn't needed, delete it






>+      PHONEMAKECALLINFO callInfo;
>
>+      callInfo.cbSize          = sizeof(PHONEMAKECALLINFO);
>
>+      callInfo.dwFlags         = aPrompt ? PMCF_PROMPTBEFORECALLING : PMCF_DEFAULT;

It looks like we would want the PMCF_ALLOWSUPSVCS flag as well, otherwise the user won't be able to send DTMFs (As I read it anyway).



>
>+ifdef WINCE
>
>+DEFINES += -DWINCE=$(WINCE)
>
>+endif 

this shouldn't be necessary 


>+      let channel = new BogusChannel (aURI, phoneNumber);

>
>+function BogusChannel(URI, phoneNumber)
>

Please change the name of BogusChannel to something more descriptive (nsWinceTelChannel?).  Also it should be wrapped in #ifdef WINCE since its not valid to call it on any other platform.


r=me with those changes

Comment 15

[Nino D'Aversa (:ndaversa)]

Attachment: Adding tel: link support to Windows Mobile v6 [Checkin: Comment 16]

Once again Brad, thanks for the review, are the the modifications you recommended, some were not possible, see notes...

> nit, fix spacing
DONE!

> if this line isn't needed, delete it
IT WASN'T, REMOVED, DONE!

> It looks like we would want the PMCF_ALLOWSUPSVCS flag as well, otherwise the
> user won't be able to send DTMFs (As I read it anyway).
PMCF_ALLOWSUPSVCS IS NO LONGER A VALID FLAG IN WINCE6, ONLY FOR 5. SEE http://msdn.microsoft.com/en-us/library/bb416268.aspx  - DONE

> this shouldn't be necessary 
I NEED THIS DEFINE OTHERWISE MY CODE IN nsTelProtocolHandler.js NEVER EXECUTES, PERHAPS THERE IS ANOTHER WAY AROUND THIS? - DONE?

> Please change the name of BogusChannel to something more descriptive
> (nsWinceTelChannel?).  Also it should be wrapped in #ifdef WINCE since its not
> valid to call it on any other platform.
I HAVE CHANGED THE NAME TO nsWinceTelChannel AS YOU HAVE SUGGESTED - DONE!

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mobile-browser/rev/1c0778791447