808231 - Make showing the homescreen instant again after bug 806029

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P3)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Because of bug 806029, we've been getting away with making the homescreen invisible, but the homescreen retaining its layer buffers.  After bug 806029, the homescreen will really actually not be visible; it'll throw away its buffers when hidden.

The effect of bug 806029 can be seen by
 (1) Open app A
 (2) Press HOME button

When transitioning back to the homescreen, there's a brief moment where the homescreen is blank (i.e. only wallpaper shows through) while it repaints its buffers after being made visible again.  This is pretty janky.  We can do better.

One solution is to never setVisible(false) the homescreen.  This has the disadvantage of undoing some of the win from bug 806029, because the homescreen allocates a relatively high amount of pmem.  e.me apps make this even less desirable.

Another solution is to add a "nextpaint" mozbrowser notification, which we can deliver after a setVisible(true).  We can reuse pretty much all the same code from the firstpaint event.  Then, the window manager would only start the app-hide animation after receiving nextpaint (or timing out).

Tim, I recall you had some plans for screenshots with the homescreen?  Depending on how that's implemented, that could be another solution here.

Comment 1

[Justin Lebar (not reading bugmail)]

> One solution is to never setVisible(false) the homescreen.  This has the disadvantage of undoing 
> some of the win from bug 806029, because the homescreen allocates a relatively high amount of 
> pmem.  e.me apps make this even less desirable.

Additionally, if you never setVisible(false) the homescreen, we'll consider it to be a foreground app (that is, we might kill an actual foreground app before we kill the homescreen), we'll never minimize memory usage in that process, we'll run it with high CPU priority...  it's sad times.

> Then, the window manager would only start the app-hide animation after receiving nextpaint (or 
> timing out).

The trick with this has always been forcing a paint after you start listening for mozafterpaint.  If we can somehow force this, then that might work.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #1)
> > One solution is to never setVisible(false) the homescreen.  This has the disadvantage of undoing 
> > some of the win from bug 806029, because the homescreen allocates a relatively high amount of 
> > pmem.  e.me apps make this even less desirable.
> 
> Additionally, if you never setVisible(false) the homescreen, we'll consider
> it to be a foreground app (that is, we might kill an actual foreground app
> before we kill the homescreen), we'll never minimize memory usage in that
> process, we'll run it with high CPU priority...  it's sad times.
> 

Yeah, with current API.  That's solvable but I agree that it makes the approach less desirable.

> > Then, the window manager would only start the app-hide animation after receiving nextpaint (or 
> > timing out).
> 
> The trick with this has always been forcing a paint after you start
> listening for mozafterpaint.  If we can somehow force this, then that might
> work.

Not a problem at all, the forcing of the repaint is done deep in the platform and keyed off the mozbrowser setVisible(true).  If the repaint never happens, the homescreen (or whatever else) never appears onscreen again.  So mozbrowser can register a paint listener just before the docshell activate triggered by setVisible(true) and be 100% confident the next paint will be the one it wants.

(Note, this only works for false->true transitions.  No guarantee for true->true.)

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Is this the Gecko bug for implementing mozbrowsernextpaint, or the Gaia fix when that is implemented?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: implement mozbrowsernextpaint event

This implements the mozbrowsernextpaint event we can build upon.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

Pointer to Github pull-request

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 684801 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

With this patch waiting for mozbrowsernextpaint before starting transitions I don't see any flickering anymore, only smooth transitions from app to homescreen. *Sometimes* the calculator app flickers once when it's already loaded and we open it again. This is caused by the huge background image that seems to take some time to load and decode. Setting the calculator background to the background image's tone of black could probably mitigate the effect a little bit.

The only drawback is that it can take quite a long time before the transition actually starts because painting the homescreen can be really slow. Same thing for painting apps that were in the background.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 684799 [details] [diff] [review]
implement mozbrowsernextpaint event

Whoa, hold on a second here.  This runs JS code on every paint.  That's what we were explicitly avoiding with mozbrowserfirstpaint.

If you just want a "next paint" event, I'd be far more comfortable with a method which lets you pass a function to be called once, on the next paint event.  That way, we don't have a permanent mozafterpaint event listener in every mozbrowser.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 684801 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

Waiting for nextpaint would be a bad idea if for some reason a bad app never paints (page loading, while(1) loop), etc. We will still need a timer to keep us from waiting too long.

Another thing this patch removed is to allow the transition follows properly cancel the previously one. What if the user hit home again before the paint event occurs? Our QA did find the issue with it ...

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: implement mozbrowsernextpaint event

I tried letting the BrowserElementParent send a message to the child that then registers the MozAfterPaint event handler. The problem is that often the message arrives *after* the next pain has happened and we must not miss that.

I came up with another solution that adds a MozAfterPaint handler right after .setVisible(false) has been called. There are no paints until we set the docShell active again. This method should have no overhead and we can add the 'mozbrowsernextpaint' listener right after (or before) we call .setVisible(true).

Comment 10

[Justin Lebar (not reading bugmail)]

> I tried letting the BrowserElementParent send a message to the child that then registers 
> the MozAfterPaint event handler. The problem is that often the message arrives *after* 
> the next paint has happened and we must not miss that.

You did

  frame.addNextPaintListener(function() { ... });
  frame.setVisible(true);

and that didn't work?  If it didn't work, I don't see why the approach in this patch would work, either.

Although I'd prefer the addNextPaintListener() approach, since it's more general, the approach here is fine too.  But we shouldn't call the event "nextpaint", in this case.  Also, we need a test for this, please.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Darn, messages are of course delivered in order, I didn't think of that (and called setVisible first). I'll attach a new patch with the more general approach and a test.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: implement mozbrowsernextpaint event

Tests aren't included, yet. I added frame.addNextPaintListener() method. Do you think that method should take a callback and call that? That would probably be easier now that I think about it instead of adding an event listener every time and forwarding the event.

Another thing I noticed while testing is that if we open an app and I press the home button immediately that is two setVisible() calls in succession. If the homescreen buffer hasn't been discarded yet, we wait forever for nextpaint because it's not happening. Is there any way to check if setVisible(false) discarded the buffer already? Can we kick off a paint?

Comment 13

[Justin Lebar (not reading bugmail)]

> Do you think that method should take a callback and call that? That would probably be 
> easier now that I think about it instead of adding an event listener every time and 
> forwarding the event.

Yeah, I think that's probably a better API.

> Another thing I noticed while testing is that if we open an app and I press the home 
> button immediately that is two setVisible() calls in succession. If the homescreen buffer 
> hasn't been discarded yet, we wait forever for nextpaint because it's not happening. Is 
> there any way to check if setVisible(false) discarded the buffer already? Can we kick off 
> a paint?

It sounds to me like perhaps the API is behaving correctly in this case, and you just need to figure out how to notice that Gaia has setVisible(false) on the frame so you can stop listening for a paint that you know isn't coming.  But perhaps I'm misunderstanding the situation.

We might want a removeNextPaintListener() method, for this case.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

This should be blocking because it's bad. And because it blocks a blocker (bug 814322).

Comment 16

[Justin Lebar (not reading bugmail)]

> Priority: -- → P1

FYI, I believe B2G release drivers &c have been using the priority field in a meaningful way, so we shouldn't reset it willy-nilly.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #16)
> FYI, I believe B2G release drivers &c have been using the priority field in
> a meaningful way, so we shouldn't reset it willy-nilly.

FYI, I didn't do that on purpose, sorry. Session restore did it for me.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: implement mozbrowsernextpaint event (WIP)

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

So I think the patch is ready and works on my machine. I'm now trying to add a test:

> dom/browser-element/mochitest/browserElement_NextPaint.js

Any hints on how to run these tests locally? I tried but wasn't successful and couldn't find any docs about this.

Comment 20

[Justin Lebar (not reading bugmail)]

> Any hints on how to run these tests locally? I tried but wasn't successful and couldn't 
> find any docs about this.

They're mochitests.  But you probably want to create the test file using the create_test.py (whatever it's called) script, so it creates all the necessary boilerplate for you.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #20)
> They're mochitests.  But you probably want to create the test file using the
> create_test.py (whatever it's called) script, so it creates all the
> necessary boilerplate for you.

Thanks, that script was a good hint. I realized they're mochitests but for some reason I can't get them to run with a b2g build. I wrote them now in my default m-c dir and used the Fx desktop build.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

GOSH, I'm sorry for resetting the priority and flags :(

Comment 23

[Justin Lebar (not reading bugmail)]

(In reply to Tim Taubert [:ttaubert] from comment #22)
> GOSH, I'm sorry for resetting the priority and flags :(

bugzilla++?  :)

> I realized they're mochitests but for some reason I can't get them to run with a b2g 
> build.

I don't know if you can; I always do a non-B2G build to run tests.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Attachment: implement mozbrowsernextpaint event

Comment 25

[Justin Lebar (not reading bugmail)]

Comment on attachment 689254 [details] [diff] [review]
implement mozbrowsernextpaint event

Looks great with some nits.

Please post patches with 8 lines of context.

>Bug 808231 - Make showing the homescreen instant again after bug 806029

We could use a more descriptive checkin comment.

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>+    addMsgListener("add-next-paint-listener", this._addNextPaintListener.bind(this));
>+    addMsgListener("remove-next-paint-listener", this._removeNextPaintListener.bind(this));

I think these might be clearer if they were called
activate/deactivate-next-paint-listener (and if the functions were changed to
match).  In the child code, we only ever have one next-paint listener, while in
the parent we can add multiple ones (using an identically-named method,
confusingly enough).

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js
>+  addMessageListener("nextpaint", function onNextPaint() {
>+    let listeners = self._nextPaintListeners;
>+    self._nextPaintListeners = [];
>+    for (let listener of listeners) {
>+      try {
>+        listener();
>+      } catch (e) {
>+        // If a listener throws we'll continue.
>+      }
>+    }
>+  });

All of the other addMessageListener()'s call a function.  (If we defined all of
those functions inside the constructor, it would be pretty unweildy.) Would you
mind moving this into its own function?

>diff --git a/dom/browser-element/mochitest/browserElement_NextPaint.js b/dom/browser-element/mochitest/browserElement_NextPaint.js

This test looks good, but I might want to run it on try before pushing, since
these tests have a way of failing in places you don't expect.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Implement add/removeNextPaintListener() methods for mozbrowsers

(In reply to Justin Lebar [:jlebar] from comment #25)
> Please post patches with 8 lines of context.

Stupid |hg export| doesn't support that. So either 8 lines of context or the patch header. Think I'll go with the former when asking for review and the latter when attaching the final patch.

> >Bug 808231 - Make showing the homescreen instant again after bug 806029
> We could use a more descriptive checkin comment.

Done.

> I think these might be clearer if they were called
> activate/deactivate-next-paint-listener (and if the functions were changed to
> match).

Done.

> All of the other addMessageListener()'s call a function. Would
> you mind moving this into its own function?

Done.

> This test looks good, but I might want to run it on try before pushing, since
> these tests have a way of failing in places you don't expect.

Will push to try in a second.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #26)
> > This test looks good, but I might want to run it on try before pushing, since
> > these tests have a way of failing in places you don't expect.
> 
> Will push to try in a second.

https://tbpl.mozilla.org/?tree=Try&rev=f250da0dff69

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 684801 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #8)
> Waiting for nextpaint would be a bad idea if for some reason a bad app never
> paints (page loading, while(1) loop), etc. We will still need a timer to
> keep us from waiting too long.

Re-added a fallback timer.

> Another thing this patch removed is to allow the transition follows properly
> cancel the previously one. What if the user hit home again before the paint
> event occurs? Our QA did find the issue with it ...

Transition is now cancelable again.

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 684801 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

I read through the change. Thanks you for the extensive fix. Please see comment on Github.

A few general note here:

-- I am not quite sure your use the frame background correctly. Frame background are stored in the system app, and being updated when firstpaint took place, That means, it will only contain the loading/initializing screen of the app. We do that for security requirement. We shouldn't be storing your bank statement on the app just because you are happening to be opening the app.
-- frame background should change to white if we are sure of the frame contains the actual content. For an app without specifying background, we don't want the app loading screen being put underneath the actual elements (I cheated a little bit in the |screenshotTaken()| function, but we should think of a way moving away from it, not toward it).
-- Based on that, for a un(first)painted frame, we would definitely need a frame background, instead of waiting for firstpaint or nextpaint. But, for a loaded frame, asking for a frame background will make less sense, given the fact that it will be the loading screen. It will not be visible to the user anyway... I don't know why you want |waitForNextPaintAndBackground| there.
-- You removed a line in the |slowTransition|. It was added because I want to make sure the transition is cancellable during that phase. It's fine though if you did the test by manually tweak the |kTransitionTimeout|.
-- Unfortunately there are still know automated test here. My experience working on bug 810738 told me that we would need to make sure all of the phases is cancelable by pressing home key during every phase. Yes, testing them takes longer than writing a fix ...
--- opening, before transition
--- opening, transitioning
--- closing, before transition
--- closing, transitioning
--- switching before transition
--- switching; closing transition
--- switching; "cards" moving
--- switching, opening transition

From reading the code I don't think there will be any fault (good job!); however just to be sure, this is one piece of the code we cannot afford regression.


Thank you again!

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 684801 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

Clear the flag, just to make sure I can see the notification again if you need my response. I can review the code again if it's being modified and/or question has been answered.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 689618 [details] [diff] [review]
Implement add/removeNextPaintListener() methods for mozbrowsers

Pushed the platform part of this bug, try was all green. Now this has a little more time to make its way to the beta branch before we can merge the Gaia change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7c95201da5

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 684801 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6612/files

Wow, that's fast; I think I can give you an r+ without manually testing this myself. Please make sure you test all the phases before landing the code!

* One more comment is being added on Github

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #29)
> I read through the change. Thanks you for the extensive fix. Please see
> comment on Github.

Comments on GitHub addressed, PR updated.

> -- Based on that, for a un(first)painted frame, we would definitely need a
> frame background, instead of waiting for firstpaint or nextpaint.

So we don't need waitForNextPaintOrBackground() and should just use setFrameBackground()? My intention was to use whatever is faster here. If nextpaint (that is also the first paint) comes first, we don't need a background. If however the background comes first we can start the transition with the background and wait a little more until the next/first paint.

> -- You removed a line in the |slowTransition|. It was added because I want
> to make sure the transition is cancellable during that phase. It's fine
> though if you did the test by manually tweak the |kTransitionTimeout|.

Ok, let me re-add that.

> -- Unfortunately there are still know automated test here. My experience
> working on bug 810738 told me that we would need to make sure all of the
> phases is cancelable by pressing home key during every phase. Yes, testing
> them takes longer than writing a fix ...
> --- opening, before transition
> --- opening, transitioning
> --- closing, before transition
> --- closing, transitioning
> --- switching before transition
> --- switching; closing transition
> --- switching; "cards" moving
> --- switching, opening transition

Thank you for the list, I'll take a look at all those situations.

Comment 34

[Justin Lebar (not reading bugmail)]

>> Please post patches with 8 lines of context.
>
> Stupid |hg export| doesn't support that.

What's in your ~/.hgrc?  I have

  [diff]
  git = 1
  showfunc = 1
  unified = 8

and |hg export| outputs with 8 lines of context for me (on my Mac, hg version 2.4).

(You know about hg bzexport, which automagically attaches patches to bugzilla, right?  I don't know if it sets the right -U property, but it's still pretty nice.)

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #34)
>   [diff]
>   unified = 8

That's the line I was missing. jdm told me about that like two minutes ago. Thank you as well for helping :)

> (You know about hg bzexport, which automagically attaches patches to
> bugzilla, right?  I don't know if it sets the right -U property, but it's
> still pretty nice.)

Yeah I heard that before, should probably give it a try.

Comment 36

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/7d7c95201da5

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3dcc488f22f
https://hg.mozilla.org/releases/mozilla-aurora/rev/a66f2999713a

Comment 38

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hey, Tim, is there anything preventing this from landing?

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #38)
> Hey, Tim, is there anything preventing this from landing?

No, sorry, was busy with other work. Tested the various situations and the patch doesn't fail. Will merge.

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

https://github.com/mozilla-b2g/gaia/commit/1f818a4bdd08d1554cc2e8063a4ab23526e36c0a