Adding Text Selection Action Bar Menu Unit Test cases.

Add text selection ActionBar tests

Currently there is insufficient coverage for text selection ActionBar
menu interaction. Add several basic tests covering such functionality.

Committed: https://crrev.com/0491238962fd5f21446be0383ac4748a8e7f600d
Cr-Commit-Position: refs/heads/master@{#301408}

[AKVT, 2014-09-04 19:12:47]

ajith.v@samsung.com changed reviewers:
+ aurimas@chromium.org, jdduke@chromium.org

[AKVT, 2014-09-04 19:12:48]

@jdduke & aurimas PTAL
Please suggest if we can add some wrapper call in SelectActionModeCallback for
just for emulating ActionBar menu clicks without interfering ImeAdapter calls.

[jdduke (slow), 2014-09-04 19:31:00]

not lgtm. I'd prefer we either figure out another way of testing this behavior
that doesn't involve exposing all of these internals, or leave it as is.

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

https://codereview.chromium.org/538103003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:276:
private SelectActionModeCallback mSelectActionModeCallback;
The testing here doesn't seem to justify the additional boilerplate and leaky
details, imho.

https://codereview.chromium.org/538103003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2094:
mSelectActionModeCallback = (SelectActionModeCallback) getContentViewClient().
I'm not sure we can make the assumption that the returned ActionMode.Callback is
of type SelectActionModeCallback.

[AKVT, 2014-09-05 10:09:15]

@jdduke Thanks for the comments.
Other than this, I am not finding easy way of testing this functionality. Other
option I thought is to programmatically click on top of the screen (Approximate
position of Action Bar when it appears) and the make all these happen
automatically. Will check on that further. Please share if you have any other
suggestions.

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

https://codereview.chromium.org/538103003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:276:
private SelectActionModeCallback mSelectActionModeCallback;
On 2014/09/04 19:31:00, jdduke wrote:
> The testing here doesn't seem to justify the additional boilerplate and leaky
> details, imho.

Agree with you, there is a leak always when we create anonymous object of
ActionMode callback, now it is captured in variable.

https://codereview.chromium.org/538103003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2094:
mSelectActionModeCallback = (SelectActionModeCallback) getContentViewClient().
On 2014/09/04 19:31:00, jdduke wrote:
> I'm not sure we can make the assumption that the returned ActionMode.Callback
is
> of type SelectActionModeCallback.
Thanks. For contentshell we can make assumption, but not for all cases. I think
keep this conversion inside ContentViewCoreSelectionTest.java file. Will do
that.

[jdduke (slow), 2014-09-05 15:14:24]

So, the problem is that this taste conflates several different concerns, which
are:

1) Does SelectActionModeCallback properly configure itself given an
ActionHandler? - This can be a unit test, there's no need to pull in
ContentViewCore and ContentShellTestBase, and this avoids needing to have
ContentViewCore expose any of its guts.

2) Does ContentViewCore properly implement ActionHandler? In this case, you
would make SelectActionModeCallback.ActionHandler a member variable (created
on-demand and cached, instead of creating a new one every time we create the
action bar), and expose it for testing. Then you can directly query the members
like |isSelectionPassword()|, |isSelectionEditable()| and the associated
actions.

[aurimas (slooooooooow), 2014-10-20 22:59:20]

aurimas@chromium.org changed reviewers:
- aurimas@chromium.org

[AKVT, 2014-10-27 15:38:53]

Patchset #3 (id:40001) has been deleted

[AKVT, 2014-10-27 16:16:02]

@jdduke PTAL. I have created a basic version.
I would like to ask whether I can keep the changes done in PS1 inside
SelctActionModeCallback.java as below;

@VisibleForTesting
public Menu getSelectActionBarMenuForTest() {
        return mMenu;
}

This will help me to add more coverage for the tests. Please share your opinion.

[jdduke (slow), 2014-10-27 16:31:25]

https://codereview.chromium.org/538103003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/538103003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1747:
mActionHandler = new SelectActionModeCallback.ActionHandler() {
Do we need to recreate this every time? Can we do "if (mActionHandler == null)
{" and only create it then?

[AKVT, 2014-10-27 16:37:12]

On 2014/10/27 16:31:25, jdduke wrote:
>
https://codereview.chromium.org/538103003/diff/60001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/538103003/diff/60001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1747:
> mActionHandler = new SelectActionModeCallback.ActionHandler() {
> Do we need to recreate this every time? Can we do "if (mActionHandler == null)
> {" and only create it then?

You mean while hiding the Select Action Bar, we should clear this variable and
recreate next time ? Or will keep it active until CVC is destroyed ?

[jdduke (slow), 2014-10-27 16:40:25]

On 2014/10/27 16:37:12, AKV wrote:
> You mean while hiding the Select Action Bar, we should clear this variable and
> recreate next time ? Or will keep it active until CVC is destroyed ?

I don't think we need to clear it, as it looks like it doesn't reference any
local state, so I would just keep it active until CVC is destroyed.

[AKVT, 2014-10-27 17:23:05]

@jdduke PTAL. I kept one time object creation for ActionHandler

https://codereview.chromium.org/538103003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/538103003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1747:
mActionHandler = new SelectActionModeCallback.ActionHandler() {
On 2014/10/27 16:31:25, jdduke wrote:
> Do we need to recreate this every time? Can we do "if (mActionHandler == null)
> {" and only create it then?

Done. Thanks

[jdduke (slow), 2014-10-27 18:18:54]

lgtm with one suggestion.

https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
(right):

https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:209:
assertTrue(mContentViewCore.getSelectActionHandler().isSelectionEditable());
I'd add a |assertNotNull(mContentViewCore.getSelectActionHandler()| both here
and below before accessing.

[AKVT, 2014-10-27 18:28:52]

On 2014/10/27 18:18:54, jdduke wrote:
> lgtm with one suggestion.
> 
>
https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
> (right):
> 
>
https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
>
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:209:
> assertTrue(mContentViewCore.getSelectActionHandler().isSelectionEditable());
> I'd add a |assertNotNull(mContentViewCore.getSelectActionHandler()| both here
> and below before accessing.

Thanks.

Below change can I keep for adding more cases in a follow up patch for
validating the menus are correct or not ?

@VisibleForTesting
public Menu getSelectActionBarMenuForTest() {
        return mMenu;
}
Otherwise I have to blindly call the options without knowing whether option is
present on action bar or not. Please share your opinion on this.

[AKVT, 2014-10-27 18:40:36]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-10-27 18:42:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/538103003/100001

[jdduke (slow), 2014-10-27 18:45:00]

On 2014/10/27 18:28:52, AKV wrote:
> On 2014/10/27 18:18:54, jdduke wrote:
> > lgtm with one suggestion.
> > 
> >
>
https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
> > File
> >
>
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
> >
>
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:209:
> > assertTrue(mContentViewCore.getSelectActionHandler().isSelectionEditable());
> > I'd add a |assertNotNull(mContentViewCore.getSelectActionHandler()| both
here
> > and below before accessing.
> 
> Thanks.
> 
> Below change can I keep for adding more cases in a follow up patch for
> validating the menus are correct or not ?
> 
> @VisibleForTesting
> public Menu getSelectActionBarMenuForTest() {
>         return mMenu;
> }
> Otherwise I have to blindly call the options without knowing whether option is
> present on action bar or not. Please share your opinion on this.

It sounds like what you really should be doing is unit testing
SelectActionModeCallback separately. We now have support for unit tests (yay),
so I would explore that option if you want to test things like whether the menu
item is present.

[AKVT, 2014-10-27 18:53:36]

On 2014/10/27 18:45:00, jdduke wrote:
> On 2014/10/27 18:28:52, AKV wrote:
> > On 2014/10/27 18:18:54, jdduke wrote:
> > > lgtm with one suggestion.
> > > 
> > >
> >
>
https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
> > > File
> > >
> >
>
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/538103003/diff/80001/content/public/android/j...
> > >
> >
>
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java:209:
> > >
assertTrue(mContentViewCore.getSelectActionHandler().isSelectionEditable());
> > > I'd add a |assertNotNull(mContentViewCore.getSelectActionHandler()| both
> here
> > > and below before accessing.
> > 
> > Thanks.
> > 
> > Below change can I keep for adding more cases in a follow up patch for
> > validating the menus are correct or not ?
> > 
> > @VisibleForTesting
> > public Menu getSelectActionBarMenuForTest() {
> >         return mMenu;
> > }
> > Otherwise I have to blindly call the options without knowing whether option
is
> > present on action bar or not. Please share your opinion on this.
> 
> It sounds like what you really should be doing is unit testing
> SelectActionModeCallback separately. We now have support for unit tests (yay),
> so I would explore that option if you want to test things like whether the
menu
> item is present.

Thanks, With next patch, I will try to cover few more in that aspect.

[commit-bot: I haz the power, 2014-10-27 19:31:38]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-10-27 19:32:27]

Patchset 5 (id:??) landed as
https://crrev.com/0491238962fd5f21446be0383ac4748a8e7f600d
Cr-Commit-Position: refs/heads/master@{#301408}