999286 - [Sora][Message]The content is lost after do some operations.

Status: RESOLVED FIXED

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

Description

[sync-1]

Mozilla build ID:20140404164003
 
 DEFECT DESCRIPTION:
 ->Re-enter message,the content is lost.
 
  REPRODUCING PROCEDURES:
 ->Enter"Message"to select a message which include number to view;
 ->Tap the number,select"Send message";
 ->Input some characters then tap home key back to idle;
 ->Re-enter message,the content is lost.->ko
 
 note:bettle lite don't have this behavior.
  EXPECTED BEHAVIOUR:
 ->Should enter the edit interface.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Julien Wajsberg [:julienw]]

I believe the "expected behavior" line above is wrong.

This happens because we use an activity and pressing the "home" button cancels the activity. This is ironic because I remember I didn't want to use an activity for this kind of in-app navigation.

Maybe we can change ActivityPicker.sendMessage to simulate an activity instead of using a real one.

First, Omega, what do you think of this?

Comment 2

[Jason Smith [:jsmith]]

Can you provide more details of exactly what happens on 1.1?

Comment 3

[Julien Wajsberg [:julienw]]

In 1.1 I believe this is not happening because we didn't use an inline activity back then.

Comment 4

[Jason Smith [:jsmith]]

QA Wanted - let's get a video of this bug.

Comment 5

[Julien Wajsberg [:julienw]]

FTR I reproduced it clearly on my phone using the STR.

Comment 6

[Julien Wajsberg [:julienw]]

And yes, it's clearly a weird behavior. Don't know if we should block though.

Comment 7

[Matthew Vaughan]

Attachment: Video of issue reproducing.

(In reply to Jason Smith [:jsmith] from comment #4)
> QA Wanted - let's get a video of this bug.

Attached is a video of the issue reproducing. This was tested using a Buri that's flashed to the 04/23/14 1.4 build. The video shows an attempt to get back to the message being composed through card view and through directly launching the Message app.

When the user taps the Home button, it seems to terminate the Compose Message screen, leaving only the Message app active. The user is unable to get back to the Compose Message screen through the card view or by directly launching the Message app again.

Device: Buri v1.4 MOZ
BuildID: 20140423000203
Gaia: 25d53d442cbf17c150575c98979e957ae040e023
Gecko: 2d1b4d36eef9
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Comment 8

[Jason Smith [:jsmith]]

Now after seeing the video - I don't think this is a blocker. This looks like the activity with the message partially done gets killed when the home button is pushed, which is annoying, but not critical. Draft support will likely ease the pain felt here.

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Now after seeing the video - I don't think this is a blocker. This looks
> like the activity with the message partially done gets killed when the home
> button is pushed, which is annoying, but not critical. Draft support will
> likely ease the pain felt here.

I checked and we actually save a draft. But I think there could be a race condition here, I'm not sure it's always saved.

Comment 10

[Jack Liu]

Hi, I think it's a blocking bug, reproduced by following STR that is normal usage and important to user:

1. enter contacts, find some one and go to contact info page.
2. click message button beside phone number field.
3. edit message, input some words.
4. press home key, exit to idle screen.
5. enter message app again, found edited content lost.

Comment 11

[Jack Liu]

Hi, further more, following the similar STR, email app is ok.

Comment 12

[Jason Smith [:jsmith]]

Opps didn't mean to reset the nom.

Comment 13

[Jason Smith [:jsmith]]

Vance - Can you find out if this is a cert blocker?

Comment 14

[Julien Wajsberg [:julienw]]

I think it would be relatively easy to have a patch but having a 1.3 blocker would still scare me somewhat, I'm afraid of bugs coming from this. Using an activity as we do now makes it easy to have a clean state; on the contrary, not using an activity, we need to be careful to cleaning all necessary state.
Still this is doable.

Comment 15

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

(In reply to Julien Wajsberg [:julienw] from comment #1)
> I believe the "expected behavior" line above is wrong.
> 
> This happens because we use an activity and pressing the "home" button
> cancels the activity. This is ironic because I remember I didn't want to use
> an activity for this kind of in-app navigation.
> 
> Maybe we can change ActivityPicker.sendMessage to simulate an activity
> instead of using a real one.
> 
> First, Omega, what do you think of this?

IMO users don't know what is Activity. So it's better to have the same behavior like what Email does.

Comment 16

[Julien Wajsberg [:julienw]]

(Cc Oleg who might be interested in this bug)

Comment 17

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> (Cc Oleg who might be interested in this bug)

I totally agree that current behavior isn't user friendly.

Julien, do you know why we close inline activity on Home button press? 

My first thought was: just because we would like to avoid switching to the originating app when inline activity is still open so that we won't have inconsistent state. If that the case, what if we don't close it but replace originating app with it in app carousel (not sure how to name it when user switches between apps after Home button long press)?

IMO, Home button doesn't imply that we want to close\cancel anything, but just to postpone it.

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> (In reply to Julien Wajsberg [:julienw] from comment #16)
> > (Cc Oleg who might be interested in this bug)
> 
> I totally agree that current behavior isn't user friendly.
> 
> Julien, do you know why we close inline activity on Home button press? 
> 
> My first thought was: just because we would like to avoid switching to the
> originating app when inline activity is still open so that we won't have
> inconsistent state. If that the case, what if we don't close it but replace
> originating app with it in app carousel (not sure how to name it when user
> switches between apps after Home button long press)?
> 
> IMO, Home button doesn't imply that we want to close\cancel anything, but
> just to postpone it.

I think the main use case it to let the user have control over activities: pressing home button will always recover even if the activity is faulty.

The fix I had in mind is changing ActivityPicker.sendMessage to not use an activity.

Comment 19

[Oleg Zasypkin [:azasypkin]]

h(In reply to Julien Wajsberg [:julienw] from comment #18)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> > (In reply to Julien Wajsberg [:julienw] from comment #16)
> > > (Cc Oleg who might be interested in this bug)
> > 
> > I totally agree that current behavior isn't user friendly.
> > 
> > Julien, do you know why we close inline activity on Home button press? 
> > 
> > My first thought was: just because we would like to avoid switching to the
> > originating app when inline activity is still open so that we won't have
> > inconsistent state. If that the case, what if we don't close it but replace
> > originating app with it in app carousel (not sure how to name it when user
> > switches between apps after Home button long press)?
> > 
> > IMO, Home button doesn't imply that we want to close\cancel anything, but
> > just to postpone it.
> 
> I think the main use case it to let the user have control over activities:
> pressing home button will always recover even if the activity is faulty.
> 
> The fix I had in mind is changing ActivityPicker.sendMessage to not use an
> activity.

Btw, just checked it with the latest sources and I can't reproduce it. Do you still see this issue on current master?

Comment 20

[Oleg Zasypkin [:azasypkin]]

If I haven't messed up with git bisect then patch for bug 916709 fixed that behavior :)

Comment 21

[Julien Wajsberg [:julienw]]

Wow, would be nice, I'll double check.

But we might still need an adhoc branch patch on 1.3/1.4 if this bug is made a blocker.

Comment 22

[Jason Smith [:jsmith]]

I'll get someone to look at this during our in-moztrap sprint.

Comment 23

[Julien Wajsberg [:julienw]]

Jason, what you want regressionwindow-wanted for?

I think the bug initially comes from the change to an "inline" activity (bug 815093), landed in 1.3.

Comment 24

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Please help to prepare a 1.3 patch since this bug is a cert blocker

Comment 25

[Jason Smith [:jsmith]]

David - Can you find someone to look at this?

Comment 26

[Julien Wajsberg [:julienw]]

Oleg, maybe you can take this bug as a urgent bug ?
See comment 18 for a possible solution (and only solution IMO, unless we uplift the system app changes which I think won't happen)

Comment 27

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #26)
> Oleg, maybe you can take this bug as a urgent bug ?
> See comment 18 for a possible solution (and only solution IMO, unless we
> uplift the system app changes which I think won't happen)

Sure, I'll look into it. Regarding to solution suggested in comment 18, we can potentially make it work for the case described in bug description, but what about other, but almost the same case mentioned in the comment 10? It's not clear for me what we can do here, except of moving back to "window" activity. Am I missing something?

Comment 28

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (v1.3)

Adding v1.3 patch with workaround suggested by Julien that fixes behavior described initially, not the one from comment 10.

Comment 29

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #27)
> (In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment
> #26)
> > Oleg, maybe you can take this bug as a urgent bug ?
> > See comment 18 for a possible solution (and only solution IMO, unless we
> > uplift the system app changes which I think won't happen)
> 
> Sure, I'll look into it. Regarding to solution suggested in comment 18, we
> can potentially make it work for the case described in bug description, but
> what about other, but almost the same case mentioned in the comment 10? It's
> not clear for me what we can do here, except of moving back to "window"
> activity. Am I missing something?

Or changing how activities work. Maybe something is deeply broken in how we designed inline/window activities in Firefox OS. We can ask Alive about this maybe?
Alive, any idea about comment 10? I don't think it's a good idea to move back to "window" for this activity just because of this ("window" activites have their own problems).

Comment 30

[Oleg Zasypkin [:azasypkin]]

I'm also wondering whether it's possible and not too risky to uplift a fraction of patch for bug 916709 that actually fixed "Home button vs inline activity" behavior?

Comment 31

[David Scravaglieri [:scravag]]

clear n? as Oleg is assigned

Comment 32

[(inactive after 6/18) Alive Kuo [:alive]]

Uh...wait, do we call activity in message app for sending a message.....? Why do we do this?

Comment 33

[(inactive after 6/18) Alive Kuo [:alive]]

Is this talking about 'message app' -> 'contact app' -> 'message activity'?
If so, the inline activity is kept when home button is pressed in master now by bug 916709.

We could do some similar thing for v1.4 branch.

Comment 34

[Oleg Zasypkin [:azasypkin]]

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #33)
> Is this talking about 'message app' -> 'contact app' -> 'message activity'?
> If so, the inline activity is kept when home button is pressed in master now
> by bug 916709.
> 
> We could do some similar thing for v1.4 branch.

Yes, we're mostly wondering about that case. I think it would be great if you can do this for 1.4 branch! Is it possible to do for 1.3 branch also?

Comment 35

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #34)
> (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #33)
> > Is this talking about 'message app' -> 'contact app' -> 'message activity'?
> > If so, the inline activity is kept when home button is pressed in master now
> > by bug 916709.
> > 
> > We could do some similar thing for v1.4 branch.
> 
> Yes, we're mostly wondering about that case. I think it would be great if
> you can do this for 1.4 branch! Is it possible to do for 1.3 branch also?

For v1.4 might be. For v1.3 I need to double check.
It seems there's already a patch here, do you want me to take this bug over or open another bug and have someone 1.4+ it?

Comment 36

[Oleg Zasypkin [:azasypkin]]

I think it depends, if you're able to uplift your changes to both 1.3 and 1.4 branches, then it will also totally resolve this issue and you can take it for sure. Otherwise we'll need two separate issues: this one for 1.3 partial workaround and new one to uplift your changes to 1.4. But let' confirm with Julien :)

Thanks!

Comment 37

[(inactive after 6/18) Alive Kuo [:alive]]

Not uplift-able, it needs rework but not too difficult.

Comment 38

[Julien Wajsberg [:julienw]]

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #32)
> Uh...wait, do we call activity in message app for sending a message.....?
> Why do we do this?

The main answer is: because it was the easiest.


(In reply to Oleg Zasypkin [:azasypkin] from comment #36)
> I think it depends, if you're able to uplift your changes to both 1.3 and
> 1.4 branches, then it will also totally resolve this issue and you can take
> it for sure. Otherwise we'll need two separate issues: this one for 1.3
> partial workaround and new one to uplift your changes to 1.4. But let'
> confirm with Julien :)

I would prefer to change the activity behavior for v1.3 because this would fix all cases (not only with the Messages app).

We could still change how we do the "send sms" behavior, but I can't decide whether it would be better or not.

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #33)
> Is this talking about 'message app' -> 'contact app' -> 'message activity'?
> If so, the inline activity is kept when home button is pressed in master now
> by bug 916709.
> 
> We could do some similar thing for v1.4 branch.

Actually, we're worried about the flow: 'contact app' -> 'message activity -> <home>'
(no need to be in the message app first)

Comment 39

[Julien Wajsberg [:julienw]]

NI alive for comment 38.

Do you think it's safe to change how the home button works in 1.3?

Comment 40

[(inactive after 6/18) Alive Kuo [:alive]]

For v1.3: we didn't render activity inside caller app container (bug 977934) in 1.3 so it needs more work, I prefer not to do that.

Comment 41

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #38)
> (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #32) 
> I would prefer to change the activity behavior for v1.3 because this would
> fix all cases (not only with the Messages app).

There's a big difference between v1.3 and v1.4 window management (bug 907013) so it may cause additional overhead and trouble to move a v1.4+ feature back to v1.3, sorry about that.

BTW, are we still able to land something in v1.3? I think it's code freeze, isn't it?

Comment 42

[Julien Wajsberg [:julienw]]

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #41)
> (In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment
> #38)
> > (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #32) 
> > I would prefer to change the activity behavior for v1.3 because this would
> > fix all cases (not only with the Messages app).
> 
> There's a big difference between v1.3 and v1.4 window management (bug
> 907013) so it may cause additional overhead and trouble to move a v1.4+
> feature back to v1.3, sorry about that.

Ok. Jack, Vance, is it fine for you if we fix only the original description and not the STR in comment 10? Admittedly I don't know how to fix it besides reverting the activity type, and I'd rather not do this now. Or do you think it's useless to do any of them?

> 
> BTW, are we still able to land something in v1.3? I think it's code freeze,
> isn't it?

I think the partners are still free to pick the patches they want on their builds. Or something like this.

Comment 43

[(inactive after 6/18) Alive Kuo [:alive]]

And what's sad: due to https://bugzilla.mozilla.org/show_bug.cgi?id=931339 if there're two activities opened at the same time, only the first opened one can handle the activity message.

Comment 44

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #43)
> And what's sad: due to https://bugzilla.mozilla.org/show_bug.cgi?id=931339
> if there're two activities opened at the same time, only the first opened
> one can handle the activity message.

So even I keep the activity alive we will have some side effect. Background activity needs to be cleaned periodically...(the best solution I have now to keep the activity and prevent the 2nd activity breaking.)

Comment 45

[Julien Wajsberg [:julienw]]

Yes, system messages are not in great shape currently, also because we didn't use them fully so didn't need all their features. And activities are starting to be great, but only since 1.4 :(

Comment 46

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #42)
> (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #41)
> > (In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment
> > #38)
> > > (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #32) 
> > > I would prefer to change the activity behavior for v1.3 because this would
> > > fix all cases (not only with the Messages app).
> > 
> > There's a big difference between v1.3 and v1.4 window management (bug
> > 907013) so it may cause additional overhead and trouble to move a v1.4+
> > feature back to v1.3, sorry about that.
> 
> Ok. Jack, Vance, is it fine for you if we fix only the original description
> and not the STR in comment 10? Admittedly I don't know how to fix it besides
> reverting the activity type, and I'd rather not do this now. Or do you think
> it's useless to do any of them?
> 

Hi Julien -

I think it is good enough to just fix the issue mentioned in the original description. Thanks for your help!!

Comment 47

[Oleg Zasypkin [:azasypkin]]

Just to summarize:

* Since it's difficult and too risky to uplift system activity-related changes to v1.3 branch, we'll introduce SMS-specific partial workaround for the case mentioned in the bug description only;

* For v1.4 branch Alive will try to uplift activity-related changes from master so that we don't need anything from SMS side. That will resolve all problems mentioned in this bug (initial STR and comment 10). This is going to be done in the scope of separate 1.4+ bug.

Hey Alive, is that correct?

Comment 48

[Julien Wajsberg [:julienw]]

Comment on attachment 8416351 [details] [review]
GitHub pull request URL (v1.3)

Looks good, except there is one issue with the following STR:

1. have a message with a phone number
2. tap this phone number, choose "send message"
3. press back
4. press "new message"

Expected:
* an empty form

Actual:
* the form is prefilled

I think MessageManager.activity is not cleared, or something like this. Now that the activity is inline, I think we didn't particularly took care to this in the normal activity flow.

Comment 49

[Julien Wajsberg [:julienw]]

Please request review when you're ready.

Comment 50

[James Zhang (Spreadtrum)]

Is this issue the same as bug 989600?

Comment 51

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to James Zhang from comment #50)
> Is this issue the same as bug 989600?

NO

Comment 52

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #47)
> Just to summarize:
> 
> * Since it's difficult and too risky to uplift system activity-related
> changes to v1.3 branch, we'll introduce SMS-specific partial workaround for
> the case mentioned in the bug description only;
> 
> * For v1.4 branch Alive will try to uplift activity-related changes from
> master so that we don't need anything from SMS side. That will resolve all
> problems mentioned in this bug (initial STR and comment 10). This is going
> to be done in the scope of separate 1.4+ bug.
> 
> Hey Alive, is that correct?

Sure I could do that for v1.4 if we have another blocker bug.
Thanks.

Comment 53

[Oleg Zasypkin [:azasypkin]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #52)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #47)
> > Just to summarize:
> > 
> > * Since it's difficult and too risky to uplift system activity-related
> > changes to v1.3 branch, we'll introduce SMS-specific partial workaround for
> > the case mentioned in the bug description only;
> > 
> > * For v1.4 branch Alive will try to uplift activity-related changes from
> > master so that we don't need anything from SMS side. That will resolve all
> > problems mentioned in this bug (initial STR and comment 10). This is going
> > to be done in the scope of separate 1.4+ bug.
> > 
> > Hey Alive, is that correct?
> 
> Sure I could do that for v1.4 if we have another blocker bug.
> Thanks.

Great! I've filed bug 1007033 for that and nominated it for 1.4?

Thanks

Comment 54

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #48)
> Comment on attachment 8416351 [details] [review]
> GitHub pull request URL (v1.3)
>
> Looks good, except there is one issue with the following STR:
> 
> 1. have a message with a phone number
> 2. tap this phone number, choose "send message"
> 3. press back
> 4. press "new message"
> 
> Expected:
> * an empty form
> 
> Actual:
> * the form is prefilled
>
> I think MessageManager.activity is not cleared, or something like this. Now
> that the activity is inline, I think we didn't particularly took care to
> this in the normal activity flow.

Yep, that is because of issue that was fixed in bug 978553. I've updated my PR with one-line fix from that patch.

Please, look at the patch once again.

Thanks!

Comment 55

[Jason Smith [:jsmith]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #53)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #52)
> > (In reply to Oleg Zasypkin [:azasypkin] from comment #47)
> > > Just to summarize:
> > > 
> > > * Since it's difficult and too risky to uplift system activity-related
> > > changes to v1.3 branch, we'll introduce SMS-specific partial workaround for
> > > the case mentioned in the bug description only;
> > > 
> > > * For v1.4 branch Alive will try to uplift activity-related changes from
> > > master so that we don't need anything from SMS side. That will resolve all
> > > problems mentioned in this bug (initial STR and comment 10). This is going
> > > to be done in the scope of separate 1.4+ bug.
> > > 
> > > Hey Alive, is that correct?
> > 
> > Sure I could do that for v1.4 if we have another blocker bug.
> > Thanks.
> 
> Great! I've filed bug 1007033 for that and nominated it for 1.4?
> 
> Thanks

From discussions on the other bug - the popup manager patch referenced there already has caused a blocking regression, so it's not safe to take to 1.4. We need to apply a low risk solution to both 1.3 & 1.4 here, so I'd prefer if we stick to a similar risk level across 1.3 & 1.4.

Comment 56

[Jack Liu]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #42)
> (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #41)
> > (In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment
> > #38)
> > > (In reply to Alive Kuo [:alive][NEEDINFO!][OOO 4/30 - 5/4] from comment #32) 
> > > I would prefer to change the activity behavior for v1.3 because this would
> > > fix all cases (not only with the Messages app).
> > 
> > There's a big difference between v1.3 and v1.4 window management (bug
> > 907013) so it may cause additional overhead and trouble to move a v1.4+
> > feature back to v1.3, sorry about that.
> 
> Ok. Jack, Vance, is it fine for you if we fix only the original description
> and not the STR in comment 10? Admittedly I don't know how to fix it besides
> reverting the activity type, and I'd rather not do this now. Or do you think
> it's useless to do any of them?
> 
> > 
> > BTW, are we still able to land something in v1.3? I think it's code freeze,
> > isn't it?
> 
> I think the partners are still free to pick the patches they want on their
> builds. Or something like this.

Hi Julien,

We have not make decision on whether or not to take the patch because more common and serious issue in comment#10 is still not solved.

But if there is a great activity solution in 1.4 as you mentioned, I think there is chance to convince operators not blocking on this issue and give them the confidence that 1.4 will bring much better user experience to end user. 

Thanks for all you efforts.

Comment 57

[Julien Wajsberg [:julienw]]

Hi Jack,

see comment 55, it seems QA doesn't want to do a more general solution for 1.4, so we're stuck with the workaround. If you think otherwise, please raise your concern in bug 1007033 and possibly reopen it.

In the mean time, we'll move forward with the work around in the SMS app.

Comment 58

[Julien Wajsberg [:julienw]]

Comment on attachment 8416351 [details] [review]
GitHub pull request URL (v1.3)

r=me

but please rebase with latest v1.3 branch because I just pushed the fix from bug 1001049 so that we can have a travis build. I want to merge with a green Travis.

Thanks !

Comment 59

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #58)
> Comment on attachment 8416351 [details] [review]
> GitHub pull request URL (v1.3)
> 
> r=me
> 
> but please rebase with latest v1.3 branch because I just pushed the fix from
> bug 1001049 so that we can have a travis build. I want to merge with a green
> Travis.
> 
> Thanks !

Thanks for review! Rebased and Travis is green now!

Hey Ryan, will sheriffs be able to merge this v1.3 fix to v1.3t branch today (Tarako deadline day)?

Comment 60

[Ryan VanderMeulen [:RyanVM]]

I do them every day ;)

Comment 61

[Ryan VanderMeulen [:RyanVM]]

That said, this doesn't approval approval-gaia-v1.3, so I can't do anything with it until then.

Also, I'm confused over what branches are affected here? Is this just v1.3 or does it need to land elsewhere as well?

Comment 62

[Julien Wajsberg [:julienw]]

Ryan: this is 1.3 and 1.3t only (for now). It might be 1.4 too, still to decide.

Comment 63

[Julien Wajsberg [:julienw]]

Comment on attachment 8416351 [details] [review]
GitHub pull request URL (v1.3)

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 #): bug 815093
[User impact] if declined: dataloss if the user presses "home" in this situation.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 64

[Ryan VanderMeulen [:RyanVM]]

v1.3: https://github.com/mozilla-b2g/gaia/commit/c79f185b2c7bd2a0cfa8ebe5e75afc50056785f8

Comment 65

[Julien Wajsberg [:julienw]]

Note that we set "wontfix" for v1.4 until we decide finally about bug 1007033, we'll set it back to "affected" if we decide to land this there. We can also do both!

Comment 66

[Julien Wajsberg [:julienw]]

After discussing with Oleg, we now think this patch is useful for both master and v1.4, independently of what will happen in bug 1007033.

For v1.4, I'm simply toggling the "status-v1.4" flag here.

For master, since this bug is not happening there and it's more an enhancement, I think we should file a separate bug. Oleg, can you take care of it, carry over my r+, do basic testing, and land it?

Comment 67

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #66)
> For master, since this bug is not happening there and it's more an
> enhancement, I think we should file a separate bug. Oleg, can you take care
> of it, carry over my r+, do basic testing, and land it?

Sure, filed a bug 1008823 for that.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/2e86cd1cecfe24917cac8f47c3aa157ed7ef49d8

Comment 69

[Ryan VanderMeulen [:RyanVM]]

And a test follow-up:
v1.3: https://github.com/mozilla-b2g/gaia/commit/26a7a59d219fc8753613b433844123e428adcd56
v1.4: https://github.com/mozilla-b2g/gaia/commit/69bd6293c3e138258257af37a33765997e49bd6e

This was failing Gaia unit tests on the v1.4 uplift, so I went ahead and merged this in on Julien's recommendation.