Move MaximizeModeWindowManager to the controller

Move MaximizeModeWindowManager to the controller

Remove MaximizeModeWindowManager from Shell. Place it within MaximizeModeController.
Move notifications to ShellObserver to after the creation of the window manager. This way checks for it being enabled are correct during notifications. Update all files that were working around this deficiency.

TEST=MaximizeModeController

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275843

[jonross, 2014-05-29 15:24:42]

Hi Rob,

Could you review this change to move maximize mode window manager into maximize
mode controller.

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:8: #include
<map>
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:202: if
(frame_->IsFullscreen()) {  // Can be clicked in immersive fullscreen.
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:212: } else if
(sender == close_button_) {
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:8: #include
<map>
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:157: }  //
namespace ash
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/frame/custom_frame_v...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/frame/custom_frame_v...
ash/frame/custom_frame_view_ash.cc:8: #include <vector>
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/root_window_controll...
ash/root_window_controller.cc:309: #endif  // OS_CHROMEOS
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:298: switch (auto_hide_behavior_) {
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:799:
!keyboard::IsKeyboardOverscrollEnabled())) {
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:1966: TestItem* item = new TestItem;
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:2018:
EXPECT_EQ(SHELF_BACKGROUND_DEFAULT, GetShelfWidget()->GetBackgroundType());
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager_unittest.cc:2072: generator.MoveMouseTo(399,
399);
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/brig...
File ash/system/chromeos/brightness/tray_brightness.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/brig...
ash/system/chromeos/brightness/tray_brightness.cc:8: 
cpplint

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:7: #include
<string>
cpplint

https://codereview.chromium.org/308683002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/308683002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/display/display_preferences_unittest.cc:9: 
cpplint

https://codereview.chromium.org/308683002/diff/20001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/308683002/diff/20001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:7: #include
<algorithm>
cpplint

https://codereview.chromium.org/308683002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/308683002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:8: 
cpplint

[flackr, 2014-05-29 16:10:06]

Nice, thanks for moving this! LGTM with a few nits.

+skuhne FYI.

https://codereview.chromium.org/308683002/diff/20001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:599: Shell* shell =
Shell::GetInstance();
nit: Don't bother saving Shell* since it's only used once.

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:799:
!keyboard::IsKeyboardOverscrollEnabled())) {
On 2014/05/29 15:24:42, jonross wrote:
> cpplint

nit: This can be aligned with the above line (i.e. at the !).

https://codereview.chromium.org/308683002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/shell.cc#newcode682
ash/shell.cc:682:
maximize_mode_controller_->EnableMaximizeModeWindowManager(false);
This should be implicit/automatic by destructing the controller.

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/brig...
File ash/system/chromeos/brightness/tray_brightness.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/brig...
ash/system/chromeos/brightness/tray_brightness.cc:120:
IsMaximizeModeWindowManagerEnabled());
nit: indent 4 more to show it's wrapped from above line.

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/rota...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/rota...
ash/system/chromeos/rotation/tray_rotation_lock.cc:48:
IsMaximizeModeWindowManagerEnabled());
nit: Indent 4 more to show wrapped from above line.

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:162:
Shell::GetInstance()->OnMaximizeModeStarted();
Can you add a TODO (probably to ShellObserver) to move these notifications to
MaximizeModeController::Observer?

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:56: void
EnableMaximizeModeWindowManager(bool enable);
I realize this is currently separate from EnableMaximizeMode for a couple
reasons, but eventually this should be the same, can you add a TODO? We could
not instantiate the event blocker and reference only blocking the chromebook
internal keyboard: http://crbug.com/362881 and only do the display locking if
there is an internal display.

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:116:
MaximizeModeController* maximize_mode_controller() const {
How about const and non-const versions which return a const or non const
pointer?

See for example Window::GetRootWindow. Although the implementation is short
enough you can do it twice instead of a const cast.

[jonross, 2014-05-29 17:39:59]

Hi Oshima,

This review cleans up all of the logic around toggling maximize mode.

https://codereview.chromium.org/308683002/diff/20001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:599: Shell* shell =
Shell::GetInstance();
On 2014/05/29 16:10:06, flackr wrote:
> nit: Don't bother saving Shell* since it's only used once.

Done.

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:799:
!keyboard::IsKeyboardOverscrollEnabled())) {
On 2014/05/29 16:10:06, flackr wrote:
> On 2014/05/29 15:24:42, jonross wrote:
> > cpplint
> 
> nit: This can be aligned with the above line (i.e. at the !).

Done.

https://codereview.chromium.org/308683002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/shell.cc#newcode682
ash/shell.cc:682:
maximize_mode_controller_->EnableMaximizeModeWindowManager(false);
On 2014/05/29 16:10:06, flackr wrote:
> This should be implicit/automatic by destructing the controller.
Creating a shutdown method on the controller.
We need to tell all observers of the shutdown so that they stop polling shell
for the controller. Which is null upon reset.

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/brig...
File ash/system/chromeos/brightness/tray_brightness.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/brig...
ash/system/chromeos/brightness/tray_brightness.cc:120:
IsMaximizeModeWindowManagerEnabled());
On 2014/05/29 16:10:06, flackr wrote:
> nit: indent 4 more to show it's wrapped from above line.

Done.

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/rota...
File ash/system/chromeos/rotation/tray_rotation_lock.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/system/chromeos/rota...
ash/system/chromeos/rotation/tray_rotation_lock.cc:48:
IsMaximizeModeWindowManagerEnabled());
On 2014/05/29 16:10:06, flackr wrote:
> nit: Indent 4 more to show wrapped from above line.

Done.

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:162:
Shell::GetInstance()->OnMaximizeModeStarted();
On 2014/05/29 16:10:06, flackr wrote:
> Can you add a TODO (probably to ShellObserver) to move these notifications to
> MaximizeModeController::Observer?

Done.

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:56: void
EnableMaximizeModeWindowManager(bool enable);
On 2014/05/29 16:10:06, flackr wrote:
> I realize this is currently separate from EnableMaximizeMode for a couple
> reasons, but eventually this should be the same, can you add a TODO? We could
> not instantiate the event blocker and reference only blocking the chromebook
> internal keyboard: http://crbug.com/362881 and only do the display locking if
> there is an internal display.

Done.

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (right):

https://codereview.chromium.org/308683002/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:116:
MaximizeModeController* maximize_mode_controller() const {
On 2014/05/29 16:10:06, flackr wrote:
> How about const and non-const versions which return a const or non const
> pointer?
> 
> See for example Window::GetRootWindow. Although the implementation is short
> enough you can do it twice instead of a const cast.

Removed the original const on 1125 to clear the need here. That way there is no
const/non-const mix up with the pointer.

[Mr4D (OOO till 08-26), 2014-05-29 22:15:24]

Nice! lgtm (see comments)

https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc#newcode682
ash/shell.cc:682: maximize_mode_controller_->Shutdown();
Combining the two is not possible? (calling shutdown on destroy)

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:141:
user_rotation_(gfx::Display::ROTATE_0) {
Just wondering - when debugging on goobuntu - is there a dummy
accelerometer_controller?

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:65: // Shutdowns down the
MaximizeModeWindowManager and notifies all observers.
Could you add a note here towards why you do not combine this with the
destructor since it got paired at least in one location?

[oshima, 2014-05-29 22:44:44]

IIRC, there was a place that is not using
ShellObserver::OnMaximizeModeStarted|Ended because
Shell::IsMaximizeModeWindowManagerEnabled() isn't accurate. skuhne@, do you
remember where was it?

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:56: // separate for several
reasons: there is no internal display when running
You can enable internal display for test using

gfx::Display::SetInternalDisplayId.

Is it easy to fix now?

[oshima, 2014-05-29 22:54:01]

never mind. you've already address it (shelf_layout_manager.h). So internal
display issue is the only question.

[jonross, 2014-05-30 14:37:50]

https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc#newcode682
ash/shell.cc:682: maximize_mode_controller_->Shutdown();
On 2014/05/29 22:15:24, Mr4D wrote:
> Combining the two is not possible? (calling shutdown on destroy)

Not right now.

Currently everything listening to OnMaximizeModeStarted/Ended ends up trigger
code that eventually polls the state.

But if we do that in the destructor then the maximize_mode_controller is null
and they segfault.

I think that would be better on a follow up CL, where we bring
OnMaximizeModeStarted/Ended into MaximizeModeController::Observer, instead of it
currently being tied to the ShellObserver.

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.cc:141:
user_rotation_(gfx::Display::ROTATE_0) {
On 2014/05/29 22:15:24, Mr4D wrote:
> Just wondering - when debugging on goobuntu - is there a dummy
> accelerometer_controller?

The controller is fully created, however there is no dummy accelerometer to
actually be read. So no readings take place, and OnAccelerometerUpdated is never
triggered.

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:56: // separate for several
reasons: there is no internal display when running
On 2014/05/29 22:44:45, oshima wrote:
> You can enable internal display for test using
> 
> gfx::Display::SetInternalDisplayId.
> 
> Is it easy to fix now?

We found then also want to add that to where accelerators turn this on. So that
when using alt+ctrl+shift+8 to trigger this mode on linux it also works.

https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:65: // Shutdowns down the
MaximizeModeWindowManager and notifies all observers.
On 2014/05/29 22:15:24, Mr4D wrote:
> Could you add a note here towards why you do not combine this with the
> destructor since it got paired at least in one location?

Done.

I'll upload on the next patch, once we decide on enabling the internal display
for tests/accelerator.

[oshima, 2014-05-30 17:04:10]

On 2014/05/30 14:37:50, jonross wrote:
> https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc#newcode682
> ash/shell.cc:682: maximize_mode_controller_->Shutdown();
> On 2014/05/29 22:15:24, Mr4D wrote:
> > Combining the two is not possible? (calling shutdown on destroy)
> 
> Not right now.
> 
> Currently everything listening to OnMaximizeModeStarted/Ended ends up trigger
> code that eventually polls the state.
> 
> But if we do that in the destructor then the maximize_mode_controller is null
> and they segfault.
> 
> I think that would be better on a follow up CL, where we bring
> OnMaximizeModeStarted/Ended into MaximizeModeController::Observer, instead of
it
> currently being tied to the ShellObserver.
> 
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> File ash/wm/maximize_mode/maximize_mode_controller.cc (right):
> 
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller.cc:141:
> user_rotation_(gfx::Display::ROTATE_0) {
> On 2014/05/29 22:15:24, Mr4D wrote:
> > Just wondering - when debugging on goobuntu - is there a dummy
> > accelerometer_controller?
> 
> The controller is fully created, however there is no dummy accelerometer to
> actually be read. So no readings take place, and OnAccelerometerUpdated is
never
> triggered.
> 
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> File ash/wm/maximize_mode/maximize_mode_controller.h (right):
> 
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller.h:56: // separate for several
> reasons: there is no internal display when running
> On 2014/05/29 22:44:45, oshima wrote:
> > You can enable internal display for test using
> > 
> > gfx::Display::SetInternalDisplayId.
> > 
> > Is it easy to fix now?
> 
> We found then also want to add that to where accelerators turn this on. So
that
> when using alt+ctrl+shift+8 to trigger this mode on linux it also works.
> 
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> ash/wm/maximize_mode/maximize_mode_controller.h:65: // Shutdowns down the
> MaximizeModeWindowManager and notifies all observers.
> On 2014/05/29 22:15:24, Mr4D wrote:
> > Could you add a note here towards why you do not combine this with the
> > destructor since it got paired at least in one location?
> 
> Done.
> 
> I'll upload on the next patch, once we decide on enabling the internal display
> for tests/accelerator.

Do you mean you'll update this CL, or wanto do that in a separate CL?

[jonross, 2014-05-30 17:25:49]

On 2014/05/30 17:04:10, oshima wrote:
> On 2014/05/30 14:37:50, jonross wrote:
> > https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc
> > File ash/shell.cc (right):
> > 
> > https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc#newcode682
> > ash/shell.cc:682: maximize_mode_controller_->Shutdown();
> > On 2014/05/29 22:15:24, Mr4D wrote:
> > > Combining the two is not possible? (calling shutdown on destroy)
> > 
> > Not right now.
> > 
> > Currently everything listening to OnMaximizeModeStarted/Ended ends up
trigger
> > code that eventually polls the state.
> > 
> > But if we do that in the destructor then the maximize_mode_controller is
null
> > and they segfault.
> > 
> > I think that would be better on a follow up CL, where we bring
> > OnMaximizeModeStarted/Ended into MaximizeModeController::Observer, instead
of
> it
> > currently being tied to the ShellObserver.
> > 
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > File ash/wm/maximize_mode/maximize_mode_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > ash/wm/maximize_mode/maximize_mode_controller.cc:141:
> > user_rotation_(gfx::Display::ROTATE_0) {
> > On 2014/05/29 22:15:24, Mr4D wrote:
> > > Just wondering - when debugging on goobuntu - is there a dummy
> > > accelerometer_controller?
> > 
> > The controller is fully created, however there is no dummy accelerometer to
> > actually be read. So no readings take place, and OnAccelerometerUpdated is
> never
> > triggered.
> > 
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > File ash/wm/maximize_mode/maximize_mode_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > ash/wm/maximize_mode/maximize_mode_controller.h:56: // separate for several
> > reasons: there is no internal display when running
> > On 2014/05/29 22:44:45, oshima wrote:
> > > You can enable internal display for test using
> > > 
> > > gfx::Display::SetInternalDisplayId.
> > > 
> > > Is it easy to fix now?
> > 
> > We found then also want to add that to where accelerators turn this on. So
> that
> > when using alt+ctrl+shift+8 to trigger this mode on linux it also works.
> > 
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > ash/wm/maximize_mode/maximize_mode_controller.h:65: // Shutdowns down the
> > MaximizeModeWindowManager and notifies all observers.
> > On 2014/05/29 22:15:24, Mr4D wrote:
> > > Could you add a note here towards why you do not combine this with the
> > > destructor since it got paired at least in one location?
> > 
> > Done.
> > 
> > I'll upload on the next patch, once we decide on enabling the internal
display
> > for tests/accelerator.
> 
> Do you mean you'll update this CL, or wanto do that in a separate CL?

I have the updated documentation ready to upload to this CL.

As for handling internal displays, I think that is best on another CL. I just
wanted to confirm that with everyone.

[oshima, 2014-05-30 18:43:07]

On 2014/05/30 17:25:49, jonross wrote:
> On 2014/05/30 17:04:10, oshima wrote:
> > On 2014/05/30 14:37:50, jonross wrote:
> > > https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc
> > > File ash/shell.cc (right):
> > > 
> > >
https://codereview.chromium.org/308683002/diff/40001/ash/shell.cc#newcode682
> > > ash/shell.cc:682: maximize_mode_controller_->Shutdown();
> > > On 2014/05/29 22:15:24, Mr4D wrote:
> > > > Combining the two is not possible? (calling shutdown on destroy)
> > > 
> > > Not right now.
> > > 
> > > Currently everything listening to OnMaximizeModeStarted/Ended ends up
> trigger
> > > code that eventually polls the state.
> > > 
> > > But if we do that in the destructor then the maximize_mode_controller is
> null
> > > and they segfault.
> > > 
> > > I think that would be better on a follow up CL, where we bring
> > > OnMaximizeModeStarted/Ended into MaximizeModeController::Observer, instead
> of
> > it
> > > currently being tied to the ShellObserver.
> > > 
> > >
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > > File ash/wm/maximize_mode/maximize_mode_controller.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > > ash/wm/maximize_mode/maximize_mode_controller.cc:141:
> > > user_rotation_(gfx::Display::ROTATE_0) {
> > > On 2014/05/29 22:15:24, Mr4D wrote:
> > > > Just wondering - when debugging on goobuntu - is there a dummy
> > > > accelerometer_controller?
> > > 
> > > The controller is fully created, however there is no dummy accelerometer
to
> > > actually be read. So no readings take place, and OnAccelerometerUpdated is
> > never
> > > triggered.
> > > 
> > >
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > > File ash/wm/maximize_mode/maximize_mode_controller.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > > ash/wm/maximize_mode/maximize_mode_controller.h:56: // separate for
several
> > > reasons: there is no internal display when running
> > > On 2014/05/29 22:44:45, oshima wrote:
> > > > You can enable internal display for test using
> > > > 
> > > > gfx::Display::SetInternalDisplayId.
> > > > 
> > > > Is it easy to fix now?
> > > 
> > > We found then also want to add that to where accelerators turn this on. So
> > that
> > > when using alt+ctrl+shift+8 to trigger this mode on linux it also works.
> > > 
> > >
> >
>
https://codereview.chromium.org/308683002/diff/40001/ash/wm/maximize_mode/max...
> > > ash/wm/maximize_mode/maximize_mode_controller.h:65: // Shutdowns down the
> > > MaximizeModeWindowManager and notifies all observers.
> > > On 2014/05/29 22:15:24, Mr4D wrote:
> > > > Could you add a note here towards why you do not combine this with the
> > > > destructor since it got paired at least in one location?
> > > 
> > > Done.
> > > 
> > > I'll upload on the next patch, once we decide on enabling the internal
> display
> > > for tests/accelerator.
> > 
> > Do you mean you'll update this CL, or wanto do that in a separate CL?
> 
> I have the updated documentation ready to upload to this CL.
> 
> As for handling internal displays, I think that is best on another CL. I just
> wanted to confirm that with everyone.

ok, lgtm then.

[jonross, 2014-05-30 18:49:20]

Hi James,

Would you be able to provide owner review to a few chrome/browser files affected
by this refactoring review?

[James Cook, 2014-05-30 19:47:50]

LGTM with nits. Nice refactor.

Next time please let me know which specific files or directories you want me to
look at. I presume you mostly needed chrome/browser/ui/views/frame.

https://codereview.chromium.org/308683002/diff/50001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/308683002/diff/50001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:8: #include
<map>
Thank you for fixing include-what-you-use

https://codereview.chromium.org/308683002/diff/50001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:91: // maximized
and is Maximize Mode is enabled. A parent view should relayout
nit: "whether maximized mode" or "if maximize mode"

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:65: // TODO(jonross): move this
into the desctrutor. Currently separated as
nit: destructor

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:68: // destructor the controller
is null, and the obersvers segfault.
nit: observers

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:69: // Shutdowns down the
MaximizeModeWindowManager and notifies all observers.
nit: shuts down

[jonross, 2014-06-02 13:47:41]

The CQ bit was checked by jonross@chromium.org

[jonross, 2014-06-02 13:48:05]

https://codereview.chromium.org/308683002/diff/50001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/308683002/diff/50001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:91: // maximized
and is Maximize Mode is enabled. A parent view should relayout
On 2014/05/30 19:47:51, James Cook wrote:
> nit: "whether maximized mode" or "if maximize mode"

Done.

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_controller.h (right):

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:65: // TODO(jonross): move this
into the desctrutor. Currently separated as
On 2014/05/30 19:47:51, James Cook wrote:
> nit: destructor

Done.

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:68: // destructor the controller
is null, and the obersvers segfault.
On 2014/05/30 19:47:51, James Cook wrote:
> nit: observers

Done.

https://codereview.chromium.org/308683002/diff/50001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_controller.h:69: // Shutdowns down the
MaximizeModeWindowManager and notifies all observers.
On 2014/05/30 19:47:51, James Cook wrote:
> nit: shuts down

Done.

[commit-bot: I haz the power, 2014-06-02 13:48:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/308683002/70001

[jonross, 2014-06-02 17:04:58]

The CQ bit was unchecked by jonross@chromium.org

[jonross, 2014-06-06 15:02:40]

This change has been rebased. This picks up changes to ShelfLayoutManager, which
had removed its dependency on MaximizeModeController. As such all changes to
those files were removed in the rebase process, and they are no longer changed
by this review. Without this dependency the shutdown failure that was picked up
by ash_unittests no longer occur.

[jonross, 2014-06-06 15:02:47]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-06-06 15:04:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/308683002/110001

[commit-bot: I haz the power, 2014-06-06 20:04:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-06 22:58:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 22:58:48]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[jonross, 2014-06-09 13:02:21]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-06-09 13:02:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/308683002/110001

[commit-bot: I haz the power, 2014-06-09 20:02:34]

Change committed as 275843