1034384 - [regression] Remote tabs panel disappears on device rotation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

1) Open the remote tabs panel
2) Rotate the device.

Expected: The panel rotates
Actual: The panel disappears

Regression from bug 1016613.

Comment 2

[Epransky]

Attachment: Bug-1034384-2.patch

I think there's a cleaner solution, so I'd be happy to hear a suggestion if you have one.  Otherwise, I think this also resolves the issue in Bug 1034607 from what I can tell...

Comment 3

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

Comment on attachment 8451444 [details] [diff] [review]
Bug-1034384-2.patch

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

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +464,5 @@
>          removeAllViews();
>  
>          LayoutInflater.from(mContext).inflate(R.layout.tabs_panel, this);
>          initialize();
> +        

nit: ws.

@@ +465,5 @@
>  
>          LayoutInflater.from(mContext).inflate(R.layout.tabs_panel, this);
>          initialize();
> +        
> +        if (mCurrentPanel == Panel.REMOTE_TABS) {

I think this code should go into `initialize()` so that way if the remote tabs panel is ever shown first, we won't have any errors.

However, this puts a dependency on mCurrentPanel != null, so then I would recommend adding an if block for that, throwing an IllegalStateException if we hit that case.

Comment 4

[Epransky]

Attachment: Bug-1034384-3.patch

Made the changes we discussed over IRC.

Comment 5

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

Comment on attachment 8451939 [details] [diff] [review]
Bug-1034384-3.patch

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

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +142,5 @@
>  
>          mPanelPrivate = (PanelView) findViewById(R.id.private_tabs_panel);
>          mPanelPrivate.setTabsPanel(this);
>  
> +        if (mCurrentPanel == null) {

You should move this code to the top of the method - typically state and error checking should occur at the top of the function as it improves visibility.

@@ +143,5 @@
>          mPanelPrivate = (PanelView) findViewById(R.id.private_tabs_panel);
>          mPanelPrivate.setTabsPanel(this);
>  
> +        if (mCurrentPanel == null) {
> +            throw new IllegalStateException("mCurrentPanel cannot be null in order for RemotePanelView to be initiated");

Add a comment to the javadoc of this function that mCurrentPanel must have a non-null value.

nit: "initiated" -> "initialized"

@@ +146,5 @@
> +        if (mCurrentPanel == null) {
> +            throw new IllegalStateException("mCurrentPanel cannot be null in order for RemotePanelView to be initiated");
> +        }
> +
> +        if (mCurrentPanel == Panel.REMOTE_TABS)

nit: While some existing code doesn't do this, from now on we intend to always brace ifs.

@@ +471,5 @@
>          removeAllViews();
>  
>          LayoutInflater.from(mContext).inflate(R.layout.tabs_panel, this);
> +
> +        //mPanelRemote must be null in order to properly initialize RemotePanelView

You should also include the information that the view mPanelRemote points to is invalidated because the layout is invalidated.

nit: Add a space after "//", i.e. `// mPanelRemote...`

Comment 6

[Epransky]

Attachment: Bug-1034384-3.patch

Tried to follow the guidelines here for Javadocs: https://wiki.mozilla.org/CloudServices/NativeSync#Formatting_Javadoc_comments

Comment 7

[Epransky]

Attachment: Bug-1034384-3.patch

This one's more correct I think

Comment 8

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

Comment on attachment 8452120 [details] [diff] [review]
Bug-1034384-3.patch

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

Sorry, I'm being pretty picky; otherwise, it looks good!

try: https://tbpl.mozilla.org/?tree=Try&rev=56859584d73c

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +138,5 @@
> +     *
> +     * @throws IllegalStateException
> +     *             mCurrentPanel must have a non-null value
> +     */
> +

nit: There should be no newline between the javadoc and the method header.

@@ +481,5 @@
>  
>          LayoutInflater.from(mContext).inflate(R.layout.tabs_panel, this);
> +
> +        // mPanelRemote must be null in order to properly initialize RemotePanelView
> +        // The View that mPanelRemote points to is invalidated because the layout is invalidated

nit: End sentences with a period. The second sentence is more important, so I'd put it on top.

Comment 9

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

I cancelled the try run in comment 8 because I accidentally included a hack patch to fix an issue I was having locally. Here's another: 

https://tbpl.mozilla.org/?tree=Try&rev=45535d12c20b

Comment 10

[Epransky]

Attachment: Bug-1034384-3.patch

Added the fix for Bug 1034607 as well.

Comment 11

[Epransky]

(In reply to Epransky from comment #10)
> Created attachment 8452814 [details] [diff] [review]
> Bug-1034384-3.patch
> 
> Added the fix for Bug 1034607 as well.

Should I do another try run for this?

Comment 12

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

Comment on attachment 8452814 [details] [diff] [review]
Bug-1034384-3.patch

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

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +118,5 @@
>              public void onResume() {
>                  if (mPanel == mPanelRemote) {
>                      // Refresh the remote panel.
>                      initializeRemotePanelView();
> +                    mPanel.show();

Why did you make this change? The other patch appears to fix bug 1034607.

Comment 13

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

(In reply to Epransky from comment #11)
> Should I do another try run for this?

If we don't add in the change that I questioned, no, since we didn't change the functionality. Otherwise, yes.

I also want to note that if this doesn't get r+'d today, we're going to back out bug 1016613 because it's been breaking Nightly for a few days. It's not a big deal, but it just means that you'll both have to reland bug 1016613 and this fix once it's ready.

Comment 14

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #12)
> Comment on attachment 8452814 [details] [diff] [review]
> Bug-1034384-3.patch
> 
> Review of attachment 8452814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tabspanel/TabsPanel.java
> @@ +118,5 @@
> >              public void onResume() {
> >                  if (mPanel == mPanelRemote) {
> >                      // Refresh the remote panel.
> >                      initializeRemotePanelView();
> > +                    mPanel.show();
> 
> Why did you make this change? The other patch appears to fix bug 1034607.

Hey,

After making some of the changes we discussed after that patch and testing, it no longer also resolved 1034607.  This change does the trick and resolves that bug as well. Can we go ahead and r+ this today so we don't have to back out 1016613? I think it works as expected.

Comment 15

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

Comment on attachment 8452814 [details] [diff] [review]
Bug-1034384-3.patch

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

Clearing review until Ethan has an explanation for how this change works, as per our discussion on IRC.

Comment 16

[Epransky]

Attachment: Bug-1034384-3.patch

So here's what I found.  Everything we had going in onResume() seems superfluous to me. I'm not sure what the actual intended behavior is here, but I removed all of the code in onResume() and achieved the same behavior as we had before the patch for Bug 1016613. The current patch addresses the issue in this bug and resolves Bug 1034607 as well.

For a short explanation, the solution here was that we needed to re-initialize mPanelRemote in refresh(). As for 1034607, I found 2 strange things which I'm still not sure why they were happening: 1) when we entered onResume(), mPanel == mPanelRemote, even though I'm not sure when this pointer was created, 2) initializing mPanelRemote (line 580) would break this pointer and cause mPanel == null.  All of this seemed unnecessary because I don't think there's any reason to retain this pointer. Calling show() never had any consequence as far as I could tell because the initial behavior was always to default back to the Normal PanelView anyways.

Does this make sense?

Comment 17

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

The reason mPanelRemote.show() [1] was added is to ensure the state of the RemoteTabsPanel is updated when Fennec is resumed. For example, if you have a synced account and the remote tabs are displayed but delete the account outside of Fennec (from the Android UI), the remote tabs panel should go back to the, "Welcome to Sync!" setup screen. However, `mPanelRemote.show()` is unintuitive and incorrect as we found out - perhaps we should add a `RemoteTabsPanel.refresh()` method and call that instead?

Thanks for the analysis.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/TabsPanel.java?rev=cb75d6cfb004#120

Comment 18

[Mark Capella [:capella]]

bug 1016613 has been backed out and this now appears to be fixed.

Comment 19

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

fixed: see comment 18.

Ethan, let's move this discussion to bug 1016613.

Comment 20

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in build:
Firefox for Android 33.0a1 (2014-07-15)
Devices:
Asus Transformer Pad TF300T (Android 4.2.1)
Samsung S3 (Android 4.3)