Closed Bug 854790 Opened 11 years ago Closed 11 years ago

B2G SMS & MMS: support filtering by thread ID

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g leo+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 7 obsolete files)

2.24 KB, patch
vicamo
: superreview+
Details | Diff | Splinter Review
1.64 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
4.08 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
13.80 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
11.60 KB, 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.
Nominate for leo? because it's essential to complete MMS thread view. See details in bug 838467 comment 21.
blocking-b2g: --- → leo?
Also depend on bug 850127, which adds threadId to SMS/MMS message types.
Depends on: 850127
blocking-b2g: leo? → leo+
Assignee: nobody → gene.lian
Blocks: b2g-sms, b2g-mms
Summary: WebSMS: supprt filtering by threadId → B2G SMS & MMS: support filtering by thread ID
Attached patch Part 1, IDL & DOM (obsolete) — Splinter Review
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)
Thanks to Vicamo's previous thoughtful design. This part is super easy. ;)
Attachment #731099 - Flags: review?(vyang)
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-
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> ... github branch[1] based on bug 838479 ...

Sorry, should be bug 838467.
Attachment #731096 - Flags: superreview?(jonas)
Attachment #731096 - Flags: review?(vyang)
Per comment #5, Vicamo has actually done this. Let him take this.
Assignee: gene.lian → vyang
leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed
Whiteboard: NO_UPLIFT
Whiteboard: NO_UPLIFT → [NO_UPLIFT]
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!
Attached patch Part 1/4: interface changes (obsolete) — Splinter Review
Attachment #731096 - Attachment is obsolete: true
Attachment #735677 - Flags: superreview?(mounir)
Attached patch Part 2/4: DOM (obsolete) — Splinter Review
Attachment #731099 - Attachment is obsolete: true
Attachment #735678 - Flags: review?(mounir)
Attached patch Part 3/4: RIL (obsolete) — Splinter Review
Almost identical to yours, but rebased. Also fixed a typo.
Attachment #735679 - Flags: review?(gene.lian)
refactor test_filter_mixed.js to have multiple message in each thread
Attachment #735680 - Flags: review?(gene.lian)
Attachment #735681 - Flags: review?(gene.lian)
Attachment #735677 - Flags: superreview?(mounir) → superreview+
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+
(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
Attachment #735679 - Flags: review?(gene.lian) → review+
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 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+
Try with comment-addressed patches: https://tbpl.mozilla.org/?tree=Try&rev=3354defac2bb
(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.
1) Add sr=mounir
2) Place left parenthesis in new line
Attachment #735677 - Attachment is obsolete: true
Attachment #736678 - Flags: superreview+
Attached patch Part 2/4: DOMSplinter Review
Add r=mounir only.
Attachment #735678 - Attachment is obsolete: true
Attachment #736679 - Flags: review+
Attached patch Part 3/4: RILSplinter Review
Add r=gene only
Attachment #735679 - Attachment is obsolete: true
Attachment #736681 - Flags: review+
Address review comments
Attachment #735680 - Attachment is obsolete: true
Attachment #736682 - Flags: review+
Address review comments and fix mochitest bustage.
Attachment #735681 - Attachment is obsolete: true
Attachment #736683 - Flags: review+
This really should use number or null, not number or undefined to be consistent with the platform.
Blocks: 863035
(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.
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.