Closed Bug 1003870 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S2 (23may)

People

(Reporter: jerry, Assigned: alive)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
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)?
Flags: needinfo?(dbaron)
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.
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.
Flags: needinfo?(dbaron)
(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.
Assignee: nobody → hshih
Status: NEW → ASSIGNED
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.
Component: Layout → Gaia::System::Window Mgmt
Product: Core → Firefox OS
Target Milestone: --- → 2.0 S2 (23may)
Steal
Assignee: hshih → alive
Attached file WIP
Let me know if this triggers reflow
Attachment #8419202 - Flags: feedback?(hshih)
Use google chrome to open this file.

We can see the reflow tasks are reduced, but the keyboard app still costs time.
Comment on attachment 8419202 [details] [review]
WIP

See attachment 8419247 [details].
This patch is helpful, but keyboard app still does reflow.
Attachment #8419202 - Flags: feedback?(hshih) → feedback+
See Also: → 1006407
(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.
Attachment #8419202 - Flags: review?(timdream) → review+
https://github.com/alivedise/gaia/commit/33bab4b3e8af2559fa5f68c19fbaf13e40c64928
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Backed out because this caused bug 1009409

https://github.com/mozilla-b2g/gaia/commit/7800803e41c3e9a598955b5ef8b8b87fc8f07cd5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8421862 - Flags: review?(timdream)
Attachment #8421862 - Flags: review?(timdream) → review+
https://github.com/mozilla-b2g/gaia/commit/da4b905173fa69d2ec925810270aa8e0532e5961 again
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 1011071
Depends on: 1014595
You need to log in before you can comment on or make changes to this bug.