Removing unnecessary context menu logic

Removing unnecessary context menu logic

The Android logic of CL https://codereview.chromium.org/1471993002 was made obsolete by https://codereview.chromium.org/2053213002. This change removes the unnecessary parts that were added.

BUG=

Committed: https://crrev.com/dbb23e2ee22c51b0c30c6980dc362aeccec77d6d
Cr-Commit-Position: refs/heads/master@{#421025}

[amaralp, 2016-09-21 21:40:18]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 21:41:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 22:00:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 22:00:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-09-21 22:03:52]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 22:04:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2016-09-21 22:07:08]

Description was changed from

==========
Removing unnecessary context menu logic

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

to

==========
Removing unnecessary context menu logic

The Android logic of CL https://codereview.chromium.org/1471993002 was made
obsolete by https://codereview.chromium.org/2053213002. This change removes the
unnecessary parts that were added.

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

[amaralp, 2016-09-21 22:07:09]

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

[amaralp, 2016-09-21 22:42:45]

PTAL

[commit-bot: I haz the power, 2016-09-22 00:04:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 00:04:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Changwan Ryu, 2016-09-22 00:05:03]

On 2016/09/21 22:42:45, amaralp wrote:
> PTAL

One quick question just to have a better understanding of
https://codereview.chromium.org/2053213002/:

Maybe I'm missing something, but do we NOT need to suspend blinking for iOS?
I assume mouseEvent(?) is coming from touch in that case, but iOS may have
similar UX as OS X.

[aelias_OOO_until_Jul13, 2016-09-22 00:13:59]

On 2016/09/22 at 00:05:03, changwan wrote:
> Maybe I'm missing something, but do we NOT need to suspend blinking for iOS?
> I assume mouseEvent(?) is coming from touch in that case, but iOS may have
similar UX as OS X.

I'm confused as to why you are bringing up iOS.  You're aware that Chrome for
iOS is a WebKit skin on top of a pretty opaque public API, right?  We have no
control at all over details like cursor behavior inside an iOS WebView, and
neither patch affects Chrome for iOS at all (it does not include Blink nor even
much of anything in content/).

[Changwan Ryu, 2016-09-22 00:33:12]

lgtm

My bad, I was mistaken when I mentioned iOS.

[aelias_OOO_until_Jul13, 2016-09-22 18:37:35]

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

[aelias_OOO_until_Jul13, 2016-09-22 18:37:50]

lgtm, adding boliu@ for content/browser OWNERS

[boliu, 2016-09-22 18:48:36]

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
File android_webview/native/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
android_webview/native/aw_web_contents_view_delegate.cc:43: if
(params.is_editable && params.selection_text.empty()) {
what if this check fails though?

[amaralp, 2016-09-22 18:58:55]

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
File android_webview/native/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
android_webview/native/aw_web_contents_view_delegate.cc:43: if
(params.is_editable && params.selection_text.empty()) {
On 2016/09/22 at 18:48:36, boliu wrote:
> what if this check fails though?

Wouldn't the behavior be the same as before the patch? The menu wouldn't be
shown.

[boliu, 2016-09-22 19:02:46]

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
File android_webview/native/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
android_webview/native/aw_web_contents_view_delegate.cc:43: if
(params.is_editable && params.selection_text.empty()) {
On 2016/09/22 18:58:54, amaralp wrote:
> On 2016/09/22 at 18:48:36, boliu wrote:
> > what if this check fails though?
> 
> Wouldn't the behavior be the same as before the patch? The menu wouldn't be
> shown.

it used to call NotifyContextMenuClosed if ths check fails, presumably to match
this call. is that not important?

[amaralp, 2016-09-22 19:15:30]

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
File android_webview/native/aw_web_contents_view_delegate.cc (right):

https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
android_webview/native/aw_web_contents_view_delegate.cc:43: if
(params.is_editable && params.selection_text.empty()) {
On 2016/09/22 at 19:02:46, boliu wrote:
> On 2016/09/22 18:58:54, amaralp wrote:
> > On 2016/09/22 at 18:48:36, boliu wrote:
> > > what if this check fails though?
> > 
> > Wouldn't the behavior be the same as before the patch? The menu wouldn't be
> > shown.
> 
> it used to call NotifyContextMenuClosed if ths check fails, presumably to
match this call. is that not important?

Previously NotifyContextMenuClosed would resume the blinking of the caret but
since on Android the blinking is not suspended this is unnecessary.

[boliu, 2016-09-22 19:21:17]

On 2016/09/22 19:15:30, amaralp wrote:
>
https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
> File android_webview/native/aw_web_contents_view_delegate.cc (right):
> 
>
https://codereview.chromium.org/2360103002/diff/20001/android_webview/native/...
> android_webview/native/aw_web_contents_view_delegate.cc:43: if
> (params.is_editable && params.selection_text.empty()) {
> On 2016/09/22 at 19:02:46, boliu wrote:
> > On 2016/09/22 18:58:54, amaralp wrote:
> > > On 2016/09/22 at 18:48:36, boliu wrote:
> > > > what if this check fails though?
> > > 
> > > Wouldn't the behavior be the same as before the patch? The menu wouldn't
be
> > > shown.
> > 
> > it used to call NotifyContextMenuClosed if ths check fails, presumably to
> match this call. is that not important?
> 
> Previously NotifyContextMenuClosed would resume the blinking of the caret but
> since on Android the blinking is not suspended this is unnecessary.

Oh that method got removed too. rs lgtm

[amaralp, 2016-09-22 19:28:11]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2016-09-22 19:28:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 19:39:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 19:39:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[amaralp, 2016-09-22 21:24:29]

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

[amaralp, 2016-09-22 21:24:30]

Adding tedchoc@ for OWNERS of Android browser-side code

[Ted C, 2016-09-22 21:46:39]

chrome lgtm

https://codereview.chromium.org/2360103002/diff/20001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://codereview.chromium.org/2360103002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:38:
content_view_core->ShowPastePopup(params.selection_start.x(),
decrease indent by two

https://codereview.chromium.org/2360103002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:40:
return;
Since we're changing this, I think we should put the return outside the
content_view_core_ check, we wouldn't want to attempt to show a context menu in
this case.

-2 indent as well

https://codereview.chromium.org/2360103002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2360103002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2498:
@CalledByNative
annotations should go right above the method declaration

[amaralp, 2016-09-22 22:03:30]

https://codereview.chromium.org/2360103002/diff/20001/chrome/browser/ui/andro...
File
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc
(right):

https://codereview.chromium.org/2360103002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:38:
content_view_core->ShowPastePopup(params.selection_start.x(),
On 2016/09/22 at 21:46:39, Ted C wrote:
> decrease indent by two

Done.

https://codereview.chromium.org/2360103002/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc:40:
return;
On 2016/09/22 at 21:46:38, Ted C wrote:
> Since we're changing this, I think we should put the return outside the
content_view_core_ check, we wouldn't want to attempt to show a context menu in
this case.
> 
> -2 indent as well

Done.

https://codereview.chromium.org/2360103002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2360103002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2498:
@CalledByNative
On 2016/09/22 at 21:46:39, Ted C wrote:
> annotations should go right above the method declaration

Done.

[amaralp, 2016-09-23 21:14:01]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2016-09-23 21:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, aelias@chromium.org, changwan@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2360103002/#ps40001
(title: "fixed nits")

[commit-bot: I haz the power, 2016-09-23 21:14:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 21:24:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 21:24:31]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[amaralp, 2016-09-26 19:48:18]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:49:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 22:40:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-26 22:43:47]

Description was changed from

==========
Removing unnecessary context menu logic

The Android logic of CL https://codereview.chromium.org/1471993002 was made
obsolete by https://codereview.chromium.org/2053213002. This change removes the
unnecessary parts that were added.

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

to

==========
Removing unnecessary context menu logic

The Android logic of CL https://codereview.chromium.org/1471993002 was made
obsolete by https://codereview.chromium.org/2053213002. This change removes the
unnecessary parts that were added.

BUG=

Committed: https://crrev.com/dbb23e2ee22c51b0c30c6980dc362aeccec77d6d
Cr-Commit-Position: refs/heads/master@{#421025}
==========

[commit-bot: I haz the power, 2016-09-26 22:43:48]

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

[PEConn, 2016-09-28 13:16:48]

peconn@chromium.org changed reviewers:
+ peconn@chromium.org

[PEConn, 2016-09-28 13:16:49]

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1789:
public void onContextMenuClosed(Menu menu) {
Was there any reason to remove this code? We need it because
SnippetArticleViewHolder.java adds listeners to mContextMenuCloseObservers and
expects to be informed when the context menu is closed.

[boliu, 2016-09-28 15:36:40]

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1789:
public void onContextMenuClosed(Menu menu) {
On 2016/09/28 13:16:48, PEConn wrote:
> Was there any reason to remove this code? We need it because
> SnippetArticleViewHolder.java adds listeners to mContextMenuCloseObservers and
> expects to be informed when the context menu is closed.

Yeah, probably should have removed the bit calling into web contents only, not
the whole method.

[amaralp, 2016-09-28 18:04:44]

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1789:
public void onContextMenuClosed(Menu menu) {
On 2016/09/28 at 15:36:40, boliu wrote:
> On 2016/09/28 13:16:48, PEConn wrote:
> > Was there any reason to remove this code? We need it because
> > SnippetArticleViewHolder.java adds listeners to mContextMenuCloseObservers
and
> > expects to be informed when the context menu is closed.
> 
> Yeah, probably should have removed the bit calling into web contents only, not
the whole method.

Yeah, you are right. Here is a patch to fix it:
https://codereview.chromium.org/2378793002. Sorry about that.

[amaralp, 2016-09-28 18:33:24]

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/2360103002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1789:
public void onContextMenuClosed(Menu menu) {
On 2016/09/28 at 18:04:44, amaralp wrote:
> On 2016/09/28 at 15:36:40, boliu wrote:
> > On 2016/09/28 13:16:48, PEConn wrote:
> > > Was there any reason to remove this code? We need it because
> > > SnippetArticleViewHolder.java adds listeners to mContextMenuCloseObservers
and
> > > expects to be informed when the context menu is closed.
> > 
> > Yeah, probably should have removed the bit calling into web contents only,
not the whole method.
> 
> Yeah, you are right. Here is a patch to fix it:
https://codereview.chromium.org/2378793002. Sorry about that.

Looks like this was already fixed in https://codereview.chromium.org/2381473002.