Fixing long press paste pop up bug

Fixing long press paste pop up bug

Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=614557
Committed: https://crrev.com/57fb476d4be50c4d0690036bf6dc0f7ffc386fbb
Cr-Commit-Position: refs/heads/master@{#395789}

[amaralp, 2016-05-24 23:46:25]

Description was changed from

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=
==========

to

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=
==========

[amaralp, 2016-05-24 23:46:25]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org

[amaralp, 2016-05-24 23:46:41]

On 2016/05/24 23:46:25, amaralp wrote:
> mailto:amaralp@chromium.org changed reviewers:
> + mailto:aelias@chromium.org

PTAL

[aelias_OOO_until_Jul13, 2016-05-24 23:53:16]

Please file a new bug on crbug and assign it to yourself for the BUG=.  (Such
bugs prove useful to track the history particularly when a reverts or
cherry-picks wind up happening.)

https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:139: || (data.isEditable
&& data.selectedText.isEmpty());
Could you remove the check for isEmpty()?  That seems like it usually wouldn't
changes the outcome, and if it does that's probably a bug (e.g. long-pressing a
<canvas> element within a ContentEditable block should still show the context
menu).

https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:374: if
(selectedWebFrame->client())
Should've thought of this on the previous patch, but can you also change this
one to return false to follow the same pattern as the rest?

[amaralp, 2016-05-25 00:12:54]

Description was changed from

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=
==========

to

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=614557
==========

[amaralp, 2016-05-25 00:18:43]

I also added the bug id

https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:139: || (data.isEditable
&& data.selectedText.isEmpty());
On 2016/05/24 23:53:16, aelias wrote:
> Could you remove the check for isEmpty()?  That seems like it usually wouldn't
> changes the outcome, and if it does that's probably a bug (e.g. long-pressing
a
> <canvas> element within a ContentEditable block should still show the context
> menu).

I copied that check from chrome_web_contents_view_delegate_android.cc had done.
I'll remove the check and see if it changes anything.

https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:374: if
(selectedWebFrame->client())
On 2016/05/24 23:53:16, aelias wrote:
> Should've thought of this on the previous patch, but can you also change this
> one to return false to follow the same pattern as the rest?

Done.

[amaralp, 2016-05-25 00:46:26]

On 2016/05/25 00:18:43, amaralp wrote:
> I also added the bug id
> 
>
https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:139: ||
(data.isEditable
> && data.selectedText.isEmpty());
> On 2016/05/24 23:53:16, aelias wrote:
> > Could you remove the check for isEmpty()?  That seems like it usually
wouldn't
> > changes the outcome, and if it does that's probably a bug (e.g.
long-pressing
> a
> > <canvas> element within a ContentEditable block should still show the
context
> > menu).
> 
> I copied that check from chrome_web_contents_view_delegate_android.cc had
done.
> I'll remove the check and see if it changes anything.
> 
>
https://codereview.chromium.org/2007213002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:374: if
> (selectedWebFrame->client())
> On 2016/05/24 23:53:16, aelias wrote:
> > Should've thought of this on the previous patch, but can you also change
this
> > one to return false to follow the same pattern as the rest?
> 
> Done.

PTAL

[aelias_OOO_until_Jul13, 2016-05-25 01:08:32]

lgtm

https://codereview.chromium.org/2007213002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/2007213002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:376: return true;
Nit: in Blink style, it's generally preferred for returns within conditionals to
be the only thing in the block.  Please rewrite this as:

if (!selectedWebFrame->client())
    return false;

selectedWebFrame->client()->showContextMenu(data);
return true;

[amaralp, 2016-05-25 01:21:02]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2016-05-25 01:21:02]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2007213002/#ps40001
(title: "Fixed nit pick")

[commit-bot: I haz the power, 2016-05-25 01:21:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007213002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007213002/40001

[commit-bot: I haz the power, 2016-05-25 03:16:06]

Description was changed from

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=614557
==========

to

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=614557
==========

[commit-bot: I haz the power, 2016-05-25 03:16:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-25 03:18:06]

Description was changed from

==========
Fixing long press paste pop up bug


Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=614557
==========

to

==========
Fixing long press paste pop up bug

Long pressing on an empty editable input didn't trigger a paste
popup menu. This change fixes that.

BUG=614557

Committed: https://crrev.com/57fb476d4be50c4d0690036bf6dc0f7ffc386fbb
Cr-Commit-Position: refs/heads/master@{#395789}
==========

[commit-bot: I haz the power, 2016-05-25 03:18:07]

Patchset 3 (id:??) landed as
https://crrev.com/57fb476d4be50c4d0690036bf6dc0f7ffc386fbb
Cr-Commit-Position: refs/heads/master@{#395789}