Don't put HTML onto the clipboard if there is file content data.

Don't put HTML onto the clipboard if there is file content data.

BUG=55879
TEST=drag an image into mail, it should work


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174655

[Avi (use Gerrit), 2012-12-22 23:07:42]

What do you think?

This affects drags with just a single image and those with text and a single
image. The former, ok. The latter? Well... not thrilled; is it worth the
tradeoff?

If a drag has two or more images then we don't get image data and this doesn't
change anything.

[dcheng, 2012-12-22 23:30:25]

Is the latter actually a problem? The latter can only occur if a selection is
being dragged and my recollection of the code is that WebKit doesn't write file
content in that case even if an image is part of the selection.

On 2012/12/22 23:07:42, Avi wrote:
> What do you think?
> 
> This affects drags with just a single image and those with text and a single
> image. The former, ok. The latter? Well... not thrilled; is it worth the
> tradeoff?
> 
> If a drag has two or more images then we don't get image data and this doesn't
> change anything.

[Avi (use Gerrit), 2012-12-22 23:32:24]

If you have a selection that includes an inline image, you get the image
includes as the file content; I was experimenting with this patch, and that
happens.

I'm curious as to who accepts html drag data that we're putting on the drag
pasteboard. What do we need to test?

[Nico, 2012-12-23 00:08:26]

https://codereview.chromium.org/11666018/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_drag_source_mac.mm (right):

https://codereview.chromium.org/11666018/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_drag_source_mac.mm:435: // data.
How does the HTML content for just an image look? Can we sniff for that and drop
html in just that case?

(Not sure if it's worth it.)

[Avi (use Gerrit), 2012-12-23 00:17:07]

Actually, Daniel is right; if you have a span of text and an inline image, you
don't get file content. I was testing on http://goodmorningkitten.com/ grabbing
an image + link and that showed up as an image. (The associated link text didn't
even show up as HTML...)

Trying on http://www-sul.stanford.edu/tools/tutorials/html2.0/img.html , this
looks pretty safe.

[Nico, 2012-12-23 00:33:54]

How _does_ the HTML content for just an image look?

[Avi (use Gerrit), 2012-12-23 18:45:21]

On 2012/12/23 00:33:54, Nico wrote:
> How _does_ the HTML content for just an image look?

It looks like:

<meta http-equiv="Content-Type" content="text/html;charset=UTF-8"><img
style="-webkit-user-select: none"
src="http://1.bp.blogspot.com/-o_cshQR0edU/T8tVKBXereI/AAAAAAAABjs/894NbAvLsDw/s1600/cute-kitten-10.jpg"
class="quimby_search_image">

Or if I reload that URL into an incog window:

<meta http-equiv="Content-Type" content="text/html;charset=UTF-8"><img
style="-webkit-user-select: none"
src="http://1.bp.blogspot.com/-o_cshQR0edU/T8tVKBXereI/AAAAAAAABjs/894NbAvLsDw/s1600/cute-kitten-10.jpg">

[Nico, 2012-12-26 18:41:47]

On 2012/12/23 18:45:21, Avi wrote:
> On 2012/12/23 00:33:54, Nico wrote:
> > How _does_ the HTML content for just an image look?
> 
> It looks like:
> 
> <meta http-equiv="Content-Type" content="text/html;charset=UTF-8"><img
> style="-webkit-user-select: none"
>
src="http://1.bp.blogspot.com/-o_cshQR0edU/T8tVKBXereI/AAAAAAAABjs/894NbAvLsDw/s1600/cute-kitten-10.jpg"
> class="quimby_search_image">
> 
> Or if I reload that URL into an incog window:
> 
> <meta http-equiv="Content-Type" content="text/html;charset=UTF-8"><img
> style="-webkit-user-select: none"
>
src="http://1.bp.blogspot.com/-o_cshQR0edU/T8tVKBXereI/AAAAAAAABjs/894NbAvLsDw/s1600/cute-kitten-10.jpg">

It looks like WebKit adds this for image documents:
https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source...

(Found via
https://code.google.com/p/chromium/source/search?q=%22-webkit-user-select%3A+...
)

Do non-standalone images work with this patch too? Without this patch? Do we
have some other way to figure out if the current document is an image document?

[Avi (use Gerrit), 2012-12-26 19:18:06]

What do you mean "non-standalone"? If I have a web page open, and I drag an
image from within that web page, that also falls under this case.

As for the "current document" we have no such notion. WebDragSourceMac gets a
DropData and that's it. There's no sense of where it came from.

[Nico, 2012-12-26 19:23:59]

On Wed, Dec 26, 2012 at 11:18 AM, <avi@chromium.org> wrote:

> What do you mean "non-standalone"? If I have a web page open, and I drag an
> image from within that web page, that also falls under this case.


"non-standalone" = from a web page, not from a tab showing nothing but an
image. Apparently that goes through that code path too.

Do you think it makes sense to sniff for this specific html and only omit
it in that case?

How does image dragging work with svg images? (eg the blue bird on
http://demosthenes.info/blog/428/Using-SVG-In-Web-Pages ) Do apps handle
that right?


>

As for the "current document" we have no such notion. WebDragSourceMac gets
> a
> DropData and that's it. There's no sense of where it came from.
>
>
https://codereview.chromium.**org/11666018/<https://codereview.chromium.org/1...
>

[Avi (use Gerrit), 2012-12-26 19:33:17]

SVG images don't appear to set dropData_->file_contents, so the behavior is
completely different.

My question is: under what conditions is file_contents set? If it's only set for
an image drag, then we don't have to sniff HTML since that's the only case we're
looking for. If not...

I'd really prefer to not sniff HTML.

[Avi (use Gerrit), 2012-12-26 20:03:16]

To answer that question:

Both file_contents and file_description_filename are only set if the drop data
is initialized from a WebDragData of type
WebDragData::Item::StorageTypeBinaryData.

Now _that_ happens when the drag data is initialized from a
ChromiumDataObjectItem of type DataTransferItem::kindFile _and_ the item has a
sharedBuffer.

That only happens with the initializer
ChromiumDataObjectItem::createFromSharedBuffer.

That only happens with the function call ChromiumDataObject::addSharedBuffer.

The only caller of that function is writeImageToDataObject in
WebCore/platform/chromium/ClipboardChromium.cpp.

Therefore, if there are file contents on the drag data, we know that it's an
image and we can safely drop the html.

[Nico, 2012-12-26 20:32:23]

Sweet, lgtm.

Can you mention parts of that in the CL description? ("File content data is
currently only present for images" or similar)

[Avi (use Gerrit), 2012-12-26 20:35:27]

On 2012/12/26 20:32:23, Nico wrote:
> Sweet, lgtm.
> 
> Can you mention parts of that in the CL description? ("File content data is
> currently only present for images" or similar)

Sure. Do you want me to be paranoid and check to see if the image UTI conforms
to public/image before dropping the html?

[Nico, 2012-12-26 20:39:27]

On Wed, Dec 26, 2012 at 12:35 PM, <avi@chromium.org> wrote:

> On 2012/12/26 20:32:23, Nico wrote:
>
>> Sweet, lgtm.
>>
>
>  Can you mention parts of that in the CL description? ("File content data
>> is
>> currently only present for images" or similar)
>>
>
> Sure. Do you want me to be paranoid and check to see if the image UTI
> conforms
> to public/image before dropping the html?
>

Up to you.


>
>
https://codereview.chromium.**org/11666018/<https://codereview.chromium.org/1...
>

[Avi (use Gerrit), 2012-12-26 21:12:26]

PTAL.

[Nico, 2012-12-26 21:52:55]

https://codereview.chromium.org/11666018/diff/9003/content/browser/web_conten...
File content/browser/web_contents/web_drag_source_mac.mm (right):

https://codereview.chromium.org/11666018/diff/9003/content/browser/web_conten...
content/browser/web_contents/web_drag_source_mac.mm:439:
UTTypeConformsTo(fileUTI_.get(), kUTTypeImage);
fileUTI_ is only set if !mimeType.empty(). Since there's an if for that case, I
assume that happens in some cases, so you might end up with an outdated fileUTI_
here in some cases (?)

[Avi (use Gerrit), 2012-12-26 22:22:10]

https://codereview.chromium.org/11666018/diff/9003/content/browser/web_conten...
File content/browser/web_contents/web_drag_source_mac.mm (right):

https://codereview.chromium.org/11666018/diff/9003/content/browser/web_conten...
content/browser/web_contents/web_drag_source_mac.mm:439:
UTTypeConformsTo(fileUTI_.get(), kUTTypeImage);
True. Fixed. I don't want to trace through webkit where that might happen. What
indeed.

[Avi (use Gerrit), 2012-12-26 22:22:42]

ptal

[Nico, 2012-12-26 22:35:51]

lgtm

[commit-bot: I haz the power, 2012-12-26 22:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11666018/11001

[commit-bot: I haz the power, 2012-12-27 03:05:45]

Change committed as 174655

[dcheng, 2012-12-27 04:33:36]

For whatever reason, I didn't get the rest of the replies in this thread. Just
replying to a few things here and there:

> I was testing on http://goodmorningkitten.com/ grabbing
an image + link and that showed up as an image. (The associated link text didn't
even show up as HTML...)

I wonder if that's a bug...

> How does the HTML content for just an image look? Can we sniff for that and
drop
html in just that case?

I think if we ever need to distinguish between image file content and non-image
file content, I'd prefer to add an extra flag to WebDragData/WebDropData first.
Sniffing the HTML could introduce a dependency on WebKit bits that generate
markup from nodes, and that seems really fragile.

> To answer that question:
>
> Both file_contents and file_description_filename are only set if the drop data
is initialized from a WebDragData of type
WebDragData::Item::StorageTypeBinaryData.

This is also tangentially covered in
https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webdropdata.h...,
but I could probably make the comment stronger. Sorry that this code path is so
convoluted =(

[Avi (use Gerrit), 2012-12-27 06:06:53]

On 2012/12/27 04:33:36, dcheng wrote:
> > I was testing on http://goodmorningkitten.com/ grabbing
> an image + link and that showed up as an image. (The associated link text
didn't
> even show up as HTML...)
> 
> I wonder if that's a bug...

Possibly, though I didn't dig.

> Sniffing the HTML [...] seems really fragile.

Screams "bad idea" to me...

> This is also tangentially covered

Yes, though it says "e.g. image" but I was asking the question of whether that
code path was _only_ images, which it is.

Quick question while I have you, then. There's file_contents and
download_metadata. I'm curious: in which situations would there be one and not
the other?

[dcheng, 2012-12-27 21:42:57]

On 2012/12/27 06:06:53, Avi wrote:
> On 2012/12/27 04:33:36, dcheng wrote:
> > > I was testing on http://goodmorningkitten.com/ grabbing
> > an image + link and that showed up as an image. (The associated link text
> didn't
> > even show up as HTML...)
> > 
> > I wonder if that's a bug...
> 
> Possibly, though I didn't dig.
> 
> > Sniffing the HTML [...] seems really fragile.
> 
> Screams "bad idea" to me...
> 
> > This is also tangentially covered
> 
> Yes, though it says "e.g. image" but I was asking the question of whether that
> code path was _only_ images, which it is.
> 
> Quick question while I have you, then. There's file_contents and
> download_metadata. I'm curious: in which situations would there be one and not
> the other?

download_metadata is only used for the special "downloadurl" type in drag and
drop. If you set "downloadurl", the corresponding data is passed in the
download_metadata member. The data has the format <mime type>:<suggested
filename>:<URL>. It's used to support things like attachment drag out in Gmail.

In the long run, this strikes me as somewhat of a hack; in an ideal world, I'd
like to treat dragging out attachments as a case of dragging out Files; however,
we don't have a way to construct a JS File object that wraps a URL today.

[Avi (use Gerrit), 2012-12-27 21:48:51]

This is indeed a rather unfortunate hack.