1040572 - Password hide/show in Sync setup should retain state after device rotation

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Sainath]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Steps to reproduce:

Firefox -> Settings -> Sync -> Get Started -> enter a text in Password field -> Tap on "Show" Button -> password is shown & button is shown as "Hide" -> Change the device orientation from portrait to Landscape, Change back to Portrait orientation.


Actual results:

Password is hidden and the Password button changed to "Show"


Expected results:

Password field & button shouldn't change on device orientation change. "Hide" Button should displayed even when the device orientation is changed.

Comment 1

[Aaron Train [:aaronmt]]

Thanks for the bug report. I'm able to reproduce. Not a regression.

Comment 2

[Nick Alexander :nalexander [he/him]]

This is general Android fragment irritation.  We need to persist the activity state across the configuration change.  This is a good mentor bug, but not a good first bug.

Comment 3

[Ankur Garg]

I am new to mozilla, can I work on this bug? 
if yes then please some info about repo, docs,etc to start on.

Comment 4

[Aaron Train [:aaronmt]]

Sure thing. The mentor assigned to this bug is away currently so we'll request info from another mentor.

Comment 5

[Ankur Garg]

please share info...

Comment 6

[Richard Newman [:rnewman]]

Do you already have Fennec building?

If not, start here:

https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

Let us know when you have a Fennec build that you can install on your device.

Comment 7

[Ankur Garg]

I was able to build Fennec, but then my phone is damaged and I tried running it in AVD, but its hanging as soon as run Fennec in it. please help

Comment 8

[Richard Newman [:rnewman]]

(In reply to Ankur Garg from comment #7)
> I was able to build Fennec, but then my phone is damaged and I tried running
> it in AVD, but its hanging as soon as run Fennec in it. please help

You need to run an x86 build, in an x86 emulator, with graphics acceleration enabled.

Some partly relevant info is here:

https://wiki.mozilla.org/Mobile/Fennec/Android#Running_the_Android_4.2_x86_emulator

Comment 9

[Harsha]

Could you point me useful info?

Comment 10

[swaroop.rao]

I'd like to take this up if nobody's working on it already. I've got a build up and running and I've managed to reproduce this defect on my Nexus 5.

Comment 11

[Nick Alexander :nalexander [he/him]]

I see I missed a request for information already, so:

Full disclaimer: I'm not 100% on what a solution to this issue looks like.  It might be easy (using onRetainInstanceState or some other Android API), or it might be hard (re-write the relevant things to use Fragments).

At [1], you can see some code that persists the password shown state across new activities.  We need to do something like this when we get rotated.  That is, we need to persist an Activity state Bundle containing this information across the configuration change and restore it in onResume.  Post some work in progress as you make it so we can discuss what this looks like.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountAbstractSetupActivity.java#396

Comment 12

[swaroop.rao]

OK, not yet very clear on how I should do this. I'll refresh my memory about the Activity lifecycle. Do you think I can also use onSaveInstanceState() and onRestoreInstanceState()?

Comment 13

[swaroop.rao]

Nick, do you know which Activity class is responsible for the screen for which this defect has been reported? Is it FxAccountGetStartedActivity.java? As a first step, I would like to implement both onSaveInstanceState() and onRestoreInstanceState() and see if it does the trick. Let me know if you have any other ideas.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #13)
> Nick, do you know which Activity class is responsible for the screen for
> which this defect has been reported? Is it FxAccountGetStartedActivity.java?
> As a first step, I would like to implement both onSaveInstanceState() and
> onRestoreInstanceState() and see if it does the trick. Let me know if you
> have any other ideas.

It's not very well-factored code.  If we were doing it again, we'd do it as a Fragment; but right now, it's some code shared by the sign in [1], sign up [2], and update credentials [3] activities.  All three inherit from FxAccountAbstractSetupActivity, which owns the password EditText and button [4].  That's where shared code for all three should live.  So you can try the onSave/RestoreInstanceState there.  I thought that was Fragments only, however...

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountSignInActivity.java

[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java

[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountUpdateCredentialsActivity.java

[4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountAbstractSetupActivity.java#83

Comment 15

[swaroop.rao]

I tried this and it seems to work. However, I think we should also include the 'Year of Birth' field while we're at it, because the contents of that field are getting reset when the orientation changes.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #15)
> I tried this and it seems to work. However, I think we should also include
> the 'Year of Birth' field while we're at it, because the contents of that
> field are getting reset when the orientation changes.

In fact, we should include the email/password and some other stuff too.  (See the re-create on change activity logic.)  I'm pleased this seems to work; can you post work in progress?

Comment 17

[swaroop.rao]

OK, can we chat for a bit on IRC? I want to know a couple of things:
a. Where can I find the 're-create on change activity logic'?
b. How do I post work in progress?

Comment 18

[swaroop.rao]

Nick, I don't understand how the variables emailEdit & passwordEdit are being used in FxAccountAbstractSetupActivity.java without being set/assigned any value in the file. onCreate() in this class just calls super.onCreate() but the parent class (FxAccountAbstractActivity) does not implement the onCreate() method at all, so I guess it gets passed up the hierarchy. I want to implement the onSaveInstanceState for the "Year" field but I don't know how to go about this. If I create a variable for this field, can I assume that it will somehow get set automatically and refer to it or do I have to do something to ensure that it will get set?

Comment 19

[swaroop.rao]

OK, never mind, I see it now. It's assigned/set in a derived class (FxAccountCreateActivity, FxAccountSignInActivity, etc.).

Comment 20

[swaroop.rao]

I'm working on both this defect as well as defect #1002888. I've got fixes ready for both and tested them. Can I submit both simultaneously? I don't know how to post work in progress, but I can post the output of 'hg diff' if that will help. The code is not yet optimized in the sense that I haven't pulled out some common code into a helper method. I've just copy-pasted the code in a different classs to see if it works. Before submitting the patch, I will make the modifications and test it.

Comment 21

[swaroop.rao]

I am submitting the patch for the fix. I'm not sure if the format of the patch is correct. I followed the instructions given in the Mozilla developer site but I'd like somebody to review and confirm.

Comment 22

[swaroop.rao]

Attachment: Patch contains changes to a couple of class files

Fix for this bug involved writing onSaveInstanceState & onRestoreInstanceState in a couple of classes.
M mobile/android/base/fxa/activities/FxAccountAbstractSetupActivity.java
M mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java

FxAccountAbstractSetupActivity.java contains information about the email address and password fields whereas FxAccountCreateAccountActivity.java contains the field for the Year of Birth which should also retain its selection if the screen is rotated.

Comment 23

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8504614 [details] [diff] [review]
Patch contains changes to a couple of class files

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

Hey, sorry for the delay, but I expect to get to this Tuesday or Wednesday next week.

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8504614 [details] [diff] [review]
Patch contains changes to a couple of class files

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

Much of this work already exists, see |makeExtrasBundle| and |updateFromIntentExtras|.  I'm glad |onSaveInstanceState| works, but I'd like to see the |onRestoreInstanceState| changes worked into the existing |onCreate| flow.  That could either be by updating the existing approach, or moving the existing stuff into |onRestoreInstanceState|.  Wherever it lives, it needs to handle switching Activities (with the existing button link), configuration changes, and App getting backgrounded.

Thanks for doing the much needed research and working out a first approach!

::: mobile/android/base/fxa/activities/FxAccountAbstractSetupActivity.java
@@ +538,5 @@
> +    outState.putString(EXTRA_EMAIL, email);
> +    outState.putString(EXTRA_PASSWORD, password);
> +
> +    boolean isPasswordShown = passwordEdit.getTransformationMethod() instanceof SingleLineTransformationMethod;
> +    outState.putBoolean(EXTRA_PASSWORD_SHOWN, isPasswordShown);

There already exist essentially this, see |makeExtrasBundle|.  Use it in onSaveInstanceState.

@@ +542,5 @@
> +    outState.putBoolean(EXTRA_PASSWORD_SHOWN, isPasswordShown);
> +  }
> +
> +  protected void onRestoreInstanceState (Bundle savedInstanceState) {
> +    emailEdit.setText(savedInstanceState.getString(EXTRA_EMAIL));

Reading http://developer.android.com/reference/android/app/Activity.html#onRestoreInstanceState%28android.os.Bundle%29, it seems that the existing code (using |makeExtrasBundle|, etc) in onCreate should work with minor tweaks.  So I'd like to see this work put into that path so that we have this type of instance state save/restore in just one place.

::: mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java
@@ +49,5 @@
>  public class FxAccountCreateAccountActivity extends FxAccountAbstractSetupActivity {
>    protected static final String LOG_TAG = FxAccountCreateAccountActivity.class.getSimpleName();
>  
>    private static final int CHILD_REQUEST_CODE = 2;
> +  public static final String EXTRA_YEAR = "year";

This already exists elsewhere.

Comment 25

[swaroop.rao]

Nick, the reason I pulled it out as a separate piece (just for EXTRA_YEAR) in FxAccountCreateAccountActivity is that FxAccountAbstractSetupActivity is a parent class for FxAccountUpdateCredentialsActivity and FxAccountSignInActivity as well. I don't think those activities have the EXTRA_YEAR field, so I cannot save/restore that field's contents in the parent class.

makeExtrasBundle() creates a new Bundle but onSaveInstanceState() actually receives an existing Bundle object as a parameter. Shouldn't I be using that? Will we lose any data in the existing bundle if I just call makeExtrasBundle()? On the other hand, if I change makeExtrasBundle() to accept a new paramter (the existing bundle), I'm not sure if it will impact existing functionality in any way. I will check if it is called anywhere else.

Regarding your comment about minor tweaks to onCreate(), I will look into it a bit. I'm assuming this applies only to the code in onRestoreInstanceState(), right?

Comment 26

[swaroop.rao]

I found something in the code that puzzles me a bit. Tried to reach you on IRC but didn't get a response. In mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java, currently, makeExtrasBundle() calls super.makeExtrasBundle() which returns a Bundle object and then adds the EXTRA_YEAR field to that bundle using a call to Bundle.putString(). However, in updateFromIntentExtras() in the same class, it is retrieving (or trying to retrieve) the EXTRA_YEAR field using getIntent().getStringExtra(). In other words, it is not retrieving it from the Bundle object at all. I tried reading the documentation of Intent.getStringExtra() and Bundle.putString() but I'm still not clear if Intent.getStringExtra() actually retrieves a String from a bundle or not. If not, that may be the real reason that it wasn't working (see my comment #15 https://bugzilla.mozilla.org/show_bug.cgi?id=1040572#c15). I can replace the call to Intent.getStringExtra() in updateFromIntentExtras() with a call to Bundle.getString().

Comment 27

[swaroop.rao]

Nick, did you get a chance to look into this?

Comment 28

[swaroop.rao]

Nick, will you be able to review my comment (comment #26) some time this week? Or, can I get somebody else to review it so I can figure out how to close this out and move on to other bugs?

Comment 29

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #26)
> I found something in the code that puzzles me a bit. Tried to reach you on
> IRC but didn't get a response. In
> mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java,
> currently, makeExtrasBundle() calls super.makeExtrasBundle() which returns a
> Bundle object and then adds the EXTRA_YEAR field to that bundle using a call
> to Bundle.putString(). However, in updateFromIntentExtras() in the same
> class, it is retrieving (or trying to retrieve) the EXTRA_YEAR field using
> getIntent().getStringExtra(). In other words, it is not retrieving it from
> the Bundle object at all. I tried reading the documentation of
> Intent.getStringExtra() and Bundle.putString() but I'm still not clear if
> Intent.getStringExtra() actually retrieves a String from a bundle or not. If
> not, that may be the real reason that it wasn't working (see my comment #15
> https://bugzilla.mozilla.org/show_bug.cgi?id=1040572#c15). I can replace the
> call to Intent.getStringExtra() in updateFromIntentExtras() with a call to
> Bundle.getString().

Hey!  Sorry for the delay.

I'm pretty sure that Intent.getStringExtra() is equivalent to Intent.getExtras().getString().  You can see this by comparing the notes for http://developer.android.com/reference/android/content/Intent.html#getExtras%28%29 and http://developer.android.com/reference/android/content/Intent.html#getStringExtra%28java.lang.String%29.

So the underlying Bundle is the same.  You might want to make that explicit, though: rename updateFromIntentExtras to be updateFromExtras, make it take a Bundle as parameter, and then pass in the Intent.getExtras() to be read from.  You could also rename makeExtrasBundle to be updateExtrasBundle, make it take a Bundle as a parameter, and then pass whatever you're supposed to update in onSaveInstanceState.

Again, sorry this took so long.  I'll try to stay on my usual schedule so that we can move this forward faster.

Comment 30

[swaroop.rao]

Attachment: Redid the changes by pasing a bundle and removing the usage of the Intent object

I had to modify another method to accept the Bundle as a parameter so there were more changes that I had planned because they had a cascading effect on a couple of other classes that used those methods. Anyway, it's all done now and basic sanity testing works correctly. Should I run the robocop tests to do more sanity testing? Anyway, I'm submitting the patch for now. Let me know your thoughts.

Comment 31

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8515025 [details] [diff] [review]
Redid the changes by pasing a bundle and removing the usage of the Intent object

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

This is looking good, but I'd like to see another pass.

I have a bigger question, too: does this whole thing get simpler if we get rid of my old "makeExtrasBundle/updateFromExtrasBundle" and just always use onSaveInstanceState and onRestoreInstanceState?  There's really only one unusual time that we want to save state, and that's in the button click for going between Sign In and Sign Up.  Presumably we could use onSaveInstanceState there and get a simpler implementation?

::: mobile/android/base/fxa/activities/FxAccountAbstractSetupActivity.java
@@ +390,5 @@
>    protected void onCreate(Bundle savedInstanceState) {
>      super.onCreate(savedInstanceState);
>    }
>  
> +  protected void updateFromExtras(Bundle bundle) {

Call this updateFromBundle, and lift the getIntent().getExtras() out to the caller.  This is a generic thing now, it shouldn't depend on the intent at all.

@@ +427,5 @@
>      };
>      task.execute();
>    }
>  
> +  protected Bundle makeExtrasBundle(Bundle savedBundle, String email, String password) {

Don't return a Bundle here, and make it an IllegalArgumentException to pass a null bundle.

@@ +429,5 @@
>    }
>  
> +  protected Bundle makeExtrasBundle(Bundle savedBundle, String email, String password) {
> +    final Bundle bundle;
> +    if (savedBundle != null)

nit: we always brace conditionals.  So:

if (savedBundle != null) {
  bundle = savedBundle;
} else {
  bundle = new Bundle();
}

::: mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java
@@ +49,5 @@
>  public class FxAccountCreateAccountActivity extends FxAccountAbstractSetupActivity {
>    protected static final String LOG_TAG = FxAccountCreateAccountActivity.class.getSimpleName();
>  
>    private static final int CHILD_REQUEST_CODE = 2;
> +  public static final String EXTRA_YEAR = "year";

I think this is already exposed in AbstractSetupActivity.  It's definitely somewhere.  Use the existing one or unify the two.

@@ +350,5 @@
>        }
>      });
>    }
> +
> +  protected void onSaveInstanceState (Bundle outState) {

nit: no space before parens: ...State(Bundle outState) {

@@ +356,5 @@
> +    String year = yearEdit.getText().toString();
> +    outState.putString(EXTRA_YEAR, year);
> +  }
> +
> +  protected void onRestoreInstanceState (Bundle savedInstanceState) {

I think this is not needed.  We should restore the state in the onCreate call, not afterwards.

::: mobile/android/base/fxa/activities/FxAccountSignInActivity.java
@@ +68,5 @@
>          startActivityInstead(FxAccountCreateAccountActivity.class, CHILD_REQUEST_CODE, extras);
>        }
>      });
>  
> +    updateFromExtras(null);

ditto: extract the intent extras, pass it through.

::: mobile/android/base/fxa/activities/FxAccountUpdateCredentialsActivity.java
@@ +77,5 @@
>  
>      TextView view = (TextView) findViewById(R.id.forgot_password_link);
>      ActivityUtils.linkTextView(view, R.string.fxaccount_sign_in_forgot_password, R.string.fxaccount_link_forgot_password);
>  
> +    updateFromExtras(null);

IIRC, we launch this with a bundle that contains the email address.  So this will break that.  Extract the intent extras bundle and feed it through.  (This is part of my comment on AbstractSetupActivity about making callers provide the bundle.)

Comment 32

[swaroop.rao]

Nick, I've completed the changes you suggested. I have a question about expected behavior in one scenario, though. If the user starts off trying to create a new Sync account (types in email address, password and selects a year of birth) and then realizes that he/she already has an account and clicks on the link provided to sign in, should the information be carried over to the new activity? Currently, this scenario not only fails but an exception is thrown at the line that I just added based on your suggestion (to throw an IllegalArgumentException if bundle is null in FxAccountAbstractSetupActivity.makeExtrasBundle(). In other words, the 'icicle' argument passed to FxAccountCreateAccountActivity.onCreate() is null. So, I have two options. I can check for icicle's "null-ness" and initialize a new Bundle object and pass it to FxAccountCreateAccountActivity.onCreate() or I can remove the line where the exception is thrown and instead return quietly (I'd rather avoid this, actually). Of course, I could also catch the exception and do something with it but I'm not sure what. Thoughts?

Comment 33

[swaroop.rao]

Attachment: v3 of fix for this bug; Incorporated review feedback and fixed other stuff

Made the changes that were recommended in Nick's review feedback. Removed onRestoreInstanceState() and moved code to updateFromBundle() because that is called from onCreate(). Also, ensured that switching from 'Create Account' to 'Sign In' and back also works (information is carried over).

Comment 34

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8517523 [details] [diff] [review]
v3 of fix for this bug; Incorporated review feedback and fixed other stuff

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

This is looking mostly good; I think we should go all the way and use on{Save,Restore}InstanceState throughout.

Scenarios that we will test:

1) swapping between Sign up and Sign in;
2) trying to Signing up with an existing account and tapping Sign in instead;
3) rotating the phone in Sign up, Sign in, and update credentials;
4) forgetting the password and tapping the update credentials activity (hard to test but I can do this).

::: mobile/android/base/fxa/activities/FxAccountAbstractSetupActivity.java
@@ +442,5 @@
>        email = emailEdit.getText().toString();
>      }
>      if (password == null) {
>        password = passwordEdit.getText().toString();
>      }

nit: delete trailing whitespace.

::: mobile/android/base/fxa/activities/FxAccountCreateAccountActivity.java
@@ +113,5 @@
>    }
>  
>    @Override
> +  protected void updateFromBundle(Bundle bundle) {
> +    super.updateFromBundle(bundle);

Morph updateFromBundle into onRestoreInstanceState.  That is, we're opting in to the default Android mechanism.  Make sure to call super!

@@ +358,5 @@
>      });
>    }
> +
> +  protected void onSaveInstanceState(Bundle outState) {
> +    makeExtrasBundle(outState, null, null);

nit: @Override.

Now onSaveInstanceState and makeExtrasBundle are doing the same thing.  Morph makeExtrasBundle *into* onSaveInstanceState throughout (you can use it with a fresh bundle wherever you want).  Make sure to call super!

::: mobile/android/base/fxa/activities/FxAccountSignInActivity.java
@@ +78,4 @@
>        }
>      });
>  
> +    updateFromBundle(icicle);

If we make updateFromBundle onRestoreInstanceState, this gets called automatically for us.

Comment 35

[swaroop.rao]

Nick, there seem to be instances when makeExtrasBundle() is called outside the context of a configuration change like when one activity invokes the next activity. In those situations, we cannot depend on onSaveInstanceState() because it won't be called, right? Don't we need to retain makeExtrasBundle() for these situations?

Comment 36

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #35)
> Nick, there seem to be instances when makeExtrasBundle() is called outside
> the context of a configuration change like when one activity invokes the
> next activity. In those situations, we cannot depend on
> onSaveInstanceState() because it won't be called, right? Don't we need to
> retain makeExtrasBundle() for these situations?

I assumed -- and maybe I'm wrong -- that we could call onSaveInstanceState(newBundle, ...) ourselves.  As a straight replacement.

One question I thought about while falling asleep: is it safe to pass one Activity's instance state into another?  That seems risky.  I think the right thing to do is to try onSaveInstanceState and to inspect the Bundles created.  We might want to whitelist certain keys of the Bundle in just this instance: that is, delete everything we don't recognize just when passing to the other Activity.

Comment 37

[swaroop.rao]

I'll try removing makeExtrasBundle()/updateFromBundle() completely and replacing them with onSaveInstanceState()/onRestoreInstanceState() and see if the app continues to work as expected, especially in the scenarios that you listed.

Regarding your other comment/question, how will I know the whitelisted keys for each activity that are safe to pass on to the next activity? Are there others apart from the ones I know of, viz. email address, password, year of birth and hide/show password?

Comment 38

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #37)
> I'll try removing makeExtrasBundle()/updateFromBundle() completely and
> replacing them with onSaveInstanceState()/onRestoreInstanceState() and see
> if the app continues to work as expected, especially in the scenarios that
> you listed.

See how that goes.

> Regarding your other comment/question, how will I know the whitelisted keys
> for each activity that are safe to pass on to the next activity? Are there
> others apart from the ones I know of, viz. email address, password, year of
> birth and hide/show password?

The whitelisted keys are the ones you're thinking of: KEY_{EMAIL,PASSWORD,YEAR,PASSWORD_SHOWN}, etc.

Comment 39

[swaroop.rao]

Attachment: Replaced makeExtrasBundle() and updateFromBundle() with onSaveInstanceState() and onRestoreInstanceState() respectively

Done. Now, we're only using onSaveInstanceState() and onRestoreInstanceState(). I think I've tested the scenarios you've mentioned and they all appeared to work correctly. Let me know if you notice any problem areas.

Comment 40

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8519000 [details] [diff] [review]
Replaced makeExtrasBundle() and updateFromBundle() with onSaveInstanceState() and onRestoreInstanceState() respectively

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

This doesn't work as written -- try going from create to sign in back to create.  You'll lose state.

I've dug into this locally; it's tricky.  I have an approach, a little different than yours, that seems to work.  I'll post WIP.  Perhaps you could test locally and see if it makes sense to you?

Comment 41

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Pass Bundle instances explicitly.

Comment 42

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Save state when configuration changes. r=rnewman

Comment 43

[Nick Alexander :nalexander [he/him]]

Here's a slightly different approach.  What I did was make clear that we're handling bundles; clarify where the bundles are coming from; and then, in the final part, save a bundle when we get killed/rotated.  The existing code (in onCreate) already correctly handled the incoming bundles.

We mis-interpreted onRestoreInstanceState; it doesn't always get called.  For instance, it's not called on a regular creation.  So we need to keep something in onCreate; might as well keep what we had.

We need to pass through a lot of what we were created with 'cuz that's how our consumers message things to us.  And changing that code too much is risky and not tested well.  (No automatic testing at all, really.)

Comment 44

[swaroop.rao]

Actually, I did test that scenario you mentioned (Sign In to Create and back) and it worked correctly for me. I just tried it now again and it works. The only issue is that the 'Year of Birth' field does not get populated because the Sign In activity does not know about that field to save in the bundle before calling the Create Account activity.

Anyway, about your WIP code, I haven't done this before so some tips/pointers would be appreciated. How do I use the two patch files? From what I understand, I should do a 'hg qpop' to undo my changes for this defect, then use 'patch -p1....' to apply your two patch files. Let me know if I should do it differently.

Comment 45

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html

Comment 46

[swaroop.rao]

I can continue working on this since it seems to be still awaiting a fix. Let me know if it's OK for me to take this. I have a build set up and ready to go.

Comment 47

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #46)
> I can continue working on this since it seems to be still awaiting a fix.
> Let me know if it's OK for me to take this. I have a build set up and ready
> to go.

Hi Swaroop -- thanks for checking in, but this code is gone as of Bug 1220892.  I don't have a good ticket for you right now, but perhaps https://bugzilla.mozilla.org/1222607 or https://bugzilla.mozilla.org/1226209 is interesting to you?