Add a BrowserViewRenderer unittest for compositor switch logic

Add a BrowserViewRenderer unittest for compositor switch logic

BUG=616955
Committed: https://crrev.com/48891cc12ea4cdeb20c1ce8e8555e1ff6267007b
Cr-Commit-Position: refs/heads/master@{#401154}

[hush (inactive), 2016-06-21 01:20:07]

Patchset #1 (id:1) has been deleted

[hush (inactive), 2016-06-21 01:20:26]

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

[hush (inactive), 2016-06-21 01:20:27]

PTAL

[hush (inactive), 2016-06-21 01:31:05]

The CQ bit was checked by hush@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 01:31:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078383004/20001

[commit-bot: I haz the power, 2016-06-21 02:12:05]

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

[commit-bot: I haz the power, 2016-06-21 02:12:05]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-06-21 17:29:47]

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

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:142:
content::SynchronousCompositor* compositor() const { return compositor_; }
GetCurrent/ActiveCompositorForTesting

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

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer_unittest.cc:55:
compositor_.reset(new content::TestSynchronousCompositor(0, 1));
this also destroyes the old one, can we have exercise having both at the same
time, for a short amount of time at least

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer_unittest.cc:90:
compositor_.reset(new content::TestSynchronousCompositor(0, 1));
ditto

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

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/test/rendering_test.h:84:
content::SynchronousCompositor* compositor() const;
add "current" or "active" to the method name

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

https://codereview.chromium.org/2078383004/diff/20001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:25:
client_->DidInitializeCompositor(this, process_id_, routing_id_);
did you have to rearrange this logic?

[hush (inactive), 2016-06-21 21:27:26]

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

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:142:
content::SynchronousCompositor* compositor() const { return compositor_; }
On 2016/06/21 17:29:47, boliu wrote:
> GetCurrent/ActiveCompositorForTesting

Done.

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

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer_unittest.cc:55:
compositor_.reset(new content::TestSynchronousCompositor(0, 1));
On 2016/06/21 17:29:47, boliu wrote:
> this also destroyes the old one, can we have exercise having both at the same
> time, for a short amount of time at least

Done.

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer_unittest.cc:90:
compositor_.reset(new content::TestSynchronousCompositor(0, 1));
On 2016/06/21 17:29:47, boliu wrote:
> ditto

Done.

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

https://codereview.chromium.org/2078383004/diff/20001/android_webview/browser...
android_webview/browser/test/rendering_test.h:84:
content::SynchronousCompositor* compositor() const;
On 2016/06/21 17:29:47, boliu wrote:
> add "current" or "active" to the method name

Done.

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

https://codereview.chromium.org/2078383004/diff/20001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:25:
client_->DidInitializeCompositor(this, process_id_, routing_id_);
On 2016/06/21 17:29:47, boliu wrote:
> did you have to rearrange this logic?

well it's saner. And... it is better to separate them so that we can have a
state where there are 2 compositors alive, but one "active"
And you do need a place to notify the destruction of the current compositor,
otherwise the compositor_map_.empty() dcheck in ~BrowserViewRenderer won't pass.

[boliu, 2016-06-21 21:31:59]

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

https://codereview.chromium.org/2078383004/diff/20001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:25:
client_->DidInitializeCompositor(this, process_id_, routing_id_);
On 2016/06/21 21:27:26, hush wrote:
> On 2016/06/21 17:29:47, boliu wrote:
> > did you have to rearrange this logic?
> 
> well it's saner. And... it is better to separate them so that we can have a
> state where there are 2 compositors alive, but one "active"
> And you do need a place to notify the destruction of the current compositor,
> otherwise the compositor_map_.empty() dcheck in ~BrowserViewRenderer won't
pass.

Didn't SetClient(NULL) used to call DidDestroyCompositor though?

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

https://codereview.chromium.org/2078383004/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer_unittest.cc:76:
content::TestSynchronousCompositor* new_compositor_;
unique_ptr

https://codereview.chromium.org/2078383004/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer_unittest.cc:120:
content::TestSynchronousCompositor* new_compositor_;
ditto

[hush (inactive), 2016-06-21 22:02:27]

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

https://codereview.chromium.org/2078383004/diff/20001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:25:
client_->DidInitializeCompositor(this, process_id_, routing_id_);
On 2016/06/21 21:31:59, boliu wrote:
> On 2016/06/21 21:27:26, hush wrote:
> > On 2016/06/21 17:29:47, boliu wrote:
> > > did you have to rearrange this logic?
> > 
> > well it's saner. And... it is better to separate them so that we can have a
> > state where there are 2 compositors alive, but one "active"
> > And you do need a place to notify the destruction of the current compositor,
> > otherwise the compositor_map_.empty() dcheck in ~BrowserViewRenderer won't
> pass.
> 
> Didn't SetClient(NULL) used to call DidDestroyCompositor though?

ok. I reverted this bit.

[boliu, 2016-06-21 22:06:24]

lgtm

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

https://codereview.chromium.org/2078383004/diff/60001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:22: if (client_) {
nit: no need for braces

[hush (inactive), 2016-06-21 22:11:32]

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

[hush (inactive), 2016-06-21 22:11:33]

Hello Daniel,
PTAL content/

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

https://codereview.chromium.org/2078383004/diff/60001/content/public/test/tes...
content/public/test/test_synchronous_compositor_android.cc:22: if (client_) {
On 2016/06/21 22:06:24, boliu wrote:
> nit: no need for braces

Done.

[no sievers, 2016-06-21 23:20:28]

On 2016/06/21 22:11:33, hush wrote:
> Hello Daniel,
> PTAL content/
> 
>
https://codereview.chromium.org/2078383004/diff/60001/content/public/test/tes...
> File content/public/test/test_synchronous_compositor_android.cc (right):
> 
>
https://codereview.chromium.org/2078383004/diff/60001/content/public/test/tes...
> content/public/test/test_synchronous_compositor_android.cc:22: if (client_) {
> On 2016/06/21 22:06:24, boliu wrote:
> > nit: no need for braces
> 
> Done.

lgtm

[hush (inactive), 2016-06-21 23:49:01]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2016-06-21 23:49:01]

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/2078383004/#ps80001
(title: "comment")

[commit-bot: I haz the power, 2016-06-21 23:50:05]

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

[commit-bot: I haz the power, 2016-06-22 01:16:42]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-06-22 01:20:57]

Description was changed from

==========
Add a BrowserViewRenderer unittest for compositor switch logic

BUG=616955
==========

to

==========
Add a BrowserViewRenderer unittest for compositor switch logic

BUG=616955

Committed: https://crrev.com/48891cc12ea4cdeb20c1ce8e8555e1ff6267007b
Cr-Commit-Position: refs/heads/master@{#401154}
==========

[commit-bot: I haz the power, 2016-06-22 01:20:58]

Patchset 4 (id:??) landed as
https://crrev.com/48891cc12ea4cdeb20c1ce8e8555e1ff6267007b
Cr-Commit-Position: refs/heads/master@{#401154}