Adding drag-drop support in and out of the Browser Action overflow menu....

Adding drag-drop support in and out of the Browser Action overflow menu.

BUG=26990
TEST=Drag icons into and out of (and within) both the Browser Action container and its overflow menu.


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

[sky, 2010-02-04 00:11:55]

http://codereview.chromium.org/570014/diff/1/3
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/570014/diff/1/3#newcode404
chrome/browser/views/browser_actions_container.cc:404:
DCHECK(!show_folder_drop_menu_task_);
Could you move this code some where common, perhaps view.

http://codereview.chromium.org/570014/diff/1/3#newcode674
chrome/browser/views/browser_actions_container.cc:674: SchedulePaint();
Do you always need to SchedulePaint? Or only when drop_indicator_Position_
changes?

http://codereview.chromium.org/570014/diff/1/3#newcode681
chrome/browser/views/browser_actions_container.cc:681: overflow_menu_ = NULL;
I'm a little worried that you might end up trying to show the overflow menu
while it's already up. I think it would be safer to have a callback from the
menu when it's deleted so that you know it's not visible anymore.

http://codereview.chromium.org/570014/diff/1/2
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/570014/diff/1/2#newcode345
chrome/browser/views/browser_actions_container.h:345: class
ShowFolderDropMenuTask : public Task {
I never converted bookmark bar over to ScopedRunnableMethodFactory, but it's
simpler to use than this. You should consider using it here.

http://codereview.chromium.org/570014/diff/1/4
File chrome/browser/views/extensions/browser_action_overflow_menu_controller.h
(right):

http://codereview.chromium.org/570014/diff/1/4#newcode24
chrome/browser/views/extensions/browser_action_overflow_menu_controller.h:24:
virtual ~BrowserActionOverflowMenuController();
You should make this private now as this class deletes itself.

It's also worth mentioning the class manages it's lifetime in the class
description.

http://codereview.chromium.org/570014/diff/1/4#newcode89
chrome/browser/views/extensions/browser_action_overflow_menu_controller.h:89:
bool for_drop_;
I think you end up deleting the controller in both cases, right?

[Finnur, 2010-02-04 20:53:16]

All addressed and added a few files (see files without a delta above).

[sky, 2010-02-04 20:58:49]

LGTM

http://codereview.chromium.org/570014/diff/5002/4004
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/570014/diff/5002/4004#newcode368
chrome/browser/views/browser_actions_container.h:368: void SetDropIndicator(int
x_pos);
x_pos is mildly confusing here as it isn't the x-coordinate you're passing in,
which x_pos sounds like. I think position would be clearer. But if you disagree,
leave what you have.

http://codereview.chromium.org/570014/diff/5002/4008
File views/view.h (right):

http://codereview.chromium.org/570014/diff/5002/4008#newcode142
views/view.h:142: static const int kShowFolderDropMenuDelay;
Does this really need to be public? Also, it should be after after the methods.