ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h - Issue 2884623002: Refactor backdrop

Side by Side Diff: ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h

Issue 2884623002: Refactor backdrop (Closed)

Patch Set: Generalize Backdrop Created 3 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « ash/test/workspace_controller_test_api.cc ('k') | ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.cc » ('j') | ash/wm/workspace/backdrop_controller.h » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2017 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #include "ash/wm/workspace/backdrop_delegate.h"

	6

	7 #include "ash/ash_export.h"

	8 #include "base/macros.h"

	9

	10 namespace ash {

	11

	12 class ASH_EXPORT MaximizeModeBackdropDelegateImpl : public BackdropDelegate {
	James Cook 2017/05/15 21:57:38 Class docs please. (Like, why is this a public cla Class docs please. (Like, why is this a public class rather than something local to MaximizeModeWindowManager? Tests, right?) optional: Also, could MaximizeModeWindowManager just implement BackdropDelegate? Or do you think that makes the lifetimes of the delegate unclear? oshima 2017/05/16 08:22:08 By public class, you mean the ctor is public? I ca Show quoted text On 2017/05/15 21:57:38, James Cook wrote: > Class docs please. (Like, why is this a public class rather than something local > to MaximizeModeWindowManager? Tests, right?) By public class, you mean the ctor is public? I can create a factory method and hide this class for test, so I'm not sure if mentioning the test is really useful. I can move the impl to MMWM, then just enter the maximized mode in the test. WDYT? Show quoted text > optional: Also, could MaximizeModeWindowManager just implement BackdropDelegate? > Or do you think that makes the lifetimes of the delegate unclear? The delegate is owned by the controller, and want to keep it that way especially because it doesn't depend on any state of the MMWM. James Cook 2017/05/16 15:12:14 Yes, I meant have this class be in the .cc file fo Show quoted text On 2017/05/16 08:22:08, oshima wrote: > On 2017/05/15 21:57:38, James Cook wrote: > > Class docs please. (Like, why is this a public class rather than something > local > > to MaximizeModeWindowManager? Tests, right?) > > By public class, you mean the ctor is public? I can create a factory method and > hide > this class for test, so I'm not sure if mentioning the test is really useful. > > I can move the impl to MMWM, then just enter the maximized mode in the test. > WDYT? > > > optional: Also, could MaximizeModeWindowManager just implement > BackdropDelegate? > > Or do you think that makes the lifetimes of the delegate unclear? > > The delegate is owned by the controller, and want to keep it that way especially > because it doesn't depend on any state of the MMWM. Yes, I meant have this class be in the .cc file for MMWM. Keeping it owned by the controller sgtm. oshima 2017/05/17 07:44:33 Looks like the some of existing tests assume it's Show quoted text On 2017/05/16 15:12:14, James Cook wrote: > On 2017/05/16 08:22:08, oshima wrote: > > On 2017/05/15 21:57:38, James Cook wrote: > > > Class docs please. (Like, why is this a public class rather than something > > local > > > to MaximizeModeWindowManager? Tests, right?) > > > > By public class, you mean the ctor is public? I can create a factory method > and > > hide > > this class for test, so I'm not sure if mentioning the test is really useful. > > > > I can move the impl to MMWM, then just enter the maximized mode in the test. > > WDYT? > > > > > optional: Also, could MaximizeModeWindowManager just implement > > BackdropDelegate? > > > Or do you think that makes the lifetimes of the delegate unclear? > > > > The delegate is owned by the controller, and want to keep it that way > especially > > because it doesn't depend on any state of the MMWM. > > Yes, I meant have this class be in the .cc file for MMWM. > > Keeping it owned by the controller sgtm. Looks like the some of existing tests assume it's NOT in maximize mode, so i just added comment.
	13 public:

	14 MaximizeModeBackdropDelegateImpl() {}
	James Cook 2017/05/15 21:57:38 nit: = default (and consider moving to .cc) nit: = default (and consider moving to .cc) oshima 2017/05/16 08:22:08 Done. Is "= default" now recommended? Just double Show quoted text On 2017/05/15 21:57:38, James Cook wrote: > nit: = default (and consider moving to .cc) Done. Is "= default" now recommended? Just double checking because there were folks didn't like it because it's actually longer. James Cook 2017/05/16 15:12:14 I don't think the style guide has an opinion. I pr Show quoted text On 2017/05/16 08:22:08, oshima wrote: > On 2017/05/15 21:57:38, James Cook wrote: > > nit: = default (and consider moving to .cc) > > Done. > > Is "= default" now recommended? Just double checking > because there were folks didn't like it because it's actually longer. > I don't think the style guide has an opinion. I prefer it.
	15 ~MaximizeModeBackdropDelegateImpl() override {}

	16

	17 protected:

	18 bool HasBackdrop(WmWindow* window) override;

	19

	20 private:

	21 DISALLOW_COPY_AND_ASSIGN(MaximizeModeBackdropDelegateImpl);

	22 };

	23

	24 } // namespace ash

OLD	NEW