1016613 - ViewStub RemoteTabsPanel

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

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

I believe we currently inflate the RemoteTabsPanel when the tabs tray is initially opened - we should hold off on this until later, when the RemoteTabsPanel is opened.

Comment 1

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

via email.

Comment 2

[Epransky]

Attachment: Bug-1016613.patch

I managed to get to this today.  I think I got the hg right this time.  Let me know what I missed.  Thanks for the help!

Comment 3

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

Comment on attachment 8441757 [details] [diff] [review]
Bug-1016613.patch

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

Almost there!

::: mobile/android/base/resources/layout-large-land-v11/tabs_panel.xml
@@ +48,5 @@
>  
> +        <ViewStub android:id="@+id/remote_tabs_panel_stub"
> +                  android:layout="@layout/remote_tabs_panel_view"
> +                  android:layout_width="fill_parent"
> +                   android:layout_height="fill_parent"/>

nit: Indentation.

nit: `fill_parent` -> `match_parent`. Since we deprecated the older Android version that did not support the latter (2.2?), we use it.

::: mobile/android/base/resources/layout/remote_tabs_panel_view.xml
@@ +6,5 @@
> +<org.mozilla.gecko.tabspanel.RemoteTabsPanel xmlns:android="http://schemas.android.com/apk/res/android"
> +                                             android:id="@+id/remote_tabs_panel"
> +                                             android:layout_width="match_parent"
> +                                             android:layout_height="match_parent"
> +                                             android:background="@color/background_tabs"/>

Is this necessary? Shouldn't it be transparent by default, which means you'll see though to the actual TabsPanel?

::: mobile/android/base/resources/layout/tabs_panel.xml
@@ +48,5 @@
>  
> +        <ViewStub android:id="@+id/remote_tabs_panel_stub"
> +                  android:layout="@layout/remote_tabs_panel_view"
> +                  android:layout_width="fill_parent"
> +                  android:layout_height="fill_parent"/>

nit: `match_parent`.

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +165,5 @@
>              show(Panel.NORMAL_TABS);
>          } else if (index == 1) {
>              show(Panel.PRIVATE_TABS);
>          } else {
> +            if (mPanelRemote == null) {

This inflation should be in the show(...) method. What if show(...) is called elsewhere?

Comment 4

[Epransky]

Attachment: Bug-1016613.patch

Made all of the fixes you suggested, except for:

::: mobile/android/base/resources/layout/remote_tabs_panel_view.xml
@@ +6,5 @@
> +<org.mozilla.gecko.tabspanel.RemoteTabsPanel xmlns:android="http://schemas.android.com/apk/res/android"
> +                                             android:id="@+id/remote_tabs_panel"
> +                                             android:layout_width="match_parent"
> +                                             android:layout_height="match_parent"
> +                                             android:background="@color/background_tabs"/>

Is this necessary? Shouldn't it be transparent by default, which means you'll see though to the actual TabsPanel?

...

Are you just referring to removing the background attribute or the whole layout file? This layout file is necessary in general because we need to tell the ViewStub to what it should inflate, correct? Maybe I didn't entirely get the concept of the ViewStub but I thought this was necessary for it to inflate the RemoteTabsPanel...

Comment 5

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

(In reply to Epransky from comment #4)
> Are you just referring to removing the background attribute or the whole
> layout file?

The background attribute - sorry for the confusion! You seem to understand ViewStubs just fine. :)

Comment 6

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

Comment on attachment 8443297 [details] [diff] [review]
Bug-1016613.patch

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

::: mobile/android/base/resources/layout/remote_tabs_panel_view.xml
@@ +6,5 @@
> +<org.mozilla.gecko.tabspanel.RemoteTabsPanel xmlns:android="http://schemas.android.com/apk/res/android"
> +                                             android:id="@+id/remote_tabs_panel"
> +                                             android:layout_width="match_parent"
> +                                             android:layout_height="match_parent"
> +                                             android:background="@color/background_tabs"/>

Just want to figure the background out, then lgtm.

Comment 7

[Epransky]

Attachment: Bug-1016613.patch

Cool.  Took away the background attribute.

Comment 8

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

Comment on attachment 8443656 [details] [diff] [review]
Bug-1016613.patch

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

This appears to be the same patch. :) `hg qrefresh`?

Comment 9

[Epransky]

Attachment: Bug-1016613.patch

Oops, you're right.  Here's the correct one

Comment 10

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

Comment on attachment 8443660 [details] [diff] [review]
Bug-1016613.patch

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

lgtm. try: https://tbpl.mozilla.org/?tree=Try&rev=b6186c926250

Comment 11

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

Comment on attachment 8443660 [details] [diff] [review]
Bug-1016613.patch

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

via the try run in comment 10, looks like we're going to have another iteration.

Comment 12

[Epransky]

> via the try run in comment 10, looks like we're going to have another
> iteration.

Not sure what's going on here :/

Comment 13

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

If you open the linked try run from comment 10, you can click on different test suites to bring up the results of the test. For example, clicking on first orange "rc1" in "Android 4.0 Opt", you'll see a description of the error:

PROCESS-CRASH | java-exception | java.lang.NullPointerException at org.mozilla.gecko.tabspanel.TabsPanel$1.onResume(TabsPanel.java:121)

As well as some open bugs (those with stars next to them) that this issue could be (which are intermittent testing errors). In our case, there are no close matches that I can see, so we likely introduced this issue (that and the issue occurs in multiple test suites). Note that you can see extended logs by clicked "view brief log" and "view full log" on the bottom left.

Going to the file and line mentioned in the stack trace above, we see that line 121 is a close brace, likely because your changes are not in mxr or changes have been made to the files since we pushed the try run. However, if we scan around a bit, we can usually find the line, which appears now to be 120 [1]:

mPanelRemote.show();

Note that if you have not yet pulled and looked at these files locally, you should have the correct revisions and the line number should be correct.

I think line 120 is the line causing the issue because we made some changes to how mPanelRemote is initialized, and it has the potential to be null (hence the NullPointerException). If it's null, then mPanel must be null for the guard above it:

if (mPanel == mPanelRemote) {

to pass. To understand the problem better, when is mPanel null? Before TabsPanel.show() is called, thus before onTabChanged is called. When is this? Honestly, it's a bit difficult to follow in the code (though with do-able using Eclipse and call hierarchies). At this point, I wanted to reproduce the error to get a better idea of what the issue was. So I looked at [2], which states the error is in testAboutHomePage. Following the assertion messages, you can find that the failure occurs after "About Fennec" is selected [3], which brings the user back to the browser from Settings. Try it out and crash! :) Makes sense, given the previous analysis.

So how to fix it? I leave that up to you!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/TabsPanel.java?rev=69f55b22fd26#120
[2]: https://tbpl.mozilla.org/php/getParsedLog.php?id=42180904&tree=Try#error0
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAboutPage.java?rev=523e67cf4b9e#38

Comment 14

[Epransky]

Attachment: Bug-1016613.patch

I think I followed the logic here correctly. Basically, in the scenario where this crashed, we tried to initialize show() on mPanelRemote when it was still null.  I think this does everything needed to initialize mPanelRemote properly.

If this is the correct solution, then there's probably a more OOP way to write this, but I'd like to see if I'm on the right path first and if you think this is sufficient.

Comment 15

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

Comment on attachment 8447003 [details] [diff] [review]
Bug-1016613.patch

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

Almost there!

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +119,5 @@
>                  if (mPanel == mPanelRemote) {
>                      // Refresh the remote panel.
> +                    if (mPanelRemote == null) {
> +                        mPanelRemote = (PanelView) ((ViewStub) findViewById(R.id.remote_tabs_panel_stub)).inflate();
> +                        mPanelRemote.setTabsPanel(TabsPanel.this);

I think you should factor the mPanelRemote initialization out into a helper method so you only need to change it in one spot if it needs to change.

Comment 16

[Epransky]

Attachment: Bug-1016613.patch

OK, yeah, that's what I thought. Something like this?

Comment 17

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

Comment on attachment 8447693 [details] [diff] [review]
Bug-1016613.patch

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

Nice.

Try: https://tbpl.mozilla.org/?tree=Try&rev=68056391b84a

Comment 18

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #17)
> Comment on attachment 8447693 [details] [diff] [review]
> Bug-1016613.patch
> 
> Review of attachment 8447693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> Try: https://tbpl.mozilla.org/?tree=Try&rev=68056391b84a

Anything else I should do here?

Comment 19

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

(In reply to Epransky from comment #18)
> Anything else I should do here?

Once the tests go green (such as now), you can add the `checkin-needed` [1] keyword to get the patch checked in - note that all patches pushed using the `checkin-needed` keyword require a green Try push to land.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 20

[Epransky]

So just what I did above? Do I put the Try push link here? If so:
https://tbpl.mozilla.org/?tree=Try&rev=68056391b84a

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Epransky from comment #20)
> So just what I did above? Do I put the Try push link here? If so:
> https://tbpl.mozilla.org/?tree=Try&rev=68056391b84a

thanks! also thanks for contributing to mozilla!

Comment 23

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

Thanks, Ethan!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

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

Backed out for the issues in bug 1034384: https://hg.mozilla.org/integration/fx-team/rev/d4f97168453a

Comment 26

[Ed Morley [:emorley]]

(In reply to Michael Comella (:mcomella) from comment #25)
> Backed out for the issues in bug 1034384:
> https://hg.mozilla.org/integration/fx-team/rev/d4f97168453a

Transplanted to mozilla-central to hopefully make today's nightly ahead of the fx-team->mozilla-central merge:
remote:   https://hg.mozilla.org/mozilla-central/rev/cb75d6cfb004

Comment 27

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #17 (Bug 1034384)
> 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?

Is it necessary to refresh the state of the RemoteTabsPanel whenever Fennec is resumed? The user will not see this tab unless they select this tab by clicking on it, so couldn't we just wait until this action to make sure the state of RemoteTabsPanel is updated?

When a tab is selected in TabsPanel, we run mPanel.show() [1]. I think the method which ensures the state of RemoteTabsPanel is updated is updateCurrentPanel() [2], which is called in show() [3] in any case. 

Is there another reason which I am missing for needing a refresh in onResume()? If so, I think we can write a method which utilizes RemoteTabsPanel.updateCurrentPanel() to update the state without actually showing the PanelView itself. Let me know what you think. Thanks!

[2] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/RemoteTabsPanel.java#65
[3] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/RemoteTabsPanel.java#48
[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/TabsPanel.java#426

Comment 28

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

(In reply to Epransky from comment #27)
> The user will not see this tab unless they select this tab by
> clicking on it, so couldn't we just wait until this action to make sure the
> state of RemoteTabsPanel is updated?

1) Open the TabsPanel
2) Click RemoteTabsPanel
3) Click android home button
4) Open Firefox

The RemoteTabsPanel is shown without pressing the remote tabs panel button in the tabs panel, hence `if (mPanel == mPanelRemote)` in onResume.

Comment 29

[Epransky]

Attachment: Bug-1016613-4.patch

I didn't make the change you suggested (i.e. `RemoteTabsPanel.refresh()` method). I was just re-testing some things before making further fixes and the attached patch seems to fix all of the problems we discussed.  I think I tested everything and the behavior is as expected.  I couldn't test the explicit scenario described in Bug 1034607 because something's causing Fennec to crash when I try to navigate to any page, including http://whatsapp.com, which is its own issue.  However, navigating away from the app using the Home button and returning causes the onResume() to be called, so I think it's enough. 

Bug 1034384 - From what I tested, this bug is addressed because the panel doesn't disappear when rotating the screen. 

Bug 1034607 - is addressed, because the Sync promo doesn't become mangled. It looks like there's something different here, because I believe that the problem used to be that in onResume(), mPanel==mPanelRemote even when one would expect it to be mPanel==mPanelNormal or mPanelPrivate. Not sure what happened to change this, but this is behaving as expected now. Though, again, I couldn't test this 100% because of the larger problem of the app crashing...

Bug 1016613 - addressed with ViewStubs, etc...

I even tested the scenario you proposed, which is:

1. Navigate to RemoteTabsPanel and sign into account.
2. While RemoteTabsPanel is still displayed, navigate away from Fennec and to Settings
3. Remove Fennec account from Android phone
4. Return to app

Now, depending on how I return to the app, I notice 2 different behaviors, depending on HOW I return to the app.  If I return to the app using the Recently opened button (double rectangles), then the app returns to the RemoteTabsPanel and the sync state is updated, prompting me with the "Welcome to Sync" screen. If I return to the app by clicking on the app icon, I don't return to the TabsPanel at all.  The app opens up to the Browser view. In this second case, if I navigate back to the RemoteTabsPanel, it also shows me "Welcome to Sync".  Not positive why these two different sequences are occurring, but it seems to be ok either way.  Let me know if it isn't.

What do you think??

Comment 30

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

Comment on attachment 8456290 [details] [diff] [review]
Bug-1016613-4.patch

This patch appears to be missing the `remote_tabs_panel_view` layout and won't compile - do you think you can give it a look?

I wanted to test this myself before given a full response, so I'll wait for your next patch.

(In reply to Epransky from comment #29)
> I even tested the scenario you proposed, which is:
> 
> 1. Navigate to RemoteTabsPanel and sign into account.
> 2. While RemoteTabsPanel is still displayed, navigate away from Fennec and
> to Settings
> 3. Remove Fennec account from Android phone
> 4. Return to app

Have you tried not fully signing into an account (i.e. going between the setup and verification screens)? So:

1) Navigate to RemoteTabsPanel and create new account
2) While the verification screen is still displayed, hit home
3) Remove the fennec account
4) Return to app

Note that you can easily create sync dummy accounts by creating a sync account with the address, "<some-name>@mockmyid.com" and searching the inbox of "<some-name>" on Mailinator, e.g. "[e-mail]" will lead you to [2]. In this way, I have some *.mockmyid.com accounts that are already set up for sync, and others which are always in the verification state for testing.

[1]: http://mailinator.com/
[2]; http://mailinator.com/inbox.jsp?to=mcomella

Comment 31

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #30)
> Comment on attachment 8456290 [details] [diff] [review]
> Bug-1016613-4.patch
> 
> This patch appears to be missing the `remote_tabs_panel_view` layout and
> won't compile - do you think you can give it a look?
> 

You solved it for me with 'hg add'.  It's in the following patch I'm about to submit.

> Have you tried not fully signing into an account (i.e. going between the
> setup and verification screens)? So:
> 
> 1) Navigate to RemoteTabsPanel and create new account
> 2) While the verification screen is still displayed, hit home
> 3) Remove the fennec account
> 4) Return to app
> 

Tried this.  Basically, Fennec opens up two instances of itself on my phone from what I can tell. One shows the Create an Account screen and returns to this when I click on the Recently opened tab for it.  The other Recently opened tab for Fennec returns me to the browser with the TabsPanel collapsed.

I think it works as intended, so let me know if I'm missing something or if you expected something else from your test.  Thanks

Comment 32

[Epransky]

Attachment: Bug-1016613-4.patch

See previous comment

Comment 33

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

Comment on attachment 8459877 [details] [diff] [review]
Bug-1016613-4.patch

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

lgtm w/ nits.

nit: Update the patch description.

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

nit: indentation.

Comment 34

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

https://tbpl.mozilla.org/?tree=Try&rev=9ba1d560e748

Comment 35

[Epransky]

Attachment: Bug-1016613-4.patch

Addressing the nits

Comment 36

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #34)
> https://tbpl.mozilla.org/?tree=Try&rev=9ba1d560e748

Seems like there are issues with this try push.  I'll look at these when I get a chance...

Comment 37

[Epransky]

Attachment: Bug-1016613-4.patch

This should fix the nullpointer error we were getting in the try push on TabsPanel.java:122.

Comment 38

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

Comment on attachment 8462676 [details] [diff] [review]
Bug-1016613-4.patch

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

Looks like a good solution.

However, I noticed that every time we use mPanelRemote, we now have to make sure to call initializeRemotePanelView beforehand. Rather than doing that, since we already have the method call overhead, why don't we create a getter that does the initialization for us, if applicable? Don't forget to add a comment saying that you should probably not be accessing mPanelRemote directly.

Comment 39

[Epransky]

Attachment: Bug-1016613-4.patch

Something like this?

Comment 40

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

Comment on attachment 8463211 [details] [diff] [review]
Bug-1016613-4.patch

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

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +145,5 @@
> +                    "mCurrentPanel cannot be null in order for RemotePanelView to be initialized");
> +        }
> +
> +        if (mCurrentPanel == Panel.REMOTE_TABS) {
> +            getRemotePanelView();

I'd just add a comment here to make it clear that this is for initialization purposes.

Comment 41

[Epransky]

Attachment: Bug-1016613-4.patch

Comment 42

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

Comment on attachment 8463658 [details] [diff] [review]
Bug-1016613-4.patch

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

lgtm. Thanks for hanging in there! ^_^

Comment 43

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #42)
> Comment on attachment 8463658 [details] [diff] [review]
> Bug-1016613-4.patch
> 
> Review of attachment 8463658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm. Thanks for hanging in there! ^_^

Cool, do we need a new try push?

Comment 44

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

https://tbpl.mozilla.org/?tree=Try&rev=160494b54226

Comment 45

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

(In reply to Epransky from comment #43)
> Cool, do we need a new try push?

Yeah, see comment 44. By the way, I'd recommend using needinfo flags to ensure your questions are seen and responded to quickly.

Comment 46

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

Push in comment 44 is green.

Comment 47

[Epransky]

(In reply to Michael Comella (:mcomella) from comment #46)
> Push in comment 44 is green.
Great, but I see one red push for "Android 2.3 Opt", but I'm not sure why I'm getting this.

Comment 48

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

(In reply to Epransky from comment #47)
> Great, but I see one red push for "Android 2.3 Opt", but I'm not sure why
> I'm getting this.

We sometimes have intermittent failures, where a test doesn't always pass. This is typically due to race conditions in the test. A good indication that a failure is intermittent is to look at different device configurations (e.g. Android versions, opt vs. dbg) and see if the test suite passed. However, this is not a perfect method because sometimes a test will fail only on a certain device configuration. You can also click on a test suite failure, where it will display related bugs, to see if it's a common failure.

Seeing the test suite pass on another device configuration, I decided to re-run the test suite, which is why two "rc3" test suites appear under Android 2.3 Opt. Note that one is orange (has at least one failure), while the other is green (all tests passed). Since all tests passed, this patch should be good to go.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing.

Comment 50

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

Attachment: Rebased patch.

Comment 51

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

Took care of the rebasing.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Ed Morley [:emorley]]

Reverted for robocop failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45942310&tree=Fx-Team#error2
https://tbpl.mozilla.org/php/getParsedLog.php?id=45942211&tree=Fx-Team#error2

Broken rebase?

remote:   https://hg.mozilla.org/integration/fx-team/rev/9d6f19dbe9d3

Comment 54

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

https://tbpl.mozilla.org/?tree=Try&rev=84535f9b1c8c

Comment 55

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

Attachment: Rebased patch v2

My bad - I'll avoid doing this without a try run in the future. The try run is
in comment 54.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

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

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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

Comment 58

[Mark Finkle (:mfinkle) (use needinfo?)]

This needs to be backed out again. It's still causing the remote tabs panel corruption it caused previously (bug 1034607) once again (bug 1054623).

Comment 59

[Ed Morley [:emorley]]

Reverted for the issue in comment 58. This is unfortunately now the second time nightly channel users have ended up with a broken build due to the same issue (it's not possible to switch tabs, since the overlay blocks clicks). Please can this be tested more thoroughly before requesting relanding, and a test added to detect this in automation in the future.

remote:   https://hg.mozilla.org/mozilla-central/rev/0aaa2d3d15cc

Comment 60

[Nick Alexander :nalexander [he/him]]

How much do we care about this?  I am not intimately familiar, but I think it is a relatively small win -- not quite "code hygiene", but maybe "lifecycle hygiene".  I'd rather see effort put into making Remote Tabs a home pager panel than get a small win that appears to be fragile into the tree.  mcomella, can you comment?

Comment 61

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

I think you're right - this is an unnecessary micro-optimization, and even less necessary given the eventually change into a HomePager panel (which inflates the page for us when the page is opened) - bug 1014994, for reference.

Sorry, Ethan, but I'm going to WONTFIX this - thanks for your hard work! If you're interested in grabbing a new bug, nalexander had a few suggestions:
  * bug 1040572
  * bug 1053016/bug 1053278 (if you would like to try touching our C++ code base)