OffscreenPreRaster and its plumbings

OffscreenPreRaster and its plumbings

If OffscreenPreRaster is enabled, the viewport rect for tile priority
and the tile memory limit calculation will be based on the size of the
webview, instead of the size of visible area of the webview.

Design doc here
https://docs.google.com/document/d/1xOMjvbEIS3dMzpWXWrsl7qV8QOgGEIZL1QxgnfOXmq4/

BUG=460638
Committed: https://crrev.com/d00f10451d36d173b4d900c9cf3b1d0ff639350e
Cr-Commit-Position: refs/heads/master@{#318126}

[hush (inactive), 2015-02-20 22:38:28]

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

[hush (inactive), 2015-02-20 22:38:28]

PTAL.
I will add some tests after things are settled.

[hush (inactive), 2015-02-20 22:45:56]

On 2015/02/20 22:38:28, hush wrote:
> PTAL.
> I will add some tests after things are settled.

For manual tests, I turned on this setting by default, and used feedly for a
while. Nothing exploded.

[boliu, 2015-02-20 23:16:07]

file a tracking bug? :)

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:221:
viewport_rect_for_tile_priority = viewport;
empty so compositor use its own viewport?

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:329: offscreen_pre_raster_ =
enable;
leave a TODO here, we need to think about if toggling this by itself should do
something

https://codereview.chromium.org/944053004/diff/20001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:1584:
mEventHandler.runOnUiThreadBlockingAndLocked(new Runnable() {
Wonder if we can avoid blocking here.

Probably not a big deal either way.

[hush (inactive), 2015-02-20 23:47:21]

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:221:
viewport_rect_for_tile_priority = viewport;
On 2015/02/20 23:16:07, boliu wrote:
> empty so compositor use its own viewport?

Done.

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:329: offscreen_pre_raster_ =
enable;
On 2015/02/20 23:16:07, boliu wrote:
> leave a TODO here, we need to think about if toggling this by itself should do
> something

Done.

https://codereview.chromium.org/944053004/diff/20001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:1584:
mEventHandler.runOnUiThreadBlockingAndLocked(new Runnable() {
On 2015/02/20 23:16:07, boliu wrote:
> Wonder if we can avoid blocking here.
> 
> Probably not a big deal either way.

Yeah. Not a big deal. I guess you meant to set the flag without posting to the
message queue and just calling the JNI with mAwSettingsLock? The real thing is I
want to post this method to the UI thread, so blocking is inevitable.

[hush (inactive), 2015-02-20 23:54:46]

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:329: offscreen_pre_raster_ =
enable;
On 2015/02/20 23:47:21, hush wrote:
> On 2015/02/20 23:16:07, boliu wrote:
> > leave a TODO here, we need to think about if toggling this by itself should
do
> > something
> 
> Done.

Right now, we only expect the java side to call this function. If the native
side toggles it, Java side (AwSettings) will go out of sync.

[hush (inactive), 2015-02-20 23:55:51]

On 2015/02/20 23:54:46, hush wrote:
>
https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
> File android_webview/browser/browser_view_renderer.cc (right):
> 
>
https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
> android_webview/browser/browser_view_renderer.cc:329: offscreen_pre_raster_ =
> enable;
> On 2015/02/20 23:47:21, hush wrote:
> > On 2015/02/20 23:16:07, boliu wrote:
> > > leave a TODO here, we need to think about if toggling this by itself
should
> do
> > > something
> > 
> > Done.
> 
> Right now, we only expect the java side to call this function. If the native
> side toggles it, Java side (AwSettings) will go out of sync.

On second thought,  I will just store the value natively.

[boliu, 2015-02-21 00:03:01]

On 2015/02/20 23:55:51, hush wrote:
> On second thought,  I will just store the value natively.

Just never toggle it from native side?

https://codereview.chromium.org/944053004/diff/20001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:1584:
mEventHandler.runOnUiThreadBlockingAndLocked(new Runnable() {
On 2015/02/20 23:47:21, hush wrote:
> On 2015/02/20 23:16:07, boliu wrote:
> > Wonder if we can avoid blocking here.
> > 
> > Probably not a big deal either way.
> 
> Yeah. Not a big deal. I guess you meant to set the flag without posting to the
> message queue and just calling the JNI with mAwSettingsLock? The real thing is
I
> want to post this method to the UI thread, so blocking is inevitable.

Not exactly what I meant.

This is post + blocking. In theory, this could post but not block, similar to
maybePostOnUiThread (but should also not post if not on UI thread). Haven't
thought through all the potential implications of this yet..

But then again, this could be argued for all of these settings, not just this
one. Can ask Mikhail about this later I guess :p. Good enough for now.

https://codereview.chromium.org/944053004/diff/40001/android_webview/native/a...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/944053004/diff/40001/android_webview/native/a...
android_webview/native/aw_settings.cc:217:
contents->SetOffscreenPreRaster(enabled);
This breaks the existing pattern that native reads the state from java (instead
of passing it in the post task). So add a @CalledByNative
getOffscreenPreRasterLocked here and use that here.

Then call this method from UpdateEverythingLocked

(I believe this is all to support using the setting object without binding to a
thread as the UI thread)

[hush (inactive), 2015-02-21 00:27:03]

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/944053004/diff/20001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:329: offscreen_pre_raster_ =
enable;
On 2015/02/20 23:54:46, hush wrote:
> On 2015/02/20 23:47:21, hush wrote:
> > On 2015/02/20 23:16:07, boliu wrote:
> > > leave a TODO here, we need to think about if toggling this by itself
should
> do
> > > something
> > 
> > Done.
> 
> Right now, we only expect the java side to call this function. If the native
> side toggles it, Java side (AwSettings) will go out of sync.

The out of sync thing is not something to worry about at the moment. I will just
leave it as is.

https://codereview.chromium.org/944053004/diff/40001/android_webview/native/a...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/944053004/diff/40001/android_webview/native/a...
android_webview/native/aw_settings.cc:217:
contents->SetOffscreenPreRaster(enabled);
On 2015/02/21 00:03:00, boliu wrote:
> This breaks the existing pattern that native reads the state from java
(instead
> of passing it in the post task). So add a @CalledByNative
> getOffscreenPreRasterLocked here and use that here.
> 
> Then call this method from UpdateEverythingLocked
> 
> (I believe this is all to support using the setting object without binding to
a
> thread as the UI thread)

Done.

[boliu, 2015-02-21 00:30:27]

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

[boliu, 2015-02-21 00:30:27]

+mnaganov for maybe glacing over the AwSettings changes later

> You need to turn this setting on in order to use VisibleState API

We don't want to do that anymore (although the doc hasn't been updated yet)

Change the description to summarize what this API does instead.

https://codereview.chromium.org/944053004/diff/60001/android_webview/native/a...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/944053004/diff/60001/android_webview/native/a...
android_webview/native/aw_settings.cc:133: UpdateRendererPreferencesLocked(env,
obj);
UpdateOffscreenPreRasterLocked

[hush (inactive), 2015-02-21 00:45:48]

https://codereview.chromium.org/944053004/diff/60001/android_webview/native/a...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/944053004/diff/60001/android_webview/native/a...
android_webview/native/aw_settings.cc:133: UpdateRendererPreferencesLocked(env,
obj);
On 2015/02/21 00:30:27, boliu wrote:
> UpdateOffscreenPreRasterLocked

Done. Sorry about that./

[boliu, 2015-02-21 00:48:42]

lgtm

Avoid using "you" in the description. Be a bit more formal :p

[hush (inactive), 2015-02-21 01:00:45]

On 2015/02/21 00:48:42, boliu wrote:
> lgtm
> 
> Avoid using "you" in the description. Be a bit more formal :p

Just updated description.
I will add an AwSettingsTest while waiting for mnaganov's opinions.

[boliu, 2015-02-21 01:02:39]

On 2015/02/21 01:00:45, hush wrote:
> On 2015/02/21 00:48:42, boliu wrote:
> > lgtm
> > 
> > Avoid using "you" in the description. Be a bit more formal :p
> 
> Just updated description.
> I will add an AwSettingsTest while waiting for mnaganov's opinions.

I think I got everything, and only added Mikhail more as a tbr :p

But you are welcome to wait.

[mnaganov (inactive), 2015-02-23 11:01:41]

lgtm

[hush (inactive), 2015-02-24 01:19:42]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2015-02-24 01:20:12]

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

[hush (inactive), 2015-02-24 01:20:49]

okay.. landing the CL, without any tests, because we don't know how to write one
to test the tiles of an offscreen webview are actually rastered.

[commit-bot: I haz the power, 2015-02-24 03:57:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-24 03:57:11]

Try jobs failed on following builders:
  android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)

[hush (inactive), 2015-02-24 06:26:31]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-02-24 06:26:43]

The CQ bit was unchecked by hush@chromium.org

[hush (inactive), 2015-02-24 19:11:45]

On 2015/02/24 03:57:11, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)

looking...

[hush (inactive), 2015-02-24 21:27:07]

Because I removed line 181-189 from browser_view_renderer.cc, BVR will call
DemandDrawHw with an empty global visible rect, which is what happens in the
test.

The dummy compositor in test returns null as the CompositorFrame which is
considered as an unsuccessful draw. So the RT DrawGL never happened.

To fix the test, I made the dummy compositor return an empty compositorFrame
instead of null.

One problem with that is the empty CompositorFrame is not a fully functional
dummy frame. It still needs the delegatedFrameData, render passes etc. to be set
for HardwareRenderer::DrawGL to work properly.

So to counter that problem without having to create a full-fledge
compositorFrame, I reset the compositorFrame in sharedRendererState to be NULL
before the DrawGL actually happens. (in WillDrawOnRT)

[hush (inactive), 2015-02-24 21:32:24]

Bo: PTAL PS6. Thank you!

[boliu, 2015-02-24 21:47:31]

On 2015/02/24 21:27:07, hush wrote:
> Because I removed line 181-189 from browser_view_renderer.cc, BVR will call
> DemandDrawHw with an empty global visible rect, which is what happens in the
> test.

Hmm, latent bug. That test should not have relied on global_visible_rect being
empty. FakeWindow::OnDrawHardware should call PrepareToDraw and set the right
visible rect.

> 
> The dummy compositor in test returns null as the CompositorFrame which is
> considered as an unsuccessful draw. So the RT DrawGL never happened.
> 
> To fix the test, I made the dummy compositor return an empty compositorFrame
> instead of null.
> 
> One problem with that is the empty CompositorFrame is not a fully functional
> dummy frame. It still needs the delegatedFrameData, render passes etc. to be
set
> for HardwareRenderer::DrawGL to work properly.

Is it super hard to put fake data the frame to get past this?

See
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/delegate...

> 
> So to counter that problem without having to create a full-fledge
> compositorFrame, I reset the compositorFrame in sharedRendererState to be NULL
> before the DrawGL actually happens. (in WillDrawOnRT)

[hush (inactive), 2015-02-24 22:02:54]

PS7:
Fixed FakeWindow::onDrawHardware so that it calls prepareToDraw with top left
quarter of the surface as the visible rect.

Thanks for the pointers to creating a dummy frame! It works.

[boliu, 2015-02-24 22:06:22]

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

https://codereview.chromium.org/944053004/diff/140001/android_webview/browser...
android_webview/browser/test/fake_window.cc:123:
view_->PrepareToDraw(gfx::Vector2d(), gfx::Rect(0, 0, 50, 50));
Use |location_|

https://codereview.chromium.org/944053004/diff/140001/content/public/test/tes...
File content/public/test/test_synchronous_compositor_android.cc (right):

https://codereview.chromium.org/944053004/diff/140001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:51:
root_pass->SetNew(cc::RenderPassId(1, 1), gfx::Rect(0, 0, 50, 50),
Use |viewport|

[boliu, 2015-02-24 22:06:36]

On 2015/02/24 22:06:22, boliu wrote:
>
https://codereview.chromium.org/944053004/diff/140001/android_webview/browser...
> File android_webview/browser/test/fake_window.cc (right):
> 
>
https://codereview.chromium.org/944053004/diff/140001/android_webview/browser...
> android_webview/browser/test/fake_window.cc:123:
> view_->PrepareToDraw(gfx::Vector2d(), gfx::Rect(0, 0, 50, 50));
> Use |location_|
> 
>
https://codereview.chromium.org/944053004/diff/140001/content/public/test/tes...
> File content/public/test/test_synchronous_compositor_android.cc (right):
> 
>
https://codereview.chromium.org/944053004/diff/140001/content/public/test/tes...
> content/public/test/test_synchronous_compositor_android.cc:51:
> root_pass->SetNew(cc::RenderPassId(1, 1), gfx::Rect(0, 0, 50, 50),
> Use |viewport|

lgtm after that

[hush (inactive), 2015-02-24 22:19:03]

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

https://codereview.chromium.org/944053004/diff/140001/android_webview/browser...
android_webview/browser/test/fake_window.cc:123:
view_->PrepareToDraw(gfx::Vector2d(), gfx::Rect(0, 0, 50, 50));
On 2015/02/24 22:06:22, boliu wrote:
> Use |location_|

Done.

https://codereview.chromium.org/944053004/diff/140001/content/public/test/tes...
File content/public/test/test_synchronous_compositor_android.cc (right):

https://codereview.chromium.org/944053004/diff/140001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:51:
root_pass->SetNew(cc::RenderPassId(1, 1), gfx::Rect(0, 0, 50, 50),
On 2015/02/24 22:06:22, boliu wrote:
> Use |viewport|

Done.

[hush (inactive), 2015-02-24 22:20:52]

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

[hush (inactive), 2015-02-24 22:20:53]

+sievers for content/

[no sievers, 2015-02-25 20:54:36]

content lgtm

https://codereview.chromium.org/944053004/diff/160001/content/public/test/tes...
File content/public/test/test_synchronous_compositor_android.cc (right):

https://codereview.chromium.org/944053004/diff/160001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:47:
scoped_ptr<cc::CompositorFrame> compositor_frame =
nit: scoped_ptr<cc::CompositorFrame> compositor_frame(new
cc::CompositorFrame()); like below

[hush (inactive), 2015-02-25 21:12:42]

https://codereview.chromium.org/944053004/diff/160001/content/public/test/tes...
File content/public/test/test_synchronous_compositor_android.cc (right):

https://codereview.chromium.org/944053004/diff/160001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:47:
scoped_ptr<cc::CompositorFrame> compositor_frame =
On 2015/02/25 20:54:36, sievers wrote:
> nit: scoped_ptr<cc::CompositorFrame> compositor_frame(new
> cc::CompositorFrame()); like below

Done.

[hush (inactive), 2015-02-25 21:12:46]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-02-25 21:12:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org, boliu@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/944053004/#ps180001
(title: " ")

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

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

[commit-bot: I haz the power, 2015-02-25 22:11:28]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-25 22:44:57]

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