Closed Bug 880561 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: ctai, Assigned: ctai)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
This bug block leo+ bug 862764. Nominate for leo+.
blocking-b2g: --- → leo?
Summary: B2G MMS: Add more error status for data connection off on share APN, no sim card, flight mode when sending MMS. → B2G MMS: Add more error status for data connection off on share APN, no sim card when sending MMS.
Gaia AP can get the flight mode information, no need this from Gecko.
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.
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
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.
Flags: needinfo?(oteo)
Flags: needinfo?(kev)
Change the summary, because of bug 862764 don't want to use error code to trigger a data connection on/off dialogue.
Summary: B2G MMS: Add more error status for data connection off on share APN, no sim card when sending MMS. → B2G MMS: Add 2 more error status (in flight mode and no sim card) when sending MMS.
I thinks this is a leo+ bug. This bug blocks bug 880257(leo+ already). Gaia developers need this quick error return.
No longer blocks: 862764
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #760744 - Attachment is obsolete: true
Attachment #760745 - Flags: feedback?(vyang)
(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).
Flags: needinfo?(oteo)
No longer blocks: 880257
See Also: → 880257
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.
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 ?).
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).
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!!!
Flags: needinfo?(vyang)
Flags: needinfo?(gene.lian)
(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.
Flags: needinfo?(vyang)
Flags: needinfo?(gene.lian)
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! ;)
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).
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.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #760745 - Attachment is obsolete: true
Attachment #760745 - Flags: feedback?(vyang)
Attachment #761855 - Flags: review?(gene.lian)
Blocks: 880257
See Also: 880257
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| ?
Attachment #761855 - Flags: review?(gene.lian)
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #761855 - Attachment is obsolete: true
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attached patch Patch v1.4 (obsolete) — Splinter Review
Attachment #763444 - Attachment is obsolete: true
Attachment #763446 - Flags: review?(gene.lian)
blocking-b2g: leo? → leo+
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
Attachment #763446 - Flags: review?(gene.lian) → review+
Attached patch Patch v1.5Splinter Review
Attachment #763446 - Attachment is obsolete: true
https://hg.mozilla.org/projects/birch/rev/d1592f77f459

Should this have tests?
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d1592f77f459
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Flags: in-moztrap-
Flags: needinfo?(kev)
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.
No longer blocks: 885652
You need to log in before you can comment on or make changes to this bug.