928821 - B2G MMS: put deliveryStatus[] into a more generic structure MmsDeliveryInfo[]

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

+++ This bug was initially created as a clone of Bug #921918 +++

This is a pre-condition of bug 921918. We're in agreement with W3C to put the deliveryStatus[] into a more generic structure, which will help the followers easily add other attributes like readStatus or deliveryTimestamp in the future. The proposal change will be:

  partial interface nsIDOMMozMmsMessage
  {
    ...
    readonly attribute jsval receivers;    // DOMString[]
    ...
    readonly attribute jsval deliveryInfo; // MmsDeliveryInfo[] (new)
    ...
  };

  dictionary MmsDeliveryInfo
  {
    DOMString                    receiver;
    readonly attribute DOMString deliveryStatus;
  };

This will break the compatibility with Gaia. I think skao might take both. ;)

Comment 1

[Shao Hang Kao [:skao]]

should I create another bug for Gaia part or just do it here with seperated patch?

Comment 2

[Gene Lian (I already quit Mozilla)]

Either works for me. It's up to you. :)

Comment 3

[Julien Wajsberg [:julienw]]

I think it's better to keep them in the same bug. (before I thought otherwise, but I changed my mind ;) ). It's easier when you need to backout ;)

Comment 4

[Shao Hang Kao [:skao]]

Attachment: Gaia part (link to PR)

Comment 5

[Shao Hang Kao [:skao]]

Attachment: Gaia part (link to PR)

Comment 6

[Shao Hang Kao [:skao]]

Attachment: Gaia part (link to PR)

Comment 7

[Shao Hang Kao [:skao]]

Attachment: Gecko Part

Comment 8

[Shao Hang Kao [:skao]]

Attachment: Gecko Part

Gene, could you suggest a reviewer for Gaia part? Thanks!

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 821450 [details] [review]
Gaia part (link to PR)

Hi Shao Hang, since you changed the message delivery status structure, it's necessary to update gaia unit test and desktop test as well. Please feel free to ask question about the tests if you face any problem, or I can help with gaia patch if you have no cycle for that, thanks.

Comment 10

[Shao Hang Kao [:skao]]

Hi Steve,

I see, will work on it.

Could you guide me how to run gaia unit test & desktop test?

I only found this document to run gaia unit test on desktop, is it enough?
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

Thanks!

Comment 11

[Steve Chung [:steveck]]

Yes, this setup guide should be sufficient for running the gaia unit test. You have to make sure all the cases in thread_ui_test pass at least.
Another guide about running gaia in firefox is here : https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_Gaia_in_Firefox. You can change the url (s/system/sms) to run message app directly. Make sure the delivery status test thread could be opened and displayed correctly. Please contact me directly if you face any problem while running these tests, thanks.

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 821461 [details] [diff] [review]
Gecko Part

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

Overall, it looks nice. I'll take another look about the assigning logic.

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +88,5 @@
>      }
>      mAttachments.AppendElement(att);
>    }
> +
> +  len = aData.deliveryInfo().Length();

uint32_t length?

@@ +433,4 @@
>    for (uint32_t i = 0; i < length; ++i) {
> +    const MmsDeliveryInfo &info = mDeliveryInfo[i];
> +
> +    JS::Rooted<JSObject*> infoObj(aCx, JS_NewObject(

s/infoObj/infoJsObj/

@@ +433,5 @@
>    for (uint32_t i = 0; i < length; ++i) {
> +    const MmsDeliveryInfo &info = mDeliveryInfo[i];
> +
> +    JS::Rooted<JSObject*> infoObj(aCx, JS_NewObject(
> +      aCx, nullptr, nullptr, nullptr));

JS::Rooted<JSObject*> infoJsObj(
  aCx, JS_NewObject(aCx, nullptr, nullptr, nullptr));

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1271,5 @@
>                                                                        retrievalMode) {
>      intermediate.type = "mms";
>      intermediate.delivery = DELIVERY_NOT_DOWNLOADED;
> +    intermediate.deliveryInfo =
> +      [{receiver:null, deliveryStatus:DELIVERY_STATUS_NOT_APPLICABLE}];

intermediate.deliveryInfo = [{ receiver: null,
                               deliveryStatus: DELIVERY_STATUS_NOT_APPLICABLE}];

@@ +1276,5 @@
>  
>      switch(retrievalMode) {
>        case RETRIEVAL_MODE_MANUAL:
> +        intermediate.deliveryInfo[0].deliveryStatus = 
> +          [DELIVERY_STATUS_MANUAL];

This is wrong. It shouldn't be an array?

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +19,4 @@
>  const RIL_GETTHREADSCURSOR_CID =
>    Components.ID("{95ee7c3e-d6f2-4ec4-ade5-0c453c036d35}");
>  
> +const DEBUG = true;

No this when landing.

@@ +350,5 @@
>          }
>  
>          // Set delivery status to error.
> +        if (messageRecord.deliveryInfo.length == 1 &&
> +            messageRecord.deliveryInfo[0].deliveryStatus == 

TS.

@@ +1247,5 @@
>              }
>            } else if (messageRecord.type == "mms") {
>              if (!receiver) {
> +              for (let i = 0; i < messageRecord.deliveryInfo.length; i++) {
> +                if (messageRecord.deliveryInfo[i].deliveryStatus != 

TS.

@@ +1299,5 @@
>  
>                  found = true;
> +                let infoFound = false;
> +                for (let j = 0; j < messageRecord.deliveryInfo.length; j++) {
> +                  if (messageRecord.deliveryInfo[j].receiver == 

TS.

@@ +1301,5 @@
> +                let infoFound = false;
> +                for (let j = 0; j < messageRecord.deliveryInfo.length; j++) {
> +                  if (messageRecord.deliveryInfo[j].receiver == 
> +                        messageRecord.receivers[i]) {
> +                    if (messageRecord.deliveryInfo[j].deliveryStatus != 

TS.

@@ +1303,5 @@
> +                  if (messageRecord.deliveryInfo[j].receiver == 
> +                        messageRecord.receivers[i]) {
> +                    if (messageRecord.deliveryInfo[j].deliveryStatus != 
> +                          deliveryStatus) {
> +                      messageRecord.deliveryInfo[j].deliveryStatus = 

TS.

@@ +1313,5 @@
> +                }
> +                if (!infoFound) {
> +                  messageRecord.deliveryInfo.push({
> +                    receiver:messageRecord.receivers[i],
> +                    deliveryStatus:deliveryStatus});

Put spaces after ":".

@@ +1464,3 @@
>        for (let i = 0; i < receivers.length; i++) {
> +        aMessage.deliveryInfo.push({
> +          receiver: receivers[i], deliveryStatus:deliveryStatus});

Put spaces after ":".

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 821461 [details] [diff] [review]
Gecko Part

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

You have to update the DB records while booting up to restructure what we've saved in the DB. Please see upgradeSchemaX().

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +88,5 @@
>      }
>      mAttachments.AppendElement(att);
>    }
> +
> +  len = aData.deliveryInfo().Length();

How can this work?

@@ +92,5 @@
> +  len = aData.deliveryInfo().Length();
> +  mDeliveryInfo.SetCapacity(len);
> +  for (uint32_t i = 0; i < len; i++) {
> +    MmsDeliveryInfo info;
> +    const MmsDeliveryInfoData &element = aData.deliveryInfo()[i];

s/element/infoData

@@ -175,3 @@
>    for (uint32_t i = 0; i < length; ++i) {
> -    if (!JS_GetElement(aCx, deliveryStatusObj, i, &statusJsVal) ||
> -        !statusJsVal.isString()) {

Can you do some checks like .isObject()?

@@ +311,5 @@
>    aData.expiryDate() = mExpiryDate;
>  
> +  aData.deliveryInfo().SetCapacity(mDeliveryInfo.Length());
> +  for (uint32_t i = 0; i < mDeliveryInfo.Length(); i++) {
> +    MmsDeliveryInfoData info;

s/info/infoData

@@ +312,5 @@
>  
> +  aData.deliveryInfo().SetCapacity(mDeliveryInfo.Length());
> +  for (uint32_t i = 0; i < mDeliveryInfo.Length(); i++) {
> +    MmsDeliveryInfoData info;
> +    const MmsDeliveryInfo &element = mDeliveryInfo[i];

s/element/info

@@ +439,5 @@
> +
> +    JS::Rooted<JS::Value> tmpJsVal(aCx);
> +    JSString* tmpJsStr;
> +
> +    // Get |info.mReceiver|.

This comment is wrong.

info.receiver

@@ +454,2 @@
>  
> +    // Get |attachment.mDeliveryStatus|.

This comment is wrong.

info.deliveryStatus

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1276,4 @@
>  
>      switch(retrievalMode) {
>        case RETRIEVAL_MODE_MANUAL:
> +        intermediate.deliveryInfo[0].deliveryStatus = 

nit: TS.

@@ +1276,5 @@
>  
>      switch(retrievalMode) {
>        case RETRIEVAL_MODE_MANUAL:
> +        intermediate.deliveryInfo[0].deliveryStatus = 
> +          [DELIVERY_STATUS_MANUAL];

OK. This is correct...

@@ +1281,3 @@
>          break;
>        case RETRIEVAL_MODE_NEVER:
> +        intermediate.deliveryInfo[0].deliveryStatus = 

nit: TS.

@@ +1285,3 @@
>          break;
>        case RETRIEVAL_MODE_AUTOMATIC:
> +        intermediate.deliveryInfo[0].deliveryStatus = 

nit: TS.

@@ +1289,4 @@
>          break;
>        case RETRIEVAL_MODE_AUTOMATIC_HOME:
>          if (gMmsConnection.isVoiceRoaming()) {
> +          intermediate.deliveryInfo[0].deliveryStatus = 

nit: TS.

@@ +1294,2 @@
>          } else {
> +          intermediate.deliveryInfo[0].deliveryStatus = 

nit: TS.

@@ +1329,4 @@
>        savable.sender = "anonymous";
>      }
>      savable.receivers = [];
> +    savable.deliveryInfo = [];

Move this line to below until we need it.

@@ +1337,5 @@
>            for (let index in intermediate.headers[type]) {
> +            let receiver = intermediate.headers[type][index].address;
> +            savable.receivers.push(receiver);
> +            savable.deliveryInfo.push({
> +              receiver: receiver, deliveryStatus: DELIVERY_STATUS_SUCCESS});

This is wrong. Please see below.

@@ +1343,5 @@
>          } else {
> +          let receiver = intermediate.headers[type].address;
> +          savable.receivers.push(receiver);
> +          savable.deliveryInfo.push({
> +            receiver: receiver, deliveryStatus: DELIVERY_STATUS_SUCCESS});

This is wrong. Please see below.

@@ -1335,5 @@
>        }
>      }
>  
>      savable.delivery = DELIVERY_RECEIVED;
> -    savable.deliveryStatus = [DELIVERY_STATUS_SUCCESS];

We should keep this as single element array. Please use getPhoneNumber() to assign the receiver. As a receiver, we don't need to care about the delivery status of others.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +322,5 @@
>            messageRecord.deliveryStatus = DELIVERY_STATUS_ERROR;
>          } else {
>            // Set delivery status to error.
> +          for (let i = 0; i < messageRecord.deliveryInfo.length; i++) {
> +            messageRecord.deliveryInfo[i].deliveryStatus = DELIVERY_STATUS_ERROR;

You have to update DB schema for this first.

@@ +1298,4 @@
>                  }
>  
>                  found = true;
> +                let infoFound = false;

You don't need this. Please see below.

@@ +1299,5 @@
>  
>                  found = true;
> +                let infoFound = false;
> +                for (let j = 0; j < messageRecord.deliveryInfo.length; j++) {
> +                  if (messageRecord.deliveryInfo[j].receiver == 

Please use:

if (messageRecord.deliveryInfo[j].receiver != messageRecord.receivers[i]) {
  continue;
}

and early return.

@@ +1313,5 @@
> +                }
> +                if (!infoFound) {
> +                  messageRecord.deliveryInfo.push({
> +                    receiver:messageRecord.receivers[i],
> +                    deliveryStatus:deliveryStatus});

Why do you need this logic? I don't get it.

Comment 14

[Gene Lian (I already quit Mozilla)]

Please see comment #12 and comment #13 for the reviews.

Comment 15

[Shao Hang Kao [:skao]]

Attachment: bug928821.patch

Comment 16

[Shao Hang Kao [:skao]]

Comment on attachment 821450 [details] [review]
Gaia part (link to PR)

Hi Steve,

I've updated the patch to include changes for unit tests & desktop mocks. But I haven't successfully run the tests on my enviroment, could you help to verify it?

Thanks!

Comment 17

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823835 [details] [diff] [review]
bug928821.patch

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

Tentatively giving review- since the logic of updating schema is a bit wrong. Close to be done!

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1271,5 @@
>                                                                        retrievalMode) {
>      intermediate.type = "mms";
>      intermediate.delivery = DELIVERY_NOT_DOWNLOADED;
> +    intermediate.deliveryInfo = [{receiver:null,
> +      deliveryStatus:DELIVERY_STATUS_NOT_APPLICABLE}];

I'd prefer the following alignment:

// As a receiver, we don't need to care about the delivery status of others.
let deliveryInfo = intermediate.deliveryInfo = [{
  receiver: this.getPhoneNumber(),
  deliveryStatus: DELIVERY_STATUS_NOT_APPLICABLE }];

and use getPhoneNumber() for the receiver.

@@ +1276,5 @@
>  
>      switch(retrievalMode) {
>        case RETRIEVAL_MODE_MANUAL:
> +        intermediate.deliveryInfo[0].deliveryStatus =
> +          DELIVERY_STATUS_MANUAL;

s/intermediate.deliveryInfo[0]/deliveryInfo[0] and don't wrap the line.

@@ +1281,4 @@
>          break;
>        case RETRIEVAL_MODE_NEVER:
> +        intermediate.deliveryInfo[0].deliveryStatus =
> +          DELIVERY_STATUS_REJECTED;

Ditto.

@@ +1285,4 @@
>          break;
>        case RETRIEVAL_MODE_AUTOMATIC:
> +        intermediate.deliveryInfo[0].deliveryStatus =
> +          DELIVERY_STATUS_PENDING;

Ditto.

@@ +1289,5 @@
>          break;
>        case RETRIEVAL_MODE_AUTOMATIC_HOME:
>          if (gMmsConnection.isVoiceRoaming()) {
> +          intermediate.deliveryInfo[0].deliveryStatus =
> +            DELIVERY_STATUS_MANUAL;

Ditto.

@@ +1294,3 @@
>          } else {
> +          intermediate.deliveryInfo[0].deliveryStatus =
> +            DELIVERY_STATUS_PENDING;

Ditto.

@@ +1329,4 @@
>        savable.sender = "anonymous";
>      }
>      savable.receivers = [];
> +    savable.deliveryInfo = [];

Move this line further below until needed.

@@ +1345,5 @@
>  
>      savable.delivery = DELIVERY_RECEIVED;
> +    // As a receiver, we don't need to care about the delivery status of others.
> +    savable.deliveryInfo[0] = {receiver: this.getPhoneNumber(),
> +      deliveryStatus: DELIVERY_STATUS_SUCCESS};

savable.deliveryInfo = [{
  receiver: this.getPhoneNumber(),
  deliveryStatus: DELIVERY_STATUS_SUCCESS }];

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +218,4 @@
>              self.upgradeSchema11(event.target.transaction, next);
>              break;
>            case 12:
> +            if (DEBUG) debug("Upgrade to version 13. Replaced deliveryStatus by deliberyInfo");

s/deliberyInfo/deliveryInfo/

@@ +354,3 @@
>          }
>  
>          // Set delivery status to error.

let deliveryInfo = messageRecord.deliveryInfo;

@@ +355,5 @@
>  
>          // Set delivery status to error.
> +        if (messageRecord.deliveryInfo.length == 1 &&
> +            messageRecord.deliveryInfo[0].deliveryStatus ==
> +              DELIVERY_STATUS_PENDING) {

s/messageRecord.deliveryInfo/deliveryInfo and don't need to wrap the line.

@@ +356,5 @@
>          // Set delivery status to error.
> +        if (messageRecord.deliveryInfo.length == 1 &&
> +            messageRecord.deliveryInfo[0].deliveryStatus ==
> +              DELIVERY_STATUS_PENDING) {
> +          messageRecord.deliveryInfo[0].deliveryStatus = DELIVERY_STATUS_ERROR;

Ditto.

@@ +800,5 @@
>  
> +  /**
> +   * Replace deliveryStatus by deliveryInfo.
> +   */
> +  upgradeSchema12: function upgradeSchema11(transaction, next) {

s/upgradeSchema11/upgradeSchema12

@@ +814,5 @@
> +      let messageRecord = cursor.value;
> +      if (messageRecord.type == "mms") {
> +        messageRecord.deliveryInfo = [];
> +
> +        for each (let idx in messageRecord.receivers) {

This is wrong.

|for each| will get the value. Should use:

for (let i = 0; i < messageRecord.receivers.length' i++) {
  ...
}

@@ +1280,3 @@
>              }
>            } else if (messageRecord.type == "mms") {
>              if (!receiver) {

let deliveryInfo = messageRecord.deliveryInfo;

@@ +1282,5 @@
>              if (!receiver) {
> +              for (let i = 0; i < messageRecord.deliveryInfo.length; i++) {
> +                if (messageRecord.deliveryInfo[i].deliveryStatus !=
> +                      deliveryStatus) {
> +                  messageRecord.deliveryInfo[i].deliveryStatus = deliveryStatus;

s/messageRecord.deliveryInfo/deliveryInfo for all and don't need to wrap the line.

@@ +1331,4 @@
>                  }
>  
>                  found = true;
> +                for (let j = 0; j < messageRecord.deliveryInfo.length; j++) {

Ditto and don't need to wrap lines.

@@ +1490,3 @@
>        for (let i = 0; i < receivers.length; i++) {
> +        aMessage.deliveryInfo.push({
> +          receiver: receivers[i], deliveryStatus: deliveryStatus});

Add one space before "}".

Comment 18

[Shao Hang Kao [:skao]]

Attachment: Gecko Part

Comment 19

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823885 [details] [diff] [review]
Gecko Part

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

Nice! Please fix the following nits and add r=gene in the commit message. Thanks!

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1271,5 @@
>                                                                        retrievalMode) {
>      intermediate.type = "mms";
>      intermediate.delivery = DELIVERY_NOT_DOWNLOADED;
> +    // As a receiver, we don't need to care about the delivery status of others.
> +    let deliveryInfo = [{

Please use my previous suggestion. :)

let deliveryInfo = intermediate.deliveryInfo = ...

This is save because JS is copy by reference.

@@ +1273,5 @@
>      intermediate.delivery = DELIVERY_NOT_DOWNLOADED;
> +    // As a receiver, we don't need to care about the delivery status of others.
> +    let deliveryInfo = [{
> +      receiver: this.getPhoneNumber(),
> +      deliveryStatus: DELIVERY_STATUS_NOT_APPLICABLE}];

Add one space before "}".

@@ +1294,5 @@
>          }
>          break;
>      }
>  
> +    intermediate.deliveryInfo = deliveryInfo;

Drop this line.

@@ +1343,5 @@
>      savable.delivery = DELIVERY_RECEIVED;
> +    // As a receiver, we don't need to care about the delivery status of others.
> +    savable.deliveryInfo = [{
> +      receiver: this.getPhoneNumber(),
> +      deliveryStatus: DELIVERY_STATUS_SUCCESS}];

Add one space before "}".

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +817,5 @@
> +
> +        for (let i = 0; i < messageRecord.length; i++) {
> +          messageRecord.deliveryInfo.push({
> +            receiver: messageRecord.receivers[i],
> +            deliveryStatus: messageRecord.deliveryStatus[i]});

Add one space before "}".

@@ +1282,5 @@
>              if (!receiver) {
> +              let deliveryInfo = messageRecord.deliveryInfo;
> +              for (let i = 0; i < deliveryInfo.length; i++) {
> +                if (deliveryInfo[i].deliveryStatus !=
> +                      deliveryStatus) {

Don't need to wrap this line.

@@ +1332,4 @@
>                  }
>  
>                  found = true;
> +                for (let j = 0; j < messageRecord.deliveryInfo.length; j++) {

This same.

let deliveryInfo = messageRecord.deliveryInfo;

and 

s/messageRecord.deliveryInfo/deliveryInfo/

@@ +1333,5 @@
>  
>                  found = true;
> +                for (let j = 0; j < messageRecord.deliveryInfo.length; j++) {
> +                  if (messageRecord.deliveryInfo[j].receiver !=
> +                        messageRecord.receivers[i]) {

Don't need to wrap this line since it's shorter now.

@@ +1337,5 @@
> +                        messageRecord.receivers[i]) {
> +                    continue;
> +                  }
> +                  if (messageRecord.deliveryInfo[j].deliveryStatus !=
> +                        deliveryStatus) {

Don't need to wrap this line since it's shorter now.

@@ +1339,5 @@
> +                  }
> +                  if (messageRecord.deliveryInfo[j].deliveryStatus !=
> +                        deliveryStatus) {
> +                    messageRecord.deliveryInfo[j].deliveryStatus =
> +                      deliveryStatus;

Don't need to wrap this line since it's shorter now.

@@ +1491,3 @@
>        for (let i = 0; i < receivers.length; i++) {
> +        aMessage.deliveryInfo.push({
> +          receiver: receivers[i], deliveryStatus: deliveryStatus});

Add one space before "}".

Comment 20

[Gene Lian (I already quit Mozilla)]

You probably need to rebase on Bug 922580 which is under way to land. ;)

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823885 [details] [diff] [review]
Gecko Part

Hi Jonas,

Since I'm taking the reviewer for this bug. Technically, we need another superreviewer to take the DOM IDL review. Do you have a chance to take a look? Or just let me be the superreviewer at the same time?

Note that this change follows our conclusions on the WebAPI mailing list and W3C is doing exactly the same thing [1], which is for sure to go. ;)

[1] http://messaging.sysapps.org/#idl-def-MmsMessage

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's ok if you do review of both the API and the implementation. Please do note though that super-review is actually different from API review. Read more about super-review here: http://www.mozilla.org/hacking/reviewers.html

Comment 23

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823885 [details] [diff] [review]
Gecko Part

sr=gene r=gene

Comment 24

[Shao Hang Kao [:skao]]

Attachment: Gecko Part (ready for checkin)

waiting for try server results

Comment 25

[Shao Hang Kao [:skao]]

try server result: https://tbpl.mozilla.org/?tree=Try&rev=93c9e9d6ff09

Comment 26

[Gene Lian (I already quit Mozilla)]

Comment on attachment 824437 [details] [diff] [review]
Gecko Part (ready for checkin)

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +814,5 @@
> +      let messageRecord = cursor.value;
> +      if (messageRecord.type == "mms") {
> +        messageRecord.deliveryInfo = [];
> +
> +        for (let i = 0; i < messageRecord.length; i++) {

This is wrong...

s/messageRecord/messageRecord.deliveryStatus

Comment 27

[Steve Chung [:steveck]]

Comment on attachment 821450 [details] [review]
Gaia part (link to PR)

The gaia patch LGTM except for the missing brace and it will cause test faied. Please update the patch and we can land both patches as close as possible.

Comment 28

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #26)
> Comment on attachment 824437 [details] [diff] [review]
> Gecko Part (ready for checkin)
> 
> Review of attachment 824437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
> @@ +814,5 @@
> > +      let messageRecord = cursor.value;
> > +      if (messageRecord.type == "mms") {
> > +        messageRecord.deliveryInfo = [];
> > +
> > +        for (let i = 0; i < messageRecord.length; i++) {
> 
> This is wrong...
> 
> s/messageRecord/messageRecord.deliveryStatus

Hi Skao, please do:

1. Refalsh the Gecko to the previous DB version.
2. Send MMS to (self + others) to have a couple of MMS records with multiple receivers.
3. Update the DB (your codes) and dump some logs to see if the updating logic is right. ;)

Comment 29

[Shao Hang Kao [:skao]]

(In reply to Steve Chung [:steveck] from comment #27)
> Comment on attachment 821450 [details] [review]
> Gaia part (link to PR)
> 
> The gaia patch LGTM except for the missing brace and it will cause test
> faied. Please update the patch and we can land both patches as close as
> possible.

fixed & updated, thanks!

Comment 30

[Shao Hang Kao [:skao]]

Attachment: Gecko Part (ready for checkin)

Gene,

Just tested the upgrading, sorry for the mistake.
Could you have a galance again and help to checkin this?

Thanks!

Comment 31

[Shao Hang Kao [:skao]]

Attachment: Gecko Part (ready for checkin)

Gene,

Updated upgradeSchema12, you may want to have a look again, thanks!

Comment 32

[Shao Hang Kao [:skao]]

Attachment: Gecko Patch (ready for checkin)

Comment 33

[Gene Lian (I already quit Mozilla)]

Comment on attachment 825166 [details] [diff] [review]
Gecko Patch (ready for checkin)

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

Nice!

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +815,5 @@
> +      if (messageRecord.type == "mms") {
> +        messageRecord.deliveryInfo = [];
> +
> +        if (messageRecord.delivery == DELIVERY_NOT_DOWNLOADED ||
> +            messageRecord.delivery == DELIVERY_RECEIVED) {

Let's do one more protection:

if (messageRecord.deliveryStatus.length == 1 &&
    (messageRecord.delivery == DELIVERY_NOT_DOWNLOADED ||
     messageRecord.delivery == DELIVERY_RECEIVED)) {

Comment 34

[Steve Chung [:steveck]]

Comment on attachment 821450 [details] [review]
Gaia part (link to PR)

Patch looks good to me now, thanks!

Comment 35

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/95b86d60f8f7

Comment 36

[Gene Lian (I already quit Mozilla)]

Gecko patch is just landed. Need someone's help land the Gaia patch.

Comment 37

[Julien Wajsberg [:julienw]]

I think we should land Gaia when the gecko patch hits central, right ?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Probably not a bad idea due to the m-c closure at the moment (don't know when b-i will merge over next). Under normal circumstances, it should generally be OK to merge PRs once we're sure the Gecko side is going to stick on b-i, though. The tests that run on TBPL make use of gaia.json, so we shouldn't run into issues in automation with a newer Gaia running on an older Gecko. But I don't want risk real devices hitting that scenario right now, though :)

Comment 39

[Julien Wajsberg [:julienw]]

We need to find a better way to handle this in the future...

Comment 40

[Ryan VanderMeulen [:RyanVM]]

I would say that in general, merging the PR after tests for the Gecko push to b-i are green is perfectly fine. We merge b-i to m-c often enough that it's unlikely a nightly will pick up the Gaia change without the Gecko change. I'm just worried that under the current circumstances, that possibility is higher.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95b86d60f8f7

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/ac8ad66ea6c603eb52ecd8329d4ba494ed254560

Comment 43

[Julien Wajsberg [:julienw]]

Thanks for merging Gaia, Ryan!

Comment 44

[Andrew Overholt [:overholt]]

This landed before gecko 28 branched so will be in 1.3.