Show selection handles around selected text.

Show selection handles around selected text.

This patch upstreams code to display text selection handles in the Android
content shell. While in selection mode, we also display an action bar with
editing commands; I have stubbed this out because we don't yet have the
necessary resources (added a TODO for that).

BUG=138468
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161353

[Iain Merrick, 2012-10-05 00:53:49]

+oli who's familiar with this Java code
+bulach for android/OWNERS
+sky for content/browser/OWNERS

[bulach, 2012-10-05 12:50:59]

thanks iain! lgtm with some nits below, feel free to land once oli / sky are
happy.

http://codereview.chromium.org/11068010/diff/2001/content/browser/android/con...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/11068010/diff/2001/content/browser/android/con...
content/browser/android/content_view_core_impl.cc:168: GetMethodID(env,
java_object_->rect_clazz, "<init>", "(IIII)V");
FYI: one of us :) will need to change this depending on when this lands:
http://codereview.chromium.org/11038015/

if that one gets in before this, s/GetMethodID/MethodID::Get<METHODTYPE_NORMAL>/

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1345:
- mNativeScrollX),
nit: I know this is the same downstream, but I think the operator should go in
the previous line (ditto in 1347, 1356 and 1358 below..)

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1512:
&& (mSelectionHandleController == null
nit: as above, operators in the previous lines.

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/CursorController.java
(right):

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/CursorController.java:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/HandleView.java
(right):

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/HandleView.java:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/HandleView.java:65:
public static final int LEFT = 0;
there's a bunch of public things here (and various methods below...) can they be
package private? apart from the @Override, I think this class is never used
anywhere else?

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/InsertionHandleController.java
(right):

http://codereview.chromium.org/11068010/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/InsertionHandleController.java:44:
public void allowAutomaticShowing() {
nit: as above, perhaps we don't need these to be public..

[olilan, 2012-10-05 13:43:59]

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1289:
// Start a new action mode with a SelectActionModeCallback.
Do we need to remove this (seeing as we'll have to add it back later)? Could we
just not call showSelectActionBar() for now instead?

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1345:
- mNativeScrollX),
AIUI the Java style is to break lines before operators, so I think this is
correct.

On 2012/10/05 12:50:59, bulach wrote:
> nit: I know this is the same downstream, but I think the operator should go in
> the previous line (ditto in 1347, 1356 and 1358 below..)

[Iain Merrick, 2012-10-05 19:01:52]

https://codereview.chromium.org/11068010/diff/2001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/11068010/diff/2001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:168: GetMethodID(env,
java_object_->rect_clazz, "<init>", "(IIII)V");
On 2012/10/05 12:50:59, bulach wrote:
> FYI: one of us :) will need to change this depending on when this lands:
> http://codereview.chromium.org/11038015/
> 
> if that one gets in before this,
s/GetMethodID/MethodID::Get<METHODTYPE_NORMAL>/

Thanks for the heads-up -- I'll rely on the trybot and commit queue to tell me
if this is needed.

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1345:
- mNativeScrollX),
On 2012/10/05 13:43:59, olilan wrote:
> AIUI the Java style is to break lines before operators, so I think this is
> correct.
> 
> On 2012/10/05 12:50:59, bulach wrote:
> > nit: I know this is the same downstream, but I think the operator should go
in
> > the previous line (ditto in 1347, 1356 and 1358 below..)

As there are repeated expressions here, I'll just pull them out into local
variables.

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1512:
&& (mSelectionHandleController == null
On 2012/10/05 12:50:59, bulach wrote:
> nit: as above, operators in the previous lines.

I think in this case I agree with Oli -- this is the correct Java style.

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/CursorController.java
(right):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/CursorController.java:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2012/10/05 12:50:59, bulach wrote:
> nit: 2012

Done (and I'll fix other occurrences too)

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/HandleView.java
(right):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/HandleView.java:65:
public static final int LEFT = 0;
On 2012/10/05 12:50:59, bulach wrote:
> there's a bunch of public things here (and various methods below...) can they
be
> package private? apart from the @Override, I think this class is never used
> anywhere else?

Good catch. I'll check, and make things private or package-private where
possible.

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/InsertionHandleController.java
(right):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InsertionHandleController.java:44:
public void allowAutomaticShowing() {
On 2012/10/05 12:50:59, bulach wrote:
> nit: as above, perhaps we don't need these to be public..

Done.

[Iain Merrick, 2012-10-08 17:18:00]

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(left):

https://codereview.chromium.org/11068010/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1289:
// Start a new action mode with a SelectActionModeCallback.
On 2012/10/05 13:43:59, olilan wrote:
> Do we need to remove this (seeing as we'll have to add it back later)? Could
we
> just not call showSelectActionBar() for now instead?

Oops, I didn't address this comment of Oli's. I'll do so now, while rebasing the
patch.

You're right, no need to remove so much code -- I hadn't realised these parts
were already upstream.

[Iain Merrick, 2012-10-08 22:54:37]

sky, do you have time to review this? Or should I find another reviewer for the
content/browser changes?

[sky, 2012-10-09 00:09:30]

https://codereview.chromium.org/11068010/diff/14001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/11068010/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:33: default:
NOTREACHED() << "Unsupported text direction " << dir;
Can you remove this and move outside the switch?

https://codereview.chromium.org/11068010/diff/14001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:275: if
(content_view_core_ && host_) {
Can content_view_core_ ever be NULL? And why the host_ check?

[Iain Merrick, 2012-10-10 15:01:39]

http://codereview.chromium.org/11068010/diff/14001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

http://codereview.chromium.org/11068010/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:33: default:
NOTREACHED() << "Unsupported text direction " << dir;
On 2012/10/09 00:09:30, sky wrote:
> Can you remove this and move outside the switch?

Done.

http://codereview.chromium.org/11068010/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:275: if
(content_view_core_ && host_) {
On 2012/10/09 00:09:30, sky wrote:
> Can content_view_core_ ever be NULL? And why the host_ check?

The host_ check seems pointless. I'll remove it.

I'm not sure about content_view_core_, but I do see a bunch of other methods
that check it for NULL, so I'll leave that in.

We generally seem to be a bit inconsistent about NULL checks in this class --
there are some methods that use host_ without checking it, for example. Maybe a
general cleanup is needed.

[sky, 2012-10-10 16:39:31]

LGTM - yes on the cleanup.

[Iain Merrick, 2012-10-10 17:01:48]

On 2012/10/10 16:39:31, sky wrote:
> LGTM - yes on the cleanup.

Great, thanks. I'll file a separate bug for that.

[commit-bot: I haz the power, 2012-10-10 17:02:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/11068010/20001

[Iain Merrick, 2012-10-10 17:42:58]

On 2012/10/10 17:01:48, Iain Merrick wrote:
> On 2012/10/10 16:39:31, sky wrote:
> > LGTM - yes on the cleanup.
> 
> Great, thanks. I'll file a separate bug for that.

Filed http://code.google.com/p/chromium/issues/detail?id=155080

[nilesh, 2012-10-10 20:35:42]

http://codereview.chromium.org/11068010/diff/20001/content/browser/android/co...
File content/browser/android/content_view_core_impl.h (right):

http://codereview.chromium.org/11068010/diff/20001/content/browser/android/co...
content/browser/android/content_view_core_impl.h:201: const gfx::Rect& end_rect,
base::i18n::TextDirection end_dir) OVERRIDE;
Unless I am missing something, you need to add this to the public interface as
well. It will break the clang bot?

[commit-bot: I haz the power, 2012-10-11 05:03:15]

List of reviewers changed. nileshagrawal@chromium.org did a drive-by without
LGTM'ing!

[Iain Merrick, 2012-10-11 09:51:22]

http://codereview.chromium.org/11068010/diff/20001/content/browser/android/co...
File content/browser/android/content_view_core_impl.h (right):

http://codereview.chromium.org/11068010/diff/20001/content/browser/android/co...
content/browser/android/content_view_core_impl.h:201: const gfx::Rect& end_rect,
base::i18n::TextDirection end_dir) OVERRIDE;
On 2012/10/10 20:35:42, nileshagrawal1 wrote:
> Unless I am missing something, you need to add this to the public interface as
> well. It will break the clang bot?

Argh, good catch! I need to add content_view_core.h to this patch.

[Iain Merrick, 2012-10-11 10:44:41]

On 2012/10/11 09:51:22, Iain Merrick wrote:
>
http://codereview.chromium.org/11068010/diff/20001/content/browser/android/co...
> File content/browser/android/content_view_core_impl.h (right):
> 
>
http://codereview.chromium.org/11068010/diff/20001/content/browser/android/co...
> content/browser/android/content_view_core_impl.h:201: const gfx::Rect&
end_rect,
> base::i18n::TextDirection end_dir) OVERRIDE;
> On 2012/10/10 20:35:42, nileshagrawal1 wrote:
> > Unless I am missing something, you need to add this to the public interface
as
> > well. It will break the clang bot?
> 
> Argh, good catch! I need to add content_view_core.h to this patch.

On second thoughts, I just removed the "virtual", as we evidently don't need
this method to be pure virtual in ContentViewCore for the currently upstreamed
code.

When we upstream something that needs this method to be virtual, we can make it
virtual then.

[nilesh, 2012-10-11 14:12:12]

LGTM, thanks

[commit-bot: I haz the power, 2012-10-11 14:12:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/11068010/23002

[commit-bot: I haz the power, 2012-10-11 16:19:09]

Change committed as 161353