1008812 - [tarako] Messages keep on MMS sending state for long time

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Peipei Cheng (needinfo if you need my action)]

According to [e-mail], current MMS sending process will automatically retry if there is a mms sending failure. It will retry every 5 minutes, maximum for 3 times. In this case user will see MMS keeps on sending status for 15 minutes if there is network problem, and then a failure.

It would be better that we show the failure first and then retry. So user could get acknowledge of MMS sending failure earlier and will not get confused by the sending status.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> may 13)]

I disagree with this but this is a UX question, so NI Omega on this.

Comment 2

[Omega Feng [:Omega] [:馮於懋]]

I think showing the failure first and then retrying is a good idea, so that user can be aware of it and retry manually right after sending failed.

Comment 3

[Ming]

(In reply to Omega Feng [:Omega] from comment #2)
> I think showing the failure first and then retrying is a good idea, so that
> user can be aware of it and retry manually right after sending failed.

Yes , i prefer it to act as that too . Pls consider to improve the user experience.

Comment 4

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

DOMRequest, or even Promise that we'll move on, has only either one success event or one error event.  Either of them terminates the transaction.  So, this means:

  1) var req = mm.send(),
  2) req.onerror raises, so req is no longer useful,
  3) listen to mm.onsent/onfailed for further state changes.

Could do, but an user can't easily know if it's still waiting for further retry.  And we may have multiple onfailed events dispatched for one single transaction.  Sounds even more weird.

Comment 5

[Ming]

How about to retry immediately, or give waiting timer a more less value?

Comment 6

[Ming]

And any reason for us to wait a 5 minutes to have another try?

Comment 7

[Steven Yang [:styang]]

We won't change the design at current stage. put it into backlog for further review in the future.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> DOMRequest, or even Promise that we'll move on, has only either one success
> event or one error event.  Either of them terminates the transaction.  So,
> this means:
> 
>   1) var req = mm.send(),
>   2) req.onerror raises, so req is no longer useful,
>   3) listen to mm.onsent/onfailed for further state changes.

This is a good opportunity to say something I have in my mind for some time now.

I think we should have the exact same events both on MessageManager and on the req object MM.send and sendMMS returns.

That means we should be able to add a req.onsending handler, for example.

The difference with MM's handler is that it will be specific to this request. It's super useful once we start thinking about activities: we have several windows, but only one of them is sending a message, and so should display an error message (for example). Both windows need to add the message to the thread though (and that's why mm.onsending is still useful).


 
> Could do, but an user can't easily know if it's still waiting for further
> retry.  And we may have multiple onfailed events dispatched for one single
> transaction.  Sounds even more weird.


Yep, I don't know the ideal API either.

Comment 9

[Wei Gao (Spreadtrum)]

Attachment: shorten the mms wait time.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):None
[User impact] if declined:Shorten the wait time to give a better user experience.
[Testing completed]:Pass
[Risk to taking this patch] (and alternatives if risky):None
[String changes made]:No

I upload one patch to shorten the waiting time of resending message. 

As the wait time is too long to give customers a good user experience. I shorten the time and make the wait time a ladder increase just like the strategy of native android.

After the first failure send, it will wait 10s to resend it. Similarly, the second wait time extended to 1 minute. At last, the third wait time is 3 minutes.

Please help to review, thanks.

Comment 10

[James Zhang (Spreadtrum)]

Comment on attachment 8423160 [details] [diff] [review]
shorten the mms wait time.patch

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

::: modules/libpref/src/init/all.js
@@ +4548,1 @@
>  

please follow dom.mms.retrievalRetryIntervals style

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8423160 [details] [diff] [review]
shorten the mms wait time.patch

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

Redirecting the review to Vicamo since he owns this code.

Comment 12

[Wei Gao (Spreadtrum)]

Attachment: shorten the mms wait time.New.patch

I upload the file "shorten the mms wait time.New.patch" again, for the previous one is not as specification as the native ffos code.

Please have a review of it.

Thanks.

Comment 13

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

Comment on attachment 8423513 [details] [diff] [review]
shorten the mms wait time.New.patch

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

Thank you :)

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1243,5 @@
>  
>            if (this.timer == null) {
>              this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>            }
> +       

nit: trailing white spaces.

Comment 14

[Wei Gao (Spreadtrum)]

Attachment: shorten_the_mms wait_time_OK.patch

Comment 15

[Wei Gao (Spreadtrum)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> Comment on attachment 8423513 [details] [diff] [review]
> shorten the mms wait time.New.patch
> 
> Review of attachment 8423513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you :)
> 
> ::: dom/mobilemessage/src/gonk/MmsService.js
> @@ +1243,5 @@
> >  
> >            if (this.timer == null) {
> >              this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> >            }
> > +       
> 
> nit: trailing white spaces.

I have deleted the trailing white spaces in shorten_the_mms wait_time_OK.patch, Thanks for your review very much.

Please help to merge this shorten_the_mms wait_time_OK.patch to 1.3t

Thank you for your kindly help once again.

Comment 16

[[:fabrice] Fabrice Desré]

Please land on master first, not directly on 1.3t. I'll uplift once on m-c

Comment 17

[Wei Gao (Spreadtrum)]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Please land on master first, not directly on 1.3t. I'll uplift once on m-c

Hi Fanrice,

 We do not have the GECKO merge permission, so could you please help to land on?
 Besides, the branch v1.4 also needs the patch , please handle it together.

 Thanks a lot.

Comment 18

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Fabrice, I think it does need to be 1.3t+ before upliting to 1.3t, and it needs an approval to land on 1.4 then, right?

Comment 19

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

This patch has no necessary info to be checked in.  Please see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for further information.

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Fabrice, I think it does need to be 1.3t+ before upliting to 1.3t, and it
> needs an approval to land on 1.4 then, right?

That's correct!

Comment 21

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Wei, there is action to be done before landing the patch, see comment 19 to change the patch, and comment 18 for the bug flagging.

Comment 22

[ying.xu]

try server
https://tbpl.mozilla.org/?tree=Try&rev=93ae452b27f4

Comment 23

[Wei Gao (Spreadtrum)]

Attachment: bug1008812_shorten_the_mms_wait_time

Hi Vicamo
This is the HG changeset patch. 
Please help to review it.
Thanks very much.

Also, I will thank xuying for his kindly help to convert this patch.
Many thanks.

Comment 24

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

needinfo for patch review please.

Comment 25

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

https://hg.mozilla.org/integration/b2g-inbound/rev/7ab1a02d403a

Comment 26

[Wei Gao (Spreadtrum)]

(In reply to Vicamo Yang [:vicamo][:vyang] (PTO May 17 - Jun 1)) from comment #25)
> https://hg.mozilla.org/integration/b2g-inbound/rev/7ab1a02d403a

Thanks for your kindly help.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ab1a02d403a

Comment 28

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Ryan, since it's backlogged this bug should be able to land on 1.3T as well.  I just marked the bug with the affected flag.  Is there a script that runs to push the patch to 1.3T?  Or do we have to manually push it?

Comment 29

[Julien Wajsberg [:julienw] (PTO -> may 13)]

It needs to be a blocker to be uplifted.

To uplift to v1.4 we'll need an approval too.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Once it has blocking status, devs are responsible for their own 1.3T uplifts.

Comment 31

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thanks, Ryan.

wei or vicamo, can you uplift to 1.3T please?  It was marked as backlog so we should be able to land it.
NI?ing bhavana as well so she is aware.

Comment 32

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #31)
> Thanks, Ryan.
> 
> wei or vicamo, can you uplift to 1.3T please?  It was marked as backlog so
> we should be able to land it.
> NI?ing bhavana as well so she is aware.

From: https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T

* Patches must have blocking-b2g:1.3T+ to land. 


I don't see how we can uplift it in the current state.

Comment 33

[Wei Gao (Spreadtrum)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #25)
> https://hg.mozilla.org/integration/b2g-inbound/rev/7ab1a02d403a

hi Vicamo

As this patch has been uplifted to master, can you help to uplift it to 1.4 as well?

Thanks.

Comment 34

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

(In reply to wei.gao from comment #33)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #25)
> hi Vicamo
> 
> As this patch has been uplifted to master, can you help to uplift it to 1.4
> as well?
> 
> Thanks.

blocking-b2g has not been set yet.

Comment 35

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Vicamo, even with 1.3t+ (not yet given), you'll need an approval request on the patch.

Comment 36

[Joe Cheng [:jcheng] (please needinfo)]

Hi Vicamo, is this patch low risk to land in 1.3T? would like to consider this bug for uplift if the changes are small and low risk. thanks

Comment 37

[bhavana bajaj [:bajaj]]

Waiting on more info from comment #36.

Comment 38

[Joe Cheng [:jcheng] (please needinfo)]

ni? kchang as well

Comment 39

[Ken Chang[:kenkai]]

As I knew, currently uplifting procedure for branches is that the patch provider should claim and make sure if his patches are good enough to uplift and request an approval after getting r+. 

Of cause, to have Vicamo's comments is a good way to know the risk of uplifting these patches. However, that should not block uplifting this patch. Vicamo is just a reviewer for this bug and have already given r+ for the patches of this bug. I would say the patch provider has to take major responsibility for his patches.

So according to current status of this bug, the next steps are that wei have to test his patches on 1.3T, and confirm his patches are good enough for uplifting. Then he can file an approval request and set the keyword, "checkin-needed", for this bug.

Comment 40

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Wei, can you test your patch for 1.3T per the last comment?

Comment 41

[Wei Gao (Spreadtrum)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #40)
> Wei, can you test your patch for 1.3T per the last comment?

Hi Naoki and Ken

Thanks for your comments.
I have test this patch on 1.3t and it can work normal. I just modified the sending time of mms, I think it is low risk for 1.3t.
I know how to commit pull request, but how can i set the keyword "checkin-needed"? I have not found this keyword.

Thanks.

Comment 42

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Hi wei, can you attach the 1.3T version of the patch to the bug please?  I placed checkin-needed in the keyword section for this bug.  Thanks!

Comment 43

[Wei Gao (Spreadtrum)]

Attachment: shorten_mms_wait_time_v1.3t.patch

Hi Naoki

This is the patch of 1.3T version.
Please help to set "checkin-needed" in the keyword section.

Thanks.

Comment 44

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Joe, can we get approval for blocking and have this uplifted please?

Comment 45

[Ryan VanderMeulen [:RyanVM]]

checkin-needed is intended for patches that are ready to be landed. Given the lack of blocking status, that isn't the case here. Removing. Also note that 1.3T+ doesn't grant auto-approval for 1.4 uplift, so you'll need to specifically request b2g30 uplift on the patch if you want it landed there as well.

Comment 46

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I see.  Thanks, Ryan.  We're looking to uplift this for 1.3T+; I am not sure about 1.4.

Comment 47

[bhavana bajaj [:bajaj]]

wei, please help with 1.3T landings.

Comment 48

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #47)
> wei, please help with 1.3T landings.

or NI vicamo/Ken in case you have access issues.

Comment 49

[Wei Gao (Spreadtrum)]

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #47)
> wei, please help with 1.3T landings.

I am glad to do it, but I don't know how to land gecko.
Should I refer to Comment 19 or recommit a pull request?

Thanks.

Comment 50

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b0e4f54a45a1

Comment 51

[Wei Gao (Spreadtrum)]

(In reply to Fabrice Desré [:fabrice] from comment #50)
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b0e4f54a45a1

Thanks a great.

Comment 52

[Wei Gao (Spreadtrum)]

(In reply to Fabrice Desré [:fabrice] from comment #50)
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b0e4f54a45a1

Hi Fabrice

This bug also affect v1.4. Can this be landed on v1.4 too?
Thanks a great.

Comment 53

[bhavana bajaj [:bajaj]]

(In reply to Wei Gao (Spreadtrum) from comment #52)
> (In reply to Fabrice Desré [:fabrice] from comment #50)
> > https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b0e4f54a45a1
> 
> Hi Fabrice
> 
> This bug also affect v1.4. Can this be landed on v1.4 too?
> Thanks a great.

Please request gecko approval-mozilla-b2g30 on this to consider 1.4 landing.

Comment 54

[Wei Gao (Spreadtrum)]

Comment on attachment 8440212 [details] [diff] [review]
shorten_mms_wait_time_v1.3t.patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): mms
User impact if declined: It will affect user experience
Testing completed: yes
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch:no

Hi bhavana

This patch is also good for v1.4.
Is this what we need?
Thanks.

Comment 55

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Wei Gao (Spreadtrum) from comment #54)
> Bug caused by (feature/regressing bug #): mms

Do you have a reference to the bug in which the feature was added or the regression was introduced?

> User impact if declined: It will affect user experience

Can you please elaborate? How will it affect the user experience?

> Testing completed: yes

Can you please elaborate? How have you tested the patch?

Comment 56

[bhavana bajaj [:bajaj]]

Thanks Wei! this is the exact approval you should be using. waiting for further clarification on :lmandel's comment #55 to act on the approval.

Comment 57

[Wei Gao (Spreadtrum)]

(In reply to Lawrence Mandel [:lmandel] from comment #55)

I am sorry for my careless. I will commit again.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):-
[User impact] if declined: As the duration of sending mms is too long, if the mms can't be sent out, the user will have to wait for a long time (more than 15 minutes) to get the result, I think the user will be very disappointed for it. As for user, the number of resend mms should not be changed, but the wait time between retransmission should be shorted.
[Testing completed]:yes, for I used tarako(6821) and dolphin(7715) to test it, and it can reduce a lot of unnecessary waiting time (less than 5 minutes).
[Risk to taking this patch] (and alternatives if risky):I think there is no risk for our system. As I only short the wait time of mms, there is no other change.
[String changes made]:no

Thanks.

Comment 58

[bhavana bajaj [:bajaj]]

(In reply to Wei Gao (Spreadtrum) from comment #57)
> (In reply to Lawrence Mandel [:lmandel] from comment #55)
> 
> I am sorry for my careless. I will commit again.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):-
> [User impact] if declined: As the duration of sending mms is too long, if
> the mms can't be sent out, the user will have to wait for a long time (more
> than 15 minutes) to get the result, I think the user will be very
> disappointed for it. As for user, the number of resend mms should not be
> changed, but the wait time between retransmission should be shorted.
> [Testing completed]:yes, for I used tarako(6821) and dolphin(7715) to test
> it, and it can reduce a lot of unnecessary waiting time (less than 5
> minutes).
> [Risk to taking this patch] (and alternatives if risky):I think there is no
> risk for our system. As I only short the wait time of mms, there is no other
> change.
> [String changes made]:no
> 
> Thanks.

Thanks for the info. I've approved, the patch, this will get landed soon.

Comment 59

[Wei Gao (Spreadtrum)]

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #58)
> Thanks for the info. I've approved, the patch, this will get landed soon.

Thanks for your kindly help.
May you the best wish.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/f5ca63e3b37d