918044 - TopSitesGridView calls layout infinite times

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

During startup, the TopSitesGridView does a recursive layout call until the screen is touched.

Comment 1

[Sriram Ramasubramanian [:sriram]]

This is happening even if the "fixed height" hack on the measure of TopSitesGridView is not used. So our onMeasure() isn't causing the problem.

I see an unfocus() event on the GridView triggering a layout pass. And the measure all have the same widthMeasureSpec (unlike two different ones between the two different measure passes). I've no more clues on what's happening at the startup that could cause this.

Comment 2

[Lucas Rocha (:lucasr)]

I'll have a look at this.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Smoothie is the panacea! This goes away with smoothie library!

Comment 4

[Sriram Ramasubramanian [:sriram]]

I/Sriram  (27332): focus: true
I/Sriram  (27332): request layout
I/Sriram  (27332): request layout
I/Sriram  (27332): focus: false
I/Sriram  (27332): request layout
I/Sriram  (27332): request layout
I/Sriram  (27332): focus: true

Oh well. What's happening with the focus?

Comment 5

[Sriram Ramasubramanian [:sriram]]

I/Sriram  (28235): list focus: true
I/Sriram  (28235): request layout
I/Sriram  (28235): list focus: false
I/Sriram  (28235): focus: true
I/Sriram  (28235): focus: false
I/Sriram  (28235): request layout
I/Sriram  (28235): list focus: true
I/Sriram  (28235): request layout
I/Sriram  (28235): list focus: false
I/Sriram  (28235): focus: true
I/Sriram  (28235): request layout
I/Sriram  (28235): request layout
I/Sriram  (28235): focus: false
I/Sriram  (28235): request layout
I/Sriram  (28235): list focus: true

Oh well. The ListView is also having a problem!

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

That's the only permutation that fixes this issue! :(

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 817455 [details] [diff] [review]
Patch

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

We don't want to make the GridView unfocusable as this will break keyboard navigation. What we want actually need here is to avoid re-layouts while handling focus in the GridView. I tried a few options and I believe I found a good enough solution. Patch coming in a minute.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Block layout requests while handling focus changes in TopSitesGridView (r=sriram)

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 817756 [details] [diff] [review]
Block layout requests while handling focus changes in TopSitesGridView (r=sriram)

Given that we know that our GridView will never scroll or change selection on focus changes (which would usually be the case for an ordinary GridView), we can safely ignore layout requests while handling focus changes in it. The GridView remains focusable with this patch.

Seems to fix the bug in my local builds.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 817756 [details] [diff] [review]
Block layout requests while handling focus changes in TopSitesGridView (r=sriram)

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

This is a good idea.

Comment 11

[Lucas Rocha (:lucasr)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bc8a298c227

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 817756 [details] [diff] [review]
Block layout requests while handling focus changes in TopSitesGridView (r=sriram)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: Infinite layout loop on about:home after fennec starts. No apparent performance problem but it's definitely worth uplifting this, just in case. 
Testing completed (on m-c, etc.): Landed on m-c, no issues found.
Risk to taking this patch (and alternatives if risky): Very low, just avoids layout requests while handling focus changes in the about:home top sites grid.
String or IDL/UUID changes made by this patch: n/a

Comment 14

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80572c953f4a