Closed Bug 811252 (message-database) Opened 8 years ago Closed 2 years ago

[meta] B2G SMS & MMS: implement message database

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-)

RESOLVED WONTFIX
blocking-b2g -

People

(Reporter: vicamo, Unassigned)

References

Details

Attachments

(4 obsolete files)

No description provided.
No longer blocks: 801632
Depends on: 801632
I used to have a bunch of experiences on the indexed DB things. Smells like my bug. Glad to take this if needed.
In https://bugzilla.mozilla.org/show_bug.cgi?id=801632, there are some discussions might be useful for your implementation.
Thanks for the info. I'll catch up that thread.
QA Contact: clian
Note to myself: MMS DOM API design is at Bug 760065. How to design the DB schema will depend on that.
Assignee: nobody → clian
QA Contact: clian
Attached patch WIP Patch (obsolete) — Splinter Review
Just a very quick WIP patch. Still have lots of work to do.
Attached patch WIP Patch V2 (obsolete) — Splinter Review
Attachment #694330 - Attachment is obsolete: true
Attachment #694742 - Flags: feedback?(vyang)
Comment on attachment 694742 [details] [diff] [review]
WIP Patch V2

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

I'd really want to have some full re-examination on what we have in SMS and what are the differences before going to implement an MMS database. Some items like:

1. We have already an SMS database, and planned to merge SMS and MMS into one. So, if we're not going to merge the database now, we'd better keep it simple and with only features we'll need right away. Or, let's just reuse SmsDatabaseService for MMS.

2. MMS has multiple receivers and many other attributes than SMS. Besides, MMS has also an internally used object type and a pdu object type. Based on these two facts, enumerating properties to be stored will block us from implementing minor features and results in problems in transaction handling. That is, can we save a whole internally used message object into database without additional interpreting? Like no received/sent differences, which should be handled by some kind of transaction service.

3. Please ensure every action call to the database service has a callback. Error callback should be mandatory, not optional.

::: dom/mms/src/ril/MmsDB.jsm
@@ +73,5 @@
> +   * @param aRecord
> +   *        The MMS record to be saved.
> +   * @param aSuccessCb
> +   *        Callback function to invoke with result.
> +   * @param aErrorCb [optional]

I think we need only one callback. We should invoke some callback on either case.

@@ +100,5 @@
> +   *        Callback function to invoke with result.
> +   * @param aErrorCb [optional]
> +   *        Callback function to invoke when there was an error.
> +   */
> +  saveReceivedMms: function saveReceivedMms(aSender, aSmil, aAttachments, aDate, aSuccessCb, aErrorCb) {

In my opinion, separating received/sent message is beyond the responsibility of a message database. I know that's how it was done in SMS database, but maybe we can simplify it.

@@ +199,5 @@
> +   *        Callback function to invoke with result.
> +   * @param aErrorCb [optional]
> +   *        Callback function to invoke when there was an error.
> +   */
> +  clearAll: function clearAll(aSuccessCb, aErrorCb) {

We don't need this call ever, right?

@@ +237,5 @@
> +   *        Callback function to invoke with result.
> +   * @param aErrorCb [optional]
> +   *        Callback function to invoke when there was an error.
> +   */
> +  getMmsByFilter: function getMmsByFilter(aFilter, aSuccessCb, aErrorCb) {

You can skip this for now.

@@ +366,5 @@
> +   *        Callback function to invoke with result.
> +   * @param aErrorCb [optional]
> +   *        Callback function to invoke when there was an error.
> +   */
> +  setMmsDelivery: function setMmsDelivery(aId, aState, aDeliveryStatus, aSuccessCb, aErrorCb) {

And this. Let's add it back in following bugs.

@@ +403,5 @@
> +   *        Callback function to invoke with result.
> +   * @param aErrorCb [optional]
> +   *        Callback function to invoke when there was an error.
> +   */
> +  setMmsRead: function setMmsRead(aId, aRead, aSuccessCb, aErrorCb) {

And this.
Attachment #694742 - Flags: feedback?(vyang)
Attached patch WIP Patch V3 (obsolete) — Splinter Review
Hi Vicamo,

This patch should depend on bug 831683. In summary, I s/receiver/receivers and provided parameters to eat SMIL and attachments.

Just a WIP. We still have lots of work to do on the caller site. Could you please return some feedback to me? Thanks!
Attachment #694742 - Attachment is obsolete: true
Attachment #708991 - Flags: feedback?(vyang)
Attached patch WIP Patch V3.1 (obsolete) — Splinter Review
Fixed some minor nits. Please see the previous comment for the summary.
Attachment #708991 - Attachment is obsolete: true
Attachment #708991 - Flags: feedback?(vyang)
Attachment #708992 - Flags: feedback?(vyang)
leo+, this bug is needed to fulfill MMS user stories for v1.1
blocking-b2g: --- → leo+
Summary: B2G MMS: implement MMS database → [meta] B2G MMS: implement MMS database
Comment on attachment 708992 [details] [diff] [review]
WIP Patch V3.1

This patch has been done and checked in at bug 839436.
Attachment #708992 - Attachment is obsolete: true
Attachment #708992 - Flags: feedback?(vyang)
Don't need an assignee for a meta bug.
Assignee: gene.lian → nobody
blocking-b2g: leo+ → ---
This is meta bug so I just cancelled the leo+ flag. Please let me know if I'm wrong. Thanks!
No longer blocks: 810067
Depends on: 810067
No longer blocks: 810091
Depends on: 810091
All sub-tasks are done.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Should keep the meta bug open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: [meta] B2G MMS: implement MMS database → [meta] B2G SMS & MMS: implement message database
Alias: message-database
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
blocking-b2g: --- → -
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 7 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.