The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: kanru)

Tracking

Trunk
mozilla14
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
When you set screen.mozEnabled, we need to inform the windows that their visibility has changed.
OS: Gonk → All
Hardware: x86 → All
Version: unspecified → Trunk
OS: All → Gonk
(Assignee)

Updated

5 years ago
Blocks: 725247
(Assignee)

Updated

5 years ago
Assignee: nobody → kchen
(Assignee)

Comment 1

5 years ago
Created attachment 610845 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off.
Attachment #610845 - Flags: review?(mwu)

Comment 2

5 years ago
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.
Attachment #610845 - Flags: review?(mwu)
(Assignee)

Comment 3

5 years ago
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.
Attachment #610845 - Attachment is obsolete: true
Attachment #611409 - Flags: review?(mwu)

Comment 4

5 years ago
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.
Attachment #611409 - Flags: review?(mwu)
(Assignee)

Comment 5

5 years ago
Created attachment 612455 [details] [diff] [review]
Send "sizemodechange" event when the screen is turned on/off. v3
Attachment #611409 - Attachment is obsolete: true
Attachment #612455 - Flags: review?(mwu)

Comment 6

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

Excellent.
Attachment #612455 - Flags: review?(mwu) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/348e9d3a6bec
Keywords: checkin-needed
(Reporter)

Comment 8

5 years ago
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?
(Assignee)

Comment 9

5 years ago
(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?
(Reporter)

Comment 10

5 years ago
(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?
(Assignee)

Comment 11

5 years ago
Maybe I could just close the fd from outside. Will try it later.
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.
http://hg.mozilla.org/mozilla-central/rev/348e9d3a6bec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
Depends on: 743064
You need to log in before you can comment on or make changes to this bug.