Closed Bug 968750 Opened 10 years ago Closed 10 years ago

B2G MMS: MmsDeliveryInfo.readStatus should go into "pending" read status

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0M+, tracking-b2g:+, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 affected, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S9 (21Nov)
blocking-b2g 2.0M+
tracking-b2g +
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: airpingu, Assigned: bevis, Mentored)

References

Details

(Whiteboard: [good first bug][priority])

Attachments

(3 files)

This is a bug. Need to fix it. I already see the root cause. Please let me know if you want to help with this.
Blocks: b2g-mms
Whiteboard: [good first bug][mentor=gene]
When you say "need", do you mean it's necessary for 1.3, 1.4, or a nice to have?
Just had discussion with Steve, he said this won't block the current work on the UI, so it's fine to put it to a lower priority.
blocking-b2g: --- → 1.4?
And this is supposed to be a bug because the read status flow should be symmetric to the delivery status.
For the SMS delivery report case, it relies on the saveSendingMessage() at [1] to save the message before sending. You can see how it sets a flag |deliveryStatusRequested| [2] to decide if it needs to set the delivery status to be "pending" or "not-applicable" in the DB [3]. MMS' delivery report does the same thing at [4].

We have to do the similar thing for MMS' read report. The current logic depends on |requestReadReport| to decide if it needs to request the read report to the receiver, but it doesn't use it to set the read status in the DB, which is the missing part we need to make up.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#4129
[2] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#4103
[3] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MobileMessageDB.jsm#2199
[4] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#2061
[5] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#1091
Assignee: nobody → btseng
There is a newbie who wants to take this as first bug.
Assignee: btseng → nobody
Thomas, would you like to take this bug?
Flags: needinfo?(tzimmermann)
Ok.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Flags: needinfo?(tzimmermann)
Moving this to 1.5? as it is doesn't conform to the de-scoped list of features for 1.4.
blocking-b2g: 1.4? → 1.5?
put to backlog as low priority
blocking-b2g: 2.0? → backlog
Whiteboard: [good first bug][mentor=gene] → [good first bug][mentor=gene] [priority]
Mentor: gene.lian
Whiteboard: [good first bug][mentor=gene] [priority] → [good first bug][priority]
I'd like to take this bug back to improve the UX problem in Message Reports mentioned in bug 1094529 comment 37. :)
Assignee: tzimmermann → btseng
This patch is to 
1. save readStatus of the sending MMS into DB according to the settings.
2. detect the "undefined" DOMString for mmsc/mmsproxy correctly which was not done properly in Bug 1032097 in m-c.

Hi Edgar,

May I have your help to review this change?

Thanks!
Attachment #8522778 - Flags: review?(echen)
Comment on attachment 8522778 [details] [diff] [review]
Patch v1: MmsDeliveryInfo.readStatus saved as "pending" read status when toggled.

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

Looks good to me. Thank you. :)

::: dom/mobilemessage/gonk/MmsService.js
@@ +228,5 @@
>      // Workaround an xpconnect issue with undefined string objects. See bug 808220.
> +    this.mmsc =
> +      (network.mmsc === "undefined") ? undefined : network.mmsc;
> +    this.mmsProxy =
> +      (network.mmsProxy === "undefined") ? undefined : network.mmsProxy;

I had met this kinds of problem before [1]. But this case is a different scenario.
In this case, if the |network.mmsc| returns "undefined", I think we should fix the |network.mmsc| itself, instead of adding protection here.

Please help to file a follow-up for removing "undefined" checks and we probably needs to review the implementation of network.

Thank you.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=935399
Attachment #8522778 - Flags: review?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #13)
> ::: dom/mobilemessage/gonk/MmsService.js
> @@ +228,5 @@
> >      // Workaround an xpconnect issue with undefined string objects. See bug 808220.
> > +    this.mmsc =
> > +      (network.mmsc === "undefined") ? undefined : network.mmsc;
> > +    this.mmsProxy =
> > +      (network.mmsProxy === "undefined") ? undefined : network.mmsProxy;
> 
> I had met this kinds of problem before [1]. But this case is a different
> scenario.
> In this case, if the |network.mmsc| returns "undefined", I think we should
> fix the |network.mmsc| itself, instead of adding protection here.
> 
> Please help to file a follow-up for removing "undefined" checks and we
> probably needs to review the implementation of network.
> 
> Thank you.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=935399

Bug 968750 has been created to address this problem. :)
> Bug 968750 has been created to address this problem. :)
sorry, it shall be bug 1101397. :p
https://hg.mozilla.org/mozilla-central/rev/0a8346276fb9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S9 (21Nov)
Hi Kai-Zhen,
Can you help to land on 2.0M?
Thanks!
Blocks: Woodduck
blocking-b2g: backlog → 2.0M+
Flags: needinfo?(kli)
Uplift into 2.0m got conflicts "unable to find 'dom/mobilemessage/gonk/MmsService.js' for patching".
Flags: needinfo?(btseng)
Hi Kai-Zhen,

Sorry, I wasn't aware that patch for 2.0m is required.
I've rebased patch for v2.0m as attached.
Please give it a trial. :)

Thanks!
Flags: needinfo?(kli)
Attachment #8528815 - Flags: review+
NI per comment 20 for the patch of 2.0m.
Flags: needinfo?(btseng) → needinfo?(kli)
(In reply to Kai-Zhen Li [:seinlin] from comment #22)
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bea61c784d89

Dear Kai-Zhen, 

Is this the final patch?
It's likely it, do you see a problem with this patch?
(In reply to Julien Wajsberg [:julienw] from comment #24)
> It's likely it, do you see a problem with this patch?

I want to confirm, there display "read: requested" below the number after send an MMS, is that by design?
(In reply to ruihua.zhang from comment #25)
> (In reply to Julien Wajsberg [:julienw] from comment #24)
> > It's likely it, do you see a problem with this patch?
> 
> I want to confirm, there display "read: requested" below the number after
> send an MMS, is that by design?

There display "read: requested" below the number of recipients when "View mesage report". Is that by design?
(In reply to ruihua.zhang from comment #26)
> (In reply to ruihua.zhang from comment #25)
> > (In reply to Julien Wajsberg [:julienw] from comment #24)
> > > It's likely it, do you see a problem with this patch?
> > 
> > I want to confirm, there display "read: requested" below the number after
> > send an MMS, is that by design?
> 
> There display "read: requested" below the number of recipients when "View
> mesage report". Is that by design?

It shall be "report: requested" instead, is it your a typo?
Ruihua, please file a separate bug if you need it, as this bug is about a Gecko issue, while you're talking about a UI issue. It will make things clearer :) Thanks!
NI ruihua to clarify comment 27 and comment 28.
Flags: needinfo?(ruihua.zhang.hz)
It's "Read: Requested", not typo. 

"Read: Requested" will changed after the MMS was readed, is that right?
I need to confirm this patch could work, and I use a SIM of Orange doing test. But it can't receive MMS now.
Flags: needinfo?(ruihua.zhang.hz)
(In reply to ruihua.zhang from comment #30)
> Created attachment 8533640 [details]
> mms-read-report-orange.png
> 
> It's "Read: Requested", not typo. 
> 
> "Read: Requested" will changed after the MMS was readed, is that right?
> I need to confirm this patch could work, and I use a SIM of Orange doing
> test. But it can't receive MMS now.

Sorry, I guess you're running v2.0m instead of m-c.
NI Julien to double confirm the string to be displayed in the report in v2.0.

BTW, is the receiving target is also running with FFOS?
If yes, please help to create a new bug with 
1. Steps to reproduce.
2. adb logcat of main and radio logs.
3. tcpdump.
For adb logcat & tcpdump, please refer to https://github.com/bevis-tseng/Debug_Tools to see how to enable the debug flags before capturing the logs.
Flags: needinfo?(ruihua.zhang.hz)
Flags: needinfo?(felash)
Steve will know.
Flags: needinfo?(felash) → needinfo?(schung)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #31)
> (In reply to ruihua.zhang from comment #30)
> > Created attachment 8533640 [details]
> > mms-read-report-orange.png
> > 
> > It's "Read: Requested", not typo. 
> > 
> > "Read: Requested" will changed after the MMS was readed, is that right?
> > I need to confirm this patch could work, and I use a SIM of Orange doing
> > test. But it can't receive MMS now.
> 
> Sorry, I guess you're running v2.0m instead of m-c.
> NI Julien to double confirm the string to be displayed in the report in v2.0.

The snapshot looks like old version. If it's 2.0m, the behavior is correct.
Flags: needinfo?(schung)
bug 1094529. It's 2.0m.

Is this patch for 2.0m ?
Flags: needinfo?(ruihua.zhang.hz)
(In reply to ruihua.zhang from comment #34)
> bug 1094529. It's 2.0m.
> 
> Is this patch for 2.0m ?

Yes, the patch has been uplift to 2.0m as well:
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bea61c784d89
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #35)
> (In reply to ruihua.zhang from comment #34)
> > bug 1094529. It's 2.0m.
> > 
> > Is this patch for 2.0m ?
> 
> Yes, the patch has been uplift to 2.0m as well:
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bea61c784d89

I use the SIM or Orange to do a test, but it still can't receive the MMS. I don't know what will happen after the MMS was read. So, if you had do a test and the patch is work. I will merge it into our codebase. Thanks!
Flags: needinfo?(btseng)
(In reply to ruihua.zhang from comment #36)
> (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #35)
> > (In reply to ruihua.zhang from comment #34)
> > > bug 1094529. It's 2.0m.
> > > 
> > > Is this patch for 2.0m ?
> > 
> > Yes, the patch has been uplift to 2.0m as well:
> > https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bea61c784d89
> 
> I use the SIM or Orange to do a test, but it still can't receive the MMS. I
> don't know what will happen after the MMS was read. So, if you had do a test
> and the patch is work. I will merge it into our codebase. Thanks!

Hi,

This patch has nothing to do to the receiving party, it more about how the read-report request status to be stored into the database and has already been tested in local network.

For other issues, please help to clarify if its network issue instead.
If not, please file another bug with
1. STR and more clear comparison of how other reference phone behaves for both sending/receiving.
2. If the receiving party is also running FFOS, as mentioned in comment 31, please have adb logcat of main/radio logs with debug flags enabled and the tcpdump for further clarification.

Thanks!
Flags: needinfo?(btseng) → needinfo?(ruihua.zhang.hz)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #37)
> (In reply to ruihua.zhang from comment #36)
> > (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #35)
> > > (In reply to ruihua.zhang from comment #34)
> > > > bug 1094529. It's 2.0m.
> > > > 
> > > > Is this patch for 2.0m ?
> > > 
> > > Yes, the patch has been uplift to 2.0m as well:
> > > https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bea61c784d89
> > 
> > I use the SIM or Orange to do a test, but it still can't receive the MMS. I
> > don't know what will happen after the MMS was read. So, if you had do a test
> > and the patch is work. I will merge it into our codebase. Thanks!
> 
> Hi,
> 
> This patch has nothing to do to the receiving party, it more about how the
> read-report request status to be stored into the database and has already
> been tested in local network.

Thank you!

> For other issues, please help to clarify if its network issue instead.
> If not, please file another bug with
> 1. STR and more clear comparison of how other reference phone behaves for
> both sending/receiving.
> 2. If the receiving party is also running FFOS, as mentioned in comment 31,
> please have adb logcat of main/radio logs with debug flags enabled and the
> tcpdump for further clarification.
> 
> Thanks!

I use the SIM of UNICOM send an MMS to self and it can receive the MMS. But the SIM of Orange can't. This should be the network issue.
Flags: needinfo?(ruihua.zhang.hz)
You need to log in before you can comment on or make changes to this bug.