Closed Bug 989600 Opened 6 years ago Closed 5 years ago

[Tarako] Message app MUST save draft when killed by LMK

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, critical)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 wontfix, b2g-v2.0 wontfix)

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix

People

(Reporter: james.zhang, Assigned: julienw)

References

Details

(Whiteboard: [partner-blocker][sprd294824][tarako_only])

Attachments

(1 file, 7 obsolete files)

When MMS or Email is killed by LMK, user maybe very angry if no draft saving.
There are a lot of similar bug in our bugzilla.

http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=294298
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=294824
blocking-b2g: --- → 1.3T?
Flags: needinfo?(fabrice)
Flags: needinfo?(ehung)
Depends on: 987022
ni-ing Julien that worked on the sms/mms drafts, and Andrew for email.
Flags: needinfo?(felash)
Flags: needinfo?(fabrice)
Flags: needinfo?(bugmail)
e-mail saves on visibilitychange.  Assuming the call triggers a prompt visibilitychange and we have a second or two to process the notification before we get killed, we're already good to go.  In the event that an incoming call is an immediate death sentence for the e-mail app, then we need to more aggressively be autosaving at some interval if changes have been made with the expectation that some small amount of user typing may be lost.
Flags: needinfo?(bugmail)
(In reply to Fabrice Desré [:fabrice] from comment #2)
> ni-ing Julien that worked on the sms/mms drafts, and Andrew for email.

Fabrice, draft is not enough, we have other bug about saving mms/email configure.
(In reply to Andrew Sutherland (:asuth) from comment #3)
> e-mail saves on visibilitychange.  Assuming the call triggers a prompt
> visibilitychange and we have a second or two to process the notification
> before we get killed, we're already good to go.  In the event that an
> incoming call is an immediate death sentence for the e-mail app, then we
> need to more aggressively be autosaving at some interval if changes have
> been made with the expectation that some small amount of user typing may be
> lost.

That's exactly the same for Messages.

I wonder if we could not save on the "idle" event as well?
Flags: needinfo?(felash)
(In reply to James Zhang from comment #4)
> Fabrice, draft is not enough, we have other bug about saving mms/email
> configure.

Can you elaborate on this?  I created an account on the spreadtrum bugzilla in order to see the bugs you linked to in comment 1, but both of them are restricted so that my account (same e-mail address as this one).  (I'm also not able to view any products if I click on "browse" on bugzilla.spreadtrum.com.  Performing a search of all open bugs likewise nets me no bugs.)
Flags: needinfo?(james.zhang)
Er, half-completed edit there.  To re-state: I can't see the bugs you linked on bugzilla.spreadtrum.com or any bugs at all.  Please provide more information on this bug.
(In reply to Andrew Sutherland (:asuth) from comment #6)
> (In reply to James Zhang from comment #4)
> > Fabrice, draft is not enough, we have other bug about saving mms/email
> > configure.
> 
> Can you elaborate on this?  I created an account on the spreadtrum bugzilla
> in order to see the bugs you linked to in comment 1, but both of them are
> restricted so that my account (same e-mail address as this one).  (I'm also
> not able to view any products if I click on "browse" on
> bugzilla.spreadtrum.com.  Performing a search of all open bugs likewise nets
> me no bugs.)

Our bugzilla only open permission for "@mozilla.com" account now.
see
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=294298
Email setting up account, incoming call, email is killed by LMK and the configure is not saved.
Flags: needinfo?(james.zhang)
(In reply to Andrew Sutherland (:asuth) from comment #7)
> Er, half-completed edit there.  To re-state: I can't see the bugs you linked
> on bugzilla.spreadtrum.com or any bugs at all.  Please provide more
> information on this bug.

Hi Andrew, let me do the translation for you. (I bet you can't read Chinese even you can successfully see the content. Some people are helping on filing corresponding bugs here, so don't worry.)

The first one is saying: An incoming call while Email app prompting "setting up account" message causes email configuration failed. (bug 989762 filed)

This bug is filed for the second one, so it's saying the same thing.
Flags: needinfo?(ehung)
Flags: needinfo?(bugmail)
On Fugu, I saw that simply pressing home, the Messages app was killed before it could save the draft. So maybe we need to save the draft a little more often: on idle, and (for example) every 5 characters, and when the user adds/remove an attachment.
Note that the SMS app does not have the Draft feature in 1.3 at all, so we won't be able to do this in 1.3 anyway.
In e-mail, we do save the draft as part of the attaching operation.

If the home button kills the app that quickly then it sounds like one or more of the following is true:
- the process killer is being way too aggressive
- effort might be better spent on reducing memory footprints so that the process killer does not need to be as aggressive
- the home screen's home button handler needs to rejigger things so that it sends the visibility notification and allows time for it to be processed before doing anything that would cause the app to be killed.

This sounds like it probably wants to be a dev-b2g or dev-gaia thread with someone Tarako-focused laying out the process life-cycle there, where there memory is going, when/why processes are being aggressively killed, etc.
Flags: needinfo?(bugmail)
Yes, I agree, since the visibilitychange event arrives asynchronously and quite late, the app is probably already killed at this stage.

I can see the homescreen loading when pressing home, that means too me that it takes memory back, and that could be the reason the app is killed.
this is not going to be intime for 1.3T?, 1.5?
blocking-b2g: 1.3T? → 1.5?
(In reply to Joe Cheng [:jcheng] from comment #14)
> this is not going to be intime for 1.3T?, 1.5?

No, it's tarako critical issue.
It sounds like we have two paths to address this: Either the system behavior needs to change to give apps enough time to respond to homescreening, or (for Email) we want to save drafts super frequently on Tarako per comment 10.

A system fix would be more ideal -- there are countless cases where apps are going to run into this problem, and asking them to essentially constantly save their state is super wasteful; even the most trivial of applications becomes much more difficult at the point the user can't reliably switch back to their app the moment anything else happens on the phone.

If this bug becomes a blocker, saving drafts more frequently is possible, but there are countless other ways Email and other apps will lose state if they have no chance to respond to an interrupt event.
Whiteboard: [MP_Blocker]
Whiteboard: [MP_Blocker]
Whiteboard: [sprd294824]
Blocks: 995119
Jesse, can you provide any patch here?
blocking-b2g: 2.0? → 1.3T?
Summary: [Tarako] mms or email MUST save draft when incoming call → [Tarako] MMS MUST save draft when incoming call
Discuss with our product PM, we need MMS draft only.
Again, there is no draft at all on 1.3. Draft was implemented in 1.4. So this is useless to ask for 1.3T?, we won't uplift all the draft functionality in 1.3.
Summary: [Tarako] MMS MUST save draft when incoming call → [Tarako] Message app MUST save draft when incoming call
ni? James, heard that your team is going to provide a patch for 1.3T?
Flags: needinfo?(james.zhang)
jesse will provide patch first.
Flags: needinfo?(james.zhang)
this patch is for draft of message.
only support last one draft, and to be recovered.
Need MMS owner to review this patch.
Flags: needinfo?(styang)
Flags: needinfo?(gal)
Flags: needinfo?(fabrice)
Whiteboard: [sprd294824] → [partner-blocker][sprd294824]
Attachment #8416854 - Flags: review?(timdream)
Flags: needinfo?(gal)
Status: NEW → ASSIGNED
Component: Gaia → Gaia::SMS
Comment on attachment 8416854 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

I am not a SMS owner.
Attachment #8416854 - Flags: review?(timdream)
Attachment #8416854 - Flags: review?(felash)
Attachment #8416854 - Flags: review?(borja.bugzilla)
Assignee: nobody → renfeng.mei
Marking [tarako_only] since draft feature exists on 1.4+ already.
Whiteboard: [partner-blocker][sprd294824] → [partner-blocker][sprd294824][tarako_only]
Flags: needinfo?(fabrice)
We must land this patch before 5.10.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #25)
> Marking [tarako_only] since draft feature exists on 1.4+ already.

Can you merge this feature to v1.3t?
(In reply to James Zhang from comment #27)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #25)
> > Marking [tarako_only] since draft feature exists on 1.4+ already.
> 
> Can you merge this feature to v1.3t?

No we can't land the draft feature to v1.3t, sorry :(
Comment on attachment 8416854 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

So, I understand we have a limited draft support for v1.3, is that the goal?

You'll need some unit tests for this new file. You'll also need to remove the various dead code left in comment.

I haven't tested the code yet (no time for this right now, I wanted to give an early feedback) but the general thing looks good. Just take care to test all paths, especially the activities when there is a draft, things like that.

::: apps/sms/index.html
@@ +60,4 @@
>      <script defer src="js/contact_renderer.js"></script>
>      <script defer src="shared/js/lazy_loader.js"></script>
>      <script defer src="js/startup.js"></script>
> +    <script defer src="js/smsdraft.js"></script>

you should load it just before startup.js, just to keep a little consistency

::: apps/sms/js/smsdraft.js
@@ +2,5 @@
> +var SMSDraft = {
> +  dom:{},
> +  initDOM:function(){
> +    this.dom.form = document.getElementById('messages-compose-form');
> +    this.dom.message = document.getElementById('messages-input');//this.dom.form.querySelector('[contenteditable]');

please remove the comment

@@ +11,5 @@
> +    this.initDOM();
> +    if (window.location.hash == ''){
> +      this.loadDraft();
> +    }else if (window.location.hash == '#new'){
> +    }

please remove the useless else block

@@ +12,5 @@
> +    if (window.location.hash == ''){
> +      this.loadDraft();
> +    }else if (window.location.hash == '#new'){
> +    }
> +    window.addEventListener('hashchange', this.onHashChange.bind(this));

you'll also want to listen to the visibilitychange event, because you don't want to keep the interval running if the phone is in standby, you'll use too much battery.

You can use the visibilitychange event to stop the monitor _and_ save the current draft.

@@ +20,5 @@
> +  },
> +  timerid:null,
> +  timerstatus:'stop',
> +  startMonitor: function(){
> +    if (this.timerstatus == 'start'){

why can't you use the timerid for this?

@@ +38,5 @@
> +  onHashChange: function(){
> +    if (window.location.hash == '#new'){
> +      this.startMonitor();
> +    }else{
> +      this.stopMonitor();

I think you should clear the draft here as well.

@@ +60,5 @@
> +    draft.content = content;
> +    draft.timestamp = Date.now();
> +    draft.threadId = this.draftMessageId;
> +    draft.type = messageType;
> +    draft.recipientsListInnerHTML = draft.recipients.length > 0 ? this.dom.recipientsList.innerHTML : null;

you don't use this

@@ +61,5 @@
> +    draft.timestamp = Date.now();
> +    draft.threadId = this.draftMessageId;
> +    draft.type = messageType;
> +    draft.recipientsListInnerHTML = draft.recipients.length > 0 ? this.dom.recipientsList.innerHTML : null;
> +    draft.messageInnerHTML = this.dom.message.innerHTML;

I don't think this works for images. And moreover I think it's a little dangerous to use innerHTML like this.

You should try to use the code from master: https://github.com/mozilla-b2g/gaia/blob/1cf9ee86885dc6f7a18e8df0a70d57dc5c722c40/apps/sms/js/compose.js#L403-L432

@@ +95,5 @@
> +};
> +
> +
> +window.addEventListener('load', function() {
> +  SMSDraft.init();

can you move this to startup.js ?
Attachment #8416854 - Flags: review?(felash)
Attachment #8416854 - Flags: review?(borja.bugzilla)
Attachment #8416854 - Flags: feedback+
Please follow up and complete the review process.
Flags: needinfo?(renfeng.mei)
Renfeng, please update the patch and reivew+, we must land it before 5.10.
(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #28)
> (In reply to James Zhang from comment #27)
> > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> > comment #25)
> > > Marking [tarako_only] since draft feature exists on 1.4+ already.
> > 
> > Can you merge this feature to v1.3t?
> 
> No we can't land the draft feature to v1.3t, sorry :(

Why? Too many conflict?
Just to make sure everyone is on the same page here:

1) This is targeted for 1.3T (Tarako device)
2) Addressing the comments provided by Julien will make this land
3) This has a very high priority as we only have a couple of days left to wrap up 1.3T

Thanks!
remove the comments reviewed by julian.

i think you are right for the innerHTML to save draft and recover.
but it can't be finished at this base cause of fromDraft has not land in this branch.

may be it is only take effect for short message.
Attachment #8416854 - Attachment is obsolete: true
Flags: needinfo?(renfeng.mei)
Flags: needinfo?(felash)
Flags: needinfo?(gal)
Flags: needinfo?(fabrice)
Attachment #8417817 - Flags: review?(felash)
James, what info do you need from :gal and :fabrice?
(In reply to renfeng.mei from comment #34)
> Created attachment 8417817 [details] [diff] [review]
> mozbug989600-spbug294824-message-app-savedraft-support.patch
> 
> remove the comments reviewed by julian.
> 
> i think you are right for the innerHTML to save draft and recover.
> but it can't be finished at this base cause of fromDraft has not land in
> this branch.
> 

I meant that you can copy this function to the branch. Hopefully it will work as is.

> may be it is only take effect for short message.

This is also a solution; if that's good enough for you, I think it's definitely safer than a solution that involves MMS.


(In reply to James Zhang from comment #32)
> (In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment
> #28)
> > (In reply to James Zhang from comment #27)
> > > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> > > comment #25)
> > > > Marking [tarako_only] since draft feature exists on 1.4+ already.
> > > 
> > > Can you merge this feature to v1.3t?
> > 
> > No we can't land the draft feature to v1.3t, sorry :(
> 
> Why? Too many conflict?

It's a big feature and like all big features it had issues and regressions. We fixed several issues during 1.4 and 2.0 development, so I'm not confident with uplifting it, it's quite certain that we'll let bugs go in :(
Flags: needinfo?(felash)
copy fromDraft from master even i do not want to pollute compose.js.

and it works fine that i tested.
Attachment #8417817 - Attachment is obsolete: true
Attachment #8417817 - Flags: review?(felash)
Comment on attachment 8417852 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

+ still the other previous comments

::: apps/sms/js/smsdraft.js
@@ +47,5 @@
> +    var content = Compose.getContent();
> +    var subject = Compose.getSubject();
> +    var messageType = Compose.type;
> +    var draft = this.draft;
> +    if (draft.content && draft.content.join() == content.join()){// no change, no save

I think this won't work if you replace an attachment with another attachment

better listen for the "input" event from Compose (`Compose.on('input', func)`) and set a boolean (`dirty = true`) to know if the user changed something.

@@ +77,5 @@
> +    this.storeDraft({});
> +  },
> +  jump2DraftView: function(draft){
> +    window.location.hash = '#new';
> +    setTimeout(function(){

use hashchange for this, otherwise you're open to race conditions.
(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #38)

> @@ +77,5 @@
> > +    this.storeDraft({});
> > +  },
> > +  jump2DraftView: function(draft){
> > +    window.location.hash = '#new';
> > +    setTimeout(function(){
> 
> use hashchange for this, otherwise you're open to race conditions.

see https://github.com/mozilla-b2g/gaia/blob/aef737f4eae863949d4b42cd6c17339aec3a5fa0/apps/sms/js/activity_handler.js#L109-L114 for a code example
Triage: 1.3T+
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(styang)
1. move recover draft to onHashChange
2. add input event to monitor dirty changed
Attachment #8417852 - Attachment is obsolete: true
(In reply to renfeng.mei from comment #41)

> 2. add input event to monitor dirty changed

I don't see this in the patch. Are you sure you have an update patch?
(In reply to renfeng.mei from comment #41)

> 1. move recover draft to onHashChange

And I don't see this either, by the way.
Comment on attachment 8417950 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

::: apps/sms/js/smsdraft.js
@@ +37,5 @@
> +      this.stopMonitor();
> +      this.clearDraft();
> +    }
> +  },
> +  draftMessageId:'smsdraftid',

please use capital letters for constants, this makes the code easier to read. Also please prefix the value by "tarako-" so that in the future we'd have less surprises.

@@ +62,5 @@
> +    this.storeDraft(draft);
> +  },
> +  loadDraft: function(){
> +    asyncStorage.getItem(this.draftMessageId, function(draft){
> +      this.draft = draft;

"this" does not work here, you need to bind the function; but I think you don't need `this.draft` if you use the "dirty" boolean, anyway ?
upload the patch
change smsdraftid 
already correct this scope
Attachment #8417950 - Attachment is obsolete: true
re-upload. sorry for not change the uploaded file.
Attachment #8417992 - Attachment is obsolete: true
Attachment #8417997 - Flags: review?(felash)
Comment on attachment 8417997 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

Some comments

I'll test on the device now.

::: apps/sms/js/smsdraft.js
@@ +32,5 @@
> +    this.timerid = null;
> +    Compose.off('input', SMSDraft.onInput);
> +  },
> +  onHashChange: function(){
> +    if (window.location.hash == '#new'){

BTW, you don't want to save the draft in the thread view too?

@@ +33,5 @@
> +    Compose.off('input', SMSDraft.onInput);
> +  },
> +  onHashChange: function(){
> +    if (window.location.hash == '#new'){
> +      this.recoverDraft();

If you keep this here, you'll recover the draft each time the user goes into the composer. Is it what you want, or do you want to recover the draft only at startup ?

If only at startup, then you need to add another hashchange handler in "loadDraft", after "jump2DraftView", and remove the recover here.

@@ +40,5 @@
> +      this.stopMonitor();
> +      this.clearDraft();
> +    }
> +  },
> +  draftMessageId:'tarako-sms-draft-index',

still this, please use DRAFT_MESSAGE_KEY instead of draftMessageId, it's easier to read when you use it.

@@ +53,5 @@
> +    var messageType = Compose.type;
> +    var draft = this.draft;
> +    if (!this.dirty){
> +       return;
> +    }

you can put this condition at the start of the function

@@ +86,5 @@
> +    window.location.hash = '#new';
> +  },
> +  recoverDraft: function(){
> +    var draft = SMSDraft.draft;
> +    if (!(draft.content && draft.content.length > 0)){

So you don't want to recover a draft with only recipients, or with only a subject?

I don't mind, it's your patch, but you need to decide.

@@ +89,5 @@
> +    var draft = SMSDraft.draft;
> +    if (!(draft.content && draft.content.length > 0)){
> +       return;
> +    }
> +    SMSDraft.dom.message.classList.remove('placeholder');

I'm not sure you need this; I think the placeholder class is removed after fromDraft.
Comment on attachment 8417997 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

still no visibilitychange event as I've asked in comment 29.
and still no tests. I don't ask for a lot of tests, but at least testing the main big cases.

Since it's a quite self contained file, it should be quite easy to test it. Please tell if you need some help.

Also, maybe I miss something, but it doesn't work on my device right now.

::: apps/sms/js/smsdraft.js
@@ +1,3 @@
> +
> +var SMSDraft = {
> +  dom:{},

can you put all variables at the start too? dirty, timerid, draft.

Also, please reinitialize them all in init() too, so that it's testable.

@@ +69,5 @@
> +    SMSDraft.dirty = true;
> +  },
> +  loadDraft: function(){
> +    asyncStorage.getItem(this.draftMessageId, function(draft){
> +      SMSDraft.draft = draft;

please add:

  if (!draft) {
    return;
  }

If it's absent we have an error right now.

@@ +85,5 @@
> +  jump2DraftView: function(draft){
> +    window.location.hash = '#new';
> +  },
> +  recoverDraft: function(){
> +    var draft = SMSDraft.draft;

please add:

  if (!draft) {
    return;
  }
(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #48)
> 
> Also, maybe I miss something, but it doesn't work on my device right now.

ok, it doesn't work for me because of:

> 
> @@ +85,5 @@
> > +  jump2DraftView: function(draft){
> > +    window.location.hash = '#new';
> > +  },
> > +  recoverDraft: function(){
> > +    var draft = SMSDraft.draft;
> 
> please add:
> 
>   if (!draft) {
>     return;
>   }


recoverDraft throws, and so the monitor is never started.
Attachment #8417997 - Flags: review?(felash) → review-
Comment on attachment 8417997 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

::: apps/sms/js/smsdraft.js
@@ +50,5 @@
> +    }
> +    var content = Compose.getContent();
> +    var subject = Compose.getSubject();
> +    var messageType = Compose.type;
> +    var draft = this.draft;

add ` || {}` here too, because `this.draft` can be null if it's set after `getItem`.
Please add `'use strict';` at the start of the file.
Comment on attachment 8417997 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

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

::: apps/sms/js/smsdraft.js
@@ +92,5 @@
> +    }
> +    SMSDraft.dom.message.classList.remove('placeholder');
> +    Compose.fromDraft(draft);
> +    draft.recipients.forEach(function(number,i){
> +      ThreadUI.recipients.add({'number':number});

it doesn't do the contact lookup but I don't have an easy way for you to do the lookup until bug 994567 is done.

Maybe you can save ThreadUI.recipients.list instead of ThreadUI.recipients.numbers in line 59? The list should have all the needed data.
Renfeng, please update patch again.
Hi Julien, can you help to uplift a patch?
Flags: needinfo?(renfeng.mei)
Is this issue the same as bug 999286?
James: bug 999286 won't resolve the issue with the incoming call.

(In reply to James Zhang from comment #53)
> Hi Julien, can you help to uplift a patch?

Of course I can, but you can as well wait the developer to do this, unless it takes too much time?
Julien, if you have cycles, get that to an acceptable state and push to 1.3t.
Flags: needinfo?(fabrice) → needinfo?(felash)
No cycle anymore today, sorry, I'll take whatever is ready tomorrow and move forward.
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #47)
> Comment on attachment 8417997 [details] [diff] [review]
> mozbug989600-spbug294824-message-app-savedraft-support.patch
> 
> Review of attachment 8417997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some comments
> 
> I'll test on the device now.
> 
> ::: apps/sms/js/smsdraft.js
> @@ +32,5 @@
> > +    this.timerid = null;
> > +    Compose.off('input', SMSDraft.onInput);
> > +  },
> > +  onHashChange: function(){
> > +    if (window.location.hash == '#new'){
> 
> BTW, you don't want to save the draft in the thread view too?

only saveDraft when edit....

> @@ +33,5 @@
> > +    Compose.off('input', SMSDraft.onInput);
> > +  },
> > +  onHashChange: function(){
> > +    if (window.location.hash == '#new'){
> > +      this.recoverDraft();
> 
> If you keep this here, you'll recover the draft each time the user goes into
> the composer. Is it what you want, or do you want to recover the draft only
> at startup ?
> 
> If only at startup, then you need to add another hashchange handler in
> "loadDraft", after "jump2DraftView", and remove the recover here.

there is a condition inside recoverDraft to make sure to load or not.


> @@ +40,5 @@
> > +      this.stopMonitor();
> > +      this.clearDraft();
> > +    }
> > +  },
> > +  draftMessageId:'tarako-sms-draft-index',
> 
> still this, please use DRAFT_MESSAGE_KEY instead of draftMessageId, it's
> easier to read when you use it.
> 
> @@ +53,5 @@
> > +    var messageType = Compose.type;
> > +    var draft = this.draft;
> > +    if (!this.dirty){
> > +       return;
> > +    }
> 
> you can put this condition at the start of the function
> 
> @@ +86,5 @@
> > +    window.location.hash = '#new';
> > +  },
> > +  recoverDraft: function(){
> > +    var draft = SMSDraft.draft;
> > +    if (!(draft.content && draft.content.length > 0)){
> 
> So you don't want to recover a draft with only recipients, or with only a
> subject?
> 
> I don't mind, it's your patch, but you need to decide.
> 
> @@ +89,5 @@
> > +    var draft = SMSDraft.draft;
> > +    if (!(draft.content && draft.content.length > 0)){
> > +       return;
> > +    }
> > +    SMSDraft.dom.message.classList.remove('placeholder');
> 
> I'm not sure you need this; I think the placeholder class is removed after
> fromDraft.
Flags: needinfo?(renfeng.mei)
1. add visibilitychange
2. init prameters
3. use strict
4. more safer conditions
5. DRAFT_MESSAGE_KEY
Attachment #8417997 - Attachment is obsolete: true
Attachment #8418495 - Flags: review?(felash)
(note: will look at it later today, and do the additional changes myself)
I discussed with Vivien, and we find strange that the Messages app is killed when we receive a call.

When monitoring the processes, we see that:

* the preloaded process could be killed before the Messages app, to save some more memory
* the homescreen is launched; we don't know if it's launched after or before the Messages app is killed, but it's worth checking

Fabrice, do you think something could be done here?

In the mean time I move forward with the patch here.
Flags: needinfo?(fabrice)
I think background Message app must be killed when low memory situation. 
Fabrice can't save more memory now, let's land MMS draft patch for tarako only.
(In reply to James Zhang from comment #62)
> I think background Message app must be killed when low memory situation. 
> Fabrice can't save more memory now, let's land MMS draft patch for tarako
> only.

As I said, it seems like there could be more memory saved in this situation (eg: the preloaded process). I think I have the latest build on my Tarako.
James, what is the STR you're using to test that it's crashing?
Flags: needinfo?(james.zhang)
(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #61)
> I discussed with Vivien, and we find strange that the Messages app is killed
> when we receive a call.
> 
> When monitoring the processes, we see that:
> 
> * the preloaded process could be killed before the Messages app, to save
> some more memory
> * the homescreen is launched; we don't know if it's launched after or before
> the Messages app is killed, but it's worth checking
> 
> Fabrice, do you think something could be done here?

No, that works as expected. The message apps goes in the background, but uses quite a lot of memory, so we kill it before the preallocated app. The homescreen is in a different class so we try to keep it alive longer. I think everything behaves as it should in general - you are just in a bad situation.
Flags: needinfo?(fabrice)
Flags: needinfo?(james.zhang)
(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #64)
> James, what is the STR you're using to test that it's crashing?

James, I'd still like to know what STR you're using. When I try to call the Tarako device, the SMS app is not killed with the most current build, so I'd like to know what you're doing. Thanks.
Flags: needinfo?(james.zhang)
(In reply to Fabrice Desré [:fabrice] from comment #65)
> (In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #61)
> > I discussed with Vivien, and we find strange that the Messages app is killed
> > when we receive a call.
> > 
> > When monitoring the processes, we see that:
> > 
> > * the preloaded process could be killed before the Messages app, to save
> > some more memory
> > * the homescreen is launched; we don't know if it's launched after or before
> > the Messages app is killed, but it's worth checking
> > 
> > Fabrice, do you think something could be done here?
> 
> No, that works as expected. The message apps goes in the background, but
> uses quite a lot of memory, so we kill it before the preallocated app. The
> homescreen is in a different class so we try to keep it alive longer.

We actually thought that in that situation, we'd rather keep the app that was in background last, and kill the homescreen. Not sure how the heuristic could be though.
Attached file v1.3t github PR
Hey Oleg,

here is the patch I talked to you before.

The goal is to do a minimal and safe enough draft feature, that would be automatically recalled when the app is launched, in case it was killed for some reason.

Only the "composer" view is affected, per comment 58, and we remove the draft as soon as we leave this view. Also, seems like Spreadtrum does not want to save messages that have no content and only recipients.

I tried various scenarios and it seems to work fine.

Maybe one scenario feels strange, with a "send message from contact" activity, if the messages app was launched in the background: when we kill the activity (with the "home" button for example), and launch the messages app in background, the draft is not loaded. But it works properly as soon as I kill the messages app and launch it again. Since on Tarako the app will likely be killed in that situation anyway, I'd say we don't need to really take care of this.

Waiting for your review !
Assignee: renfeng.mei → felash
Attachment #8418495 - Attachment is obsolete: true
Attachment #8418495 - Flags: review?(felash)
Attachment #8418897 - Flags: review?(azasypkin)
Attachment #8418897 - Flags: feedback?(james.zhang)
Comment on attachment 8418897 [details] [review]
v1.3t github PR

James, I added the new code in separate commits on the pull request, it would be nice if you could have a look to the code. Thanks.
Attachment #8418897 - Flags: feedback?(james.zhang) → feedback+
Flags: needinfo?(james.zhang)
Comment on attachment 8418897 [details] [review]
v1.3t github PR

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #68)
> Created attachment 8418897 [details] [review]
> v1.3t github PR
> 
> Hey Oleg,
> 
> here is the patch I talked to you before.
> 
> The goal is to do a minimal and safe enough draft feature, that would be
> automatically recalled when the app is launched, in case it was killed for
> some reason.
> 
> Only the "composer" view is affected, per comment 58, and we remove the
> draft as soon as we leave this view. Also, seems like Spreadtrum does not
> want to save messages that have no content and only recipients.
> 
> I tried various scenarios and it seems to work fine.
> 
> Maybe one scenario feels strange, with a "send message from contact"
> activity, if the messages app was launched in the background: when we kill
> the activity (with the "home" button for example), and launch the messages
> app in background, the draft is not loaded. But it works properly as soon as
> I kill the messages app and launch it again. Since on Tarako the app will
> likely be killed in that situation anyway, I'd say we don't need to really
> take care of this.
> 
> Waiting for your review !

I've left few comments at GitHub, the majority are just stylistic nits + some questions.

It works fine on my device, except one strange issue - when I have draft with recipients entered like that: "1" "Contact(with phone number 2)", where "1" is just custom phone number, then I'm getting "1" "1" on draft recover. If I change the order like "Contact(with phone number 2)" "1", then it recovers correctly. Maybe it's unrelated issue.

If you feel that nits can be ignored considering the urgency for this patch, I'm also fine with it, then it's r+ from my side :)
Attachment #8418897 - Flags: review?(azasypkin)
Oh, forgot to mentioned I've been testing patch on buri(v1.3 branch) as I don't have Tarako yet. So maybe issue with recipients that I described above doesn't affect v1.3t.
I remember we fixed something like this in v1.4 when we were doing the draft functionality. I'll dig later today to try to find the fix.
James, please, I still want to know the STR you use here. Can you please tell us what you're doing to make the SMS app crash in a way that this bug tries to solve?

It's the 3rd time I ask this (see previous comment 64 and comment 66).

Thanks.
Flags: needinfo?(james.zhang)
(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #73)
> James, please, I still want to know the STR you use here. Can you please
> tell us what you're doing to make the SMS app crash in a way that this bug
> tries to solve?
> 
> It's the 3rd time I ask this (see previous comment 64 and comment 66).
> 
> Thanks.

I'll talk to James and our BJ QA team for the clear STR to verify it and update here.
Steven, can you also please check if we should save/restore drafts with only a subject and/or recipients, but no body? Thanks a lot !
Flags: needinfo?(styang)
Comment on attachment 8418897 [details] [review]
v1.3t github PR

Hey Oleg,

I fixed all nits you reported and I changed how we saved drafts like you suggested (it was a very good suggestion!).

I also found the "several recipients" issue and I fixed it by removing the "questionable" and "lookupable" nature of the recipient. I think the issue does not happen in master because when we recall the draft, we look up the contact, and as a result we introduce an asynchronous behavior. Still the same fix could be done in master, along with other fixes (for example, set isQuestionable or isLookupable to false once the recipient has been looked up; the "index" in "validateContact" could be passed in the emitted "add" event).

What do you think of this patch for v1.3t then?
Attachment #8418897 - Flags: review?(azasypkin)
Flags: needinfo?(james.zhang)
Summary: [Tarako] Message app MUST save draft when incoming call → [Tarako] Message app MUST save draft when killed by LMK
I have updated the summary.
1.Write MMS
2.Incoming call
3.Add a new call
4.End call, and MMS is killed by LMK

Or
1.Write MMS
2.press home key
3.enter contact and new and take a picture, and MMS is killed by LMK
4.press home key
5.Reenter MMS
Please use test case 2, it can be reproduced easy.
(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #75)
> Steven, can you also please check if we should save/restore drafts with only
> a subject and/or recipients, but no body? Thanks a lot !

Hi Julian, I think body is the most important part because users may be frustrated if he/she lose lots of words been typed. Besides, let me know is the STR stated in #comment 77 clear for you? thanks.
Well, I agreed that being killed when receiving a call was bad, because we couldn't predict when that would happen.

Now, if we're being killed when the user does an action it self, is it that bad?

I mean, is it bad enough to introduce a quite big chunk of code that might introduce bugs and regressions?

That's my concern, Steven.
Comment on attachment 8418897 [details] [review]
v1.3t github PR

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #76)
> Comment on attachment 8418897 [details] [review]
> v1.3t github PR
> 
> Hey Oleg,
> 
> I fixed all nits you reported and I changed how we saved drafts like you
> suggested (it was a very good suggestion!).
> 
> I also found the "several recipients" issue and I fixed it by removing the
> "questionable" and "lookupable" nature of the recipient. I think the issue
> does not happen in master because when we recall the draft, we look up the
> contact, and as a result we introduce an asynchronous behavior. Still the
> same fix could be done in master, along with other fixes (for example, set
> isQuestionable or isLookupable to false once the recipient has been looked
> up; the "index" in "validateContact" could be passed in the emitted "add"
> event).
> 
> What do you think of this patch for v1.3t then?

I think we're good to go now, thanks!! Just left a few nits at GitHub, mainly regarding to unit tests.
Attachment #8418897 - Flags: review?(azasypkin) → review+
Waiting for a green travis before merging. Please don't merge from the PR.
master: 3be9b8aed46d941576d90c7a14932a1e3e8fbcdd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
julien,

there is a case that the last editing recipient can not be saved.

when manual inputing a contact, then pressing-home to remove sms-app, the last inputs not being saved.

because it is not fire ThreadUI.recipients.add.

any ideas for this case?
Flags: needinfo?(felash)
No idea except listening for keypress or input event.

Is this case a blocker for you?
Flags: needinfo?(felash)
Note that's why I asked a feedback in comment 69 _before_ landing the patch. I don't want to do forward patch like this.
(In reply to Julien Wajsberg [:julienw] from comment #85)
> No idea except listening for keypress or input event.
> 
> Is this case a blocker for you?

I will check with our PM. Hope it's not a blocker.
Depends on: 1011573
Flags: needinfo?(styang)
Flags: needinfo?(gal)
You need to log in before you can comment on or make changes to this bug.