Make panels not show on top when there is an app running in full screen mode.

Make panels not show on top when there is an app running in full screen mode.

BUG=102731
TEST=Manual steps in the bug report.

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

[prasadt, 2011-11-30 00:47:16]

This is not fully implemented/tested yet but wanted to get feedback on shape of
the code.

jianli - Please review shape of the code. I just have stubs for windows that
need to be filled.

dimich - Please review mac changes.

[jennb, 2011-11-30 01:03:30]

Is there no other way besides polling to tell if fullscreen mode has changed?

[dcheng, 2011-11-30 01:07:27]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
Can we use an enum instead of a bool?

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:157: if (num_panels() == 1) {
Use panels_.size() -- it makes the connection more obvious--I have to read the
.h to know that num_panels() is actually panels_.size().

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:225: if (num_panels() == 0)
Use panels_.empty().

[Dmitry Titov, 2011-11-30 01:35:52]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
Naming: It is unclear if this is about the panel being full-screen or something
else being full screen... Maybe DodgeFullScreenApp? :-) Or
YieldToFullscreenWindow?

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
On 2011/11/30 01:07:28, dcheng wrote:
> Can we use an enum instead of a bool?

I think it's ok here. It is mandatory only if the callsites have to pass in
literal bool values. in this case, it is not so.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel.h
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:68: void FullScreenModeChanged(bool
is_full_screen_mode_on);
btw, here naming is ok.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:190:
panels_[i]->FullScreenModeChanged(is_full_screen_mode_on_);
if you do panel->native_panel()->FooBar() here, you don't need a re-translating
method on Panel.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:94: // Overridden from
PanelMouseWatcher::Observer:
Isn't it now PanelMouseWatcherObserver?

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:250: static const int
kFullScreenModeCheckInterval = 1000;
this is implementation detail, does not have to be in .h file. It is tricky to
have initialized statics in .h file, not recommended in fact. There was a thread
on chromium-dev about it recently.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:572: if
(isFullScreenModeOn) {
'{' is not needed (and not balanced here).
Since only a value to set is different, I'd prefer this form:
[window setLevel:(isFullScreenModeOn ? one : another)];

[prasadt, 2011-11-30 01:44:44]

On 2011/11/30 01:03:30, jennb wrote:
> Is there no other way besides polling to tell if fullscreen mode has changed?

Not that I know of. Notifications code also uses polling currently. In its timer
method, notifications code is also looking for screen locked state, so I didn't
try to combine them.

[jianli, 2011-11-30 01:53:15]

I think we can at least try to avoid using the timer to know when the
full-screen kicks in. On Windows, there is a notification for this (see
http://social.msdn.microsoft.com/Forums/en/windowsuidevelopment/thread/18cb3f...).

When full-screen mode is on, it is probably OK to use a timer to find out when
the fullscreen mode ends and this is what desktop notification code is doing.
For desktop notifications, it is OK to keep top-most balloons intact when the
fullscreen mode is on and we assume that the user will take action for them. But
for panels, we need to handle this case.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:65: // Turn off always on top for
the panel.
Can you update the comment and all others to indicate that we need to hide the
panels instead?

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:250: static const int
kFullScreenModeCheckInterval = 1000;
Can this constant be moved to .cc file?

[jennb, 2011-11-30 02:06:27]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:157: if (num_panels() == 1) {
On 2011/11/30 01:07:28, dcheng wrote:
> Use panels_.size() -- it makes the connection more obvious--I have to read the
> .h to know that num_panels() is actually panels_.size().

panels_ is going away. use num_panels().

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:225: if (num_panels() == 0)
On 2011/11/30 01:07:28, dcheng wrote:
> Use panels_.empty().

Ditto.

[prasadt, 2011-11-30 02:38:47]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
On 2011/11/30 01:07:28, dcheng wrote:
> Can we use an enum instead of a bool?

I initially started with an enum but bool felt more appropriate here as its just
on/off.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> Naming: It is unclear if this is about the panel being full-screen or
something
> else being full screen... Maybe DodgeFullScreenApp? :-) Or
> YieldToFullscreenWindow?

The function does both dodging and undodging though. So neither suggested
function name would work.  CurrentActiveAppFullScreenModeChanged? I'm open for
suggestions.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> On 2011/11/30 01:07:28, dcheng wrote:
> > Can we use an enum instead of a bool?
> 
> I think it's ok here. It is mandatory only if the callsites have to pass in
> literal bool values. in this case, it is not so. 

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel.h
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:68: void FullScreenModeChanged(bool
is_full_screen_mode_on);
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> btw, here naming is ok.

ok

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:65: // Turn off always on top for
the panel.
On 2011/11/30 01:53:15, jianli wrote:
> Can you update the comment and all others to indicate that we need to hide the
> panels instead?

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:157: if (num_panels() == 1) {
On 2011/11/30 02:06:27, jennb wrote:
> On 2011/11/30 01:07:28, dcheng wrote:
> > Use panels_.size() -- it makes the connection more obvious--I have to read
the
> > .h to know that num_panels() is actually panels_.size().
> 
> panels_ is going away. use num_panels().

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:157: if (num_panels() == 1) {
On 2011/11/30 01:07:28, dcheng wrote:
> Use panels_.size() -- it makes the connection more obvious--I have to read the
> .h to know that num_panels() is actually panels_.size().

See Jenn's comment.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:190:
panels_[i]->FullScreenModeChanged(is_full_screen_mode_on_);
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> if you do panel->native_panel()->FooBar() here, you don't need a
re-translating
> method on Panel.

Currently PanelManager does not access native_panel directly. Is there value in
keeping that separation?

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:225: if (num_panels() == 0)
On 2011/11/30 02:06:27, jennb wrote:
> On 2011/11/30 01:07:28, dcheng wrote:
> > Use panels_.empty().
> 
> Ditto.

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:225: if (num_panels() == 0)
On 2011/11/30 01:07:28, dcheng wrote:
> Use panels_.empty().

See Jenn's comment.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:94: // Overridden from
PanelMouseWatcher::Observer:
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> Isn't it now PanelMouseWatcherObserver?

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:250: static const int
kFullScreenModeCheckInterval = 1000;
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> this is implementation detail, does not have to be in .h file. It is tricky to
> have initialized statics in .h file, not recommended in fact. There was a
thread
> on chromium-dev about it recently.

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:250: static const int
kFullScreenModeCheckInterval = 1000;
On 2011/11/30 01:53:15, jianli wrote:
> Can this constant be moved to .cc file?

Done.

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:572: if
(isFullScreenModeOn) {
On 2011/11/30 01:35:53, Dmitry Titov wrote:
> '{' is not needed (and not balanced here).
> Since only a value to set is different, I'd prefer this form:
> [window setLevel:(isFullScreenModeOn ? one : another)];

Done. And oops :-) I haven't yet compiled on mac.

[Dmitry Titov, 2011-12-01 04:04:26]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:190:
panels_[i]->FullScreenModeChanged(is_full_screen_mode_on_);
On 2011/11/30 02:38:47, prasadt wrote:
> On 2011/11/30 01:35:53, Dmitry Titov wrote:
> > if you do panel->native_panel()->FooBar() here, you don't need a
> re-translating
> > method on Panel.
> 
> Currently PanelManager does not access native_panel directly. Is there value
in
> keeping that separation?

What would be the reason for such separation? NativePanel is an abstract
interface specifically introduced to insulate the caller from platform-dependent
implementation and extend BrowserWindow which is not always enough for our
purposes. Interface itself is very platform-independent so I don't see any
reason to avoid using it in PanelManager.

There is a reason to keep it separate from Panel and BrowserWindow, but that's
an orthogonal story (mostly because it is implemented separately, esp on Mac).

Don't see any reason to add more translating methods.

[prasadt, 2011-12-01 04:20:47]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:190:
panels_[i]->FullScreenModeChanged(is_full_screen_mode_on_);
On 2011/12/01 04:04:26, Dmitry Titov wrote:
> On 2011/11/30 02:38:47, prasadt wrote:
> > On 2011/11/30 01:35:53, Dmitry Titov wrote:
> > > if you do panel->native_panel()->FooBar() here, you don't need a
> > re-translating
> > > method on Panel.
> > 
> > Currently PanelManager does not access native_panel directly. Is there value
> in
> > keeping that separation?
> 
> What would be the reason for such separation? NativePanel is an abstract
> interface specifically introduced to insulate the caller from
platform-dependent
> implementation and extend BrowserWindow which is not always enough for our
> purposes. Interface itself is very platform-independent so I don't see any
> reason to avoid using it in PanelManager.
> 
> There is a reason to keep it separate from Panel and BrowserWindow, but that's
> an orthogonal story (mostly because it is implemented separately, esp on Mac).
> 
> Don't see any reason to add more translating methods.

Done.

NativePanel interface is currently protected and Panel/PanelBrowserTest are
friends. Made the interface public so I can use it from PanelStrip and removed
friends. It seems an odd pattern to make it protected and then add friends.

[jennb, 2011-12-01 20:10:33]

Drive by

http://codereview.chromium.org/8686012/diff/15001/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/15001/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:31: public:
Why not keep protected and have all panel strips go through Panel interface for
the full screen mode change?

http://codereview.chromium.org/8686012/diff/15001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/15001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.cc:115:
panel_strip_->OnFullScreenModeChanged(is_full_screen_mode_on_);
Could you add // TODO(jianli) overflow_strip_->OnFullScreenModeChanged() so he
knows to update this method when adding his overflow strip? Thx.

[Dmitry Titov, 2011-12-01 20:14:45]

http://codereview.chromium.org/8686012/diff/15001/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/15001/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:31: public:
On 2011/12/01 20:10:33, jennb wrote:
> Why not keep protected and have all panel strips go through Panel interface
for
> the full screen mode change?

I answered this above. It creates a lot of 'retranslator' methods on Panel w/o
any benefit of encapsulation, actually.

[jennb, 2011-12-01 21:22:09]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:190:
panels_[i]->FullScreenModeChanged(is_full_screen_mode_on_);
On 2011/12/01 04:04:26, Dmitry Titov wrote:
> On 2011/11/30 02:38:47, prasadt wrote:
> > On 2011/11/30 01:35:53, Dmitry Titov wrote:
> > > if you do panel->native_panel()->FooBar() here, you don't need a
> > re-translating
> > > method on Panel.
> > 
> > Currently PanelManager does not access native_panel directly. Is there value
> in
> > keeping that separation?
> 
> What would be the reason for such separation? NativePanel is an abstract
> interface specifically introduced to insulate the caller from
platform-dependent
> implementation and extend BrowserWindow which is not always enough for our
> purposes. Interface itself is very platform-independent so I don't see any
> reason to avoid using it in PanelManager.
> 
> There is a reason to keep it separate from Panel and BrowserWindow, but that's
> an orthogonal story (mostly because it is implemented separately, esp on Mac).
> 
> Don't see any reason to add more translating methods.

Another reason for the separation is that the Panel often performs some
platform-independent logic first, then calls the NativePanel layer to do some
platform-dependent logic. Without the separation, the caller has to know when
it's safe to go straight to the platform-dependent layer.

[prasadt, 2011-12-01 21:35:16]

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:190:
panels_[i]->FullScreenModeChanged(is_full_screen_mode_on_);
On 2011/12/01 21:22:09, jennb wrote:
> On 2011/12/01 04:04:26, Dmitry Titov wrote:
> > On 2011/11/30 02:38:47, prasadt wrote:
> > > On 2011/11/30 01:35:53, Dmitry Titov wrote:
> > > > if you do panel->native_panel()->FooBar() here, you don't need a
> > > re-translating
> > > > method on Panel.
> > > 
> > > Currently PanelManager does not access native_panel directly. Is there
value
> > in
> > > keeping that separation?
> > 
> > What would be the reason for such separation? NativePanel is an abstract
> > interface specifically introduced to insulate the caller from
> platform-dependent
> > implementation and extend BrowserWindow which is not always enough for our
> > purposes. Interface itself is very platform-independent so I don't see any
> > reason to avoid using it in PanelManager.
> > 
> > There is a reason to keep it separate from Panel and BrowserWindow, but
that's
> > an orthogonal story (mostly because it is implemented separately, esp on
Mac).
> > 
> > Don't see any reason to add more translating methods.
> 
> Another reason for the separation is that the Panel often performs some
> platform-independent logic first, then calls the NativePanel layer to do some
> platform-dependent logic. Without the separation, the caller has to know when
> it's safe to go straight to the platform-dependent layer.

Now that I think about it, this makes sense to me.

One way to think of this could be to Panel+NavitePanelImpl as one class that's
separated for implementation reasons. If we had a single platform to support,
this would be one class. An alternate but messy implementation would've been one
class with a bunch of #ifdefs.

[Dmitry Titov, 2011-12-01 21:47:06]

I agree the case when Panel does something platform-independent and then calls
NativePanel is a reason to have NativePanel protected and have translating
methods. I take my note back.

It feels weird in this particular case but I don't have a better idea.

[prasadt, 2011-12-01 22:49:29]

On 2011/12/01 21:47:06, Dmitry Titov wrote:
> I agree the case when Panel does something platform-independent and then calls
> NativePanel is a reason to have NativePanel protected and have translating
> methods. I take my note back.
> 
> It feels weird in this particular case but I don't have a better idea.

ok. Reverted the last upload to keep NativePanel interface private to Panel.

[prasadt, 2011-12-01 23:39:20]

On 2011/11/30 01:53:15, jianli wrote:
> I think we can at least try to avoid using the timer to know when the
> full-screen kicks in. On Windows, there is a notification for this (see
>
http://social.msdn.microsoft.com/Forums/en/windowsuidevelopment/thread/18cb3f...).

Opened bug for this - crbug.com/106125

> When full-screen mode is on, it is probably OK to use a timer to find out when
> the fullscreen mode ends and this is what desktop notification code is doing.
> For desktop notifications, it is OK to keep top-most balloons intact when the
> fullscreen mode is on and we assume that the user will take action for them.
But
> for panels, we need to handle this case.

The intent of the code is to handle this. Tested it with dimich's help on Mac.
Windows implementation just has stubs at the moment.

[prasadt, 2011-12-01 23:40:39]

Uploaded bug fix for where setting panel window to NSNormalWindowLevel vs
NSStatusWindowLevel was inverted.

[Dmitry Titov, 2011-12-01 23:53:08]

lgtm

BTW, there is an event for Mac as well. See implementation in fullscreen_mac.mm.
It is possible to listen to it for both going fullscreen and getting out of
fullscreen.

However, looking at fullscreen_win.cc, I am not sure it's possible to listen to
windows notificaitons about presentation mode and be accurate. It seems to
detect all kinds of full-screen situations, some sort of periodic check is
needed.

If this is not correct, we should fix it. I think it makes sense to file a code
cleanup bug on pri-3 to replace timer with event listening where possible.

[prasadt, 2011-12-02 00:00:36]

On 2011/12/01 23:53:08, Dmitry Titov wrote:
> lgtm
> 
> BTW, there is an event for Mac as well. See implementation in
fullscreen_mac.mm.
> It is possible to listen to it for both going fullscreen and getting out of
> fullscreen.
> 
> However, looking at fullscreen_win.cc, I am not sure it's possible to listen
to
> windows notificaitons about presentation mode and be accurate. It seems to
> detect all kinds of full-screen situations, some sort of periodic check is
> needed.
> 
> If this is not correct, we should fix it. I think it makes sense to file a
code
> cleanup bug on pri-3 to replace timer with event listening where possible.

crbug.com/106125 as mentioned in an earlier comment. Updated the bug with the
pointer to fullscreen_mac.mm.

[jianli, 2011-12-02 00:04:45]

LGTM w/ several nits.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
nit: is_full_screen_mode_on seems to be long. Can you shorten it to
is_full_screen?

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.cc:29: static const int
kFullScreenModeCheckInterval = 1000;
nit: either add comment to mention ms or add ms suffix to the variable name.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.h:104: bool is_full_screen_mode_on() 
const { return is_full_screen_mode_on_; }
nit: extra space.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.h:187: void CheckFullScreenMode();
nit: it'd better to move this declaration to the above method area. Also please
add an empty line between full_screen_mode_timer_ or is_full_screen_mode_on_.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:128: // See
Panel::FullScreenModeChanged
nit: end with period.

[prasadt, 2011-12-02 00:29:39]

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:61: virtual void
FullScreenModeChanged(bool is_full_screen_mode_on) = 0;
On 2011/12/02 00:04:45, jianli wrote:
> nit: is_full_screen_mode_on seems to be long. Can you shorten it to
> is_full_screen?

Done.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.cc:29: static const int
kFullScreenModeCheckInterval = 1000;
On 2011/12/02 00:04:45, jianli wrote:
> nit: either add comment to mention ms or add ms suffix to the variable name.

Done.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.h:104: bool is_full_screen_mode_on() 
const { return is_full_screen_mode_on_; }
On 2011/12/02 00:04:45, jianli wrote:
> nit: extra space.

Done.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.h:187: void CheckFullScreenMode();
On 2011/12/02 00:04:45, jianli wrote:
> nit: it'd better to move this declaration to the above method area. Also
please
> add an empty line between full_screen_mode_timer_ or is_full_screen_mode_on_.

Done.

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/8686012/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:128: // See
Panel::FullScreenModeChanged
On 2011/12/02 00:04:45, jianli wrote:
> nit: end with period.

Done.

[commit-bot: I haz the power, 2011-12-02 03:51:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prasadt@chromium.org/8686012/12005

[commit-bot: I haz the power, 2011-12-02 05:36:43]

Try job failure for 8686012-12005 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...