Move SharedRendererState ownership to AwContents

Move SharedRendererState ownership to AwContents

In order to support asynchronous functor destruction, we need
to decouple the lifetimes of SharedRendererState and
BrowserViewRenderer. AwContents becomes responsible for
informing BVR of the SharedRendererState instance to use,
and SRS talks to AwC instead of BVR.

BUG=597167
Committed: https://crrev.com/af7b098d0d2cd8b567d19fbf8a233bb00532b0ae
Cr-Commit-Position: refs/heads/master@{#384031}

[boliu, 2016-03-23 15:47:19]

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

[boliu, 2016-03-23 15:47:20]

Let's go a step further: SRS and BVR no longer includes each other.

For AwContents->BVR->SRS calls, make them just AwContents->SRS calls. If BVR
needs a piece of info from SRS, then just have AwContents get it from SRS first,
and pass it to BVR.

SRS instead of talking to BVR, can define a new client interface that AwContents
implements. Then AwContents can ask BVR.

Makes sense?

https://codereview.chromium.org/1816283005/diff/1/android_webview/browser/bro...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/1816283005/diff/1/android_webview/browser/bro...
android_webview/browser/browser_view_renderer.cc:169: if (level >=
TRIM_MEMORY_MODERATE && shared_renderer_state_) {
this should just be aw_contents -> SRS call, no need to go through BVR

[boliu, 2016-03-23 20:42:33]

Very close to BVR not including SRS, so let's just remove the last use case.

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

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:356: offscreen_pre_raster_ =
enable;
It's ok to use the constraints from last draw here, instead of doing the whole
dance with AwContents.

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

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:130: gfx::Vector2d
last_on_draw_scroll_offset() {
I was wrong. last_on_draw_scroll_offset_ can be removed from BVR altogether, and
just stay in AwC.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:135: void
UpdateMemoryPolicy(gfx::Rect interest_rect);
GetInterestRect and UpdateMemoryPolicy can be merged into a single

void SetParentDrawConstraints(const ParentCompositorDrawConstraints&);

UpdateMemoryPolicy don't need to be exposed to AwC I think

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:159: SharedRendererState*
shared_renderer_state_;
should remove this from this patch, only one usage left.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:201: // same
undo this change

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

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:323: void
SharedRendererState::ReleaseCompositorResourcesIfNeededOnUI() {
DeleteHardwareRendererOnUIThread actually sounds better

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:364: void
SharedRendererState::TrimMemoryOnUI(int level) {
The level check got dropped?

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:975: CleanUpReturnedResources();
return resources before UpdateMemoryPolicy.

Actually you can just put this near the beginning of OnDraw. Doesn't hurt to
call it more often than needed.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:977: scoped_ptr<ChildFrame> child_frame =
browser_view_renderer_.OnDrawHardware(
indent

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1261:
shared_renderer_state_.TrimMemoryOnUI(level);
trim SRS first (to match old code)

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.h:168: void CleanUpUncommittedFrame();
new methods should be private

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.h:349: SharedRendererState
shared_renderer_state_;
Destruction is in reverse declare order, so as currently implemented, SRS needs
to be above BVR.

Might change after you fix some of these comments though, so be careful here.

[boliu, 2016-03-23 20:46:11]

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

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:135: void
UpdateMemoryPolicy(gfx::Rect interest_rect);
On 2016/03/23 20:42:32, boliu wrote:
> GetInterestRect and UpdateMemoryPolicy can be merged into a single
> 
> void SetParentDrawConstraints(const ParentCompositorDrawConstraints&);
> 
> UpdateMemoryPolicy don't need to be exposed to AwC I think

As in SetParentDrawConstraints will also do the UpdateMemoryPolicy work. Just
that the name doesn't need to be exposed to AwC.

Actually, all this should just be done as part of OnDrawHardware.

[Tobias Sargeant, 2016-03-24 16:53:05]

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

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:130: gfx::Vector2d
last_on_draw_scroll_offset() {
On 2016/03/23 20:42:32, boliu wrote:
> I was wrong. last_on_draw_scroll_offset_ can be removed from BVR altogether,
and
> just stay in AwC.

Done.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:201: // same
On 2016/03/23 20:42:32, boliu wrote:
> undo this change

Done.

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

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:323: void
SharedRendererState::ReleaseCompositorResourcesIfNeededOnUI() {
On 2016/03/23 20:42:32, boliu wrote:
> DeleteHardwareRendererOnUIThread actually sounds better

Done.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:364: void
SharedRendererState::TrimMemoryOnUI(int level) {
On 2016/03/23 20:42:32, boliu wrote:
> The level check got dropped?

Done. Deciding what to trim and when moved to AwContents.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:975: CleanUpReturnedResources();
On 2016/03/23 20:42:32, boliu wrote:
> return resources before UpdateMemoryPolicy.
> 
> Actually you can just put this near the beginning of OnDraw. Doesn't hurt to
> call it more often than needed.

Done.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:977: scoped_ptr<ChildFrame> child_frame =
browser_view_renderer_.OnDrawHardware(
On 2016/03/23 20:42:32, boliu wrote:
> indent

Done.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:1261:
shared_renderer_state_.TrimMemoryOnUI(level);
On 2016/03/23 20:42:32, boliu wrote:
> trim SRS first (to match old code)

Done.

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/1816283005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.h:168: void CleanUpUncommittedFrame();
On 2016/03/23 20:42:33, boliu wrote:
> new methods should be private

Done.

[boliu, 2016-03-24 17:34:45]

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

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:119: void
BrowserViewRenderer::TrimMemory(const int level) {
don't need to take level at all now

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:373: hardware_enabled_ = false;
Feels like this should be

hardware_enabled_ = false;
UpdateMemoryPolicy();

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:74: bool offscreen_pre_raster()
{ return offscreen_pre_raster_; }
remove

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:76: void
SetExternalDrawConstraints(const ParentCompositorDrawConstraints&);
remove

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:123: void
UpdateParentDrawConstraints(const ParentCompositorDrawConstraints&);
style: param needs a name

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:323: bool hardware_initialized
= client_->IsHardwareEnabled();
This used to call BVR clean up, so was checking with BVR. Now feels like this
should just be an SRS internal bool state, of whether there is anything that
needs to be cleaned up on RT, ie if HR has been created.

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:358: void
SharedRendererState::TrimMemoryOnUI(int level) {
don't need level arg

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/1816283005/diff/40001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:99: SharedRendererStateClient*
client_;
SRSC* const client_

[boliu, 2016-03-30 16:12:13]

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (left):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:370: if (offscreen_pre_raster_
!= enable && compositor_)
You can leave the first half of this check

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

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:193:
DCHECK(shared_renderer_state_);
Maybe leave a comment that client is responsible for ensuring this.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:255: const
ParentCompositorDrawConstraints& external_draw_constraints) {
Maybe this should not take any args, and should have a
DCHECK(shared_renderer_state_), to match OnDrawHardware

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:18: #include
"cc/resources/returned_resource.h"
header not needed by BVR. Is it used by aw_contents?

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:98: SharedRendererStateClient*
client_;
const

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/shared_renderer_state_client.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/shared_renderer_state_client.h:11: #include
"ui/gfx/geometry/vector2d_f.h"
none of these includes are needed

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

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/fake_window.cc:68:
functor_->DeleteHardwareRendererOnUI();
this is wrong order compared to aw_contents

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/fake_window.cc:80: functor_ = nullptr;
Can't do this anymore. Window can attach/detach many times.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/fake_window.cc:180: if (render_thread_.get()) {
.get() not needed, can remove them from DCHECKs too

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

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/rendering_test.h:86:
scoped_ptr<SharedRendererState> shared_renderer_state_;
need to be above BVR like in aw_contents

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
android_webview/native/aw_contents.cc:960: 
nit: don't remove empty line

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
android_webview/native/aw_contents.h:344: const gfx::Vector2d& scroll);
these new methods don't exist anymore

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
File android_webview/native/java_browser_view_renderer_helper.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
android_webview/native/java_browser_view_renderer_helper.h:24: const
base::android::JavaRef<jobject>& java_canvas,
unrelated change?

[Tobias Sargeant, 2016-03-30 17:17:17]

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (left):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:370: if (offscreen_pre_raster_
!= enable && compositor_)
On 2016/03/30 16:12:13, boliu wrote:
> You can leave the first half of this check

Wasn't this wrong before? UpdateMemoryPolicy uses the value of
offscreen_pre_raster_, which we're definitely about to change.

Changed my version to avoid doing anything if we set it to the value it already
is.

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

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:193:
DCHECK(shared_renderer_state_);
On 2016/03/30 16:12:13, boliu wrote:
> Maybe leave a comment that client is responsible for ensuring this.

Done.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:255: const
ParentCompositorDrawConstraints& external_draw_constraints) {
On 2016/03/30 16:12:13, boliu wrote:
> Maybe this should not take any args, and should have a
> DCHECK(shared_renderer_state_), to match OnDrawHardware

Done, removed ParentDrawConstraintsUpdated from BVRClient.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:18: #include
"cc/resources/returned_resource.h"
On 2016/03/30 16:12:13, boliu wrote:
> header not needed by BVR. Is it used by aw_contents?

Removed. Was a leftover from when AwContents used BVR methods to return
resources.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:98: SharedRendererStateClient*
client_;
On 2016/03/30 16:12:13, boliu wrote:
> const

Done.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
File android_webview/browser/shared_renderer_state_client.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/shared_renderer_state_client.h:11: #include
"ui/gfx/geometry/vector2d_f.h"
On 2016/03/30 16:12:13, boliu wrote:
> none of these includes are needed

Done.

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

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/fake_window.cc:68:
functor_->DeleteHardwareRendererOnUI();
On 2016/03/30 16:12:13, boliu wrote:
> this is wrong order compared to aw_contents

It isn't possible to match the old AwContents order perfectly, and this matches
the new AwContents order.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/fake_window.cc:80: functor_ = nullptr;
On 2016/03/30 16:12:13, boliu wrote:
> Can't do this anymore. Window can attach/detach many times.

Done.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/fake_window.cc:180: if (render_thread_.get()) {
On 2016/03/30 16:12:13, boliu wrote:
> .get() not needed, can remove them from DCHECKs too

Done.

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

https://codereview.chromium.org/1816283005/diff/80001/android_webview/browser...
android_webview/browser/test/rendering_test.h:86:
scoped_ptr<SharedRendererState> shared_renderer_state_;
On 2016/03/30 16:12:13, boliu wrote:
> need to be above BVR like in aw_contents

Done.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
android_webview/native/aw_contents.cc:960: 
On 2016/03/30 16:12:13, boliu wrote:
> nit: don't remove empty line

Done.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
android_webview/native/aw_contents.h:344: const gfx::Vector2d& scroll);
On 2016/03/30 16:12:13, boliu wrote:
> these new methods don't exist anymore

Done.

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
File android_webview/native/java_browser_view_renderer_helper.h (right):

https://codereview.chromium.org/1816283005/diff/80001/android_webview/native/...
android_webview/native/java_browser_view_renderer_helper.h:24: const
base::android::JavaRef<jobject>& java_canvas,
On 2016/03/30 16:12:13, boliu wrote:
> unrelated change?

Removed. Maybe should add a separate CL to get rid of bare jobject parameters.

[boliu, 2016-03-30 17:21:53]

lgtm % remove stuff from next patch

https://codereview.chromium.org/1816283005/diff/100001/android_webview/native...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/1816283005/diff/100001/android_webview/native...
android_webview/native/aw_contents.h:92: void CreateSharedRendererState();
new stuff from next patch, here and on line 104?

[boliu, 2016-03-30 17:22:51]

oh, write a better description

[Tobias Sargeant, 2016-03-30 17:52:56]

Description was changed from

==========
Move SharedRendererState ownership to AwContents

BUG=597167
==========

to

==========
Move SharedRendererState ownership to AwContents

In order to support asynchronous functor destruction, we need
to decouple the lifetimes of SharedRendererState and
BrowserViewRenderer. AwContents becomes responsible for
informing BVR of the SharedRendererState instance to use,
and SRS talks to AwC instead of BVR.

BUG=597167
==========

[Tobias Sargeant, 2016-03-30 17:54:12]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-03-30 17:54:12]

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

[commit-bot: I haz the power, 2016-03-30 17:54:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1816283005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1816283005/120001

[commit-bot: I haz the power, 2016-03-30 18:33:14]

Description was changed from

==========
Move SharedRendererState ownership to AwContents

In order to support asynchronous functor destruction, we need
to decouple the lifetimes of SharedRendererState and
BrowserViewRenderer. AwContents becomes responsible for
informing BVR of the SharedRendererState instance to use,
and SRS talks to AwC instead of BVR.

BUG=597167
==========

to

==========
Move SharedRendererState ownership to AwContents

In order to support asynchronous functor destruction, we need
to decouple the lifetimes of SharedRendererState and
BrowserViewRenderer. AwContents becomes responsible for
informing BVR of the SharedRendererState instance to use,
and SRS talks to AwC instead of BVR.

BUG=597167
==========

[commit-bot: I haz the power, 2016-03-30 18:33:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-30 18:34:10]

Description was changed from

==========
Move SharedRendererState ownership to AwContents

In order to support asynchronous functor destruction, we need
to decouple the lifetimes of SharedRendererState and
BrowserViewRenderer. AwContents becomes responsible for
informing BVR of the SharedRendererState instance to use,
and SRS talks to AwC instead of BVR.

BUG=597167
==========

to

==========
Move SharedRendererState ownership to AwContents

In order to support asynchronous functor destruction, we need
to decouple the lifetimes of SharedRendererState and
BrowserViewRenderer. AwContents becomes responsible for
informing BVR of the SharedRendererState instance to use,
and SRS talks to AwC instead of BVR.

BUG=597167

Committed: https://crrev.com/af7b098d0d2cd8b567d19fbf8a233bb00532b0ae
Cr-Commit-Position: refs/heads/master@{#384031}
==========

[commit-bot: I haz the power, 2016-03-30 18:34:11]

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