Downward drag after mouse-down is now causing bookmark bar folders to open, r...

Downward drag after mouse-down is now causing bookmark bar folders to open, rather than drag. The bar folder can still be dragged in any other direction.

BUG=9016
TEST=Open bookmark bar. Select folder button and try to drag it down. Observe that now if dragged straight downward, the folder button will open its menu.

[Ben Goodger (Google), 2009-12-02 05:42:24]

Does this activate the item you release the mouse on? I don't think
the change has value independent of that behavior.

-Ben

On Tue, Dec 1, 2009 at 9:22 PM,  <zelidrag@chromium.org> wrote:
> Reviewers: Ben Goodger,
>
> Description:
> Downward drag after mouse-down is now causing bookmark bar folders to open,
> rather than drag. The bar folder can still be dragged in any other
> direction.
>
> BUG=9016
> TEST=Open bookmark bar. Select folder button and try to drag it down.
> Observe
> that now if dragged straight downward, the folder button will open its menu.
>
>
> Please review this at http://codereview.chromium.org/463001
>
> SVN Base: http://src.chromium.org/svn/trunk/src/
>
> Affected files:
>  M     chrome/browser/views/bookmark_bar_view.h
>  M     chrome/browser/views/bookmark_bar_view.cc
>  M     chrome/browser/views/toolbar_view.h
>  M     views/view.h
>  M     views/view.cc
>
>
>

[Ben Goodger (Google), 2009-12-02 21:34:22]

Sky can give final LGTM, but these are my comments...

http://codereview.chromium.org/463001/diff/12/16
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/463001/diff/12/16#newcode1139
chrome/browser/views/bookmark_bar_view.cc:1139: if (!sender) {
nit for here and elsewhere in this CL: no braces for single line ifs.

http://codereview.chromium.org/463001/diff/12/14
File views/view.h (right):

http://codereview.chromium.org/463001/diff/12/14#newcode89
views/view.h:89: // Returns true if drag operation can be started.
Can you note what press_x,y and x,y are?

[zel, 2009-12-02 22:08:53]

http://codereview.chromium.org/463001/diff/12/16
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/463001/diff/12/16#newcode1139
chrome/browser/views/bookmark_bar_view.cc:1139: if (!sender) {
On 2009/12/02 21:34:23, Ben Goodger wrote:
> nit for here and elsewhere in this CL: no braces for single line ifs.

Done.

http://codereview.chromium.org/463001/diff/12/14
File views/view.h (right):

http://codereview.chromium.org/463001/diff/12/14#newcode89
views/view.h:89: // Returns true if drag operation can be started.
On 2009/12/02 21:34:23, Ben Goodger wrote:
> Can you note what press_x,y and x,y are?

Done.

[sky, 2009-12-02 22:32:45]

http://codereview.chromium.org/463001/diff/12/16
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/463001/diff/12/16#newcode1139
chrome/browser/views/bookmark_bar_view.cc:1139: if (!sender) {
Why the null check here?

http://codereview.chromium.org/463001/diff/12/16#newcode1149
chrome/browser/views/bookmark_bar_view.cc:1149: if
(!View::ExceededDragThreshold(press_x - x, 0) && press_y < y) {
This check is cheap. Can we do it first so that we only loop through the buttons
if necessary.

http://codereview.chromium.org/463001/diff/12/16#newcode1150
chrome/browser/views/bookmark_bar_view.cc:1150: views::MenuButton* menu_button =
GetMenuButtonForNode(node);
Cast 'sender' rather than GetMenuButtonForNode.

http://codereview.chromium.org/463001/diff/12/14
File views/view.h (right):

http://codereview.chromium.org/463001/diff/12/14#newcode89
views/view.h:89: // Returns true if drag operation can be started.
nit: 'a' drag operation.

[zel, 2009-12-02 23:37:56]

http://codereview.chromium.org/463001/diff/12/16
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/463001/diff/12/16#newcode1139
chrome/browser/views/bookmark_bar_view.cc:1139: if (!sender) {
On 2009/12/02 22:32:45, sky wrote:
> Why the null check here?

A general defensive programming habit. Yes, NULL is not going to cause any
damage below. I guess I can remove it then.

Done.

http://codereview.chromium.org/463001/diff/12/16#newcode1149
chrome/browser/views/bookmark_bar_view.cc:1149: if
(!View::ExceededDragThreshold(press_x - x, 0) && press_y < y) {
On 2009/12/02 22:32:45, sky wrote:
> This check is cheap. Can we do it first so that we only loop through the
buttons
> if necessary.

Makes sense. Done.

http://codereview.chromium.org/463001/diff/12/16#newcode1150
chrome/browser/views/bookmark_bar_view.cc:1150: views::MenuButton* menu_button =
GetMenuButtonForNode(node);
On 2009/12/02 22:32:45, sky wrote:
> Cast 'sender' rather than GetMenuButtonForNode.

Done.

http://codereview.chromium.org/463001/diff/12/14
File views/view.h (right):

http://codereview.chromium.org/463001/diff/12/14#newcode89
views/view.h:89: // Returns true if drag operation can be started.
On 2009/12/02 22:32:45, sky wrote:
> nit: 'a' drag operation.

Done.

[Ben Goodger (Google), 2009-12-03 00:41:33]

On Wed, Dec 2, 2009 at 3:37 PM,  <zelidrag@chromium.org> wrote:
> http://codereview.chromium.org/463001/diff/12/16
> File chrome/browser/views/bookmark_bar_view.cc (right):
>
> http://codereview.chromium.org/463001/diff/12/16#newcode1139
> chrome/browser/views/bookmark_bar_view.cc:1139: if (!sender) {
> On 2009/12/02 22:32:45, sky wrote:
>>
>> Why the null check here?
>
> A general defensive programming habit. Yes, NULL is not going to cause
> any damage below. I guess I can remove it then.

In general in Chrome, better to crash unexpectedly and get a stack via
the crash reporting tools than to fail silently and later encounter
weird side effects. In this case, it's expected that only View calls
this function.

-Ben

[sky, 2009-12-03 21:39:21]

LGTM