Closed Bug 1016613 Opened 6 years ago Closed 5 years ago

ViewStub RemoteTabsPanel

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mcomella, Assigned: Epransky, Mentored)

References

Details

(Whiteboard: [lang=java][needs test before relanding])

Attachments

(2 files, 12 obsolete files)

8.63 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
8.68 KB, patch
Details | Diff | Splinter Review
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.
via email.
Assignee: nobody → Epransky
Status: NEW → ASSIGNED
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java] → [lang=java]
Attached patch Bug-1016613.patch (obsolete) — Splinter Review
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!
Mentor: michael.l.comella
Attachment #8441757 - Flags: review?(michael.l.comella)
Mentor: michael.l.comella
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?
Attachment #8441757 - Flags: review?(michael.l.comella) → review-
Attached patch Bug-1016613.patch (obsolete) — Splinter Review
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...
Attachment #8441757 - Attachment is obsolete: true
Attachment #8443297 - Flags: review?(michael.l.comella)
(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 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.
Attachment #8443297 - Flags: review?(michael.l.comella) → feedback+
Attached patch Bug-1016613.patch (obsolete) — Splinter Review
Cool.  Took away the background attribute.
Attachment #8443297 - Attachment is obsolete: true
Attachment #8443656 - Flags: review?(michael.l.comella)
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`?
Attachment #8443656 - Flags: review?(michael.l.comella) → review-
Attached patch Bug-1016613.patch (obsolete) — Splinter Review
Oops, you're right.  Here's the correct one
Attachment #8443656 - Attachment is obsolete: true
Attachment #8443660 - Flags: review?(michael.l.comella)
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
Attachment #8443660 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8443660 - Flags: review+ → review-
> via the try run in comment 10, looks like we're going to have another
> iteration.

Not sure what's going on here :/
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
Attached patch Bug-1016613.patch (obsolete) — Splinter Review
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.
Attachment #8443660 - Attachment is obsolete: true
Attachment #8447003 - Flags: review?(michael.l.comella)
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.
Attachment #8447003 - Flags: review?(michael.l.comella) → feedback+
Attached patch Bug-1016613.patch (obsolete) — Splinter Review
OK, yeah, that's what I thought. Something like this?
Attachment #8447003 - Attachment is obsolete: true
Attachment #8447693 - Flags: review?(michael.l.comella)
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
Attachment #8447693 - Flags: review?(michael.l.comella) → review+
(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?
(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
Keywords: checkin-needed
So just what I did above? Do I put the Try push link here? If so:
https://tbpl.mozilla.org/?tree=Try&rev=68056391b84a
https://hg.mozilla.org/integration/fx-team/rev/e01dbdf8a218
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
(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!
https://hg.mozilla.org/mozilla-central/rev/e01dbdf8a218
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 33
Depends on: 1034609
Depends on: 1034607
Backed out for the issues in bug 1034384: https://hg.mozilla.org/integration/fx-team/rev/d4f97168453a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
(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
(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.
Attached patch Bug-1016613-4.patch (obsolete) — Splinter Review
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??
Attachment #8447693 - Attachment is obsolete: true
Attachment #8456290 - Flags: review?(michael.l.comella)
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. "mcomella@mockmyid.com" 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
Attachment #8456290 - Flags: review?(michael.l.comella) → review-
(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
Attached patch Bug-1016613-4.patch (obsolete) — Splinter Review
See previous comment
Attachment #8456290 - Attachment is obsolete: true
Attachment #8459877 - Flags: review?(michael.l.comella)
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.
Attachment #8459877 - Flags: review?(michael.l.comella) → review+
Attachment #8459877 - Flags: review+ → feedback+
Attached patch Bug-1016613-4.patch (obsolete) — Splinter Review
Addressing the nits
Attachment #8459877 - Attachment is obsolete: true
Attachment #8462356 - Flags: review?(michael.l.comella)
(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...
Attached patch Bug-1016613-4.patch (obsolete) — Splinter Review
This should fix the nullpointer error we were getting in the try push on TabsPanel.java:122.
Attachment #8462356 - Attachment is obsolete: true
Attachment #8462356 - Flags: review?(michael.l.comella)
Attachment #8462676 - Flags: review?(michael.l.comella)
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.
Attachment #8462676 - Flags: review?(michael.l.comella) → feedback+
Attached patch Bug-1016613-4.patch (obsolete) — Splinter Review
Something like this?
Attachment #8462676 - Attachment is obsolete: true
Attachment #8463211 - Flags: review?(michael.l.comella)
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.
Attachment #8463211 - Flags: review?(michael.l.comella) → feedback+
Attachment #8463211 - Attachment is obsolete: true
Attachment #8463658 - Flags: review?(michael.l.comella)
Comment on attachment 8463658 [details] [diff] [review]
Bug-1016613-4.patch

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

lgtm. Thanks for hanging in there! ^_^
Attachment #8463658 - Flags: review?(michael.l.comella) → review+
(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?
(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.
Push in comment 44 is green.
Flags: needinfo?(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.
Flags: needinfo?(Epransky)
Flags: needinfo?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella)
Keywords: checkin-needed
This needs rebasing.
Keywords: checkin-needed
Assignee: Epransky → michael.l.comella
Status: REOPENED → ASSIGNED
Took care of the rebasing.
Assignee: michael.l.comella → Epransky
Keywords: checkin-needed
Attachment #8463658 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/70cda3c55557
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
Attached patch Rebased patch v2Splinter Review
My bad - I'll avoid doing this without a try run in the future. The try run is
in comment 54.
Attachment #8472485 - Attachment is obsolete: true
Attachment #8473255 - Attachment description: Replaced RemoteTabsPanel view with ViewStub in tabs_panel.xml and created new layout file to inflate the ViewStub when user clicks on 3rd button in TabsPanel. This was done in order to save on resources and not create the RemoteTabsPanel until needed → Rebased patch v2
Attachment #8463658 - Attachment is obsolete: false
Keywords: checkin-needed
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
https://hg.mozilla.org/integration/fx-team/rev/efe3c7a0091c
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/efe3c7a0091c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
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).
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [lang=java] → [lang=java][needs test before relanding]
Target Milestone: Firefox 33 → ---
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?
Flags: needinfo?(michael.l.comella)
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)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(michael.l.comella)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.