1044330 - The screen is waken up both on initializing the ScreenManager and on bootstrapping

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

[Robert Bindar]

Steps to reproduce:
- you could create an Error object inside ScreenManager.turnScreenOn(..) and then dump error.stack and you'll see it called both times once here [1] and later here [2]

[1] http://mxr.mozilla.org/gaia/source/apps/system/js/bootstrap.js#105
[2] http://mxr.mozilla.org/gaia/source/apps/system/js/screen_manager.js#155

The problem is that trying to use the ScreenManager in a marionette test for instance, you'll end up with a bootstrapping turnScreenOn() call (which could be ok to deal with) and another random turnScreenOn() call somewhere in the middle of your test and it can really be a pain sometimes.

Comment 2

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

I agree this should be fixed but I am really not sure if we have the bandwidth to. :'(

Gregor, do you have designated developer on ScreenManager?

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

Let's have Robert fix this, since he found it.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (OOO ~Arg 9) (MoCo-TPE) (please ni?) from comment #2)
> I agree this should be fixed but I am really not sure if we have the
> bandwidth to. :'(
> 

Nothing better :/

Comment 5

[Robert Bindar]

Attachment: Turn the screen on only on bootstrapping

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8470975 [details] [review]
Turn the screen on only on bootstrapping

I think Tim is more appropriate here as he wrote most of the screen manager code IIRC.

Comment 7

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

Comment on attachment 8470975 [details] [review]
Turn the screen on only on bootstrapping

I am not sure the comment in line 149 is still valid or device dependent, but it's never in the spec so if that "feature" bite you we could remove it. Please remove the entire |if (!self._firstOn) {| block instead of just that line.

If you want to be more careful we can ask UX about this.

Comment 8

[Robert Bindar]

Could be device dependent, there is no setting about brightness in bootstrap.js, we should ask UX though, could you point me to someone from UX who could answer this question?

Comment 9

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

There isn't a obvious owner on screen brightness on the UX team wiki; I am redirecting this needinfo to the general :fxosux.

Comment 10

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

So the question here for UX is how the screen brightness should behave when the phone boots up. 

The legacy code I wrote set the screen brightness to 0.5 and slowly transition to the user-set value and we intend to remove it; which would make the screen brightness jumps from whatever value (set by the bootloader) to the user value.

Comment 11

[Stephany Wilkes]

Flagging Patrick to see if he can help redirect this.

Comment 12

[Patryk Adamczyk [:patryk] UX]

Hey Tim, theoretically your current implementation would be the ideal, however I do find the brightness transition is not smooth, it feels like the screen almost flickers.

So I suggest in the boot sequence we keep the screen at 50% brightness, then right before the lock screen / first run experience it quickly adjusts to the OS setting. It wouldn't be a smooth (light gradient) transition but rather a quick cut like seen in movies.

Comment 13

[Robert Bindar]

Tim can you clarify the things a little bit here? I think Patryk is right, the screen dimming makes the phone look very laggy and it'd be nice to fix this.

Isn't it possible to just adjust the dimming step to make the transition look smoother?

Comment 14

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

We could try I think, but that's probably not what this bug was intended to do.

ScreenManager, w/o knowing the internal working of the phone, adjusts the screen brightness with 1/100 of the max value.

https://github.com/mozilla-b2g/gaia/blob/b630b8bcaf/apps/system/js/screen_manager.js#L77-L81

However after looking into the code a bit more I realized it's actually a 8-bit number internally, so that's 256 steps.

http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp#727-742

Comment 15

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

Actually, look back at comment 0 I realized turnScreenOn() should never been called directly in bootstrap.js.

It was added in bug 799714.
https://github.com/mozilla-b2g/gaia/commit/419dc2fff45ec46736520c00857e572e6a62b637

I wonder if simply removing that can fix the problem here. We can clone another bug to fix comment 14.

Comment 16

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

I've filed bug 1064683.

Comment 17

[Robert Bindar]

Thanks for filing bug 106483.
I think removing either of those two calls should work here. I'll try to remove the bootstrap.js call and see if it works as expected, but I have some doubts regarding the booting animations visibility after removing that call.

Comment 18

[Robert Bindar]

Attachment: remove_bootstrap_turnScreenOn

screen_manager.js is loaded before bootstrap.js when the system app is started, so the boostrap.js call is useless

Comment 19

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

Comment on attachment 8486227 [details] [review]
remove_bootstrap_turnScreenOn

I'll take your word for it! Please wait for Gaia-Try to turn green before merging.

Comment 20

[Robert Bindar]

Gij is red. I don't think it's related, I tried it locally on both prebuilt binary and custom b2g desktop, I'll investigate more.

Comment 21

[BMO Automation]

Firefox OS is not being worked on