Create a new store "message" with an auto increment key "id", and populate it with all messages from the original "sms" object store. Drop "sms" store on done.

Sorry for the noises. Update interface uuid.

It seems we'll also need a PR to Gaia because it has some prebuilt database binaries for workload tests. See https://github.com/mozilla-b2g/gaia/blob/master/test_media/reference-workload/makeReferenceWorkload.sh#L133

I'll review this tonight or tomorrow early morning. Sorry for the delay.

Comment on attachment 770762 [details] [diff] [review] Gecko patch : v2 Review of attachment 770762 [details] [diff] [review]: ----------------------------------------------------------------- Giving review- because there is a couple of obvious logic errors. Need to correct them. ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl @@ +47,5 @@ > * > * Note: |deliveryStatus| should only contain single string to specify > * the delivery status of MMS message for the phone owner self. > */ > + void saveReceivedMessage(in jsval aMessage, I'm afraid we need to notify the partner about this change. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +724,5 @@ > + autoIncrement: true }); > + > + // Originally created in createSchema(). > + messageStore.createIndex("timestamp", "timestamp", { unique: false }); > + // Originally created in upgradeSchema6(). Please add one blank line above this comment. @@ +728,5 @@ > + // Originally created in upgradeSchema6(). > + messageStore.createIndex("delivery", "deliveryIndex"); > + messageStore.createIndex("number", "numberIndex", { multiEntry: true }); > + messageStore.createIndex("read", "readIndex"); > + // Originally created in upgradeSchema7(). Please add one blank line above this comment. @@ +732,5 @@ > + // Originally created in upgradeSchema7(). > + messageStore.createIndex("threadId", "threadIdIndex"); > + messageStore.createIndex("participantIds", "participantIdsIndex", > + { multiEntry: true }); > + // Originally created in upgradeSchema8(). Please add one blank line above this comment. @@ +735,5 @@ > + { multiEntry: true }); > + // Originally created in upgradeSchema8(). > + messageStore.createIndex("transactionId", "transactionIdIndex", { unique: true }); > + > + let smsStore = transaction.objectStore(SMS_STORE_NAME); Could you please add some comments to describe what you're wanting to do? That is, copy all the records from the old store to the new one and delete the deprecated one. @@ +744,5 @@ > + return; > + } > + > + let messageRecord = cursor.value; > + messageStore.put(aMessageRecord); This is wrong. You'll create new store with nothing. s/aMessageRecord/messageRecord @@ +1026,5 @@ > aAddresses, true, > function (threadRecord, > participantIds) { > if (!participantIds) { > + txn.abort(); Nice catch. Otherwise we'll call notifyResult(Cr.NS_ERROR_FAILURE) twice... @@ +1045,5 @@ > // Really add to message store. > + messageStore.add(aMessageRecord).onsuccess = function (event) { > + let messageId = event.target.result; > + aMessageRecord.id = messageId; > + if (threadRecord.lastMessageId == null) { I don't like this check. Please see below. @@ +1082,5 @@ > + threadRecord.lastMessageId = aMessageRecord.id; > + } else { > + // Clear lastMessageId explicitly and re-assign later in > + // insertMessageRecord(). > + delete threadRecord.lastMessageId; It's a bit tricky to me to use the presence of |threadRecord.lastMessageId| to decide the logic. I'd think using |isOverriding| is already sufficient. @@ +1094,5 @@ > needsUpdate = true; > } > > if (needsUpdate) { > threadStore.put(threadRecord); Something wrong here... It seems we're going to update the threadRecord twice by threadStore.put(threadRecord) and the following insertMessageRecord(threadRecord). @@ +1110,5 @@ > + unreadCount: aMessageRecord.read ? 0 : 1, > + lastMessageType: aMessageRecord.type > + }; > + if (isOverriding) { > + threadRecord.lastMessageId = aMessageRecord.id; Do we really need this check? It's impossible to find no threads when overriding an old record. Right? I think you can directly move this assignment into the above object.

(In reply to Gene Lian [:gene] from comment #6) > Review of attachment 770762 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js > @@ +744,5 @@ > > + return; > > + } > > + > > + let messageRecord = cursor.value; > > + messageStore.put(aMessageRecord); > > This is wrong. You'll create new store with nothing. > > s/aMessageRecord/messageRecord Oops! Thanks for the catch. > @@ +1045,5 @@ > > // Really add to message store. > > + messageStore.add(aMessageRecord).onsuccess = function (event) { > > + let messageId = event.target.result; > > + aMessageRecord.id = messageId; > > + if (threadRecord.lastMessageId == null) { > > I don't like this check. Please see below. > > @@ +1082,5 @@ > > + threadRecord.lastMessageId = aMessageRecord.id; > > + } else { > > + // Clear lastMessageId explicitly and re-assign later in > > + // insertMessageRecord(). > > + delete threadRecord.lastMessageId; > > It's a bit tricky to me to use the presence of |threadRecord.lastMessageId| > to decide the logic. I'd think using |isOverriding| is already sufficient. No, it's not. We update |threadRecord.lastMessageId| when last message changes. "|isOverriding| is true" doesn't follow "last message changes". > @@ +1094,5 @@ > > needsUpdate = true; > > } > > > > if (needsUpdate) { > > threadStore.put(threadRecord); > > Something wrong here... It seems we're going to update the threadRecord > twice by threadStore.put(threadRecord) and the following > insertMessageRecord(threadRecord). It's correct because both threadRecord and messageRecord have auto incremented keys now and we can only know their key after that record is stored into database. So somehow we must update one of them twice: 1) save threadRecord into DB 2) got threadRecord.id, and assign to messageRecord.threadId 3) save messageRecord into DB 4) got messageRecord.id, and assign to threadRecord.lastMessageId 5) update threadRecord with correct lastMessageId I choose updating threadRecord twice because it's a much more tiny object in comparison to messageRecord. > @@ +1110,5 @@ > > + unreadCount: aMessageRecord.read ? 0 : 1, > > + lastMessageType: aMessageRecord.type > > + }; > > + if (isOverriding) { > > + threadRecord.lastMessageId = aMessageRecord.id; > > Do we really need this check? It's impossible to find no threads when > overriding an old record. Right? > I think you can directly move this assignment into the above object. You're right! Will remove the check.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7) > (In reply to Gene Lian [:gene] from comment #6) > > Review of attachment 770762 [details] [diff] [review]: > > ----------------------------------------------------------------- > > ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js > > @@ +1110,5 @@ > > > + unreadCount: aMessageRecord.read ? 0 : 1, > > > + lastMessageType: aMessageRecord.type > > > + }; > > > + if (isOverriding) { > > > + threadRecord.lastMessageId = aMessageRecord.id; > > > > Do we really need this check? It's impossible to find no threads when > > overriding an old record. Right? > > I think you can directly move this assignment into the above object. > > You're right! Will remove the check. Apparently I need some comments to remind myself, too. We may have two different cases here: 1) when we've received/sent a new message to a new contact, so isOverriding === false, 2) got a just retrieved MMS message, which have different participants in M-Notification.ind and M-Retrieve.conf PDUs, so isOverriding === true. So isOverriding is not always true or false here. The if-block cannot be removed.

Address review comments.

Comment on attachment 772664 [details] [diff] [review] Gecko patch : v3 Review of attachment 772664 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +25,5 @@ > > const DB_NAME = "sms"; > +const DB_VERSION = 12; > +const MESSAGE_STORE_NAME = "message"; > +const SMS_STORE_NAME = "sms"; Could you please add a comment after this line? // Deprecated. Use "message" for new message store. @@ +736,5 @@ > + messageStore.createIndex("participantIds", "participantIdsIndex", > + { multiEntry: true }); > + > + // Originally created in upgradeSchema8(). > + messageStore.createIndex("transactionId", "transactionIdIndex", { unique: true }); nit: fold the third arg into the second line. @@ +1062,5 @@ > if (!isOverriding) { > // Really add to message store. > + messageStore.add(aMessageRecord).onsuccess = function (event) { > + let messageId = event.target.result; > + aMessageRecord.id = messageId; Btw, do you really need this assignment? let messageStore = db.createObjectStore(MESSAGE_STORE_NAME, { keyPath: "id", autoIncrement: true }); We've already declare .id in the KeyPath. Right? @@ +1145,2 @@ > let threadId = event.target.result; > + threadRecord.id = threadId; Ditto. Do we need this?

OK. I misunderstood the .id part. The patch is OK.

Address review comment 10.

We need some test cases for database upgrading process before landing this patch.

Please this into backlog.

Firefox OS is not being worked on