Last Comment Bug 707589 - [Gonk] When screen is enabled/disabled, send visibility change events to windows
: [Gonk] When screen is enabled/disabled, send visibility change events to windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on: 702256 743064
Blocks: 725247
  Show dependency treegraph
 
Reported: 2011-12-04 17:59 PST by Justin Lebar (not reading bugmail)
Modified: 2012-04-05 16:28 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Send "sizemodechange" event when the screen is turned on/off. (7.64 KB, patch)
2012-03-30 03:36 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Splinter Review
Send "sizemodechange" event when the screen is turned on/off. v2 (7.87 KB, patch)
2012-04-02 03:03 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Splinter Review
Send "sizemodechange" event when the screen is turned on/off. v3 (6.37 KB, patch)
2012-04-04 22:11 PDT, Kan-Ru Chen [:kanru] (UTC+8)
mwu.code: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-12-04 17:59:59 PST
When you set screen.mozEnabled, we need to inform the windows that their visibility has changed.
Comment 1 Kan-Ru Chen [:kanru] (UTC+8) 2012-03-30 03:36:48 PDT
Created attachment 610845 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off.
Comment 2 Michael Wu [:mwu] 2012-03-31 16:05:08 PDT
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+8) 2012-04-02 03:03:48 PDT
Created attachment 611409 [details] [diff] [review]
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] 2012-04-04 20:14:14 PDT
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+8) 2012-04-04 22:11:50 PDT
Created attachment 612455 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off. v3
Comment 6 Michael Wu [:mwu] 2012-04-04 22:33:41 PDT
Comment on attachment 612455 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off. v3

Excellent.
Comment 7 [:fabrice] Fabrice Desré 2012-04-04 23:04:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/348e9d3a6bec
Comment 8 Justin Lebar (not reading bugmail) 2012-04-05 06:04:39 PDT
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+8) 2012-04-05 07:04:29 PDT
(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) 2012-04-05 07:14:19 PDT
(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+8) 2012-04-05 07:37:36 PDT
Maybe I could just close the fd from outside. Will try it later.
Comment 12 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-04-05 08:17:28 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.