Very basic Android browser-side compositing support.

Very basic Android browser-side compositing support.

This makes the ContentShell render through the compositor
instead of issueing GL calls directly.

BUG=136923
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155061

[no sievers, 2012-08-16 21:55:13]

https://chromiumcodereview.appspot.com/10828356/diff/1/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/ui/gl/gl_surface.cc#ne...
ui/gl/gl_surface.cc:83: //  NOTIMPLEMENTED();
I will fix this issue properly.

[piman, 2012-08-17 00:00:08]

FYI, https://bugs.webkit.org/show_bug.cgi?id=94174 is coming that changes the
ownership patterns for WebLayer and friends - see also
http://codereview.chromium.org/10832355/

[no sievers, 2012-08-17 00:58:59]

Thanks for the heads-up. Yea I think this patch can wait to land after the
webkit change lands and I adjusted it to account for that.

On 2012/08/17 00:00:08, piman wrote:
> FYI, https://bugs.webkit.org/show_bug.cgi?id=94174 is coming that changes the
> ownership patterns for WebLayer and friends - see also
> http://codereview.chromium.org/10832355/

[aelias_OOO_until_Jul13, 2012-08-17 04:35:42]

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:27: return
Singleton<CompositorImpl>::get();
Singletons are usually better avoided.  For example, it will be tricky to pass
in initialization parameters.  Should the content shell own it?  (Future users
like standalone mode and WebView will need to create their own versions, but
that's actually desirable since we want different behavior in each case.)

[no sievers, 2012-08-17 16:23:13]

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:27: return
Singleton<CompositorImpl>::get();
I could also add a static Initialize() function.
For now the singleton was convenient, because RenderWidgetHostView needed to get
to it also, but maybe we can somehow make this all a bit nicer as things come
together.

On 2012/08/17 04:35:42, aelias wrote:
> Singletons are usually better avoided.  For example, it will be tricky to pass
> in initialization parameters.  Should the content shell own it?  (Future users
> like standalone mode and WebView will need to create their own versions, but
> that's actually desirable since we want different behavior in each case.)

[klobag.chromium, 2012-08-22 07:01:18]

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/androi...
File content/browser/android/graphics_context.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/androi...
content/browser/android/graphics_context.cc:52: return
context_->insertSyncPoint();
Is the only reason to have this context_ is to make this call?

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/androi...
File content/browser/android/graphics_context.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/androi...
content/browser/android/graphics_context.h:27: virtual
WebKit::WebGraphicsContext3D* GetContext3D() = 0;
Who is calling this one now?

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_delegate.h:412: virtual void
AttachLayer(WebKit::WebLayer& layer) {}
Should we use const WebLayer&?

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:306:
gfx::GLSurfaceHandle handle =
We don't care about the return any more as it is same for every instance, right?
Should we just return gfx::kNullPluginWindow or gfx::kDummyPluginWindow?

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1374:
delegate_->AttachLayer(this, layer);
Isn't this assume WebContentsImpl only have one WebLayer?

WebContentsImpl uses RenderViewHostManager to manage the RenderWidgetHost. We
need to be careful of this.

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:400: virtual void
AttachLayer(WebKit::WebLayer& layer) OVERRIDE;
const WebLayer&

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/web_co...
File content/browser/web_contents/web_contents_view_android.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_view_android.cc:71: view->Show();
Should we only call Show if content_view_core_ is not null?

https://chromiumcodereview.appspot.com/10828356/diff/1/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/public/browser...
content/public/browser/web_contents_delegate.h:432: WebKit::WebLayer& layer) {}
const WebLayer&?

https://chromiumcodereview.appspot.com/10828356/diff/1/content/shell/android/...
File content/shell/android/shell_manager.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/shell/android/...
content/shell/android/shell_manager.h:28: void
ShellAttachLayer(WebKit::WebLayer& layer);
const WebLayer&

https://chromiumcodereview.appspot.com/10828356/diff/1/content/shell/shell.h
File content/shell/shell.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/shell/shell.h#...
content/shell/shell.h:106: virtual void AttachLayer(WebContents* web_contents,
const WebLayer&

https://chromiumcodereview.appspot.com/10828356/diff/1/content/shell/shell_an...
File content/shell/shell_android.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/1/content/shell/shell_an...
content/shell/shell_android.cc:73: void Shell::AttachLayer(WebContents*
web_contents, WebKit::WebLayer& layer) {
const WebLayer&

[no sievers, 2012-08-22 21:17:40]

http://codereview.chromium.org/10828356/diff/1/content/browser/android/graphi...
File content/browser/android/graphics_context.cc (right):

http://codereview.chromium.org/10828356/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.cc:52: return
context_->insertSyncPoint();
It also takes care of referencing the ANativeWindow for its lifetime and
allocating the texture image transport surface and freeing the textures.
I'll revisit this further when adding 'in process' GL support for uebercomp.
Then this can either go away, or if we want to support both it could become some
sort of factory.

On 2012/08/22 07:01:18, klobag.chromium wrote:
> Is the only reason to have this context_ is to make this call?

http://codereview.chromium.org/10828356/diff/1/content/browser/android/graphi...
File content/browser/android/graphics_context.h (right):

http://codereview.chromium.org/10828356/diff/1/content/browser/android/graphi...
content/browser/android/graphics_context.h:27: virtual
WebKit::WebGraphicsContext3D* GetContext3D() = 0;
removed
On 2012/08/22 07:01:19, klobag.chromium wrote:
> Who is calling this one now?

http://codereview.chromium.org/10828356/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/10828356/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_view_host_delegate.h:412: virtual void
AttachLayer(WebKit::WebLayer& layer) {}
On 2012/08/22 07:01:19, klobag.chromium wrote:
> Should we use const WebLayer&?

Unfortunately removeFromParent() is non-const :(

http://codereview.chromium.org/10828356/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

http://codereview.chromium.org/10828356/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host_view_android.cc:306:
gfx::GLSurfaceHandle handle =
On 2012/08/22 07:01:19, klobag.chromium wrote:
> We don't care about the return any more as it is same for every instance,
right?
> Should we just return gfx::kNullPluginWindow or gfx::kDummyPluginWindow?

This is actually not the browser-side compositor, but render-side compositor
surface (double-buffered FBO), which is still one per view.

http://codereview.chromium.org/10828356/diff/1/content/browser/web_contents/w...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10828356/diff/1/content/browser/web_contents/w...
content/browser/web_contents/web_contents_impl.cc:1374:
delegate_->AttachLayer(this, layer);
On 2012/08/22 07:01:19, klobag.chromium wrote:
> Isn't this assume WebContentsImpl only have one WebLayer?
> 
> WebContentsImpl uses RenderViewHostManager to manage the RenderWidgetHost. We
> need to be careful of this.

It could end up adding more than one theoretically, but since I set
drawsContent() to true/false in RWHVA::Show()/Hide(), only one should render at
any given time.

http://codereview.chromium.org/10828356/diff/1/content/browser/web_contents/w...
File content/browser/web_contents/web_contents_view_android.cc (right):

http://codereview.chromium.org/10828356/diff/1/content/browser/web_contents/w...
content/browser/web_contents/web_contents_view_android.cc:71: view->Show();
On 2012/08/22 07:01:19, klobag.chromium wrote:
> Should we only call Show if content_view_core_ is not null?

This eliminates a little diff vs. downstream, where it checks inside Show() for
content_view_core_ being non-null.

[klobag.chromium, 2012-08-23 02:08:11]

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/an...
File content/browser/android/graphics_context.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/an...
content/browser/android/graphics_context.cc:48: virtual
WebKit::WebGraphicsContext3D* GetContext3D() OVERRIDE {
remove this? Shouldn't OVERRIDE give you error?

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/compositor_impl_android.h:31: static
CompositorImpl* GetInstance();
I agree with Alex. I don't think it is a good idea to have a singleton
Compositor.

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/render_view_host_delegate.h:412: virtual void
AttachLayer(WebKit::WebLayer& layer) {}
at least we can do const WebLayer for this one.

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

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:150:
texture_layer_.setDrawsContent(true);
If we always call Show after the constructor, should we check
content_view_core_?

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:162: gfx::Size
bounds = CompositorImpl::GetInstance()->GetWindowBounds();
I am not convinced that compositor should provides the window bounds. As Alex
said earlier, in the WebView case, each WebView will have its own compositor. I
would like to see some other global if you needed the window bounds, instead of
making Compositor singleton.

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:301:
gfx::GLSurfaceHandle handle =
Still not clear for this. You are using the browser-side compositor surface for
the render-side compositor?

[no sievers, 2012-08-27 18:07:01]

http://codereview.chromium.org/10828356/diff/13001/content/browser/android/gr...
File content/browser/android/graphics_context.cc (right):

http://codereview.chromium.org/10828356/diff/13001/content/browser/android/gr...
content/browser/android/graphics_context.cc:48: virtual
WebKit::WebGraphicsContext3D* GetContext3D() OVERRIDE {
On 2012/08/23 02:08:12, klobag.chromium wrote:
> remove this? Shouldn't OVERRIDE give you error?

Done. (Yea unfortunately upstream does not build with clang for Android.)

http://codereview.chromium.org/10828356/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/10828356/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_delegate.h:412: virtual void
AttachLayer(WebKit::WebLayer& layer) {}
Actually, I just noticed that addChild() only takes a non-const pointer also.

http://codereview.chromium.org/10828356/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

http://codereview.chromium.org/10828356/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:150:
texture_layer_.setDrawsContent(true);
Eventually we should do that. But it would break ContentShell upstream right
now, because it does not have the WebContentsChanged logic in
ContentViewCoreImpl yet.

http://codereview.chromium.org/10828356/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:162: gfx::Size
bounds = CompositorImpl::GetInstance()->GetWindowBounds();
On 2012/08/23 02:08:12, klobag.chromium wrote:
> I am not convinced that compositor should provides the window bounds. As Alex
> said earlier, in the WebView case, each WebView will have its own compositor.
I
> would like to see some other global if you needed the window bounds, instead
of
> making Compositor singleton.

Done.

http://codereview.chromium.org/10828356/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_android.cc:301:
gfx::GLSurfaceHandle handle =
On 2012/08/23 02:08:12, klobag.chromium wrote:
> Still not clear for this. You are using the browser-side compositor surface
for
> the render-side compositor?

The handle is for the renderer-side compositor. But currently
TextureImageTransportSurface has a dependency on the parent UI context, that's
why we need to ask the compositor here (it uses the same surface to make the
renderer-side compositor context current and other things). Also, the two
textures are created from the UI context, see 
GraphicsContext::CreateForUI().
Should we keep using this mechanism in the longer run, I'd like to change that
though.

[klobag.chromium, 2012-08-27 23:44:55]

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

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:150:
texture_layer_.setDrawsContent(true);
Hmm, this can break instance where it doesn't have a content_view_core_, right?

On 2012/08/27 18:07:02, Daniel Sievers wrote:
> Eventually we should do that. But it would break ContentShell upstream right
> now, because it does not have the WebContentsChanged logic in
> ContentViewCoreImpl yet.

https://chromiumcodereview.appspot.com/10828356/diff/13001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_android.cc:301:
gfx::GLSurfaceHandle handle =
I think this deserves a comment.

On 2012/08/27 18:07:02, Daniel Sievers wrote:
> On 2012/08/23 02:08:12, klobag.chromium wrote:
> > Still not clear for this. You are using the browser-side compositor surface
> for
> > the render-side compositor?
> 
> The handle is for the renderer-side compositor. But currently
> TextureImageTransportSurface has a dependency on the parent UI context, that's
> why we need to ask the compositor here (it uses the same surface to make the
> renderer-side compositor context current and other things). Also, the two
> textures are created from the UI context, see 
> GraphicsContext::CreateForUI().
> Should we keep using this mechanism in the longer run, I'd like to change that
> though.

[klobag.chromium, 2012-08-28 00:12:06]

https://chromiumcodereview.appspot.com/10828356/diff/7004/content/browser/ren...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/7004/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:45:
WebKit::WebCompositor::initialize(NULL);
If SurfaceView is shown and hidden and shown again, SetWindowSurface will be
called with ANativeWindow, NULL, ANativeWindow, will this initialization do
twice? Will it have any side effect?

https://chromiumcodereview.appspot.com/10828356/diff/7004/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:52:
context_.reset(NULL);
Should you reset size_ as you use it to compare in SetWindowBounds?

https://chromiumcodereview.appspot.com/10828356/diff/7004/content/browser/ren...
content/browser/renderer_host/compositor_impl_android.cc:76:
WebKit::WebGraphicsContext3D::Attributes attrs;
Should you check context_ for null first as it may be reset?

[no sievers, 2012-08-29 02:30:49]

http://codereview.chromium.org/10828356/diff/7004/content/browser/renderer_ho...
File content/browser/renderer_host/compositor_impl_android.cc (right):

http://codereview.chromium.org/10828356/diff/7004/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:45:
WebKit::WebCompositor::initialize(NULL);
On 2012/08/28 00:12:07, klobag.chromium wrote:
> If SurfaceView is shown and hidden and shown again, SetWindowSurface will be
> called with ANativeWindow, NULL, ANativeWindow, will this initialization do
> twice? Will it have any side effect?

Done. Added a static Compositor::Initialize() function to be called by the app.

http://codereview.chromium.org/10828356/diff/7004/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:52:
context_.reset(NULL);
On 2012/08/28 00:12:07, klobag.chromium wrote:
> Should you reset size_ as you use it to compare in SetWindowBounds?

Done.

http://codereview.chromium.org/10828356/diff/7004/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:76:
WebKit::WebGraphicsContext3D::Attributes attrs;
On 2012/08/28 00:12:07, klobag.chromium wrote:
> Should you check context_ for null first as it may be reset?

Done.

[klobag.chromium, 2012-08-29 04:29:43]

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
File content/shell/android/shell_manager.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
content/shell/android/shell_manager.cc:87: Compositor::Initialize();
I like the old way (in #3) where you create compositor in Init().

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
content/shell/android/shell_manager.cc:92: if
(!g_global_state.Get().root_layer.get())
If compositor is created once, do we want to set root layer every time surface
is created? Should this be in init?

[no sievers, 2012-08-29 16:17:52]

http://codereview.chromium.org/10828356/diff/21001/content/shell/android/shel...
File content/shell/android/shell_manager.cc (right):

http://codereview.chromium.org/10828356/diff/21001/content/shell/android/shel...
content/shell/android/shell_manager.cc:87: Compositor::Initialize();
On 2012/08/29 04:29:43, klobag.chromium wrote:
> I like the old way (in #3) where you create compositor in Init().

But I cannot call Initialize() in Init() because it seems to be called before
the webkit thread and platform support stuff is initialized.

http://codereview.chromium.org/10828356/diff/21001/content/shell/android/shel...
content/shell/android/shell_manager.cc:92: if
(!g_global_state.Get().root_layer.get())
On 2012/08/29 04:29:43, klobag.chromium wrote:
> If compositor is created once, do we want to set root layer every time surface
> is created? Should this be in init?

Yes, I'll change that.

[klobag.chromium, 2012-08-29 22:55:04]

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
File content/shell/android/shell_manager.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
content/shell/android/shell_manager.cc:87: Compositor::Initialize();
How about in CreateShellView()?

On 2012/08/29 16:17:52, Daniel Sievers wrote:
> On 2012/08/29 04:29:43, klobag.chromium wrote:
> > I like the old way (in #3) where you create compositor in Init().
> 
> But I cannot call Initialize() in Init() because it seems to be called before
> the webkit thread and platform support stuff is initialized.

[no sievers, 2012-08-29 23:09:21]

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
File content/shell/android/shell_manager.cc (right):

https://chromiumcodereview.appspot.com/10828356/diff/21001/content/shell/andr...
content/shell/android/shell_manager.cc:87: Compositor::Initialize();
That works. Done.

[klobag.chromium, 2012-08-29 23:29:04]

lgtm

thanks

[no sievers, 2012-08-29 23:32:55]

jam for OWNERS

[jam, 2012-08-30 18:51:53]

https://codereview.chromium.org/10828356/diff/28001/content/browser/android/g...
File content/browser/android/graphics_context.cc (right):

https://codereview.chromium.org/10828356/diff/28001/content/browser/android/g...
content/browser/android/graphics_context.cc:24: class CmdBufferGraphicsContext :
public content::GraphicsContext {
nit: now that this is not an interface, can this move to the header?

https://codereview.chromium.org/10828356/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/10828356/diff/28001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:51: class WebLayer;
I can't see this class in WebKit tree yet, so I'm guessing it hasn't landed. I
can't tell whether this type exists for all platforms or just android? I.e.
whether this needs an ifdef or not, also same below and in other files

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

https://codereview.chromium.org/10828356/diff/28001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:18: #include
"content/public/browser/render_view_host.h"
it's a layering violation of RenderWidgetHost talks to RenderViewHost. how this
is usually done is that RWHImpl calls a virtual function on itself, which
RVWImpl overrides.

https://codereview.chromium.org/10828356/diff/28001/content/public/browser/we...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/10828356/diff/28001/content/public/browser/we...
content/public/browser/web_contents_delegate.h:437: virtual void
AttachLayer(WebContents* web_contents,
this needs to be behind OS_ANDROID as well.

can you move ShouldOverrideLoading down here, so that the ifdef section is
beside each other (the convention in the content api is that platform specific
section is at the end)? and update the cc files where it's implemented so that
they're in the same order.

[no sievers, 2012-08-31 01:31:10]

https://codereview.chromium.org/10828356/diff/28001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/10828356/diff/28001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:51: class WebLayer;
On 2012/08/30 18:51:54, John Abd-El-Malek wrote:
> I can't see this class in WebKit tree yet, so I'm guessing it hasn't landed. I
> can't tell whether this type exists for all platforms or just android? I.e.
> whether this needs an ifdef or not, also same below and in other files

Actually this exists for all platforms in
third_party/WebKit/Source/Platform/chromium/public/WebLayer.h

I'll just remove the accidental ifdef in web_contents_delegate.h, ok?

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

https://codereview.chromium.org/10828356/diff/28001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:18: #include
"content/public/browser/render_view_host.h"
On 2012/08/30 18:51:54, John Abd-El-Malek wrote:
> it's a layering violation of RenderWidgetHost talks to RenderViewHost. how
this
> is usually done is that RWHImpl calls a virtual function on itself, which
> RVWImpl overrides.

Done.

https://codereview.chromium.org/10828356/diff/28001/content/public/browser/we...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/10828356/diff/28001/content/public/browser/we...
content/public/browser/web_contents_delegate.h:437: virtual void
AttachLayer(WebContents* web_contents,
On 2012/08/30 18:51:54, John Abd-El-Malek wrote:
> this needs to be behind OS_ANDROID as well.
> 
> can you move ShouldOverrideLoading down here, so that the ifdef section is
> beside each other (the convention in the content api is that platform specific
> section is at the end)? and update the cc files where it's implemented so that
> they're in the same order.

See comment in render_view_host_delegate.h. I removed the ifdef above.

[jam, 2012-08-31 22:20:41]

looks much better, thanks.

It seems that those functions on RWH, RVH, RVHD, WC are android only, and it's a
bit confusing when they're not behind ifdefs (i.e. people on other ports will
wonder if they need to call them as well). can we put them behind ifdefs?

[no sievers, 2012-08-31 22:52:30]

On 2012/08/31 22:20:41, John Abd-El-Malek wrote:
> looks much better, thanks.
> 
> It seems that those functions on RWH, RVH, RVHD, WC are android only, and it's
a
> bit confusing when they're not behind ifdefs (i.e. people on other ports will
> wonder if they need to call them as well). can we put them behind ifdefs?

Yes, I was debating that too. I guess this will most likely only be used on
Android. So let me just ifdef it all.

[jam, 2012-09-04 16:44:47]

https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:50: #if
defined(OS_ANDROID)
nit: since this class exists on all platforms, get rid of the ifdef here to make
the code easier on the eyes

https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:374: void
DidCancelPopupMenu();
nit: you have this twice now.. also, I think it's better to keep one ifdef right
beside the interface for the overridden methods, and another below for other
android-only methods.

https://codereview.chromium.org/10828356/diff/33002/content/public/browser/we...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/10828356/diff/33002/content/public/browser/we...
content/public/browser/web_contents_delegate.h:60: #if defined(OS_ANDROID)
ditto

https://codereview.chromium.org/10828356/diff/33002/content/public/browser/we...
content/public/browser/web_contents_delegate.h:435: virtual void
AttachLayer(WebContents* web_contents,
nit: document these methods

[no sievers, 2012-09-05 16:18:56]

All done.

On 2012/09/04 16:44:47, John Abd-El-Malek wrote:
>
https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
> File content/browser/renderer_host/render_view_host_delegate.h (right):
> 
>
https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
> content/browser/renderer_host/render_view_host_delegate.h:50: #if
> defined(OS_ANDROID)
> nit: since this class exists on all platforms, get rid of the ifdef here to
make
> the code easier on the eyes
> 
>
https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
> File content/browser/renderer_host/render_view_host_impl.h (right):
> 
>
https://codereview.chromium.org/10828356/diff/33002/content/browser/renderer_...
> content/browser/renderer_host/render_view_host_impl.h:374: void
> DidCancelPopupMenu();
> nit: you have this twice now.. also, I think it's better to keep one ifdef
right
> beside the interface for the overridden methods, and another below for other
> android-only methods.
> 
>
https://codereview.chromium.org/10828356/diff/33002/content/public/browser/we...
> File content/public/browser/web_contents_delegate.h (right):
> 
>
https://codereview.chromium.org/10828356/diff/33002/content/public/browser/we...
> content/public/browser/web_contents_delegate.h:60: #if defined(OS_ANDROID)
> ditto
> 
>
https://codereview.chromium.org/10828356/diff/33002/content/public/browser/we...
> content/public/browser/web_contents_delegate.h:435: virtual void
> AttachLayer(WebContents* web_contents,
> nit: document these methods

[jam, 2012-09-05 16:33:47]

lgtm

[commit-bot: I haz the power, 2012-09-05 16:35:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/10828356/31004

[commit-bot: I haz the power, 2012-09-05 18:01:36]

Try job failure for 10828356-31004 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-05 18:06:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/10828356/31004

[commit-bot: I haz the power, 2012-09-05 23:07:23]

Try job failure for 10828356-31004 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...