Closed
Bug 854790
Opened 12 years ago
Closed 12 years ago
B2G SMS & MMS: support filtering by thread ID
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
(Keywords: dev-doc-needed)
Attachments
(5 files, 7 obsolete files)
(deleted),
patch
|
vicamo
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
In bug 850127, we introduced threadId to both message types and ThreadListItem for b2g18 branch. In bug 849739, we turned getThreadList() into a DOMCursor-based getThreads(). However, the way to retrieve a list of message of the same thread is still through looking of messages of the same sender or receiver depending on the delivery type.
This method will be a wrong with MMS messages and we need a dedicated way for thread message retrieval. In this bug we add yet another filtering condition 'threadId'. I planed to implement this in bug 847760, but this just doesn't fit its bug description so I closed bug 847760 and open a new one instead.
Assignee | ||
Comment 1•12 years ago
|
||
Nominate for leo? because it's essential to complete MMS thread view. See details in bug 838467 comment 21.
blocking-b2g: --- → leo?
Assignee | ||
Comment 2•12 years ago
|
||
Also depend on bug 850127, which adds threadId to SMS/MMS message types.
Depends on: 850127
Updated•12 years ago
|
blocking-b2g: leo? → leo+
Updated•12 years ago
|
Assignee: nobody → gene.lian
Updated•12 years ago
|
Updated•12 years ago
|
Blocks: message-database
Comment 3•12 years ago
|
||
Hi Vicamo & Jonas,
In this patch, I also need your suggestion about how to represent an invalid thread DB ID? which is going to be passed through IPC. In the DB, we need an invalid value to indicate no need for filtering.
I use |INT32_MIN| for now but I'm not sure if it's proper enough. I'm worrying about the |INT32_MIN| could also be a valid value returned from .add() or .put().
Attachment #731096 -
Flags: superreview?(jonas)
Attachment #731096 -
Flags: review?(vyang)
Comment 4•12 years ago
|
||
Thanks to Vicamo's previous thoughtful design. This part is super easy. ;)
Attachment #731099 -
Flags: review?(vyang)
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 731099 [details] [diff] [review]
Part 2, support filtering for thread ID in DB
Review of attachment 731099 [details] [diff] [review]:
-----------------------------------------------------------------
I'm sorry, I didn't take this bug right away. Actually I've made it in my github branch[1] based on bug 838479 & bug 849739.
[1]: https://github.com/vicamo/b2g_releases-mozilla-central/tree/bugzilla/854790/sms-filter-thread-id
Attachment #731099 -
Flags: review?(vyang) → review-
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> ... github branch[1] based on bug 838479 ...
Sorry, should be bug 838467.
Updated•12 years ago
|
Attachment #731096 -
Flags: superreview?(jonas)
Attachment #731096 -
Flags: review?(vyang)
Comment 7•12 years ago
|
||
Per comment #5, Vicamo has actually done this. Let him take this.
Assignee: gene.lian → vyang
Comment 8•12 years ago
|
||
leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed
Whiteboard: NO_UPLIFT
Updated•12 years ago
|
Blocks: b2g-mms-dom-api
Updated•12 years ago
|
Keywords: dev-doc-needed
Updated•12 years ago
|
Whiteboard: NO_UPLIFT → [NO_UPLIFT]
Comment 9•12 years ago
|
||
Hi Vicamo, are you going to take this? If yes, you need to find reviewers for you. I'm fine to re-take this since my patches are almost as the same as yours. Please let me know. Thanks!
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #731096 -
Attachment is obsolete: true
Attachment #735677 -
Flags: superreview?(mounir)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #731099 -
Attachment is obsolete: true
Attachment #735678 -
Flags: review?(mounir)
Assignee | ||
Comment 12•12 years ago
|
||
Almost identical to yours, but rebased. Also fixed a typo.
Attachment #735679 -
Flags: review?(gene.lian)
Assignee | ||
Comment 13•12 years ago
|
||
refactor test_filter_mixed.js to have multiple message in each thread
Attachment #735680 -
Flags: review?(gene.lian)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #735681 -
Flags: review?(gene.lian)
Updated•12 years ago
|
Attachment #735677 -
Flags: superreview?(mounir) → superreview+
Comment 15•12 years ago
|
||
Comment on attachment 735678 [details] [diff] [review]
Part 2/4: DOM
Review of attachment 735678 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/SmsFilter.cpp
@@ +275,5 @@
> +NS_IMETHODIMP
> +SmsFilter::SetThreadId(JSContext* aCx, const JS::Value& aThreadId)
> +{
> + if (aThreadId == JSVAL_VOID) {
> + mData.threadId() = 0;
Why isn't that throwing NS_ERROR_INVALID_ARG? I would remove that condition and simply rely on the one below that throws if it is not a number.
Attachment #735678 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #15)
> Comment on attachment 735678 [details] [diff] [review]
> Part 2/4: DOM
> -----------------------------------------------------------------
>
> ::: dom/mobilemessage/src/SmsFilter.cpp
> @@ +275,5 @@
> > +NS_IMETHODIMP
> > +SmsFilter::SetThreadId(JSContext* aCx, const JS::Value& aThreadId)
> > +{
> > + if (aThreadId == JSVAL_VOID) {
> > + mData.threadId() = 0;
>
> Why isn't that throwing NS_ERROR_INVALID_ARG? I would remove that condition
> and simply rely on the one below that throws if it is not a number.
That's for reverting previous setting to the filter. See http://dxr.mozilla.org/mozilla-central/dom/mobilemessage/tests/test_smsfilter.html#l44
Updated•12 years ago
|
Attachment #735679 -
Flags: review?(gene.lian) → review+
Comment 17•12 years ago
|
||
Comment on attachment 735681 [details] [diff] [review]
Part 4-2/4: test cases for filtering threadId
Review of attachment 735681 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/tests/marionette/test_filter_mixed.js
@@ +174,5 @@
>
> +let INVALID_THREAD_ID;
> +tasks.push(function assignInvalidThreadID() {
> + INVALID_THREAD_ID = threadIds[threadIds.length - 1] + 1;
> + log("Assign INVALID_THREAD_ID to " + INVALID_THREAD_ID);
log("Set INVALID_THREAD_ID to be" + INVALID_THREAD_ID);
@@ +467,5 @@
> + let message = messages[i];
> + is(message.threadId, filter.threadId, "message threadId");
> + if (!(checkSenderOrReceiver(message, filter.numbers[0]) ||
> + checkSenderOrReceiver(message, filter.numbers[1]))) {
> + ok(false, "message sendor or receiver number");
NIT: s/sendor/sender and others within this file.
Attachment #735681 -
Flags: review?(gene.lian) → review+
Comment 18•12 years ago
|
||
Comment on attachment 735680 [details] [diff] [review]
Part 4-1/4: test case refactoring
Review of attachment 735680 [details] [diff] [review]:
-----------------------------------------------------------------
Please run try before checking in the test case changes for these 2 patches. Thanks!
::: dom/mobilemessage/tests/marionette/test_filter_mixed.js
@@ +177,1 @@
> ok(false, "message sendor or receiver number");
Just a NIT since you're here: s/sendor/sender and others within this file.
@@ +209,5 @@
> getAllMessages(function (messages) {
> + // { delivery: "received", sender: "5555315550", read: true },
> + // { delivery: "received", sender: "5555315552", read: true },
> + // { delivery: "received", sender: "5555315554", read: true },
> + // { delivery: "received", sender: "5555315556", read: true }, and
remove "and"
@@ +391,5 @@
> let message = messages[i];
> + if (!((message.sender == "+15555315550")
> + || (message.sender == "5555315550")
> + || (message.receiver == "+15555315550")
> + || (message.receiver == "5555315550"))) {
!checkSenderOrReceiver(message, "5555315550")
Attachment #735680 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Try with comment-addressed patches: https://tbpl.mozilla.org/?tree=Try&rev=3354defac2bb
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> Try with comment-addressed patches:
Busted test_smsfilter mochitest. Somehow `read` uses `undefined` to clear previous set value while others use `null`. This difference comes from bug 733320 comment 33. I'll correct the test case to use `undefined` and also add omitted test cases for `read` back.
Assignee | ||
Comment 21•12 years ago
|
||
1) Add sr=mounir
2) Place left parenthesis in new line
Attachment #735677 -
Attachment is obsolete: true
Attachment #736678 -
Flags: superreview+
Assignee | ||
Comment 22•12 years ago
|
||
Add r=mounir only.
Attachment #735678 -
Attachment is obsolete: true
Attachment #736679 -
Flags: review+
Assignee | ||
Comment 23•12 years ago
|
||
Add r=gene only
Attachment #735679 -
Attachment is obsolete: true
Attachment #736681 -
Flags: review+
Assignee | ||
Comment 24•12 years ago
|
||
Address review comments
Attachment #735680 -
Attachment is obsolete: true
Attachment #736682 -
Flags: review+
Assignee | ||
Comment 25•12 years ago
|
||
Address review comments and fix mochitest bustage.
Attachment #735681 -
Attachment is obsolete: true
Attachment #736683 -
Flags: review+
Assignee | ||
Comment 26•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/15acf0c7597c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9413f23d5cd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f81432f8494f
https://hg.mozilla.org/integration/mozilla-inbound/rev/226521477f7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc732a49eae
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/15acf0c7597c
https://hg.mozilla.org/mozilla-central/rev/9413f23d5cd5
https://hg.mozilla.org/mozilla-central/rev/f81432f8494f
https://hg.mozilla.org/mozilla-central/rev/226521477f7d
https://hg.mozilla.org/mozilla-central/rev/5bc732a49eae
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment 28•12 years ago
|
||
This really should use number or null, not number or undefined to be consistent with the platform.
Assignee | ||
Comment 29•12 years ago
|
||
(In reply to :Ms2ger from comment #28)
> This really should use number or null, not number or undefined to be
> consistent with the platform.
Filed bug 863035 to address this.
Assignee | ||
Comment 30•12 years ago
|
||
Uplift WIP: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/838467/b2g18 , verifying.
Assignee | ||
Comment 31•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/48af69015b7d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0d31c26a81d3
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2491f4bec7ef
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4053c13774bb
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5352e56e2adf
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → wontfix
status-firefox23:
--- → fixed
Whiteboard: [NO_UPLIFT]
Updated•12 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•