Fullscreen video in Android WebView

Fullscreen video in Android WebView

Make StreamTextureFactory and StreamTextureProxy abstract
base classes, and create a stub StreamTextureFactory for
Android Webview using synchronous compositor.

This provides enough plumbing to make full screen video
work in Android WebView. In-page video still requires
the in-process context to support streaming texture
manager.

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

[boliu, 2013-08-10 00:41:47]

Make StreamTextureFactory and StreamTextureProxy abstract base classes, and
create a stub StreamTextureFactory for android webview. In-page which requires a
context does not work, but full screen works.

Triggers an unrelated crash in power_save_blocker in webview so need to get that
fixed first.

[boliu, 2013-08-10 00:58:18]

+cc scherkus.

Man, this is my intern project all over again. Except this time it only took an
afternoon :)

[joth, 2013-08-10 01:43:44]

lgtm  for synchronous_compositor stuffs if other reviews are happy with the rest
:)

https://codereview.chromium.org/22766004/diff/1/content/browser/android/in_pr...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/22766004/diff/1/content/browser/android/in_pr...
content/browser/android/in_process/synchronous_compositor_impl.cc:79: }
time for this class to become out-of-line eh? (and its own cc/h. not this patch)

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/stream_texture_factory_android.h (right):

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
content/renderer/media/android/stream_texture_factory_android.h:8: //
TODO(boliu): move headers
don't understand comment? ah, to common or somethign.

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
content/renderer/media/android/stream_texture_factory_android.h:25: virtual
~StreamTextureProxy() {}
gives a clang warning? Or not if it's pure interface?

[no sievers, 2013-08-10 02:08:46]

lgtm with nits

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
File content/renderer/media/android/stream_texture_factory_android_impl.h
(right):

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
content/renderer/media/android/stream_texture_factory_android_impl.h:21:
GpuChannelHost* channel,
nit: indent

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
File
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.h
(right):

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.h:26:
const gfx::Size& size) OVERRIDE;
private:
  DISALLOW_IMPLICIT_CONSTRUCTORS()

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.h:31:
#endif  // CONTENT_RENDERER_MEDIA_ANDROID_STREAM_TEXTURE_FACTORY_ANDROID_IMPL_H_
nit: // CONTENT_RENDERER_MEDIA_ANDROID_STREAM_TEXTURE_FACTORY_ANDROID_SYNCHRONOU
    S_IMPL_H_

[qinmin, 2013-08-10 02:45:43]

lgtm % nit

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/stream_texture_factory_android.h (right):

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
content/renderer/media/android/stream_texture_factory_android.h:52: virtual
~StreamTextureFactory() {}
nit: move to cc file

[boliu, 2013-08-10 04:17:03]

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/stream_texture_factory_android.h (right):

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
content/renderer/media/android/stream_texture_factory_android.h:8: //
TODO(boliu): move headers
On 2013/08/10 01:43:44, joth wrote:
> don't understand comment? ah, to common or somethign.

Meant moving to the impl.h header. Done in PS2.

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
content/renderer/media/android/stream_texture_factory_android.h:25: virtual
~StreamTextureProxy() {}
On 2013/08/10 01:43:44, joth wrote:
> gives a clang warning? Or not if it's pure interface?

This is allowed by chromium style guide for inlining destructor of abstract base
classes with no members:
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....

Also compiled locally with clang and it didn't complain.

https://codereview.chromium.org/22766004/diff/1/content/renderer/media/androi...
content/renderer/media/android/stream_texture_factory_android.h:52: virtual
~StreamTextureFactory() {}
On 2013/08/10 02:45:43, qinmin wrote:
> nit: move to cc file

Again, this is allowed by the style guide:
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-When-yo....

Also if we do move this to separate cc file, it would only contain two empty
destructors, too much churn.

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
File content/renderer/media/android/stream_texture_factory_android_impl.h
(right):

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
content/renderer/media/android/stream_texture_factory_android_impl.h:21:
GpuChannelHost* channel,
On 2013/08/10 02:08:46, sievers wrote:
> nit: indent

Done.

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
File
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.h
(right):

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.h:26:
const gfx::Size& size) OVERRIDE;
On 2013/08/10 02:08:46, sievers wrote:
> private:
>   DISALLOW_IMPLICIT_CONSTRUCTORS()

Done.

https://codereview.chromium.org/22766004/diff/5001/content/renderer/media/and...
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.h:31:
#endif  // CONTENT_RENDERER_MEDIA_ANDROID_STREAM_TEXTURE_FACTORY_ANDROID_IMPL_H_
On 2013/08/10 02:08:46, sievers wrote:
> nit: //
CONTENT_RENDERER_MEDIA_ANDROID_STREAM_TEXTURE_FACTORY_ANDROID_SYNCHRONOU
>     S_IMPL_H_

Done.

[boliu, 2013-08-10 04:26:31]

Antoine, could you stamp content_renderer.gypi and render_view_impl.cc, and
maybe skim through other files.

This is mostly an android only change.

[piman, 2013-08-10 05:18:16]

lgtm

[commit-bot: I haz the power, 2013-08-10 05:20:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/22766004/14001

[commit-bot: I haz the power, 2013-08-10 15:10:44]

Change committed as 216864