Android should be able to copy HTML snippets to the system clipboard.

Android should be able to copy HTML snippets to the system clipboard.

Currently the only clipboard data we actually push to the Android system
clipboard is plain-text data, so any formatting and links will be removed
when someone pastes content from Chromium to, for example, Gmail.

In order to push HTML data on the Android clipboard, we do need to know
the plain-text representation so that non-HTML consumers can still use
what's on there (there only is one primary clip). For that reason, we only
push HTML to the Android system clipboard if there also is a plain-text
representation available in the ClipboardMap.

There are a few situations in which we end up calling Clipboard::WriteHTML.
Android doesn't care about Pepper and drag and drop data right now. The
WebClipboardImpl::writeImage() method will write a HTML snippet, but we'll
ignore that because no plain-text data will be pasted. The fourth case is
WebClipboardImpl::writeHTML(), used for DOM fragments and rich content,
which will work fine after this patch.

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

[tony, 2013-10-02 16:49:22]

I think this is OK, but it seems really fragile and likely to be broken by
accident.  Can we at least write a unittest to make sure the fourth case works?

[dcheng, 2013-10-02 22:24:14]

https://codereview.chromium.org/25668005/diff/1/ui/base/clipboard/clipboard_a...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/25668005/diff/1/ui/base/clipboard/clipboard_a...
ui/base/clipboard/clipboard_android.cc:118: env, clipboard_manager_.obj(),
html.obj(), text.obj());
What is the expected behavior if a JS paste handler does something like this:

function onPaste(e) {
  e.setData('text/html', '<b>Hello world!</b>');
  e.preventDefault();
}

Do we want nothing on the clipboard in that case? It seems like we still ought
to write HTML to the clipboard in that case (with empty text content as the
fallback, since that's no worse than what it would have originally been).

[Peter Beverloo, 2013-10-23 17:11:34]

Sorry for the long silence -- now trying to find the right way to test this.

https://codereview.chromium.org/25668005/diff/1/ui/base/clipboard/clipboard_a...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/25668005/diff/1/ui/base/clipboard/clipboard_a...
ui/base/clipboard/clipboard_android.cc:118: env, clipboard_manager_.obj(),
html.obj(), text.obj());
On 2013/10/02 22:24:15, dcheng wrote:
> What is the expected behavior if a JS paste handler does something like this:
> 
> function onPaste(e) {
>   e.setData('text/html', '<b>Hello world!</b>');
>   e.preventDefault();
> }
> 
> Do we want nothing on the clipboard in that case? It seems like we still ought
> to write HTML to the clipboard in that case (with empty text content as the
> fallback, since that's no worse than what it would have originally been).

That's a good question.  Pasting HTML on Android requires the plain-text
representation because the HTML API is relatively new, and not all applications
will support that.  The experience which users would get following this
suggestion is that their older apps indicate that there is data on the clipboard
(i.e. "Paste" is enabled), but when pasting data nothing happens.

We could mitigate this by implementing Android-specific behavior that if a
ScopedClipboardWriter only writes HTML data, we call some API to normalize this
to a text-only representation.  I don't know whether such an API is available,
however.

[joth, 2013-10-24 03:18:20]

On 2013/10/23 17:11:34, Peter Beverloo wrote:
> Sorry for the long silence -- now trying to find the right way to test this.
> 
>
https://codereview.chromium.org/25668005/diff/1/ui/base/clipboard/clipboard_a...
> File ui/base/clipboard/clipboard_android.cc (right):
> 
>
https://codereview.chromium.org/25668005/diff/1/ui/base/clipboard/clipboard_a...
> ui/base/clipboard/clipboard_android.cc:118: env, clipboard_manager_.obj(),
> html.obj(), text.obj());
> On 2013/10/02 22:24:15, dcheng wrote:
> > What is the expected behavior if a JS paste handler does something like
this:
> > 
> > function onPaste(e) {
> >   e.setData('text/html', '<b>Hello world!</b>');
> >   e.preventDefault();
> > }
> > 
> > Do we want nothing on the clipboard in that case? It seems like we still
ought
> > to write HTML to the clipboard in that case (with empty text content as the
> > fallback, since that's no worse than what it would have originally been).
> 
> That's a good question.  Pasting HTML on Android requires the plain-text
> representation because the HTML API is relatively new, and not all
applications
> will support that.  The experience which users would get following this
> suggestion is that their older apps indicate that there is data on the
clipboard
> (i.e. "Paste" is enabled), but when pasting data nothing happens.
> 
> We could mitigate this by implementing Android-specific behavior that if a
> ScopedClipboardWriter only writes HTML data, we call some API to normalize
this
> to a text-only representation.  I don't know whether such an API is available,
> however.

If there's nothing appropriate in the chrome toolkit, you could try
Html.fromHtml() as a last resort.
http://developer.android.com/reference/android/text/Html.html#fromHtml(java.l...

[Peter Beverloo, 2013-10-24 17:22:20]

+bulach

Ready for re-review, now including a content_shell-based instrumentation test.

[bulach, 2013-10-25 14:32:47]

lgtm, thanks for the test! :)

there's one comment in the code that I didn't quite understand, specially with
joth's comment to get the text representation... a suggestion below:

https://codereview.chromium.org/25668005/diff/57001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/25668005/diff/57001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_android.cc:107: // keys being ordered, any
plain-text representation will be set already.
I'm not quite sure what std::map order has to do here?
presumably the caller can call just Set(kHTMLFormat, "<b>Foo</b>") without ever
calling Set(kPlainTextFormat, "..."); ?

maybe:

// ..requires a plain-text representation to be available as well.
// If there isn't kPlainTextFormat data, don't put just the HTML
// representation, otherwise older apps may not be able to paste it.

[Peter Beverloo, 2013-10-25 15:29:37]

Thank you for the review, Marcus!  Tony/Daniel, any further comments?

https://codereview.chromium.org/25668005/diff/57001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/25668005/diff/57001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_android.cc:107: // keys being ordered, any
plain-text representation will be set already.
On 2013/10/25 14:32:47, bulach wrote:
> I'm not quite sure what std::map order has to do here?
> presumably the caller can call just Set(kHTMLFormat, "<b>Foo</b>") without
ever
> calling Set(kPlainTextFormat, "..."); ?
> 
> maybe:
> 
> // ..requires a plain-text representation to be available as well.
> // If there isn't kPlainTextFormat data, don't put just the HTML
> // representation, otherwise older apps may not be able to paste it.

Done.

[tony, 2013-10-25 16:44:57]

LGTM

[commit-bot: I haz the power, 2013-10-28 10:06:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/25668005/117001

[commit-bot: I haz the power, 2013-10-28 16:25:04]

Change committed as 231337