Closed Bug 1180690 Opened 9 years ago Closed 6 years ago

Bug 1170944 made the opening/closing animation regress a lot

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: etienne, Unassigned)

References

Details

Attachments

(1 file)

We have short transition times now. But you can toggle the slowTransition flag in AppWindowManager to get an idea.

When opening an app
- we loose the homescreen content right away, but still do expensive transition on what's basically and transparent div

when closing an app
- we loose the app content right away and transition a color div (the app theme-color) instead

Since this change is also causing performance issues ([1]) I'd like to see if there might be a better compromise.


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1170944#c15
Most of the people who worked on bug 1170944 are now inactive :/

Tim, can you help me figure out what was the goal with this change, is it still valid? (Not sure while reading through bug 1154231).

Alternatively, if we absolutely need to setVisible(false) before the transition, we should come up with a new transition that fits this model, not keep the broken one.
Flags: needinfo?(timdream)
It also impacts the lockscreen transition.
(In reply to Etienne Segonzac (:etienne) from comment #1)
> Most of the people who worked on bug 1170944 are now inactive :/
> 
> Tim, can you help me figure out what was the goal with this change, is it
> still valid? (Not sure while reading through bug 1154231).
> 
> Alternatively, if we absolutely need to setVisible(false) before the
> transition, we should come up with a new transition that fits this model,
> not keep the broken one.

After bug 1154231, setVisible(false) should not change the content. But looks like it regressed; we loose the the content when animating.
(In reply to Kan-Ru Chen [:kanru] from comment #4)
> (In reply to Etienne Segonzac (:etienne) from comment #1)
> > Most of the people who worked on bug 1170944 are now inactive :/
> > 
> > Tim, can you help me figure out what was the goal with this change, is it
> > still valid? (Not sure while reading through bug 1154231).
> > 
> > Alternatively, if we absolutely need to setVisible(false) before the
> > transition, we should come up with a new transition that fits this model,
> > not keep the broken one.
> 
> After bug 1154231, setVisible(false) should not change the content. But
> looks like it regressed; we loose the the content when animating.

Firstly, I'm very happy that bug 1154231 has landed. It is a great step forward.

If my understanding is correct, the reason why it does not work today is because the cached resources are still discarded.
And this happens because iframe.setVisible(false) triggers a change in priorities, which results into a memory pressure call (see http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#1097).

This memory pressure calls then force the compositor cached resources to be cleared.

This memory-pressure observer has some other side effects fwiw. In bug 1002430 I found that it forces the decoded data of images to be throw away, which sometimes results into a broken rendering.

What about a special reason for this particular (forced) memory pressure, so we can make it not flush things that affects the rendering ?

On a different note the patch on bug 1170944 does not solve all the visual glitches that can be removed once bug 1154231 will be fixed. For example, as of today the System app force the iframe rendering to be hidden when the app window moves to the background.
A good way to fix that could be to:
 - Apply the css style to make the iframe rendering hidden only when the layer tree has been removed (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/window.css#L425) instead of instantly when AppWindow.setVisible(false) is called.
 - In order to do that we need to fix the mozbrowser API to expose the information about a discarded layer tree. This information is at the moment only exposed to chrome code in http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#2934
 Would be nice to have an abstract API on top of the mozbrowser API as there are a few different use cases in the system app.


As this is christmas and I'm making a wishlist, it would be useful if we can fix the mozbrowser API to use the MozAfterRemotePaint of the same TabParent.cpp file for the addNextPaintListener. It will avoid a some ipc calls and will make the event more reliable since the event use in the child process does not really reflect when something is really painted on the screen. (because of this we are doing really dirty things today in the system app to make sure the content is painted). This will also be useful for bug 1173286 :)
Attached patch PoCSplinter Review
Actually I first checked the memory-pressure event but it's not the main cause. It's because the system app applied |visibility: hidden| to the still visible frame.

Apply this patch and the issue is gone.
(In reply to Kan-Ru Chen [:kanru] from comment #6)
> Created attachment 8630855 [details] [diff] [review]
> PoC
> 
> Actually I first checked the memory-pressure event but it's not the main
> cause. It's because the system app applied |visibility: hidden| to the still
> visible frame.
> 
> Apply this patch and the issue is gone.

I'll fix the gaia part as part of bug 1179040, leaving this one open for the gecko part.
There's still a lot of value in having the layer tree kept alive as long as we don't need to reclaim the memory. (as opposed to just keeping it alive for a few seconds like it is right now)
(In reply to Etienne Segonzac (:etienne) from comment #1)
> Tim, can you help me figure out what was the goal with this change, is it
> still valid? (Not sure while reading through bug 1154231).

The goal of the change is to tell the application to start cleaning up their data *before* transition starts, by moving the timing of visibilitychange event there. We were previously prevented to do so because setVisible(false) call was coupled with layer recycling.

Previously we have many, many workaround in place to ask apps to clean up ASAP (like of like a visibilitywillchange event), and I want to remove these workaround too to make FxOS more mature and behavior more closer to the normal web platform.
Flags: needinfo?(timdream)
So the remaining gecko work here is to not collect layer data when the memory-pressure is a synthesized one. We could change the "reason" parameter to "visibility-hidden" instead of "low-memory" as currently used in PriorityManager.

I'll open a new bug for this. I think we could close this bug as worksforme.
(In reply to Kan-Ru Chen [:kanru] from comment #6)
> Created attachment 8630855 [details] [diff] [review]
> PoC
> 
> Actually I first checked the memory-pressure event but it's not the main
> cause. It's because the system app applied |visibility: hidden| to the still
> visible frame.
> 
> Apply this patch and the issue is gone.

It's not.


The way we use to check is:
 1. Connect WebIDE to the system app
 2. Open an application like the Settings app
 3. In the WebIDE console type:
  appWindowManager.getActiveApp().iframe.setVisible(false);

 4. Wait a few seconds or try to interact with the app content.

Expected result:
 5. Nothing special

Actual result:
 6. The rendering goes black.
Depends on: 1181518
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #8)
> (In reply to Etienne Segonzac (:etienne) from comment #1)
> > Tim, can you help me figure out what was the goal with this change, is it
> > still valid? (Not sure while reading through bug 1154231).
> 
> The goal of the change is to tell the application to start cleaning up their
> data *before* transition starts, by moving the timing of visibilitychange
> event there. We were previously prevented to do so because setVisible(false)
> call was coupled with layer recycling.
> 
> Previously we have many, many workaround in place to ask apps to clean up
> ASAP (like of like a visibilitywillchange event), and I want to remove these
> workaround too to make FxOS more mature and behavior more closer to the
> normal web platform.

Thanks for the info!
Looks like between bug 1179040 and bug 1181518 we'll be able to continue triggering the visibilitychange earlier.
Depends on: 1179040
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: