707589 - [Gonk] When screen is enabled/disabled, send visibility change events to windows

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

When you set screen.mozEnabled, we need to inform the windows that their visibility has changed.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Send "sizemodechange" event when the screen is turned on/off.

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 610845 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off.

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

Looks good overall, but I would like to see the changes in nsAppShell.cpp move to nsWindow.cpp. It should simplify things.

::: widget/gonk/nsAppShell.cpp
@@ +142,5 @@
> +        int fd;
> +
> +        // Cannot use epoll here because kSleepFile and kWakeFile are
> +        // always ready to read and blocking.
> +        for (;;) {

Generally, I would prefer having code dispatch itself to the thread at the end instead of an infinite loop like this.

@@ +143,5 @@
> +
> +        // Cannot use epoll here because kSleepFile and kWakeFile are
> +        // always ready to read and blocking.
> +        for (;;) {
> +            fd = open(kSleepFile, O_RDONLY, 0);

I recommend using ScopedClose

@@ +158,5 @@
> +            close(fd);
> +            NS_DispatchToMainThread(new ScreenOnOffEvent(true));
> +        }
> +
> +        return NULL;

NS_IMETHOD methods should return NS_* (nsresult) return codes. In this case, NS_OK.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Send "sizemodechange" event when the screen is turned on/off. v2

Move monitor thread to nsWindow.cpp and replace infinite loop with self dispatching.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 611409 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off. v2

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

Looks good but I think we can simplify the code further.

::: widget/gonk/nsWindow.cpp
@@ +108,5 @@
> +        char buf;
> +
> +        // Cannot use epoll here because kSleepFile and kWakeFile are
> +        // always ready to read and blocking.
> +        ScopedClose fd(open(kSleepFile, O_RDONLY, 0));

I recommend putting each of these read sections into their own block so they can close the fd immediately after finishing.

@@ +123,5 @@
> +        NS_WARN_IF_FALSE(len >= 0, "WAIT_FOR_FB_WAKE failed");
> +        NS_DispatchToMainThread(new ScreenOnOffEvent(true));
> +
> +        // Dispatch to ourself.
> +        NS_DispatchToCurrentThread(new FramebufferWatcher());

Can you use |this| instead of creating a new object?

@@ +225,5 @@
>      return gFocusedWindow->mEventCallback(&aEvent);
>  }
>  
> +/* static */ void
> +nsWindow::TurnOnScreen()

Let's inline TurnOnScreen/TurnOffScreen into ScreenOnOffEvent::Run. You can use DispatchEvent to fire events on the windows since mEventCallback is protected.

@@ +229,5 @@
> +nsWindow::TurnOnScreen()
> +{
> +    for (unsigned int i = 0; i < sTopWindows.Length(); i++) {
> +        nsWindow *win = sTopWindows[i];
> +        nsSizeModeEvent event(true, NS_SIZEMODE, win);

Set event.time. Some of the events fired in this file don't set it unfortunately, but it's a bug.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Send "sizemodechange" event when the screen is turned on/off. v3

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 612455 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off. v3

Excellent.

Comment 7

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/348e9d3a6bec

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 612455 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off. v3

Wait a second...how does FramebufferWatcher shut down?

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Justin Lebar [:jlebar] from comment #8)
> Comment on attachment 612455 [details] [diff] [review]
> Send "sizemodechange" event when the screen is turned on/off. v3
> 
> Wait a second...how does FramebufferWatcher shut down?

We could call mFramebufferWatcher->Shutdown() or set a exit flag, but it still requires a screen toggle to break the read(). Could we kill the thread?

Comment 10

[Justin Lebar (not reading bugmail)]

(In reply to Kan-Ru Chen [:kanru] from comment #9)
> We could call mFramebufferWatcher->Shutdown() or set a exit flag, but it
> still requires a screen toggle to break the read(). Could we kill the thread?

I don't know.  Benjamin?

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Maybe I could just close the fd from outside. Will try it later.

Comment 12

[Benjamin Smedberg]

The NSPR functions do not provide any way to kill a thread, because in general it is not safe to do so because that thread may be in a mutex or have some other inconsistent state. You need to find a way to unblock the thread and have it terminate itself.

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/348e9d3a6bec