1203122 - Current tab is not selected in tab tray on gingerbread devices

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Environment: 
Device: Samsung Galaxy R (Android 2.3.4);
Build: Nightly 43.0a1 (2015-09-09);

Steps to reproduce:
1. Open a few tabs;
2. Open tab tray.

Expected result:
The current tab is selected in the tab tray.

Actual result:
The current tab is not selected in the tab tray.

Comment 1

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

Martyn?

Comment 2

[Martyn Haigh (:mhaigh)]

I can reproduce.  I have an idea what it may be.  This should only affect the gridview, which will soon be behind a nightly flag.

Comment 3

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e794214e98c3

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: MozReview Request: Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r?mcomella

Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r?mcomella

Comment 5

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

Comment on attachment 8659301 [details]
MozReview Request: Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r?mcomella

https://reviewboard.mozilla.org/r/18789/#review16921

If it works, wfm.

If you have any doubts, check that your recent Nightly flag changes don't effect the new-tabs-tray disabled state too.

::: mobile/android/base/tabs/TabsGridLayout.java:266
(Diff revision 1)
> -            if (AppConstants.Versions.feature11Plus) {
> +        if (AppConstants.Versions.feature11Plus) {

Can we just run the bottom algorithm on all views instead of having the branch here? I'd like to reduce the number of code paths if possible.

::: mobile/android/base/tabs/TabsGridLayout.java:267
(Diff revision 1)
> +            for (int i = 0; i < tabsAdapter.getCount(); i++) {

I think you want `getChildCount` instead of `tabsAdapter.getCount` – it's safer because otherwise you're assuming we have a view for every type, which we might not when we're recycling views.

::: mobile/android/base/tabs/TabsGridLayout.java:273
(Diff revision 1)
> +            for (int i = getFirstVisiblePosition(); i < tabsAdapter.getCount(); i++) {

Similarly, `tabsAdapter.getCount()` -> `getLastVisiblePosition`?

::: mobile/android/base/tabs/TabsGridLayout.java:280
(Diff revision 1)
> +                            ((TabsLayoutItemView) getViewForTab(tab)).setChecked(checked);

Store `getViewForTab` in a temporary variable to avoid calling it twice.

Comment 6

[Martyn Haigh (:mhaigh)]

> 
> ::: mobile/android/base/tabs/TabsGridLayout.java:266
> (Diff revision 1)
> > -            if (AppConstants.Versions.feature11Plus) {
> > +        if (AppConstants.Versions.feature11Plus) {
> 
> Can we just run the bottom algorithm on all views instead of having the
> branch here? I'd like to reduce the number of code paths if possible.
> 

Unfortunately just having setChecked doesn't work in my Lollipop test device, so looks like we'll have to keep both code paths in.

Comment 7

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/a7090080e87144f1f0cf92494c130f2f846eafdc
Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r=mcomella

Comment 8

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/812b25892cb6951f20aa754e47f089144f95e79b
Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r=mcomella

Comment 9

[Martyn Haigh (:mhaigh)]

Actually I was wrong - I've stripped it down to one loop with a conditional statement within for 11plus devices

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/812b25892cb6

Comment 11

[Flaviu Cos]

Verified as fixed in build 44.0a1 2015-09-25;
Device: Samsung Galaxy R (Android 2.3.4).