Make content_browsertests for Chrome on Android running with valid Views

Make content_browsertests for Chrome on Android running with valid Views

- adds test activity layout into real view hierarchy.
- runs tests after Activity.onCreate() is done.
- doesn't load a default URL when testing.
- enables TouchInputBrowserTest by setting initial scale for Android.
- disables ScreenOrientationBrowserTest because it uses --disable-gpu
- updates expected results of DumpAccessibilityTreeTest

BUG=338011
Committed: https://crrev.com/6f5632dd60a95005f7902a1ca24022d64ac2134c
Cr-Commit-Position: refs/heads/master@{#317979}

[Jaekyun Seok (inactive), 2015-02-09 23:04:49]

jaekyun@chromium.org changed reviewers:
+ sievers@chromium.org, tedchoc@chromium.org

[Jaekyun Seok (inactive), 2015-02-09 23:04:50]

Please review this CL.

[no sievers, 2015-02-09 23:11:20]

https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
File content/browser/renderer_host/input/touch_input_browsertest.cc (right):

https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_input_browsertest.cc:158:
->GenerateNewFrameForTesting();
Why do we need to do this, i.e. fake onSizeChanged()?

[Jaekyun Seok (inactive), 2015-02-09 23:23:14]

https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
File content/browser/renderer_host/input/touch_input_browsertest.cc (right):

https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
content/browser/renderer_host/input/touch_input_browsertest.cc:158:
->GenerateNewFrameForTesting();
On 2015/02/09 23:11:20, sievers wrote:
> Why do we need to do this, i.e. fake onSizeChanged()?

RenderWidgetHostImpl::OnUpdateRect() wasn't called at all without this. Actually
I don't understand deeply how ViewHostMsg_UpdateRect is generated.
Is there any other proper way?

[no sievers, 2015-02-11 00:42:25]

On 2015/02/09 23:23:14, Jaekyun Seok wrote:
>
https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_input_browsertest.cc (right):
> 
>
https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
> content/browser/renderer_host/input/touch_input_browsertest.cc:158:
> ->GenerateNewFrameForTesting();
> On 2015/02/09 23:11:20, sievers wrote:
> > Why do we need to do this, i.e. fake onSizeChanged()?
> 
> RenderWidgetHostImpl::OnUpdateRect() wasn't called at all without this.
Actually
> I don't understand deeply how ViewHostMsg_UpdateRect is generated.
> Is there any other proper way?

So if the view gets attached to the parent view with the right layout params and
so fort, then onSizeChanged() should happen correctly already.
Maybe something is wrong with the hierarchy or layout (you could try
hierarchy-viewer), or there is some subtlety with the nested message loops and
test expectations or so.

[Jaekyun Seok (inactive), 2015-02-11 02:05:29]

On 2015/02/11 00:42:25, sievers wrote:
> On 2015/02/09 23:23:14, Jaekyun Seok wrote:
> >
>
https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
> > File content/browser/renderer_host/input/touch_input_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/888793002/diff/370001/content/browser/rendere...
> > content/browser/renderer_host/input/touch_input_browsertest.cc:158:
> > ->GenerateNewFrameForTesting();
> > On 2015/02/09 23:11:20, sievers wrote:
> > > Why do we need to do this, i.e. fake onSizeChanged()?
> > 
> > RenderWidgetHostImpl::OnUpdateRect() wasn't called at all without this.
> Actually
> > I don't understand deeply how ViewHostMsg_UpdateRect is generated.
> > Is there any other proper way?
> 
> So if the view gets attached to the parent view with the right layout params
and
> so fort, then onSizeChanged() should happen correctly already.
> Maybe something is wrong with the hierarchy or layout (you could try
> hierarchy-viewer), or there is some subtlety with the nested message loops and
> test expectations or so.

I confirmed that onSizeChanged() was called, but
RenderWidgetHostImpl::OnUpdateRect() wasn't called at all.
Moreover, I was able to see the web contents on the screen.

One interesting thing is that RenderWidgetHostImpl::OnUpdateRect() isn't called
at all even when launching ContentShell.
After clicking a URL bar, RenderWidgetHostImpl::OnUpdateRect() is called. I
confirmed that clicking a URL bar caused resizing.

[Jaekyun Seok (inactive), 2015-02-12 08:33:54]

PTAL.

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:109:
// In this case, we should resize it before viewport size is set.
I'm not sure whether this could be a solution for the problem that
RenderWidgetHostImpl::OnUpdateRect() isn't called.

But I confirmed that it wasn't called when viewport size was set before valid
physical backing size was set.

Such thing is happening only on ContentShell and ContentBrowserTests, not on
ChromeShell and Chrome.

[Jaekyun Seok (inactive), 2015-02-12 08:33:57]

PTAL.

[Jaekyun Seok (inactive), 2015-02-13 10:15:22]

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:109:
// In this case, we should resize it before viewport size is set.
I found that resizing ack is ignored in
https://cs.corp.google.com/#clankium/src/content/renderer/render_widget.cc&l=822
when only physical backing size is changed.

So RenderWidgetHostImpl::OnUpdateRect() can be called only if viewport size is
changed.

That is why RenderWidgetHostImpl::OnUpdateRect() isn't called when viewport size
is set before valid physical backing size is set.

I tried to modify RenderWidget::Resize(), but I failed to make a proper change.

Daniel, are you OK with this workaround? Or do you have any better idea to fix
the resizing ack?

[no sievers, 2015-02-18 22:01:09]

sievers@chromium.org changed reviewers:
+ jdduke@chromium.org

[no sievers, 2015-02-18 22:02:38]

+jdduke@ for changes to touch_input_browsertest.cc. Do we really have to do this
ifdef everywhere that we use SyntheticWebTouchEvent?

Can you explain why the accessibility test expectations need to change? These
tests ran and passed before, no?

[jdduke (slow), 2015-02-18 22:07:03]

https://codereview.chromium.org/888793002/diff/390001/content/browser/rendere...
File content/browser/renderer_host/input/touch_input_browsertest.cc (right):

https://codereview.chromium.org/888793002/diff/390001/content/browser/rendere...
content/browser/renderer_host/input/touch_input_browsertest.cc:43: "<body
onload='setup();'>"
Can you just give this a mobile viewport?

<head>
<meta name="viewport" content="width=device-width, initial-scale=1">
</head>

Then I think you don't need to multiply by the page scale.

[no sievers, 2015-02-21 00:45:54]

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:109:
// In this case, we should resize it before viewport size is set.
On 2015/02/13 10:15:22, Jaekyun Seok (OOOuntilFeb23) wrote:
> I found that resizing ack is ignored in
>
https://cs.corp.google.com/#clankium/src/content/renderer/render_widget.cc&l=822
> when only physical backing size is changed.
> 
> So RenderWidgetHostImpl::OnUpdateRect() can be called only if viewport size is
> changed.
> 
> That is why RenderWidgetHostImpl::OnUpdateRect() isn't called when viewport
size
> is set before valid physical backing size is set.
> 
> I tried to modify RenderWidget::Resize(), but I failed to make a proper
change.
> 
> Daniel, are you OK with this workaround? Or do you have any better idea to fix
> the resizing ack?
> 

These changes in here shouldn't be needed. I think
https://codereview.chromium.org/948603002/ takes care of the problem, and I
described it over there.

[Jaekyun Seok (inactive), 2015-02-23 10:41:34]

PTAL.

I removed applying page scale factor.

And I added logic to disable actual composition to avoid crash on tests which
set  a "disable-gpu" switch.

https://codereview.chromium.org/888793002/diff/390001/content/browser/rendere...
File content/browser/renderer_host/input/touch_input_browsertest.cc (right):

https://codereview.chromium.org/888793002/diff/390001/content/browser/rendere...
content/browser/renderer_host/input/touch_input_browsertest.cc:43: "<body
onload='setup();'>"
On 2015/02/18 22:07:03, jdduke wrote:
> Can you just give this a mobile viewport?
> 
> <head>
> <meta name="viewport" content="width=device-width, initial-scale=1">
> </head>
> 
> Then I think you don't need to multiply by the page scale.

I confirmed that your suggestion worked well. I will remove applying page scale
factor.

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/888793002/diff/390001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:109:
// In this case, we should resize it before viewport size is set.
On 2015/02/21 00:45:54, sievers wrote:
> On 2015/02/13 10:15:22, Jaekyun Seok (OOOuntilFeb23) wrote:
> > I found that resizing ack is ignored in
> >
>
https://cs.corp.google.com/#clankium/src/content/renderer/render_widget.cc&l=822
> > when only physical backing size is changed.
> > 
> > So RenderWidgetHostImpl::OnUpdateRect() can be called only if viewport size
is
> > changed.
> > 
> > That is why RenderWidgetHostImpl::OnUpdateRect() isn't called when viewport
> size
> > is set before valid physical backing size is set.
> > 
> > I tried to modify RenderWidget::Resize(), but I failed to make a proper
> change.
> > 
> > Daniel, are you OK with this workaround? Or do you have any better idea to
fix
> > the resizing ack?
> > 
> 
> These changes in here shouldn't be needed. I think
> https://codereview.chromium.org/948603002/ takes care of the problem, and I
> described it over there.

I confirmed that your CL resolved the resizing problem. I will remove these
changes after your CL is landed.

https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:172:
if (!CommandLine.getInstance().hasSwitch("disable-gpu")) {
I found that ChildDiscardableSharedMemoryManagerBrowserTest.* and
ScreenOrientationBrowserTest.* crashed after applying patchset #21.
And a "disable-gpu" switch was set for both tests.
I'm not sure whether disabling composition is a proper solution or not, but I
confirmed that it worked well for those tests.
At least, it will not cause any regression because composition doesn't happen
anyway at all without this CL.

[no sievers, 2015-02-24 02:02:33]

https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:172:
if (!CommandLine.getInstance().hasSwitch("disable-gpu")) {
On 2015/02/23 10:41:34, Jaekyun Seok (OOOuntilFeb23) wrote:
> I found that ChildDiscardableSharedMemoryManagerBrowserTest.* and
> ScreenOrientationBrowserTest.* crashed after applying patchset #21.
> And a "disable-gpu" switch was set for both tests.
> I'm not sure whether disabling composition is a proper solution or not, but I
> confirmed that it worked well for those tests.
> At least, it will not cause any regression because composition doesn't happen
> anyway at all without this CL.

--disable-gpu is not supported on Android.

Let's figure out if the crashing tests are relevant on Android (they probably
are), and then either fix or disable them.

[Jaekyun Seok (inactive), 2015-02-24 12:49:50]

PTAL.

I disabled tests using --disable-gpu.

https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:172:
if (!CommandLine.getInstance().hasSwitch("disable-gpu")) {
On 2015/02/24 02:02:32, sievers wrote:
> On 2015/02/23 10:41:34, Jaekyun Seok (OOOuntilFeb23) wrote:
> > I found that ChildDiscardableSharedMemoryManagerBrowserTest.* and
> > ScreenOrientationBrowserTest.* crashed after applying patchset #21.
> > And a "disable-gpu" switch was set for both tests.
> > I'm not sure whether disabling composition is a proper solution or not, but
I
> > confirmed that it worked well for those tests.
> > At least, it will not cause any regression because composition doesn't
happen
> > anyway at all without this CL.
> 
> --disable-gpu is not supported on Android.
> 
> Let's figure out if the crashing tests are relevant on Android (they probably
> are), and then either fix or disable them.

Then I will disable those tests as a workaround, and after this CL is landed I
will try to fix them.

[no sievers, 2015-02-24 19:46:12]

On 2015/02/24 12:49:50, Jaekyun Seok (OOOuntilFeb23) wrote:
> PTAL.
> 
> I disabled tests using --disable-gpu.
> 
>
https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
> File
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
> (right):
> 
>
https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
>
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:172:
> if (!CommandLine.getInstance().hasSwitch("disable-gpu")) {
> On 2015/02/24 02:02:32, sievers wrote:
> > On 2015/02/23 10:41:34, Jaekyun Seok (OOOuntilFeb23) wrote:
> > > I found that ChildDiscardableSharedMemoryManagerBrowserTest.* and
> > > ScreenOrientationBrowserTest.* crashed after applying patchset #21.
> > > And a "disable-gpu" switch was set for both tests.
> > > I'm not sure whether disabling composition is a proper solution or not,
but
> I
> > > confirmed that it worked well for those tests.
> > > At least, it will not cause any regression because composition doesn't
> happen
> > > anyway at all without this CL.
> > 
> > --disable-gpu is not supported on Android.
> > 
> > Let's figure out if the crashing tests are relevant on Android (they
probably
> > are), and then either fix or disable them.
> 
> Then I will disable those tests as a workaround, and after this CL is landed I
> will try to fix them.

See https://codereview.chromium.org/952833004/. kDisableGpu cmdline is being
removed, it wasn't needed.

[Jaekyun Seok (inactive), 2015-02-25 00:23:43]

On 2015/02/24 19:46:12, sievers wrote:
> On 2015/02/24 12:49:50, Jaekyun Seok (OOOuntilFeb23) wrote:
> > PTAL.
> > 
> > I disabled tests using --disable-gpu.
> > 
> >
>
https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
> > File
> content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/888793002/diff/410001/content/shell/android/j...
> >
>
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:172:
> > if (!CommandLine.getInstance().hasSwitch("disable-gpu")) {
> > On 2015/02/24 02:02:32, sievers wrote:
> > > On 2015/02/23 10:41:34, Jaekyun Seok (OOOuntilFeb23) wrote:
> > > > I found that ChildDiscardableSharedMemoryManagerBrowserTest.* and
> > > > ScreenOrientationBrowserTest.* crashed after applying patchset #21.
> > > > And a "disable-gpu" switch was set for both tests.
> > > > I'm not sure whether disabling composition is a proper solution or not,
> but
> > I
> > > > confirmed that it worked well for those tests.
> > > > At least, it will not cause any regression because composition doesn't
> > happen
> > > > anyway at all without this CL.
> > > 
> > > --disable-gpu is not supported on Android.
> > > 
> > > Let's figure out if the crashing tests are relevant on Android (they
> probably
> > > are), and then either fix or disable them.
> > 
> > Then I will disable those tests as a workaround, and after this CL is landed
I
> > will try to fix them.
> 
> See https://codereview.chromium.org/952833004/. kDisableGpu cmdline is being
> removed, it wasn't needed.

PTAL.

I removed ChildDiscardableSharedMemoryManager.* from
build/android/pylib/gtest/filter/content_browsertests_disabled.

But the test was disabled anyway by https://codereview.chromium.org/928903004 .

[jdduke (slow), 2015-02-25 00:53:36]

Could you also update the patch description to reflect what changes were made?
Thanks!

[no sievers, 2015-02-25 01:05:27]

This is great, thanks for being patient and letting us get the details all
sorted out!

Can you explain why the accessibility test expectations needed to change (in the
cl description ideally)?

https://codereview.chromium.org/888793002/diff/450001/build/android/pylib/gte...
File build/android/pylib/gtest/filter/content_browsertests_disabled (right):

https://codereview.chromium.org/888793002/diff/450001/build/android/pylib/gte...
build/android/pylib/gtest/filter/content_browsertests_disabled:19: # Crashes due
to using --disable-gpu
Can you file a bug and reference it here?

https://codereview.chromium.org/888793002/diff/450001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/888793002/diff/450001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:114:
super.onSizeChanged(wPix, hPix, owPix, ohPix);
This can go now after my other patches land, right?

[Jaekyun Seok (inactive), 2015-02-25 01:32:38]

> Can you explain why the accessibility test expectations needed to change (in
the cl description ideally)?

Actually I don't know the exact reason, but the changed results are
corresponding to ones of other platforms like Mac and windows.

I believe that the previous results were incorrect because android Views weren't
used actually.

https://codereview.chromium.org/888793002/diff/450001/build/android/pylib/gte...
File build/android/pylib/gtest/filter/content_browsertests_disabled (right):

https://codereview.chromium.org/888793002/diff/450001/build/android/pylib/gte...
build/android/pylib/gtest/filter/content_browsertests_disabled:19: # Crashes due
to using --disable-gpu
On 2015/02/25 01:05:27, sievers wrote:
> Can you file a bug and reference it here?

Done.

https://codereview.chromium.org/888793002/diff/450001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/888793002/diff/450001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:114:
super.onSizeChanged(wPix, hPix, owPix, ohPix);
On 2015/02/25 01:05:27, sievers wrote:
> This can go now after my other patches land, right?

Yes. I will add TODO about it.

[Jaekyun Seok (inactive), 2015-02-25 01:34:29]

On 2015/02/25 00:53:36, jdduke wrote:
> Could you also update the patch description to reflect what changes were made?
> Thanks!

Done.

[no sievers, 2015-02-25 01:35:06]

LGTM if Jared is happy

[jdduke (slow), 2015-02-25 02:04:32]

On 2015/02/25 01:35:06, sievers wrote:
> LGTM if Jared is happy

Yup, description looks good, thanks.

[Jaekyun Seok (inactive), 2015-02-25 02:29:54]

On 2015/02/25 02:04:32, jdduke wrote:
> On 2015/02/25 01:35:06, sievers wrote:
> > LGTM if Jared is happy
> 
> Yup, description looks good, thanks.

Thanks for all the reviews!!!

[Jaekyun Seok (inactive), 2015-02-25 02:30:04]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2015-02-25 02:31:13]

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

[commit-bot: I haz the power, 2015-02-25 05:01:47]

Committed patchset #25 (id:470001)

[commit-bot: I haz the power, 2015-02-25 05:02:18]

Patchset 25 (id:??) landed as
https://crrev.com/6f5632dd60a95005f7902a1ca24022d64ac2134c
Cr-Commit-Position: refs/heads/master@{#317979}