Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Description • 9 years ago

Comparing to the original Android software on the phone which could turn on/off the screen almost instantly, our response time seems to be lagging behind tens of milliseconds.

I think fixing this could improve the perceived performance.

	Bobby Chien
	Comment 1 • 9 years ago

[Tracking Requested - why for this release]:

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 2 • 9 years ago

Profiles:

Turn on: https://people.mozilla.org/~bgirard/cleopatra/#report=630ac70103603801c40e520ed113a2b33888b511
Turn off (from homescreen): https://people.mozilla.org/~bgirard/cleopatra/#report=8124cbd33b7184c0b988a50d331865b36c8a28a7

The ioctl call for setting screen enabled/disabled runs on main thread which blocks for >200ms.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 3 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #2)
> The ioctl call for setting screen enabled/disabled runs on main thread which
> blocks for >200ms.

Correction: the ioctl call is for blank hwcomposer.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 4 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #3)
> Correction: the ioctl call is for blank hwcomposer.

This exists also on Android.

But for turnning on the screen, Android first paint and then unblank, but we now do unblank first and then JS refresh the UI. Tried to set screenEnabled true after fireScreenChangeEvent() [1], and it feels better, just can't unlock...

[1] http://lxr.mozilla.org/gaia/source/apps/system/js/screen_manager.js#496

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 5 • 9 years ago

Also there's a #screen restyle because of [1]. I've asked Tim, the reason to set screen black is long time ago, when screen on, the outdated screen will be shown, so hide all the elements and set background black when screen goes off.

[1] http://mxr.mozilla.org/gaia/source/apps/system/js/screen_manager.js#487

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 6 • 9 years ago

Current idea is to let LockScreenWindowManager to receive screenchange event at first, set screenEnabled true after first paint, and then fire screenchange event to other listeners. Since it can not separate LockScreenWindowManager from other screenchange listeners, I'll probably create a new custom event.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 7 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #6)
> Current idea is to let LockScreenWindowManager to receive screenchange event
> at first, set screenEnabled true after first paint, and then fire
> screenchange event to other listeners. Since it can not separate
> LockScreenWindowManager from other screenchange listeners, I'll probably
> create a new custom event.

Actually remove screenoff class is good enough, but the clock, charging status and date could flicker, I am checking why.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 8 • 9 years ago

When screen on, changing the textContent of #lockscreen-clock-time, #lcokscreen-date and #lockscreen-charging will first remove the text frame, and then trigger nsCSSFrameConstructor::ContentAppended() with aAllowLazyConstruction true, but the function just returns since MaybeConstructLazily() is true [1]. This seems why nsTextFrame::BuildDisplayList() is not called and causes flickering.

Wonder is there a way to force a frame construction.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7037

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 10 • 9 years ago

Never mind comment 9, it's just nested nsBlockFrame::BuildDisplayList() invocation.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 11 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #8)
> When screen on, changing the textContent of #lockscreen-clock-time,
> #lcokscreen-date and #lockscreen-charging will first remove the text frame,
> and then trigger nsCSSFrameConstructor::ContentAppended() with
> aAllowLazyConstruction true, but the function just returns since
> MaybeConstructLazily() is true [1]. This seems why
> nsTextFrame::BuildDisplayList() is not called and causes flickering.
> 
> Wonder is there a way to force a frame construction.

I've asked dbaron about this, he said data can be used instead of textContent so the text node won't be removed and inserted.

Besides, he expects the text frame will be constructed since nsPresShell::WillPaint() will call FlushPendingNotificatons(). I checked, and found MaybeConstructLazily() does set NODE_DESCENDANTS_NEED_FRAMES for rootElement:

  html@aa725580 lang="en-US" dir="ltr" langs="en-US"

but nsCSSFrameConstructor::CreateNeededFrames() is checking a different rootElement:

  html@ae0691c0 sizemode="fullscreen" windowtype="navigator:browser" id="shell" xmlns="http://www.w3.org/1999/xhtml" chromehidden="menubar toolbar location directories status "

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 12 • 9 years ago

There're two PresShell instances, one for shell.html, and one for system app index.html. When screen is on, the PresShell of system app's index.html has mIsActive and IsVisible() false, so CreateNeededFrames() does nothing.

	David Baron :dbaron:
	Comment 13 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #11)
> I've asked dbaron about this, he said data can be used instead of
> textContent so the text node won't be removed and inserted.

Note that data is a property of the text node, whereas textContent's is a setter on both elements and text nodes, and on elements it replaces the contents of the element with a single new text node.

(In reply to Ting-Yu Chou [:ting] from comment #12)
> There're two PresShell instances, one for shell.html, and one for system app
> index.html. When screen is on, the PresShell of system app's index.html has
> mIsActive and IsVisible() false, so CreateNeededFrames() does nothing.

I don't know what the distinction is between shell.html and the system app index.html is... but that sounds like it may be problematic.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 14 • 9 years ago

dbaron, thank you for your comments, you saved me a day!

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 16 • 9 years ago

Comment on attachment 8685240 [details]
[gaia] janus926:bug-1188232 > mozilla-b2g:master

Update lockscreen clock and battery status by |data| instead of |textContent|, so the text nodes are not replaced, which cause flickering when screen is on. Note docshell.isActive of system app is set to true after two async events (sizemodechange, and browser API setVisible()), so PresShell::WillPaint() could do nothing if it comes earlier.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 17 • 9 years ago

Comment on attachment 8685240 [details]
[gaia] janus926:bug-1188232 > mozilla-b2g:master

r+ with nits: could you add some comments about the issue you have found during debugging the patch? Without such comments the reason to do the change is unclear.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 18 • 9 years ago

Thanks for reviewing. Comments added.

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/c32c56d2a1fde63cc5892d2f7bda7cdacaef3887

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 20 • 9 years ago

This bug introduces a use of deprecated mozL10n.get in:

+  // could cause flickering while transitioning docshell.isActive to true.
+  this.elements.charging.firstChild.data =
+    navigator.mozL10n.get(l10nAttrs.id, l10nAttrs.args);

Can you please switch it to either declarative (setAttribute data-l10n-id, data-l10n-args) or if you need to manipulate around DOM (I don't think you should), use asynchronous mozL10n.formatValue please?

We're trying to remove all remaining mozL10n.get calls in this cycle.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 21 • 9 years ago

Ting-Yu:

Reverted: https://github.com/mozilla-b2g/gaia/commit/27bc9412ca607648bc398b25bb1ae25653b2b278

You can set review to Zibi this time to make sure the API part is correct.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 23 • 9 years ago

Comment on attachment 8686411 [details]
[gaia] janus926:bug-1188232 > mozilla-b2g:master

Use formatValue() to avoid changing textContent which causes flickering.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 24 • 9 years ago

Comment on attachment 8686411 [details]
[gaia] janus926:bug-1188232 > mozilla-b2g:master

Thanks!

	Carsten Book [:Tomcat]
	Comment 25 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/0365a042254af38d60b6ab90c71a8e2fccf3d31d