810097 - B2G MMS: Retry retrieval on error

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

See also bug 810067 for retrieval mode and bug 810091 for duplicated notification.

Comment 1

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

Attachment: Retry Retrieval

Comment 2

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

Attachment: Retry Retrieval

Comment 3

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

Comment on attachment 718278 [details] [diff] [review]
Retry Retrieval

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

Let's hide retrial in RetrieveTransaction::run(). Have something like:

RetrieveTransaction::run(callback) {
  this.retryCount = 0;
  this.retrieve(function (mmsStatus, msg) {
    if (mmsStatus == MMS.MMS_PDU_STATUS_DEFERRED &&
        this.retryCount < MAX_RETRY_COUNT) {
      this.retryCount++;
      let self = arguments.callee;
      new timer(ONE_SHOT, function () {
        this.retrieve(self);
      });
      return;
    }

    if (callback) {
      callback
    }
  });
}

RetrieveTransaction::retrieve(callback) {
  // original run()
}

Comment 4

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

Attachment: Retry Retrieval

Comment 5

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

Attachment: Retry Retrieval

Comment 6

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

Comment on attachment 718319 [details] [diff] [review]
Retry Retrieval

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

::: dom/mms/src/ril/MmsService.js
@@ +39,5 @@
>  const RETRIEVAL_MODE_AUTOMATIC = "automatic";
>  const RETRIEVAL_MODE_NEVER = "never";
>  
> +const MAX_RETRY_COUNT = 3;
> +const DELAY_TIME_TO_RETRY = 5000;

How about "dom.mms.retrievalRetryCount", "dom.mms.retrievalRetryInterval"?

@@ +504,5 @@
> +    }).bind(this));
> +  },
> +
> +  /**
> +   * @param callback [optional]

That's not optional now.

@@ +511,5 @@
> +   */
> +  retrieve: function retrieve(callback) {
> +    let callbackIfValid = function callbackIfValid(mmsStatus, msg) {
> +      if (callback) {
> +        callback(mmsStatus, msg);

ditto.

Comment 7

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

Attachment: Retry Retrieval

Comment 8

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

Comment on attachment 718326 [details] [diff] [review]
Retry Retrieval

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

Thank you :)

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/931ad73babc6

Comment 10

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

Attachment: Retry for TransientError

Comment 11

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

Attachment: Retry for TransientError

Comment 12

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

Comment on attachment 718836 [details] [diff] [review]
Retry for TransientError

It is not necessary.

Comment 13

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

Comment on attachment 718836 [details] [diff] [review]
Retry for TransientError

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

::: dom/mms/src/ril/MmsService.js
@@ +498,5 @@
> +    this.retryCount = 0;
> +    let that = this;
> +    this.retrieve((function retryCallback(mmsStatus, msg) {
> +      if ((MMS.MMS_PDU_STATUS_DEFERRED == mmsStatus ||
> +          gMmsTransactionHelper.isTransientError(mmsStatus)) &&

We don't need this. It's already a X-Mms-Status. Just check whether does it equals to MMS.MMS_PDU_STATUS_DEFERRED is sufficient. Therefore, we don't need isTransientError(), either.

@@ -818,5 @@
>          debug("retrievedMsg = " + JSON.stringify(retrievedMsg));
> -        if (this.isTransientError(mmsStatus)) {
> -          // TODO: remove this check after bug 810097 is landed.
> -          return;
> -        }

Removing code here should have done, but just upload a follow-up instead.

Comment 14

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

Attachment: Follow-up-810097.patch

Comment 15

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

Two inbound check-ins in total:
https://hg.mozilla.org/integration/mozilla-inbound/rev/931ad73babc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b082c2abd269

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/931ad73babc6
https://hg.mozilla.org/mozilla-central/rev/b082c2abd269

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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