1003870 - Do not do reflow for non-visible app on phone rotation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: all app do reflow for rotation

In attachment, we see all app do reflow when we rotate the phone.
I think we only need to handle the foreground app(or visible app) reflow task. And we should not spend the cpu power to do the unnecessary reflow task for other app.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: check_visibility_in_PresShell.patch

I try to check the "document.hidden" attribute in PressShell::IsVisible() to skip the app reflow task, but it's not work.
Maybe I need to check this condition at other place.

David, what do you think about the "document.hidden" attribute? Is this condition enough to filter the hidden app(in b2g) and non-focus tab(in desktop browser)?

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8415325 [details] [diff] [review]
check_visibility_in_PresShell.patch

TabChild::RecvUpdateDimensions seems to be doing a lot of work that we would probably want to skip and only do when that app becomes foreground again.

Comment 3

[David Baron :dbaron:]

First -- not sure why you'd want to structure the patch this way, which seems to break all the other checks in IsVisible -- rather than making it an additional condition under which we'd consider a pres shell not visible.

Second, the code to suppress resize reflows for hidden documents **does not use PresShell::IsVisible**; that's what I was suggesting yesterday be fixed in bug 465216.  So I wouldn't expect a change to PresShell::IsVisible to affect it.

Third, I'm not particularly confident that this state is correctly maintained across processes; it comes from calls to nsDocShell::SetIsActive, but the cross-process call made as a result (in PresShell::SetIsActive) is TabChild::MakeVisible/MakeHidden, which doesn't sound like it quite corresponds.  Though maybe it's just bad naming; I didn't trace it through.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #3)
> First -- not sure why you'd want to structure the patch this way, which
> seems to break all the other checks in IsVisible -- rather than making it an
> additional condition under which we'd consider a pres shell not visible.
Yes, maybe I should check the nsIDocument::Hidden() or nsIDocShell::GetIsActive() to another place.

> Second, the code to suppress resize reflows for hidden documents **does not
> use PresShell::IsVisible**; that's what I was suggesting yesterday be fixed
> in bug 465216.  So I wouldn't expect a change to PresShell::IsVisible to
> affect it.
Sorry, I can't catch your idea. Even though I skip the backgroud app to do nsViewManager::SetWindowDimensions(), I still can see the css computing in FlushPendingNotification().
I still try to find a right place to put the visibility(or active) condition to prevent the unnecessary reflow.

> Third, I'm not particularly confident that this state is correctly
> maintained across processes; it comes from calls to nsDocShell::SetIsActive,
> but the cross-process call made as a result (in PresShell::SetIsActive) is
> TabChild::MakeVisible/MakeHidden, which doesn't sound like it quite
> corresponds.  Though maybe it's just bad naming; I didn't trace it through.
I had checked the nsIDocument::Hidden() status. I saw all backgrond app hidden in my test. I think that it works well.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

This should be a very old bug from long time ago in system...

The root cause might be: the app iframe's width/height is 100%-something/100%-something with system app. So if we don't resize in app starting/opening, it will change the width/height according to the css rules.

The reason we never resize an app on its opening process is because we don't want to reflow causing the launch slower. But I don't know if we resize to the same width/height will cause reflow or not.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Steal

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: WIP

Let me know if this triggers reflow

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: profile result after applying attachment 8419202

Use google chrome to open this file.

We can see the reflow tasks are reduced, but the keyboard app still costs time.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8419202 [details] [review]
WIP

See attachment 8419247 [details].
This patch is helpful, but keyboard app still does reflow.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jerry Shih[:jerry] from comment #9)
> Comment on attachment 8419202 [details] [review]
> WIP
> 
> See attachment 8419247 [details].
> This patch is helpful, but keyboard app still does reflow.

I prefer to have another bug for keyboard since it's not under current window management.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8419202 [details] [review]
WIP

See https://bugzilla.mozilla.org/show_bug.cgi?id=1003870#c5

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/alivedise/gaia/commit/33bab4b3e8af2559fa5f68c19fbaf13e40c64928

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

Backed out because this caused bug 1009409

https://github.com/mozilla-b2g/gaia/commit/7800803e41c3e9a598955b5ef8b8b87fc8f07cd5

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/19209/files

The "correct" fix is to change logic of statusbar state to know the active app and maintain its height but it involves lots of change. I am going to file another bug for statusbar refactoring.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/commit/da4b905173fa69d2ec925810270aa8e0532e5961 again