Preliminary fullscreen mode support....

Preliminary fullscreen mode support.

There is no UI for this (yet), just F11 to toggle in and out.

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

[sky, 2009-02-17 18:28:51]

http://codereview.chromium.org/20423/diff/1/4
File chrome/app/chrome_dll.rc (right):

http://codereview.chromium.org/20423/diff/1/4#newcode54
Line 54: VK_F11,         IDC_FULLSCREEN,             VIRTKEY
Shouldn't that be FULL_SCREEN? I searched and seems like there is a mix of both.

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

http://codereview.chromium.org/20423/diff/1/6#newcode2098
Line 2098: command_updater_.UpdateCommandEnabled(IDC_FOCUS_SEARCH,
show_main_ui);
Is the plan to allow you to enable these somehow? It would be seriously lame if
you couldn't find in full screem mode.

http://codereview.chromium.org/20423/diff/1/6#newcode2116
Line 2116: }
Bookmarking the page shows a bubble relative to the star on the location bar.
How does this look with your patch when the star isn't really visible?

http://codereview.chromium.org/20423/diff/1/9
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/20423/diff/1/9#newcode184
Line 184: fullscreen_(false),
should you initialize style_ and ex_style_ too?

http://codereview.chromium.org/20423/diff/1/10
File chrome/browser/views/frame/browser_view.h (right):

http://codereview.chromium.org/20423/diff/1/10#newcode385
Line 385: gfx::Rect window_rect_;
Your call here, but it would be slightly clearer if this was wrapped in a
structure whose name made it clear of the intent of this information.

[Peter Kasting, 2009-02-18 17:17:44]

http://codereview.chromium.org/20423/diff/1/4
File chrome/app/chrome_dll.rc (right):

http://codereview.chromium.org/20423/diff/1/4#newcode54
Line 54: VK_F11,         IDC_FULLSCREEN,             VIRTKEY
On 2009/02/17 18:28:51, sky wrote:
> Shouldn't that be FULL_SCREEN? I searched and seems like there is a mix of
both.

No, because this is "Fullscreen Mode", not "Full Screen Mode".

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

http://codereview.chromium.org/20423/diff/1/6#newcode2098
Line 2098: command_updater_.UpdateCommandEnabled(IDC_FOCUS_SEARCH,
show_main_ui);
On 2009/02/17 18:28:51, sky wrote:
> Is the plan to allow you to enable these somehow? It would be seriously lame
if
> you couldn't find in full screem mode.

I think you misinterpreted what these commands are.  These are things like
ctrl-l and ctrl-k, which put the caret in the location bar.  Find (ctrl-f) works
fine in fullscreen mode.

http://codereview.chromium.org/20423/diff/1/6#newcode2116
Line 2116: }
On 2009/02/17 18:28:51, sky wrote:
> Bookmarking the page shows a bubble relative to the star on the location bar.
> How does this look with your patch when the star isn't really visible?

Bad.  I need to deal with this.  Also when you toggle fullscreen on/off the
bubble doesn't reposition.

I'm going to punt this issue to a followup patch and just write it on my list of
problems for now.

http://codereview.chromium.org/20423/diff/1/9
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/20423/diff/1/9#newcode184
Line 184: fullscreen_(false),
On 2009/02/17 18:28:51, sky wrote:
> should you initialize style_ and ex_style_ too?

It didn't seem like I needed to because I never read them unless they've already
been set.  Now that they're in a struct I'm not sure whether it's moot... do
structs get a default constructor the inits them to 0?  Maybe not... anyway,
this doesn't seem important, yell if you disagree.

http://codereview.chromium.org/20423/diff/1/10
File chrome/browser/views/frame/browser_view.h (right):

http://codereview.chromium.org/20423/diff/1/10#newcode385
Line 385: gfx::Rect window_rect_;
On 2009/02/17 18:28:51, sky wrote:
> Your call here, but it would be slightly clearer if this was wrapped in a
> structure whose name made it clear of the intent of this information.

I agree.  Done.

[sky, 2009-02-18 17:47:56]

> No, because this is "Fullscreen Mode", not "Full Screen Mode".

Oh, that makes it so much clearer:)

LGTM