880561 - B2G MMS: Add 2 more error status (in flight mode and no sim card) when sending MMS.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

For some cases, we should notify the error when sending MMS fail.
Add more error status for data connection off on share APN, no sim card, flight mode when sending MMS.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

This bug block leo+ bug 862764. Nominate for leo+.

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Gaia AP can get the flight mode information, no need this from Gecko.

Comment 3

[Gene Lian (I already quit Mozilla)]

I'm wondering we really need this. We've already had Settings API to know the dynamic status of Data Connection. The content app can check that setting and have the corresponding UI when it gets the "failed" event for sending MMS.

Comment 4

[Gene Lian (I already quit Mozilla)]

Just for the records. If we decide to add more error codes to fix this issue, please firstly choose the existing enumerations at [1][2]. If it's too limited to specify our needs then we can start considering adding new types.

[1] dom/base/nsDOMException.cpp
[2] http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#domexception

Comment 5

[Peter Dolanjski [:pdol]]

Kev/Maria, I'll defer to you here.  Do we need to get more granular wrt MMS sending error codes (and the associated UI to the user)?  I see it as useful, but not critical to the release.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Change the summary, because of bug 862764 don't want to use error code to trigger a data connection on/off dialogue.

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I thinks this is a leo+ bug. This bug blocks bug 880257(leo+ already). Gaia developers need this quick error return.

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.0

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.1

Comment 10

[Maria Angeles Oteo (:oteo)]

(In reply to Peter Dolanjski [:pdol] from comment #5)
> Kev/Maria, I'll defer to you here.  Do we need to get more granular wrt MMS
> sending error codes (and the associated UI to the user)?  I see it as
> useful, but not critical to the release.

Hi Peter,

 At least we should do it for the flight mode as it is totally inconsistent with the behaviour already implemented for SMS. I.e. we should block on the flght mode information.

 If we add the same kind of alert for the no SIM Card scenario for SMS (bug 8767999) we should do the same for MMS. Blocking on NO SIM Card depends on whether bug 876799 becomes a blocker or not. 

 However, what is important is that whenever an error is permanent (no SIM Card, Flight Mode, etc...) the message is directly marked as not possible to be sent (e.g. with the exclamation mark) and not showing the "trying to send" spinner we are currently showing for MMS under those conditions (bug 880257).

Comment 11

[Julien Wajsberg [:julienw]]

For SMS, we only get the failed event, and then we check if we're in airplane mode using the Settings API. We can also use window.navigator.mozMobileConnection to know if we eg have a SIM card.

Therefore, I don't get why we need to add new error status, because we already have all the information we need.

Comment 12

[Julien Wajsberg [:julienw]]

I'd be tempted to close WONTFIX, unless we bugmorph this bug to handle all this in Gaia (but I think we have other bugs for this, right ?).

Comment 13

[Julien Wajsberg [:julienw]]

I missed that we need to fail early (without the retry mechanism) in some situations. If that's what this bug is about, then I'm ok with this (but still not with adding more error status).

Comment 14

[Borja Salguero [:borjasalguero]]

Hi Vicamo and Gene! We have been reviewing the status management when a MMS is sent with no SIM Card or Flight mode and differs from SMS Status. Here we have the following:

NO SIM CARD: 	SMS (Error)  | MMS (Sending status forever)
Flight mode: 	        SMS (Error)  | MMS (Sending status forever)

Probably is something related with our re-trial policy… However, Could we have the same policy for these 2 scenarios when sending a SMS and MMS? When 'NO SIM Card' or 'Flight mode' could we retrieve a 'fail/error' state? This let the user to send it again if needed, showing an alert informing about the reason. Wdyt? Thanks!!!

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Borja Salguero [:borjasalguero] from comment #14)
> However, Could we
> have the same policy for these 2 scenarios when sending a SMS and MMS? When
> 'NO SIM Card' or 'Flight mode' could we retrieve a 'fail/error' state? This
> let the user to send it again if needed, showing an alert informing about
> the reason. Wdyt? Thanks!!!

We definitely should report errors as soon as possible.  The problem here is the original setupDataConnectionByType call does not callback whenever it's success or not.  That's a big trouble, but should be fixed anyway.

Comment 16

[Borja Salguero [:borjasalguero]]

We have some stuff in Gaia to add based on this change... Do you know if there is any bug for fixing this issue? Is there anybody working on it? Thanks a lot! Gracias! ;)

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Borja, I already do a patch(https://bugzilla.mozilla.org/attachment.cgi?id=760745) on this bug.

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Julien, yes, this bug and patch is try to return failure early. I think those two more error status is necessary. Because might be other SMS AP don't deal with this situation. For gecko part, we already know there is no-sim card or flight mode error. Might be other SMS AP want to do other behaviour other than stock SMS AP. 

(In reply to Julien Wajsberg [:julienw] from comment #13)
> I missed that we need to fail early (without the retry mechanism) in some
> situations. If that's what this bug is about, then I'm ok with this (but
> still not with adding more error status).

Comment 19

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Vicamo, actually we don't need to call setupDataConnectionByType if we know it is no-sim card and flight mode. So I don't think this bug is related to the original setupDataConnectionByType call does not callback.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> (In reply to Borja Salguero [:borjasalguero] from comment #14)
> > However, Could we
> > have the same policy for these 2 scenarios when sending a SMS and MMS? When
> > 'NO SIM Card' or 'Flight mode' could we retrieve a 'fail/error' state? This
> > let the user to send it again if needed, showing an alert informing about
> > the reason. Wdyt? Thanks!!!
> 
> We definitely should report errors as soon as possible.  The problem here is
> the original setupDataConnectionByType call does not callback whenever it's
> success or not.  That's a big trouble, but should be fixed anyway.

Comment 20

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.2

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 761855 [details] [diff] [review]
Patch v1.2

Review of attachment 761855 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
@@ +26,5 @@
> +  const unsigned short NOT_FOUND_ERROR           = 2;
> +  const unsigned short UNKNOWN_ERROR             = 3;
> +  const unsigned short INTERNAL_ERROR            = 4;
> +  const unsigned short NO_SIM_CARD_ERROR         = 5;
> +  const unsigned short FLIGHT_MODE_ERROR         = 6;

|FLIGHT_MODE_ERROR| is a bit implicit. Also, I'd prefer using |RADIO_DISABLED_ERROR| because you're actually listening to the radio status which doesn't always imply the flight mode status. For example, I can turn off the radio without turning off the flight mode. Right?

::: dom/mobilemessage/src/MobileMessageCallback.cpp
@@ +88,5 @@
> +    case nsIMobileMessageCallback::NO_SIM_CARD_ERROR:
> +      mDOMRequest->FireError(NS_LITERAL_STRING("NoSimCardError"));
> +      break;
> +    case nsIMobileMessageCallback::FLIGHT_MODE_ERROR:
> +      mDOMRequest->FireError(NS_LITERAL_STRING("FlightModeError"));

Can we use "RadioDisabledError"?

::: dom/mobilemessage/src/ril/MmsService.js
@@ +128,5 @@
>      mmsc: null,
>      proxy: null,
>      port: null,
>  
> +    // For flight mode.

// For keeping track of the radio status.

@@ +188,5 @@
>          }
>          this.proxy = Services.prefs.getCharPref("ril.mms.mmsproxy");
>          this.port = Services.prefs.getIntPref("ril.mms.mmsport");
>          this.updateProxyInfo();
> +        this.radioDisabled = Services.prefs.getBoolPref("ril.radio.disabled");

Please don't put this line in this try-catch which should only handle the MMS proxy settings.

@@ +287,5 @@
>        this.mmsc = null;
>        this.proxy = null;
>        this.port = null;
>        this.proxyInfo = null;
> +      this.radioDisabled = false;

Please don't put this assignment in the .clearMmsProxySettings(...) which is not doing this task as it is doing.

@@ +340,5 @@
>                case "ril.mms.mmsport":
>                  this.port = Services.prefs.getIntPref("ril.mms.mmsport");
>                  this.updateProxyInfo();
>                  break;
> +              case "ril.radio.disabled":

The same. I think we should put this line out of this try-catch block and handle this separately.

@@ +1472,5 @@
>                            function notifySendingResult(aRv, aDomMessage) {
>        if (DEBUG) debug("Saving sending message is done. Start to send.");
> +
> +      // For flight mode error.
> +      if(gMmsConnection.radioDisabled) {

This is a bit weird to me, because gMmsConnection.radioDisabled doesn't imply the flight mode is on. Right?

@@ +1473,5 @@
>        if (DEBUG) debug("Saving sending message is done. Start to send.");
> +
> +      // For flight mode error.
> +      if(gMmsConnection.radioDisabled) {
> +        if (DEBUG) debug("Error! Flight mode is on when sending MMS.");

We should write:

"Error! Radio is disabled when sending MMS."

@@ +1474,5 @@
> +
> +      // For flight mode error.
> +      if(gMmsConnection.radioDisabled) {
> +        if (DEBUG) debug("Error! Flight mode is on when sending MMS.");
> +        sendTransactionCb(aDomMessage.id, Ci.nsIMobileMessageCallback.FLIGHT_MODE_ERROR);

s/FLIGHT_MODE_ERROR/RADIO_DISABLED_ERROR

@@ +1500,5 @@
>          let isSentSuccess = (aMmsStatus == MMS.MMS_PDU_ERROR_OK);
>          if (DEBUG) debug("The sending status of sendTransaction.run(): " + aMmsStatus);
> +        sendTransactionCb(aDomMessage.id, isSentSuccess?
> +          Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR:
> +          Ci.nsIMobileMessageCallback.INTERNAL_ERROR);

sendTransactionCb(aDomMessage.id, isSentSuccess?
                  Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR :
                  Ci.nsIMobileMessageCallback.INTERNAL_ERROR);

Maybe it looks better.

::: embedding/android/GeckoSmsManager.java
@@ +306,5 @@
> +  public final static int kNotFoundError          = 2;
> +  public final static int kUnknownError           = 3;
> +  public final static int kInternalError          = 4;
> +  public final static int kNoSimCardError         = 5;
> +  public final static int kFlightModeError        = 6;

Can we use |kRadioDisabledError| ?

Comment 22

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.3

Comment 23

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.4

Comment 24

[Gene Lian (I already quit Mozilla)]

Comment on attachment 763446 [details] [diff] [review]
Patch v1.4

Review of attachment 763446 [details] [diff] [review]:
-----------------------------------------------------------------

r=gene after the following minor nits fixed. Thanks!

::: dom/mobilemessage/src/ril/MmsService.js
@@ +190,5 @@
>          this.port = Services.prefs.getIntPref("ril.mms.mmsport");
>          this.updateProxyInfo();
>        } catch (e) {
> +        if (DEBUG) debug("Unable to initialize the MMS proxy settings from " +
> +                         "the preference. This could happen at the first-run." +

Add one space at:

"... first-run. "

@@ +198,5 @@
> +
> +      try {
> +        this.radioDisabled = Services.prefs.getBoolPref("ril.radio.disabled");
> +      } catch (e) {
> +        if (DEBUG) debug("Get perference 'ril.radio.disabled' fail.");

Please refer to bug 866366, comment #58. Let's rewrite this log to be present progressive:

s/Get/Getting/
s/perference/preference
s/fail/fails

@@ +330,5 @@
>            }
>            break;
>          }
>          case kPrefenceChangedObserverTopic: {
> +          if (data == "ril.radio.disabled") {

Please add a |return;| for this if-block to make it early returned so that we don't need to fall thorough the following MMS proxy setting things.

@@ +334,5 @@
> +          if (data == "ril.radio.disabled") {
> +            try {
> +              this.radioDisabled = Services.prefs.getBoolPref("ril.radio.disabled");
> +            } catch (e) {
> +              if (DEBUG) debug("Update perference 'ril.radio.disabled' fail.");

s/Update/Updating/
s/perference/preference/
s/fail/fails/

@@ +1448,5 @@
>  
>      let self = this;
>  
> +    let sendTransactionCb = function sendTransactionCb(aRecordId, aErrorCode) {
> +      if (DEBUG) debug("The success status of sending transaction: " + aErrorCode);

s/success status/error code/

@@ +1490,5 @@
> +        sendTransactionCb(aDomMessage.id, Ci.nsIMobileMessageCallback.RADIO_DISABLED_ERROR);
> +        return;
> +      }
> +
> +      // For sim card is not ready.

s/sim/SIM for special terms written in the comment or log.

@@ +1492,5 @@
> +      }
> +
> +      // For sim card is not ready.
> +      if(gRIL.rilContext.cardState != "ready") {
> +        if (DEBUG) debug("Error! Sim card is not ready when sending MMS.");

s/Sim/SIM

Comment 25

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/d1592f77f459

Should this have tests?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1592f77f459

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8c992666a448

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8c992666a448

Comment 30

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Borja, I don't think this bug block bug 885652.
This bug is for MMS not SMS. Bug 885652 is talking about SMS.
For this bug, I am pretty sure the onerror and onsuccess callback is triggered correctly.
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=880257#c31 for more information.