BrowserSearch Fragment is destroyed when hidden

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: psd, Mentored)

Tracking

unspecified
Firefox 43
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch 1186683.patch (obsolete) — Splinter Review
Attachment #8655014 - Flags: review?(michael.l.comella)
Assignee: nobody → prabhjyotsingh95
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?
Flags: needinfo?(michael.l.comella)
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?
Flags: needinfo?(michael.l.comella)
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
Attachment #8655014 - Flags: review?(michael.l.comella) → review+
Posted patch 1186683.patchSplinter Review
Attachment #8655014 - Attachment is obsolete: true
Attachment #8657418 - Flags: review?(michael.l.comella)
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
Attachment #8657418 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
Prabhjyot, if you're interested in more bugs, may I recommend bug 1202091 or bug 1056031?
https://hg.mozilla.org/mozilla-central/rev/b34347f5bfb8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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?
No longer blocks: 1206628, 1206639
Depends on: 1206628, 1206639
Flags: needinfo?(prabhjyotsingh95)
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.
Duplicate of this bug: 1186703
Clearing old Need info, as query was resolved on IRC.
Flags: needinfo?(prabhjyotsingh95)
You need to log in before you can comment on or make changes to this bug.