Fullscreen JS API implementation.

Fullscreen JS API implementation.


BUG=
TEST=


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

[koz (OOO until 15th September), 2011-07-27 02:12:18]

Hi all,

This is a proof of concept for implementing a the JS fullscreen API by making
use of the current fullscreen functionality in Chrome. I was hoping to get
feedback on whether or not this approach is suitable and whether this code is
roughly right.

My thinking is that any behavioural changes we need to make for the JS
fullscreen API can be implemented as new parameters to the function that
currently puts the browser into fullscreen (eg: disabling keystrokes, showing
extra UI, etc).

Thanks!
James

[Peter Kasting, 2011-07-27 17:58:44]

You assigned four reviewers.  Can you say what you wanted each to look at?

[koz (OOO until 15th September), 2011-07-27 18:46:41]

I ran this idea before Andrew before writing this patch and he suggested the
people I've added as reviewers as people who might have an opinion about how
the fullscreen API gets implemented.

My intent was to see if any of you objected to this approach before I
continue any further - the discussion of how to implement the feature before
I picked it up seemed to revolve around the idea of creating a new
fullscreen window, so I was wondering if there was something wrong with just
calling ToggleFullscreen().

As to the code itself, my only specific question is what would the
appropriate way to acquire a Browser* in ChromeContentBrowserClient be?
Could I get you to answer that, Peter?

Also, if any of you can suggest who would be suitable as a sole reviewer for
this feature going forward, that'd be much appreciated.
On Jul 28, 2011 3:58 AM, <pkasting@chromium.org> wrote:
> You assigned four reviewers. Can you say what you wanted each to look at?
>
> http://codereview.chromium.org/7461059/

[Peter Kasting, 2011-07-27 18:56:05]

On 2011/07/27 18:46:41, koz wrote:
> My intent was to see if any of you objected to this approach before I
> continue any further - the discussion of how to implement the feature before
> I picked it up seemed to revolve around the idea of creating a new
> fullscreen window, so I was wondering if there was something wrong with just
> calling ToggleFullscreen().

No opinion.  I'm not familiar with the JS API so one of the other reviewers like
Darin would probably be better at answering.

> As to the code itself, my only specific question is what would the
> appropriate way to acquire a Browser* in ChromeContentBrowserClient be?
> Could I get you to answer that, Peter?

You probably want to ask sky or ben as I'm not really sure about this.

[koz (OOO until 15th September), 2011-07-27 22:53:38]

On 2011/07/27 18:56:05, Peter Kasting wrote:
> On 2011/07/27 18:46:41, koz wrote:
> > My intent was to see if any of you objected to this approach before I
> > continue any further - the discussion of how to implement the feature before
> > I picked it up seemed to revolve around the idea of creating a new
> > fullscreen window, so I was wondering if there was something wrong with just
> > calling ToggleFullscreen().
> 
> No opinion.  I'm not familiar with the JS API so one of the other reviewers
like
> Darin would probably be better at answering.
> 
> > As to the code itself, my only specific question is what would the
> > appropriate way to acquire a Browser* in ChromeContentBrowserClient be?
> > Could I get you to answer that, Peter?
> 
> You probably want to ask sky or ben as I'm not really sure about this.

Cool, thanks for your comments! I'll add sky as a reviewer.

[koz (OOO until 15th September), 2011-07-27 22:55:54]

[-pkasting, +sky]

[sky, 2011-07-28 00:45:54]

http://codereview.chromium.org/7461059/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7461059/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:970:
content::GetContentClient()->browser()->EnterFullscreenMode();
ContentClient isn't intended for this sort of call.
Instead put enter/exit on RenderViewDelegate and have it route it to
TabContentsDelegate. Browser is the TabContentsDelegate for chrome code.

[koz (OOO until 15th September), 2011-07-28 03:20:46]

Thanks, sky!

http://codereview.chromium.org/7461059/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7461059/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:970:
content::GetContentClient()->browser()->EnterFullscreenMode();
On 2011/07/28 00:45:54, sky wrote:
> ContentClient isn't intended for this sort of call.
> Instead put enter/exit on RenderViewDelegate and have it route it to
> TabContentsDelegate. Browser is the TabContentsDelegate for chrome code.

Done.

[sky, 2011-07-28 03:53:28]

http://codereview.chromium.org/7461059/diff/8002/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7461059/diff/8002/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:504: void EnterFullscreenMode();
These should be with the other TabContentsDelegate overrides, virtual and with
OVERRIDE.

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
This description makes it sound as though you intended to make Browser select
the tab that invoked it. Your implementation in Browser doesn't select the tab
though. Did you want to?

http://codereview.chromium.org/7461059/diff/8002/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:685: virtual void
EnterFullscreenMode();
OVERRIDE on both of these.

[koz (OOO until 15th September), 2011-07-28 05:14:29]

http://codereview.chromium.org/7461059/diff/8002/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7461059/diff/8002/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:504: void EnterFullscreenMode();
On 2011/07/28 03:53:29, sky wrote:
> These should be with the other TabContentsDelegate overrides, virtual and with
> OVERRIDE.

Done.

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
On 2011/07/28 03:53:29, sky wrote:
> This description makes it sound as though you intended to make Browser select
> the tab that invoked it. Your implementation in Browser doesn't select the tab
> though. Did you want to?

I think so. What exactly does 'select the tab' mean in this context? This
function will be called by a JS API (webkitRequestFullScreen()) triggered by a
user gesture on a page and I want that page to be fullscreened. What changes
would I need to make to my implementation to achieve that?

http://codereview.chromium.org/7461059/diff/8002/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:685: virtual void
EnterFullscreenMode();
On 2011/07/28 03:53:29, sky wrote:
> OVERRIDE on both of these.

Done.

[sky, 2011-07-28 16:07:02]

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
On 2011/07/28 05:14:30, koz wrote:
> On 2011/07/28 03:53:29, sky wrote:
> > This description makes it sound as though you intended to make Browser
select
> > the tab that invoked it. Your implementation in Browser doesn't select the
tab
> > though. Did you want to?
> 
> I think so. What exactly does 'select the tab' mean in this context? This
> function will be called by a JS API (webkitRequestFullScreen()) triggered by a
> user gesture on a page and I want that page to be fullscreened. What changes
> would I need to make to my implementation to achieve that?

The tabstrip has a bunch of tabs, but only one active (the one whose content you
see). In Browser you can make a tab active by way of ActivateContents.

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:684: 
remove newline.

[scherkus (not reviewing), 2011-07-28 22:43:08]

I've got some nits on naming but don't wait on me to land this patch (it's
pretty much LG modulo nits and I'm going on vacation tomorrow)

spec-wise, there are some mentions of having animations which this won't
accomplish, but at the same time re-using completely functional (and stable!)
fullscreen code seems like A Good Thing

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3481: if (window_->IsFullscreen())
are these DCHECK worthy or plausible calls?

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3487: if (!window_->IsFullscreen())
ditto

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.cc:275: void
TabContentsDelegate::EnterFullscreenMode() {
NOTIMPLEMENTED ?

http://codereview.chromium.org/7461059/diff/14001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7461059/diff/14001/content/common/view_message...
content/common/view_messages.h:1964:
IPC_MESSAGE_ROUTED0(ViewHostMsg_EnterFullscreenForElement);
nit: the xxxForElement name is simply an artifact of the call from WebKit

I'd probably rename this EnterFullscreen since that's what the browser-side code
looks like (and browser-side code certainly knows nothing about elements)

http://codereview.chromium.org/7461059/diff/14001/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7461059/diff/14001/content/renderer/render_vie...
content/renderer/render_view.h:401: virtual void enterFullscreenForElement()
OVERRIDE;
nit: ditto w/ the naming, but unfortunately you'll need to change WebKit to
probably be just "enter/exitFullscreen"

...but that patch can wait :)

[sky, 2011-07-28 22:45:37]

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3481: if (window_->IsFullscreen())
On 2011/07/28 22:43:08, scherkus wrote:
> are these DCHECK worthy or plausible calls?

Since the user can always enter/exit fullscreen at their discretion it doesn't
seem like they should be DCHECKs.

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.cc:275: void
TabContentsDelegate::EnterFullscreenMode() {
On 2011/07/28 22:43:08, scherkus wrote:
> NOTIMPLEMENTED ?

See other empty implementations in this class. Folks only override this that
care to.

[koz (OOO until 15th September), 2011-07-29 04:12:57]

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
On 2011/07/28 16:07:02, sky wrote:
> On 2011/07/28 05:14:30, koz wrote:
> > On 2011/07/28 03:53:29, sky wrote:
> > > This description makes it sound as though you intended to make Browser
> select
> > > the tab that invoked it. Your implementation in Browser doesn't select the
> tab
> > > though. Did you want to?
> > 
> > I think so. What exactly does 'select the tab' mean in this context? This
> > function will be called by a JS API (webkitRequestFullScreen()) triggered by
a
> > user gesture on a page and I want that page to be fullscreened. What changes
> > would I need to make to my implementation to achieve that?
> 
> The tabstrip has a bunch of tabs, but only one active (the one whose content
you
> see). In Browser you can make a tab active by way of ActivateContents.

Is it necessary to call ActivateContents() if this call can only be triggered by
a user gesture? Does that not imply that the tab in question would already be
active?

I've added a call to ActivateContents() to TabContents before it delegates to
TabContentsDelegate (ie: Browser) to activate the tab that the JS call was made
from, is that ok?

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3481: if (window_->IsFullscreen())
On 2011/07/28 22:45:38, sky wrote:
> On 2011/07/28 22:43:08, scherkus wrote:
> > are these DCHECK worthy or plausible calls?
> 
> Since the user can always enter/exit fullscreen at their discretion it doesn't
> seem like they should be DCHECKs.

Right - there's nothing to stop a site calling webkitRequestFullScreen() on an
element when another element is fullscreened (though I don't know what the
intended behavoiur is in that case...)

http://codereview.chromium.org/7461059/diff/14001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3487: if (!window_->IsFullscreen())
On 2011/07/28 22:43:08, scherkus wrote:
> ditto

As above.

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7461059/diff/14001/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:684: 
On 2011/07/28 16:07:02, sky wrote:
> remove newline.

Done.

http://codereview.chromium.org/7461059/diff/14001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7461059/diff/14001/content/common/view_message...
content/common/view_messages.h:1964:
IPC_MESSAGE_ROUTED0(ViewHostMsg_EnterFullscreenForElement);
On 2011/07/28 22:43:08, scherkus wrote:
> nit: the xxxForElement name is simply an artifact of the call from WebKit
> 
> I'd probably rename this EnterFullscreen since that's what the browser-side
code
> looks like (and browser-side code certainly knows nothing about elements)

Done.

http://codereview.chromium.org/7461059/diff/14001/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7461059/diff/14001/content/renderer/render_vie...
content/renderer/render_view.h:401: virtual void enterFullscreenForElement()
OVERRIDE;
On 2011/07/28 22:43:08, scherkus wrote:
> nit: ditto w/ the naming, but unfortunately you'll need to change WebKit to
> probably be just "enter/exitFullscreen"
> 
> ...but that patch can wait :)

Yep, sounds good :-)

[sky, 2011-07-29 15:51:46]

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
On 2011/07/29 04:12:57, koz wrote:
> On 2011/07/28 16:07:02, sky wrote:
> > On 2011/07/28 05:14:30, koz wrote:
> > > On 2011/07/28 03:53:29, sky wrote:
> > > > This description makes it sound as though you intended to make Browser
> > select
> > > > the tab that invoked it. Your implementation in Browser doesn't select
the
> > tab
> > > > though. Did you want to?
> > > 
> > > I think so. What exactly does 'select the tab' mean in this context? This
> > > function will be called by a JS API (webkitRequestFullScreen()) triggered
by
> a
> > > user gesture on a page and I want that page to be fullscreened. What
changes
> > > would I need to make to my implementation to achieve that?
> > 
> > The tabstrip has a bunch of tabs, but only one active (the one whose content
> you
> > see). In Browser you can make a tab active by way of ActivateContents.
> 
> Is it necessary to call ActivateContents() if this call can only be triggered
by
> a user gesture? Does that not imply that the tab in question would already be
> active?

With the IPC calls it might be possible for a user to switch tabs before the JS
is processed. Probably pretty unlikely though.

> I've added a call to ActivateContents() to TabContents before it delegates to
> TabContentsDelegate (ie: Browser) to activate the tab that the JS call was
made
> from, is that ok?

If you're going to activate the tab, I think that is best done in Browser, not
TabContents. But, I'm fine with you not activating at all. I only raised this
because your comments in TabContentsDelegate say 'focussing on this tab' which
implies it's calling activate.

[koz (OOO until 15th September), 2011-08-01 03:43:38]

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/8002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
On 2011/07/29 15:51:46, sky wrote:
> On 2011/07/29 04:12:57, koz wrote:
> > On 2011/07/28 16:07:02, sky wrote:
> > > On 2011/07/28 05:14:30, koz wrote:
> > > > On 2011/07/28 03:53:29, sky wrote:
> > > > > This description makes it sound as though you intended to make Browser
> > > select
> > > > > the tab that invoked it. Your implementation in Browser doesn't select
> the
> > > tab
> > > > > though. Did you want to?
> > > > 
> > > > I think so. What exactly does 'select the tab' mean in this context?
This
> > > > function will be called by a JS API (webkitRequestFullScreen())
triggered
> by
> > a
> > > > user gesture on a page and I want that page to be fullscreened. What
> changes
> > > > would I need to make to my implementation to achieve that?
> > > 
> > > The tabstrip has a bunch of tabs, but only one active (the one whose
content
> > you
> > > see). In Browser you can make a tab active by way of ActivateContents.
> > 
> > Is it necessary to call ActivateContents() if this call can only be
triggered
> by
> > a user gesture? Does that not imply that the tab in question would already
be
> > active?
> 
> With the IPC calls it might be possible for a user to switch tabs before the
JS
> is processed. Probably pretty unlikely though.
> 
> > I've added a call to ActivateContents() to TabContents before it delegates
to
> > TabContentsDelegate (ie: Browser) to activate the tab that the JS call was
> made
> > from, is that ok?
> 
> If you're going to activate the tab, I think that is best done in Browser, not
> TabContents. But, I'm fine with you not activating at all. I only raised this
> because your comments in TabContentsDelegate say 'focussing on this tab' which
> implies it's calling activate.

Ah, I see. I'll rephrase the comment and remove the call to Activate(). Sorry
for the confusion!

[sky, 2011-08-01 14:03:47]

LGTM

[koz (OOO until 15th September), 2011-08-09 00:56:38]

On 2011/08/01 14:03:47, sky wrote:
> LGTM

sky could I get you to take another look? The following behavioural changes have
been made, which should make this patch suitable to land in trunk:
- now when the user exits fullscreen, chrome IPCs into WebKit to tell it to
leave fullscreen mode
- changing tabs while fullscreened from the JS API is disabled
- closing a tab that has been fullscreened from the JS API exits fullscreen mode

[koz (OOO until 15th September), 2011-08-11 00:51:11]

On 2011/08/09 00:56:38, koz wrote:
> On 2011/08/01 14:03:47, sky wrote:
> > LGTM
> 
> sky could I get you to take another look? The following behavioural changes
have
> been made, which should make this patch suitable to land in trunk:
> - now when the user exits fullscreen, chrome IPCs into WebKit to tell it to
> leave fullscreen mode
> - changing tabs while fullscreened from the JS API is disabled
> - closing a tab that has been fullscreened from the JS API exits fullscreen
mode

It seems sky is on leave. Ben, could you please take a look at my changes to
browser.cc?

[Peter Kasting, 2011-08-11 01:12:25]

I can try and look at this tomorrow.

I think banning tab switching while fullscreened from JS is a mistake.  I think
instead switching tabs in this case should trigger Chrome to exit fullscreen
mode (and tell WebKit to do so).

[koz (OOO until 15th September), 2011-08-11 02:00:41]

On 2011/08/11 01:12:25, Peter Kasting wrote:
> I can try and look at this tomorrow.
> 
> I think banning tab switching while fullscreened from JS is a mistake.  I
think
> instead switching tabs in this case should trigger Chrome to exit fullscreen
> mode (and tell WebKit to do so).

Thanks Peter, that'd be great.

You're right about it making more sense to exit fullscreen mode on ctrl-tab.
That's more consistent with how we exit fullscreen mode when (for example) the
tab loses focus. I've uploaded a new patch with that change.

[Peter Kasting, 2011-08-11 17:53:38]

In one of my comments below I condense a pair of enter/exit functions into a
single "toggle" function.  This is because I think in general you could simplify
the API surface for this whole pipeline by making all the functions and IPC
messages be similarly collapsed from two no-arg messages into a single one-arg
message.  (I'm not suggesting you also have to change WebCore, just the Chrome
side.)  You don't have to make this change, but we have so much functionality
plumbed through these layers that keeping the additional surface for each new
feature as small as possible seems like a win.

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1512: if (fullscreen_for_tab_) {
Nit: No need for {} (3 places)

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3506: fullscreen_for_tab_ = true;
It seems like a mistake to set this variable before checking the existing
fullscreen state.  If the user has already entered "global fullscreen" mode,
then it seems like that should override tab-specific fullscreen mode, in that I
should be able to change tabs etc. like I normally do without suddenly being
forced to exit fullscreen mode.

This also raises the question of what should happen when the renderer asks us to
_exit_ from fullscreen mode when we're in global fullscreen mode.  I'm not sure;
my instinct is to ignore the request.

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:883: virtual void EnterFullscreenModeForCurrentTab()
OVERRIDE;
I think these functions should take a TabContents* (like most of the functions
above).  Then the browser should ignore the requests if the requesting tab is
not the active tab.  This handles the case where a tab-switch races the
renderer's request to toggle fullscreen modes.

http://codereview.chromium.org/7461059/diff/37001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/37001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
Nit: Talking about browsers and tabs here seems like a little bit of a layering
violation (I know, a few comments already do it above); maybe:

  // Called when the renderer wants to switch between fullscreen and
  // non-fullscreen modes.
  virtual void ToggleFullscreenModeForRVH(bool show_fullscreen);

http://codereview.chromium.org/7461059/diff/37001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/37001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:295: // Exit fullscreen
mode.
Nit: Exit -> Exits

[koz (OOO until 15th September), 2011-08-15 05:50:26]

Thanks, Peter. I took your advice and merged the Enter/Exit functions into one
with a boolean parameter.

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1512: if (fullscreen_for_tab_) {
On 2011/08/11 17:53:38, Peter Kasting wrote:
> Nit: No need for {} (3 places)

Done.

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3506: fullscreen_for_tab_ = true;
On 2011/08/11 17:53:38, Peter Kasting wrote:
> It seems like a mistake to set this variable before checking the existing
> fullscreen state.  If the user has already entered "global fullscreen" mode,
> then it seems like that should override tab-specific fullscreen mode, in that
I
> should be able to change tabs etc. like I normally do without suddenly being
> forced to exit fullscreen mode.

I've changed the behaviour here so that instead of exiting fullscreen mode on
these events, only the tab itself gets put out of fullscreen mode. So for
example, if you put the browser into fullscreen mode with the toggle button, and
then fullscreen a slideshow from within a page, ctrl-tabbing to another tab will
make the slideshow stop rendering fullscreen style, but your window will remain
fullscreen.

Jeremy and I were discussing how desirable it would be to not evict the tab in
the above situation from fullscreen mode (and de-fullscreening all fullscreened
tabs at the point when the user exits fullscreen mode with the toggle button). I
think it is probably preferable, but that change would be best left for another
patch (assuming that the current behaviour is unacceptable).

> 
> This also raises the question of what should happen when the renderer asks us
to
> _exit_ from fullscreen mode when we're in global fullscreen mode.  I'm not
sure;
> my instinct is to ignore the request.

I think ignoring the request makes sense too, so I've implemented that.

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7461059/diff/37001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:883: virtual void EnterFullscreenModeForCurrentTab()
OVERRIDE;
On 2011/08/11 17:53:38, Peter Kasting wrote:
> I think these functions should take a TabContents* (like most of the functions
> above).  Then the browser should ignore the requests if the requesting tab is
> not the active tab.  This handles the case where a tab-switch races the
> renderer's request to toggle fullscreen modes.

Done.

http://codereview.chromium.org/7461059/diff/37001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/37001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:361: // Puts the
browser into fullscreen mode, focussing on this tab.
On 2011/08/11 17:53:38, Peter Kasting wrote:
> Nit: Talking about browsers and tabs here seems like a little bit of a
layering
> violation (I know, a few comments already do it above); maybe:
> 
>   // Called when the renderer wants to switch between fullscreen and
>   // non-fullscreen modes.
>   virtual void ToggleFullscreenModeForRVH(bool show_fullscreen);

I took the name and changed the wording on the comment slightly. How does the
new comment sound?

http://codereview.chromium.org/7461059/diff/37001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/37001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:295: // Exit fullscreen
mode.
On 2011/08/11 17:53:38, Peter Kasting wrote:
> Nit: Exit -> Exits

Done.

[Peter Kasting, 2011-08-15 17:51:28]

On 2011/08/15 05:50:26, koz wrote:
> I've changed the behaviour here so that instead of exiting fullscreen mode on
> these events, only the tab itself gets put out of fullscreen mode. So for
> example, if you put the browser into fullscreen mode with the toggle button,
and
> then fullscreen a slideshow from within a page, ctrl-tabbing to another tab
will
> make the slideshow stop rendering fullscreen style, but your window will
remain
> fullscreen.

Let me double-check I'm understanding you correctly.

We have a concept of the entire browser being in fullscreen mode, which in the
past has been manually triggerable by the user, and results in the current
window going fullscreen and staying that way until the user toggles this mode
back off.  The renderer is unaware of this change.

We now also have the more recent concept of "renderer fullscreen" mode, which is
triggered by something within the content area.  I think in this mode the
renderer knows it is "fullscreen" and thus content may draw differently? 
(Presumably using something like CSS media queries?  If I'm wrong in this, my
whole explanation here falls apart, so correct me.)  Also, this triggers the
browser to go into "browser fullscreen" mode, but in a way that will not persist
if the user closes or changes tabs.

Finally we have the third case, where both these are on at once.  (Is it
possible to trigger "renderer fullscreen" and then afterwards "browser
fullscreen" modes?  Or is it only possible to do it in the opposite order
because if you hit F11 while in "renderer fullscreen" mode you'll drop out of
fullscreen mode?)  What you're saying you've implemented is that in this case,
changing tabs will leave "browser fullscreen" mode on but tell the renderer that
the content should go back to drawing in "normal" rather than "fullscreen" mode.
 E.g. a media query will go back to matching 'normal" presentation, or however
this thing is implemented.

Did I get all that right?

If so, have you thought about making "browser fullscreen" mode auto-trigger
"renderer fullscreen" mode for all tabs?  Or does that throw too many wrenches
into things to make sense?

[koz (OOO until 15th September), 2011-08-15 20:58:18]

On 2011/08/15 17:51:28, Peter Kasting wrote:
> On 2011/08/15 05:50:26, koz wrote:
> > I've changed the behaviour here so that instead of exiting fullscreen mode
on
> > these events, only the tab itself gets put out of fullscreen mode. So for
> > example, if you put the browser into fullscreen mode with the toggle button,
> and
> > then fullscreen a slideshow from within a page, ctrl-tabbing to another tab
> will
> > make the slideshow stop rendering fullscreen style, but your window will
> remain
> > fullscreen.
> 
> Let me double-check I'm understanding you correctly.
> 
> We have a concept of the entire browser being in fullscreen mode, which in the
> past has been manually triggerable by the user, and results in the current
> window going fullscreen and staying that way until the user toggles this mode
> back off.  The renderer is unaware of this change.
> 
> We now also have the more recent concept of "renderer fullscreen" mode, which
is
> triggered by something within the content area.  I think in this mode the
> renderer knows it is "fullscreen" and thus content may draw differently? 
> (Presumably using something like CSS media queries?  If I'm wrong in this, my
> whole explanation here falls apart, so correct me.)  Also, this triggers the
> browser to go into "browser fullscreen" mode, but in a way that will not
persist
> if the user closes or changes tabs.
> 
> Finally we have the third case, where both these are on at once.  (Is it
> possible to trigger "renderer fullscreen" and then afterwards "browser
> fullscreen" modes?  Or is it only possible to do it in the opposite order
> because if you hit F11 while in "renderer fullscreen" mode you'll drop out of
> fullscreen mode?)  What you're saying you've implemented is that in this case,
> changing tabs will leave "browser fullscreen" mode on but tell the renderer
that
> the content should go back to drawing in "normal" rather than "fullscreen"
mode.
>  E.g. a media query will go back to matching 'normal" presentation, or however
> this thing is implemented.
> 
> Did I get all that right?
> 
> If so, have you thought about making "browser fullscreen" mode auto-trigger
> "renderer fullscreen" mode for all tabs?  Or does that throw too many wrenches
> into things to make sense?

Your explanation is spot on. The missing piece is that a renderer specifies an
element when going fullscreen, which will be the only thing that is rendered
while the renderer remains in fullscreen mode. For example the video element in
youtube will expand to be the only thing visible when the renderer enters
fullscreen mode. This means we can't force tabs into fullscreen on browser
fullscreen, as you suggest.

And you're right about only being able to go "browser fullscreen" then "tab
fullscreen" not the other way around because the "browser fullscreen" toggle
button has the effect of kicking the browser out of "tab fullscreen" if we are
in it.

[Ben Goodger (Google), 2011-08-15 22:37:12]

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1527: ExitTabbedFullscreenMode();
For all of these... these aren't the only ways that tabs can be selected. It
seems it'd be safer to do this in the TabStripModelObserver.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1637: if (!notify_tab_of_fullscreen_exit_)
Would be nice for the logic of this function to live in the TCW... I don't like
seeing browser-level code doing things like sending IPC messages.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3513: if (enter_fullscreen) {
These toggling functions make my brain hurt.

Given the logic... I think it'd be clearer if there were two functions...
EnterFullscreen and ExitFullscreen.

http://codereview.chromium.org/7461059/diff/42001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:294: bool
enter_fullscreen);
Nix the "ForRVH" suffix.

[Peter Kasting, 2011-08-15 22:42:54]

Ben's comments about where to listen for tab changes seem pretty critical. 
Other stuff LGTM.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3513: if (enter_fullscreen) {
On 2011/08/15 22:37:12, Ben Goodger (Google) wrote:
> These toggling functions make my brain hurt.
> 
> Given the logic... I think it'd be clearer if there were two functions...
> EnterFullscreen and ExitFullscreen.

BTW Ben, that's how things originally were, and I asked for this change, because
it reduced a lot of API surface and redundancy.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3513: if (enter_fullscreen) {
Nit: Shorter:

  notify_tab_of_fullscreen_exit_ = enter_fullscreen;
  if (enter_fullscreen && !window_->IsFullscreen())
    tab_caused_fullscreen_ = true;
  if (tab_caused_fullscreen_)
    ToggleFullscreenMode();

Given sufficient comments about what's going on, this hopefully obviates the
need for Ben's revert-to-two-functions suggestion.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:883: virtual void
ToggleFullscreenModeForRVH(TabContents* tab,
Nit: Since this function takes a TabContents*, it should probably be named
"...ForTab".

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1134: void ExitTabbedFullscreenMode();
Nit: I think adding "IfNecessary" to both these function names (and modifying
the comments similarly) might improve things, as both of them are happy to no-op
if we're not in fullscreen mode.

http://codereview.chromium.org/7461059/diff/42001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:361: // Notification
that the page has gone into or out of fullscreen mode.
Nit: has gone -> wants to go?

http://codereview.chromium.org/7461059/diff/42001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:294: bool
enter_fullscreen);
On 2011/08/15 22:37:12, Ben Goodger (Google) wrote:
> Nix the "ForRVH" suffix.

I think it'd be better kept but called "ForTab" instead, since that helps
disambiguate this from a request to put the whole browser into fullscreen mode.

http://codereview.chromium.org/7461059/diff/42001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/common/view_message...
content/common/view_messages.h:1966: // Puts the browser into fullscreen mode.
Nit: Slightly more commentary about tab- versus browser-fullscreen mode?

[koz (OOO until 15th September), 2011-08-16 07:06:55]

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1527: ExitTabbedFullscreenMode();
On 2011/08/15 22:37:12, Ben Goodger (Google) wrote:
> For all of these... these aren't the only ways that tabs can be selected. It
> seems it'd be safer to do this in the TabStripModelObserver.

Done.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1637: if (!notify_tab_of_fullscreen_exit_)
On 2011/08/15 22:37:12, Ben Goodger (Google) wrote:
> Would be nice for the logic of this function to live in the TCW... I don't
like
> seeing browser-level code doing things like sending IPC messages.

Done.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3513: if (enter_fullscreen) {
On 2011/08/15 22:42:54, Peter Kasting wrote:
> Nit: Shorter:
> 
>   notify_tab_of_fullscreen_exit_ = enter_fullscreen;
>   if (enter_fullscreen && !window_->IsFullscreen())
>     tab_caused_fullscreen_ = true;
>   if (tab_caused_fullscreen_)
>     ToggleFullscreenMode();
> 
> Given sufficient comments about what's going on, this hopefully obviates the
> need for Ben's revert-to-two-functions suggestion.

This is shorter, but I feel it is less semantic. Maybe if I add an Enter/Exit
function, but leave the public interface as the single Toggle function, it'd be
the best of both worlds? I'll implement that and upload it, let me know what you
think.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:883: virtual void
ToggleFullscreenModeForRVH(TabContents* tab,
On 2011/08/15 22:42:54, Peter Kasting wrote:
> Nit: Since this function takes a TabContents*, it should probably be named
> "...ForTab".

Done.

http://codereview.chromium.org/7461059/diff/42001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1134: void ExitTabbedFullscreenMode();
On 2011/08/15 22:42:54, Peter Kasting wrote:
> Nit: I think adding "IfNecessary" to both these function names (and modifying
> the comments similarly) might improve things, as both of them are happy to
no-op
> if we're not in fullscreen mode.

Done.

http://codereview.chromium.org/7461059/diff/42001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_delegate.h:361: // Notification
that the page has gone into or out of fullscreen mode.
On 2011/08/15 22:42:54, Peter Kasting wrote:
> Nit: has gone -> wants to go?

Done.

http://codereview.chromium.org/7461059/diff/42001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:294: bool
enter_fullscreen);
On 2011/08/15 22:42:54, Peter Kasting wrote:
> On 2011/08/15 22:37:12, Ben Goodger (Google) wrote:
> > Nix the "ForRVH" suffix.
> 
> I think it'd be better kept but called "ForTab" instead, since that helps
> disambiguate this from a request to put the whole browser into fullscreen
mode.

I've renamed to *ForTab.

http://codereview.chromium.org/7461059/diff/42001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7461059/diff/42001/content/common/view_message...
content/common/view_messages.h:1966: // Puts the browser into fullscreen mode.
On 2011/08/15 22:42:54, Peter Kasting wrote:
> Nit: Slightly more commentary about tab- versus browser-fullscreen mode?

Good point. I've extended the comment slightly - I feel like the only
differences between "browser" and "tab" fullscreen are minor behavioural
differences that only manifest in the handling of edge cases, so the code here
wouldn't benefit from too much more of an explanation.

[Peter Kasting, 2011-08-16 18:04:43]

http://codereview.chromium.org/7461059/diff/48001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/48001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3147: int active_index =
tab_handler_->GetTabStripModel()->active_index();
Nit: This variable seems unused?  In fact this whole function should just be
unchanged.

http://codereview.chromium.org/7461059/diff/48001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3508: EnterFullscreenModeForTab(tab);
I don't think this is a win over your previous patch.  It's just more lines of
code, without much other benefit I can see.

[koz (OOO until 15th September), 2011-08-17 03:47:05]

http://codereview.chromium.org/7461059/diff/48001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/48001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3147: int active_index =
tab_handler_->GetTabStripModel()->active_index();
On 2011/08/16 18:04:43, Peter Kasting wrote:
> Nit: This variable seems unused?  In fact this whole function should just be
> unchanged.

Done.

http://codereview.chromium.org/7461059/diff/48001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3508: EnterFullscreenModeForTab(tab);
On 2011/08/16 18:04:43, Peter Kasting wrote:
> I don't think this is a win over your previous patch.  It's just more lines of
> code, without much other benefit I can see.

I think this is clearer because each function is easy to digest in isolation.
Having them in the same scope means you need to swallow all that logic at once,
which is a little overwhelming.

That said, it's a minor point and I'm happy to defer. I've implemented your
suggestion from last patch here.

[Peter Kasting, 2011-08-17 17:36:22]

Any particular reason you switched |notify_on_fullscreen_exit_| from a bool to a
TCW*?  It seems like this implies to a reader that we may want to notify
arbitrary tabs when we exit fullscreen mode, which is not as clear as the old
"notify the selected tab on exit" code.

[koz (OOO until 15th September), 2011-08-17 21:51:34]

On 2011/08/17 17:36:22, Peter Kasting wrote:
> Any particular reason you switched |notify_on_fullscreen_exit_| from a bool to
a
> TCW*?  It seems like this implies to a reader that we may want to notify
> arbitrary tabs when we exit fullscreen mode, which is not as clear as the old
> "notify the selected tab on exit" code.

Yep - Ben's suggestion to move the ExitTabbedFullscreenModeIfNecessary() code
into the TabStripModelObserver means that the TabContentsWrapper that went
fullscreen won't necessarily be available with a call to
GetCurrentTabContents().

I could rename it fullscreened_tab_, which might make it more clear what it
holds?

[Peter Kasting, 2011-08-17 21:55:24]

On 2011/08/17 21:51:34, koz wrote:
> On 2011/08/17 17:36:22, Peter Kasting wrote:
> > Any particular reason you switched |notify_on_fullscreen_exit_| from a bool
to
> a
> > TCW*?  It seems like this implies to a reader that we may want to notify
> > arbitrary tabs when we exit fullscreen mode, which is not as clear as the
old
> > "notify the selected tab on exit" code.
> 
> Yep - Ben's suggestion to move the ExitTabbedFullscreenModeIfNecessary() code
> into the TabStripModelObserver means that the TabContentsWrapper that went
> fullscreen won't necessarily be available with a call to
> GetCurrentTabContents().

Oh, because you're getting called after the tab has already changed?  (Isn't
there a callback that happens before the active tab changes?)

> I could rename it fullscreened_tab_, which might make it more clear what it
> holds?

Yeah, that'd probably be good.

[koz (OOO until 15th September), 2011-08-18 00:22:57]

On 2011/08/17 21:55:24, Peter Kasting wrote:
> On 2011/08/17 21:51:34, koz wrote:
> > On 2011/08/17 17:36:22, Peter Kasting wrote:
> > > Any particular reason you switched |notify_on_fullscreen_exit_| from a
bool
> to
> > a
> > > TCW*?  It seems like this implies to a reader that we may want to notify
> > > arbitrary tabs when we exit fullscreen mode, which is not as clear as the
> old
> > > "notify the selected tab on exit" code.
> > 
> > Yep - Ben's suggestion to move the ExitTabbedFullscreenModeIfNecessary()
code
> > into the TabStripModelObserver means that the TabContentsWrapper that went
> > fullscreen won't necessarily be available with a call to
> > GetCurrentTabContents().
> 
> Oh, because you're getting called after the tab has already changed?  (Isn't
> there a callback that happens before the active tab changes?)

Yep, that's right. I thought that TabDeactivated() would be called when I wanted
it to, but it's actually called after GetSelectedTabContentsWrapper() changes.

> 
> > I could rename it fullscreened_tab_, which might make it more clear what it
> > holds?
> 
> Yeah, that'd probably be good.

Done.

[Peter Kasting, 2011-08-18 00:48:51]

LGTM with one question

http://codereview.chromium.org/7461059/diff/58001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/58001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:2929: ExitTabbedFullscreenModeIfNecessary();
I wonder if it would be better to move this into TabDeactivated() in case
somehow a tab can be deactivated without a new one being activated.  If so I
wonder if it's important to check the provided |contents| against the stored TCW
pointer.

[commit-bot: I haz the power, 2011-08-23 06:42:55]

Presubmit check for 7461059-60001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/renderer/render_view.cc,content/renderer/render_view.h,content/common/view_messages.h

Presubmit checks took 1.3s to calculate.

[koz (OOO until 15th September), 2011-08-23 06:46:56]

On 2011/08/23 06:42:55, I haz the power (commit-bot) wrote:
> Presubmit check for 7461059-60001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].
> 
> If this change requires manual test instructions to QA team, add
> TEST=[instructions].
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
content/renderer/render_view.cc,content/renderer/render_view.h,content/common/view_messages.h
> 
> Presubmit checks took 1.3s to calculate.

Ben, could you LGTM for owners?

[Ben Goodger (Google), 2011-08-23 16:07:21]

Do you have any tests for this behavior? If not, you should.

[koz (OOO until 15th September), 2011-08-24 00:16:53]

On 2011/08/23 16:07:21, Ben Goodger (Google) wrote:
> Do you have any tests for this behavior? If not, you should.

I've added a browsertest in browser_browsertest.cc.

[brettw, 2011-08-24 00:28:31]

LGTM

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.cc:284: bool
enter_fullscreen) {
Align to TabContents.

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:303: bool
enter_fullscreen);
nit: align this to the TabContents param since it fits.

[koz (OOO until 15th September), 2011-08-24 00:36:08]

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.cc:284: bool
enter_fullscreen) {
On 2011/08/24 00:28:31, brettw wrote:
> Align to TabContents.

Done.

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.h (right):

http://codereview.chromium.org/7461059/diff/65001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.h:303: bool
enter_fullscreen);
On 2011/08/24 00:28:31, brettw wrote:
> nit: align this to the TabContents param since it fits.

Done.

[Ben Goodger (Google), 2011-08-24 18:21:48]

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1280: FROM_HERE, method_factory_.NewRunnableMethod(
4-space indents here

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1286: chrome::NOTIFICATION_FULLSCREEN_CHANGED,
and here

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:646:
browser()->ToggleFullscreenModeForTab(fullscreen_tab, true);
Can you add a test for exiting fullscreen mode using this method too? (as I'd
imagine it'd happen from calling your API in both directions).

[koz (OOO until 15th September), 2011-08-24 21:39:33]

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1280: FROM_HERE, method_factory_.NewRunnableMethod(
On 2011/08/24 18:21:49, Ben Goodger (Google) wrote:
> 4-space indents here

Done.

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1286: chrome::NOTIFICATION_FULLSCREEN_CHANGED,
On 2011/08/24 18:21:49, Ben Goodger (Google) wrote:
> and here

Done.

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7461059/diff/66019/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:646:
browser()->ToggleFullscreenModeForTab(fullscreen_tab, true);
On 2011/08/24 18:21:49, Ben Goodger (Google) wrote:
> Can you add a test for exiting fullscreen mode using this method too? (as I'd
> imagine it'd happen from calling your API in both directions).

Done.

[Ben Goodger (Google), 2011-08-24 21:42:56]

LGTM

[commit-bot: I haz the power, 2011-08-25 02:14:35]

Can't apply patch for file chrome/browser/ui/browser.h.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/browser.h
Hunk #1 succeeded at 466 (offset -6 lines).
Hunk #2 succeeded at 795 (offset 4 lines).
Hunk #3 FAILED at 898.
Hunk #4 succeeded at 1037 (offset 10 lines).
Hunk #5 succeeded at 1179 (offset 10 lines).
Hunk #6 succeeded at 1330 (offset 11 lines).
1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/ui/browser.h.rej

[commit-bot: I haz the power, 2011-08-25 06:30:53]

Change committed as 98193