981577 - [Messages] We should show the correct error message when trying to download a MMS in airplane mode

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* set "MMS Automatic retrieval" to off
* receive a MMS with attachment
* enable airplane mode
* try to download the MMS

Expected:
* we should see an error related to airplane mode

Actual:
* no error is displayed, only the error icon is added.

Note that I didn't find any error message related to airplane mode for this case (we have one for _sending_ messages but it's not suited for this bug).

NI ayman for providing more information.
QAwanted to know the behavior in v1.1.

Comment 1

[Julien Wajsberg [:julienw]]

Actual in 1.4:
* the "missing sim card" error is displayed.

Comment 2

[Julien Wajsberg [:julienw]]

Related to Bug 981077.

When sending, we get a "RadioDisabledError" error from Gecko. Maybe we should have the same one here?

(and in that case we'll need Gaia change because we'll need to distinguish sending errors from receiving errors).

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Yes, for retrieving, we should check airplane mode settings with higher priority than checking iccid.

Comment 5

[Jason Smith [:jsmith]]

Minor bug, so there's no value to pursuing a branch comparison here.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part1: refactor fetching ril.radio.disabled preference into getRadioDisabledState(). r=vyang

1. Move internally utilities to the same code segment.
2. Wrap fetching ril.radio.disabled into new function of getRadioDisabledState().

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part2: Check Radio State in prior to the ICC Id. r=vyang.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part3: Add Test case to verify RadioDisabledError. r=vyang.

1. Move redundant code into head.js.
2. Add new test case to verify error code of retrieving while airplane mode is on.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part3 v2: Add Test case to verify RadioDisabledError. r=vyang.

I'd like to run this test in chrome process for more flexible content creation of MMS notification that could also be used in bug 981077.

Comment 10

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

Comment on attachment 8396172 [details] [diff] [review]
Patch Part3 v2: Add Test case to verify RadioDisabledError. r=vyang.

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

::: dom/mobilemessage/tests/marionette/head.js
@@ +372,5 @@
>    return runEmulatorCmdSafe(command);
>  }
>  
>  /**
> + * Send raw SMS TPDU to emulator and wait

nit: 'multiple raw ...'

@@ +387,5 @@
> + *   result -- an array of emulator response lines.
> + *
> + * @return A deferred promise.
> + */
> +function sendRawSmsAndWait(aPdus) {

nit: please rename to `sendMultipleRawSmsToEmulatorAndWait`.

::: dom/mobilemessage/tests/marionette/test_error_of_mms_manual_retrieval.js
@@ +4,5 @@
> +MARIONETTE_TIMEOUT = 60000;
> +// We apply "chrome" context to be more flexible to
> +// specify the content of M-Notification.ind such as iccId
> +// for different kinds of testing.
> +MARIONETTE_CONTEXT = "chrome";

I think we can reuse the |buildBinaryPdus| function you had in “test_mt_sms_concatenation.js” to construct an MMS notification indication PDU.  This way, we don't have to run this script in chrome context and can reuse all the utility functions in "head.js".

::: dom/mobilemessage/tests/marionette/test_replace_short_message_type.js
@@ +38,5 @@
>    let pdu = buildPdu(aSender, aPid, PDU_UD_B);
>    ok(true, "Sending " + pdu);
>  
> +  return sendRawSmsAndWait([pdu])
> +    .then((results) => function(results) {

function(results) {

@@ +59,5 @@
>  function testPid(aPid) {
>    log("Test message PID '" + aPid + "'");
>  
> +  return sendRawSmsAndWait([buildPdu(PDU_SENDER_0, aPid, PDU_UD_A)])
> +    .then((results) => function(results) {

ditto.

@@ +67,5 @@
>        for (let pid of PDU_PIDS) {
>          let verify = (aPid !== PDU_PID_NORMAL && pid === aPid)
>                     ? verifyReplaced : verifyNotReplaced;
>          promise =
> +          promise.then(verify.bind(null, receivedMsg, PDU_SENDER_0, pid))

() => verify(receivedMsg, PDU_SENDER_0, pid)

@@ +73,1 @@
>                                                PDU_SENDER_1, pid));

() => verifyNotReplaced(receivedMsg, PDU_SENDER_1, pid)

Comment 11

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

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #9)
> I'd like to run this test in chrome process for more flexible content
> creation of MMS notification that could also be used in bug 981077.

That's not really a big problem.  Your previous revision is better for me.

Comment 12

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

Comment on attachment 8396172 [details] [diff] [review]
Patch Part3 v2: Add Test case to verify RadioDisabledError. r=vyang.

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

Per offline discuss, routing mms notification indication through emulator console will always get an binary message that contains a correct ICCID.  Unless we supports changing ICCID at runtime, this is really a big problem for testing bug 981077.  So, thank you, Bevis.

::: dom/mobilemessage/tests/marionette/test_error_of_mms_manual_retrieval.js
@@ +116,5 @@
> +    Services.prefs.setBoolPref("ril.radio.disabled", aDisabled);
> +};
> +
> +Promise.resolve()
> +  .then(() => testRetrieve(Ci.nsIMobileMessageCallback.RADIO_DISABLED_ERROR,

You may just have:

  testRetrieve(...)
    .then(...);

@@ +118,5 @@
> +
> +Promise.resolve()
> +  .then(() => testRetrieve(Ci.nsIMobileMessageCallback.RADIO_DISABLED_ERROR,
> +        setRadioDisabled.bind(null, true),
> +        setRadioDisabled.bind(null, false)))

nit: align to |Ci.foo|

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part3 v3: Add Test case to verify RadioDisabledError. r=vyang.

address comment 11 and comment 12.

Comment 14

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

Comment on attachment 8396960 [details] [diff] [review]
Patch Part3 v3: Add Test case to verify RadioDisabledError. r=vyang.

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

Thank you :)

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

try server result is green:
https://tbpl.mozilla.org/?tree=Try&rev=981e7f548e3b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f12296b754c7
https://hg.mozilla.org/integration/b2g-inbound/rev/7d665a2dab28
https://hg.mozilla.org/integration/b2g-inbound/rev/d7f03eb86cdb

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f12296b754c7
https://hg.mozilla.org/mozilla-central/rev/7d665a2dab28
https://hg.mozilla.org/mozilla-central/rev/d7f03eb86cdb

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Julien,

Please be informed to have the corresponding warning message to be displayed for 
|retrieving| when airplane mode is on. :)

Comment 19

[Julien Wajsberg [:julienw]]

Filed bug 990021