Correctly update drag status for drags over renderer. This makes things look ...

Gtk: DnD extravaganza

- Correctly update drag status for drags over renderer. This makes things look quite a bit better.
- Rework the dnd utils so that we specify the drag targets in the order we prefer them (for destinations)
- Refactor code in bookmark bar so folder and toolbar drags share more code
- Allow bookmark bar to accept URI lists and plain text drags (for plain text we require the text to be a valid url)

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

[Elliot Glaysher, 2009-07-27 22:06:04]

Bookmarks and gtk utils parts LGTM.

[tony, 2009-07-27 22:31:02]

LGTM

http://codereview.chromium.org/159419/diff/5/13
File chrome/browser/gtk/gtk_dnd_util.h (right):

http://codereview.chromium.org/159419/diff/5/13#newcode46
Line 46: static void SetDestTargetList(GtkWidget* dest, int target_code, ...);
I personally don't like var args, but I guess this is ok.  My only suggestion is
to make a const int GtkDndUtil::END_DEST_TARGET_LIST = 0 or something so it's
more obvious that this is a var args.

[Evan Stade, 2009-07-27 22:37:06]

http://codereview.chromium.org/159419/diff/5/13
File chrome/browser/gtk/gtk_dnd_util.h (right):

http://codereview.chromium.org/159419/diff/5/13#newcode46
Line 46: static void SetDestTargetList(GtkWidget* dest, int target_code, ...);
On 2009/07/27 22:31:02, tony wrote:
> I personally don't like var args, but I guess this is ok.  My only suggestion
is
> to make a const int GtkDndUtil::END_DEST_TARGET_LIST = 0 or something so it's
> more obvious that this is a var args.

the alternative (array or vector) seemed too cumbersome on the caller side. I
thought about using INVALID_TARGET as the end of the arg list but that looks
strange on the caller side and (fwiw) doesn't match gtk style. I don't care a
whole lot though, so I'll use INVALID_TARGET as the end marker if you want.

[brettw, 2009-07-27 22:38:02]

I'd personally prefer passing an int* and calling it with something like
  int foo[3] = {FOO_A, FOO-B, 0};
  SetDestTargetList(dest, code, foo);

[tony, 2009-07-27 23:07:22]

On 2009/07/27 22:37:06, Evan Stade wrote:
> http://codereview.chromium.org/159419/diff/5/13
> File chrome/browser/gtk/gtk_dnd_util.h (right):
> 
> http://codereview.chromium.org/159419/diff/5/13#newcode46
> Line 46: static void SetDestTargetList(GtkWidget* dest, int target_code, ...);
> On 2009/07/27 22:31:02, tony wrote:
> > I personally don't like var args, but I guess this is ok.  My only
suggestion
> is
> > to make a const int GtkDndUtil::END_DEST_TARGET_LIST = 0 or something so
it's
> > more obvious that this is a var args.
> 
> the alternative (array or vector) seemed too cumbersome on the caller side. I
> thought about using INVALID_TARGET as the end of the arg list but that looks
> strange on the caller side and (fwiw) doesn't match gtk style. I don't care a
> whole lot though, so I'll use INVALID_TARGET as the end marker if you want.

I'm not sure there's a set gtk style (gtk_list_store_new specifies number of
args first, gtk_list_store_set uses -1 to terminate).  I agree that
INVALID_TARGET seems weird, which is why I suggested a new int just for this (I
guess it would be kEndDestTargetList since it's an int).

I'm ok with brettw's suggestion as well.  Or -1, which seems slightly better
than 0.

I don't have a strong opinion here.

[Evan Stade, 2009-07-27 23:22:44]

did it the brettw way

[tony, 2009-07-27 23:33:03]

LGTM

http://codereview.chromium.org/159419/diff/1012/45
File chrome/browser/gtk/bookmark_bar_gtk.cc (right):

http://codereview.chromium.org/159419/diff/1012/45#newcode51
Line 51: const int kDestTargetList[] = { GtkDndUtil::CHROME_BOOKMARK_ITEM,
Nit: Comment?

http://codereview.chromium.org/159419/diff/1012/41
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/159419/diff/1012/41#newcode57
Line 57: const int kDestTargetList[] = { GtkDndUtil::CHROME_BOOKMARK_ITEM, 0 };
Nit: Comment?