aw: Rendering test harness and end-to-end smoke test

aw: Rendering test harness and end-to-end smoke test

The test harness is heavily modelled after LayerTreeTests in
cc.

Add implementation for simulating invalidation and functor
behavior. Add in enough hooks to allow a end-to-end smoke
test from invalidation to draw to functor draw.

BUG=418346
Committed: https://crrev.com/ec5d6de3135398882a0975c95de98d5111aa6c2e
Cr-Commit-Position: refs/heads/master@{#307812}

[boliu, 2014-12-08 23:50:46]

boliu@chromium.org changed reviewers:
+ hush@chromium.org, mkosiba@chromium.org

[boliu, 2014-12-08 23:50:47]

2 people to look over everything :)

I'm not attached to the "FakeViewRootImpl" name. Open to better suggestions.

[boliu, 2014-12-10 01:52:27]

ping! :)

[hush (inactive), 2014-12-10 20:07:58]

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
File android_webview/browser/test/fake_view_root_impl.cc (right):

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:103:
on_draw_hardware_pending_ = true;
could there be a race condition with line 111?
PostInvalidate and OnDrawHardware are run on two different threads right?

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:128: {
why is this in its own block?

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:142: draw_info.clip_left =
location.x();
just use location_ in this function?

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:178: void
FakeViewRootImpl::CheckRenderThread() {
this function is basically "InitializeRenderThreadIfNeeded"?

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
File android_webview/browser/test/fake_view_root_impl.h (right):

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.h:63: BrowserViewRenderer*
view_;
so the following pointers need to be const?

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.h:68: gfx::Rect location_;
how is the location a rect? You mean the view itself right? (its origin + its
size)

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
File android_webview/browser/test/rendering_test.h (right):

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/rendering_test.h:68: private:
you have multiple "protected" and "private" sections in this header file. Merge
them?

[boliu, 2014-12-10 21:30:32]

No new patch set, just replying to comments

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
File android_webview/browser/test/fake_view_root_impl.cc (right):

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:103:
on_draw_hardware_pending_ = true;
On 2014/12/10 20:07:58, hush wrote:
> could there be a race condition with line 111?
> PostInvalidate and OnDrawHardware are run on two different threads right?

PostInvalidate and OnDrawHardware are both called on the UI thread.

Although I should add thread checkers to this class.

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:128: {
On 2014/12/10 20:07:58, hush wrote:
> why is this in its own block?

So AwDrawGLInfo can't be reused for the kModeDraw call below. Just a nicety

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:142: draw_info.clip_left =
location.x();
On 2014/12/10 20:07:58, hush wrote:
> just use location_ in this function?

I imagined location_ to be UI only.

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.cc:178: void
FakeViewRootImpl::CheckRenderThread() {
On 2014/12/10 20:07:57, hush wrote:
> this function is basically "InitializeRenderThreadIfNeeded"?

sgtm!

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
File android_webview/browser/test/fake_view_root_impl.h (right):

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.h:63: BrowserViewRenderer*
view_;
On 2014/12/10 20:07:58, hush wrote:
> so the following pointers need to be const?

Umm, I think I can declare it "BVR* const view" or something, ie a const pointer
to a non-const object.

The more expected "const BVR* view_" is a non-const pointer to a const object,
which means I can't call any non-const methods.

I thought the first form is uncommon, but apparently
$ git grep "\* const" | wc -l
5578

So I'll do that

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/fake_view_root_impl.h:68: gfx::Rect location_;
On 2014/12/10 20:07:58, hush wrote:
> how is the location a rect? You mean the view itself right? (its origin + its
> size)

Yeah.

This is supposed to be the viewrootimpl or the window, so conceptually you could
have multiple webviews at different locations. Of course I didn't write this to
support multiple webviews because I don't think we'll need that level of
support.

Hey, maybe FakeWindow is a better name.

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
File android_webview/browser/test/rendering_test.h (right):

https://codereview.chromium.org/786533002/diff/140001/android_webview/browser...
android_webview/browser/test/rendering_test.h:68: private:
On 2014/12/10 20:07:58, hush wrote:
> you have multiple "protected" and "private" sections in this header file.
Merge
> them?

Style guide agrees with you:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

I'll update it.

[boliu, 2014-12-10 22:06:01]

new patch set up, ptal

[hush (inactive), 2014-12-10 22:41:38]

lgtm

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
File android_webview/browser/test/fake_window.h (right):

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
android_webview/browser/test/fake_window.h:73: // UI on
on UI?

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
File android_webview/browser/test/rendering_test.cc (right):

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
android_webview/browser/test/rendering_test.cc:14: ui_proxy_ =
base::MessageLoopProxy::current();
why do you remove it from initialization list?

[boliu, 2014-12-10 23:14:11]

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
File android_webview/browser/test/fake_window.h (right):

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
android_webview/browser/test/fake_window.h:73: // UI on
On 2014/12/10 22:41:38, hush wrote:
> on UI?

UI thread only memebers.

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
File android_webview/browser/test/rendering_test.cc (right):

https://codereview.chromium.org/786533002/diff/160001/android_webview/browser...
android_webview/browser/test/rendering_test.cc:14: ui_proxy_ =
base::MessageLoopProxy::current();
On 2014/12/10 22:41:38, hush wrote:
> why do you remove it from initialization list?

Initializer list must come in the order things are declared. And with the
protected/private shuffle, message_loop_ is declared after ui_proxy_. But
getting the current proxy requires having a MessageLoop first/

[boliu, 2014-12-10 23:16:28]

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

[boliu, 2014-12-10 23:16:29]

+sievers for stamping DEPS

...and maybe also scan FakeWindow::*OnRT where I create a real context and fake
a functor with it.

[no sievers, 2014-12-10 23:24:11]

On 2014/12/10 23:16:29, boliu wrote:
> +sievers for stamping DEPS
> 
> ...and maybe also scan FakeWindow::*OnRT where I create a real context and
fake
> a functor with it.

lgtm

[boliu, 2014-12-10 23:26:18]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-12-10 23:28:46]

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

[commit-bot: I haz the power, 2014-12-11 00:54:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-12-11 00:55:45]

Patchset 10 (id:??) landed as
https://crrev.com/ec5d6de3135398882a0975c95de98d5111aa6c2e
Cr-Commit-Position: refs/heads/master@{#307812}