Part 1 of dragging extensions on the shelf. This part was just about getting...

Part 1 of dragging extensions on the shelf.  This part was just about getting the mechanics of the shelf handle and the dragging correct.  Part 2 will actually allow the order to be changed.TEST=hover over an extension toolstrip, grab the handle and drag.  release.

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

[sky, 2009-06-03 21:21:27]

http://codereview.chromium.org/119103/diff/14/16
File chrome/browser/extensions/extension_shelf.cc (left):

http://codereview.chromium.org/119103/diff/14/16#oldcode97
Line 97: AddChildView(title_);
Now that you're no longer adding title_ as a child view, it's going to leak. You
could make it a non-pointer or use a scoped_pointer.

http://codereview.chromium.org/119103/diff/14/16
File chrome/browser/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/119103/diff/14/16#newcode20
Line 20: #include "views/screen.h"
nit: screen should be before views/widget/root_view.h

http://codereview.chromium.org/119103/diff/14/16#newcode111
Line 111: if (!extension_view_->extension()) {
nit: no braces.

http://codereview.chromium.org/119103/diff/14/16#newcode156
Line 156: SizeToPreferredSize();
Invoking SizeToPreferredSize here makes me nervous. Layout isn't meant to change
the bounds of the receiver, which is what you may be doing with this call.

http://codereview.chromium.org/119103/diff/14/16#newcode185
Line 185: if (y_delta > kDragThreshold) {
Can you use GetVerticalDragThreshold here?

http://codereview.chromium.org/119103/diff/14/16#newcode206
Line 206: bool canceled) {
Does it make sense to treat canceled as revert to original location?

http://codereview.chromium.org/119103/diff/14/16#newcode464
Line 464: drag_placeholder_view_ = NULL;
Is drag_placeholder_view_ leaked here?

http://codereview.chromium.org/119103/diff/14/21
File chrome/browser/views/browser_bubble.cc (right):

http://codereview.chromium.org/119103/diff/14/21#newcode41
Line 41: BrowserView::GetBrowserViewForNativeWindow(frame_native_view_);
nit: spacing

http://codereview.chromium.org/119103/diff/14/21#newcode51
Line 51: BrowserView::GetBrowserViewForNativeWindow(frame_native_view_);
nit: spacing

[Erik does not do reviews, 2009-06-03 22:15:14]

new snapshot uploaded

http://codereview.chromium.org/119103/diff/14/16
File chrome/browser/extensions/extension_shelf.cc (left):

http://codereview.chromium.org/119103/diff/14/16#oldcode97
Line 97: AddChildView(title_);
On 2009/06/03 21:21:27, sky wrote:
> Now that you're no longer adding title_ as a child view, it's going to leak.
You
> could make it a non-pointer or use a scoped_pointer.

good catch.

http://codereview.chromium.org/119103/diff/14/16
File chrome/browser/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/119103/diff/14/16#newcode156
Line 156: SizeToPreferredSize();
On 2009/06/03 21:21:27, sky wrote:
> Invoking SizeToPreferredSize here makes me nervous. Layout isn't meant to
change
> the bounds of the receiver, which is what you may be doing with this call.

OK.  I moved the calls to a safer location.

http://codereview.chromium.org/119103/diff/14/16#newcode185
Line 185: if (y_delta > kDragThreshold) {
On 2009/06/03 21:21:27, sky wrote:
> Can you use GetVerticalDragThreshold here?

thanks.  didn't know about that.

http://codereview.chromium.org/119103/diff/14/16#newcode206
Line 206: bool canceled) {
On 2009/06/03 21:21:27, sky wrote:
> Does it make sense to treat canceled as revert to original location?

Yes, but since I don't actually do anything other than cancel on drop, I didn't
think to do anything else here.  I added the cancel bool to DropExtension so
that I can handle the cancel case when Drop does something useful.

http://codereview.chromium.org/119103/diff/14/16#newcode464
Line 464: drag_placeholder_view_ = NULL;
On 2009/06/03 21:21:27, sky wrote:
> Is drag_placeholder_view_ leaked here?

yep.  I thought RemoveChildView deleted it since it was parent owned.

[sky, 2009-06-03 22:58:30]

LGTM