1129168 - Remote Tabs panel flickers after Bug 1063742

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

STR: Scroll to Synced Tabs panel; scroll to adjacent panel; return to Synced Tabs panel; observe flicker as panel is re-painted.

Is this due to the fragment cache not working?  Is this due to forced load() calls?  Is it possible we're seeing multiple loading paints?  Do we have an issue with the Firefox Account Loader?

This is a regression from Bug 1063742.

Comment 1

[Nick Alexander :nalexander [he/him]]

I would like this to track Fennec 38.

Comment 2

[Nick Alexander :nalexander [he/him]]

margaret: I think this is related to the load issue you described in https://bugzilla.mozilla.org/show_bug.cgi?id=1063742#c38.

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: 1129168.patch

Patch changes:
1) Fragment Cache extended to include orientation along with action
2) Panel reload avoided for every rotation in tablets


@Nick:
Can you please test this on tablet

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8563160 [details] [diff] [review]
1129168.patch

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

This is all sensible, but it doesn't address the actual problem.  To see the flash (on all of my devices), select Synced Tabs, scroll manually all the way to the end of the panels, then scroll back to Synced Tabs.  You should see the flash.

What is happening is that the underlying RemoteTabsPanel fragment is *different*.  That clears the cache entirely.  I don't know why we see the internal fragments when we scroll back -- I guess they are still in the view hierarchy? -- when the RemoteTabsPanel fragment is replaced.  You can verify this by printing this and the fragment cache, etc, during the show operation.

I checked out the relevant sources from revision 145b1a56988e^ (the revision before landing Bug 1063742) and the flash is present.  It may have to do with reloading the tab data itself.

In any case, this doesn't address the issue on any of my devices.  I'm not sure we care enough to keep pushing on this right now.

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +23,5 @@
>  import android.os.Bundle;
>  import android.support.v4.app.Fragment;
>  import android.support.v4.app.LoaderManager.LoaderCallbacks;
>  import android.support.v4.content.Loader;
> +import android.support.v4.util.Pair;

Are you kidding me?  How did I not know about this!

@@ +54,5 @@
>      private Fragment mCurrentFragment;
>  
>      // A lazily-populated cache of fragments corresponding to the possible
>      // system account states. We don't want to re-create panels unnecessarily,
> +    // because that can cause flickering. Key is a pair of {@link android.content.res.Configuration}

Can you enumerate the expected values: one of 

ORIENTATION_LANDSCAPE, ORIENTATION_PORTRAIT, or ORIENTATION_UNDEFINED (for the case when it doesn't matter).

@@ +216,5 @@
>              }
>              return mFallbackFragment;
>          }
>  
> +        // Using orientation to generate key pair in tablets.

try:

// When the layout depends on the device orientation, cache the fragments for each orientation keyed by the orientation constant.  Otherwise, cache the fragment keyed by unknown.

And this is one of the rare times when the helper method obscures rather than clarifies.  I'd fold the logic inline here to keep it all in one place.

@@ +224,3 @@
>          // On a tablet devices with accounts authenticated, create a new fragment based on the current orientation.
>          // The cached fragment in the above case may not be the valid fragment for the current orientation.
>          if (fragment == null || (HardwareUtils.isTablet() && actionNeeded == Action.None)) {

Shouldn't you not need the || clause here?  I expect you to handle this special case at the caching layer.

Comment 5

[Nick Alexander :nalexander [he/him]]

margaret: you are quite familiar with the panel loading code -- did you ever see the behaviour above?  Did you work around it some way?

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 1129168.patch

Remote panel flicker changes.
* Cached fragment based on orientation
* mCurrentFragment updated from fragment manager, this avoids unnecessarily reloading a new fragment, which Fragments are recreated.

Comment 7

[Vivek Balakrishnan[:vivek]]

@Nick: 
Can you please help me test this on tablet

Comment 8

[:Margaret Leibovic]

Vivek, what's the status here? I remember nalexander talking to me last week about how you two figured out what's going wrong here. Is this patch still the most up-to-date version? I can help you test on a tablet if you need that.

Comment 9

[Vivek Balakrishnan[:vivek]]

Margaret, I intend to submit new patch by EOD. The idea was to get rid of cache in RemoteTabsPanel and use the combination of childFragmentManager and tags based on action and orientation to avoid reloading the fragment if it was already handled by the framework.

Comment 10

[Vivek Balakrishnan[:vivek]]

Attachment: 1129168.patch

A patch to prevent flickering:
* Fragment are committed to transaction with  a tag. Account action and current orientation are stored as fragment args.
* Upon fragment recreation, these values are retrieved from the fragment in the back-stack to decide whether a new fragment (for current orientation) needs to created.
* Fragment cache was removed. Fragment life-cycle is handled automagically by the android framework.

@Margaret : can you please help me test this on a tablet.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8570228 [details] [diff] [review]
1129168.patch

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

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +71,5 @@
> +            final int fragmentOrientation = bundle.getInt(FRAGMENT_ORIENTATION);
> +            if (fragmentAction.equals(Action.None.name()) && fragmentOrientation != GeckoScreenOrientation.getInstance().getAndroidOrientation()) {
> +                // The Fragment in the back-stack is not for the current configuration.
> +                // As the fragment becomes visible only after onStart callback, We can safely pop it from the back-stack.
> +                getChildFragmentManager().popBackStackImmediate();

I don't understand this.  How do you know the fragment you found using findFragmentByTag is at the top of the stack?

I feel like we might want to beginTransaction, remove, endTransaction to get it out of the back stack robustly.  Maybe?

@@ +85,5 @@
>          getLoaderManager().initLoader(LOADER_ID_ACCOUNT, null, mAccountLoaderCallbacks);
>      }
>  
> +    private void showSubPanel(Account account) {
> +        final Action actionNeeded = getActionNeeded(account);

Can you extract some helper that takes the actionNeeded and does the fishing out of the fragment manager below?  It seems like we do the same logic here and above.

Comment 12

[Vivek Balakrishnan[:vivek]]

Attachment: 1129168.patch

A new patch with review nits corrected.
* fragment removed through transaction
* common code moved to a function.

@Nick: can you please help me and test this in a tablet. thanks.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8573522 [details] [diff] [review]
1129168.patch

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

This looks good and works well on my tablet!  I'll land with some cosmetic changes (renaming and comments).

Comment 14

[Nick Alexander :nalexander [he/him]]

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

Comment 15

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

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

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8573522 [details] [diff] [review]
1129168.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1137655, among others.
[User impact if declined]: crashes switching from Synced Tabs panel.
[Describe test coverage new/current, TreeHerder]: manual testing by nalexander.
[Risks and why]: we could mutate the crash stack.
[String/UUID change made/needed]: none.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18b6678e7a96