Closed
Bug 1003870
Opened 11 years ago
Closed 11 years ago
Do not do reflow for non-visible app on phone rotation
Categories
(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S2 (23may)
People
(Reporter: jerry, Assigned: alive)
References
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.
Reporter | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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•11 years ago
|
||
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)
Reporter | ||
Comment 4•11 years ago
|
||
(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
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Blocks: window-management
Assignee | ||
Comment 7•11 years ago
|
||
Let me know if this triggers reflow
Attachment #8419202 -
Flags: feedback?(hshih)
Reporter | ||
Comment 8•11 years ago
|
||
Use google chrome to open this file. We can see the reflow tasks are reduced, but the keyboard app still costs time.
Reporter | ||
Comment 9•11 years ago
|
||
Comment on attachment 8419202 [details] WIP See attachment 8419247 [details]. This patch is helpful, but keyboard app still does reflow.
Attachment #8419202 -
Flags: feedback?(hshih) → feedback+
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Jerry Shih[:jerry] from comment #9) > Comment on attachment 8419202 [details] > 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.
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8419202 [details] WIP See https://bugzilla.mozilla.org/show_bug.cgi?id=1003870#c5
Attachment #8419202 -
Flags: review?(timdream)
Updated•11 years ago
|
Attachment #8419202 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 12•11 years ago
|
||
https://github.com/alivedise/gaia/commit/33bab4b3e8af2559fa5f68c19fbaf13e40c64928
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 13•11 years ago
|
||
Backed out because this caused bug 1009409 https://github.com/mozilla-b2g/gaia/commit/7800803e41c3e9a598955b5ef8b8b87fc8f07cd5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8421862 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 15•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/da4b905173fa69d2ec925810270aa8e0532e5961 again
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•