Remove redundant android clipboard API calls from clipboard writing code

Remove redundant android clipboard API calls from clipboard writing code

Remove android clipboard sync calls during writing clipboard data.
Also restructure the code to avoid redundant setText() calls when
HTML text is being set to clipboard.

BUG=467532

[ams, 2015-03-17 07:32:17]

amit.srkr@samsung.com changed reviewers:
+ dcheng@chromium.org

[ams, 2015-03-17 07:33:19]

On 2015/03/17 07:32:17, ams wrote:
> mailto:amit.srkr@samsung.com changed reviewers:
> + mailto:dcheng@chromium.org

PTAL

[dcheng, 2015-03-18 03:42:46]

What is the motivation for this change? Will this address functional bugs like
https://crbug.com/369101?

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

https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_android.cc:55: void CommitToClipboard();
Probably should be "CommitToAndroidClipboard()". It's a bit confusing to read
otherwise.

https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_android.cc:166: env,
map_[kPlainTextFormat].c_str());
The c_str() here (and below) is superfluous.

https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_android.cc:169: if
(!Java_Clipboard_isHTMLClipboardSupported(env)) {
Why wasn't this check previously needed?

https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_android.cc:169: if
(!Java_Clipboard_isHTMLClipboardSupported(env)) {
Why wasn't this check previously needed?

[ams, 2015-03-18 09:42:42]

On 2015/03/18 03:42:46, dcheng wrote:
> What is the motivation for this change? Will this address functional bugs like
> https://crbug.com/369101
> 

This patch does not address the above mentioned functional bug. 
Main goal for this patch is to remove additional redundant JNI as well as
Android Clipboard API calls during copy operation and optimize.
Also additional benefit of this change is that new method can be easily extended
even if the clipboard supports multiple clipData.
Currently there is always an overhead of setText() when we want to do
setHTMLText() and Sync calls. We can avoid these.

>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> File ui/base/clipboard/clipboard_android.cc (right):
> 
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> ui/base/clipboard/clipboard_android.cc:55: void CommitToClipboard();
> Probably should be "CommitToAndroidClipboard()". It's a bit confusing to read
> otherwise.
> 

Done.

>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> ui/base/clipboard/clipboard_android.cc:166: env,
> map_[kPlainTextFormat].c_str());
> The c_str() here (and below) is superfluous.
> 

Done.

>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> ui/base/clipboard/clipboard_android.cc:169: if
> (!Java_Clipboard_isHTMLClipboardSupported(env)) {
> Why wasn't this check previously needed?
> 
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> ui/base/clipboard/clipboard_android.cc:169: if
> (!Java_Clipboard_isHTMLClipboardSupported(env)) {
> Why wasn't this check previously needed?

Earlier since setText() was always called even during copying HTML into android
clipboard, this check was not required since a text version was always copied
first. However since we have done away with setText() call when we want to copy
HTML content, we needed to check if HTML clipboard support is available and go
for fallback approach, i.e. call setText() in case HTML clipboard not supported.

However this check can be moved at starting if condition, since then else case
will automatically do the needful.
Please suggest.

[ams, 2015-04-13 06:10:57]

On 2015/03/18 09:42:42, ams wrote:
> On 2015/03/18 03:42:46, dcheng wrote:
> > What is the motivation for this change? Will this address functional bugs
like
> > https://crbug.com/369101
> > 
> 
> This patch does not address the above mentioned functional bug. 
> Main goal for this patch is to remove additional redundant JNI as well as
> Android Clipboard API calls during copy operation and optimize.
> Also additional benefit of this change is that new method can be easily
extended
> even if the clipboard supports multiple clipData.
> Currently there is always an overhead of setText() when we want to do
> setHTMLText() and Sync calls. We can avoid these.
> 
> >
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> > File ui/base/clipboard/clipboard_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> > ui/base/clipboard/clipboard_android.cc:55: void CommitToClipboard();
> > Probably should be "CommitToAndroidClipboard()". It's a bit confusing to
read
> > otherwise.
> > 
> 
> Done.
> 
> >
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> > ui/base/clipboard/clipboard_android.cc:166: env,
> > map_[kPlainTextFormat].c_str());
> > The c_str() here (and below) is superfluous.
> > 
> 
> Done.
> 
> >
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> > ui/base/clipboard/clipboard_android.cc:169: if
> > (!Java_Clipboard_isHTMLClipboardSupported(env)) {
> > Why wasn't this check previously needed?
> > 
> >
>
https://codereview.chromium.org/1012673004/diff/1/ui/base/clipboard/clipboard...
> > ui/base/clipboard/clipboard_android.cc:169: if
> > (!Java_Clipboard_isHTMLClipboardSupported(env)) {
> > Why wasn't this check previously needed?
> 
> Earlier since setText() was always called even during copying HTML into
android
> clipboard, this check was not required since a text version was always copied
> first. However since we have done away with setText() call when we want to
copy
> HTML content, we needed to check if HTML clipboard support is available and go
> for fallback approach, i.e. call setText() in case HTML clipboard not
supported.
> 
> However this check can be moved at starting if condition, since then else case
> will automatically do the needful.
> Please suggest.

I updated the position for HTMLClipboard check in Patch set #3.

PTAL again :-)

[dcheng, 2015-04-13 22:25:53]

Unofrtunately, I don't feel comfortable L-G-T-Ming Android clipboard patches
until we get the unit tests on Android working again.

I started working on this in https://codereview.chromium.org/1027993002 (which
is a pre-requisite for clipboard tests on Android), but don't currently have
time to follow up. Is this something you'd be willing to pick up?