Closed Bug 1034384 Opened 6 years ago Closed 6 years ago

[regression] Remote tabs panel disappears on device rotation

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox33 --- verified
fennec 33+ ---

People

(Reporter: mcomella, Assigned: Epransky, Mentored)

References

Details

(Keywords: regression, reproducible)

Attachments

(1 file, 5 obsolete files)

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

Expected: The panel rotates
Actual: The panel disappears

Regression from bug 1016613.
tracking-fennec: --- → ?
Mentor: michael.l.comella
Duplicate of this bug: 1034609
Attached patch Bug-1034384-2.patch (obsolete) — Splinter Review
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...
Attachment #8451444 - Flags: review?(michael.l.comella)
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.
Attachment #8451444 - Flags: review?(michael.l.comella) → feedback+
Mentor: michael.l.comella
Attached patch Bug-1034384-3.patch (obsolete) — Splinter Review
Made the changes we discussed over IRC.
Attachment #8451444 - Attachment is obsolete: true
Attachment #8451939 - Flags: review?(michael.l.comella)
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...`
Attachment #8451939 - Flags: review?(michael.l.comella) → feedback+
Attached patch Bug-1034384-3.patch (obsolete) — Splinter Review
Tried to follow the guidelines here for Javadocs: https://wiki.mozilla.org/CloudServices/NativeSync#Formatting_Javadoc_comments
Attachment #8451939 - Attachment is obsolete: true
Attachment #8452107 - Flags: review?(michael.l.comella)
Attached patch Bug-1034384-3.patch (obsolete) — Splinter Review
This one's more correct I think
Attachment #8452107 - Attachment is obsolete: true
Attachment #8452107 - Flags: review?(michael.l.comella)
Attachment #8452120 - Flags: review?(michael.l.comella)
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.
Attachment #8452120 - Flags: review?(michael.l.comella) → feedback+
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
Attached patch Bug-1034384-3.patch (obsolete) — Splinter Review
Added the fix for Bug 1034607 as well.
Attachment #8452120 - Attachment is obsolete: true
Attachment #8452814 - Flags: review?(michael.l.comella)
(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 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.
(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.
(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 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.
Attachment #8452814 - Flags: review?(michael.l.comella)
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?
Attachment #8452814 - Attachment is obsolete: true
Attachment #8453554 - Flags: review?(michael.l.comella)
tracking-fennec: ? → 33+
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
bug 1016613 has been backed out and this now appears to be fixed.
fixed: see comment 18.

Ethan, let's move this discussion to bug 1016613.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8453554 - Flags: review?(michael.l.comella)
Target Milestone: --- → Firefox 33
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.