[Android] Upstream content detection and ChromeBrowserProvider tests.

[Android] Upstream content detection and ChromeBrowserProvider tests.


BUG=125390, 138755
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=161101

[Leandro Graciá Gil, 2012-10-09 00:52:53]

sky: change in android_provider_backend.cc

yfriedman: upstreaming the Android code. The only relevant changes compared with
downstream is the workaround introduced in DOMUtils.java in order to produce
correct tap coordinates with fixed page scale factors until our scaling code is
re-implemented and upstreamed.

Also, there seems to be currently some issues in chromium test shell. Depending
on how these goes, the ChromeBrowserProvider tests might be dropped from this
patch.

[Yaron, 2012-10-09 02:12:45]

lgtm % some comments and +Karin

I think you need to split the .png files into a separate commit and dcommit
that. Also, the ContentViewCore change can be deferred if you want.

One thing that strikes me is that I think we have the tests in the wrong folder.
I think the content tests should be in content/public/android/javatests and the
chrome test should be in chrome/android/javatests. Karin?

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:114:
// scale settings of 1.0 until the new compositor correctly provides the
ContentView page scale.
This is missing code for the new --ui-compositor not code that needs to be
upstreamed, right?

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/JavaScriptUtils.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/JavaScriptUtils.java:24:
final ContentView view, TestCallbackHelperContainer viewClient, final String
code)
We should probably make this operate on a ContentViewCore

[Leandro Graciá Gil, 2012-10-09 13:16:27]

A few comments:

On 2012/10/09 02:12:45, Yaron wrote:
> I think you need to split the .png files into a separate commit and dcommit
> that.

I think that was required when adding Chrome resources because there were issues
with the automatic id generation and grit. However, these are simply files that
will be pushed to the device by the test scripts before running the tests. I
think we probably don't need to upload them separately in this case, but if
there are any issues I definitely will.


> One thing that strikes me is that I think we have the tests in the wrong
folder.
> I think the content tests should be in content/public/android/javatests and
the
> chrome test should be in chrome/android/javatests. Karin?

According to this they are in the right place:
https://sites.google.com/a/chromium.org/dev/developers/testing/android-tests

[sky, 2012-10-09 16:05:35]

android_provider_backend LGTM

[klundberg, 2012-10-09 17:07:01]

See inline comments wrt location of the tests.

https://codereview.chromium.org/11085008/diff/6001/chrome/android/javatests/s...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/util/BookmarkUtils.java
(right):

https://codereview.chromium.org/11085008/diff/6001/chrome/android/javatests/s...
chrome/android/javatests/src/org/chromium/chrome/browser/test/util/BookmarkUtils.java:5:
package org.chromium.chrome.testshell.util;
Would it ever make sense to share this with downstream code?
If so, it should be placed in chrome/test/android/javatests/...

https://codereview.chromium.org/11085008/diff/6001/chrome/android/testshell/j...
File
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ProviderBookmarkNodeTest.java
(right):

https://codereview.chromium.org/11085008/diff/6001/chrome/android/testshell/j...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ProviderBookmarkNodeTest.java:22:
public class ProviderBookmarkNodeTest extends ChromiumTestShellTestBase {
The question is whether this is a high level integration test or whether this
specifically test code in chrome/android/java.

If it specifically tests code in chrome/android/java, it should live in
chrome/android/javatests (although still extending ChromiumTestShellTestBase).

As mentioned in a comment for the content test, I will update the documentation
to reflect this.

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TouchCommon.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TouchCommon.java:19:
public TouchCommon(ActivityInstrumentationTestCase2 activityTestCase) {
It would be better to just pass the activity instead of the test case (law of
demeter - pass only what you really need).

And you can use org.chromium.content.browser.test.util.UiUtil to run on the UI
thread when you have the activity so you don't really need the test case.

https://codereview.chromium.org/11085008/diff/6001/content/shell/android/java...
File
content/shell/android/javatests/src/org/chromium/content_shell/AddressDetectionTest.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/shell/android/java...
content/shell/android/javatests/src/org/chromium/content_shell/AddressDetectionTest.java:15:
public class AddressDetectionTest extends ContentDetectionTestBase {
So the test should in general live next to the code that it tests. 

Is the code for this in content/public? Then this should be moved to
content/public/android/javatests.
Similar comment for the other tests.

This is not explicitly pointed out in
https://sites.google.com/a/chromium.org/dev/developers/testing/android-tests but
we had a discussion about this in a CL.
I'll update the documentation.

[klundberg, 2012-10-09 17:20:12]

And I have updated the documentation:
https://sites.google.com/a/chromium.org/dev/developers/testing/android-tests

Please let me know if you have any comments to the documentation or the general
guidelines.

On 2012/10/09 17:07:01, klundberg wrote:
> See inline comments wrt location of the tests.
> 
>
https://codereview.chromium.org/11085008/diff/6001/chrome/android/javatests/s...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/test/util/BookmarkUtils.java
> (right):
> 
>
https://codereview.chromium.org/11085008/diff/6001/chrome/android/javatests/s...
>
chrome/android/javatests/src/org/chromium/chrome/browser/test/util/BookmarkUtils.java:5:
> package org.chromium.chrome.testshell.util;
> Would it ever make sense to share this with downstream code?
> If so, it should be placed in chrome/test/android/javatests/...
> 
>
https://codereview.chromium.org/11085008/diff/6001/chrome/android/testshell/j...
> File
>
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ProviderBookmarkNodeTest.java
> (right):
> 
>
https://codereview.chromium.org/11085008/diff/6001/chrome/android/testshell/j...
>
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ProviderBookmarkNodeTest.java:22:
> public class ProviderBookmarkNodeTest extends ChromiumTestShellTestBase {
> The question is whether this is a high level integration test or whether this
> specifically test code in chrome/android/java.
> 
> If it specifically tests code in chrome/android/java, it should live in
> chrome/android/javatests (although still extending ChromiumTestShellTestBase).
> 
> As mentioned in a comment for the content test, I will update the
documentation
> to reflect this.
> 
>
https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
> File
>
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TouchCommon.java
> (right):
> 
>
https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
>
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TouchCommon.java:19:
> public TouchCommon(ActivityInstrumentationTestCase2 activityTestCase) {
> It would be better to just pass the activity instead of the test case (law of
> demeter - pass only what you really need).
> 
> And you can use org.chromium.content.browser.test.util.UiUtil to run on the UI
> thread when you have the activity so you don't really need the test case.
> 
>
https://codereview.chromium.org/11085008/diff/6001/content/shell/android/java...
> File
>
content/shell/android/javatests/src/org/chromium/content_shell/AddressDetectionTest.java
> (right):
> 
>
https://codereview.chromium.org/11085008/diff/6001/content/shell/android/java...
>
content/shell/android/javatests/src/org/chromium/content_shell/AddressDetectionTest.java:15:
> public class AddressDetectionTest extends ContentDetectionTestBase {
> So the test should in general live next to the code that it tests. 
> 
> Is the code for this in content/public? Then this should be moved to
> content/public/android/javatests.
> Similar comment for the other tests.
> 
> This is not explicitly pointed out in
> https://sites.google.com/a/chromium.org/dev/developers/testing/android-tests
but
> we had a discussion about this in a CL.
> I'll update the documentation.

[Leandro Graciá Gil, 2012-10-10 00:35:54]

I'm temporarily disabling all the provider tests. There seems to be random
flakiness affecting the chromium test shell and a few of these tests have
already been disabled downstream by reasons that don't seem related to the tests
themselves (bug 154683). Still, these tests should pass and should be re-enabled
again once the flakiness issues are solved.

https://codereview.chromium.org/11085008/diff/6001/chrome/android/javatests/s...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/util/BookmarkUtils.java
(right):

https://codereview.chromium.org/11085008/diff/6001/chrome/android/javatests/s...
chrome/android/javatests/src/org/chromium/chrome/browser/test/util/BookmarkUtils.java:5:
package org.chromium.chrome.testshell.util;
On 2012/10/09 17:07:01, klundberg wrote:
> Would it ever make sense to share this with downstream code?
> If so, it should be placed in chrome/test/android/javatests/...

Discussed offline and agreed to leave it here for now. Might be moved to
chrome/test/android/javatests/... in the future.

https://codereview.chromium.org/11085008/diff/6001/chrome/android/testshell/j...
File
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ProviderBookmarkNodeTest.java
(right):

https://codereview.chromium.org/11085008/diff/6001/chrome/android/testshell/j...
chrome/android/testshell/javatests/src/org/chromium/chrome/testshell/ProviderBookmarkNodeTest.java:22:
public class ProviderBookmarkNodeTest extends ChromiumTestShellTestBase {
Moving this to chrome/android/javatests.

On 2012/10/09 17:07:01, klundberg wrote:
> The question is whether this is a high level integration test or whether this
> specifically test code in chrome/android/java.
> 
> If it specifically tests code in chrome/android/java, it should live in
> chrome/android/javatests (although still extending ChromiumTestShellTestBase).
> 
> As mentioned in a comment for the content test, I will update the
documentation
> to reflect this.

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:114:
// scale settings of 1.0 until the new compositor correctly provides the
ContentView page scale.
On 2012/10/09 02:12:45, Yaron wrote:
> This is missing code for the new --ui-compositor not code that needs to be
> upstreamed, right?

Right. Once we have the new compositor we can simply use view.getScale() instead
of this method and we will get the correct values. Until then this is probably
the best we can do.

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/JavaScriptUtils.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/JavaScriptUtils.java:24:
final ContentView view, TestCallbackHelperContainer viewClient, final String
code)
On 2012/10/09 02:12:45, Yaron wrote:
> We should probably make this operate on a ContentViewCore

Deferring this to later patches / follow-ups as suggested.

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TouchCommon.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/public/test/androi...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TouchCommon.java:19:
public TouchCommon(ActivityInstrumentationTestCase2 activityTestCase) {
On 2012/10/09 17:07:01, klundberg wrote:
> It would be better to just pass the activity instead of the test case (law of
> demeter - pass only what you really need).
> 
> And you can use org.chromium.content.browser.test.util.UiUtil to run on the UI
> thread when you have the activity so you don't really need the test case.

Done.

https://codereview.chromium.org/11085008/diff/6001/content/shell/android/java...
File
content/shell/android/javatests/src/org/chromium/content_shell/AddressDetectionTest.java
(right):

https://codereview.chromium.org/11085008/diff/6001/content/shell/android/java...
content/shell/android/javatests/src/org/chromium/content_shell/AddressDetectionTest.java:15:
public class AddressDetectionTest extends ContentDetectionTestBase {
On 2012/10/09 17:07:01, klundberg wrote:
> So the test should in general live next to the code that it tests. 
> 
> Is the code for this in content/public? Then this should be moved to
> content/public/android/javatests.
> Similar comment for the other tests.
> 
> This is not explicitly pointed out in
> https://sites.google.com/a/chromium.org/dev/developers/testing/android-tests
but
> we had a discussion about this in a CL.
> I'll update the documentation.

Done.

[commit-bot: I haz the power, 2012-10-10 00:36:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11085008/2004

[commit-bot: I haz the power, 2012-10-10 00:36:25]

Can't process patch for file
chrome/test/data/android/device_files/provider/icon1.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[Leandro Graciá Gil, 2012-10-10 00:42:06]

Despite this bug very probably doesn't affect this case I'll be doing as
suggested. Here's the separate CL with the resources:
https://codereview.chromium.org/11066092  (no approval needed)

On 2012/10/10 00:36:25, I haz the power (commit-bot) wrote:
> Can't process patch for file
> chrome/test/data/android/device_files/provider/icon1.png.
> Binary file support is temporarilly disabled due to a bug. Please commit
blindly
> the binary files first then commit the source change as a separate CL. Sorry
for
> the annoyance.

[commit-bot: I haz the power, 2012-10-10 01:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11085008/10005

[commit-bot: I haz the power, 2012-10-10 01:30:16]

Presubmit check for 11085008-10005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
 
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionTestBase.java,
line 18, 101 chars \
 
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java,
line 11, 101 chars

Presubmit checks took 1.6s to calculate.

[Yaron, 2012-10-10 01:34:59]

On 2012/10/10 01:30:16, I haz the power (commit-bot) wrote:
> Presubmit check for 11085008-10005 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> Found lines longer than 80 characters (first 5 shown).
>  
>
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionTestBase.java,
> line 18, 101 chars \
>  
>
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java,
> line 11, 101 chars
> 
> Presubmit checks took 1.6s to calculate.

Ugh. Siva just landed a fix (https://chromiumcodereview.appspot.com/11043031)
but I guess it hasn't been propagated.

[commit-bot: I haz the power, 2012-10-10 01:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11085008/10005

[commit-bot: I haz the power, 2012-10-10 01:36:49]

Presubmit check for 11085008-10005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
 
content/public/android/javatests/src/org/chromium/content/browser/ContentDetectionTestBase.java,
line 18, 101 chars \
 
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java,
line 11, 101 chars

Presubmit checks took 1.8s to calculate.