Adding new maximize menu according to spec

Adding new maximize menu according to specification.


BUG=132686
TEST=Visually tested


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

[Mr4D (OOO till 08-26), 2012-07-25 22:39:27]

Sky,

I am still waiting for some artwork to come in, but that should not hinder to
start the review.

Note two things:
1. The radial menu is pure experimental and behind a flag. I therefore made sure
that all code segments related to it can be easily found and removed.
2. The radial menu menu (display) code is the same as in our previous review
(only slightly moved)

Many thanks in advance!
-Stefan

[msw, 2012-07-25 23:11:55]

Drive-by for bubble_delegate.[h|cc], I didn't look elsewhere.

http://codereview.chromium.org/10823025/diff/4001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10823025/diff/4001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:88: // Get the arrow's anchor rect in screen
space.
nit: add a blank line above.

http://codereview.chromium.org/10823025/diff/4001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:175: // If set (default) the arrow will be
mirrored if it helps to fit the menu
nit: remove "(default)" or explicitly state "(defaults to true)". Also
s/menu/bubble/. Consider an alternate comment:
// If true, the arrow may be mirrored to fit the bubble on screen better.

[Mr4D (OOO till 08-26), 2012-07-26 01:17:20]

Addressed comments.

Have another look!

http://codereview.chromium.org/10823025/diff/4001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10823025/diff/4001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:88: // Get the arrow's anchor rect in screen
space.
On 2012/07/25 23:11:56, msw wrote:
> nit: add a blank line above.

Done.

http://codereview.chromium.org/10823025/diff/4001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:175: // If set (default) the arrow will be
mirrored if it helps to fit the menu
On 2012/07/25 23:11:56, msw wrote:
> nit: remove "(default)" or explicitly state "(defaults to true)". Also
> s/menu/bubble/. Consider an alternate comment:
> // If true, the arrow may be mirrored to fit the bubble on screen better.

Done.

[Mr4D (OOO till 08-26), 2012-07-30 23:47:05]

I assume that sky might not have time to look at this so I asked brettw to have
a look.

Many thanks in advance!

For the radial code - that was already in a previous CL which didn't make it in
(due to other reasons) but the included part seemed to be ok.

-Stefan

[tfarina, 2012-07-31 02:21:03]

On Mon, Jul 30, 2012 at 8:47 PM,  <skuhne@chromium.org> wrote:
> I assume that sky might not have time to look at this so I asked brettw to
> have
> a look.
>
For views/ash/aura, you may want to try Ben if Scott is out of office.
In case Ben is busy, you can try Sadrul or Mike as well.

-- 
Thiago

[sky, 2012-07-31 16:11:06]

As discussed, Stefan is going to pull out the radial menu changes from this cl
and move to a future cl.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc
File ash/wm/window_maximize.cc (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:5: // Needs to be included first to get the enum
resolved.
That indicates the header includes aren't set up right.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:75: // In case of dragging the menu extends till
eternity.
This comment and name don't make sense to me. Could you expand on it?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:123: owner_->width(),
Isn't owner_ the same as view? Can you remove owner_ ?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:177: to_close_->Close();
This to should move with the EventFilter.

Also, I'm don't understand the need for this. Close() on Widget is async.
Meaning it posts a task that does CloseNow then the delete. Are you sure this is
needed?

If you need to control the life time, you can make the widget owner the native
widget (ownership = WIDGET_OWNS_NATIVE_WIDGET) and then delete the widget when
you need it.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:193: virtual ~BubbleMenuButton() {};
remove ;

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:269: }
How about a bunch of elses so that you can have a NOTREACHED> Same on 281 below.
Also, if you set a tag on the buttons then you won't need to maintain fields for
each of the buttons.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:306: void
MaximizeBubble::BubbleMenuButton::OnMouseExited(
What about capture lost?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:316: : bubble_(bubble) {
Member initialize buttons_View_ and label_view_ here to NULL.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:383: // this menu gets creates as a separate window
and the SnapSizer will be
creates -> created

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:444: // We don't want to loose the focus on our parent
window.
Why do you need to do this?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:446:
owner_->frame_maximize_button()->parent()->GetWidget();
Why do you need parent() here?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h
File ash/wm/window_maximize.h (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:5: #ifndef ASH_WM_WINDOW_MAXIMIZE_H_
File name should match class name. So, this file should be named MaximizeBubble.
That said, MaximizeBubbleController is a better name.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:25: // - a helo menu (experimental behind a flag).
halo?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:28: // Note: The EventFilter dependency is only part of
the radial menu and will
Why is the EventFilter part of this class? Shouldn't radial menu handle it?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:34: bool is_maximized);
indent to match previous line.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:62: virtual bool PreHandleKeyEvent(aura::Window*
target,
indent to match previous line. If indenting makes everything not fit, then wrap
first arg. Same comment for following lines.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:97: bool is_maximized() { return is_maximized_; }
const

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:120: maximizer_.reset(NULL);
.reset()

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:128: void
FrameMaximizeButton::OnWindowBoundsChanged(
Make order match header.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:132: maximizer_.reset(NULL);
Cancel() ?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:148: if
(!window_moved_listener_installed_for_ && parent() &&
No need to check parent() here, you can invoke GetWidget() on 'this'

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:152:
window_moved_listener_installed_for_->AddObserver(this);
You need to remove the observer at the right point, especially if 'this' is
deleted and is still installed as a listener.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:188: if (!is_snap_enabled_) {
All of this should be moved to Cancel().

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:191: SnapButtonHovered(SNAP_NONE);
spacing is off.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:251: // Prepare the help / helo menu.
halo?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.h (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.h:140: aura::Window*
window_moved_listener_installed_for_;
Name this |window_| with a comment indicating this is the window_ we're
contained in.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.h (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.h:53: // Stack the phantom window
above the given window upon creation, but below
How about: If set the phantom window is stacked below this window, otherwise it
is stacked above the window passed to the constructor.

http://codereview.chromium.org/10823025/diff/4002/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10823025/diff/4002/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:84: bool try_mirroring_arrow() { return
try_mirroring_arrow_; }
const

[sky, 2012-07-31 16:11:29]

I removed Brett since he isn't appropriate for this.

[Mr4D (OOO till 08-26), 2012-08-01 20:48:21]

Addressed and radial removed. Please have another look!

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc
File ash/wm/window_maximize.cc (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:5: // Needs to be included first to get the enum
resolved.
On 2012/07/31 16:11:06, sky wrote:
> That indicates the header includes aren't set up right.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:75: // In case of dragging the menu extends till
eternity.
Done. If still unclear let me know!

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:123: owner_->width(),
The |owner_| is the content of the bubble - the |view| contains the border space
itself. I was therefore using the metrics of the |owner_| to draw the "border"
around the content. So no.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:177: to_close_->Close();
It is needed for the radial menu - will come then up with another way of
addressing this. Removed.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:193: virtual ~BubbleMenuButton() {};
On 2012/07/31 16:11:06, sky wrote:
> remove ;

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:269: }
Done here. Line 281 is actually correct the way it is. If the user hits spacing
nothing should get selected.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:306: void
MaximizeBubble::BubbleMenuButton::OnMouseExited(
I don't think we need it since a capture is given to a button only upon press.
When that happens the action is triggered and the operation is executed (and the
window gets closed). But I agree that it sounds like a good idea. Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:316: : bubble_(bubble) {
On 2012/07/31 16:11:06, sky wrote:
> Member initialize buttons_View_ and label_view_ here to NULL.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:383: // this menu gets creates as a separate window
and the SnapSizer will be
On 2012/07/31 16:11:06, sky wrote:
> creates -> created

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:444: // We don't want to loose the focus on our parent
window.
Because the button otherwise looses the "Active state" (shown to the user).
Since the bubble is only a "help" dialog and not a not a "real thing".
Furthermore I am recon that in case of a gesture the window lost the focus and
thus the gesture was killed since it did go out of focus.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.cc#n...
ash/wm/window_maximize.cc:446:
owner_->frame_maximize_button()->parent()->GetWidget();
Because we want to keep the focus on the "parent" window and not take it onto
the dialog. See above.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h
File ash/wm/window_maximize.h (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:5: #ifndef ASH_WM_WINDOW_MAXIMIZE_H_
Done & renamed.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:25: // - a helo menu (experimental behind a flag).
On 2012/07/31 16:11:06, sky wrote:
> halo?

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:28: // Note: The EventFilter dependency is only part of
the radial menu and will
No. The radial menu is a "renderer" with a hit test function. This way it is
independent of the user interaction component and can easily used for all kind
of usage scenarios (touch - n fingers down, release on m fingers up, mouse, ...
you name it). That said - one way of removing the dependency would be to create
a sub class for radial and one for normal menus and select the implementation
upon creation (which is then a local implementation detail).

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:34: bool is_maximized);
On 2012/07/31 16:11:06, sky wrote:
> indent to match previous line.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:62: virtual bool PreHandleKeyEvent(aura::Window*
target,
On 2012/07/31 16:11:06, sky wrote:
> indent to match previous line. If indenting makes everything not fit, then
wrap
> first arg. Same comment for following lines.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/window_maximize.h#ne...
ash/wm/window_maximize.h:97: bool is_maximized() { return is_maximized_; }
On 2012/07/31 16:11:06, sky wrote:
> const

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:120: maximizer_.reset(NULL);
On 2012/07/31 16:11:06, sky wrote:
> .reset()

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:128: void
FrameMaximizeButton::OnWindowBoundsChanged(
On 2012/07/31 16:11:06, sky wrote:
> Make order match header.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:132: maximizer_.reset(NULL);
On 2012/07/31 16:11:06, sky wrote:
> Cancel() ?

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:148: if
(!window_moved_listener_installed_for_ && parent() &&
Hmmm.. But I *want* to add the observer on the parent (browser window) and not
on the button itself.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:152:
window_moved_listener_installed_for_->AddObserver(this);
Please correct me if I am wrong - but isn't the
"FrameMaximizeButton::OnWindowDestroying" doing just that? Did I miss something?

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:188: if (!is_snap_enabled_) {
On 2012/07/31 16:11:06, sky wrote:
> All of this should be moved to Cancel().

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:191: SnapButtonHovered(SNAP_NONE);
On 2012/07/31 16:11:06, sky wrote:
> spacing is off.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:251: // Prepare the help / helo menu.
On 2012/07/31 16:11:06, sky wrote:
> halo?

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.h (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.h:140: aura::Window*
window_moved_listener_installed_for_;
On 2012/07/31 16:11:06, sky wrote:
> Name this |window_| with a comment indicating this is the window_ we're
> contained in.

Done.

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.h (right):

http://codereview.chromium.org/10823025/diff/4002/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.h:53: // Stack the phantom window
above the given window upon creation, but below
On 2012/07/31 16:11:06, sky wrote:
> How about: If set the phantom window is stacked below this window, otherwise
it
> is stacked above the window passed to the constructor.

Done.

http://codereview.chromium.org/10823025/diff/4002/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10823025/diff/4002/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:84: bool try_mirroring_arrow() { return
try_mirroring_arrow_; }
On 2012/07/31 16:11:06, sky wrote:
> const

Done.

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

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:36: enum RadialMenuCommands {
Remove this.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:66: const int kBubbleAppearanceDelay = 200;
 // msec
nit: name MS like you did on line 50 to make it clear.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:71: const int kHoverRadialMenuExtension =
3;
Remove the constants you aren't using in this patch.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:226: void
SetMenuState(FrameMaximizeButton::SnapType snap_type);
SetSnapType

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:230: MaximizeBubbleController* GetOwner() {
Name this owner()

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:267:
left_button_->SetImage(views::CustomButton::BS_NORMAL,
Create a function for constructor that can be shared for the three buttons.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:308: if (!bubble_->GetOwner())
Why do you need this? At a minimum add a comment (same on 323).

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:375: void
SetMenuState(FrameMaximizeButton::SnapType snap_type) {
SetSnapType()

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:426: ) OVERRIDE {
Don't wrap like this.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:620:
MaximizeBubbleController::~MaximizeBubbleController() {
Order doesn't match header.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.h (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:20: // The menu gets created every time
after an initial delay. The delay can be
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.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:24: explicit
MaximizeBubbleController(FrameMaximizeButton* frame_maximize_button,
no explicit

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:32: void
SetMenuState(FrameMaximizeButton::SnapType snap_type);
How about SetSnapType?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:34: // Return a window handle underneath the
snap animation window should
The name of this method is rather confusing. I think you mean GetBubbleWindow()
with a description to match.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:38: // Reset the delay of the menu creation
to the beg.
beg?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:41: protected:
Why is any of this protected?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:59: class Bubble;
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.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:65: void DelayedBubbleCreation();
How about CreateBubble().

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:111: maximizer_(NULL) {
no need for this.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:146: if (!window_ && parent() &&
parent()->GetWidget()) {
I think GetWidget() should be a DCHECK here (especially since line 152 would
crash if it returns NULL).

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:152:
frame_->GetWidget()->IsMaximized()));
This should be consistent with 146/147.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:161: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
The event has a location, can't you use it?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:282: // Remove our help / halo menu if
the mouse cursor is not still over it.
help/halo -> bubble

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:283: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
event has a location, use it.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:285: maximizer_.reset();
You sure we don't want to reset always? In particular seems like it would be
annoying for gestures that could leave the bubble up.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:352: snap_type_ = type;
Cancel()? Maybe you can't because you don't want the maximizer reset. If that's
the case add a boolean to Cancel for maximizer.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:359: DCHECK(false);
NOTREACHED

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:463: void
FrameMaximizeButton::ExecuteSnapAndCloseMenu(SnapType snap_type) {
Make sure order matches header.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:465: Snap();
DCHECK_NE(snap_type_, SNAP_NONE).

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:470: if (maximizer_.get())
no if. and comment is misleading since a minimize won't change the bounds.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:500: void
FrameMaximizeButton::DestroyMaximizeMenu() {
Make order match header.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:502: maximizer_.reset();
No need for the if. maximizer_.reset() does the right thing.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.h (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.h:32: enum SnapType {
Move this to its own file to minimize imports. Name the file snap_types.

http://codereview.chromium.org/10823025/diff/3019/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10823025/diff/3019/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:176: // If true, the arrow may be mirrored to
fit the bubble on screen better.
Document the default.

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

Try closely reviewing your own CLs before requesting review.
That helps you catch nits, etc. earlier and helps reviewers.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:11: #include "base/command_line.h"
nit: is this include necessary? Please only IWYU.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:16: #include "ui/aura/aura_switches.h"
nit: is this include necessary? Please only IWYU.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:17: #include "ui/base/ui_base_switches.h"
nit: is this include necessary? Please only IWYU.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:43: // Bubble constants
nit: Nix comment or extrapolate and append a period.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:45: const int kArrowOffset = 10;
nit: is this used?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:50: const int kAnimationDurationForPopupMS
= 200;
nit: move down with kBubbleAppearanceDelay or vice versa? (group time consts)

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:56: const SkColor kBubbleTextColor =
0xffffffff;
nit: Use SK_ColorWHITE (or theme service/provider if applicable).

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:58: // The line width of the bubble.
nit: s/width/height/

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:61: // The pixel dimensions of the arrow
nit: append a period.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:76: const int kDragRadialMenuExtension =
10000;
nit: is this still used?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:78: class MaximizeBubbleBorder : public
views::BubbleBorder {
Can this work be merged with existing/needed bubble border/arrow code?
Each bubble shouldn't need to implement a new custom border.

1) Make design changes (or add settings) to the default bubble border instead?
2) Merged with the launcher/tray bubble border work?
3) Sync up with the efforts behind crbug.com/135200 and crbug.com/133292?
4) Move this to a separate file for re-use elsewhere if applicable.
Or at least add Comment as to why this custom bubble border is necessary.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:80: MaximizeBubbleBorder(views::View*
content_view,
nit: Do not use inline definitions here and below.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:99: // Position the bubble to center the
box on the anchor.
Can this be accomplished via ArrowLocation and SetArrowOffset?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:100: int x = -insets.left() -
Can this be obtained from a view's origin point?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:105: gfx::Point view_topleft(x, y);
nit: is this a view origin? please rename.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:108: return gfx::Rect(view_topleft.x(),
view_topleft.y(),
nit: use Rect(const gfx::Point& origin, const gfx::Size& size);

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:113: virtual void Paint(const views::View&
view,
Again, why does this require custom code?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:176: class
MaximizeBubbleController::BubbleMenuButton : public views::ImageButton {
This seems odd, can't each button handle mouse entered/exited/move events?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:195: // It creates a "bubble border" and
the content accordingly.
nit: nix quotes around bubble border.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:197: // this menu gets creates as a
separate window and the SnapSizer will be
nit: "gets creates" and "will be creates"

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:210: // Overridden from View
nit: add a period for consistency.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:220: // Called from the owning class to
indicate that the menu should get
nit: use "controller" or owner terminology consistently.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:230: MaximizeBubbleController* GetOwner() {
Make this MaximizeBubbleController* owner() const { return owner_; }
Consider using consistent naming; controller() / controller_ / etc.?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:250: // A class that creates all buttons
and put them into a view.
This seems unnecessary, just do this in BubbleContentsView.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:271: // TODO(skuhne): Replace images as
soon as they come in.
nit: consolidate redundant TODO comments.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:502: // We don't want to loose the focus on
our parent window because the button
nit: s/loose/lose/ here and below.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:614: if (!bubble_) {
nit: remove {}

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:651: if (bubble_)
Should this instead DCHECK(bubble_) as it should only be called during bubble
lifetime?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.h (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:11: #include "ui/aura/event_filter.h"
Is this include necessary?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:20: // The menu gets created every time
after an initial delay. The delay can be
nit: "every time" what happens? Consider just appending "after a delay" to the
line above.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:27: // the menu itself will delete itself
then.
nit: redundant "itself will delete itself"

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:30: // Update the visible menu to reflect
the previewed |snap_type| snapping
nit: nix "visible" for a single line comment :)

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:34: // Return a window handle underneath the
snap animation window should
nit: grammar / clarity; did you mean "underneath which", or rephrase.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:60: class BubbleContentsView;
Are these forward decls necessary?

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:243: // Prepare the help / halo menu.
nit: what's " help / halo"? Update comment.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:403: if (maximizer_.get())
nit: if the order here isn't critical, combine this with the if block above.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:123: DCHECK(phantom_widget_ ==
NULL);
nit: DCHECK(!phantom_widget_) or DCHECK_EQ

http://codereview.chromium.org/10823025/diff/3019/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10823025/diff/3019/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5291: +      <message
name="IDS_FLAGS_TOUCH_RADIAL_MENU_UI_NAME" desc="Title of the touch-radial-menu
UI flag." >
Is this still needed?

http://codereview.chromium.org/10823025/diff/3019/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5294: +      <message
name="IDS_FLAGS_TOUCH_RADIAL_MENU_UI_DESCRIPTION" desc="Description of the
touch-optimized UI flag.">
Is this still needed?

[msw, 2012-08-02 18:58:24]

Please also update the CL title, description, and TEST=

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

Updated. Please have another look!

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:11: #include "base/command_line.h"
I removed the radial menu and it had some tentacles all over the place. The next
CL will include it again (so I have to add all that stuff back in). Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:16: #include "ui/aura/aura_switches.h"
On 2012/08/02 18:56:59, msw wrote:
> nit: is this include necessary? Please only IWYU.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:17: #include "ui/base/ui_base_switches.h"
On 2012/08/02 18:56:59, msw wrote:
> nit: is this include necessary? Please only IWYU.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:36: enum RadialMenuCommands {
On 2012/08/02 18:30:30, sky wrote:
> Remove this.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:43: // Bubble constants
On 2012/08/02 18:56:59, msw wrote:
> nit: Nix comment or extrapolate and append a period.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:45: const int kArrowOffset = 10;
On 2012/08/02 18:56:59, msw wrote:
> nit: is this used?

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:50: const int kAnimationDurationForPopupMS
= 200;
On 2012/08/02 18:56:59, msw wrote:
> nit: move down with kBubbleAppearanceDelay or vice versa? (group time consts)

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:56: const SkColor kBubbleTextColor =
0xffffffff;
On 2012/08/02 18:56:59, msw wrote:
> nit: Use SK_ColorWHITE (or theme service/provider if applicable).

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:58: // The line width of the bubble.
On 2012/08/02 18:56:59, msw wrote:
> nit: s/width/height/

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:61: // The pixel dimensions of the arrow
On 2012/08/02 18:56:59, msw wrote:
> nit: append a period.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:66: const int kBubbleAppearanceDelay = 200;
 // msec
On 2012/08/02 18:30:30, sky wrote:
> nit: name MS like you did on line 50 to make it clear.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:71: const int kHoverRadialMenuExtension =
3;
You are right. That slipped through

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:76: const int kDragRadialMenuExtension =
10000;
Was already deleted.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:78: class MaximizeBubbleBorder : public
views::BubbleBorder {
I have spent at least two to three days trying to use any of the other bubbles
and it did not work (that excludes the latest bubble efforts). Things like "only
pointing down" or "close relationships" with the tray lead to that. In the end I
was going back to the most common thing which is what you can see here. Yes, I
have seen the drawing code somewhere else, but after spending so much time
trying to use the less then 80 lines of code I gave up and added them here.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:80: MaximizeBubbleBorder(views::View*
content_view,
On 2012/08/02 18:56:59, msw wrote:
> nit: Do not use inline definitions here and below.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:99: // Position the bubble to center the
box on the anchor.
The designer requests states that the button should be flush to the right side
of the window and / or at least centered underneath the button.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:100: int x = -insets.left() -
This code has slightly changes after I got the final artwork. But again - as
requested by designers.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:105: gfx::Point view_topleft(x, y);
On 2012/08/02 18:56:59, msw wrote:
> nit: is this a view origin? please rename.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:108: return gfx::Rect(view_topleft.x(),
view_topleft.y(),
On 2012/08/02 18:56:59, msw wrote:
> nit: use Rect(const gfx::Point& origin, const gfx::Size& size);

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:113: virtual void Paint(const views::View&
view,
See above.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:176: class
MaximizeBubbleController::BubbleMenuButton : public views::ImageButton {
You only get clicked events no hover events from CustomButtons. If that is what
you are referring to.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:195: // It creates a "bubble border" and
the content accordingly.
Why is that? Done

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:197: // this menu gets creates as a
separate window and the SnapSizer will be
On 2012/08/02 18:56:59, msw wrote:
> nit: "gets creates" and "will be creates"

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:210: // Overridden from View
On 2012/08/02 18:56:59, msw wrote:
> nit: add a period for consistency.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:220: // Called from the owning class to
indicate that the menu should get
On 2012/08/02 18:56:59, msw wrote:
> nit: use "controller" or owner terminology consistently.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:226: void
SetMenuState(FrameMaximizeButton::SnapType snap_type);
On 2012/08/02 18:30:30, sky wrote:
> SetSnapType

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:230: MaximizeBubbleController* GetOwner() {
On 2012/08/02 18:30:30, sky wrote:
> Name this owner()

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:230: MaximizeBubbleController* GetOwner() {
On 2012/08/02 18:56:59, msw wrote:
> Make this MaximizeBubbleController* owner() const { return owner_; }
> Consider using consistent naming; controller() / controller_ / etc.?

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:267:
left_button_->SetImage(views::CustomButton::BS_NORMAL,
On 2012/08/02 18:30:30, sky wrote:
> Create a function for constructor that can be shared for the three buttons.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:271: // TODO(skuhne): Replace images as
soon as they come in.
Got the artwork in the meantime and added it accordingly.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:308: if (!bubble_->GetOwner())
On 2012/08/02 18:30:30, sky wrote:
> Why do you need this? At a minimum add a comment (same on 323).

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:375: void
SetMenuState(FrameMaximizeButton::SnapType snap_type) {
On 2012/08/02 18:30:30, sky wrote:
> SetSnapType()

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:426: ) OVERRIDE {
On 2012/08/02 18:30:30, sky wrote:
> Don't wrap like this.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:502: // We don't want to loose the focus on
our parent window because the button
On 2012/08/02 18:56:59, msw wrote:
> nit: s/loose/lose/ here and below.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:614: if (!bubble_) {
On 2012/08/02 18:56:59, msw wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:620:
MaximizeBubbleController::~MaximizeBubbleController() {
On 2012/08/02 18:30:30, sky wrote:
> Order doesn't match header.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:651: if (bubble_)
The short answer is: No. The bubble lives and dies independently. It can be
shown (and operated) through hovering as well as through touch events. Depending
on which kind of activation was done either the menu or the owning frame
maximize button deletes. Furthermore the deletion has to be asynchronous in many
cases (when triggered while in an event handler) which did cost me some time to
get right.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.h (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:11: #include "ui/aura/event_filter.h"
On 2012/08/02 18:56:59, msw wrote:
> Is this include necessary?

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:20: // The menu gets created every time
after an initial delay. The delay can be
On 2012/08/02 18:30:30, sky wrote:
> 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.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:20: // The menu gets created every time
after an initial delay. The delay can be
On 2012/08/02 18:56:59, msw wrote:
> nit: "every time" what happens? Consider just appending "after a delay" to the
> line above.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:24: explicit
MaximizeBubbleController(FrameMaximizeButton* frame_maximize_button,
On 2012/08/02 18:30:30, sky wrote:
> no explicit

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:27: // the menu itself will delete itself
then.
On 2012/08/02 18:56:59, msw wrote:
> nit: redundant "itself will delete itself"

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:30: // Update the visible menu to reflect
the previewed |snap_type| snapping
On 2012/08/02 18:56:59, msw wrote:
> nit: nix "visible" for a single line comment :)

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:32: void
SetMenuState(FrameMaximizeButton::SnapType snap_type);
On 2012/08/02 18:30:30, sky wrote:
> How about SetSnapType?

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:34: // Return a window handle underneath the
snap animation window should
On 2012/08/02 18:56:59, msw wrote:
> nit: grammar / clarity; did you mean "underneath which", or rephrase.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:34: // Return a window handle underneath the
snap animation window should
On 2012/08/02 18:30:30, sky wrote:
> The name of this method is rather confusing. I think you mean
GetBubbleWindow()
> with a description to match.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:38: // Reset the delay of the menu creation
to the beg.
On 2012/08/02 18:30:30, sky wrote:
> beg?

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:41: protected:
See below. I don't want to have the destruction function be available to anyone
but the Bubble class.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:59: class Bubble;
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.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:60: class BubbleContentsView;
Again, removed most of them.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.h:65: void DelayedBubbleCreation();
I thought it is clearer to show the fact that this will be done delayed. Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:111: maximizer_(NULL) {
On 2012/08/02 18:30:30, sky wrote:
> no need for this.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:146: if (!window_ && parent() &&
parent()->GetWidget()) {
On 2012/08/02 18:30:30, sky wrote:
> I think GetWidget() should be a DCHECK here (especially since line 152 would
> crash if it returns NULL).

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:152:
frame_->GetWidget()->IsMaximized()));
Ok

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:161: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
First off: I am using the mouse coordinate here and somehow I think it is faster
to simply get this then copying the coordinate and then calculating the screen
coordinate. But I tried it and using

gfx::Point screen_location(event.location());
views::View::ConvertPointToScreen(this, &screen_location);

behaved poorly (the menu was flickering). Not sure why that is though.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:243: // Prepare the help / halo menu.
On 2012/08/02 18:56:59, msw wrote:
> nit: what's " help / halo"? Update comment.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:282: // Remove our help / halo menu if
the mouse cursor is not still over it.
On 2012/08/02 18:30:30, sky wrote:
> help/halo -> bubble

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:283: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
As stated in the comment: an event comes to an end, but the mouse cursor is
independent of it. To avoid closing the menu while the mouse cursor is still
over it (note it won't come back until the mouse re-enters the button) I check
explicitly against the mouse position.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:285: maximizer_.reset();
See comment above.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:352: snap_type_ = type;
There are two distinct operations: Button hover and menu operation (and they
should not interfere). At least one thing should not happen for sure: the menu
closes when no button is hovered. So added a new parameter as requested.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:359: DCHECK(false);
On 2012/08/02 18:30:30, sky wrote:
> NOTREACHED

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:403: if (maximizer_.get())
Not for this menu - but for the other one. Okay - changed (will re-introduce
then later again)

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:463: void
FrameMaximizeButton::ExecuteSnapAndCloseMenu(SnapType snap_type) {
On 2012/08/02 18:30:30, sky wrote:
> Make sure order matches header.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:465: Snap();
I have to admit that I won't see any harm done if this would happen, but sure -
otherwise it'll scream later (for no real reason) :)

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:470: if (maximizer_.get())
On 2012/08/02 18:30:30, sky wrote:
> no if. and comment is misleading since a minimize won't change the bounds.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:500: void
FrameMaximizeButton::DestroyMaximizeMenu() {
On 2012/08/02 18:30:30, sky wrote:
> Make order match header.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:502: maximizer_.reset();
On 2012/08/02 18:30:30, sky wrote:
> No need for the if. maximizer_.reset() does the right thing.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.h (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.h:32: enum SnapType {
On 2012/08/02 18:30:30, sky wrote:
> Move this to its own file to minimize imports. Name the file snap_types.

Done.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:123: DCHECK(phantom_widget_ ==
NULL);
On 2012/08/02 18:56:59, msw wrote:
> nit: DCHECK(!phantom_widget_) or DCHECK_EQ

Done.

http://codereview.chromium.org/10823025/diff/3019/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10823025/diff/3019/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5291: +      <message
name="IDS_FLAGS_TOUCH_RADIAL_MENU_UI_NAME" desc="Title of the touch-radial-menu
UI flag." >
Good one missed that. Done.

http://codereview.chromium.org/10823025/diff/3019/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5294: +      <message
name="IDS_FLAGS_TOUCH_RADIAL_MENU_UI_DESCRIPTION" desc="Description of the
touch-optimized UI flag.">
Ditto

http://codereview.chromium.org/10823025/diff/3019/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10823025/diff/3019/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:176: // If true, the arrow may be mirrored to
fit the bubble on screen better.
I had that in there but was told to remove it. Re-added it. Done.

[sky, 2012-08-03 19:42:00]

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.cc:389: mouse_watcher_->Start();
Don't you want to wait until you know the mouse is over the bubble before
starting the watcher?

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.cc:440: if
(owner_->frame_maximize_button()->GetBoundsInScreen().Contains(
nit: make this a return, eg
return owner_->frame_max...

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.cc:623: timer_.reset(NULL);
remove NULL

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
File ash/wm/maximize_bubble_controller.h (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.h:13: class Timer;
don't indent for namespaces like this (same comment on 17).

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:146: if (!window_ && parent()) {
remove parent() here

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:147: window_ =
parent()->GetWidget()->GetNativeWindow();
remove parent() here

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:152:
frame_->GetWidget()->IsMaximized()));
remove frame_ here

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:162: if
(!maximizer_->GetBubbleWindow()->bounds().Contains(screen_location)) {
bounds() is not necessarily in the same coordinates as GetCursorScreenPoint.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:311: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
ProcessEndEvent may be invoked from a gesture event, in which case this doesn't
make sense. If we get here from a touch event you should always destroy the
maximizer.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.h (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:52: // Updates |snap_type_| based on a
a given snap type. This is used by
move all non-virtual method to above overrides.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:64: bool is_snap_enabled() { return
is_snap_enabled_; }
const

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:81: void Cancel(bool keep_menu_open);
Document |keep_menu_open|

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:120: // This is the window_ we are
contained in.
window_ -> Window

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/phantom_w...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/phantom_w...
ash/wm/workspace/phantom_window_controller.cc:127: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
This needs to change, otherwise we're going to leak the widget.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/snap_types.h
File ash/wm/workspace/snap_types.h (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/snap_type...
ash/wm/workspace/snap_types.h:10: namespace workspace {
remove this namespace. Keep it in just ash.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/snap_type...
ash/wm/workspace/snap_types.h:12: enum SnapType {
Add a description.

[msw, 2012-08-03 19:58:54]

Sorry if some of these comments no longer apply, I drafted them amid your
uploading of new patch sets. (this also wasn't a complete thorough look).

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:78: class MaximizeBubbleBorder : public
views::BubbleBorder {
On 2012/08/02 23:10:38, Mr4D wrote:
> I have spent at least two to three days trying to use any of the other bubbles
> and it did not work (that excludes the latest bubble efforts). Things like
"only
> pointing down" or "close relationships" with the tray lead to that. In the end
I
> was going back to the most common thing which is what you can see here. Yes, I
> have seen the drawing code somewhere else, but after spending so much time
> trying to use the less then 80 lines of code I gave up and added them here.

Take a look at Varun's work in codereview.chromium.org/10834140 
At least add a TODO to consolidate with that or other Bubble drawing code.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:99: // Position the bubble to center the
box on the anchor.
On 2012/08/02 23:10:38, Mr4D wrote:
> The designer requests states that the button should be flush to the right side
> of the window and / or at least centered underneath the button.

Ok, consider adding a TODO to support centered arrows via BubbleBorder.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:100: int x = -insets.left() -
On 2012/08/02 23:10:38, Mr4D wrote:
> This code has slightly changes after I got the final artwork. But again - as
> requested by designers.

Ok, this seems fine, thanks.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:176: class
MaximizeBubbleController::BubbleMenuButton : public views::ImageButton {
On 2012/08/02 23:10:38, Mr4D wrote:
> You only get clicked events no hover events from CustomButtons. If that is
what
> you are referring to.

Ah, I think I just misunderstood what you're doing here; this seems okay.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:195: // It creates a "bubble border" and
the content accordingly.
On 2012/08/02 23:10:38, Mr4D wrote:
> Why is that? Done

I typically reserve quotes for actual quotations, or rarely for terminology that
belies its actual usage/implementation/intent, but this class literally creates
a BubbleBorder subclass instance (etc.), right? So why use quotes? If you're
alluding to a noteworthy distinction from typical bubble borders (ex/ square
corners), then mention that instead of implying something vague via quotes.

http://codereview.chromium.org/10823025/diff/3019/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:250: // A class that creates all buttons
and put them into a view.
On 2012/08/02 18:56:59, msw wrote:
> This seems unnecessary, just do this in BubbleContentsView.

I still think this separate class is somewhat unnecessary.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:35: const int kLayoutSpacing = -1;
Why did this become negative since a previous Patch Set?

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:40: // The text color within the bubble
nit: add a period.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:161: // The constructor also assigns all
button states and
nit: "and"...

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:497: // Overridden from ButtonListener.
nit: move comment to declaration.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:501: // While shutting down, the connection
to the owner might already be
nit: this comment fits on one line.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:506:
bubble_->controller()->OnButtonClicked(workspace::SNAP_LEFT);
Please keep terminology consistent, rename the controller function
OnButtonPressed if possible.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:511: else {
Remove the braces or add them to the other blocks here.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:516: // Called from BubbleDialogButton.
nit: move comment to declaration.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:517: void
BubbleContentsButtonRow::ButtonHovered(
nit: this signature fits on one line.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:519: // While shutting down, the connection
to the owner might already be
nit: this comment fits on one line.

http://codereview.chromium.org/10823025/diff/4004/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:566: return;
nit: break instead of return here and below.

[Mr4D (OOO till 08-26), 2012-08-03 20:39:28]

Addressed! Please have another look!

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.cc:389: mouse_watcher_->Start();
No. Because of the same reason why I need this in the first place: There is no
event for entering the border of the bubble. So I don't know when to start.
(well it's a bit more complicated then that, but I don't get an event).

That said - this is raising a good point since the menu could also have been
started by the means of a gesture. I changed that accordingly.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.cc:440: if
(owner_->frame_maximize_button()->GetBoundsInScreen().Contains(
Thought about that, but I was under the impression that that would not be
desired. Changed! :)

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.cc:623: timer_.reset(NULL);
On 2012/08/03 19:42:00, sky wrote:
> remove NULL

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
File ash/wm/maximize_bubble_controller.h (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/maximize_bubble_con...
ash/wm/maximize_bubble_controller.h:13: class Timer;
On 2012/08/03 19:42:00, sky wrote:
> don't indent for namespaces like this (same comment on 17).

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:146: if (!window_ && parent()) {
Okay. I don't quite get this. Why is the widget of the button the complete
window? Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:147: window_ =
parent()->GetWidget()->GetNativeWindow();
On 2012/08/03 19:42:00, sky wrote:
> remove parent() here

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:152:
frame_->GetWidget()->IsMaximized()));
Fascinating. How is it possible that the widget of the button is the same widget
as the frame_? Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:162: if
(!maximizer_->GetBubbleWindow()->bounds().Contains(screen_location)) {
The Bubble window is a top level window and the coordinates should therefore be
relative to the screen. That said: I changed it now to use GetBoundsInScreen.
Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:311: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
As said in your earlier review: The idea is to keep the menu open if the mouse
is also over the button. This makes sense since the menu would otherwise not
show up until the user leaves and re-enters the button. Discussing this
yesterday it made sense to you. Still want to get it removed?

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.h (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:52: // Updates |snap_type_| based on a
a given snap type. This is used by
On 2012/08/03 19:42:00, sky wrote:
> move all non-virtual method to above overrides.

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:64: bool is_snap_enabled() { return
is_snap_enabled_; }
On 2012/08/03 19:42:00, sky wrote:
> const

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:81: void Cancel(bool keep_menu_open);
On 2012/08/03 19:42:00, sky wrote:
> Document |keep_menu_open|

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.h:120: // This is the window_ we are
contained in.
On 2012/08/03 19:42:00, sky wrote:
> window_ -> Window

Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/phantom_w...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/phantom_w...
ash/wm/workspace/phantom_window_controller.cc:127: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
Ahhh. Interesting! Thanks for figuring that out! Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/snap_types.h
File ash/wm/workspace/snap_types.h (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/snap_type...
ash/wm/workspace/snap_types.h:10: namespace workspace {
Odd - recently someone told me that the namespace should follow the directories.
I guess there are then some exceptions to the rule after all. Done.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/snap_type...
ash/wm/workspace/snap_types.h:12: enum SnapType {
On 2012/08/03 19:42:00, sky wrote:
> Add a description.

Done.

[sky, 2012-08-03 20:43:08]

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:311: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
On 2012/08/03 20:39:28, Mr4D wrote:
> As said in your earlier review: The idea is to keep the menu open if the mouse
> is also over the button. This makes sense since the menu would otherwise not
> show up until the user leaves and re-enters the button. Discussing this
> yesterday it made sense to you. Still want to get it removed?

We hide the mouse cursor when you touch, so it doesn't make sense to do anything
based on the mouse when in touch. Lets say you touch the button and at the end
of the gesture the mouse happens to be in the button. This would result in the
maximizer staying around when the mouse cursor isn't visible. That would be
confusing.

[Mr4D (OOO till 08-26), 2012-08-03 20:47:31]

Okay, done. Please have another look!

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:311: gfx::Point screen_location =
gfx::Screen::GetCursorScreenPoint();
I suppose it is either way confusing. Okay, will do as you were asking for.

[sky, 2012-08-03 23:10:16]

LGTM

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:146: if (!window_ && parent()) {
On 2012/08/03 20:39:28, Mr4D wrote:
> Okay. I don't quite get this. Why is the widget of the button the complete
> window? Done.

That's how the hierarchy is set up. Most views share the same widget.

http://codereview.chromium.org/10823025/diff/13002/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:162: if
(!maximizer_->GetBubbleWindow()->bounds().Contains(screen_location)) {
On 2012/08/03 20:39:28, Mr4D wrote:
> The Bubble window is a top level window and the coordinates should therefore
be
> relative to the screen. That said: I changed it now to use GetBoundsInScreen.
> Done.

toplevel window and screen coordinates are two different things.

http://codereview.chromium.org/10823025/diff/1092/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823025/diff/1092/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:127: params.ownership =
views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET;
You can remove this line NATIVE_WIDGET_OWNS_WIDGET is the default.

[Mr4D (OOO till 08-26), 2012-08-04 00:42:23]

Sky, many thanks for your review! I was however running into one last minute
crash with full screen applications. Minor tweaks, but please have a look before
I commit!

http://codereview.chromium.org/10823025/diff/1092/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823025/diff/1092/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:127: params.ownership =
views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET;
On 2012/08/03 23:10:17, sky wrote:
> You can remove this line NATIVE_WIDGET_OWNS_WIDGET is the default.

Done.

[msw, 2012-08-04 01:21:30]

IIRC, the bots may have trouble with a patch that contains new image resources
as well as their IDS & usage.

http://codereview.chromium.org/10823025/diff/7010/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10823025/diff/7010/ash/ash.gyp#newcode257
ash/ash.gyp:257: 'wm/maximize_bubble_controller.cc',
nit: alphabetize maximize_* before overlay_*.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:11: #include "base/bind.h"
nit: is this include necessary?

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:12: #include "base/message_loop.h"
nit: is this include necessary?

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:38: // The background color
nit: period or remove comment (name is self-explanatory)

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:41: // The text color within the bubble
nit: period or remove comment (name is self-explanatory)

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:65: // Overridden from views::Border
nit: period or colon

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:93: int x = (-border_size.width() +
anchor_->width())/2;
nit: spaces around division '/'

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:243: : public views::View,
nit: move to the line above.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:506: // broken.
nit: fits on line above.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:515: else {
nit: remove {} to be consistent with the other blocks.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:523: // broken.
nit: fits on line above.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:567:
label_view_->SetText(rb.GetLocalizedString(IDS_ASH_SNAP_WINDOW_LEFT));
Consider setting a local int string_id = ... for each case then doing
label_view_->SetText(rb.GetLocalizedString(...) just once below.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/7010/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:416: SnapType type) const {
nit: fits on line above.

[Mr4D (OOO till 08-26), 2012-08-04 02:56:22]

Addressed

http://codereview.chromium.org/10823025/diff/7010/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10823025/diff/7010/ash/ash.gyp#newcode257
ash/ash.gyp:257: 'wm/maximize_bubble_controller.cc',
On 2012/08/04 01:21:30, msw wrote:
> nit: alphabetize maximize_* before overlay_*.

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
File ash/wm/maximize_bubble_controller.cc (right):

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:11: #include "base/bind.h"
On 2012/08/04 01:21:30, msw wrote:
> nit: is this include necessary?

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:12: #include "base/message_loop.h"
On 2012/08/04 01:21:30, msw wrote:
> nit: is this include necessary?

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:38: // The background color
On 2012/08/04 01:21:30, msw wrote:
> nit: period or remove comment (name is self-explanatory)

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:41: // The text color within the bubble
On 2012/08/04 01:21:30, msw wrote:
> nit: period or remove comment (name is self-explanatory)

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:65: // Overridden from views::Border
On 2012/08/04 01:21:30, msw wrote:
> nit: period or colon

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:93: int x = (-border_size.width() +
anchor_->width())/2;
On 2012/08/04 01:21:30, msw wrote:
> nit: spaces around division '/'

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:243: : public views::View,
On 2012/08/04 01:21:30, msw wrote:
> nit: move to the line above.

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:506: // broken.
On 2012/08/04 01:21:30, msw wrote:
> nit: fits on line above.

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:515: else {
On 2012/08/04 01:21:30, msw wrote:
> nit: remove {} to be consistent with the other blocks.

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:523: // broken.
On 2012/08/04 01:21:30, msw wrote:
> nit: fits on line above.

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/maximize_bubble_cont...
ash/wm/maximize_bubble_controller.cc:567:
label_view_->SetText(rb.GetLocalizedString(IDS_ASH_SNAP_WINDOW_LEFT));
On 2012/08/04 01:21:30, msw wrote:
> Consider setting a local int string_id = ... for each case then doing
> label_view_->SetText(rb.GetLocalizedString(...) just once below.

Done.

http://codereview.chromium.org/10823025/diff/7010/ash/wm/workspace/frame_maxi...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/10823025/diff/7010/ash/wm/workspace/frame_maxi...
ash/wm/workspace/frame_maximize_button.cc:416: SnapType type) const {
On 2012/08/04 01:21:30, msw wrote:
> nit: fits on line above.

Done.

[msw, 2012-08-04 03:11:55]

Please provide a meaningful CL description.

I still think some code could be merged with existing bubble code and generally
simplified (I'm happy to review/advise on followup CLs). Again, you may have
trouble landing the assets and usage simultaneously. But you have Scott's
approval and I don't see any more glaring nits, so fire away :)

[commit-bot: I haz the power, 2012-08-04 03:20:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10823025/19003

[commit-bot: I haz the power, 2012-08-04 04:39:41]

Change committed as 150012