Closed Bug 810099 Opened 9 years ago Closed 8 years ago

B2G MMS: support onretrieving event

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: vicamo, Assigned: ctai)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(5 files, 16 obsolete files)

4.02 KB, patch
Details | Diff | Splinter Review
19.20 KB, patch
Details | Diff | Splinter Review
4.03 KB, patch
Details | Diff | Splinter Review
19.31 KB, patch
Details | Diff | Splinter Review
3.62 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
Notify user whenever a retrieval is on going.
Blocks: b2g-mms
Summary: B2G MMS: support onretrieve event → B2G MMS: support onretrieving event
No longer blocks: 804679
this is extracted from carrier requirement
blocking-b2g: --- → leo?
We can't block unless this is a P1 requirement. What's the user-story ID that this supports?
Then let's wait for a user story first.
blocking-b2g: leo? → ---
Depends on: 844431
No longer depends on: b2g-mms-dom-api
Attached patch Part 2/2: Patch v1.0 (obsolete) — Splinter Review
Comment on attachment 747181 [details] [diff] [review]
Part 2/2: Patch v1.0

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

::: dom/mms/src/ril/MmsService.js
@@ +1466,5 @@
>  
>        let url =  aMessageRecord.headers["x-mms-content-location"].uri;
>        // For X-Mms-Report-Allowed
>        let wish = aMessageRecord.headers["x-mms-delivery-report"];
> +      Services.obs.notifyObservers(aDomMessage, kSmsReceivedObserverTopic, null);

It seems I can't get domMessage in current design.....Can Gene and Vicamo provide some suggestion?
Attachment #747181 - Flags: feedback?(gene.lian)
Attachment #747179 - Flags: feedback?(vyang)
Attachment #747181 - Flags: feedback?(vyang)
We might need a new function |getDomMessageById| in MobileMessageDatabaseService.js.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> Comment on attachment 747181 [details] [diff] [review]
> Part 2/2: Patch v1.0
> 
> Review of attachment 747181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mms/src/ril/MmsService.js
> @@ +1466,5 @@
> >  
> >        let url =  aMessageRecord.headers["x-mms-content-location"].uri;
> >        // For X-Mms-Report-Allowed
> >        let wish = aMessageRecord.headers["x-mms-delivery-report"];
> > +      Services.obs.notifyObservers(aDomMessage, kSmsReceivedObserverTopic, null);
> 
> It seems I can't get domMessage in current design.....Can Gene and Vicamo
> provide some suggestion?
BTW, this bug might be needed for bug Bug 868218.
OK, I think I can use getMessage to getDomMessage.
But that seems be wierd to use getMessage in getMessageRecordById's callback.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #7)
> We might need a new function |getDomMessageById| in
> MobileMessageDatabaseService.js.
> 
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> > Comment on attachment 747181 [details] [diff] [review]
> > Part 2/2: Patch v1.0
> > 
> > Review of attachment 747181 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/mms/src/ril/MmsService.js
> > @@ +1466,5 @@
> > >  
> > >        let url =  aMessageRecord.headers["x-mms-content-location"].uri;
> > >        // For X-Mms-Report-Allowed
> > >        let wish = aMessageRecord.headers["x-mms-delivery-report"];
> > > +      Services.obs.notifyObservers(aDomMessage, kSmsReceivedObserverTopic, null);
> > 
> > It seems I can't get domMessage in current design.....Can Gene and Vicamo
> > provide some suggestion?
Attached patch Part 2/2: Patch v1.1 (obsolete) — Splinter Review
Attachment #747181 - Attachment is obsolete: true
Attachment #747181 - Flags: feedback?(vyang)
Attachment #747181 - Flags: feedback?(gene.lian)
Attached patch Part 2/2: Patch v1.2 (obsolete) — Splinter Review
Attachment #747201 - Attachment is obsolete: true
Attachment #747215 - Flags: feedback?(vyang)
Attachment #747215 - Flags: feedback?(gene.lian)
Comment on attachment 747215 [details] [diff] [review]
Part 2/2: Patch v1.2

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

Addressing comment #9. I think we could have 2 solutions to solving that:

1. Expose .createDomMessageFromRecord() by nsIRilMobileMessageDatabaseService.idl so that you can call it to generate a DOM message in MmsService.js.
2. Let the nsIRilMobileMessageDatabaseRecordCallback carry one more DOM message:

   interface nsIRilMobileMessageDatabaseRecordCallback : nsISupports
   {
     void notify(in nsresult aRv, in jsval aMessageRecord, in nsISupports aDomMessage);
   };
   
   where the aDomMessage can be generated within the MobileMessageDatabaseService.js.

Both work for me but I'd prefer solution #2.

::: dom/mms/src/ril/MmsService.js
@@ +1470,5 @@
> +      gMobileMessageDatabaseService.getMessage(id, {
> +        notifyMessageGot: function notifyMessageGot(aDomMessage) {
> +          Services.obs.notifyObservers(aDomMessage, kSmsReceivingObserverTopic, null);
> +        },
> +        notifyGetMessageFailed: function notifyGetMessageFailed(aRv, aDomMessage) {

This is wrong. The only parameter is |aRv|. Also, you cannot proceed to do the retrieving process. You need to call:

aRequest.notifyGetMessageFailed(aRv);

and return the .retrieve(...). Anyway, please see my suggestion on the top of the review. You don't need these codes.
Attachment #747215 - Flags: feedback?(gene.lian)
Attached patch Part 1/2: Interface v1.1 (obsolete) — Splinter Review
Attachment #747179 - Attachment is obsolete: true
Attachment #747179 - Flags: feedback?(vyang)
Attached patch Part 2/2: Patch v1.3 (obsolete) — Splinter Review
Attachment #747215 - Attachment is obsolete: true
Attachment #747215 - Flags: feedback?(vyang)
Assignee: nobody → ctai
Attached patch Part 1/2: Interface v1.2 (obsolete) — Splinter Review
Attachment #747722 - Attachment is obsolete: true
Attached patch Part 2/2: Patch v1.4 (obsolete) — Splinter Review
Attachment #747723 - Attachment is obsolete: true
Attachment #747727 - Flags: feedback?(gene.lian)
Attachment #747726 - Flags: feedback?(gene.lian)
Attachment #747726 - Flags: feedback?(gene.lian) → feedback+
Comment on attachment 747727 [details] [diff] [review]
Part 2/2: Patch v1.4

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

Looks good!

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1290,4 @@
>            return;
>          }
> +        let domMessage = self.createDomMessageFromRecord(messageRecord);
> +        aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR, messageRecord, domMessage);

Fold this a bit:

aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR,
                 messageRecord, domMessage);

@@ +1336,4 @@
>            return;
>          }
> +        let domMessage = self.createDomMessageFromRecord(messageRecord);
> +        aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR, messageRecord, domMessage);

Fold this a bit:

aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR,
                 messageRecord, domMessage);

@@ +1355,5 @@
>     */
>  
>    getMessage: function getMessage(aMessageId, aRequest) {
>      if (DEBUG) debug("Retrieving message with ID " + aMessageId);
>      let self = this;

You don't need |self|.

@@ +1357,5 @@
>    getMessage: function getMessage(aMessageId, aRequest) {
>      if (DEBUG) debug("Retrieving message with ID " + aMessageId);
>      let self = this;
>      let notifyCallback = {
> +      notify: function notify(aRv, aMessageRecord, domMessage) {

s/domMessage/aDomMessage

@@ +1363,1 @@
>            aRequest.notifyMessageGot(domMessage);

s/domMessage/aDomMessage
Attachment #747727 - Flags: feedback?(gene.lian) → feedback+
Comment on attachment 747727 [details] [diff] [review]
Part 2/2: Patch v1.4

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

Wait! It's impossible to work without IPC. Please see SmsParent.cpp.
Attachment #747727 - Flags: feedback+ → feedback-
Attached patch Part 2/2: Patch v1.5 (obsolete) — Splinter Review
Attachment #747727 - Attachment is obsolete: true
Attachment #748296 - Flags: feedback?(gene.lian)
Comment on attachment 748296 [details] [diff] [review]
Part 2/2: Patch v1.5

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

Overall, it looks good. I'll then look into the IPC details when you come back with a formal patch that is verified on the device.
Attachment #748296 - Flags: feedback?(gene.lian) → feedback+
Attached patch Part 2/2: Patch v1.6 (obsolete) — Splinter Review
Attachment #748296 - Attachment is obsolete: true
Attachment #747726 - Flags: review?(gene.lian)
Attachment #747726 - Flags: review?(gene.lian) → review?(vyang)
Attachment #749734 - Flags: review?(gene.lian)
Attachment #749734 - Flags: review?(vyang)
Attachment #747726 - Flags: review?(gene.lian)
Verified by adding onreceiving callback function in message_manager.js in Gaia AP.
Can received this event in Gaia side.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #21)
> Created attachment 749734 [details] [diff] [review]
> Part 2/2: Patch v1.6
Comment on attachment 747726 [details] [diff] [review]
Part 1/2: Interface v1.2

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

We're adding "onretrieving" event, not "onreceiving", right?
Attachment #747726 - Flags: review?(vyang)
Attachment #747726 - Flags: review?(gene.lian)
Now bug 868218 query the delivery status to know retrieving or not. Support onretrieving event let developer can add callback function to know retrieving or not.
Block leo+ bug 868218.
blocking-b2g: --- → leo?
Depends on: 868218
Attached patch Part 1/2: Interface v1.3 (obsolete) — Splinter Review
Change to onretrieving event.
Attachment #747726 - Attachment is obsolete: true
Attached patch Part 2/2: Patch v1.7 (obsolete) — Splinter Review
Change to onretrieving event.
Attachment #749734 - Attachment is obsolete: true
Attachment #749734 - Flags: review?(vyang)
Attachment #749734 - Flags: review?(gene.lian)
Attachment #749790 - Flags: review?(vyang)
Attachment #749790 - Flags: review?(gene.lian)
Attached patch Part 2/2: Patch v1.8 (obsolete) — Splinter Review
Attachment #749792 - Attachment is obsolete: true
Attachment #749794 - Flags: review?(vyang)
Attachment #749794 - Flags: review?(gene.lian)
Patches look good, but we had a conclusion to expose .createDomMessageFromRecord(...). Right?
blocking-b2g: leo? → leo+
Blocks: 868218
No longer depends on: 868218
After talk with Vicamo, he think current implmentation is OK.
No need to expose |createDomMessageFromRecord|.

(In reply to Gene Lian [:gene] from comment #29)
> Patches look good, but we had a conclusion to expose
> .createDomMessageFromRecord(...). Right?
:gnarf, according the conversation in Portland MMS work week, you said you might need this event "onRetrieving". This bug is for supporting onRetrieving while you call retrieveMMS. Could you please help to confirm this need?
Flags: needinfo?(gnarf37)
Attachment #749790 - Flags: review?(vyang)
Attachment #749790 - Flags: review?(gene.lian)
Attachment #749790 - Flags: review+
Comment on attachment 749794 [details] [diff] [review]
Part 2/2: Patch v1.8

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

::: dom/mms/src/ril/MmsService.js
@@ +1067,3 @@
>      // TODO: bug 809832 - support customizable max incoming/outgoing message size.
> +    Services.obs.notifyObservers(aDomMessage, kSmsRetrievingObserverTopic, null);
> +    let transaction = new RetrieveTransaction(aContentLocation);

nit: please have:

  // TODO: bug 809832 ...

  // Notifying observers an MMS message is retrieving.
  Services.obs.notifyObservers(aDomMessage, kSmsRetrievingObserverTopic, null);

  let transaction ...

I'd always want the reader know there are several independent small steps in this function.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1290,5 @@
>            return;
>          }
> +        let domMessage = self.createDomMessageFromRecord(messageRecord);
> +        aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR,
> +                         messageRecord, domMessage);

We never need a DOM message instance when calling `getMessageRecordByTransactionId`, and actually we don't need that message record, either.  I'd rather have some more comments on that callback interface and save extra work to do here.
Attachment #749794 - Flags: review?(vyang)
Attachment #749794 - Flags: review?(gene.lian)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
>   // TODO: bug 809832 ...

Just found bug 809832 has been resolved, please also removes this line.  Thank you :)
Attached patch Part 1/2: Interface v1.4 (obsolete) — Splinter Review
Add some comments in nsIRilMobileMessageDatabaseRecordCallback.
Attachment #749790 - Attachment is obsolete: true
Attachment #752068 - Attachment is obsolete: true
Attached patch Part 2/2: Patch v1.9 (obsolete) — Splinter Review
Attachment #749794 - Attachment is obsolete: true
Attachment #752071 - Flags: review?(vyang)
Comment on attachment 752071 [details] [diff] [review]
Part 2/2: Patch v1.9

Modified according to comment 32.
I worked around this in my not-downloaded patch, but I would actually prefer to see it in the API.  I think it makes more sense to have it in the API, anytime any message changes for any reason, there should be an event to listen to.

For reference, bug 868218 is the not-downloaded patch - currently whenever you tap on a not downloaded, i call retrieve and manually have to update the status
Flags: needinfo?(gnarf37)
Thanks for your supplement. :)

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #38)
> I worked around this in my not-downloaded patch, but I would actually prefer
> to see it in the API.  I think it makes more sense to have it in the API,
> anytime any message changes for any reason, there should be an event to
> listen to.
> 
> For reference, bug 868218 is the not-downloaded patch - currently whenever
> you tap on a not downloaded, i call retrieve and manually have to update the
> status
Comment on attachment 752070 [details] [diff] [review]
Part 1/2: Interface v1.4

Could you please do a superreview for this DOM API change? Gaia developers need this event for their development (see comment 38). In this patch, we add onretrieving event to notify Gaia AP when gecko start to actually retrieving the MMS. Thanks.
Attachment #752070 - Flags: superreview?(mounir)
Comment on attachment 752071 [details] [diff] [review]
Part 2/2: Patch v1.9

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

Thank you :)

::: dom/mms/src/ril/MmsService.js
@@ +27,5 @@
>  const kSmsSendingObserverTopic           = "sms-sending";
>  const kSmsSentObserverTopic              = "sms-sent";
>  const kSmsFailedObserverTopic            = "sms-failed";
>  const kSmsReceivedObserverTopic          = "sms-received";
> +const kSmsRetrievingObserverTopic         = "sms-retrieving";

nit: alignment.
Attachment #752071 - Flags: review?(vyang) → review+
Attached patch Part 2/2: Patch v1.10 (obsolete) — Splinter Review
Fix alignment.
Attachment #752071 - Attachment is obsolete: true
Comment on attachment 752070 [details] [diff] [review]
Part 1/2: Interface v1.4

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

Why can't we rely on retrieveMMs() returned request?
Attachment #752070 - Flags: superreview?(mounir)
:mounir, thanks for your question.
The reasons are below:
1. DomRequest only has two function: onsuccess and onerror.
2. The purpose of |retrieveMMs| is download MMS entity form MMS proxy in manually download mode. So the onsuccess is for the event we successfully download the message. onerror is for any error in that process.
3. The requirement of onretrieving event is Gaia developer want to listen the message status changed to retrieving. It is neither success nor error. So I don't think we can only rely on the dom request.

(In reply to Mounir Lamouri (:mounir) from comment #43)
> Comment on attachment 752070 [details] [diff] [review]
> Part 1/2: Interface v1.4
> 
> Review of attachment 752070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why can't we rely on retrieveMMs() returned request?
Attachment #752070 - Flags: superreview?(mounir)
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #44)
> :mounir, thanks for your question.
> The reasons are below:
> 1. DomRequest only has two function: onsuccess and onerror.
> 2. The purpose of |retrieveMMs| is download MMS entity form MMS proxy in
> manually download mode. So the onsuccess is for the event we successfully
> download the message. onerror is for any error in that process.
> 3. The requirement of onretrieving event is Gaia developer want to listen
> the message status changed to retrieving. It is neither success nor error.
> So I don't think we can only rely on the dom request.

Isn't a message moving to 'retrieving' state when retriveMMS() is called on it?
Attachment #752070 - Flags: superreview?(mounir)
The current implementation of retriveMMS is below:
1. In nsIDOMMobileMessageManager.idl has nsIDOMDOMRequest retrieveMMS(in long id);

2. MobileMessageManager::RetrieveMMS will new a object "MobileMessageCallback" which take nsIDOMDOMRequest as argument.

3. nsIMmsService.idl has void retrieve(in long id, in nsIMobileMessageCallback request);
MobileMessageManager::RetrieveMMS pass the object of "MobileMessageCallback" into |retrieve| in MmsService.js

4. When |retrieve| in MmsService.js is called. It will change the delivery status from manual to pending. In the mean time should notify the Gaia AP this change. Should we trigger the event onsuccess/onerror of dom request in this moment?

5.In |retrieve|, it will call MobileMessageCallback::notifyGetMessageFailed when error occurred. MobileMessageCallback::notifyGetMessageFailed will call mDOMRequest->FireError eventually. Otherwise, we should successfully retrieved the message. It will call MobileMessageCallback::notifyMessageGot which call mDOMRequest->FireSuccess in the end.

I have no idea how to moving the 'retrieving' state in this flow. Should we trigger the event onsuccess/onerror of dom request in step 4. Or, could you please give me some suggestions? Thanks.



(In reply to Mounir Lamouri (:mounir) from comment #45)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #44)
> > :mounir, thanks for your question.
> > The reasons are below:
> > 1. DomRequest only has two function: onsuccess and onerror.
> > 2. The purpose of |retrieveMMs| is download MMS entity form MMS proxy in
> > manually download mode. So the onsuccess is for the event we successfully
> > download the message. onerror is for any error in that process.
> > 3. The requirement of onretrieving event is Gaia developer want to listen
> > the message status changed to retrieving. It is neither success nor error.
> > So I don't think we can only rely on the dom request.
> 
> Isn't a message moving to 'retrieving' state when retriveMMS() is called on
> it?
Flags: needinfo?(mounir)
Sorry, I do not understand the previous comment. After reading the code, it seems that calling retrieveMMS() on m-c will do an HTTP GET on a resource. Is that correct? When is the message being on the retrieving state? When the HTTP GET starts? Is there other situations when a message can be retrieving?
Flags: needinfo?(mounir)
Try to answer your question.
1. retrieveMMS do an HTTP GET on an uri(content location).
2. Before calling retrieveMMS, the state of an incoming MMS notification in Manual mode is delivery=DELIVERY_NOT_DOWNLOADED and delivery_status=DELIVERY_STATUS_MANUAL.
3. The |retrieveMMS| checks some status and changes the delivery_status to DELIVERY_STATUS_PENDING. Then we need to notify the Messaging AP this status change. It is the moment of onretrieving event fired.
4. After that, retrieveMMS is trying to build a data connection and do a HTTP request "POST" on a content location.
5. If everything is good, we get the message and save the retrieved message and change the delivery=DELIVERY_RECEIVED and delivery_status =DELIVERY_STATUS_SUCCESS. Then notify the Messaging AP the retrieved message via aRequest.notifyMessageGot(domMessage).
6. The flow of aRequest.notifyMessageGot is MobileMessageCallback::notifyMessageGot -> mDOMRequest->FireSuccess -> request.onsuccess in |retrieveMMS| of gaia/apps/sms/js/thread_ui.js. 

Do you mean we should use notifyMessageGot to notify in step 3? The current patch uses Services.obs.notifyObservers(aDomMessage, kSmsRetrievingObserverTopic, null) in step 3.



(In reply to Mounir Lamouri (:mounir) from comment #47)
> Sorry, I do not understand the previous comment. After reading the code, it
> seems that calling retrieveMMS() on m-c will do an HTTP GET on a resource.
> Is that correct? When is the message being on the retrieving state? When the
> HTTP GET starts? Is there other situations when a message can be retrieving?
Flags: needinfo?(mounir)
Whiteboard: RN6/7
After discussing with Chia-Hung my feeling is that this isn't needed but if you guys want it to make your life easier, feel free to do so. This API isn't going to be exposed to anything but the Messaging application anyway.

No need to ask for a sr.
Flags: needinfo?(mounir)
Keywords: checkin-needed
Whiteboard: RN6/7
Rebased...
Attachment #752550 - Attachment is obsolete: true
For uplift to b2g18.
For uplift to b2g18.
https://hg.mozilla.org/mozilla-central/rev/a3fcf6de6388
https://hg.mozilla.org/mozilla-central/rev/457c9f9544bd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 757785 [details] [diff] [review]
Follow-up-810099-v1.0

Fix the rebased error.
Attachment #757785 - Flags: review?(gene.lian)
Introduce new bug during re-based. Has a follow-up fix "Follow-up-810099-v1.0" to fix it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #757785 - Flags: review?(gene.lian) → review+
(In reply to Gene Lian [:gene] from comment #60)
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8d75614010e4

Unless the patch is v1.1hd-specific or specifically requested, please don't land on v1.1hd. I am merging patches from b2g18 over to it, which makes for cleaner history and avoid missing anything. Double-landing complicates it.
https://hg.mozilla.org/mozilla-central/rev/83b67d255bf3
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen [:RyanVM] from comment #61)
> (In reply to Gene Lian [:gene] from comment #60)
> > https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8d75614010e4
> 
> Unless the patch is v1.1hd-specific or specifically requested, please don't
> land on v1.1hd. I am merging patches from b2g18 over to it, which makes for
> cleaner history and avoid missing anything. Double-landing complicates it.

Got it! Sorry for confusing you. I won't do this next time.
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.