Closed
Bug 968750
Opened 11 years ago
Closed 10 years ago
B2G MMS: MmsDeliveryInfo.readStatus should go into "pending" read status
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:2.0M+, tracking-b2g:+, 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)
(deleted),
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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.
Reporter | ||
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=gene]
Comment 1•11 years ago
|
||
When you say "need", do you mean it's necessary for 1.3, 1.4, or a nice to have?
Reporter | ||
Comment 2•11 years ago
|
||
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?
Reporter | ||
Comment 3•11 years ago
|
||
And this is supposed to be a bug because the read status flow should be symmetric to the delivery status.
Reporter | ||
Comment 4•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → btseng
Reporter | ||
Comment 5•11 years ago
|
||
There is a newbie who wants to take this as first bug.
Assignee | ||
Updated•11 years ago
|
Assignee: btseng → nobody
Comment 7•11 years ago
|
||
Ok.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Flags: needinfo?(tzimmermann)
Comment 8•11 years ago
|
||
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?
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=gene] → [good first bug][mentor=gene] [priority]
Updated•10 years ago
|
Mentor: gene.lian
Whiteboard: [good first bug][mentor=gene] [priority] → [good first bug][priority]
Assignee | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
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)
Updated•10 years ago
|
tracking-b2g:
--- → +
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
(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. :)
Assignee | ||
Comment 15•10 years ago
|
||
update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=5a438f91c247
Keywords: checkin-needed
Assignee | ||
Comment 16•10 years ago
|
||
> Bug 968750 has been created to address this problem. :)
sorry, it shall be bug 1101397. :p
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S9 (21Nov)
Comment 18•10 years ago
|
||
Hi Kai-Zhen,
Can you help to land on 2.0M?
Thanks!
Blocks: Woodduck
blocking-b2g: backlog → 2.0M+
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
Flags: needinfo?(kli)
Comment 19•10 years ago
|
||
Uplift into 2.0m got conflicts "unable to find 'dom/mobilemessage/gonk/MmsService.js' for patching".
Flags: needinfo?(btseng)
Keywords: branch-patch-needed
Assignee | ||
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
NI per comment 20 for the patch of 2.0m.
Flags: needinfo?(btseng) → needinfo?(kli)
Comment 22•10 years ago
|
||
Flags: needinfo?(kli)
Updated•10 years ago
|
Keywords: branch-patch-needed
Comment 23•10 years ago
|
||
(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?
Comment 24•10 years ago
|
||
It's likely it, do you see a problem with this patch?
Comment 25•10 years ago
|
||
(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?
Comment 26•10 years ago
|
||
(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?
Assignee | ||
Comment 27•10 years ago
|
||
(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?
Comment 28•10 years ago
|
||
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!
Assignee | ||
Comment 29•10 years ago
|
||
NI ruihua to clarify comment 27 and comment 28.
Flags: needinfo?(ruihua.zhang.hz)
Comment 30•10 years ago
|
||
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)
Assignee | ||
Comment 31•10 years ago
|
||
(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)
Comment 33•10 years ago
|
||
(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)
Comment 34•10 years ago
|
||
bug 1094529. It's 2.0m.
Is this patch for 2.0m ?
Flags: needinfo?(ruihua.zhang.hz)
Assignee | ||
Comment 35•10 years ago
|
||
(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
Comment 36•10 years ago
|
||
(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)
Assignee | ||
Comment 37•10 years ago
|
||
(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)
Comment 38•10 years ago
|
||
(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.
Description
•