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

Status: RESOLVED FIXED

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

Description

[James Zhang (Spreadtrum)]

When MMS or Email is killed by LMK, user maybe very angry if no draft saving.

Comment 1

[James Zhang (Spreadtrum)]

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

Comment 2

[[:fabrice] Fabrice Desré]

ni-ing Julien that worked on the sms/mms drafts, and Andrew for email.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 4

[James Zhang (Spreadtrum)]

(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.

Comment 5

[Julien Wajsberg [:julienw]]

(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?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(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.)

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 8

[James Zhang (Spreadtrum)]

(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.

Comment 9

[Evelyn Hung]

(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.

Comment 10

[Julien Wajsberg [:julienw]]

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.

Comment 11

[Julien Wajsberg [:julienw]]

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.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 13

[Julien Wajsberg [:julienw]]

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.

Comment 14

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

this is not going to be intime for 1.3T?, 1.5?

Comment 15

[James Zhang (Spreadtrum)]

(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.

Comment 16

[Marcus Cavanaugh [:mcav]]

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.

Comment 17

[James Zhang (Spreadtrum)]

Jesse, can you provide any patch here?

Comment 18

[James Zhang (Spreadtrum)]

Discuss with our product PM, we need MMS draft only.

Comment 19

[Julien Wajsberg [:julienw]]

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.

Comment 20

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

ni? James, heard that your team is going to provide a patch for 1.3T?

Comment 21

[James Zhang (Spreadtrum)]

jesse will provide patch first.

Comment 22

[renfeng.mei]

Attachment: mozbug989600-spbug294824-message-app-savedraft-support.patch

this patch is for draft of message.
only support last one draft, and to be recovered.

Comment 23

[James Zhang (Spreadtrum)]

Need MMS owner to review this patch.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8416854 [details] [diff] [review]
mozbug989600-spbug294824-message-app-savedraft-support.patch

I am not a SMS owner.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Marking [tarako_only] since draft feature exists on 1.4+ already.

Comment 26

[James Zhang (Spreadtrum)]

We must land this patch before 5.10.

Comment 27

[James Zhang (Spreadtrum)]

(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?

Comment 28

[Julien Wajsberg [:julienw]]

(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 29

[Julien Wajsberg [:julienw]]

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 ?

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Please follow up and complete the review process.

Comment 31

[James Zhang (Spreadtrum)]

Renfeng, please update the patch and reivew+, we must land it before 5.10.

Comment 32

[James Zhang (Spreadtrum)]

(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?

Comment 33

[Faramarz Rashed]

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!

Comment 34

[renfeng.mei]

Attachment: 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.

may be it is only take effect for short message.

Comment 35

[Faramarz Rashed]

James, what info do you need from :gal and :fabrice?

Comment 36

[Julien Wajsberg [:julienw]]

(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 :(

Comment 37

[renfeng.mei]

Attachment: mozbug989600-spbug294824-message-app-savedraft-support.patch

copy fromDraft from master even i do not want to pollute compose.js.

and it works fine that i tested.

Comment 38

[Julien Wajsberg [:julienw]]

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.

Comment 39

[Julien Wajsberg [:julienw]]

(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

Comment 40

[Steven Yang [:styang]]

Triage: 1.3T+

Comment 41

[renfeng.mei]

Attachment: mozbug989600-spbug294824-message-app-savedraft-support.patch

1. move recover draft to onHashChange
2. add input event to monitor dirty changed

Comment 42

[Julien Wajsberg [:julienw]]

(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?

Comment 43

[Julien Wajsberg [:julienw]]

(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 44

[Julien Wajsberg [:julienw]]

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 ?

Comment 45

[renfeng.mei]

Attachment: mozbug989600-spbug294824-message-app-savedraft-support.patch

upload the patch
change smsdraftid 
already correct this scope

Comment 46

[renfeng.mei]

Attachment: mozbug989600-spbug294824-message-app-savedraft-support.patch

re-upload. sorry for not change the uploaded file.

Comment 47

[Julien Wajsberg [:julienw]]

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 48

[Julien Wajsberg [:julienw]]

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;
  }

Comment 49

[Julien Wajsberg [:julienw]]

(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.

Comment 50

[Julien Wajsberg [:julienw]]

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`.

Comment 51

[Julien Wajsberg [:julienw]]

Please add `'use strict';` at the start of the file.

Comment 52

[Julien Wajsberg [:julienw]]

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.

Comment 53

[James Zhang (Spreadtrum)]

Renfeng, please update patch again.
Hi Julien, can you help to uplift a patch?

Comment 54

[James Zhang (Spreadtrum)]

Is this issue the same as bug 999286?

Comment 55

[Julien Wajsberg [:julienw]]

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?

Comment 56

[[:fabrice] Fabrice Desré]

Julien, if you have cycles, get that to an acceptable state and push to 1.3t.

Comment 57

[Julien Wajsberg [:julienw]]

No cycle anymore today, sorry, I'll take whatever is ready tomorrow and move forward.

Comment 58

[renfeng.mei]

(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.

Comment 59

[renfeng.mei]

Attachment: mozbug989600-spbug294824-message-app-savedraft-support.patch

1. add visibilitychange
2. init prameters
3. use strict
4. more safer conditions
5. DRAFT_MESSAGE_KEY

Comment 60

[Julien Wajsberg [:julienw]]

(note: will look at it later today, and do the additional changes myself)

Comment 61

[Julien Wajsberg [:julienw]]

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.

Comment 62

[James Zhang (Spreadtrum)]

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.

Comment 63

[Julien Wajsberg [:julienw]]

(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.

Comment 64

[Julien Wajsberg [:julienw]]

James, what is the STR you're using to test that it's crashing?

Comment 65

[[:fabrice] Fabrice Desré]

(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.

Comment 66

[Julien Wajsberg [:julienw]]

(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.

Comment 67

[Julien Wajsberg [:julienw]]

(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.

Comment 68

[Julien Wajsberg [:julienw]]

Attachment: 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 !

Comment 69

[Julien Wajsberg [:julienw]]

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.

Comment 70

[Oleg Zasypkin [:azasypkin]]

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 :)

Comment 71

[Oleg Zasypkin [: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.

Comment 72

[Julien Wajsberg [:julienw]]

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.

Comment 73

[Julien Wajsberg [:julienw]]

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.

Comment 74

[Steven Yang [:styang]]

(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.

Comment 75

[Julien Wajsberg [:julienw]]

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 !

Comment 76

[Julien Wajsberg [:julienw]]

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?

Comment 77

[James Zhang (Spreadtrum)]

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

Comment 78

[James Zhang (Spreadtrum)]

Please use test case 2, it can be reproduced easy.

Comment 79

[Steven Yang [:styang]]

(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.

Comment 80

[Julien Wajsberg [:julienw]]

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 81

[Oleg Zasypkin [:azasypkin]]

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.

Comment 82

[Julien Wajsberg [:julienw]]

Waiting for a green travis before merging. Please don't merge from the PR.

Comment 83

[Julien Wajsberg [:julienw]]

master: 3be9b8aed46d941576d90c7a14932a1e3e8fbcdd

Comment 84

[renfeng.mei]

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?

Comment 85

[Julien Wajsberg [:julienw]]

No idea except listening for keypress or input event.

Is this case a blocker for you?

Comment 86

[Julien Wajsberg [:julienw]]

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.

Comment 87

[renfeng.mei]

(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.