Making it easier to hook a ContentViewCore to the SurfaceView.

Making it easier to hook a ContentViewCore to the SurfaceView.

This CL changes the way we deal with WebLayers, so that a ContentViewCore does
not need native code anymore to hook with the surface view.
The ContentViewCore now keeps track of the WebLayers as they are
attached/detached. The ContentViewRenderView is used to render a specific
ContentView by retrieving the ContentViewCore WebLlayer and adding it to the
compositor.

BUG=
TEST=ContentShell and ChromiumTestShell should still work.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168166

[David Trainor- moved to gerrit, 2012-11-14 23:10:40]

lgtm

http://codereview.chromium.org/11293264/diff/4018/content/browser/android/con...
File content/browser/android/content_view_render_view.cc (right):

http://codereview.chromium.org/11293264/diff/4018/content/browser/android/con...
content/browser/android/content_view_render_view.cc:34:
ContentViewRenderView::ContentViewRenderView() : scheduled_composite_(false) {
Shouldn't this be on the next line for formatting?  Not sure.

http://codereview.chromium.org/11293264/diff/4018/content/browser/android/con...
content/browser/android/content_view_render_view.cc:79:
DrawDelegate::GetInstance()->SetBounds(size);
DrawDelegate dies once my CL lands.

[Jay Civelli, 2012-11-14 23:54:41]

Ted,
Could you review files under:
chrome/android
content/browser/android
content/public/android

Scott,
Could you review:
content/browser/

Thanks!

https://codereview.chromium.org/11293264/diff/4018/content/browser/android/co...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/11293264/diff/4018/content/browser/android/co...
content/browser/android/content_view_render_view.cc:34:
ContentViewRenderView::ContentViewRenderView() : scheduled_composite_(false) {
On 2012/11/14 23:10:41, dtrainor wrote:
> Shouldn't this be on the next line for formatting?  Not sure.
It's OK per style-guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

https://codereview.chromium.org/11293264/diff/4018/content/browser/android/co...
content/browser/android/content_view_render_view.cc:79:
DrawDelegate::GetInstance()->SetBounds(size);
On 2012/11/14 23:10:41, dtrainor wrote:
> DrawDelegate dies once my CL lands.
I removed the call.

[Ted C, 2012-11-15 00:27:02]

lgtm

huzzah for deleting duplicate code

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:523:
int getNative() {
This already exists: getNativeContentViewCore

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:17:
* Call setCurrentContentView() with the contentView that should be displayed.
Do {@link #setCurrentContentView(ContentView)} so it will refactor more easily.

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:68:
public void setCurrentContentView(ContentView contentView) {
javadocs these public methods

https://codereview.chromium.org/11293264/diff/8005/content/shell/android/java...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/11293264/diff/8005/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:94:
contentView.onHide();
make this a one liner

[Jay Civelli, 2012-11-15 00:51:34]

Joi,
Could you review content/public files?

Pavel,
Could you review content/shell (the non-android part)?

Thanks!

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:523:
int getNative() {
On 2012/11/15 00:27:03, Ted C wrote:
> This already exists: getNativeContentViewCore

Ah! Awesome.

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:17:
* Call setCurrentContentView() with the contentView that should be displayed.
On 2012/11/15 00:27:03, Ted C wrote:
> Do {@link #setCurrentContentView(ContentView)} so it will refactor more
easily.

Done.

https://codereview.chromium.org/11293264/diff/8005/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:68:
public void setCurrentContentView(ContentView contentView) {
On 2012/11/15 00:27:03, Ted C wrote:
> javadocs these public methods

Done.

https://codereview.chromium.org/11293264/diff/8005/content/shell/android/java...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/11293264/diff/8005/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:94:
contentView.onHide();
On 2012/11/15 00:27:03, Ted C wrote:
> make this a one liner

Done.

[sky, 2012-11-15 01:49:38]

LGTM

https://codereview.chromium.org/11293264/diff/7009/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/11293264/diff/7009/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:55:
content_view_core_(content_view_core),
Seems less error prone to initialize content_view_core_ to NULL.

[pfeldman, 2012-11-15 05:24:50]

shell rubber stamp lgtm

https://codereview.chromium.org/11293264/diff/7009/content/shell/android/java...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/11293264/diff/7009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:93:
if (contentView != null) contentView.onHide();
I haven't seen Java code style for ages, but I thought we needed a line break
here.

[Jói, 2012-11-15 09:29:47]

content/public LGTM.

[commit-bot: I haz the power, 2012-11-15 17:23:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11293264/17001

[Ted C, 2012-11-15 18:03:38]

https://codereview.chromium.org/11293264/diff/7009/content/shell/android/java...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/11293264/diff/7009/content/shell/android/java...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:93:
if (contentView != null) contentView.onHide();
On 2012/11/15 05:24:50, pfeldman wrote:
> I haven't seen Java code style for ages, but I thought we needed a line break
> here.

FYI in Android land, this is allowed:
http://source.android.com/source/code-style.html#use-standard-brace-style

[commit-bot: I haz the power, 2012-11-15 19:39:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11293264/26001

[commit-bot: I haz the power, 2012-11-15 20:36:07]

Retried try job too often for step(s) content_unittests

[commit-bot: I haz the power, 2012-11-15 20:58:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11293264/26001

[commit-bot: I haz the power, 2012-11-15 20:58:27]

Failed to apply patch for content/browser/android/content_view_core_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/android/content_view_core_impl.cc
  Hunk #2 succeeded at 146 (offset 11 lines).
  Hunk #3 FAILED at 200.
  Hunk #4 succeeded at 550 (offset 53 lines).
  Hunk #5 succeeded at 599 (offset 53 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
content/browser/android/content_view_core_impl.cc.rej

Patch:       content/browser/android/content_view_core_impl.cc
Index: content/browser/android/content_view_core_impl.cc
diff --git a/content/browser/android/content_view_core_impl.cc
b/content/browser/android/content_view_core_impl.cc
index
9151608f7a8dc8f451036d9e6dfe2c36a7167ca3..de2c4a81e92800b21b3f2a5e91e4e80d0d4b030a
100644
--- a/content/browser/android/content_view_core_impl.cc
+++ b/content/browser/android/content_view_core_impl.cc
@@ -59,6 +59,7 @@ using base::android::ScopedJavaLocalRef;
 using WebKit::WebGestureEvent;
 using WebKit::WebInputEvent;
 using WebKit::WebInputEventFactory;
+using WebKit::WebLayer;
 
 // Describes the type and enabled state of a select popup item.
 // Keep in sync with the value defined in SelectPopupDialog.java
@@ -134,6 +135,7 @@ ContentViewCoreImpl::ContentViewCoreImpl(JNIEnv* env,
jobject obj,
                                          ui::WindowAndroid* window_android)
     : java_ref_(env, obj),
       web_contents_(static_cast<WebContentsImpl*>(web_contents)),
+      root_layer_(WebLayer::create()),
       tab_crashed_(false),
       window_android_(window_android) {
   DCHECK(web_contents) <<
@@ -198,6 +200,7 @@ void
ContentViewCoreImpl::OnJavaContentViewCoreDestroyed(JNIEnv* env,
 
 void ContentViewCoreImpl::InitWebContents() {
   DCHECK(web_contents_);
+
   notification_registrar_.Add(this,
       NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
       Source<NavigationController>(&web_contents_->GetController()));
@@ -495,11 +498,6 @@ unsigned int ContentViewCoreImpl::GetScaledContentTexture(
   return view->GetScaledContentTexture(size);
 }
 
-WebKit::WebLayer* ContentViewCoreImpl::GetWebLayer() {
-  RenderWidgetHostViewAndroid* view = GetRenderWidgetHostViewAndroid();
-  return view ? view->GetWebLayer() : NULL;
-}
-
 void ContentViewCoreImpl::StartContentIntent(const GURL& content_url) {
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobject> j_obj = java_ref_.get(env);
@@ -549,16 +547,28 @@ gfx::Rect ContentViewCoreImpl::GetBounds() const {
                    Java_ContentViewCore_getHeight(env, j_obj.obj()));
 }
 
+void ContentViewCoreImpl::AttachWebLayer(WebLayer* layer) {
+  root_layer_->addChild(layer);
+}
+
+void ContentViewCoreImpl::RemoveWebLayer(WebLayer* layer) {
+  layer->removeFromParent();
+}
+
 void ContentViewCoreImpl::LoadUrl(
     NavigationController::LoadURLParams& params) {
   GetWebContents()->GetController().LoadURLWithParams(params);
   tab_crashed_ = false;
 }
 
-ui::WindowAndroid* ContentViewCoreImpl::GetWindowAndroid() {
+ui::WindowAndroid* ContentViewCoreImpl::GetWindowAndroid() const {
   return window_android_;
 }
 
+WebLayer* ContentViewCoreImpl::GetWebLayer() const {
+  return root_layer_.get();
+}
+
 // ----------------------------------------------------------------------------
 // Methods called from Java via JNI
 // ----------------------------------------------------------------------------

[no sievers, 2012-11-15 21:02:21]

https://chromiumcodereview.appspot.com/11293264/diff/26001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/11293264/diff/26001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:294: void
RenderWidgetHostViewAndroid::Destroy() {
Is it possible that somebody calls SetContentViewCore(NULL)?
Should that maybe also remove the layer from the current one.
And then add a NULL check here?

[Jay Civelli, 2012-11-15 21:16:48]

https://codereview.chromium.org/11293264/diff/7009/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/11293264/diff/7009/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:55:
content_view_core_(content_view_core),
On 2012/11/15 01:49:39, sky wrote:
> Seems less error prone to initialize content_view_core_ to NULL.

Done.

https://codereview.chromium.org/11293264/diff/26001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/11293264/diff/26001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:294: void
RenderWidgetHostViewAndroid::Destroy() {
On 2012/11/15 21:02:21, Daniel Sievers wrote:
> Is it possible that somebody calls SetContentViewCore(NULL)?
> Should that maybe also remove the layer from the current one.
> And then add a NULL check here?

Done.

[no sievers, 2012-11-15 21:19:02]

I think you still need to remove Attach/RemoveLayer in
render_view_host_delegate.h

[no sievers, 2012-11-15 21:22:20]

Also interstitial_page_impl.cc/.h

[Jay Civelli, 2012-11-15 21:27:25]

On 2012/11/15 21:22:20, Daniel Sievers wrote:
> Also interstitial_page_impl.cc/.h

Good catch!
Removed from render_view_host_delegate.h and interstitial_page_impl.

[no sievers, 2012-11-15 21:27:27]

https://chromiumcodereview.appspot.com/11293264/diff/23004/content/browser/an...
File content/browser/android/content_view_render_view.cc (right):

https://chromiumcodereview.appspot.com/11293264/diff/23004/content/browser/an...
content/browser/android/content_view_render_view.cc:85: FROM_HERE,
base::Bind(&ContentViewRenderView::Composite, this));
Is this the only reason this class is refcounted?
Maybe use a weakptr factory member variable instead then.

[Jay Civelli, 2012-11-15 21:52:45]

https://codereview.chromium.org/11293264/diff/23004/content/browser/android/c...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/11293264/diff/23004/content/browser/android/c...
content/browser/android/content_view_render_view.cc:85: FROM_HERE,
base::Bind(&ContentViewRenderView::Composite, this));
On 2012/11/15 21:27:28, Daniel Sievers wrote:
> Is this the only reason this class is refcounted?
> Maybe use a weakptr factory member variable instead then.

Done.

[no sievers, 2012-11-15 23:19:54]

Cool thanks! This is much nicer with going through ContentViewCore for the layer
and unifying the code.

On 2012/11/15 21:52:45, Jay Civelli wrote:
>
https://codereview.chromium.org/11293264/diff/23004/content/browser/android/c...
> File content/browser/android/content_view_render_view.cc (right):
> 
>
https://codereview.chromium.org/11293264/diff/23004/content/browser/android/c...
> content/browser/android/content_view_render_view.cc:85: FROM_HERE,
> base::Bind(&ContentViewRenderView::Composite, this));
> On 2012/11/15 21:27:28, Daniel Sievers wrote:
> > Is this the only reason this class is refcounted?
> > Maybe use a weakptr factory member variable instead then.
> 
> Done.

[commit-bot: I haz the power, 2012-11-15 23:31:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11293264/24018

[commit-bot: I haz the power, 2012-11-16 06:06:48]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-16 06:19:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11293264/24018

[commit-bot: I haz the power, 2012-11-16 08:09:38]

Change committed as 168166

[boliu, 2012-11-16 16:37:07]

This broken android webview instrumentation tests. I *think* because this new
code doesn't work in single process mode, but I can't be sure. Here is the crash
stack. Anyone have a suggested fix (or revert)?

signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0000001c
  RELADDR   FUNCTION                                                       
FILE:LINE
  v------>  base::Thread::message_loop_proxy() const                       
/ssd_code/chromium/src/out/Debug/../../base/threading/thread.h:130
  0033600c  content::ChildProcess::io_message_loop_proxy()+2               
/ssd_code/chromium/src/out/Debug/../../content/common/child_process.h:35
  00bb549b  content::ChildThread::Init()+62                                
/ssd_code/chromium/src/out/Debug/../../content/common/child_thread.cc:102
  00bb572f  content::ChildThread::ChildThread(std::string const&)+74       
/ssd_code/chromium/src/out/Debug/../../content/common/child_thread.cc:93
  0033674f  content::GpuChildThread::GpuChildThread(std::string const&)+18 
/ssd_code/chromium/src/out/Debug/../../content/gpu/gpu_child_thread.cc:64
  00968dbf  content::GpuMainThread::Init()+70                              
/ssd_code/chromium/src/out/Debug/../../content/browser/gpu/gpu_process_host.cc:219
  0047c78f  base::Thread::ThreadMain()+74                                  
/ssd_code/chromium/src/out/Debug/../../base/threading/thread.cc:188
  00479faf  base::(anonymous namespace)::ThreadFunc(void*)+26              
/ssd_code/chromium/src/out/Debug/../../base/threading/platform_thread_posix.cc:65
  v------>  _pthread_internal_remove_locked                                
/usr/local/google/code_hdd/clank/bionic/libc/bionic/pthread.c:109
  v------>  _pthread_internal_remove                                       
/usr/local/google/code_hdd/clank/bionic/libc/bionic/pthread.c:127
  0000e3d8  pthread_exit+464                                               
/usr/local/google/code_hdd/clank/bionic/libc/bionic/pthread.c:585
  0000dac4  _init_thread+60                                                
/usr/local/google/code_hdd/clank/bionic/libc/bionic/pthread.c:226

[no sievers, 2012-11-16 17:07:38]

https://codereview.chromium.org/11416038