1056225 - Factor out shared pieces of HomeFragment

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Nick Alexander :nalexander [he/him]]

There's some duplication as HomeFragments have grown.  Two I've noticed:

1) Most HomeFragments have:

    // On URL open listener
    private OnUrlOpenListener mUrlOpenListener;

2) Most HomeFragments have:

    @Override
    public void onConfigurationChanged(Configuration newConfig) {
        super.onConfigurationChanged(newConfig);

        // Detach and reattach the fragment as the layout changes.
        if (isVisible()) {
            getFragmentManager().beginTransaction()
                                .detach(this)
                                .attach(this)
                                .commitAllowingStateLoss();
        }
    }

(With variable degrees of commenting.)

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Lift mUrlOpenListener to HomeFragment. r=margaret

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret

I took the most commented version.  There are two things to note here:

* Several panels did not define onConfigurationChanged.  It's not clear
  if these panels didn't need it (after some analysis?) or if they just
  didn't copy-paste thoroughly.  I think this version is always safe, if
  potentially inefficient.

* The order of operations for the Bookmarks panel is delicate.  I
  preserved the original order (save stack first, then detach and
  attach).  I tested that the folder stack was preserved across device
  rotations locally.

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #2)
> Created attachment 8476132 [details] [diff] [review]
> Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret
> 
> I took the most commented version.  There are two things to note here:
> 
> * Several panels did not define onConfigurationChanged.  It's not clear
>   if these panels didn't need it (after some analysis?) or if they just
>   didn't copy-paste thoroughly.  I think this version is always safe, if
>   potentially inefficient.

A HomeFragment only needs to handle onConfiguration change (i.e. re-attach) if their UI needs to change (i.e. re-inflate layouts, use different styles, etc) for different device orientations. Handling configuration changes in all HomeFragments will simply cause some redundant re-inflations on device rotation. But please double-check any subtle assumptions around the fragment's life-cycle in each panel.

ps: I haven't looked at the patch. Just providing more context here.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> (In reply to Nick Alexander :nalexander from comment #2)
> > Created attachment 8476132 [details] [diff] [review]
> > Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret
> > 
> > I took the most commented version.  There are two things to note here:
> > 
> > * Several panels did not define onConfigurationChanged.  It's not clear
> >   if these panels didn't need it (after some analysis?) or if they just
> >   didn't copy-paste thoroughly.  I think this version is always safe, if
> >   potentially inefficient.
> 
> A HomeFragment only needs to handle onConfiguration change (i.e. re-attach)
> if their UI needs to change (i.e. re-inflate layouts, use different styles,
> etc) for different device orientations. Handling configuration changes in
> all HomeFragments will simply cause some redundant re-inflations on device
> rotation. But please double-check any subtle assumptions around the
> fragment's life-cycle in each panel.

Yep, that's my understanding.  I think I'll fold a version of your comment into the HomeFragment.onConfigurationChanged docstring.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8476132 [details] [diff] [review]
Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret

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

Looks good to me.

It looks like BrowserSearch and HistoryPanel are the other panels that don't currently have this logic, so it might be worth just testing those two thing out to make sure there's no problems there.

::: mobile/android/base/home/BookmarksPanel.java
@@ +145,3 @@
>          }
> +
> +        super.onConfigurationChanged(newConfig);

I think you should add a comment here explaining that there's a reason for this ordering.

Comment 6

[Nick Alexander :nalexander [he/him]]

Landed, with reversed onConfigurationChange order in BookmarksPanel (and comments explaining why):

https://hg.mozilla.org/integration/fx-team/rev/11fb420e79b6
https://hg.mozilla.org/integration/fx-team/rev/cb646684a158

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/11fb420e79b6
https://hg.mozilla.org/mozilla-central/rev/cb646684a158