Split out fullscreen logic from Browser into FullscreenController.

Split out fullscreen logic from Browser into FullscreenController.


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

[koz (OOO until 15th September), 2011-11-03 05:53:37]

My approach here has been to lift all the relevant functions out of Browser into
FullscreenController and have Browser delegate the incoming calls to
FullscreenController.

I also moved the logic that Browser runs when the fullscreen state of the window
changes so that it runs based on notifications (ie: in Browser::Observe())
rather than a call to WindowFullscreenStateChanged(), so that
FullscreenController doesn't need to also be responsible for notifying Browser
of a state change explicitly.

Future changes from here might be to remove the reference to Browser from
FullscreenController - it's currently not used for much, but I figure small
refactors like that can be left for future changes.

[Peter Kasting, 2011-11-03 18:24:56]

There are a number of forwarding-style functions in the Browser.  Would it make
sense to remove those and have callers deal with the FullscreenController
directly or would that just complicate callers too much?

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:521: fullscreen_controller_ = new
FullscreenController(window_, profile_, this);
Nit: Seems like this might be better just below the CreateBrowserWindow() call.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3799: bool enter_fullscreen) {
Nit: While here, indent this arg even with the other

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:1412: FullscreenController* fullscreen_controller_;
Nit: I'd put this above |window_has_shown_|

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:25: method_factory_(this) {}
Nit: Linebreak between {} when entire constructor is not all on one line

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:27: void
FullscreenController::ToggleFullscreenModeForTab(TabContents* tab,
Please order the functions in this file in the same order as in the header.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:97: bool result = wrapper && wrapper
== fullscreened_tab_;
Nit: Seems clearer:

  if (!wrapper || (wrapper != fullscreened_tab_))
    return false;
  DCHECK(tab == browser_->GetSelectedTabContents());
  DCHECK(window_->IsFullscreen());
  return true;

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:143: HostContentSettingsMap*
settings_map = profile_->GetHostContentSettingsMap();
Nit: Could be rolled into next line

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:152: DCHECK_EQ(mouse_lock_state_,
MOUSELOCK_NOT_REQUESTED);
Nit: (EXPECTED, ACTUAL)

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:167:
window_->UpdateFullscreenExitBubbleContent(
Tiny nit: I tend to prefer this:

  window_->UpdateFullscreenExitBubbleContent(url,
                                             GetFullscreenExitBubbleType());

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:172: if (IsFullscreenForTab()) {
Nit: You can avoid {} if you reverse the conditional

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:179: // TODO(koz): Pull these methods
out of Browser into FullscreenController.
Nit: This TODO is confusing?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:269:
ContentSettingsPattern::FromURL(url),
Nit: Can go on previous line (2 places)

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:308:
FullscreenController::GetFullscreenExitBubbleType() const {
Nit: Indent 4

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:311: if (!tab_fullscreen_requested &&
!tab_fullscreen_accepted_) {
Nit: If |tab_fullscreen_accepted_| implies |fullscreened_tab_|. then this
conditional can just be:

  if (!fullscreened_tab_)

...and |tab_fullscreen_requested| can just be inlined into the next conditional.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:312: DCHECK_EQ(mouse_lock_state_,
MOUSELOCK_NOT_REQUESTED);
Nit: (EXPECTED, ACTUAL) (2 places)

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:329: FROM_HERE,
method_factory_.NewRunnableMethod(
Please convert this to using base::Bind().

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:19: // There are two different kinds
of fullscreen mode - "tab fullscreen" and
Nit: Seems like this is line-wrapped much earlier than 80 columns

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:26: // user forces an exit from
fullscreen, we need to notify the tab so it can
Nit: exit from fullscreen -> exit from tab fullscreen?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:32:
FullscreenController(BrowserWindow* window,
Nit: Since Browser has window() and profile() accessors, it seems like it'd be
better to just pass in Browser* alone, and then use these accessors elsewhere.

We could also conceivably cache the return values in members like you're
effectively doing in this change, but the nice part about using the accessors
everywhere is that it would allow Browser to construct the FullscreenController
in its constructor, and it would make the code agnostic to any future changes in
the browser to make these functions return something more dynamic.

Note that this goes the opposite direction of your suggestion to remove Browser
from FullscreenController but that seems OK to me.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:35: ~FullscreenController();
This does not seem to be defined in the .cc file?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:79: ContentSetting
GetFullscreenSetting(const GURL& url);
Nit: Can these two functions be const?

[tfarina, 2011-11-04 01:09:31]

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:1412: FullscreenController* fullscreen_controller_;
Aren't we leaking this? Could you use scoped_ptr?

[yzshen1, 2011-11-04 01:56:32]

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:150: #include
"content/browser/tab_contents/navigation_details.h"
Why this is needed?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:406: bool Browser::IsFullscreenForTab() const {
Please try to keep the method definition the same order as the declaration in .h
(if possible).

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1711:
fullscreen_controller_->TogglePresentationMode(for_tab);
FYI, jeremy is changing the content of this method.
(http://code.google.com/p/chromium/issues/detail?id=102786) Please make sure you
update your code accordingly if he checks in his CL first.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4122: case chrome::NOTIFICATION_FULLSCREEN_CHANGED:
[Not sure whether it affects anything, just want to make sure that you have
considered it.]
The original code runs these two methods synchronously (when
WindowFullscreenStateChanged is on the stack), while the current code runs them
asynchronously. Please make sure this change won't affect anything.

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:834: void OnAcceptFullscreenPermission(const GURL&
url,
You could make fullscreen exit bubble directly notify fullscreen controller.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:9: #include
"chrome/browser/ui/fullscreen_controller.h"
This should be the first include.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:25: method_factory_(this) {}
ALLOW_THIS_IN_INITIALIZER_LIST ?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:26: 
You haven't defined the destructor, right?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:28: bool enter_fullscreen) {
Please indent to align with the previous parameter.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:13: class Profile;
Sort them, please.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:55: FullscreenExitBubbleType
bubble_type);
It seems you need to included .h for FullscreenExitBubbleType.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:59: 
One empty line?

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:100: };
Disallow copy and assign?

[koz (OOO until 15th September), 2011-11-06 23:30:20]

Thanks for the comments, all.

Peter, I think it may make sense to try and remove the forwarding methods from
Browser, but it seems like it might be a case-by-case thing. I'll investigate
this in a follow up change in the interests of keeping each step of this
refactor small, in particular the On{Accept,Deny}FullscreenPermission() methods
as Yuzhu mentioned below.

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:150: #include
"content/browser/tab_contents/navigation_details.h"
On 2011/11/04 01:56:32, yzshen1 wrote:
> Why this is needed?

Hm, turns out it's not. I'm not sure how that got in there.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:406: bool Browser::IsFullscreenForTab() const {
On 2011/11/04 01:56:32, yzshen1 wrote:
> Please try to keep the method definition the same order as the declaration in
.h
> (if possible).

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:521: fullscreen_controller_ = new
FullscreenController(window_, profile_, this);
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Seems like this might be better just below the CreateBrowserWindow()
call.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1711:
fullscreen_controller_->TogglePresentationMode(for_tab);
On 2011/11/04 01:56:32, yzshen1 wrote:
> FYI, jeremy is changing the content of this method.
> (http://code.google.com/p/chromium/issues/detail?id=102786) Please make sure
you
> update your code accordingly if he checks in his CL first.

Yep, I will. Thanks for the tip.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3799: bool enter_fullscreen) {
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: While here, indent this arg even with the other

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4122: case chrome::NOTIFICATION_FULLSCREEN_CHANGED:
On 2011/11/04 01:56:32, yzshen1 wrote:
> [Not sure whether it affects anything, just want to make sure that you have
> considered it.]
> The original code runs these two methods synchronously (when
> WindowFullscreenStateChanged is on the stack), while the current code runs
them
> asynchronously. Please make sure this change won't affect anything.

I did consider this. I tested the UpdateCommands...() method manually and the
bookmark bar state is updated asynchronously above already so I'm assuming it's
fine.

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:834: void OnAcceptFullscreenPermission(const GURL&
url,
On 2011/11/04 01:56:32, yzshen1 wrote:
> You could make fullscreen exit bubble directly notify fullscreen controller.

Would you mind if I do that in a follow up change? I'd like to keep Browser's
public interface intact in this change to make it clear that this is a straight
lifting of functions out of it into FullscreenController. I've added a TODO.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:1412: FullscreenController* fullscreen_controller_;
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: I'd put this above |window_has_shown_|

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:1412: FullscreenController* fullscreen_controller_;
On 2011/11/04 01:09:31, tfarina wrote:
> Aren't we leaking this? Could you use scoped_ptr?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:9: #include
"chrome/browser/ui/fullscreen_controller.h"
On 2011/11/04 01:56:32, yzshen1 wrote:
> This should be the first include.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:25: method_factory_(this) {}
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Linebreak between {} when entire constructor is not all on one line

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:25: method_factory_(this) {}
On 2011/11/04 01:56:32, yzshen1 wrote:
> ALLOW_THIS_IN_INITIALIZER_LIST ?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:26: 
On 2011/11/04 01:56:32, yzshen1 wrote:
> You haven't defined the destructor, right?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:27: void
FullscreenController::ToggleFullscreenModeForTab(TabContents* tab,
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Please order the functions in this file in the same order as in the header.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:28: bool enter_fullscreen) {
On 2011/11/04 01:56:32, yzshen1 wrote:
> Please indent to align with the previous parameter.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:97: bool result = wrapper && wrapper
== fullscreened_tab_;
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Seems clearer:
> 
>   if (!wrapper || (wrapper != fullscreened_tab_))
>     return false;
>   DCHECK(tab == browser_->GetSelectedTabContents());
>   DCHECK(window_->IsFullscreen());
>   return true;

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:143: HostContentSettingsMap*
settings_map = profile_->GetHostContentSettingsMap();
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Could be rolled into next line

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:152: DCHECK_EQ(mouse_lock_state_,
MOUSELOCK_NOT_REQUESTED);
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: (EXPECTED, ACTUAL)

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:167:
window_->UpdateFullscreenExitBubbleContent(
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Tiny nit: I tend to prefer this:
> 
>   window_->UpdateFullscreenExitBubbleContent(url,
>                                              GetFullscreenExitBubbleType());

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:172: if (IsFullscreenForTab()) {
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: You can avoid {} if you reverse the conditional

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:179: // TODO(koz): Pull these methods
out of Browser into FullscreenController.
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: This TODO is confusing?

Removed.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:269:
ContentSettingsPattern::FromURL(url),
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Can go on previous line (2 places)

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:308:
FullscreenController::GetFullscreenExitBubbleType() const {
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Indent 4

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:311: if (!tab_fullscreen_requested &&
!tab_fullscreen_accepted_) {
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: If |tab_fullscreen_accepted_| implies |fullscreened_tab_|. then this
> conditional can just be:
> 
>   if (!fullscreened_tab_)
> 
> ...and |tab_fullscreen_requested| can just be inlined into the next
conditional.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:312: DCHECK_EQ(mouse_lock_state_,
MOUSELOCK_NOT_REQUESTED);
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: (EXPECTED, ACTUAL) (2 places)

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:329: FROM_HERE,
method_factory_.NewRunnableMethod(
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Please convert this to using base::Bind().

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:13: class Profile;
On 2011/11/04 01:56:32, yzshen1 wrote:
> Sort them, please.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:19: // There are two different kinds
of fullscreen mode - "tab fullscreen" and
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Seems like this is line-wrapped much earlier than 80 columns

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:26: // user forces an exit from
fullscreen, we need to notify the tab so it can
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: exit from fullscreen -> exit from tab fullscreen?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:32:
FullscreenController(BrowserWindow* window,
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Since Browser has window() and profile() accessors, it seems like it'd be
> better to just pass in Browser* alone, and then use these accessors elsewhere.
> 
> We could also conceivably cache the return values in members like you're
> effectively doing in this change, but the nice part about using the accessors
> everywhere is that it would allow Browser to construct the
FullscreenController
> in its constructor, and it would make the code agnostic to any future changes
in
> the browser to make these functions return something more dynamic.
> 
> Note that this goes the opposite direction of your suggestion to remove
Browser
> from FullscreenController but that seems OK to me.

I'd prefer to continue down the path of removing dependence on Browser as such a
dependency makes it harder to unit test.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:35: ~FullscreenController();
On 2011/11/03 18:24:56, Peter Kasting wrote:
> This does not seem to be defined in the .cc file?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:55: FullscreenExitBubbleType
bubble_type);
On 2011/11/04 01:56:32, yzshen1 wrote:
> It seems you need to included .h for FullscreenExitBubbleType.

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:59: 
On 2011/11/04 01:56:32, yzshen1 wrote:
> One empty line?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:79: ContentSetting
GetFullscreenSetting(const GURL& url);
On 2011/11/03 18:24:56, Peter Kasting wrote:
> Nit: Can these two functions be const?

Done.

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.h:100: };
On 2011/11/04 01:56:32, yzshen1 wrote:
> Disallow copy and assign?

Done.

[tfarina, 2011-11-06 23:36:17]

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:11: #include
"chrome/common/content_settings.h"
Can you forward declare this too?

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:12: #include "googleurl/src/gurl.h"
You can forward declare this instead.

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:31: class FullscreenController :
public base::RefCounted<FullscreenController> {
include base/memory/ref_counted.h

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:99:
ScopedRunnableMethodFactory<FullscreenController> method_factory_;
Change this to base::WeakPtrFactory

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:101:
DISALLOW_COPY_AND_ASSIGN(FullscreenController);
include base/basictypes.h for this.

[koz (OOO until 15th September), 2011-11-06 23:50:24]

Thanks for the quick review, Thiago.

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:11: #include
"chrome/common/content_settings.h"
On 2011/11/06 23:36:18, tfarina wrote:
> Can you forward declare this too?

No, I use a non-reference to it in GetFullscreenSetting() and
GetMouseLockSetting().

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:12: #include "googleurl/src/gurl.h"
On 2011/11/06 23:36:18, tfarina wrote:
> You can forward declare this instead.

Done.

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:31: class FullscreenController :
public base::RefCounted<FullscreenController> {
On 2011/11/06 23:36:18, tfarina wrote:
> include base/memory/ref_counted.h

Done.

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:99:
ScopedRunnableMethodFactory<FullscreenController> method_factory_;
On 2011/11/06 23:36:18, tfarina wrote:
> Change this to base::WeakPtrFactory

Actually, this is unused now (after translating to base::Bind() and
base::RefCounted) so I've removed it instead.

http://codereview.chromium.org/8423035/diff/11001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:101:
DISALLOW_COPY_AND_ASSIGN(FullscreenController);
On 2011/11/06 23:36:18, tfarina wrote:
> include base/basictypes.h for this.

Done.

[yzshen1, 2011-11-08 00:55:41]

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

http://codereview.chromium.org/8423035/diff/2001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:834: void OnAcceptFullscreenPermission(const GURL&
url,
Okay.

On 2011/11/06 23:30:20, koz wrote:
> On 2011/11/04 01:56:32, yzshen1 wrote:
> > You could make fullscreen exit bubble directly notify fullscreen controller.
> 
> Would you mind if I do that in a follow up change? I'd like to keep Browser's
> public interface intact in this change to make it clear that this is a
straight
> lifting of functions out of it into FullscreenController. I've added a TODO.

http://codereview.chromium.org/8423035/diff/14008/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/14008/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:316:
DCHECK_NE(MOUSELOCK_NOT_REQUESTED, mouse_lock_state_);
This should be MOUSELOCK_ACCEPTED.

[koz (OOO until 15th September), 2011-11-08 03:16:37]

http://codereview.chromium.org/8423035/diff/14008/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/14008/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:316:
DCHECK_NE(MOUSELOCK_NOT_REQUESTED, mouse_lock_state_);
On 2011/11/08 00:55:41, yzshen1 wrote:
> This should be MOUSELOCK_ACCEPTED.

Done.

[yzshen1, 2011-11-08 18:56:51]

lgtm

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

Peter, LGTY?

[Peter Kasting, 2011-11-14 21:17:39]

I'm sorry I've not gotten to this.  I was unexpectedly OOO several days last
week and am WebKit sheriff this week.  I will try to get to this as soon as I
can.

[koz (OOO until 15th September), 2011-11-14 23:04:13]

On 2011/11/14 21:17:39, Peter Kasting wrote:
> I'm sorry I've not gotten to this.  I was unexpectedly OOO several days last
> week and am WebKit sheriff this week.  I will try to get to this as soon as I
> can.

Cool, thanks for the update.

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

On 2011/11/14 23:04:13, koz wrote:
> On 2011/11/14 21:17:39, Peter Kasting wrote:
> > I'm sorry I've not gotten to this.  I was unexpectedly OOO several days last
> > week and am WebKit sheriff this week.  I will try to get to this as soon as
I
> > can.
> 
> Cool, thanks for the update.

Peter, as this change is holding up future work on fullscreen (or creating more
merge issues) would you mind rubber stamping it and I can resolve any issues you
find in a follow up CL?

[Peter Kasting, 2011-11-15 00:59:59]

On 2011/11/15 00:57:38, koz wrote:
> Peter, as this change is holding up future work on fullscreen (or creating
more
> merge issues) would you mind rubber stamping it and I can resolve any issues
you
> find in a follow up CL?

Rubber-stamp LGTM so you can move forward.  I will still try to do a review of
this later.  If I don't see any issues, I'll just remain silent.

[koz (OOO until 15th September), 2011-11-15 01:01:15]

On 2011/11/15 00:59:59, Peter Kasting wrote:
> On 2011/11/15 00:57:38, koz wrote:
> > Peter, as this change is holding up future work on fullscreen (or creating
> more
> > merge issues) would you mind rubber stamping it and I can resolve any issues
> you
> > find in a follow up CL?
> 
> Rubber-stamp LGTM so you can move forward.  I will still try to do a review of
> this later.  If I don't see any issues, I'll just remain silent.

Great, thanks.

[commit-bot: I haz the power, 2011-11-21 00:49:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/8423035/20009

[commit-bot: I haz the power, 2011-11-21 04:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/8423035/32002

[commit-bot: I haz the power, 2011-11-21 05:40:56]

Change committed as 110886

[Peter Kasting, 2011-11-21 19:17:45]

Here's the belated followup review.

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

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:841: // TODO(koz): Remove this and have callers call
FullscreenController directly.
This is probably also what we should do for ToggleFullscreenMode(),
TogglePresentationMode(), IsFullscreenForTab(), the mouse-lock-related methods,
and any other fullscreen methods on this class -- just provide an accessor to
the fullscreen controller and no other passthrough methods.

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:1426: scoped_refptr<FullscreenController>
fullscreen_controller_;
Why is this a refcounted object?  It seems like a standard scoped_ptr would be
fine.

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:329: ContentSetting
Nit: Prefer to wrap these next two functions like:

ContentSetting FullscreenController::GetFullscreenSetting(
    const GURL& url) const {

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:336: 
Nit: Extra blank line

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:22: // There are two different kinds
of fullscreen mode - "tab fullscreen" and
Nit: This comment seems sort of randomly placed here.  I'd normally expect to
find a description of the whole class behavior.  I'm not quite sure where this
more specific comment about fullscreen behavior types goes.

[sky, 2011-12-08 23:34:13]

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:256:
base::Bind(&FullscreenController::NotifyFullscreenChange, this));
I just came across this in tracking down a regression (105915). How come this
notification is now async? I think this means we'll do a layout at fullscreen,
then change things around when this is processed.

[koz (OOO until 15th September), 2011-12-08 23:45:38]

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:256:
base::Bind(&FullscreenController::NotifyFullscreenChange, this));
On 2011/12/08 23:34:13, sky wrote:
> I just came across this in tracking down a regression (105915). How come this
> notification is now async? I think this means we'll do a layout at fullscreen,
> then change things around when this is processed.

This notification has always been async (see the deleted lines in browser.cc),
but what has changed is that the calls to UpdateCommandsForFullscreenMode() and
UpdateBookmarkBarState() now happen in response to this notification instead of
synchronously.

I didn't realise this would have that effect. If that is so we can move those
function calls into Browser::WindowFullscreenStateChanged().

[sky, 2011-12-08 23:51:07]

On Thu, Dec 8, 2011 at 3:45 PM,  <koz@chromium.org> wrote:
>
>
http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
> File chrome/browser/ui/fullscreen_controller.cc (right):
>
>
http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
> chrome/browser/ui/fullscreen_controller.cc:256:
> base::Bind(&FullscreenController::NotifyFullscreenChange, this));
> On 2011/12/08 23:34:13, sky wrote:
>>
>> I just came across this in tracking down a regression (105915). How
>
> come this
>>
>> notification is now async? I think this means we'll do a layout at
>
> fullscreen,
>>
>> then change things around when this is processed.
>
>
> This notification has always been async (see the deleted lines in
> browser.cc), but what has changed is that the calls to
> UpdateCommandsForFullscreenMode() and UpdateBookmarkBarState() now
> happen in response to this notification instead of synchronously.

Yes, this is what I meant.

> I didn't realise this would have that effect. If that is so we can move
> those function calls into Browser::WindowFullscreenStateChanged().

Yes, I think that would be better. Jeremy was looking at fixing 105915
by making browser observe the notification. If
Browser::WindowFullscreenStateChanged is always invoked, then we
should move the call to update the bookmark state back to
WindowFullScreenStateChanged. That way it won't be async.

  -Scott

[koz (OOO until 15th September), 2011-12-08 23:57:40]

Yep, it is always invoked. If Jeremy's looking at the fix then I'll leave
it to him.

James

On Thu, Dec 8, 2011 at 3:51 PM, Scott Violet <sky@chromium.org> wrote:

> On Thu, Dec 8, 2011 at 3:45 PM,  <koz@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
> > File chrome/browser/ui/fullscreen_controller.cc (right):
> >
> >
>
http://codereview.chromium.org/8423035/diff/32002/chrome/browser/ui/fullscree...
> > chrome/browser/ui/fullscreen_controller.cc:256:
> > base::Bind(&FullscreenController::NotifyFullscreenChange, this));
> > On 2011/12/08 23:34:13, sky wrote:
> >>
> >> I just came across this in tracking down a regression (105915). How
> >
> > come this
> >>
> >> notification is now async? I think this means we'll do a layout at
> >
> > fullscreen,
> >>
> >> then change things around when this is processed.
> >
> >
> > This notification has always been async (see the deleted lines in
> > browser.cc), but what has changed is that the calls to
> > UpdateCommandsForFullscreenMode() and UpdateBookmarkBarState() now
> > happen in response to this notification instead of synchronously.
>
> Yes, this is what I meant.
>
> > I didn't realise this would have that effect. If that is so we can move
> > those function calls into Browser::WindowFullscreenStateChanged().
>
> Yes, I think that would be better. Jeremy was looking at fixing 105915
> by making browser observe the notification. If
> Browser::WindowFullscreenStateChanged is always invoked, then we
> should move the call to update the bookmark state back to
> WindowFullScreenStateChanged. That way it won't be async.
>
>  -Scott
>