1023695 - [B2G][SMS] Introduce sms-deleted event to provide the awareness of change in MobileMessageDB for multiple apps interested in SMS.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

In Bug 1022575, we saw that it is possible to have multiple apps (SMS, Cost Control) to modify the MobileMessageDB.
However, there is no easy for this apps to be aware of the change if some messages deleted by one of the application.

In addition, if we plan to migrate to DataStore in the future, 
Since DataStore provides the possibility for multiple apps to access the DB,
this might not be necessary anymore after migrating to DataStore.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

mark blocking-b2g to 2.0? because bug 1022575 depends on this bug has been marked as 2.0+.

Comment 2

[Sandip Kamat]

Bevis, What is the user impact of this use case? It is same as 1022575? If it is same, I recommend tagging it with 2.0+.

Comment 3

[Julien Wajsberg [:julienw]]

Sandip: yes, this bug should be the simplest/cleanest way to fix bug 1022575.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Julien,

With new |ondeleted| event provided by MobileMessageManager, we suppose to provide the the id list of the deleted messages.
Is this also what you expect to receive from |ondeleted| event?

Comment 5

[Julien Wajsberg [:julienw]]

Having the thread_id could be useful too, but otherwise it stil should work with only the id (we'd have a little more work to do on the Gaia side but that's all).

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Having the thread_id could be useful too, but otherwise it stil should work
> with only the id (we'd have a little more work to do on the Gaia side but
> that's all).

How about have 2 different events:
1. |onmessagesdeleted| which reports the id list of the deleted messages request by delete() in MobileMessageManager.
2. |onthreaddeleted| which reports the deleted thread id when happens.

Which means it is possible to have 1 |onmessagedeleted| event and multiple |onthreaddeleted| events for a single delete() request.

Comment 7

[Julien Wajsberg [:julienw]]

Mmm I'd very much prefer one single event :)

Only providing the list of ids should be enough, we'll need to do something to find the thread for one id but we'll need it for bug 1024835 too anyway.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

I'd prefer to have |onthreaddeleted| in separated event because 
1. it's not only triggered by deletion of messages but also happened in the scenario in bug 1024835 when the message is updated with more information after completely retrieved.
2. we'll have to add unnecessary logic to wait for another asynchronous update result in thread DB during each deletion of the messages which introduces some overhead compared to original deleting operation if we have to combine these deleted information into one single event.

Hence, if the proposal in comment 6 is also workable to you for fixing bug 1022575 and bug 1024835, I'd like to add it in that way. :)

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #8)
> I'd prefer to have |onthreaddeleted| in separated event because 
> 1. it's not only triggered by deletion of messages but also happened in the
> scenario in bug 1024835 when the message is updated with more information
> after completely retrieved.
> 2. we'll have to add unnecessary logic to wait for another asynchronous
> update result in thread DB during each deletion of the messages which
> introduces some overhead compared to original deleting operation if we have
> to combine these deleted information into one single event.
> 
> Hence, if the proposal in comment 6 is also workable to you for fixing bug
> 1022575 and bug 1024835, I'd like to add it in that way. :)

(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #7)
> Mmm I'd very much prefer one single event :)
> 
> Only providing the list of ids should be enough, we'll need to do something
> to find the thread for one id but we'll need it for bug 1024835 too anyway.

BTW, if there is any strong need from Gaia perspective for single event compared to the separate ones, 
please let me know. I'll review it internally to see the possibility of supporting this. :)

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #8)
> I'd prefer to have |onthreaddeleted| in separated event because 
> 1. it's not only triggered by deletion of messages but also happened in the
> scenario in bug 1024835 when the message is updated with more information
> after completely retrieved.
> 2. we'll have to add unnecessary logic to wait for another asynchronous
> update result in thread DB during each deletion of the messages which
> introduces some overhead compared to original deleting operation if we have
> to combine these deleted information into one single event.
> 
> Hence, if the proposal in comment 6 is also workable to you for fixing bug
> 1022575 and bug 1024835, I'd like to add it in that way. :)

After further study and tracing the logs, I just realized that there shall not be any overhead in item#2 if we merge all this information into single event, because all the operations are done in the same transaction. 
What I should do is to collect these deleted information and fire the event after the transaction is complete.

Hence, single |ondelete| event looks okay to me as well. :)

Comment 11

[Julien Wajsberg [:julienw]]

The problem I see with onthreaddeleted is that we'll have possibly 2 different events for one delete operation.

If we eventually have these 2 events, we'll need to spec carefully the semantics: when they happen, in which sequence, etc.

With only one event it's simpler.

Moreover, since we eventually want to move away from the threads-in-gecko way, it's better if we don't rely on this :)

Comment 12

[Ken Chang[:kenkai]]

Bevis is working on this...:)

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Ken Chang[:ken] from comment #12)
> Bevis is working on this...:)

Thanks, Ken.
Sorry for not setting the assignee to myself. :p

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v1: Define .idl/.ipdl for notifying Deleted Info.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v1: Add implementation of .idl/.ipdl.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v1: .webidl change to support ondeleted event.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v1: Implementation of .webidl change.

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v1: Notify deleted info after DB has been changed.

This patch is to fix the following problems:
1. Awareness to SMS App for message deletion by Cost Control App. (Bug 1022575)
2. Update Deleted thread of the mms notification with dummy address '1' after its address was updated to the real one. (Bug 1024835)
3. Remove the observed topic "mobile-message-deleted" and apply the new "sms-deleted" which is more reliable because it is notified after transaction is complete.

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v1: Add Marionette Test Case to Verify ondeleted Event.

Add test case to verify the ondeleted event.

Totally, 6 patches are uploaded.

May I have your time for review? :)

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

update target milestone & ETA.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v2: Define .idl/.ipdl for notifying Deleted Info.

1. use [array] instead of jsval.
2. s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v2: Add implementation of .idl/.ipdl.

1. use [array] instead of jsval.
2. s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v2: .webidl change to support ondeleted event.

1. use sequence<> instead of nsIVariant to represent a array in webidl.
2. s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v2: Implementation of .webidl change.

1. use sequence<> instead of nsIVariant to represent a array in webidl.
2. s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v2: Notify deleted info after DB has been changed.

s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v2: Add Marionette Test Case to Verify ondeleted Event.

s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v3: Implementation of .webidl change.

Refine the usage of Nullable.SetValue().

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8448531 [details] [diff] [review]
Patch Part 3 v2: .webidl change to support ondeleted event.

This is to add a new ondeleted event from MobileMessageManager to allow multiple Apps to be aware of the deletion of messages in the database to fix the problem in bug 1022575.

Hi Olli,

May I have your review for this change? Thanks!

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8448567 [details] [diff] [review]
Patch Part 4 v3: Implementation of .webidl change.

This is to add a new ondeleted event from MobileMessageManager to allow multiple Apps to be aware of the deletion of messages in the database to fix the problem in bug 1022575.

Hi Olli,

May I have your review for this change? Thanks!

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8448531 [details] [diff] [review]
Patch Part 3 v2: .webidl change to support ondeleted event.

Since the event will be used with MozMobileMessageManager, its interface
should be enabled only when MozMobileMessageManager is enabled.
So, add [Pref="dom.sms.enabled"] to MozMessageDeletedEvent.
With that, r+

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8448567 [details] [diff] [review]
Patch Part 4 v3: Implementation of .webidl change.

>+MobileMessageManager::DispatchTrustedDeletedEventToSelf(const char* aTopic,
>+                                                    nsISupports* aDeletedInfo)
You don't need aTopic param, so remove it and move nsISupports* aDeletedInfo to be right after (

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8448531 [details] [diff] [review]
> Patch Part 3 v2: .webidl change to support ondeleted event.
> 
> Since the event will be used with MozMobileMessageManager, its interface
> should be enabled only when MozMobileMessageManager is enabled.
> So, add [Pref="dom.sms.enabled"] to MozMessageDeletedEvent.
> With that, r+
(In reply to Olli Pettay [:smaug] from comment #31)
> Comment on attachment 8448567 [details] [diff] [review]
> Patch Part 4 v3: Implementation of .webidl change.
> 
> >+MobileMessageManager::DispatchTrustedDeletedEventToSelf(const char* aTopic,
> >+                                                    nsISupports* aDeletedInfo)
> You don't need aTopic param, so remove it and move nsISupports* aDeletedInfo
> to be right after (

Thanks for your advice. I'll address this in next updated patches.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8448530 [details] [diff] [review]
Patch Part 2 v2: Add implementation of .idl/.ipdl.

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

::: dom/mobilemessage/src/DeletedMessageInfo.cpp
@@ +33,5 @@
> +                           uint32_t  aThreadCount,
> +                           nsIDeletedMessageInfo** aDeletedInfo)
> +{
> +  NS_ENSURE_ARG_POINTER(aDeletedInfo);
> +  NS_ENSURE_TRUE((aMsgCount + aThreadCount) > 0, NS_ERROR_INVALID_ARG);

nit: please check (aMsgCount || aThreadCount) instead. The original expression may, although pretty unlikely, overflow with large aMessageCount and aThreadCount.

@@ +47,5 @@
> +  }
> +
> +  // Append deleted thread ids.
> +  for (i = 0; i < aThreadCount; i++) {
> +    data.deletedThreadIds().AppendElement(aThreadIds[i]);

I'd really like to avoid copying data again and again. We can have another overloaded ctor that accept (int32_t*, uint32_t, uin64_t*, uint32_t).

@@ +86,5 @@
> +  }
> +  mDeletedMessageIds->SetAsArray(nsIDataType::VTYPE_INT32,
> +                               nullptr,
> +                               length,
> +                               reinterpret_cast<void *>(array));

mDeletedMessageIds->SetAsArray(nsIDataType::VTYPE_INT32,
                               nullptr,
                               length,
                               mData.deletedMessageIds().Elements());

And check return value of SetAsArray().

@@ +126,5 @@
> +  }
> +  mDeletedThreadIds->SetAsArray(nsIDataType::VTYPE_UINT64,
> +                               nullptr,
> +                               length,
> +                               reinterpret_cast<void *>(array));

ditto.

::: dom/mobilemessage/src/DeletedMessageInfo.h
@@ +18,5 @@
> +class DeletedMessageInfo MOZ_FINAL : public nsIDeletedMessageInfo
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(DeletedMessageInfo)

There is no cycle reference in this class at all.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #33)
> Comment on attachment 8448530 [details] [diff] [review]
> Patch Part 2 v2: Add implementation of .idl/.ipdl.
> 
> Review of attachment 8448530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/DeletedMessageInfo.cpp
> @@ +33,5 @@
> > +                           uint32_t  aThreadCount,
> > +                           nsIDeletedMessageInfo** aDeletedInfo)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aDeletedInfo);
> > +  NS_ENSURE_TRUE((aMsgCount + aThreadCount) > 0, NS_ERROR_INVALID_ARG);
> 
> nit: please check (aMsgCount || aThreadCount) instead. The original
> expression may, although pretty unlikely, overflow with large aMessageCount
> and aThreadCount.
> 
> @@ +47,5 @@
> > +  }
> > +
> > +  // Append deleted thread ids.
> > +  for (i = 0; i < aThreadCount; i++) {
> > +    data.deletedThreadIds().AppendElement(aThreadIds[i]);
> 
> I'd really like to avoid copying data again and again. We can have another
> overloaded ctor that accept (int32_t*, uint32_t, uin64_t*, uint32_t).
> 
> @@ +86,5 @@
> > +  }
> > +  mDeletedMessageIds->SetAsArray(nsIDataType::VTYPE_INT32,
> > +                               nullptr,
> > +                               length,
> > +                               reinterpret_cast<void *>(array));
> 
> mDeletedMessageIds->SetAsArray(nsIDataType::VTYPE_INT32,
>                                nullptr,
>                                length,
>                                mData.deletedMessageIds().Elements());
> 
> And check return value of SetAsArray().
> 
> @@ +126,5 @@
> > +  }
> > +  mDeletedThreadIds->SetAsArray(nsIDataType::VTYPE_UINT64,
> > +                               nullptr,
> > +                               length,
> > +                               reinterpret_cast<void *>(array));
> 
> ditto.
> 
> ::: dom/mobilemessage/src/DeletedMessageInfo.h
> @@ +18,5 @@
> > +class DeletedMessageInfo MOZ_FINAL : public nsIDeletedMessageInfo
> > +{
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_CLASS(DeletedMessageInfo)
> 
> There is no cycle reference in this class at all.

Thanks for advice. I'll address this in next update. :)

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8448567 [details] [diff] [review]
Patch Part 4 v3: Implementation of .webidl change.

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +538,5 @@
> +    if (msgIdLength) {
> +      Sequence<int32_t>& deletedMsgIds = init.mDeletedMessageIds.SetValue();
> +      for (uint32_t i = 0; i < msgIdLength; i++) {
> +        deletedMsgIds.AppendElement(info->GetData().deletedMessageIds()[i]);
> +      }

Sequence<T> is also a derived of FallibleTArray<T>. So you can use nsTArray::AppendElements() for the job.

@@ +545,5 @@
> +    uint32_t threadIdLength = info->GetData().deletedThreadIds().Length();
> +    if (threadIdLength) {
> +      Sequence<uint64_t>& deletedThreadIds = init.mDeletedThreadIds.SetValue();
> +      for (uint32_t i = 0; i < threadIdLength; i++) {
> +        deletedThreadIds.AppendElement(info->GetData().deletedThreadIds()[i]);

ditto.

Comment 36

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #35)
> Comment on attachment 8448567 [details] [diff] [review]
> Patch Part 4 v3: Implementation of .webidl change.
> 
> Review of attachment 8448567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/MobileMessageManager.cpp
> @@ +538,5 @@
> > +    if (msgIdLength) {
> > +      Sequence<int32_t>& deletedMsgIds = init.mDeletedMessageIds.SetValue();
> > +      for (uint32_t i = 0; i < msgIdLength; i++) {
> > +        deletedMsgIds.AppendElement(info->GetData().deletedMessageIds()[i]);
> > +      }
> 
> Sequence<T> is also a derived of FallibleTArray<T>. So you can use
> nsTArray::AppendElements() for the job.
> 
> @@ +545,5 @@
> > +    uint32_t threadIdLength = info->GetData().deletedThreadIds().Length();
> > +    if (threadIdLength) {
> > +      Sequence<uint64_t>& deletedThreadIds = init.mDeletedThreadIds.SetValue();
> > +      for (uint32_t i = 0; i < threadIdLength; i++) {
> > +        deletedThreadIds.AppendElement(info->GetData().deletedThreadIds()[i]);
> 
> ditto.

Thanks. I should be more familiar with these utility classes to prevent these redundant codes.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8448535 [details] [diff] [review]
Patch Part 6 v2: Add Marionette Test Case to Verify ondeleted Event.

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

::: dom/mobilemessage/tests/marionette/head.js
@@ +362,5 @@
>    }
>  
> +  let promises = [];
> +  promises.push(waitForManagerEvent("deleted")
> +  .then((aEvent) => { return aEvent; }));

You don't need this |then| step.

@@ +368,3 @@
>    let request = manager.delete(aMessageIds);
> +  promises.push(wrapDomRequestAsPromise(request)
> +    .then((aEvent) => { return aEvent.target.result; }));

You may also merge this line into:

  return Promise.all(promises)
    .then((aResults) => { return { deletedInfo: aResults[0],
                                   deletedFlags: aResults[1].target.result }; });

::: dom/mobilemessage/tests/marionette/test_ondeleted_event.js
@@ +15,5 @@
> +       JSON.stringify(aThreadIds), 'Check Thread Id.');
> +}
> +
> +function testDeletingMessagesInOneThreadOneByOne() {
> +  let sendMessages = [];

nit: sentMessages

Comment 38

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #37)
> Comment on attachment 8448535 [details] [diff] [review]
> Patch Part 6 v2: Add Marionette Test Case to Verify ondeleted Event.
> 
> Review of attachment 8448535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/tests/marionette/head.js
> @@ +362,5 @@
> >    }
> >  
> > +  let promises = [];
> > +  promises.push(waitForManagerEvent("deleted")
> > +  .then((aEvent) => { return aEvent; }));
> 
> You don't need this |then| step.
> 
> @@ +368,3 @@
> >    let request = manager.delete(aMessageIds);
> > +  promises.push(wrapDomRequestAsPromise(request)
> > +    .then((aEvent) => { return aEvent.target.result; }));
> 
> You may also merge this line into:
> 
>   return Promise.all(promises)
>     .then((aResults) => { return { deletedInfo: aResults[0],
>                                    deletedFlags: aResults[1].target.result
> }; });
> 
> ::: dom/mobilemessage/tests/marionette/test_ondeleted_event.js
> @@ +15,5 @@
> > +       JSON.stringify(aThreadIds), 'Check Thread Id.');
> > +}
> > +
> > +function testDeletingMessagesInOneThreadOneByOne() {
> > +  let sendMessages = [];
> 
> nit: sentMessages

Thanks for the review!
I'll address these nits in next update. :)

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8448534 [details] [diff] [review]
Patch Part 5 v2: Notify deleted info after DB has been changed.

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2092,5 @@
>          return;
>        }
>  
> +      // Collect changed info done during this transaction for further notification.
> +      let dbChangedInfo = {};

Please have:

  let deletedInfo = { messageIds: [], threadIds: [] };

"dbChangedInfo" sounds like something applied to any changes to the whole database, and that "changed" term leaves no clue to what's stored inside. Then you can shorten the attribute name "deletedMessageIds" to "messageIds".

@@ +2508,5 @@
>            if (!cursor) {
>              if (DEBUG) {
>                debug("Deleting mru entry for thread id " + threadId);
>              }
> +            threadStore.delete(threadId).onsuccess = function(event) {

We don't have to wait for an onsuccess event here because we'll only reporting the event when transaction is completed successfully.

@@ +2510,5 @@
>                debug("Deleting mru entry for thread id " + threadId);
>              }
> +            threadStore.delete(threadId).onsuccess = function(event) {
> +              if (dbChangedInfo) {
> +                self.fillDbChangedInfo(dbChangedInfo, "deletedThreadIds", threadId);

Update dbChangedInfo directly by |dbChangedInfo.threadIds.push(threadId);|

@@ +2544,5 @@
>        }
>      };
>    },
>  
> +  fillDbChangedInfo: function(changedInfo, key, value) {

Then we don't need this.

@@ +2562,5 @@
> +      return;
> +    }
> +
> +    // Notify 'sms-deleted' topic if either messages or threads are deleted.
> +    if (changedInfo["deletedMessageIds"] || changedInfo["deletedThreadIds"]) {

And we'll always have |changedInfo["deletedMessageIds"]| set to an JS array, so this is always true.

@@ +3039,5 @@
>          return;
>        }
> +
> +      // Collect changed info done during this transaction for further notification.
> +      let dbChangedInfo = {};

let dbChangedInfo = { messageIds: [], threadIds: [] };

@@ +3068,5 @@
>  
>              // First actually delete the message.
>              messageStore.delete(messageId).onsuccess = function(event) {
>                if (DEBUG) debug("Message id " + messageId + " deleted");
> +              self.fillDbChangedInfo(dbChangedInfo, "deletedMessageIds", messageId);

dbChangedInfo.messageIds.push(messageId);

Comment 40

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8448534 [details] [diff] [review]
Patch Part 5 v2: Notify deleted info after DB has been changed.

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2092,5 @@
>          return;
>        }
>  
> +      // Collect changed info done during this transaction for further notification.
> +      let dbChangedInfo = {};

I'll make these change be more specific to the deleting operations to simplify the implementation.

@@ +2508,5 @@
>            if (!cursor) {
>              if (DEBUG) {
>                debug("Deleting mru entry for thread id " + threadId);
>              }
> +            threadStore.delete(threadId).onsuccess = function(event) {

You are right! I'll address this in next update.

Comment 41

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v3: Define .idl/.ipdl for notifying Deleted Info. r=vyang

Comment 42

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v3: Add implementation of .idl/.ipdl.

Address suggestion in comment 33.

Comment 43

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v3: .webidl change to support ondeleted event. r=vyang,smaug.

Comment 44

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v4: Implementation of .webidl change.

1. Use AppendElements() to copy array elements.
2. Remove |aTopic| parameter from DispatchTrustedDeletedEventToSelf().
3. Add MozMessageDeletedEvent into dom/tests/mochitest/general/test_interfaces.html to prevent test failure in try server.

Comment 45

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v3: Notify deleted info after DB has been changed.

Address the suggestion in comment 39.

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v3: Add Marionette Test Case to Verify ondeleted Event. r=vyang

address nits in comment 37.

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v4: Add implementation of .idl/.ipdl.

add #include "nsVariant.h" in DeletedMessageInfo.cpp to prevent build break in Linux build.

Comment 48

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v4: .webidl change to support ondeleted event. r=vyang,smaug.

re-base to prevent merge conflict.

Comment 49

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v4: Implementation of .webidl change.

Add MozMessageDeletedEvent into dom/events/test/test_all_synthetic_events.html.

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8452233 [details] [diff] [review]
Patch Part 2 v4: Add implementation of .idl/.ipdl.

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

::: dom/mobilemessage/src/DeletedMessageInfo.cpp
@@ +25,5 @@
> +{
> +  uint32_t i;
> +  for (i = 0; i < aMsgCount; i++) {
> +    mData.deletedMessageIds().AppendElement(aMessageIds[i]);
> +  }

Use |nsTArray_Impl::AppendElements(aMessageIds, aMsgCount);| instead.

@@ +29,5 @@
> +  }
> +
> +  for (i = 0; i < aThreadCount; i++) {
> +    mData.deletedThreadIds().AppendElement(aThreadIds[i]);
> +  }

ditto.

Comment 51

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #50)
> Comment on attachment 8452233 [details] [diff] [review]
> Patch Part 2 v4: Add implementation of .idl/.ipdl.
> 
> Review of attachment 8452233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/DeletedMessageInfo.cpp
> @@ +25,5 @@
> > +{
> > +  uint32_t i;
> > +  for (i = 0; i < aMsgCount; i++) {
> > +    mData.deletedMessageIds().AppendElement(aMessageIds[i]);
> > +  }
> 
> Use |nsTArray_Impl::AppendElements(aMessageIds, aMsgCount);| instead.
> 
> @@ +29,5 @@
> > +  }
> > +
> > +  for (i = 0; i < aThreadCount; i++) {
> > +    mData.deletedThreadIds().AppendElement(aThreadIds[i]);
> > +  }
> 
> ditto.

Thanks for reminding this. I'll address this in next update. :)

Comment 52

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1: Define .idl/.ipdl for notifying Deleted Info. r=vyang

Update final patches with positive result in try server.

Comment 53

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2: Add implementation of .idl/.ipdl. r=vyang

1. Update final patches with positive result in try server.
2. Apply the suggestion in comment 50.

Comment 54

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3: .webidl change to support ondeleted event. r=vyang,smaug.

Update final patches with positive result in try server.

Comment 55

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4: Implementation of .webidl change. r=vyang,smaug.

Update final patches with positive result in try server.

Comment 56

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5: Notify deleted info after DB has been changed. r=vyang

Update final patches with positive result in try server.

Comment 57

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6: Add Marionette Test Case to Verify ondeleted Event. r=vyang

Update final patches with positive result in try server.

Comment 58

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0] Patch Part 1: Define .idl/.ipdl for notifying Deleted Info. r=vyang, a=2.0+

Rebase for 2.0 branch.

Comment 59

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0] Patch Part 2: Add implementation of .idl/.ipdl. r=vyang, a=2.0+

rebase for 2.0 branch

Comment 60

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0] Patch Part 3: .webidl change to support ondeleted event. r=vyang,smaug,a=2.0+

rebase for 2.0 branch.

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0] Patch Part 4: Implementation of .webidl change. r=vyang,smaug, a=2.0+

rebase for 2.0 branch

Comment 62

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0] Patch Part 5: Notify deleted info after DB has been changed. r=vyang, a=2.0+

Rebase for 2.0 branch.

Comment 63

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.0] Patch Part 6: Add Marionette Test Case to Verify ondeleted Event. r=vyang, a=2.0+

Rebase for 2.0 branch.

Comment 64

[Bevis Tseng[:bevis][:btseng](Exited)]

tryserver is green:
https://tbpl.mozilla.org/?tree=Try&rev=9b5fa22a17aa

Comment 65

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Patch Part 4: Implementation of .webidl change. r=vyang,smaug. : v2

Rebased after bug 1035394.

Comment 66

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/29da1b86af3a
https://hg.mozilla.org/integration/b2g-inbound/rev/ef9dbb2245c8
https://hg.mozilla.org/integration/b2g-inbound/rev/5f5ef8d480bf
https://hg.mozilla.org/integration/b2g-inbound/rev/cfa5fcf861f5
https://hg.mozilla.org/integration/b2g-inbound/rev/bd756f5d5cf2
https://hg.mozilla.org/integration/b2g-inbound/rev/6dceb20498b3

Comment 67

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #65)
> Created attachment 8453644 [details] [diff] [review]
> Patch Part 4: Implementation of .webidl change. r=vyang,smaug. : v2
> 
> Rebased after bug 1035394.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #66)
> https://hg.mozilla.org/integration/b2g-inbound/rev/29da1b86af3a
> https://hg.mozilla.org/integration/b2g-inbound/rev/ef9dbb2245c8
> https://hg.mozilla.org/integration/b2g-inbound/rev/5f5ef8d480bf
> https://hg.mozilla.org/integration/b2g-inbound/rev/cfa5fcf861f5
> https://hg.mozilla.org/integration/b2g-inbound/rev/bd756f5d5cf2
> https://hg.mozilla.org/integration/b2g-inbound/rev/6dceb20498b3

Thanks for rebasing & checking in these patches. :)

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29da1b86af3a
https://hg.mozilla.org/mozilla-central/rev/ef9dbb2245c8
https://hg.mozilla.org/mozilla-central/rev/5f5ef8d480bf
https://hg.mozilla.org/mozilla-central/rev/cfa5fcf861f5
https://hg.mozilla.org/mozilla-central/rev/bd756f5d5cf2
https://hg.mozilla.org/mozilla-central/rev/6dceb20498b3

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/787768129673
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce27e012d817
https://hg.mozilla.org/releases/mozilla-aurora/rev/d6752789d710
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf117b204a28
https://hg.mozilla.org/releases/mozilla-aurora/rev/775ba85429bc
https://hg.mozilla.org/releases/mozilla-aurora/rev/6dd0fbb7c2e5