Issue 21044008: [Android] Add selection handle tests

Issue 21044008: [Android] Add selection handle tests (Closed)

Created:
7 years, 4 months ago by cjhopman

Modified:
7 years, 4 months ago

Reviewers:
Ted C

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

[Android] Add selection handle tests Add tests for: (all for editable and noneditable text) Long-press on text shows selection handles. Dragging selection handle changes selected text. Single-click dismisses selection handles. Long-press on text shows ActionMode with correct actions. Single click dismisses ActionMode. BUG=155548 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217056

Patch Set 1 #

Total comments: 45

Patch Set 2 : #

Total comments: 2

Patch Set 3 : Rebase #

Patch Set 4 : #

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+502 lines, -34 lines)			Patch
M	content/public/android/java/src/org/chromium/content/browser/ContentView.java	View	1 2	1 chunk	+0 lines, -4 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java	View	1 2 3	3 chunks	+21 lines, -1 line	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/input/InsertionHandleController.java	View		2 chunks	+3 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/input/SelectionHandleController.java	View		2 chunks	+12 lines, -0 lines	0 comments	Download
M	content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionTestBase.java	View		1 chunk	+0 lines, -16 lines	0 comments	Download
M	content/public/android/javatests/src/org/chromium/content/browser/input/InsertionHandleTest.java	View	1	1 chunk	+2 lines, -13 lines	0 comments	Download
A	content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java	View	1 2 3	1 chunk	+448 lines, -0 lines	0 comments	Download
M	content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java	View		1 chunk	+16 lines, -0 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Ted C

Yay for tests! I think only major question I have is whether you should be ...

7 years, 4 months ago (2013-08-01 18:25:25 UTC) #2

cjhopman

https://codereview.chromium.org/21044008/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentView.java File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/21044008/diff/1/content/public/android/java/src/org/chromium/content/browser/ContentView.java#newcode283 content/public/android/java/src/org/chromium/content/browser/ContentView.java:283: public String getSelectedText() { On 2013/08/01 18:25:25, Ted C ...

7 years, 4 months ago (2013-08-07 00:02:37 UTC) #3

https://codereview.chromium.org/21044008/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/21044008/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:283:
public String getSelectedText() {
On 2013/08/01 18:25:25, Ted C wrote:
> This didn't have any usages before this change, can we just remove it and just
> use the ContentViewCore one instead?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/21044008/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1160:
public String getSelectedText() {
On 2013/08/01 18:25:25, Ted C wrote:
> non private methods == needs javadoc

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java
(right):

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/08/01 18:25:25, Ted C wrote:
> 2013 is the wave of the future

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:15:
import android.util.Log;
On 2013/08/01 18:25:25, Ted C wrote:
> not used?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:83:
private static final int HANDLE_POSITION_X_TOLERANCE = 20;
On 2013/08/01 18:25:25, Ted C wrote:
> can you add a suffix to specify PX or DP

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:87:
EDITABLE(TEXTAREA_ID, TEXTAREA_DATA_URL, true),
On 2013/08/01 18:25:25, Ted C wrote:
> +2 indent to the contents of this enum

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:90:
String mNodeId;
On 2013/08/01 18:25:25, Ted C wrote:
> Should these be final?  Are they by default in enums (I don't remember)?
> 
> Only private member variables need the m prefix if you prefer to leave it out.

> We aren't very consistent with that, but it's actually not the correct style
to
> have it for anything that isn't private. 
>
http://source.android.com/source/code-style.html#follow-field-naming-conventions

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:101:
public void launchWithUrl(String url) throws Throwable {
On 2013/08/01 18:25:25, Ted C wrote:
> any reason this needs to be public?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:131:
* @throws Throwable
On 2013/08/01 18:25:25, Ted C wrote:
> I would leave out the throws for all of these since you don't describe the
usage
> (and shouldn't really need to for tests)

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:175:
assertEquals(startHandle.getPositionY(), endHandle.getPositionY());
On 2013/08/01 18:25:25, Ted C wrote:
> Don't you need "white-space: nowrap" to enforce no wrapping ever?
The text contains only single-letter words and so never has wrapping.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:181:
assertWaitForHandleViewStopped(getStartHandle());
On 2013/08/01 18:25:25, Ted C wrote:
> Is it worth calling this inside of clickNodeToShowSelectionHandles?  Maybe it
> could move it away from the center and then move it back just to isolate this
> bug and keep it consistent across all the tests?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:216:
if ((int)Math.signum(startChange) != expectedStartChange) return false;
On 2013/08/01 18:25:25, Ted C wrote:
> need space after cast
> 
> same below

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:244:
int mPositionX = -1;
On 2013/08/01 18:25:25, Ted C wrote:
> private for both of these

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:250:
mPositionX = handle.getPositionX();
On 2013/08/01 18:25:25, Ted C wrote:
> should you be getting the positions and state off the UI thread?  You might be
> getting results across multiple calls if you don't.

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:295:
return (Math.abs(handle.getAdjustedPositionX() - x) <
HANDLE_POSITION_X_TOLERANCE) &&
On 2013/08/01 18:25:25, Ted C wrote:
> again, anytime you're getting positions of stuff, should it be on the UI
thread?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:313:
private String handlePositionString(HandleView handle) {
On 2013/08/01 18:25:25, Ted C wrote:
> remove these two debug methods?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:328:
private void assertWaitForHandlesShowingEquals(final boolean shouldBeShowing)
throws Throwable {
On 2013/08/01 18:25:25, Ted C wrote:
> I would drop off the Equals myself

Hm. I would usually agree since adding the Equals doesn't really add any
information here or where the function is called (since you can see that it
takes the flag). In stack traces on failures, though, you don't get to see that
the function takes an argument. 

That being said, I'm not particularly attached to the Equals.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:342:
int steps) throws Throwable {
On 2013/08/01 18:25:25, Ted C wrote:
> you don't seem to use steps

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:381:
private Rect getNodeWindowBounds(String nodeId) throws Throwable {
On 2013/08/01 18:25:25, Ted C wrote:
> maybe add Pix to the end of the method name to be clear?

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:389:
int left = (int)renderCoordinates.fromLocalCssToPix(nodeBounds.left) + offsetX;
On 2013/08/01 18:25:25, Ted C wrote:
> space between casts

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:412:

On 2013/08/01 18:25:25, Ted C wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/21044008/diff/1/content/public/test/android/j...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CriteriaHelper.java
(right):

https://codereview.chromium.org/21044008/diff/1/content/public/test/android/j...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CriteriaHelper.java:83:
final AtomicBoolean satisfied = new AtomicBoolean();
On 2013/08/01 18:25:25, Ted C wrote:
> can you use runOnUiThreadBlocking(Callable<T> c) from ThreadUtils to avoid
using
> the AtomicBoolean?
> 
> And this isn't actually polling, which is the point of this class.  Maybe this
> should be removed and you should call ThreadUtils at the calling sites
instead?

Done. This is just a relic of a bigger change to Criteria that I was toying
with.

Ted C

lgtm https://codereview.chromium.org/21044008/diff/1/content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java File content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java (right): https://codereview.chromium.org/21044008/diff/1/content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java#newcode328 content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:328: private void assertWaitForHandlesShowingEquals(final boolean shouldBeShowing) throws Throwable { ...

7 years, 4 months ago (2013-08-07 20:52:45 UTC) #4

lgtm

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java
(right):

https://codereview.chromium.org/21044008/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:328:
private void assertWaitForHandlesShowingEquals(final boolean shouldBeShowing)
throws Throwable {
On 2013/08/07 00:02:37, cjhopman wrote:
> On 2013/08/01 18:25:25, Ted C wrote:
> > I would drop off the Equals myself
> 
> Hm. I would usually agree since adding the Equals doesn't really add any
> information here or where the function is called (since you can see that it
> takes the flag). In stack traces on failures, though, you don't get to see
that
> the function takes an argument. 
> 
> That being said, I'm not particularly attached to the Equals.

For the sake of argument, I think the Equals doesn't make sense as long as you
take an argument at all.  Wouldn't you need a Equals and NotEquals to make it
easier to see in stacks?

Oh well...carry on

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

https://codereview.chromium.org/21044008/diff/9002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1167:
* Gets the selected text (empty if no text selected).
add @return in front of this (and below)

https://codereview.chromium.org/21044008/diff/9002/content/public/android/jav...
File
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java
(right):

https://codereview.chromium.org/21044008/diff/9002/content/public/android/jav...
content/public/android/javatests/src/org/chromium/content/browser/input/SelectionHandleTest.java:236:
Point lastPosition = new Point(position);
you should be able to just do lastPosition = position.  getHandlePosition isn't
mutating the same variable.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/21044008/35001

7 years, 4 months ago (2013-08-09 23:23:34 UTC) #5

commit-bot: I haz the power

Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=186134

7 years, 4 months ago (2013-08-10 05:36:58 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/21044008/35001

7 years, 4 months ago (2013-08-12 15:24:12 UTC) #7

Message was sent while issue was closed.

Change committed as 217056

Expand Messages | Collapse Messages