[Mac] Enables drag N' drop for the buttons within the Browser Actions contain...

[Mac] Enables drag N' drop for the buttons within the Browser Actions container. Also fixes an issue where the grippy was being shown when no Browser Actions were installed.
Known issue: You can drag the buttons outside of the container, even though they will recover by snapping back into place. This will be fixed in a further revision.

TEST=try dragging browser action buttons in order to reorder them.
BUG=26990
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42845

[Mark Mentovai, 2010-03-26 21:47:29]

LG otherwise

http://codereview.chromium.org/1418003/diff/1/3
File chrome/browser/cocoa/extensions/browser_action_button.h (right):

http://codereview.chromium.org/1418003/diff/1/3#newcode42
chrome/browser/cocoa/extensions/browser_action_button.h:42: // We use this
animation object to move the button and query whether a button
we

I bet if you reworded without “we use this animation object,” you could fit the
comment on a single line.

http://codereview.chromium.org/1418003/diff/1/3#newcode46
chrome/browser/cocoa/extensions/browser_action_button.h:46: // Whether the
button is currently being dragged.
Better potential for struct packing if this comes after tabId_, unless you put
it here because you think it belongs here.

http://codereview.chromium.org/1418003/diff/1/4
File chrome/browser/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/1418003/diff/1/4#newcode43
chrome/browser/cocoa/extensions/browser_action_button.mm:43: const CGFloat
kAnimationDuration = 0.2;
It’s nice to put units in the constant names.  kAnimationDurationSeconds?

http://codereview.chromium.org/1418003/diff/1/4#newcode189
chrome/browser/cocoa/extensions/browser_action_button.mm:189: // mouse-up).  So
we check.
The sentence containing “we” isn’t even necessary.

http://codereview.chromium.org/1418003/diff/1/4#newcode209
chrome/browser/cocoa/extensions/browser_action_button.mm:209: object:self];
Line your colon up.

http://codereview.chromium.org/1418003/diff/1/5
File chrome/browser/cocoa/extensions/browser_actions_container_view.mm (right):

http://codereview.chromium.org/1418003/diff/1/5#newcode59
chrome/browser/cocoa/extensions/browser_actions_container_view.mm:59: if (![self
isHidden])
Necessary?  Doesn’t -setHidden: check this anyway?

http://codereview.chromium.org/1418003/diff/1/6
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/1418003/diff/1/6#newcode50
chrome/browser/cocoa/extensions/browser_actions_controller.mm:50: // Creates and
then adds the given extension's action button to the container
This whole part of the file is hard to read because there aren’t enough blank
lines.

http://codereview.chromium.org/1418003/diff/1/6#newcode612
chrome/browser/cocoa/extensions/browser_actions_controller.mm:612: CGFloat
dragThreshold = floor(kBrowserActionWidth / 2);
#include <cmath> and call std::floor to make sure you get the right (C++) floor?

[Bons, 2010-03-26 22:10:29]

Blank lines are present in my client but not uploaded yet. Waiting for try-bots
and then will submit.

Thanks, Mark.

http://codereview.chromium.org/1418003/diff/1/3
File chrome/browser/cocoa/extensions/browser_action_button.h (right):

http://codereview.chromium.org/1418003/diff/1/3#newcode42
chrome/browser/cocoa/extensions/browser_action_button.h:42: // We use this
animation object to move the button and query whether a button
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> we
> 
> I bet if you reworded without “we use this animation object,” you could fit
the
> comment on a single line.

Done.

http://codereview.chromium.org/1418003/diff/1/3#newcode46
chrome/browser/cocoa/extensions/browser_action_button.h:46: // Whether the
button is currently being dragged.
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> Better potential for struct packing if this comes after tabId_, unless you put
> it here because you think it belongs here.

Done.

http://codereview.chromium.org/1418003/diff/1/4
File chrome/browser/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/1418003/diff/1/4#newcode189
chrome/browser/cocoa/extensions/browser_action_button.mm:189: // mouse-up).  So
we check.
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> The sentence containing “we” isn’t even necessary.

Done.

http://codereview.chromium.org/1418003/diff/1/4#newcode209
chrome/browser/cocoa/extensions/browser_action_button.mm:209: object:self];
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> Line your colon up.

Done.

http://codereview.chromium.org/1418003/diff/1/5
File chrome/browser/cocoa/extensions/browser_actions_container_view.mm (right):

http://codereview.chromium.org/1418003/diff/1/5#newcode59
chrome/browser/cocoa/extensions/browser_actions_container_view.mm:59: if (![self
isHidden])
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> Necessary?  Doesn’t -setHidden: check this anyway?

Done.

http://codereview.chromium.org/1418003/diff/1/6
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/1418003/diff/1/6#newcode50
chrome/browser/cocoa/extensions/browser_actions_controller.mm:50: // Creates and
then adds the given extension's action button to the container
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> This whole part of the file is hard to read because there aren’t enough blank
> lines.

Done.

http://codereview.chromium.org/1418003/diff/1/6#newcode612
chrome/browser/cocoa/extensions/browser_actions_controller.mm:612: CGFloat
dragThreshold = floor(kBrowserActionWidth / 2);
On 2010/03/26 21:47:30, Mark Mentovai wrote:
> #include <cmath> and call std::floor to make sure you get the right (C++)
floor?

Done.