Adding new maximize menu according to spec

ash/wm/maximize_bubble_controller.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_WM_MAXIMIZE_BUBBLE_CONTROLLER_H_
+#define ASH_WM_MAXIMIZE_BUBBLE_CONTROLLER_H_
+
+#include "ash/ash_export.h"
+#include "ash/wm/workspace/frame_maximize_button.h"
+#include "base/memory/scoped_ptr.h"
+#include "ui/aura/event_filter.h"

[msw, 2012/08/02 18:56:59]

Is this include necessary?

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+
+namespace base {
+  class Timer;
+}
+
+namespace ash {
+
+// A class which shows a helper UI for the maximize button.
+// The menu gets created every time after an initial delay. The delay can be

[sky, 2012/08/02 18:30:30]

Using menu is confusing here since this class doesn't use a menu at all. I think
bubble makes more sense in this context. Alternatively if you're worried because
it'll get menu functionality than I think it's better to be obscure and use UI
every where, but in the class description mention either bubble or menu.

[msw, 2012/08/02 18:56:59]

nit: "every time" what happens? Consider just appending "after a delay" to the
line above.

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+// reset through a function.
+class ASH_EXPORT MaximizeBubbleController {
+ public:
+  explicit MaximizeBubbleController(FrameMaximizeButton* frame_maximize_button,

[sky, 2012/08/02 18:30:30]

no explicit

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+                                    bool is_maximized);
+  // Called from the outside to destroy the interface to the menu and
+  // the menu itself will delete itself then.

[msw, 2012/08/02 18:56:59]

nit: redundant "itself will delete itself"

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+  virtual ~MaximizeBubbleController();
+
+  // Update the visible menu to reflect the previewed |snap_type| snapping

[msw, 2012/08/02 18:56:59]

nit: nix "visible" for a single line comment :)

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+  // state.
+  void SetMenuState(FrameMaximizeButton::SnapType snap_type);

[sky, 2012/08/02 18:30:30]

How about SetSnapType?

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+
+  // Return a window handle underneath the snap animation window should

[sky, 2012/08/02 18:30:30]

The name of this method is rather confusing. I think you mean GetBubbleWindow()
with a description to match.

[msw, 2012/08/02 18:56:59]

nit: grammar / clarity; did you mean "underneath which", or rephrase.

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+  // operate.
+  aura::Window* GetMenuWindow();
+
+  // Reset the delay of the menu creation to the beg.

[sky, 2012/08/02 18:30:30]

beg?

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Done.

+  void DelayCreation();
+
+ protected:

[sky, 2012/08/02 18:30:30]

Why is any of this protected?

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

See below. I don't want to have the destruction function be available to anyone
but the Bubble class.

+  FrameMaximizeButton* frame_maximize_button() {
+    return frame_maximize_button_;
+  }
+
+  // Called from the menu itself: Closes the bubble menu at the earliest time
+  // and destroys |this| object through the owner.
+  void RequestDestructionThroughOwner();
+
+  // Called when a button was clicked.
+  void OnButtonClicked(FrameMaximizeButton::SnapType snap_type);
+
+  // Tell the owner that the hover status for a button has changed.
+  // |snap_type| can be either SNAP_LEFT, SNAP_RIGHT, SNAP_MINIMIZE or
+  // SNAP_NONE.
+  void OnButtonHover(FrameMaximizeButton::SnapType snap_type);
+
+ private:
+  class Bubble;

[sky, 2012/08/02 18:30:30]

Do all of these really need to be inner classes? For the ones that don't move
them into an anonymous namespace in the .cc. This way its clear they won't
access any private state.

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

I have removed all but one from this class. I want the function
"RequestDestructionThroughOwner" to be protected because this should only get
called from the class itself.

Since the other classes however use the Bubble class, they cannot be moved into
the anonymous namespace.

+  class BubbleContentsView;

[msw, 2012/08/02 18:56:59]

Are these forward decls necessary?

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

Again, removed most of them.

+  class BubbleContentsButtonRow;
+  class BubbleMenuButton;
+
+  // The function which creates the bubble once the delay is elapsed.
+  void DelayedBubbleCreation();

[sky, 2012/08/02 18:30:30]

How about CreateBubble().

[Mr4D (OOO till 08-26), 2012/08/02 23:10:38]

I thought it is clearer to show the fact that this will be done delayed. Done.

+
+  // The status of the associated window: Maximized or normal.
+  bool is_maximized() const { return is_maximized_; }
+
+  // The owning button which is also the anchor for the menu.
+  FrameMaximizeButton* frame_maximize_button_;
+
+  // The bubble menu.
+  Bubble* bubble_;
+
+  // If true the owning window is maximized.
+  const bool is_maximized_;
+
+  // The timer for the delayed creation of the menu.
+  scoped_ptr<base::Timer> timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(MaximizeBubbleController);
+};
+
+}  // namespace ash
+
+#endif  // ASH_WM_MAXIMIZE_BUBBLE_CONTROLLER_H_