Added code to disable 'paste' if nothing is present in clipboard to paste.

Added code to disable 'paste' if nothing is present in clipboard to paste.

On right click in editable node context menu was always having 'paste'
option as enabled. Added check that if clipboard is not empty
then only show 'paste' option.

BUG=396616

[ankit, 2014-07-23 13:55:31]

PTAL

[ankit, 2014-07-24 15:11:24]

PTAL

[jamesr, 2014-07-24 18:27:15]

https://codereview.chromium.org/410873005/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/410873005/diff/1/Source/core/editing/Editor.c...
Source/core/editing/Editor.cpp:244: String text =
Pasteboard::generalPasteboard()->plainText();
this is going to do a synchronous IPC to the browser process on every call,
fetch the entire paste buffer, and copy it back to the renderer just to throw
everything away but 1 bit.  This function appears to be called pretty often.  Is
there no better way to do this?

[ankit, 2014-07-25 10:08:30]

On 2014/07/24 18:27:15, jamesr wrote:
>
https://codereview.chromium.org/410873005/diff/1/Source/core/editing/Editor.cpp
> File Source/core/editing/Editor.cpp (right):
> 
>
https://codereview.chromium.org/410873005/diff/1/Source/core/editing/Editor.c...
> Source/core/editing/Editor.cpp:244: String text =
> Pasteboard::generalPasteboard()->plainText();
> this is going to do a synchronous IPC to the browser process on every call,
> fetch the entire paste buffer, and copy it back to the renderer just to throw
> everything away but 1 bit.  This function appears to be called pretty often. 
Is
> there no better way to do this?


@jamesr
I am not able to find any other workaround or better way to do this.
Only way I could find to check if paste operation is allowed or not to check 
clipboard data. 

There are two clipboards available, one is ui::clipboard.
ui::clipboard is getting used to check paste option when right click is
performed on url bar(omnibox_view_views.cc).
if (command_id == IDS_APP_PASTE)
    return !read_only() && !GetClipboardText().empty();

In same manner I am using available clipboard to check if some data is present 
or not. 
Please suggest if we can achieve same in some better way.

[ankit, 2014-07-27 16:43:51]

On 2014/07/25 10:08:30, ankit wrote:
> On 2014/07/24 18:27:15, jamesr wrote:
> >
>
https://codereview.chromium.org/410873005/diff/1/Source/core/editing/Editor.cpp
> > File Source/core/editing/Editor.cpp (right):
> > 
> >
>
https://codereview.chromium.org/410873005/diff/1/Source/core/editing/Editor.c...
> > Source/core/editing/Editor.cpp:244: String text =
> > Pasteboard::generalPasteboard()->plainText();
> > this is going to do a synchronous IPC to the browser process on every call,
> > fetch the entire paste buffer, and copy it back to the renderer just to
throw
> > everything away but 1 bit.  This function appears to be called pretty often.

> Is
> > there no better way to do this?
> 
> 
> @jamesr
> I am not able to find any other workaround or better way to do this.
> Only way I could find to check if paste operation is allowed or not to check 
> clipboard data. 
> 
> There are two clipboards available, one is ui::clipboard.
> ui::clipboard is getting used to check paste option when right click is
> performed on url bar(omnibox_view_views.cc).
> if (command_id == IDS_APP_PASTE)
>     return !read_only() && !GetClipboardText().empty();
> 
> In same manner I am using available clipboard to check if some data is present

> or not. 
> Please suggest if we can achieve same in some better way.

@jamesr
PTAL at comment and suggest.

[abarth-chromium, 2014-07-29 16:40:12]

not lgtm

The fix for this issue is worse than the underlying problem.

[ankit, 2014-07-30 04:08:08]

On 2014/07/29 16:40:12, abarth wrote:
> not lgtm
> 
> The fix for this issue is worse than the underlying problem.

I will try to find some better solution which does not cause any side effeect.
Thanks.

[groby-ooo-7-16, 2014-07-30 23:24:26]

According to the bug, this affects the context menu for text fields in the
content area - that's the RenderViewContextMenu.

You want to add a check in RenderViewContextMenu::IsCommandIdEnabled

You probably want to use Clipboard::ReadavailableTypes, since you don't want to
disable paste for data types other than text.

This keeps everything strictly on the UI side, invoked when displaying the menu.

[ankit, 2014-07-31 09:34:03]

On 2014/07/30 23:24:26, groby wrote:
> According to the bug, this affects the context menu for text fields in the
> content area - that's the RenderViewContextMenu.
> 
> You want to add a check in RenderViewContextMenu::IsCommandIdEnabled
> 
> You probably want to use Clipboard::ReadavailableTypes, since you don't want
to
> disable paste for data types other than text.
> 
> This keeps everything strictly on the UI side, invoked when displaying the
menu.


@groby
Please correct me if my understanding is not correct.
If I check for ReadavailableTypes. I will have to check size 
of types returned and accordingly enable/disable paste option.
But in that case It's possible that I will end up showing paste
option for text field even if clipboard contains some other format 
than text(most likely image).

Instead of ReadavailableTypes isn't it better to check for
length of the result returned by this call
clipboard->ReadText(ui::CLIPBOARD_TYPE_COPY_PASTE,  &result)?

[groby-ooo-7-16, 2014-08-01 00:23:35]

You cannot know that the paste target is a text field. The HTML clipboard API
supports pasting of image content, so any contenteditable can handle paste
events.

[ankit, 2014-08-01 04:37:25]

On 2014/08/01 00:23:35, groby wrote:
> You cannot know that the paste target is a text field. The HTML clipboard API
> supports pasting of image content, so any contenteditable can handle paste
> events.

@groby
PTAL new patch

[ankit, 2014-08-04 03:55:56]

PTAL

[Avi (use Gerrit), 2014-08-04 16:56:13]

Seems like you already have plenty of reviewers.

[ankit, 2014-08-05 11:29:31]

@groby 
@abarth

PTAL

[groby-ooo-7-16, 2014-08-05 17:58:55]

LGTM - but not owner. You need abarth to sign off.

https://codereview.chromium.org/410873005/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/410873005/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1373: return
types.size() > 0;
nit: !types.empty(), please

[abarth-chromium, 2014-08-05 20:55:06]

I have no idea what the consequences are of this change.

[groby-ooo-7-16, 2014-08-05 21:07:23]

abarth: Ah, sorry, I thought you might be owner. Either way, your NOT will block
submit until you retract, no? Or do I have that wrong?

ankit: You'll need to get somebody who's OWNER to review. First step, I suggest
you clean out existing reviewers list, and _just_ pick an owner. Nobody else on
the list.

[abarth-chromium, 2014-08-05 23:13:53]

On 2014/08/05 at 21:07:23, groby wrote:
> abarth: Ah, sorry, I thought you might be owner. Either way, your NOT will
block submit until you retract, no? Or do I have that wrong?

IMHO, ankit should just make a new CL.  My comment was about a change to a
different repo entirely.

> ankit: You'll need to get somebody who's OWNER to review. First step, I
suggest you clean out existing reviewers list, and _just_ pick an owner. Nobody
else on the list.

[ankit, 2014-08-06 08:25:32]

On 2014/08/05 23:13:53, abarth wrote:
> On 2014/08/05 at 21:07:23, groby wrote:
> > abarth: Ah, sorry, I thought you might be owner. Either way, your NOT will
> block submit until you retract, no? Or do I have that wrong?
> 
> IMHO, ankit should just make a new CL.  My comment was about a change to a
> different repo entirely.
> 
> > ankit: You'll need to get somebody who's OWNER to review. First step, I
> suggest you clean out existing reviewers list, and _just_ pick an owner.
Nobody
> else on the list.

Closing this issue as per above suggestion.
Raised a new CL as suggested(https://codereview.chromium.org/443073002/).

@groby
PTAL new CL