This bug blocks leo+ bug(862311). So nominate to leo+.

Triage 4/29 - leo+ as part of required MMS feature.

Comment on attachment 742254 [details] [diff] [review] Part 2/2: Patch v1.0 Review of attachment 742254 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1184,5 @@ > let retrievalMode = RETRIEVAL_MODE_MANUAL; > try { > retrievalMode = Services.prefs.getCharPref(PREF_RETRIEVAL_MODE); > } catch (e) {} > + debug("retrievalMode = " + retrievalMode); nit: debug code ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +626,5 @@ > > + upgradeSchema10: function upgradeSchema10(transaction) { > + let threadStore = transaction.objectStore(THREAD_STORE_NAME); > + > + // Populate threadStore. "Add 'lastMessageType' to each thread record."

Changed according to comment 6. (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #8) > Created attachment 743131 [details] [diff] [review] > Part 2/2: Patch v1.2

Comment on attachment 742258 [details] [diff] [review] Part 1/2: Interface v1.1 We need to change the DOM interface nsIDOMMozMobileMessageThread, because we need a variable to tell the last message is sms or mms. Those changes need a superreview for DOM api change.

Thanks for letting us know, we will incorporate this change in commercial RIL shortly.

Comment on attachment 743131 [details] [diff] [review] Part 2/2: Patch v1.2 Review of attachment 743131 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +631,5 @@ > + threadStore.openCursor().onsuccess = function(event) { > + let cursor = event.target.result; > + if (!cursor) { > + return; > + } nit: add a new line. @@ +994,5 @@ > } > > + if (aMessageRecord.type !== threadRecord.lastMessageType) { > + threadRecord.lastMessageType = aMessageRecord.type; > + needsUpdate = true; No. Please move this |lastMessageType| assignment into |if (threadRecord.lastTimestamp <= timestamp) {| block. We don't update thread record unless the adding message becomes the last one in the thread.

Comment on attachment 742258 [details] [diff] [review] Part 1/2: Interface v1.1 Review of attachment 742258 [details] [diff] [review]: ----------------------------------------------------------------- It's not clear to me how this is going to make things in bug 862311 better. The idea is to show an attachment icon when the message has an attachment but MMS don't always have an attachment in the user perspective.

Add new line and move |lastMessageType| assignment into |if (threadRecord.lastTimestamp <= timestamp) {| block. (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #14) > Created attachment 744154 [details] [diff] [review] > Part 2/2: Patch v1.3

(In reply to Mounir Lamouri (:mounir) from comment #13) > Comment on attachment 742258 [details] [diff] [review] > Part 1/2: Interface v1.1 > > Review of attachment 742258 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's not clear to me how this is going to make things in bug 862311 better. > The idea is to show an attachment icon when the message has an attachment > but MMS don't always have an attachment in the user perspective. Yes, it's not ideal, but that's what we're shipping in 1.1. All the product stakeholders and UX have agreed to this, so it's not a valid reason to r- this patch. We should try and make things better in 1.2, but paperclip-even-for-multi-chunk-text-messages approach is agreed upon to be enough to ship for 1.1.

Comment on attachment 744154 [details] [diff] [review] Part 2/2: Patch v1.3 Review of attachment 744154 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +987,5 @@ > threadRecord.subject = aMessageRecord.body; > threadRecord.lastMessageId = aMessageRecord.id; > + if (aMessageRecord.type !== threadRecord.lastMessageType) { > + threadRecord.lastMessageType = aMessageRecord.type; > + } nit: you don't really have to compare them first. Just assign it.

(In reply to Dietrich Ayala (:dietrich) from comment #16) > Yes, it's not ideal, but that's what we're shipping in 1.1. All the product > stakeholders and UX have agreed to this, so it's not a valid reason to r- > this patch. I'm not sure why UX and product decisions are not allowed to be discussed: showing an attachment indication in the thread view when the message has actually no attachment seems worse than not showing the indication in the thread view. I do not understand why showing that icon is so important that we are okay to show it even when it should not appear. For the moment, the only answer I got in the other bug is that MMS must have an attachment but if this can be true for sent ones, it isn't for received ones. Why don't we add something that says if the last message has an attachment instead of giving the type of the last message?

Of course they are allowed to be discussed - it was discussed by the MMS devs and the product team and UX team long ago. Unfortunately not everyone can be in every discussion. The important point is let's land this initial basic support and indicator first, and file a follow-up for further changes.

Assign lastMessageType directly. (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #20) > Created attachment 744661 [details] [diff] [review] > Part 2/2: Patch v1.4

I just file a bug 868031 - B2G MMS: Find a better data structure for nsIDOMMozMobileMessageThread. Would you mind to discuss this issue in this bug?

Mounir, the paperclip tells the user that it's an MMS, which may cost money. Yes, it's not awesome. In the future we can come with some other way to indicate it's an SMS-that's-only-MMS-because-it's-too-long, but this should do that purpose for 1.1.

Comment on attachment 742258 [details] [diff] [review] Part 1/2: Interface v1.1 Could you please help to review this patch? Thanks.

Comment on attachment 742258 [details] [diff] [review] Part 1/2: Interface v1.1 Review of attachment 742258 [details] [diff] [review]: ----------------------------------------------------------------- sr- for two reasons: 1. Passing a string that whether provide "sms" or "mms" instead of an enum is a waste of memory and CPU time (in the ipdlh); 2. No one answered my suggestion about having a .hasAttachment boolean instead of a .lastMessageType string. .hasAttachment is the API you actually want, right?

Comment on attachment 745317 [details] [diff] [review] Part 2/2: Patch v1.5 Rebased and change lastMessageType into enum.

Comment on attachment 745316 [details] [diff] [review] Part 1/2: Interface v1.2 Rebased and change lastMessageType into enum.

Vicamo, could you please help to review it again? Because I change the lastMessageType into enum. Thanks. (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #28) > Comment on attachment 745317 [details] [diff] [review] > Part 2/2: Patch v1.5 > > Rebased and change lastMessageType into enum.

Mounir, thanks for your suggestion about having a .hasAttachment boolean instead of a .lastMessageType string. But we don't do that because of it can't be done in a short time. This bug blocks thread view list, and thread view list should be landed ASAP. The reason of why change to hasAttachment will take a lot of time is the design of DB for SMS and MMS. MMS pdu has a structure called multi-parts. Currently we take each part in multi-parts to Blob, even it is a plain-text. So in DB related code, we don't know this message is text only or with attachments. Sure we can know by adding more codes. But it takes time (coding, debuging, reviewing....) and we have the attribute called type in the message record. When saving the message into DB, it also saves thread related information as a thread record. Then the getThread function try to get the thread result to Messaging AP. If we want to support hasAttachment, it also means we need to deal with below situations. 1. MMS message, only has subject. 2. MMS message, has subject and text. 3. MMS message, has subject and text and attachments. 4. MMS message, no subject but has text. 5. MMS message, no subject but has text and attachments. 6. SMS message, have text. We need to find out this MMS message has real attachments(not only one text attachment). And also need to know the subject exist or not. Also the text need to replace the body in thread record. It definitely takes more time to let it happen. Another way is provide the whole message from the nsIDOMMozMobileMessageThread. I mean pass nsIDOMMozMmsMessage through ipc to Messagign AP. It means pass the all blob of last image (it might be an image, a video...) for each thread. I don't think it is a good idea. So could we use lastMessageType in this bug and find a better solution in another bug 868031 I filed? This solution came from Madrid work week. Vicamo, gene and I discussed with UX team(Ayman). We know it is not a perfect solution, but it accepted by UX team. Once this bug is land, then Messaging AP developers can land their thread view bugs in a short time. They already have WIP patches. Please feel free to ping me.Thanks. (In reply to Mounir Lamouri (:mounir) from comment #25) > Comment on attachment 742258 [details] [diff] [review] > Part 1/2: Interface v1.1 > > Review of attachment 742258 [details] [diff] [review]: > ----------------------------------------------------------------- > > sr- for two reasons: > 1. Passing a string that whether provide "sms" or "mms" instead of an enum > is a waste of memory and CPU time (in the ipdlh); > 2. No one answered my suggestion about having a .hasAttachment boolean > instead of a .lastMessageType string. .hasAttachment is the API you actually > want, right?

Comment on attachment 745317 [details] [diff] [review] Part 2/2: Patch v1.5 Review of attachment 745317 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/Constants.h @@ +40,5 @@ > #define MESSAGE_CLASS_CLASS_2 NS_LITERAL_STRING("class-2") > #define MESSAGE_CLASS_CLASS_3 NS_LITERAL_STRING("class-3") > > +#define MESSAGE_SMS NS_LITERAL_STRING("sms") > +#define MESSAGE_MMS NS_LITERAL_STRING("mms") MESSAGE_TYPE_foo please. ::: dom/mobilemessage/src/MobileMessageThread.cpp @@ +187,5 @@ > + case eDeliveryState_Unknown: > + case eDeliveryState_EndGuard: > + default: > + MOZ_NOT_REACHED("We shouldn't get any other delivery state!"); > + return NS_ERROR_UNEXPECTED; nit: alignment.

Revised according to comment 32.

Comment on attachment 745316 [details] [diff] [review] Part 1/2: Interface v1.2 Review of attachment 745316 [details] [diff] [review]: ----------------------------------------------------------------- I wasn't clear with .hasAttachment. I meant that this attribute could be implemented exactly like you are implementing the .lastMessageType but you could improve the implementation later. However, I feel like I might be wasting everybody's time here so feel free to land this as-is.

Mounir, thanks for your suggestion. We will change the name if we are implementing it as having attachment. We need more discussion for this implementation.

https://hg.mozilla.org/projects/birch/rev/76107e8b0df9 https://hg.mozilla.org/projects/birch/rev/db79b5ea167b

Pushed the interdiff between v1.6 and v1.7 to fix the Werror bustage. https://hg.mozilla.org/projects/birch/rev/9ba22af0a840

*sigh* https://hg.mozilla.org/projects/birch/rev/415c369420a9

Thanks! (In reply to Ryan VanderMeulen [:RyanVM] from comment #39) > *sigh* > https://hg.mozilla.org/projects/birch/rev/415c369420a9

https://hg.mozilla.org/mozilla-central/rev/76107e8b0df9 https://hg.mozilla.org/mozilla-central/rev/415c369420a9

Part 2 needs a b2g18-specific patch.

Ryan, I put the b2g18-specific patch. Thanks.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7aab895899c3 https://hg.mozilla.org/releases/mozilla-b2g18/rev/bba0f27a1c1f

Comment on attachment 746496 [details] [diff] [review] Part 2/2: Patch v1.7 Review of attachment 746496 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +662,5 @@ > + debug("Caught error on transaction", event.target.errorCode); > + } > + } > + }; > + cursor.continue(); One defect here. Please see bug 887701, comment #30.

Hi Ctai, would you mind reopening this bug to come up with a follow-up patch for solving a potential issue at bug 887701, comment #30?

According comment 47, reopen it.

Comment on attachment 778240 [details] [diff] [review] bug-863241-follow-up Review of attachment 778240 [details] [diff] [review]: ----------------------------------------------------------------- Great! Thank you!

Not to be a process stickler, but let's try to avoid reopening bugs, especially ones that are more than 2 months old. Can you file a followup instead and land the patch there?

File a follow-up bug 895735.