1186683 - BrowserSearch Fragment is destroyed when hidden

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Even when it's just from deleting the last character causing the search engines to be re-retrieved. We remove the Fragment [1] but perhaps we should hide [2] or detach [3] it to prevent (minimal?) memory churn.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=4d01cb784d07#2835

[2]: http://developer.android.com/reference/android/app/FragmentTransaction.html#hide%28android.app.Fragment%29
[3]: http://developer.android.com/reference/android/app/FragmentTransaction.html#detach%28android.app.Fragment%29

Comment 1

[Prabhjyot Sodhi [:psd]]

Attachment: 1186683.patch

Comment 2

[Prabhjyot Sodhi [:psd]]

Hey Michael!
Just got to know that you were the mentor for Fennec's intern this summer. An internship with Mozilla sounds too cool! I gotta apply :)
Anyway, do you have some other bug that I could hack on?

Comment 3

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

Sorry, I'll get to this review tomorrow.

(In reply to Prabhjyot Sodhi [:psd] from comment #2)
> Just got to know that you were the mentor for Fennec's intern this summer.
> An internship with Mozilla sounds too cool! I gotta apply :)

:)

> Anyway, do you have some other bug that I could hack on?

Perhaps bug 1178519?

Comment 4

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

Comment on attachment 8655014 [details] [diff] [review]
1186683.patch

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

With the exception of the boolean comment below, looks good to me! Thanks Prabhjyot!

Don't forget to update the commit message of your patch.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a4a3cf59e6

::: mobile/android/base/BrowserApp.java
@@ +2816,5 @@
>          fm.executePendingTransactions();
>  
> +        Fragment f = fm.findFragmentById(R.id.search_container);
> +
> +        if(!mAddedBrowserSearchFragment) {

nit: `if (`

Rather than keeping a boolean for the added state, it looks like findFragmentById returns null if the fragment is not found [1].

[1]: https://developer.android.com/reference/android/app/FragmentManager.html#findFragmentById%28int%29

Comment 5

[Prabhjyot Sodhi [:psd]]

Attachment: 1186683.patch

Comment 6

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

Comment on attachment 8657418 [details] [diff] [review]
1186683.patch

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

lgtm – for the sake of avoiding churn, let's just leave the nit in.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbfad5a57c2

::: mobile/android/base/BrowserApp.java
@@ +2810,5 @@
>          // calling FragmentTransaction#add immediately after FragmentTransaction#remove won't add the fragment's
>          // view to the layout. Calling FragmentManager#executePendingTransactions before re-adding the fragment
>          // prevents this issue.
>          fm.executePendingTransactions();
> +        Fragment f = fm.findFragmentById(R.id.search_container);

nit: we generally prefer more descriptive variable names. 

i.e. here I might use searchContainerFragment

Comment 7

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a004fc4f59

Comment 8

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

Prabhjyot, if you're interested in more bugs, may I recommend bug 1202091 or bug 1056031?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b34347f5bfb8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b34347f5bfb8

Comment 11

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

Prabhjyot, it was decided in triage that we should fix the regressions from bug 1206628 and bug 1206639 before this ships so we're going to back it out of our aurora channel (FF43). It will stay on Nightly – do you have time to fix the regressing bugs?

Comment 12

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

Backout: https://hg.mozilla.org/releases/mozilla-aurora/rev/944086c87a88

Not sure if I should update Target Milestone & other flags.

Comment 13

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

Just FYI, it seems we never handled state restoration at all – there are 3 blocking bugs and more discovered in bug 1207872 comment 4 (soon to be blocking?). Let's make sure these are cleaned up by the end of the cycle. Backout is also possible but it may start to get intertwined with Ally's search suggestion work and be difficult to do.

Comment 15

[Prabhjyot Sodhi [:psd]]

Clearing old Need info, as query was resolved on IRC.