Adds unit tests for AwContentsClientCallbackHelper

Adds unit tests for AwContentsClientCallbackHelper

Also filled in some JavaDocs to satisfy the linter.

BUG=355072
TEST=AndroidWebviewTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263041

[mkosiba (inactive), 2014-04-10 09:06:05]

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java
(right):

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:79:
static final float OLD_SCALE = 2.0f;
uber-nit: since you're verifying the ratio of the two having one of the values
== 1 isn't desirable :) or you could just change scale helper to also have
new/old getters.

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:97:
public void testCallback_loadResource() throws Exception {
the Chromium C++ naming convention is hacka_style_naming and the Java one is
goodOlCamelCase. mixing is not allowed so please change this to something like
testLoadResource.

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:151:
// before mLooper processes the first so we run them in the mLooper thread
This works because a) we schedule two callbacks in one runnable and b) we run
the runnable on the same thread on which the callbacks are processed on. The
comment seems to only cover a) and not b).

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:170:
pictureListenerHelper.waitForCallback(onNewPictureCount + 1);
It's a bit unfortunate to do it this way since we'll end up adding an
unnecessary 5 sec to our test time.
What you could do instead is to schedule an extra Runnable on the mLooper and
assert that when that runnable has run only one callback has been processed.

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

https://codereview.chromium.org/230673004/diff/1/content/public/test/android/...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:93:
public static class OnDownloadStartHelper extends CallbackHelper {
these could just live in TestAwContentsClient (like
ShouldOverrideUrlLoadingHelper does). This class is primarily used by a
different set of tests.

[hjd_google, 2014-04-10 12:48:40]

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java
(right):

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:79:
static final float OLD_SCALE = 2.0f;
On 2014/04/10 09:06:05, mkosiba wrote:
> uber-nit: since you're verifying the ratio of the two having one of the values
> == 1 isn't desirable :) or you could just change scale helper to also have
> new/old getters.

Done.

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:97:
public void testCallback_loadResource() throws Exception {
On 2014/04/10 09:06:05, mkosiba wrote:
> the Chromium C++ naming convention is hacka_style_naming and the Java one is
> goodOlCamelCase. mixing is not allowed so please change this to something like
> testLoadResource.

Done.

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:151:
// before mLooper processes the first so we run them in the mLooper thread
On 2014/04/10 09:06:05, mkosiba wrote:
> This works because a) we schedule two callbacks in one runnable and b) we run
> the runnable on the same thread on which the callbacks are processed on. The
> comment seems to only cover a) and not b).

Done.

https://codereview.chromium.org/230673004/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:170:
pictureListenerHelper.waitForCallback(onNewPictureCount + 1);
On 2014/04/10 09:06:05, mkosiba wrote:
> It's a bit unfortunate to do it this way since we'll end up adding an
> unnecessary 5 sec to our test time.
> What you could do instead is to schedule an extra Runnable on the mLooper and
> assert that when that runnable has run only one callback has been processed.

Done.

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

https://codereview.chromium.org/230673004/diff/1/content/public/test/android/...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:93:
public static class OnDownloadStartHelper extends CallbackHelper {
On 2014/04/10 09:06:05, mkosiba wrote:
> these could just live in TestAwContentsClient (like
> ShouldOverrideUrlLoadingHelper does). This class is primarily used by a
> different set of tests.

Done.

[mkosiba (inactive), 2014-04-10 14:34:52]

lgtm

+Sami for content/public/test OWNERS rubber-stamp

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java
(right):

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:96:
public void testLoadResource() throws Exception {
nit: sorry, the name should be testOnLoadResource since the callback is
onLoadResource. Should just be a matter of %s/test/testOn/gc though :)

[Sami, 2014-04-10 14:48:59]

Good job, lgmt.

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java
(right):

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:184:
public void testRecivedLoginRequest() throws Exception {
typo: Received

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:198:
public void testRecivedError() throws Exception {
ditto

[Sami, 2014-04-10 14:49:07]

erm, lgtm

[mkosiba (inactive), 2014-04-10 14:56:19]

The CQ bit was checked by mkosiba@chromium.org

[mkosiba (inactive), 2014-04-10 14:56:22]

The CQ bit was unchecked by mkosiba@chromium.org

[hjd_google, 2014-04-10 15:15:23]

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java
(right):

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:96:
public void testLoadResource() throws Exception {
On 2014/04/10 14:34:53, mkosiba wrote:
> nit: sorry, the name should be testOnLoadResource since the callback is
> onLoadResource. Should just be a matter of %s/test/testOn/gc though :)

Done.

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:184:
public void testRecivedLoginRequest() throws Exception {
On 2014/04/10 14:48:59, Sami wrote:
> typo: Received
oops :(

https://codereview.chromium.org/230673004/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientCallbackHelperTest.java:198:
public void testRecivedError() throws Exception {
On 2014/04/10 14:48:59, Sami wrote:
> ditto

Done.

[mkosiba (inactive), 2014-04-10 15:21:57]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-04-10 15:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hjd@google.com/230673004/30001

[commit-bot: I haz the power, 2014-04-10 19:09:20]

Change committed as 263041