Adds support for autoscrolling on drag to bookmark tree/table....

Adds support for autoscrolling on drag to bookmark tree/table.

BUG=674
TEST=none


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

[ncarter (slow), 2008-11-04 01:23:08]

http://codereview.chromium.org/9042/diff/202/40
File chrome/browser/bookmarks/bookmark_drop_info.cc (right):

http://codereview.chromium.org/9042/diff/202/40#newcode27
Line 27: scroll_up_ = (last_y <= top_margin_ + views::kAutoscrollSize);
Why does the scroll-down logic include a bounds check (last_y <
client_rect.bottom) when the scroll_up logic doesn't?

http://codereview.chromium.org/9042/diff/202/40#newcode31
Line 31: should_autoscroll =
might be better to write this as [ bool scroll_down = (); ] followed by [bool
should_autoscroll = scroll_down || scroll_up].

http://codereview.chromium.org/9042/diff/202/41
File chrome/browser/bookmarks/bookmark_drop_info.h (right):

http://codereview.chromium.org/9042/diff/202/41#newcode6
Line 6: #define CHROME_BROWSER_BOOKMARKS_BOOKMARK_DROP_INFO_
You forgot the trailing "H_".

http://codereview.chromium.org/9042/diff/202/41#newcode21
Line 21: class BookmarkDropInfo {
There doesn't seem to be much that's bookmark-specific about this class.  It's
really more like a DragAutoscrollTimer.  If you take that direction, you might
add a Delegate interface to replace the Scrolled() virtual member.

http://codereview.chromium.org/9042/diff/202/41#newcode31
Line 31: BookmarkDragData data;
Public data members in a class that has nontrivial non-data members (i.e., is
not a struct) is a violation of the style guide.  It is as if you have two
classes here -- an autoscroll timer class interlarded with a DropInfo struct. 
Can you separate them?

http://codereview.chromium.org/9042/diff/202/41#newcode44
Line 44: int drop_operation;
If it's not used at all in this class, does it belong in the class?

http://codereview.chromium.org/9042/diff/202/42
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/9042/diff/202/42#newcode149
Line 149: int PreferredDropOperation(int source_operations, int operation) {
|operation| -> |operations|

http://codereview.chromium.org/9042/diff/202/42#newcode152
Line 152: return 0;
You can delete these two lines; line 159 will handle this case.

http://codereview.chromium.org/9042/diff/202/43
File chrome/browser/bookmarks/bookmark_utils.h (right):

http://codereview.chromium.org/9042/diff/202/43#newcode24
Line 24: int PreferredDropOperation(int soure_operations, int operation);
"soure" -> "source"; "operation" -> "operations"

http://codereview.chromium.org/9042/diff/202/37
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/9042/diff/202/37#newcode1673
Line 1673: bookmark_utils::PreferredDropOperation(event.GetSourceOperations(),
ops);
80 cols.

http://codereview.chromium.org/9042/diff/202/38
File chrome/browser/views/bookmark_folder_tree_view.cc (right):

http://codereview.chromium.org/9042/diff/202/38#newcode61
Line 61: drop_info_->Update(event);
I am finding it confusing that UpdateDropInfo doesn't call drop_info_->Update().

http://codereview.chromium.org/9042/diff/202/38#newcode285
Line 285: drop_info_->drop_on = drop_on;
Overall, the design of the drop info calculation code is strained because
DropInfo encapsulates the "current" drop state, but there's no encapsulation for
the "next" drop state, so you wind up passing around <parent, index, on> tuples
that eventually get assigned into fields of DropInfo.

Would it make sense to break up FolderTreeView::DropInfo so that it contains a
FolderTreeView::DropTarget (or DropLocation, or DropParent) struct which
encapsulates the <parent, index, on> tuple?  Then SetDropParent,
CalculateDropOperation, and CalculateDropParent could be parameterized on this
type.

http://codereview.chromium.org/9042/diff/202/36
File chrome/browser/views/bookmark_folder_tree_view.h (right):

http://codereview.chromium.org/9042/diff/202/36#newcode45
Line 45: DropInfo(BookmarkFolderTreeView* view)
explicit

http://codereview.chromium.org/9042/diff/202/36#newcode47
Line 47: view_(view),
Put view_ last in the initializer list.

http://codereview.chromium.org/9042/diff/202/36#newcode64
Line 64: int drop_index;
The public data members here have the same issue as those in the parent class. 
If you move to a Delegate model for "BookmarkDropInfo/AutoscrollTimer", then
possibly BookmarkFolderTreeView could participate as the delegate, and you could
let DropInfo be a struct again.

http://codereview.chromium.org/9042/diff/202/34
File chrome/browser/views/bookmark_table_view.cc (right):

http://codereview.chromium.org/9042/diff/202/34#newcode198
Line 198: bool drop_on) {
As with the FolderTreeView, you could opt to use a <index, drop_on> tuple here
to enhance clarity.

[sky, 2008-11-04 17:27:31]

Disregard my comments about not refactoring the drop position into a struct. In
the end I did it and you were right, it makes things cleaner. I believe I got
all your other suggestions as well.

New snapshot uploaded.

http://codereview.chromium.org/9042/diff/202/40
File chrome/browser/bookmarks/bookmark_drop_info.cc (right):

http://codereview.chromium.org/9042/diff/202/40#newcode27
Line 27: scroll_up_ = (last_y <= top_margin_ + views::kAutoscrollSize);
On 2008/11/04 01:23:08, nick wrote:
> Why does the scroll-down logic include a bounds check (last_y <
> client_rect.bottom) when the scroll_up logic doesn't?

No good reason; nuked.

http://codereview.chromium.org/9042/diff/202/40#newcode31
Line 31: should_autoscroll =
On 2008/11/04 01:23:08, nick wrote:
> might be better to write this as [ bool scroll_down = (); ] followed by [bool
> should_autoscroll = scroll_down || scroll_up].

I cleaned up a bit more than this.

http://codereview.chromium.org/9042/diff/202/41
File chrome/browser/bookmarks/bookmark_drop_info.h (right):

http://codereview.chromium.org/9042/diff/202/41#newcode6
Line 6: #define CHROME_BROWSER_BOOKMARKS_BOOKMARK_DROP_INFO_
On 2008/11/04 01:23:08, nick wrote:
> You forgot the trailing "H_".

Done.

http://codereview.chromium.org/9042/diff/202/41#newcode21
Line 21: class BookmarkDropInfo {
On 2008/11/04 01:23:09, nick wrote:
> There doesn't seem to be much that's bookmark-specific about this class.  It's
> really more like a DragAutoscrollTimer.  If you take that direction, you might
> add a Delegate interface to replace the Scrolled() virtual member.

It's got the BookmarkDragData member and I need to subclass anyway, so I prefer
this approach.

http://codereview.chromium.org/9042/diff/202/41#newcode31
Line 31: BookmarkDragData data;
On 2008/11/04 01:23:09, nick wrote:
> Public data members in a class that has nontrivial non-data members (i.e., is
> not a struct) is a violation of the style guide.  It is as if you have two
> classes here -- an autoscroll timer class interlarded with a DropInfo struct. 
> Can you separate them?

I ended up making all members private and adding setters/getters. I truly hate
the style guide for a class like this that is nothing more than a glorified
helper, but there is no fighting the style guide:(

http://codereview.chromium.org/9042/diff/202/41#newcode44
Line 44: int drop_operation;
On 2008/11/04 01:23:09, nick wrote:
> If it's not used at all in this class, does it belong in the class?

This class isn't meant as a general class, it's the commonality of the two
places I need it. Hence promoting here and avoiding documentation duplication.
That's another reason why I hate the style guide in this case. This class isn't
meant for general use...

http://codereview.chromium.org/9042/diff/202/42
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/9042/diff/202/42#newcode149
Line 149: int PreferredDropOperation(int source_operations, int operation) {
On 2008/11/04 01:23:09, nick wrote:
> |operation| -> |operations|

Done.

http://codereview.chromium.org/9042/diff/202/42#newcode152
Line 152: return 0;
On 2008/11/04 01:23:09, nick wrote:
> You can delete these two lines; line 159 will handle this case.

Why not early out though?

http://codereview.chromium.org/9042/diff/202/43
File chrome/browser/bookmarks/bookmark_utils.h (right):

http://codereview.chromium.org/9042/diff/202/43#newcode24
Line 24: int PreferredDropOperation(int soure_operations, int operation);
On 2008/11/04 01:23:09, nick wrote:
> "soure" -> "source"; "operation" -> "operations"

Done.

http://codereview.chromium.org/9042/diff/202/37
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/9042/diff/202/37#newcode1673
Line 1673: bookmark_utils::PreferredDropOperation(event.GetSourceOperations(),
ops);
On 2008/11/04 01:23:09, nick wrote:
> 80 cols.

Done.

http://codereview.chromium.org/9042/diff/202/38
File chrome/browser/views/bookmark_folder_tree_view.cc (right):

http://codereview.chromium.org/9042/diff/202/38#newcode61
Line 61: drop_info_->Update(event);
On 2008/11/04 01:23:09, nick wrote:
> I am finding it confusing that UpdateDropInfo doesn't call
drop_info_->Update().

It can't as UpdateDropInfo doesn't have the event. This is why BookmarkDropInfo
caches last_y, source_ops and what not. If I scroll during drag we don't
immediately get a drag update event and things are slightly out of sync. Without
this yucky code path the drop indicator danced around during autoscroll and it
looked craptastic.

http://codereview.chromium.org/9042/diff/202/38#newcode285
Line 285: drop_info_->drop_on = drop_on;
On 2008/11/04 01:23:09, nick wrote:
> Overall, the design of the drop info calculation code is strained because
> DropInfo encapsulates the "current" drop state, but there's no encapsulation
for
> the "next" drop state, so you wind up passing around <parent, index, on>
tuples
> that eventually get assigned into fields of DropInfo.
> 
> Would it make sense to break up FolderTreeView::DropInfo so that it contains a
> FolderTreeView::DropTarget (or DropLocation, or DropParent) struct which
> encapsulates the <parent, index, on> tuple?  Then SetDropParent,
> CalculateDropOperation, and CalculateDropParent could be parameterized on this
> type.

I'm inclined to keep it the way it is, but if you feel strongly I'll move the
three parameters into a struct.

http://codereview.chromium.org/9042/diff/202/36
File chrome/browser/views/bookmark_folder_tree_view.h (right):

http://codereview.chromium.org/9042/diff/202/36#newcode45
Line 45: DropInfo(BookmarkFolderTreeView* view)
On 2008/11/04 01:23:09, nick wrote:
> explicit

Done.

http://codereview.chromium.org/9042/diff/202/36#newcode47
Line 47: view_(view),
On 2008/11/04 01:23:09, nick wrote:
> Put view_ last in the initializer list.

It now matches declaration order.

http://codereview.chromium.org/9042/diff/202/36#newcode64
Line 64: int drop_index;
On 2008/11/04 01:23:09, nick wrote:
> The public data members here have the same issue as those in the parent class.

> If you move to a Delegate model for "BookmarkDropInfo/AutoscrollTimer", then
> possibly BookmarkFolderTreeView could participate as the delegate, and you
could
> let DropInfo be a struct again.

All setters/getters now:(

http://codereview.chromium.org/9042/diff/202/34
File chrome/browser/views/bookmark_table_view.cc (right):

http://codereview.chromium.org/9042/diff/202/34#newcode198
Line 198: bool drop_on) {
On 2008/11/04 01:23:09, nick wrote:
> As with the FolderTreeView, you could opt to use a <index, drop_on> tuple here
> to enhance clarity.

I don't think it buys much in the way of clarity, but as with the tree comment
if you feel strongly I'll add the struct (or just typedef pair here).

[ncarter (slow), 2008-11-04 18:39:47]

The new design looks a lot better, and I'm happy to see that the DropPosition
idea worked out.

LGTM.

http://codereview.chromium.org/9042/diff/209/56
File chrome/browser/views/bookmark_folder_tree_view.cc (right):

http://codereview.chromium.org/9042/diff/209/56#newcode137
Line 137: BookmarkFolderTreeView::DropPosition BookmarkFolderTreeView::
Is the first scope resolution necessary?

http://codereview.chromium.org/9042/diff/209/54
File chrome/browser/views/bookmark_folder_tree_view.h (right):

http://codereview.chromium.org/9042/diff/209/54#newcode122
Line 122: int FolderIndexToBookmarkIndex(FolderNode* node, int index, bool
drop_on);
This could be made to take a DropPosition too.

http://codereview.chromium.org/9042/diff/209/52
File chrome/browser/views/bookmark_table_view.cc (right):

http://codereview.chromium.org/9042/diff/209/52#newcode280
Line 280: if (position.on) {
btw, what confused me about this function in the earlier review was that I
thought this if() was conditioned on turn_on, not drop_on.