Do not use onFinishInflate in RemoteTabs*Panel

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 33
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 fixed, fennec33+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

As per bug 1002303 comment 27, lucasr mentioned that onFinishInflate can be unreliable, so replace it! He gave an alternate solution in that comment.

We might want to uplift this to the same places as bug 1014999 and 958889 because it could cause problems on the larger range of devices in the field.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Couldn't quite do it the same way as in bug 1002303 because the views are added dynamically.
Attachment #8439634 - Flags: review?(lucasr.at.mozilla)
Attachment #8439631 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8439631 [details] [diff] [review]
Part 1: Use the correct containing layout in the RemoteTabsVerificationPanel.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1007442 (this should be uplifted first)

User impact if declined:
  Users will crash if they are in the remote tabs panel's emailverification screen and click on the tab tray (as in users cannot navigate to different tabs)
 
Testing completed (on m-c, etc.):
  Local

Risk to taking this patch (and alternatives if risky):
  Low, well-understood one liner

String or IDL/UUID changes made by this patch: None
Attachment #8439631 - Flags: approval-mozilla-aurora?
Depends on: 1025938
Duplicate of this bug: 1025938
Comment on attachment 8439634 [details] [diff] [review]
Part 2: Use initialize instead of onFinishInflate in RemoteTabs panels.

(In reply to Michael Comella (:mcomella) from comment #2)
> Created attachment 8439634 [details] [diff] [review]
> Part 2: Use initialize instead of onFinishInflate in RemoteTabs panels.
> 
> Couldn't quite do it the same way as in bug 1002303 because the views are
> added dynamically.

Could you give some more context around the 'views are added dynamically' here? Not sure I follow.
(In reply to Lucas Rocha (:lucasr) from comment #7)
> Could you give some more context around the 'views are added dynamically'
> here? Not sure I follow.

In bug 1002303 comment 27, you suggested a method to avoid using the `onFinishInflate` method. However, both `RemoteTabsSetupPanel` and `RemoteTabsVerificationPanel` are inflated dynamically in `RemoteTabsPanel`, rather than being declared in xml, so I can't follow this method exactly.

I think the crux of that method is to inflate the class' layout (which has a `<merge>` tag) in the constructor of the class. However, since the class views (e.g. RemoteTabsSetupPanel) are not declared in XML, they would have to add the layout_width, layout_height, and visibilitiy attributes dynamically, which is not desireable. As such, I opted to add an `initialize` method to do this initialization.
Comment on attachment 8439631 [details] [diff] [review]
Part 1: Use the correct containing layout in the RemoteTabsVerificationPanel.

Aurora approval granted.
Attachment #8439631 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8439634 [details] [diff] [review]
Part 2: Use initialize instead of onFinishInflate in RemoteTabs panels.

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

::: mobile/android/base/tabspanel/RemoteTabsPanel.java
@@ +122,2 @@
>              case CONTAINER:
>                  inflatedView = inflater.inflate(R.layout.remote_tabs_container_panel, null);

You could simply replace these inflations with normal instantiation, like new RemoteTabsContainerPanel(context) and inflate their content in their constructors.

This is actually another reason why I tend to avoid onFinishInflate() in custom views, your views end up assuming that they will always be instantiated from inflation i.e. they break if you just use their constructors directly.
Attachment #8439634 - Flags: review?(lucasr.at.mozilla) → review-
tracking-fennec: --- → 33+
This patch shares nothing in common with the original part 2.

I wasn't sure if I should change the RemoteTabsContainerPanel to be consistent
with the other views - I opted not to (to avoid churn).
Attachment #8443073 - Flags: review?(lucasr.at.mozilla)
Attachment #8439634 - Attachment is obsolete: true
The bug 1025938 is the top crash on Aurora right now, does it only need uplift of the already-approved patch in here or does it require the additional ongoing work as well?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #12)
> The bug 1025938 is the top crash on Aurora right now, does it only need
> uplift of the already-approved patch in here or does it require the
> additional ongoing work as well?

I believe it only requires uplift of the already-approved patch.
Whiteboard: [leave-open] → [leave-open][checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/c429a00cf5b6

I'm looking into why this didn't show up in any of my uplift queries. AFAICT, it should have. Also, calling this "fixed" for firefox32 for tracking purposes. If Part 2 is eventually destined for Aurora uplift as well, please set the status back to affected.
Whiteboard: [leave-open][checkin-needed-aurora]
Comment on attachment 8443073 [details] [diff] [review]
Inflate RemoteTabs*Panel in constructor.

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

Looks nice, thanks.

(In reply to Michael Comella (:mcomella) from comment #11)
> I wasn't sure if I should change the RemoteTabsContainerPanel to be
> consistent
> with the other views - I opted not to (to avoid churn).

Just wondering: how's RemoteTabsContainerPanel different than the other views?

::: mobile/android/base/resources/layout/remote_tabs_setup_panel.xml
@@ +5,5 @@
>  
> +<merge xmlns:android="http://schemas.android.com/apk/res/android"
> +       android:layout_width="match_parent"
> +       android:layout_height="match_parent"
> +       android:visibility="gone">

Not sure how the attributes in the merge tag are transferred to the root view during inflation. Please double-check those are behaving as expected.
Attachment #8443073 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #15)
> Just wondering: how's RemoteTabsContainerPanel different than the other
> views?

RemoteTabsContainerPanel does not do post-inflation initialization, like the
other panels, thus never used `onFinishInflate` and does not need to change for
this bug to be fixed.

> Not sure how the attributes in the merge tag are transferred to the root
> view during inflation. Please double-check those are behaving as expected.

They don't get transferred and they're redefined on the parent element.
Including them on the merge tag is a mistake and I removed these attributes
from this revision of the patch.
Attachment #8443073 - Attachment is obsolete: true
Lucas, because of the inconsistency of onFinishInflate, do you feel this should be uplifted?

Also, if you want to make RemoteTabsContainerPanel consistent, we can do so in a separate bug (that does not need to be uplifted).

https://hg.mozilla.org/integration/fx-team/rev/9fc70845e27c
Flags: needinfo?(lucasr.at.mozilla)
https://hg.mozilla.org/mozilla-central/rev/9fc70845e27c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
(In reply to Michael Comella (:mcomella) from comment #17)
> Lucas, because of the inconsistency of onFinishInflate, do you feel this
> should be uplifted?

Let's only uplift these patches if we start seeing crashers around bogus inflation in these views or something.
Flags: needinfo?(lucasr.at.mozilla)
You need to log in before you can comment on or make changes to this bug.