Closed Bug 1197008 Opened 9 years ago Closed 9 years ago

[WebSMS] Pass "hasThreadId" along with "threadId" instead of using 0 = not passed.

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: reuben, Assigned: reuben)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Comment on attachment 8650770 [details] [diff] [review]
Pass hasThreadId along with threadId parameter in the relevant calls

Hey Hsin,

The assumption that valid thread ids are != 0 doesn't work on Android, so that's the reason for this change, but I thought it made sense to have the explicit parameter everywhere.
Attachment #8650770 - Flags: review?(htsai)
Comment on attachment 8650770 [details] [diff] [review]
Pass hasThreadId along with threadId parameter in the relevant calls

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

Bevis, who is the Message expert, should do the review :)
Attachment #8650770 - Flags: review?(htsai) → review?(btseng)
Comment on attachment 8650770 [details] [diff] [review]
Pass hasThreadId along with threadId parameter in the relevant calls

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

Please see my comments below.
To ensure there is no function broken in b2g,
Please make sure that there is no mobilemessage related failure in mnw in tryserver.
You can verify your change locally by running the following command in your emulator:
$./mach marionette-webapi /home/bevis/Projects/Builds/gecko/src_2.2r/dom/mobilemessage/tests/marionette/manifest.ini

In addition, it seems that you are going to extend the support of these web API in Android back-end.
I don't have too much information about how these APIs are currently used in android back-end.
However, we have a new plan under discussion to define a newly separated set of mobilemessage APIs to replace current one 
which provides a new concept to have message DB management to be done in application side instead and the system will only
help to manage the outgoing/incoming transactions of SMS/MMS.
The purpose of the new design is to provide the possibility if App layer to have more flexible way to define the UX to
group the messages together, filtering, search according to the message context without restriction of the web API design.

You can also check this from the following etherpad to see if there is any impact to you in the future:
https://etherpad.mozilla.org/MobileMessageAPI2
Slides for the API introduction and the sequential diagrams:
https://docs.google.com/a/mozilla.com/presentation/d/1qi-wQAzuRIv0cywNWTfZClASSGfFIWtJkM53dlaIkCY/edit?usp=sharing

::: dom/mobilemessage/MobileMessageManager.cpp
@@ +462,5 @@
>    if (hasRead) {
>      read = aFilter.mRead.Value();
>    }
>  
> +  bool hasThreadId = !aFilter.mThreadId.IsNull();

Unfortunately, the |hasThreadId| will always be true if getMessages() without providing the |filter|.
To make this works, the default value of the MobileMessageFilter::threadId in MozMobileMessageManager.webidl has to be changed from 0 to null.

::: dom/mobilemessage/gonk/MobileMessageDB.jsm
@@ +4044,5 @@
>     *         The cursor to iterate on messages.
>     */
>    createMessageCursor: function(aHasStartDate, aStartDate, aHasEndDate,
>                                  aEndDate, aNumbers, aNumbersCount, aDelivery,
> +                                aHasRead, aRead, aHasThreadId, aThreadId,

Please also help to change add on more parameter for aHasThreadId in mmdb_head.js used by marionette test case. [1]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/mmdb_head.js?offset=0#344

::: dom/mobilemessage/interfaces/nsIMobileMessageDatabaseService.idl
@@ +34,5 @@
>                                                  in uint32_t numbersCount,
>                                                  [Null(Null), Undefined(Null)] in DOMString delivery,
>                                                  in boolean hasRead,
>                                                  in boolean read,
> +                                                in boolean hasThreadId,

Please renew uid of nsIMobileMessageDatabaseService.
Attachment #8650770 - Flags: review?(btseng) → review-
Hi Bevis, thanks for the review. With the new API we'd have message data duplicated in the Android ContentProviders and the Gaia app(s), but I guess that's unavoidable. I'll attach an updated patch soon.
Comment on attachment 8655782 [details] [diff] [review]
Pass hasThreadId along with threadId parameter in the relevant calls, v2

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

Looks good to me, Thanks!

r? hsinyi for the change in MozMobileMessageManager.webidl.
Attachment #8655782 - Flags: review?(htsai)
Attachment #8655782 - Flags: review?(btseng)
Attachment #8655782 - Flags: review+
Attachment #8655782 - Flags: review?(htsai) → review+
https://hg.mozilla.org/mozilla-central/rev/763a6d65f36c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: