[Android WebView] Migrate the rendering code to a separate set of classes.

[Android WebView] Migrate the rendering code to a separate set of classes.

It takes from https://codereview.chromium.org/11823027/
and assumes SW rendering and Capture Picture to be ready and enabled.

Most changes just move around code. The major structural changes are:
- Introduce a browser-layer view renderer interface and move the code to its implementation.
- Take out the rendering-related IPC to its own separate set of host/renderer classes.
- Change the way the view hierarchy and the compositor are initialized. Now they are created and set on BrowserViewRendererImpl construction.
- Content is now provided via a ContentViewCore object when initialized, updating the layer to use and the WebContents to observe.
- Remove/update the DEPS and gyp changes introduced to support rendering in the native layer.

BUG=167913, 167908, 161409
NOTRY=true

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

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

[benm (inactive), 2013-01-21 20:34:49]

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
File android_webview/browser/renderer_host/view_renderer_host.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/view_renderer_host.cc:25: // Might lead to
crashes if the connection is not ready yet.
What might lead to crashes? Calling this function or the if statement below?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
File android_webview/browser/renderer_host/view_renderer_host.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/view_renderer_host.h:37: using
content::WebContentsObserver::Observe;
nit: Is this needed? Even if so, I don't think we are supposed to have 'using'
in headers (only .cc)

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.cc:13: bool g_is_skia_version_compatible =
false;
nit: maybe a comment to explain about skia compatibility?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:37:
base::android::ScopedJavaLocalRef<jobject> picture) = 0;
const reference?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:125: void
ViewRendererImpl::SetContents(ContentViewCore* content_view_core) {
Could this param be WebContents rather than CVC? Then function name becomes
SetWebContents?


aah, just seen below you are using CVC to get the root layer. Guess you can't do
that with just a WebContents?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:149: // TODO(leandrogracia):
remove when crbug.com/164140 is closed.
What is this section for?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:478: void
ViewRendererImpl::OnSwapBuffersCompleted() {
is this a TODO or intentionally a no-op?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:526: void
ViewRendererImpl::Invalidate() {
Should this function be a TODO: remove when using UberCompositor?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:543: //compositor_->Composite();
intended to be commented? When should it be uncommented?

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/RenderHelper.java
(right):

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:35:
bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
in the case that bitmap != null, would bitmap.recycle be helpful?

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
File android_webview/native/render_helper.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
android_webview/native/render_helper.h:22: JNIEnv* env, int width, int height)
OVERRIDE;
nit: indent

[joth, 2013-01-21 22:54:23]

https://codereview.chromium.org/12041009/diff/1/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/DEPS#newcode10
android_webview/DEPS:10: "!chrome/browser/component",
fyi this can go now too. (I think it's an empty path now).

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
File android_webview/browser/renderer_host/view_renderer_host.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/view_renderer_host.cc:39: if (client_)
nit: use { } when the conditional is multiline

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:27: class ViewRenderer {
The normal naming scheme in chromium is if FooBar is a renderer/ class, then
FooBarHost is the browser side of the same.

This is getting confused here by the render_host/ separation.

We could have
renderer/  - ViewRenderer
browser/render_host/ - ViewRendererHost
browser/ [Aw|Browser]ViewRenderer

?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:37:
base::android::ScopedJavaLocalRef<jobject> picture) = 0;
On 2013/01/21 20:34:49, benm wrote:
> const reference?

can even be const JavaRef<jobject>&

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:106: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:434: return
java_helper()->RecordRasterizedBitmap(env, jbitmap.obj());
there's a subtle snag here. As you're using the RenderHelper::CreateBitmap, than
maybe holding an internal soft-reference to the bitmap. The recorded picture
also just holds a reference to the bitmap. So a subsequent capturePicture or
DrawSW call could end up overrighting the content of that bitmap (if it's still
got a valid soft ref) and so modify the historical captured pictures.

As this is only in the fallback non-matching skia version path I think it's
fine. May want to just comment this here though.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer_impl.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.h:23: class ViewRendererImpl : public
ViewRenderer,
Does this need to be a separate class to ViewRenderer? Normally we've just done
that when the base class needs to be declared (and used) in browser/ but the
impl part needs to be in native (to access JNI). This is the other way though,
so I don't know the abstract base class adds much?

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:306: //
reinterpret_cast back to AwRenderer pointer in the native DrawGLFunction.
what's AwRenderer ?

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/RenderHelper.java
(right):

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:32:
private static Bitmap createBitmap(int width, int height) {
maybe  createTemporaryBitmap? to reflect fact the returned bitmap shouldn't be
held onto beyond the current callstack.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:35:
bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
On 2013/01/21 20:34:49, benm wrote:
> in the case that bitmap != null, would bitmap.recycle be helpful?

In general this could be dangerous, as the caller maybe still holding a
reference to that old bitmap (see my comment in
https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...)
and recycle actually destroys the bitmap.

The complication is this one until method can be used for 2 different purposes,
(both only as fallbacks for dev purposes...) and only one is truly 'temporary'
bitmap (as the capture-picture could theoretically hold onto the bitmap
indefinitely).

Hmm. as this is all fallback mode only, how about we just remove the
SoftReference altogether. Spend the effort on making the real supported modes
efficient instead :)

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:55:
private static Picture recordRasterizedBitmap(Bitmap bitmap) {
maybe clearer as createPictureFromBitmap or convertBitmapToPicture or
recordBitmapIntoPicture.

The fact that this is a "rasterized" bitmap isn't very interesting (what other
sort of bitmap is there?) and 'record' was ambiguous to me without the word
"Picture' in the name too.

https://codereview.chromium.org/12041009/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.cc:538: view_renderer_->EnableOnNewPicture(
Mode m = ViewRenderer::kOnNewPictureDisabled;
if (enabled) {
  m = invalidation_only ? ViewRenderer::kOnNewPictureInvalidationOnly :
                          ViewRenderer::kOnNewPictureEnabled;
}

https://codereview.chromium.org/12041009/diff/1/android_webview/renderer/view...
File android_webview/renderer/view_renderer_rv.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/renderer/view...
android_webview/renderer/view_renderer_rv.h:14: class ViewRendererRV : public
content::RenderViewObserver {
The RV in the name is non-obvious.
But BrowserViewRenderer and RendererViewRenderer are even more clumsy :)

ViewRendererHost and ViewRenderer seem OK to me.

We can bikeshed over whether the Aw prefix would help pigeon hole it better
too...

[mkosiba (inactive), 2013-01-22 02:13:34]

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.cc:32: LOG(WARNING) << "Skia native
versions are not compatible.";
nit: maybe explain the impact ("..are not compatible, rendering performance will
suffer"?)

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.cc:41: bool
ViewRenderer::IsSkiaVersionCompatible() {
DCHECK(g_sw_draw_functions != NULL) ?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:44: class JavaHelper {
since there is a coresponding java-side class this should have a "full" name.
maybe JavaViewRendererHelper (or JavaViewRendererDelegate?)

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:72: virtual void
SetContents(content::ContentViewCore* content_view_core) = 0;
naming nit: SetContents sounds like a WebContents-related method.
SetContentViewCore ?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:48: jobject jbitmap,
can we use a ScopedLocalJavaRef instead?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:82: LOG(WARNING) << "Error
unlocking java bitmap pixels.";
LOG(ERROR)? same for the previous methods?

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:130:
view_renderer_host_->Observe(web_contents_);
as I think joth pointed out. The naming seems off... how can a ViewRenderer
have-a ViewRendererHost. The host should be on the other side of the IPC channel
and thus impossible to access via pointer.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/RenderHelper.java
(right):

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:22:
public abstract class RenderHelper {
nit1: like in my other comment - you should probably call this the same as the
C++ class (maybe JavaViewRendererHelper (or JavaViewRendererDelegate?)

ni2: rather than marking it abstract (which means "inherit me to make me
useful", make the constructor private).

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
File android_webview/native/render_helper.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
android_webview/native/render_helper.h:25: jobject jcanvas) OVERRIDE;
again.. ScopedJava..Ref nit.

[Leandro Graciá Gil, 2013-01-22 08:18:46]

https://codereview.chromium.org/12041009/diff/1/android_webview/DEPS
File android_webview/DEPS (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/DEPS#newcode10
android_webview/DEPS:10: "!chrome/browser/component",
On 2013/01/21 22:54:24, joth wrote:
> fyi this can go now too. (I think it's an empty path now).

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
File android_webview/browser/renderer_host/view_renderer_host.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/view_renderer_host.cc:25: // Might lead to
crashes if the connection is not ready yet.
On 2013/01/21 20:34:49, benm wrote:
> What might lead to crashes? Calling this function or the if statement below?

Oh, this should not be here. This is the workaround for one of the IPC fixes and
hopefully we should not need it. What might lead to crashes is the synchronous
IPC call if we don't have a connection.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/view_renderer_host.cc:39: if (client_)
On 2013/01/21 22:54:24, joth wrote:
> nit: use { } when the conditional is multiline

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
File android_webview/browser/renderer_host/view_renderer_host.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/view_renderer_host.h:37: using
content::WebContentsObserver::Observe;
On 2013/01/21 20:34:49, benm wrote:
> nit: Is this needed? Even if so, I don't think we are supposed to have 'using'
> in headers (only .cc)

It's used to change the WebContents to observe in ViewRenderer::SetContents. We
had a similar issue in aw_render_view_ext.h (now in view_renderer_rv.h) to fix a
method visibility problem, and that was actually a solution proposed by joi.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.cc:13: bool g_is_skia_version_compatible =
false;
On 2013/01/21 20:34:49, benm wrote:
> nit: maybe a comment to explain about skia compatibility?

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.cc:32: LOG(WARNING) << "Skia native
versions are not compatible.";
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> nit: maybe explain the impact ("..are not compatible, rendering performance
will
> suffer"?)

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.cc:41: bool
ViewRenderer::IsSkiaVersionCompatible() {
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> DCHECK(g_sw_draw_functions != NULL) ?

Nope. It will be null for tests.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:27: class ViewRenderer {
On 2013/01/21 22:54:24, joth wrote:
> The normal naming scheme in chromium is if FooBar is a renderer/ class, then
> FooBarHost is the browser side of the same.
> 
> This is getting confused here by the render_host/ separation.
> 
> We could have
> renderer/  - ViewRenderer
> browser/render_host/ - ViewRendererHost
> browser/ [Aw|Browser]ViewRenderer
> 
> ?

Going for:
renderer/ - ViewRenderer
browser/renderer_host/ - ViewRendererHost
browser/ - BrowserViewRenderer

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:37:
base::android::ScopedJavaLocalRef<jobject> picture) = 0;
On 2013/01/21 22:54:24, joth wrote:
> On 2013/01/21 20:34:49, benm wrote:
> > const reference?
> 
> can even be const JavaRef<jobject>&

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:44: class JavaHelper {
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> since there is a coresponding java-side class this should have a "full" name.
> maybe JavaViewRendererHelper (or JavaViewRendererDelegate?)

There is a Java-side class implementing this, but since the browser layer cannot
see native, it's simply defining an interface that exposes the Java requirements
from rendering. That's why it's simply called JavaHelper and doesn't make any
name reference to its implementor in the native layer. We can, however, name
that implementor JavaViewRenderHelper or whatever you suggest.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:72: virtual void
SetContents(content::ContentViewCore* content_view_core) = 0;
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> naming nit: SetContents sounds like a WebContents-related method.
> SetContentViewCore ?

Actually, it gets the WebContents and the content cc::Layer from
ContentViewCore. We could either change the name or the arguments to provide
these two. Whatever you prefer.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer.h:106: };
On 2013/01/21 22:54:24, joth wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:48: jobject jbitmap,
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> can we use a ScopedLocalJavaRef instead?

Actually, I think we could use a const JavaRef<jobject>& here. Thanks for
pointing this.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:82: LOG(WARNING) << "Error
unlocking java bitmap pixels.";
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> LOG(ERROR)? same for the previous methods?

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:125: void
ViewRendererImpl::SetContents(ContentViewCore* content_view_core) {
On 2013/01/21 20:34:49, benm wrote:
> Could this param be WebContents rather than CVC? Then function name becomes
> SetWebContents?
> 
> 
> aah, just seen below you are using CVC to get the root layer. Guess you can't
do
> that with just a WebContents?

We could go the other way around: WebContents -> ContentViewCore -> root layer
(as it lives in ContentViewCoreImpl). However this method is currently called
when ContentViewCore has been initialized, and passing WebContents instead might
lead to think we don't need to care about that, just have WebContents set.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:130:
view_renderer_host_->Observe(web_contents_);
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> as I think joth pointed out. The naming seems off... how can a ViewRenderer
> have-a ViewRendererHost. The host should be on the other side of the IPC
channel
> and thus impossible to access via pointer.

Naming fixed (I hope).

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:149: // TODO(leandrogracia):
remove when crbug.com/164140 is closed.
On 2013/01/21 20:34:49, benm wrote:
> What is this section for?

This is our temporary manual GL state restoration. It has well defined blocks so
that the framework guys could remove code from here in order to easily test any
possible state mismatch issues.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:434: return
java_helper()->RecordRasterizedBitmap(env, jbitmap.obj());
On 2013/01/21 22:54:24, joth wrote:
> there's a subtle snag here. As you're using the RenderHelper::CreateBitmap,
than
> maybe holding an internal soft-reference to the bitmap. The recorded picture
> also just holds a reference to the bitmap. So a subsequent capturePicture or
> DrawSW call could end up overrighting the content of that bitmap (if it's
still
> got a valid soft ref) and so modify the historical captured pictures.
> 
> As this is only in the fallback non-matching skia version path I think it's
> fine. May want to just comment this here though.

Good point. I'll be removing the soft reference.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:478: void
ViewRendererImpl::OnSwapBuffersCompleted() {
On 2013/01/21 20:34:49, benm wrote:
> is this a TODO or intentionally a no-op?

Intentionally a no-op, but I just noticed that this is an empty virtual rather
than a pure one, so we can safely remove it.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:526: void
ViewRendererImpl::Invalidate() {
On 2013/01/21 20:34:49, benm wrote:
> Should this function be a TODO: remove when using UberCompositor?

Nope. This is the method that triggers invalidate in the container Java view.
UberCompositor should call this.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.cc:543: //compositor_->Composite();
On 2013/01/21 20:34:49, benm wrote:
> intended to be commented? When should it be uncommented?

Same as above, once we use the compositor for SW rendering. I left that
commented to remind that is_composite_pending_ must be set to false once SW
rendering composites. Given the many other fixes we'll need, we're likely to
forget and produce a bug.

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
File android_webview/browser/view_renderer_impl.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...
android_webview/browser/view_renderer_impl.h:23: class ViewRendererImpl : public
ViewRenderer,
On 2013/01/21 22:54:24, joth wrote:
> Does this need to be a separate class to ViewRenderer? Normally we've just
done
> that when the base class needs to be declared (and used) in browser/ but the
> impl part needs to be in native (to access JNI). This is the other way though,
> so I don't know the abstract base class adds much?

We could just have the implementation if we want. I created a separate interface
in case we ever want to introduce mocks and unit testing in the future, and to
make clearer what exact methods are required for rendering. If you still prefer
me to change this let me know.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:306: //
reinterpret_cast back to AwRenderer pointer in the native DrawGLFunction.
On 2013/01/21 22:54:24, joth wrote:
> what's AwRenderer ?

Good catch. That was my first temporary name for the ViewRenderer class, and
this is the consequence of running regex replace in vim without checking all the
cases. It should be ViewRenderer (now BrowserViewRenderer) instead.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/RenderHelper.java
(right):

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:22:
public abstract class RenderHelper {
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> nit1: like in my other comment - you should probably call this the same as the
> C++ class (maybe JavaViewRendererHelper (or JavaViewRendererDelegate?)
> 
> ni2: rather than marking it abstract (which means "inherit me to make me
> useful", make the constructor private).

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:32:
private static Bitmap createBitmap(int width, int height) {
On 2013/01/21 22:54:24, joth wrote:
> maybe  createTemporaryBitmap? to reflect fact the returned bitmap shouldn't be
> held onto beyond the current callstack.

Leaving as it is since we're removing the soft reference code.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:35:
bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
On 2013/01/21 22:54:24, joth wrote:
> On 2013/01/21 20:34:49, benm wrote:
> > in the case that bitmap != null, would bitmap.recycle be helpful?
> 
> In general this could be dangerous, as the caller maybe still holding a
> reference to that old bitmap (see my comment in
>
https://codereview.chromium.org/12041009/diff/1/android_webview/browser/view_...)
> and recycle actually destroys the bitmap.
> 
> The complication is this one until method can be used for 2 different
purposes,
> (both only as fallbacks for dev purposes...) and only one is truly 'temporary'
> bitmap (as the capture-picture could theoretically hold onto the bitmap
> indefinitely).
> 
> Hmm. as this is all fallback mode only, how about we just remove the
> SoftReference altogether. Spend the effort on making the real supported modes
> efficient instead :)

Removing SoftReference as suggested.

https://codereview.chromium.org/12041009/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/RenderHelper.java:55:
private static Picture recordRasterizedBitmap(Bitmap bitmap) {
On 2013/01/21 22:54:24, joth wrote:
> maybe clearer as createPictureFromBitmap or convertBitmapToPicture or
> recordBitmapIntoPicture.
> 
> The fact that this is a "rasterized" bitmap isn't very interesting (what other
> sort of bitmap is there?) and 'record' was ambiguous to me without the word
> "Picture' in the name too.

Using recordBitmapIntoPicture to make it consistent with
https://codereview.chromium.org/11823027/.

https://codereview.chromium.org/12041009/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.cc:538: view_renderer_->EnableOnNewPicture(
On 2013/01/21 22:54:24, joth wrote:
> Mode m = ViewRenderer::kOnNewPictureDisabled;
> if (enabled) {
>   m = invalidation_only ? ViewRenderer::kOnNewPictureInvalidationOnly :
>                           ViewRenderer::kOnNewPictureEnabled;
> }

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
File android_webview/native/render_helper.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
android_webview/native/render_helper.h:22: JNIEnv* env, int width, int height)
OVERRIDE;
On 2013/01/21 20:34:49, benm wrote:
> nit: indent

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/native/render...
android_webview/native/render_helper.h:25: jobject jcanvas) OVERRIDE;
On 2013/01/22 02:13:34, Martin Kosiba wrote:
> again.. ScopedJava..Ref nit.

Done.

https://codereview.chromium.org/12041009/diff/1/android_webview/renderer/view...
File android_webview/renderer/view_renderer_rv.h (right):

https://codereview.chromium.org/12041009/diff/1/android_webview/renderer/view...
android_webview/renderer/view_renderer_rv.h:14: class ViewRendererRV : public
content::RenderViewObserver {
On 2013/01/21 22:54:24, joth wrote:
> The RV in the name is non-obvious.
> But BrowserViewRenderer and RendererViewRenderer are even more clumsy :)
> 
> ViewRendererHost and ViewRenderer seem OK to me.
> 
> We can bikeshed over whether the Aw prefix would help pigeon hole it better
> too... 

Using ViewRenderer & ViewRendererHost + BrowserViewRenderer.

[Leandro Graciá Gil, 2013-02-06 19:58:23]

Ok, this is not review-only any more. After
https://codereview.chromium.org/11861008/ lands this should be ready to go, so
it's time for the real (long) review. Again, it's basically moving things
around.

[joth, 2013-02-06 20:26:31]

only nits now - lgtm with these fixed up and if others' comments were all
addressed.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/DEPS
File android_webview/browser/DEPS (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/D...
android_webview/browser/DEPS:14: "+ui/gfx",
nit: \n before the ui/.... section

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer.h:109: JavaHelper* java_helper_;
I think if we're going to make this a real interface (all pure-virtual) we
should remove the data members from here too.

(I'm fine with either no interface or pure interface as you see best)

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:67: bool succeeded =
false;
nit: you can remove = false here, as it's unconditionally assigned below.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:383:
base::Bind(&BrowserViewRendererImpl::RenderSW, base::Unretained(this))))
     if (!RasterizeIntoBitmap(env, jbitmap, clip.x(), clip.y(),
                              base::Bind(&BrowserViewRendererImpl::RenderSW,
                                         base::Unretained(this)))) {
        return false;
      }

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:401: SkCanvas
canvas(&device);
feels like there's some repetition here with RasterizeIntoBitmap but I can't see
the whole picture clear enough to suggest an alternative factoring

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:463: bool window_visible)
{
indent

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.h (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.h:26: public
content::Compositor::Client {
is this indenting style normal? Anyway, it's certainly nice to read :)

https://codereview.chromium.org/12041009/diff/14001/android_webview/java/src/...
File
android_webview/java/src/org/chromium/android_webview/JavaBrowserViewRendererHelper.java
(right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/JavaBrowserViewRendererHelper.java:2:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
File android_webview/renderer/view_renderer.cc (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
android_webview/renderer/view_renderer.cc:24: capture_picture_enabled_(true) {
unindent

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
android_webview/renderer/view_renderer.cc:48:
//RendererPictureMap::GetInstance()->SetRendererPicture(routing_id(), picture);
comment? or remove.

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
android_webview/renderer/view_renderer.cc:58: routing_id(),
render_view()->CapturePicture());*/
ditto

[mkosiba (inactive), 2013-02-07 10:10:29]

lgtm

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:288:
transform.Translate(hw_rendering_scroll_.x(), hw_rendering_scroll_.y());
like we discussed earlier - when we have syncrhonous rendering we should be
undoing the translation coming from the compositor, not the translation coming
from the Android view system, because the translation coming from Android could
be outside the bounds of the compositor transformation (for example, when
overscrolling we'd have a negative scroll offset which is something the
compositor might not support).

So maybe add a TODO? :)

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:333: color_mask[3]);
nit: indent

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:341: blend_src_alpha,
blend_dest_alpha);
nit:indent

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:353: scissor_box[3]);
nit: indent

[Leandro Graciá Gil, 2013-02-07 12:43:56]

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/DEPS
File android_webview/browser/DEPS (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/D...
android_webview/browser/DEPS:14: "+ui/gfx",
On 2013/02/06 20:26:31, joth wrote:
> nit: \n before the ui/.... section

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer.h:109: JavaHelper* java_helper_;
On 2013/02/06 20:26:31, joth wrote:
> I think if we're going to make this a real interface (all pure-virtual) we
> should remove the data members from here too.
> 
> (I'm fine with either no interface or pure interface as you see best)

Made a pure interface. Moving data members and SetAwDrawSWFunctionTable to the
implementation class.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.cc (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:67: bool succeeded =
false;
On 2013/02/06 20:26:31, joth wrote:
> nit: you can remove = false here, as it's unconditionally assigned below.

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:288:
transform.Translate(hw_rendering_scroll_.x(), hw_rendering_scroll_.y());
On 2013/02/07 10:10:29, Martin Kosiba wrote:
> like we discussed earlier - when we have syncrhonous rendering we should be
> undoing the translation coming from the compositor, not the translation coming
> from the Android view system, because the translation coming from Android
could
> be outside the bounds of the compositor transformation (for example, when
> overscrolling we'd have a negative scroll offset which is something the
> compositor might not support).
> 
> So maybe add a TODO? :)

Good point. Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:333: color_mask[3]);
On 2013/02/07 10:10:29, Martin Kosiba wrote:
> nit: indent

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:341: blend_src_alpha,
blend_dest_alpha);
On 2013/02/07 10:10:29, Martin Kosiba wrote:
> nit:indent

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:353: scissor_box[3]);
On 2013/02/07 10:10:29, Martin Kosiba wrote:
> nit: indent

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:383:
base::Bind(&BrowserViewRendererImpl::RenderSW, base::Unretained(this))))
On 2013/02/06 20:26:31, joth wrote:
>      if (!RasterizeIntoBitmap(env, jbitmap, clip.x(), clip.y(),
>                               base::Bind(&BrowserViewRendererImpl::RenderSW,
>                                          base::Unretained(this)))) {
>         return false;
>       }

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:401: SkCanvas
canvas(&device);
On 2013/02/06 20:26:31, joth wrote:
> feels like there's some repetition here with RasterizeIntoBitmap but I can't
see
> the whole picture clear enough to suggest an alternative factoring

It's similar but not quite the same. RasterizeIntoBitmap creates a SkCanvas to
draw in a Java Bitmap object, while this creates a SkCanvas that mimics another
bitmap-based SkCanvas in the Android side.

We could try creating some kind of "SkCanvas from abstract bitmap" method, but
that would require about 5 arguments for only a few lines and the canvas would
be tricky to return (SkCanvas and SkDevice require arguments on construction and
SkCanvas has no copy constructor).

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:463: bool window_visible)
{
On 2013/02/06 20:26:31, joth wrote:
> indent

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.h (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.h:26: public
content::Compositor::Client {
On 2013/02/06 20:26:31, joth wrote:
> is this indenting style normal? Anyway, it's certainly nice to read :)

It seems I got this bit from the WebKit-styled files in cc/. Updating it to the
Chromium style, although I can leave it like this if you think that's ok.

https://codereview.chromium.org/12041009/diff/14001/android_webview/java/src/...
File
android_webview/java/src/org/chromium/android_webview/JavaBrowserViewRendererHelper.java
(right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/JavaBrowserViewRendererHelper.java:2:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/02/06 20:26:31, joth wrote:
> nit: 2013

Done.

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
File android_webview/renderer/view_renderer.cc (right):

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
android_webview/renderer/view_renderer.cc:24: capture_picture_enabled_(true) {
On 2013/02/06 20:26:31, joth wrote:
> unindent

That's still part of the constructor initialization list. As such, that should
be the correct indentation.

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
android_webview/renderer/view_renderer.cc:48:
//RendererPictureMap::GetInstance()->SetRendererPicture(routing_id(), picture);
On 2013/02/06 20:26:31, joth wrote:
> comment? or remove.

This is the part of the patch depending on
https://codereview.chromium.org/11861008/. I've temporarily commented them out
to give a first try in the bots. It will be uncommented when rebasing.

https://codereview.chromium.org/12041009/diff/14001/android_webview/renderer/...
android_webview/renderer/view_renderer.cc:58: routing_id(),
render_view()->CapturePicture());*/
On 2013/02/06 20:26:31, joth wrote:
> ditto

Same as above.

[Leandro Graciá Gil, 2013-02-15 13:59:06]

Landing with NOTRY=true as soon as the Android bots are happy.

[commit-bot: I haz the power, 2013-02-15 15:01:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/12041009/31001

[commit-bot: I haz the power, 2013-02-15 15:03:22]

Change committed as 182710

[commit-bot: I haz the power, 2013-02-15 15:23:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/12041009/29026

[commit-bot: I haz the power, 2013-02-15 15:47:25]

Change committed as 182717