Closed Bug 1188232 Opened 7 years ago Closed 7 years ago

[Aries] Turing on/off screen using power button takes longer than expected

Categories

(Firefox OS Graveyard :: Gaia::System, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
FxOS-S11 (13Nov)
tracking-b2g backlog

People

(Reporter: kanru, Assigned: ting)

References

Details

(Whiteboard: [perf-wanted])

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [perf-wanted]
[Tracking Requested - why for this release]:
Whiteboard: [perf-wanted] → [perf-wanted][Profile-wanted]
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.
(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.
(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
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
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.
(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.
Priority: -- → P2
Whiteboard: [perf-wanted][Profile-wanted] → [perf-wanted]
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
Never mind comment 9, it's just nested nsBlockFrame::BuildDisplayList() invocation.
(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 "
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.
(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.
dbaron, thank you for your comments, you saved me a day!
Comment on attachment 8685240 [details] [review]
[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.
Attachment #8685240 - Flags: review?(gweng)
Comment on attachment 8685240 [details] [review]
[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.
Attachment #8685240 - Flags: review?(gweng) → review+
Thanks for reviewing. Comments added.
Assignee: nobody → janus926
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/c32c56d2a1fde63cc5892d2f7bda7cdacaef3887
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S11 (13Nov)
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.
Flags: needinfo?(janus926)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8686411 [details] [review]
[gaia] janus926:bug-1188232 > mozilla-b2g:master

Use formatValue() to avoid changing textContent which causes flickering.
Flags: needinfo?(janus926)
Attachment #8686411 - Flags: review?(gandalf)
Attachment #8685240 - Attachment is obsolete: true
Comment on attachment 8686411 [details] [review]
[gaia] janus926:bug-1188232 > mozilla-b2g:master

Thanks!
Attachment #8686411 - Flags: review?(gandalf) → review+
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/0365a042254af38d60b6ab90c71a8e2fccf3d31d
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1234731
You need to log in before you can comment on or make changes to this bug.