More web contents dragging.

More web contents dragging.

TEST=drag an image (such as the google logo from google.com) into gimp or onto the desktop.
BUG=http://crbug.com/15429

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

[Evan Stade, 2009-06-29 19:37:38]

http://codereview.chromium.org/151033/diff/14/15
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/151033/diff/14/15#newcode415
Line 415: // TODO(estade): change relative links to be absolute using
is there some easy way to do this?

[tony, 2009-06-29 20:22:57]

http://codereview.chromium.org/151033/diff/14/16
File chrome/browser/gtk/dnd_registry.h (right):

http://codereview.chromium.org/151033/diff/14/16#newcode11
Line 11: namespace dnd {
Ick, we should not be adding more namespaces like this.  We're trying to follow
the google3 style, which is to name them as the full path (e.g.,
chrome_browser_gtk_dnd_registry).  However, that's long and makes the namespace
less useful, so instead, we normally just make a class with static members.  So
rather than the dnd namespace, something like "class GtkDndRegistry" or class
"GtkDragDrop" or something.

http://codereview.chromium.org/151033/diff/14/15
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/151033/diff/14/15#newcode415
Line 415: // TODO(estade): change relative links to be absolute using
On 2009/06/29 19:37:39, Evan Stade wrote:
> is there some easy way to do this?

I didn't think we needed to do this resolving ourselves.  I thought the app that
received the drop would do the resolving, no?

[Evan Stade, 2009-06-29 20:29:01]

http://codereview.chromium.org/151033/diff/14/16
File chrome/browser/gtk/dnd_registry.h (right):

http://codereview.chromium.org/151033/diff/14/16#newcode11
Line 11: namespace dnd {
On 2009/06/29 20:22:57, tony wrote:
> Ick, we should not be adding more namespaces like this.  We're trying to
follow
> the google3 style, which is to name them as the full path (e.g.,
> chrome_browser_gtk_dnd_registry).  However, that's long and makes the
namespace
> less useful, so instead, we normally just make a class with static members. 
So
> rather than the dnd namespace, something like "class GtkDndRegistry" or class
> "GtkDragDrop" or something.

There are lots of *_util{s,} namespaces. Can we make this dnd_util or similar?

http://codereview.chromium.org/151033/diff/14/15
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/151033/diff/14/15#newcode415
Line 415: // TODO(estade): change relative links to be absolute using
On 2009/06/29 20:22:57, tony wrote:
> On 2009/06/29 19:37:39, Evan Stade wrote:
> > is there some easy way to do this?
> 
> I didn't think we needed to do this resolving ourselves.  I thought the app
that
> received the drop would do the resolving, no?
> 

no, there's no standard for passing along some HTML and the base url. Firefox
passes "text/html" with the links all absolute.

[tony, 2009-06-29 20:43:21]

http://codereview.chromium.org/151033/diff/14/16
File chrome/browser/gtk/dnd_registry.h (right):

http://codereview.chromium.org/151033/diff/14/16#newcode11
Line 11: namespace dnd {
On 2009/06/29 20:29:01, Evan Stade wrote:
> 
> There are lots of *_util{s,} namespaces. Can we make this dnd_util or similar?

No, we stopped adding _util namespaces a long time ago.  I'm not sure how
gtk_util got by, but it shouldn't have.  The old ones exist because no one has
taken the time to clean them up.  In short, you can't add new namespaces unless
you follow the style guide.

http://codereview.chromium.org/151033/diff/14/15
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/151033/diff/14/15#newcode415
Line 415: // TODO(estade): change relative links to be absolute using
On 2009/06/29 20:29:01, Evan Stade wrote:
> On 2009/06/29 20:22:57, tony wrote:
> > On 2009/06/29 19:37:39, Evan Stade wrote:
> > > is there some easy way to do this?
> > 
> > I didn't think we needed to do this resolving ourselves.  I thought the app
> that
> > received the drop would do the resolving, no?
> > 
> 
> no, there's no standard for passing along some HTML and the base url. Firefox
> passes "text/html" with the links all absolute.

I see.  We should try to normalize this in webkit then before we pass the data
through to the browser process.  I guess this isn't a problem on windows because
most apps will just use CF_HTML.

[Evan Stade, 2009-06-29 21:18:29]

fixed

[tony, 2009-06-29 21:28:58]

LGTM once I understand what the mime_util.h change is for.

http://codereview.chromium.org/151033/diff/36/41
File base/mime_util.h (right):

http://codereview.chromium.org/151033/diff/36/41#newcode21
Line 21: std::string GetDataMimeType(const std::string& data);
Is this file supposed to be in this change?

http://codereview.chromium.org/151033/diff/36/50
File chrome/browser/gtk/gtk_dnd_util.h (right):

http://codereview.chromium.org/151033/diff/36/50#newcode5
Line 5: #ifndef CHROME_BROWSER_GTK_DND_UTIL_H_
Nit: I think there's a missing GTK_ in here.

http://codereview.chromium.org/151033/diff/36/37
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/151033/diff/36/37#newcode427
Line 427: GtkDndUtil::GetAtomForTarget(GtkDndUtil::X_CHROME_TEXT_HTML), 8,
Nit: Can we have a const for the 8?  A const in the function is fine for me.

http://codereview.chromium.org/151033/diff/36/37#newcode434
Line 434: view->drag_file_mime_type_, 8, reinterpret_cast<const guchar*>(
Same nit as above.

[Evan Stade, 2009-06-29 21:50:05]

http://codereview.chromium.org/151033/diff/36/41
File base/mime_util.h (right):

http://codereview.chromium.org/151033/diff/36/41#newcode21
Line 21: std::string GetDataMimeType(const std::string& data);
On 2009/06/29 21:28:58, tony wrote:
> Is this file supposed to be in this change?

yes, it's needed for the file_contents drag type. We need to know the mime type
for the file contents, and get it by inspecting the data. We can't use the
extension because there is none (and that isn't as robust anyway).

http://codereview.chromium.org/151033/diff/36/50
File chrome/browser/gtk/gtk_dnd_util.h (right):

http://codereview.chromium.org/151033/diff/36/50#newcode5
Line 5: #ifndef CHROME_BROWSER_GTK_DND_UTIL_H_
On 2009/06/29 21:28:58, tony wrote:
> Nit: I think there's a missing GTK_ in here.

Done.

http://codereview.chromium.org/151033/diff/36/37
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/151033/diff/36/37#newcode390
Line 390: mime_util::GetDataMimeType(drop_data.file_contents).c_str(), FALSE);
this is where the new function is used.

http://codereview.chromium.org/151033/diff/36/37#newcode427
Line 427: GtkDndUtil::GetAtomForTarget(GtkDndUtil::X_CHROME_TEXT_HTML), 8,
On 2009/06/29 21:28:58, tony wrote:
> Nit: Can we have a const for the 8?  A const in the function is fine for me.

I hate this parameter. Done.

[tony, 2009-06-29 22:01:08]

LGTM!