Closed Bug 988355 Opened 6 years ago Closed 6 years ago

Only force reload on configuration changes in DynamicPanel

Categories

(Firefox for Android :: Awesomescreen, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: lucasr, Assigned: lucasr)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

The built-in panels already reconnect with their loaders automatically.
Comment on attachment 8397201 [details] [diff] [review]
Properly handle device rotation in DynamicPanel (r=margaret)

This is applied on top of bug 942281 and bug 987963 btw.
Attachment #8397201 - Flags: review?(margaret.leibovic)
Comment on attachment 8397201 [details] [diff] [review]
Properly handle device rotation in DynamicPanel (r=margaret)

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

::: mobile/android/base/home/DynamicPanel.java
@@ +143,5 @@
> +        // Restore whatever the UI mode the fragment had before
> +        // a device rotation.
> +        if (mUIMode != null) {
> +            setUIMode(mUIMode);
> +        }

We should also set mUIMode to null in onDestroyView, right?

@@ +252,5 @@
> +                // (e.g. auth cache changes) and the fragment is allowed
> +                // to load its contents. Any loaders associated with the
> +                // panel layout will be automatically re-bound after a
> +                // device rotation, no need to explicitly load it again.
> +                if (mUIMode != mode && canLoad()) {

Shouldn't this mUIMode != mode check happen much earlier? It seems we could avoid doing all of the work in this method if the UIMode isn't changing.

::: mobile/android/base/home/HomeFragment.java
@@ -207,1 @@
>      }

Get rid of this whole method, since it's not needed anymore.
Attachment #8397201 - Flags: review?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8397201 [details] [diff] [review]
> Properly handle device rotation in DynamicPanel (r=margaret)
> 
> Review of attachment 8397201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/DynamicPanel.java
> @@ +143,5 @@
> > +        // Restore whatever the UI mode the fragment had before
> > +        // a device rotation.
> > +        if (mUIMode != null) {
> > +            setUIMode(mUIMode);
> > +        }
> 
> We should also set mUIMode to null in onDestroyView, right?

Nope. On device rotation, we detach and then re-attach the fragment to the activity, see DynamicPanel's onConfigurationChanged(). This means the Fragment instance itself is retained on device rotation but the UI views are re-created using the normal flow onDestroyView -> onDetach -> onAttach -> onCreateView -> onViewCreated -> onActivityCreated, and so forth.

What this patch does is using the retained mUImode state to avoid re-querying the auth state asynchronously in order to decide about the UI mode. This means we're able to restore the UI mode synchronously on device rotation to take advantage of the loader reconnection (i.e. an onLoadFinished() call) on device rotation.

> @@ +252,5 @@
> > +                // (e.g. auth cache changes) and the fragment is allowed
> > +                // to load its contents. Any loaders associated with the
> > +                // panel layout will be automatically re-bound after a
> > +                // device rotation, no need to explicitly load it again.
> > +                if (mUIMode != mode && canLoad()) {
> 
> Shouldn't this mUIMode != mode check happen much earlier? It seems we could
> avoid doing all of the work in this method if the UIMode isn't changing.

We want to allow re-setting the UI mode to the same value to properly support the behaviour described above on device rotations. However, we only want to trigger load() if the UI mode is actually changing from auth to panel. 

> ::: mobile/android/base/home/HomeFragment.java
> @@ -207,1 @@
> >      }
> 
> Get rid of this whole method, since it's not needed anymore.

The review tool is not really showing which method you're talking about here.
(In reply to Lucas Rocha (:lucasr) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > Comment on attachment 8397201 [details] [diff] [review]
> > Properly handle device rotation in DynamicPanel (r=margaret)
> > 
> > Review of attachment 8397201 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/DynamicPanel.java
> > @@ +143,5 @@
> > > +        // Restore whatever the UI mode the fragment had before
> > > +        // a device rotation.
> > > +        if (mUIMode != null) {
> > > +            setUIMode(mUIMode);
> > > +        }
> > 
> > We should also set mUIMode to null in onDestroyView, right?
> 
> Nope. On device rotation, we detach and then re-attach the fragment to the
> activity, see DynamicPanel's onConfigurationChanged(). This means the
> Fragment instance itself is retained on device rotation but the UI views are
> re-created using the normal flow onDestroyView -> onDetach -> onAttach ->
> onCreateView -> onViewCreated -> onActivityCreated, and so forth.
> 
> What this patch does is using the retained mUImode state to avoid
> re-querying the auth state asynchronously in order to decide about the UI
> mode. This means we're able to restore the UI mode synchronously on device
> rotation to take advantage of the loader reconnection (i.e. an
> onLoadFinished() call) on device rotation.

Thanks for the clarification!

> > ::: mobile/android/base/home/HomeFragment.java
> > @@ -207,1 @@
> > >      }
> > 
> > Get rid of this whole method, since it's not needed anymore.
> 
> The review tool is not really showing which method you're talking about here.

Oh, sorry, that would be the onConfigurationChanged, which was just added in bug 987963.
Comment on attachment 8397201 [details] [diff] [review]
Properly handle device rotation in DynamicPanel (r=margaret)

Changing to an r+, just get rid of that neccessary onConfigurationChanged.
Attachment #8397201 - Flags: feedback+ → review+
https://hg.mozilla.org/mozilla-central/rev/420663b2faaa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.
Blocks: 1014030
You need to log in before you can comment on or make changes to this bug.