Do not show notifications when in fullscreen or screensaver mode....

Do not show notifications when in fullscreen or screensaver mode.

I add full-screen/presentation mode detection for all 3 platforms. I also
add screensaver detection for MacOSX and Linux since it is missing in these
2 platforms.

BUG=25061
TEST=Manual test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72539

[evanm, 2011-01-21 00:39:08]

Can you fix the review description to not include the extra text?  :)

(In the future you can include your review request info as part of the message
when you mail the change...)

[evanm, 2011-01-21 00:42:05]

+ Linux window management expert

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
chrome/browser/notifications/notification_ui_manager.cc:150:
!IsFullScreenMode();
IsFullScreenMode, at least in the Linux implementation, will take a lot of time
to complete.

Is this on the UI thread?
If so, you're blocking the UI thread.
If not, you can't make UI calls from non-UI threads...

[evanm, 2011-01-21 00:43:07]

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux.cc
File chrome/browser/fullscreen_linux.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:33: for (int i =
static_cast<int>(num_children - 1); i >= 0; --i)
One more minor comment:

static_cast<int>(num_children - 1)

if num_children is zero this will overflow.  Maybe that can't happen, but it'd
make me feel better if you did

static_cast<int>(num_children) - 1

[jianli, 2011-01-21 01:15:50]

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
chrome/browser/notifications/notification_ui_manager.cc:150:
!IsFullScreenMode();
On 2011/01/21 00:42:05, evanm wrote:
> IsFullScreenMode, at least in the Linux implementation, will take a lot of
time
> to complete.
> 
> Is this on the UI thread?
> If so, you're blocking the UI thread.
> If not, you can't make UI calls from non-UI threads...

Yes, it is on UI thread. The Win32/MacOSX implementation should run quite fast.
For Linux, do you know any better way to do the detection? I can do a couple
following things to optimize the Linux experience:
1) Combine screensaver and fullscreen detection code so that we do not need to
call XQueryTree twice for each separate detection.
2) We can also cache the last user state from a very small period of time ago,
say 0.1s.

[jianli, 2011-01-21 01:24:35]

One other thought. Can I delegate the detection logic to BACKGROUND_X11
thread on Linux? Thanks.


On Thu, Jan 20, 2011 at 5:15 PM, <jianli@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
> File chrome/browser/notifications/notification_ui_manager.cc (right):
>
>
>
http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
> chrome/browser/notifications/notification_ui_manager.cc:150:
> !IsFullScreenMode();
> On 2011/01/21 00:42:05, evanm wrote:
>
>> IsFullScreenMode, at least in the Linux implementation, will take a
>>
> lot of time
>
>> to complete.
>>
>
>  Is this on the UI thread?
>> If so, you're blocking the UI thread.
>> If not, you can't make UI calls from non-UI threads...
>>
>
> Yes, it is on UI thread. The Win32/MacOSX implementation should run
> quite fast. For Linux, do you know any better way to do the detection? I
> can do a couple following things to optimize the Linux experience:
> 1) Combine screensaver and fullscreen detection code so that we do not
> need to call XQueryTree twice for each separate detection.
> 2) We can also cache the last user state from a very small period of
> time ago, say 0.1s.
>
>
> http://codereview.chromium.org/6359008/
>

[Daniel Erat, 2011-01-21 01:28:17]

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux.cc
File chrome/browser/fullscreen_linux.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:51: XID topmost_window =
FindTopMostWindow(window);
This seems wrong.  It looks like it gets called recursively until you don't find
any child windows.  Ideally you'd just search with a depth of one -- in the
common case, the window manager is a reparenting one, so there will be one level
of windows between the root and the client windows.  In the less-common case,
the client windows will be direct children of the root.

Why not use gtk_util::EnumerateTopLevelWindows() from browser/ui/gtk/gtk_util.h,
which knows how to use _NET_CLIENT_LIST_STACKING if the WM supports it (most do)
to find the top client window without having to walk the tree?

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:59: bool DoFullScreenCheck() {
Please give this function a more-descriptive name; "FullScreenCheck" doesn't
mean anything to me.  Something like "IsTopmostWindowFullscreen()" seems like it
might be more accurate.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:78: GdkWindow* gwindow =
gdk_window_lookup(topmost_window);
The docs for gdk_window_lookup() are vague: will it return a GdkWindow for every
X window, or only for X windows opened by GDK but possibly by other clients (I
don't know how it could check this unless GDK sets a property on every window it
opens), or only for X windows opened by this client?

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:88: bool IsX11WindowFullScreen(XID window) {
If you want to call this from other places (it looks like you do), it should
probably go in ui/base/x/x11_util.h instead.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:90: Atom atom =
gdk_x11_get_xatom_by_name_for_display(
It'd be good to cache this so you don't need to do a round trip to look it up
each time this is called.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:95: "_NET_WM_STATE",
fix indenting

http://codereview.chromium.org/6359008/diff/1/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/6359008/diff/1/ui/base/x/x11_util.h#newcode81
ui/base/x/x11_util.h:81: bool ExistsProperty(XID window, const std::string&
property_name);
nit: "PropertyExists"

[John Gregg, 2011-01-21 07:14:50]

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
chrome/browser/notifications/notification_ui_manager.cc:156:
ShowNotifications();
This doesn't look right at first glance because  CheckAndShowNotifications() is
already calling ShowNotifications().  Can you add a comment that this is
detecting (with the a timer) the first time that the user becomes active?

[dmac, 2011-01-21 19:34:44]

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.mm
File chrome/browser/fullscreen_mac.mm (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:7: #import "base/logging.h"
base/logging should be under system imports.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:17: - (BOOL) isFullScreen;
no space between ) and i (same on line below)

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:18: - (void)
setIsFullScreen:(BOOL)newIsFullScreen;
cocoa naming would be isFullScreen and setFullScreen

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:34: /*outActualType*/ NULL,
get rid of /*value*/ and do you want to check for an error on this?

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:47: // Check if the user is in presentation
mode initially. We do this by
When this is initialized, do we expect Chrome to be in the background? If we are
in the foreground could we not use

GetSystemUIMode?

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:98: g_fullScreenMonitor = nil;
[g_fullScreenMonitor release]; ?

http://codereview.chromium.org/6359008/diff/1/chrome/browser/idle_mac.mm
File chrome/browser/idle_mac.mm (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/idle_mac.mm#newc...
chrome/browser/idle_mac.mm:53: isScreensaverRunning_ = YES;
cocoa conventions would not have the "is" in front of var names.

[jianli, 2011-01-21 20:21:00]

All comments are addressed.

For Evan, since now we're calling gtk_util::EnumerateTopLevelWindows to
enumerate all top-level windows which is normally much faster than XQueryTree,
we might be fine to run the detection code in UI thread.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux.cc
File chrome/browser/fullscreen_linux.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:33: for (int i =
static_cast<int>(num_children - 1); i >= 0; --i)
On 2011/01/21 00:43:07, evanm wrote:
> One more minor comment:
> 
> static_cast<int>(num_children - 1)
> 
> if num_children is zero this will overflow.  Maybe that can't happen, but it'd
> make me feel better if you did
> 
> static_cast<int>(num_children) - 1

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:51: XID topmost_window =
FindTopMostWindow(window);
On 2011/01/21 01:28:17, Daniel Erat wrote:
> This seems wrong.  It looks like it gets called recursively until you don't
find
> any child windows.  Ideally you'd just search with a depth of one -- in the
> common case, the window manager is a reparenting one, so there will be one
level
> of windows between the root and the client windows.  In the less-common case,
> the client windows will be direct children of the root.
> 
> Why not use gtk_util::EnumerateTopLevelWindows() from
browser/ui/gtk/gtk_util.h,
> which knows how to use _NET_CLIENT_LIST_STACKING if the WM supports it (most
do)
> to find the top client window without having to walk the tree?

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:59: bool DoFullScreenCheck() {
On 2011/01/21 01:28:17, Daniel Erat wrote:
> Please give this function a more-descriptive name; "FullScreenCheck" doesn't
> mean anything to me.  Something like "IsTopmostWindowFullscreen()" seems like
it
> might be more accurate.

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:78: GdkWindow* gwindow =
gdk_window_lookup(topmost_window);
On 2011/01/21 01:28:17, Daniel Erat wrote:
> The docs for gdk_window_lookup() are vague: will it return a GdkWindow for
every
> X window, or only for X windows opened by GDK but possibly by other clients (I
> don't know how it could check this unless GDK sets a property on every window
it
> opens), or only for X windows opened by this client?

I am not sure either. But I think it would be nice to check for this if we can
get a GDK window. Or I can remove this if X11 check is enough.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:88: bool IsX11WindowFullScreen(XID window) {
On 2011/01/21 01:28:17, Daniel Erat wrote:
> If you want to call this from other places (it looks like you do), it should
> probably go in ui/base/x/x11_util.h instead.

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:90: Atom atom =
gdk_x11_get_xatom_by_name_for_display(
On 2011/01/21 01:28:17, Daniel Erat wrote:
> It'd be good to cache this so you don't need to do a round trip to look it up
> each time this is called.

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_linux...
chrome/browser/fullscreen_linux.cc:95: "_NET_WM_STATE",
On 2011/01/21 01:28:17, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.mm
File chrome/browser/fullscreen_mac.mm (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:7: #import "base/logging.h"
On 2011/01/21 19:34:44, dmac wrote:
> base/logging should be under system imports.

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:17: - (BOOL) isFullScreen;
On 2011/01/21 19:34:44, dmac wrote:
> no space between ) and i (same on line below)

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:18: - (void)
setIsFullScreen:(BOOL)newIsFullScreen;
On 2011/01/21 19:34:44, dmac wrote:
> cocoa naming would be isFullScreen and setFullScreen

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:34: /*outActualType*/ NULL,
On 2011/01/21 19:34:44, dmac wrote:
> get rid of /*value*/ and do you want to check for an error on this?

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:47: // Check if the user is in presentation
mode initially. We do this by
On 2011/01/21 19:34:44, dmac wrote:
> When this is initialized, do we expect Chrome to be in the background? If we
are
> in the foreground could we not use
> 
> GetSystemUIMode?

No. InitFullScreenMonitor is called when NotificationUIManager is initialized
which happens in UI thread.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/fullscreen_mac.m...
chrome/browser/fullscreen_mac.mm:98: g_fullScreenMonitor = nil;
On 2011/01/21 19:34:44, dmac wrote:
> [g_fullScreenMonitor release]; ?

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/idle_mac.mm
File chrome/browser/idle_mac.mm (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/idle_mac.mm#newc...
chrome/browser/idle_mac.mm:53: isScreensaverRunning_ = YES;
On 2011/01/21 19:34:44, dmac wrote:
> cocoa conventions would not have the "is" in front of var names.

Done.

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/6359008/diff/1/chrome/browser/notifications/no...
chrome/browser/notifications/notification_ui_manager.cc:156:
ShowNotifications();
On 2011/01/21 07:14:51, John Gregg wrote:
> This doesn't look right at first glance because  CheckAndShowNotifications()
is
> already calling ShowNotifications().  Can you add a comment that this is
> detecting (with the a timer) the first time that the user becomes active?  

Done.

http://codereview.chromium.org/6359008/diff/1/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/6359008/diff/1/ui/base/x/x11_util.h#newcode81
ui/base/x/x11_util.h:81: bool ExistsProperty(XID window, const std::string&
property_name);
On 2011/01/21 01:28:17, Daniel Erat wrote:
> nit: "PropertyExists"

Done.

[levin, 2011-01-21 20:41:08]

LGTM

Some trivial things to consider below.

http://codereview.chromium.org/6359008/diff/22001/chrome/browser/fullscreen_w...
File chrome/browser/fullscreen_win.cc (right):

http://codereview.chromium.org/6359008/diff/22001/chrome/browser/fullscreen_w...
chrome/browser/fullscreen_win.cc:14: if
(FAILED(SHQueryUserNotificationState(&state)))
Seems like you'd want to put a :: here since you put it in for all other windows
api calls.

http://codereview.chromium.org/6359008/diff/22001/chrome/browser/fullscreen_w...
chrome/browser/fullscreen_win.cc:30: RECT wnd_rc;
Whenever I see "rc", I think resource compiler.

rect would be better.

http://codereview.chromium.org/6359008/diff/22001/chrome/browser/fullscreen_w...
chrome/browser/fullscreen_win.cc:83: return false;
Or just

  return IsPlatformFullScreenMode() || IsFullScreenWindowMode() ||
IsFullScreenConsoleMode();

Whatever you find more readable. (I like the concise version but it is
debateable so either way.)

[jianli, 2011-01-22 02:17:00]

I did some more changes on Linux. Daniel, could you please take a look at my
Linux related changes.

gtk_util::EnumerateTopLevelWindows does not always work to get the desired
top-most window for full-screen check, like Firefox or Movie Player window. This
is because ui::EnumerateChildren checks if the window is named or not, which
seems to cause problem. I do not want to change the current behavior to mess
other caller of this function. So I add my own implementation of
EnumerateAllChildWindows and put a TODO there for fixing the problem later.

In addition, I fixed a couple explicit problems I found in EnumerateChildren and
GetXWindowStack (x11_util.cc).
1) Change to use std::vector<>, instead of std::set<> in EnumerateChildren since
std::set<> does not keep the elements in the insertion order.
2) Change to use push_back in GetXWindowsStack to improve the perf.

[Daniel Erat, 2011-01-24 19:54:57]

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/fullscreen_l...
File chrome/browser/fullscreen_linux.cc (right):

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/fullscreen_l...
chrome/browser/fullscreen_linux.cc:98: int index;
doesn't look like you're using this

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/fullscreen_l...
chrome/browser/fullscreen_linux.cc:112: static Atom atom =
gdk_x11_get_xatom_by_name_for_display(
nit: rename to something like 'desktop_atom'

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.cc
File chrome/browser/idle_linux.cc (right):

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:35: bool isScreensaverWindow(XID window) const {
should be "IsScreensaverWindow"

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:36: // It should occupy full screen.
s/occupy full/occupy the full/

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:45: // should contains "screensaver".
s/contains/contain/

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:48: size_t pos = 0, len = value.length();
I don't understand why you're stepping through the string here.  A single call
to value.find("screensaver") be sufficient if you just want to see if
"screensaver" occurs somewhere in |value|.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:53: pos += str.length() + 1;
Additionally, isn't this wrong?  str.length() should be equal to value.length()
(additional nit: I think that size() is the more common name), so I think you'd
only ever go through the loop a single time.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:65: bool ExistsScreensaverWindow() {
nit: rename to ScreensaverWindowExists() (but since this function is tiny and
only appears to get called in one place, I'd probably just move it into
IsScreensaverRunning())

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:81: // Usually screensaver is used to lock the
screen. So we do not need to check
minor grammatical nits:

s/Usually screensaver/Usually the screensaver/
s/screen\. So/screen, so/

[jianli, 2011-01-24 23:36:26]

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/fullscreen_l...
File chrome/browser/fullscreen_linux.cc (right):

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/fullscreen_l...
chrome/browser/fullscreen_linux.cc:98: int index;
On 2011/01/24 19:54:57, Daniel Erat wrote:
> doesn't look like you're using this

Leftover from debugging. Removed.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/fullscreen_l...
chrome/browser/fullscreen_linux.cc:112: static Atom atom =
gdk_x11_get_xatom_by_name_for_display(
On 2011/01/24 19:54:57, Daniel Erat wrote:
> nit: rename to something like 'desktop_atom'

Done.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.cc
File chrome/browser/idle_linux.cc (right):

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:35: bool isScreensaverWindow(XID window) const {
On 2011/01/24 19:54:57, Daniel Erat wrote:
> should be "IsScreensaverWindow"

Done.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:36: // It should occupy full screen.
On 2011/01/24 19:54:57, Daniel Erat wrote:
> s/occupy full/occupy the full/

Done.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:45: // should contains "screensaver".
On 2011/01/24 19:54:57, Daniel Erat wrote:
> s/contains/contain/

Done.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:48: size_t pos = 0, len = value.length();
On 2011/01/24 19:54:57, Daniel Erat wrote:
> I don't understand why you're stepping through the string here.  A single call
> to value.find("screensaver") be sufficient if you just want to see if
> "screensaver" occurs somewhere in |value|.

WM_CLASS property (per http://tronche.com/gui/x/icccm/sec-4.html#WM_CLASS)
contains two consecutive null-terminated strings. Previously I thought
std::string:find() is not able to find the pattern in the 2nd C-string. But it
turned out it worked fine. So I changed to call value.find() as you suggested.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:53: pos += str.length() + 1;
On 2011/01/24 19:54:57, Daniel Erat wrote:
> Additionally, isn't this wrong?  str.length() should be equal to
value.length()
> (additional nit: I think that size() is the more common name), so I think
you'd
> only ever go through the loop a single time.

Removed. See last comment.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:65: bool ExistsScreensaverWindow() {
On 2011/01/24 19:54:57, Daniel Erat wrote:
> nit: rename to ScreensaverWindowExists() (but since this function is tiny and
> only appears to get called in one place, I'd probably just move it into
> IsScreensaverRunning())

Done.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:81: // Usually screensaver is used to lock the
screen. So we do not need to check
On 2011/01/24 19:54:57, Daniel Erat wrote:
> minor grammatical nits:
> 
> s/Usually screensaver/Usually the screensaver/
> s/screen\. So/screen, so/

Done.

[Daniel Erat, 2011-01-25 00:34:32]

Thanks, looks okay to me for Linux.

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.cc
File chrome/browser/idle_linux.cc (right):

http://codereview.chromium.org/6359008/diff/34001/chrome/browser/idle_linux.c...
chrome/browser/idle_linux.cc:65: bool ExistsScreensaverWindow() {
On 2011/01/24 23:36:26, jianli wrote:
> On 2011/01/24 19:54:57, Daniel Erat wrote:
> > nit: rename to ScreensaverWindowExists() (but since this function is tiny
and
> > only appears to get called in one place, I'd probably just move it into
> > IsScreensaverRunning())
> 
> Done.

Is it?  I still see the old name.

[jianli, 2011-01-25 00:36:27]

On Mon, Jan 24, 2011 at 4:34 PM, <derat@chromium.org> wrote:

> IsScreensaverRunning())


It is removed in latest patch (patch 4). Please check it.

[Daniel Erat, 2011-01-25 00:58:01]

On Mon, Jan 24, 2011 at 4:36 PM, Jian Li <jianli@chromium.org> wrote:
> On Mon, Jan 24, 2011 at 4:34 PM, <derat@chromium.org> wrote:
>> IsScreensaverRunning())
>
> It is removed in latest patch (patch 4). Please check it.

That particular comment was about either renaming
ExistsScreensaverWindow() to ScreensaverWindowExists()  or moving it
into IsScreensaverRunning().  In the latest patch set (6), I see
IsScreensaverRunning() as being moved into ExistsScreensaverWindow()
(which is fine), but it's still named ExistsScreensaverWindow()
instead of ScreensaverWindowExists().

[jianli, 2011-01-25 01:01:44]

Done. Please check the latest patch. Thanks.


On Mon, Jan 24, 2011 at 4:57 PM, Daniel Erat <derat@chromium.org> wrote:

> On Mon, Jan 24, 2011 at 4:36 PM, Jian Li <jianli@chromium.org> wrote:
> > On Mon, Jan 24, 2011 at 4:34 PM, <derat@chromium.org> wrote:
> >> IsScreensaverRunning())
> >
> > It is removed in latest patch (patch 4). Please check it.
>
> That particular comment was about either renaming
> ExistsScreensaverWindow() to ScreensaverWindowExists()  or moving it
> into IsScreensaverRunning().  In the latest patch set (6), I see
> IsScreensaverRunning() as being moved into ExistsScreensaverWindow()
> (which is fine), but it's still named ExistsScreensaverWindow()
> instead of ScreensaverWindowExists().
>

[Daniel Erat, 2011-01-25 01:20:21]

Thanks.

On Mon, Jan 24, 2011 at 5:01 PM, Jian Li <jianli@chromium.org> wrote:
> Done. Please check the latest patch. Thanks.
>
> On Mon, Jan 24, 2011 at 4:57 PM, Daniel Erat <derat@chromium.org> wrote:
>>
>> On Mon, Jan 24, 2011 at 4:36 PM, Jian Li <jianli@chromium.org> wrote:
>> > On Mon, Jan 24, 2011 at 4:34 PM, <derat@chromium.org> wrote:
>> >> IsScreensaverRunning())
>> >
>> > It is removed in latest patch (patch 4). Please check it.
>>
>> That particular comment was about either renaming
>> ExistsScreensaverWindow() to ScreensaverWindowExists()  or moving it
>> into IsScreensaverRunning().  In the latest patch set (6), I see
>> IsScreensaverRunning() as being moved into ExistsScreensaverWindow()
>> (which is fine), but it's still named ExistsScreensaverWindow()
>> instead of ScreensaverWindowExists().
>
>

[dmac, 2011-01-25 05:04:37]

Couple more comments. The conversion to properties is optional, but would be
nice. Sorry I'm a little late reviewing.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
File chrome/browser/fullscreen_mac.mm (right):

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
chrome/browser/fullscreen_mac.mm:19: - (void)setFullScreen:(BOOL)newFullScreen;
You could also do this with properties

@property (nonatomic, getter=isFullScreen) BOOL fullScreen;

and then in the implementation
@synthesize fullScreen = fullScreen_;

and get rid of the setter/getter interfaces and impl.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
chrome/browser/fullscreen_mac.mm:66: (void) status;  // Work around unused
variable warning.
possibly a better option here would be
if (status) {
  [self release];
  self = nil;
}

Then even if it fails, IsFullScreenMode will also return no because [nil
isFullScreen] will return 0.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
chrome/browser/fullscreen_mac.mm:87: FullScreenMonitor* g_fullScreenMonitor =
nil;
should this be static? or in an anonymous namespace?

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm
File chrome/browser/idle_mac.mm (left):

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm#...
chrome/browser/idle_mac.mm:10: unsigned int idle_time =
CGEventSourceSecondsSinceLastEventType(
you are changing a double (CFTimeInterval) into an unsigned int. Perhaps you
should static_cast it if this is what you really want to do?

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm
File chrome/browser/idle_mac.mm (right):

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm#...
chrome/browser/idle_mac.mm:16: - (BOOL)isScreensaverRunning;
You could also do this with properties

@property (readonly, nonatomic, getter=isScreensaverRunning) BOOL
screenSaverRunning;
@property (readonly, nonatomic, getter=isScreenLocked) BOOL screenLocked;

and then in the implementation
@synthesize screenSaverRunning = screenSaverRunning_;
@synthesize screenLocked = screenLocked_;


and get rid of the getter interfaces and impl.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm#...
chrome/browser/idle_mac.mm:78: MacScreenMonitor* g_screenMonitor = nil;
should this be static or in an anonymous namespace?

[John Gregg, 2011-01-25 17:41:05]

notification_ui_manager.* parts LGTM

[jianli, 2011-01-25 19:30:15]

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
File chrome/browser/fullscreen_mac.mm (right):

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
chrome/browser/fullscreen_mac.mm:19: - (void)setFullScreen:(BOOL)newFullScreen;
On 2011/01/25 05:04:37, dmac wrote:
> You could also do this with properties
> 
> @property (nonatomic, getter=isFullScreen) BOOL fullScreen;
> 
> and then in the implementation
> @synthesize fullScreen = fullScreen_;
> 
> and get rid of the setter/getter interfaces and impl.

Done.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
chrome/browser/fullscreen_mac.mm:66: (void) status;  // Work around unused
variable warning.
On 2011/01/25 05:04:37, dmac wrote:
> possibly a better option here would be
> if (status) {
>   [self release];
>   self = nil;
> }
> 
> Then even if it fails, IsFullScreenMode will also return no because [nil
> isFullScreen] will return 0.

Done.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/fullscreen_m...
chrome/browser/fullscreen_mac.mm:87: FullScreenMonitor* g_fullScreenMonitor =
nil;
On 2011/01/25 05:04:37, dmac wrote:
> should this be static? or in an anonymous namespace?

I forgot to put static back when I moved the code around.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm
File chrome/browser/idle_mac.mm (left):

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm#...
chrome/browser/idle_mac.mm:10: unsigned int idle_time =
CGEventSourceSecondsSinceLastEventType(
On 2011/01/25 05:04:37, dmac wrote:
> you are changing a double (CFTimeInterval) into an unsigned int. Perhaps you
> should static_cast it if this is what you really want to do?

This is from old code written by other people. Thanks for finding the potential
problem here. Changed to use CFTimeInterval.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm
File chrome/browser/idle_mac.mm (right):

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm#...
chrome/browser/idle_mac.mm:16: - (BOOL)isScreensaverRunning;
On 2011/01/25 05:04:37, dmac wrote:
> You could also do this with properties
> 
> @property (readonly, nonatomic, getter=isScreensaverRunning) BOOL
> screenSaverRunning;
> @property (readonly, nonatomic, getter=isScreenLocked) BOOL screenLocked;
> 
> and then in the implementation
> @synthesize screenSaverRunning = screenSaverRunning_;
> @synthesize screenLocked = screenLocked_;
> 
> 
> and get rid of the getter interfaces and impl.

Done.

http://codereview.chromium.org/6359008/diff/43003/chrome/browser/idle_mac.mm#...
chrome/browser/idle_mac.mm:78: MacScreenMonitor* g_screenMonitor = nil;
On 2011/01/25 05:04:37, dmac wrote:
> should this be static or in an anonymous namespace?

Done.

[dmac, 2011-01-25 19:34:04]

Mac stuff LGTM.