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

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

(Blocks: 1 bug)

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [p=5])

Attachments

(12 attachments, 23 obsolete attachments)

5.38 KB, patch
bevis
: review+
Details | Diff | Splinter Review
15.80 KB, patch
bevis
: review+
Details | Diff | Splinter Review
2.66 KB, patch
bevis
: review+
Details | Diff | Splinter Review
15.14 KB, patch
bevis
: review+
Details | Diff | Splinter Review
4.40 KB, patch
bevis
: review+
Details | Diff | Splinter Review
5.38 KB, patch
bevis
: review+
Details | Diff | Splinter Review
15.73 KB, patch
bevis
: review+
Details | Diff | Splinter Review
2.81 KB, patch
bevis
: review+
Details | Diff | Splinter Review
11.46 KB, patch
bevis
: review+
Details | Diff | Splinter Review
15.14 KB, patch
bevis
: review+
Details | Diff | Splinter Review
4.41 KB, patch
bevis
: review+
Details | Diff | Splinter Review
11.25 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Summary: [B2G][SMS] Introduce sms-deleted event to provide the awareness of change for multiple apps interesting in SMS. → [B2G][SMS] Introduce sms-deleted event to provide the awareness of change in MobileMessageDB for multiple apps interested in SMS.
(Assignee)

Comment 1

4 years ago
mark blocking-b2g to 2.0? because bug 1022575 depends on this bug has been marked as 2.0+.
blocking-b2g: --- → 2.0?

Comment 2

4 years ago
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+.

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
Sandip: yes, this bug should be the simplest/cleanest way to fix bug 1022575.
(Assignee)

Updated

4 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Whiteboard: [p=5]
Target Milestone: --- → 2.0 S4 (20june)
(Assignee)

Comment 4

4 years ago
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?
Flags: needinfo?(felash)
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).
Flags: needinfo?(felash)
(Assignee)

Comment 6

4 years ago
(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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
(Assignee)

Comment 8

4 years ago
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. :)
(Assignee)

Comment 9

4 years ago
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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. :)
(Assignee)

Comment 10

4 years ago
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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. :)
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

4 years ago
Bevis is working on this...:)
Assignee: nobody → btseng
(Assignee)

Comment 13

4 years ago
(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
(Assignee)

Comment 14

4 years ago
Created attachment 8447095 [details] [diff] [review]
Patch Part 1 v1: Define .idl/.ipdl for notifying Deleted Info.
Attachment #8447095 - Flags: review?(vyang)
(Assignee)

Comment 15

4 years ago
Created attachment 8447096 [details] [diff] [review]
Patch Part 2 v1: Add implementation of .idl/.ipdl.
Attachment #8447096 - Flags: review?(vyang)
(Assignee)

Comment 16

4 years ago
Created attachment 8447097 [details] [diff] [review]
Patch Part 3 v1: .webidl change to support ondeleted event.
Attachment #8447097 - Flags: review?(vyang)
(Assignee)

Comment 17

4 years ago
Created attachment 8447098 [details] [diff] [review]
Patch Part 4 v1: Implementation of .webidl change.
Attachment #8447098 - Flags: review?(vyang)
(Assignee)

Updated

4 years ago
Attachment #8447095 - Attachment description: Patch Part 1 v1: Define .idl/.ipdl for notifying Deleted Info. r=vyang. → Patch Part 1 v1: Define .idl/.ipdl for notifying Deleted Info.
(Assignee)

Updated

4 years ago
Attachment #8447096 - Attachment description: Patch Part 2 v1: Add implementation of .idl/.ipdl. r=vyang. → Patch Part 2 v1: Add implementation of .idl/.ipdl.
(Assignee)

Updated

4 years ago
Attachment #8447097 - Attachment description: Patch Part 3 v1: .webidl change to support ondeleted event. r=vyang. → Patch Part 3 v1: .webidl change to support ondeleted event.
(Assignee)

Comment 18

4 years ago
Created attachment 8447100 [details] [diff] [review]
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.
Attachment #8447100 - Flags: review?(vyang)
(Assignee)

Comment 19

4 years ago
Created attachment 8447102 [details] [diff] [review]
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? :)
Attachment #8447102 - Flags: review?(vyang)
(Assignee)

Comment 20

4 years ago
update target milestone & ETA.
Whiteboard: [p=5] → [p=5][ETA=7/11]
Target Milestone: 2.0 S4 (20june) → 2.0 S6 (18july)
(Assignee)

Comment 21

4 years ago
Created attachment 8448527 [details] [diff] [review]
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
Attachment #8447095 - Attachment is obsolete: true
Attachment #8447095 - Flags: review?(vyang)
Attachment #8448527 - Flags: review?(vyang)
(Assignee)

Comment 22

4 years ago
Created attachment 8448530 [details] [diff] [review]
Patch Part 2 v2: Add implementation of .idl/.ipdl.

1. use [array] instead of jsval.
2. s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds
Attachment #8447096 - Attachment is obsolete: true
Attachment #8447096 - Flags: review?(vyang)
Attachment #8448530 - Flags: review?(vyang)
(Assignee)

Comment 23

4 years ago
Created attachment 8448531 [details] [diff] [review]
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
Attachment #8447097 - Attachment is obsolete: true
Attachment #8447097 - Flags: review?(vyang)
Attachment #8448531 - Flags: review?(vyang)
(Assignee)

Comment 24

4 years ago
Created attachment 8448532 [details] [diff] [review]
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
Attachment #8447098 - Attachment is obsolete: true
Attachment #8447098 - Flags: review?(vyang)
Attachment #8448532 - Flags: review?(vyang)
(Assignee)

Updated

4 years ago
Attachment #8448531 - Attachment description: 1023695_part3_v2.patch → Patch Part 3 v2: .webidl change to support ondeleted event.
(Assignee)

Comment 25

4 years ago
Created attachment 8448534 [details] [diff] [review]
Patch Part 5 v2: Notify deleted info after DB has been changed.

s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds
Attachment #8447100 - Attachment is obsolete: true
Attachment #8447100 - Flags: review?(vyang)
Attachment #8448534 - Flags: review?(vyang)
(Assignee)

Comment 26

4 years ago
Created attachment 8448535 [details] [diff] [review]
Patch Part 6 v2: Add Marionette Test Case to Verify ondeleted Event.

s/deletedMessages/deletedMessageIds and s/deletedThreads/deletedThreadIds
Attachment #8447102 - Attachment is obsolete: true
Attachment #8447102 - Flags: review?(vyang)
Attachment #8448535 - Flags: review?(vyang)
(Assignee)

Comment 27

4 years ago
Created attachment 8448567 [details] [diff] [review]
Patch Part 4 v3: Implementation of .webidl change.

Refine the usage of Nullable.SetValue().
Attachment #8448532 - Attachment is obsolete: true
Attachment #8448532 - Flags: review?(vyang)
Attachment #8448567 - Flags: review?(vyang)
(Assignee)

Comment 28

4 years ago
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!
Attachment #8448531 - Flags: review?(bugs)
(Assignee)

Comment 29

4 years ago
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!
Attachment #8448567 - Flags: review?(bugs)
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+
Attachment #8448531 - Flags: review?(bugs) → review+
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 (
Attachment #8448567 - Flags: review?(bugs) → review+
(Assignee)

Comment 32

4 years ago
(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.
Attachment #8448527 - Flags: review?(vyang) → review+
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.
Attachment #8448530 - Flags: review?(vyang)
Attachment #8448531 - Flags: review?(vyang) → review+
(Assignee)

Comment 34

4 years ago
(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 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.
Attachment #8448567 - Flags: review?(vyang)
(Assignee)

Comment 36

4 years ago
(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 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
Attachment #8448535 - Flags: review?(vyang) → review+
(Assignee)

Comment 38

4 years ago
(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 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);
Attachment #8448534 - Flags: review?(vyang)
(Assignee)

Comment 40

4 years ago
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.
(Assignee)

Comment 41

4 years ago
Created attachment 8452149 [details] [diff] [review]
Patch Part 1 v3: Define .idl/.ipdl for notifying Deleted Info. r=vyang
Attachment #8448527 - Attachment is obsolete: true
Attachment #8452149 - Flags: review+
(Assignee)

Comment 42

4 years ago
Created attachment 8452153 [details] [diff] [review]
Patch Part 2 v3: Add implementation of .idl/.ipdl.

Address suggestion in comment 33.
Attachment #8448530 - Attachment is obsolete: true
Attachment #8452153 - Flags: review?(vyang)
(Assignee)

Updated

4 years ago
Attachment #8452153 - Attachment description: Patch Part 2 v2: Add implementation of .idl/.ipdl. → Patch Part 2 v3: Add implementation of .idl/.ipdl.
(Assignee)

Comment 43

4 years ago
Created attachment 8452156 [details] [diff] [review]
Patch Part 3 v3: .webidl change to support ondeleted event. r=vyang,smaug.
Attachment #8448531 - Attachment is obsolete: true
Attachment #8452156 - Flags: review+
(Assignee)

Comment 44

4 years ago
Created attachment 8452157 [details] [diff] [review]
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.
Attachment #8448567 - Attachment is obsolete: true
Attachment #8452157 - Flags: review?(vyang)
(Assignee)

Comment 45

4 years ago
Created attachment 8452160 [details] [diff] [review]
Patch Part 5 v3: Notify deleted info after DB has been changed.

Address the suggestion in comment 39.
Attachment #8448534 - Attachment is obsolete: true
Attachment #8452160 - Flags: review?(vyang)
(Assignee)

Comment 46

4 years ago
Created attachment 8452162 [details] [diff] [review]
Patch Part 6 v3: Add Marionette Test Case to Verify ondeleted Event. r=vyang

address nits in comment 37.
Attachment #8448535 - Attachment is obsolete: true
Attachment #8452162 - Flags: review+
(Assignee)

Comment 47

4 years ago
Created attachment 8452233 [details] [diff] [review]
Patch Part 2 v4: Add implementation of .idl/.ipdl.

add #include "nsVariant.h" in DeletedMessageInfo.cpp to prevent build break in Linux build.
Attachment #8452153 - Attachment is obsolete: true
Attachment #8452153 - Flags: review?(vyang)
Attachment #8452233 - Flags: review?(vyang)
(Assignee)

Comment 48

4 years ago
Created attachment 8452234 [details] [diff] [review]
Patch Part 3 v4: .webidl change to support ondeleted event. r=vyang,smaug.

re-base to prevent merge conflict.
Attachment #8452156 - Attachment is obsolete: true
Attachment #8452234 - Flags: review+
(Assignee)

Comment 49

4 years ago
Created attachment 8452235 [details] [diff] [review]
Patch Part 4 v4: Implementation of .webidl change.

Add MozMessageDeletedEvent into dom/events/test/test_all_synthetic_events.html.
Attachment #8452157 - Attachment is obsolete: true
Attachment #8452157 - Flags: review?(vyang)
Attachment #8452235 - Flags: review?(vyang)
Attachment #8452235 - Flags: review?(vyang) → review+
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.
Attachment #8452233 - Flags: review?(vyang) → review+
Attachment #8452160 - Flags: review?(vyang) → review+
(Assignee)

Comment 51

4 years ago
(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. :)
(Assignee)

Comment 52

4 years ago
Created attachment 8453525 [details] [diff] [review]
Patch Part 1: Define .idl/.ipdl for notifying Deleted Info. r=vyang

Update final patches with positive result in try server.
Attachment #8453525 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8452149 - Attachment is obsolete: true
(Assignee)

Comment 53

4 years ago
Created attachment 8453527 [details] [diff] [review]
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.
Attachment #8452233 - Attachment is obsolete: true
Attachment #8453527 - Flags: review+
(Assignee)

Comment 54

4 years ago
Created attachment 8453528 [details] [diff] [review]
Patch Part 3: .webidl change to support ondeleted event. r=vyang,smaug.

Update final patches with positive result in try server.
Attachment #8452234 - Attachment is obsolete: true
Attachment #8453528 - Flags: review+
(Assignee)

Comment 55

4 years ago
Created attachment 8453530 [details] [diff] [review]
Patch Part 4: Implementation of .webidl change. r=vyang,smaug.

Update final patches with positive result in try server.
Attachment #8452235 - Attachment is obsolete: true
Attachment #8453530 - Flags: review+
(Assignee)

Comment 56

4 years ago
Created attachment 8453531 [details] [diff] [review]
Patch Part 5: Notify deleted info after DB has been changed. r=vyang

Update final patches with positive result in try server.
Attachment #8452160 - Attachment is obsolete: true
Attachment #8453531 - Flags: review+
(Assignee)

Comment 57

4 years ago
Created attachment 8453532 [details] [diff] [review]
Patch Part 6: Add Marionette Test Case to Verify ondeleted Event. r=vyang

Update final patches with positive result in try server.
Attachment #8452162 - Attachment is obsolete: true
Attachment #8453532 - Flags: review+
(Assignee)

Comment 58

4 years ago
Created attachment 8453533 [details] [diff] [review]
[2.0] Patch Part 1: Define .idl/.ipdl for notifying Deleted Info. r=vyang, a=2.0+

Rebase for 2.0 branch.
Attachment #8453533 - Flags: review+
(Assignee)

Comment 59

4 years ago
Created attachment 8453534 [details] [diff] [review]
[2.0] Patch Part 2: Add implementation of .idl/.ipdl. r=vyang, a=2.0+

rebase for 2.0 branch
Attachment #8453534 - Flags: review+
(Assignee)

Comment 60

4 years ago
Created attachment 8453535 [details] [diff] [review]
[2.0] Patch Part 3: .webidl change to support ondeleted event. r=vyang,smaug,a=2.0+

rebase for 2.0 branch.
Attachment #8453535 - Flags: review+
(Assignee)

Comment 61

4 years ago
Created attachment 8453536 [details] [diff] [review]
[2.0] Patch Part 4: Implementation of .webidl change. r=vyang,smaug, a=2.0+

rebase for 2.0 branch
Attachment #8453536 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8453535 - Attachment description: [2.0]Patch Part 3: .webidl change to support ondeleted event. r=vyang,smaug.,a=2.0+ → [2.0] Patch Part 3: .webidl change to support ondeleted event. r=vyang,smaug,a=2.0+
(Assignee)

Comment 62

4 years ago
Created attachment 8453537 [details] [diff] [review]
[2.0] Patch Part 5: Notify deleted info after DB has been changed. r=vyang, a=2.0+

Rebase for 2.0 branch.
Attachment #8453537 - Flags: review+
(Assignee)

Comment 63

4 years ago
Created attachment 8453538 [details] [diff] [review]
[2.0] Patch Part 6: Add Marionette Test Case to Verify ondeleted Event. r=vyang, a=2.0+

Rebase for 2.0 branch.
Attachment #8453538 - Flags: review+
(Assignee)

Comment 64

4 years ago
tryserver is green:
https://tbpl.mozilla.org/?tree=Try&rev=9b5fa22a17aa
Keywords: checkin-needed
Created attachment 8453644 [details] [diff] [review]
Patch Part 4: Implementation of .webidl change. r=vyang,smaug. : v2

Rebased after bug 1035394.
Attachment #8453530 - Attachment is obsolete: true
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
status-b2g-v2.0: --- → affected
Depends on: 1040230
(Assignee)

Updated

3 years ago
Blocks: 1181466
You need to log in before you can comment on or make changes to this bug.