Wait for the app to be fully repainted before starting the warm launch transition

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:fugu+, b2g-v1.2 affected)

Details

(Whiteboard: [Fugu] [v1.2f-uplift-needed])

Attachments

(2 attachments)

Created attachment 816686 [details] [diff] [review]
warm.launch.wait.for.full.repaint.patch

Right now the layer tree is rebuilding in the user face when the application is warm and it is reopened.

the patch ensure that the layer tree has been reconstructed/repainted before starting the transition.
Attachment #816686 - Flags: review?(alive)
Comment on attachment 816686 [details] [diff] [review]
warm.launch.wait.for.full.repaint.patch

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

The original appWindow._waitForNext has a timer to ensure the callback finally occurs.
I don't know what's the latency of |getScreenshot| but if it's fast enough I'm O.K.

BTW, |waitForFullRepaint| looks to me should be named after |ensureFullRepaint| or whatever. But I don't mind if you don't change the name.
Attachment #816686 - Flags: review?(alive) → review+
https://github.com/mozilla-b2g/gaia/commit/e8356415358cc1c9bce496427ff14c88d9d77ec3

I landed it with the name change. About the timeout I think one of the main reasons for having it on waitForNextPaint is because the app could be in  state where no repainting are occuring, which is different since we are kind of forcing a repaint with getScreenshot. But if we noticed that it cause issues I won't mind to add a timer :)

Thanks for the quick review.
This seems fix bug 922546(but I don't know why yet)
blocking-b2g: --- → koi?
Hi Alive,

First of all, what is the user impact of not taking this bug fix?

Also, can we be certain that this bug does fix 922546.
Flags: needinfo?(alive)
I could only tell user impact is the unstable feeling about UI flicking for bug 922546

(In reply to Preeti Raghunath(:Preeti) from comment #4)
> Hi Alive,
> 
> First of all, what is the user impact of not taking this bug fix?
> 
> Also, can we be certain that this bug does fix 922546.

As Sotaro said
https://bugzilla.mozilla.org/show_bug.cgi?id=922546#c28
Flags: needinfo?(alive)

Updated

5 years ago
Blocks: 936200
Blocking to resolve https://bugzilla.mozilla.org/show_bug.cgi?id=922546
blocking-b2g: koi? → koi+

Updated

5 years ago
Blocks: 922546

Updated

5 years ago
No longer blocks: 922546
Duplicate of this bug: 922546
Setting the assignee to Vivien given he has authored the patch. Vivien is this good to land ?
Assignee: nobody → 21
This is already landed on master, need uplift.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g-v1.2: --- → affected
Resolution: --- → FIXED
Component: Gaia::System → Gaia::System::Window Mgmt
The merged commit is e8356415358cc1c9bce496427ff14c88d9d77ec3
I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 e8356415358cc1c9bce496427ff14c88d9d77ec3
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(21)
Sigh. This stuff probably need a completely custom patch to the window manager to land on 1.2. Do we really want this, this is pretty risky :(
Flags: needinfo?(21)
Asking koi? again since I had to introduce a 400ms delay to wait instead of ensuring that the app is fully repainted. The main reason for that is because some apps sometimes takes a few seconds to repaint and this waiting that long makes the interface feels unresponsive. And with the delay the application can still flicker when it comes back from the card view...
blocking-b2g: koi+ → koi?

Updated

5 years ago
blocking-b2g: koi? → -

Updated

5 years ago
Duplicate of this bug: 945656
blocking-b2g: - → fugu?
Flags: needinfo?(styang)

Updated

5 years ago
blocking-b2g: fugu? → fugu+
Flags: needinfo?(styang)

Updated

5 years ago
Whiteboard: [Fugu need]

Updated

5 years ago
Whiteboard: [Fugu need] → [Fugu needs]
Vivien,

What is the impact to fugu?
Flags: needinfo?(21)
Created attachment 8347019 [details] [diff] [review]
247432.patch

patch for v1.2 or v1.2f, verified.
Attachment #8347019 - Flags: review?(ehung)
Comment on attachment 8347019 [details] [diff] [review]
247432.patch

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

You want to backport bug 941024 on top of this one or it will create its own set of other issues.
Attachment #8347019 - Flags: review?(ehung) → review-
(In reply to Preeti Raghunath(:Preeti) from comment #15)
> Vivien,
> 
> What is the impact to fugu?

The impact if the patch from bug 941024 is not backported on top of it, is when you click re-open the application by clicking the icon on the homescreen it may take a while, and make the device feels unresponsive.

With bug 941024 on top of it, the situation is better. If the app take a while to repaint then we still start to open it and it repaints in the face of user for the rest of the time.
Flags: needinfo?(21)

Updated

5 years ago
Whiteboard: [Fugu needs] → [Fugu] [v1.2f-uplift-needed]

Updated

5 years ago
Duplicate of this bug: 936200
You need to log in before you can comment on or make changes to this bug.