Adds support for capturing a sub rect of the compositing surface.

Adds support for capturing a sub rect of the compositing surface.

Also removes the old PopulateBitmapWithContents API.

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

[powei, 2014-02-22 02:06:32]

+sievers@ as owner

lgtm with one comment

https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:256: gfx::Rect
src_subrect,
Can you add a dcheck to make sure that src_subrect is inside the bounds of the
layer?

[powei, 2014-02-22 02:15:20]

On 2014/02/22 02:06:32, powei wrote:
> +sievers@ as owner
> 
> lgtm with one comment
> 
>
https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/render_widget_host_view_android.cc:256:
gfx::Rect
> src_subrect,
> Can you add a dcheck to make sure that src_subrect is inside the bounds of the
> layer?

Sorry for adding more comments afterwards, but would it be possible to remove
PopulateBitmapWithContents in this patch as well?  Was the NTP the only use of
this method?

[no sievers, 2014-02-22 02:17:23]

On 2014/02/22 02:15:20, powei wrote:
> On 2014/02/22 02:06:32, powei wrote:
> > +sievers@ as owner
> > 
> > lgtm with one comment
> > 
> >
>
https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
> > content/browser/renderer_host/render_widget_host_view_android.cc:256:
> gfx::Rect
> > src_subrect,
> > Can you add a dcheck to make sure that src_subrect is inside the bounds of
the
> > layer?
> 
> Sorry for adding more comments afterwards, but would it be possible to remove
> PopulateBitmapWithContents in this patch as well?  Was the NTP the only use of
> this method?

lgtm if you have to.

But would prefer to remove it. Also powei was working on routing these readback
APIs through RenderWidgetHost instead.
We really only need one entry point.

[clholgat, 2014-02-24 16:26:03]

On 2014/02/22 02:17:23, sievers wrote:
> On 2014/02/22 02:15:20, powei wrote:
> > On 2014/02/22 02:06:32, powei wrote:
> > > +sievers@ as owner
> > > 
> > > lgtm with one comment
> > > 
> > >
> >
>
https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
> > > File content/browser/renderer_host/render_widget_host_view_android.cc
> (right):
> > > 
> > >
Looks like we were the only ones using the API, I'll remove it.

> >
>
https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
> > > content/browser/renderer_host/render_widget_host_view_android.cc:256:
> > gfx::Rect
> > > src_subrect,
> > > Can you add a dcheck to make sure that src_subrect is inside the bounds of
> the
> > > layer?
> > 
> > Sorry for adding more comments afterwards, but would it be possible to
remove
> > PopulateBitmapWithContents in this patch as well?  Was the NTP the only use
of
> > this method?
> 
> lgtm if you have to.
> 
> But would prefer to remove it. Also powei was working on routing these
readback
> APIs through RenderWidgetHost instead.
> We really only need one entry point.

[clholgat, 2014-02-24 17:15:55]

https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/173023005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:256: gfx::Rect
src_subrect,
On 2014/02/22 02:06:32, powei wrote:
> Can you add a dcheck to make sure that src_subrect is inside the bounds of the
> layer?

Done.

[miu, 2014-02-24 19:45:11]

It's not clear why code should call RWHVAndroid::GetScaledContentMap() instead
of directly calling RWHVAnrdoid::CopyFromCompositingSurface().  Is the extra
method needed?  Also, where are the comments explaining its purpose and usage in
the .h file?

[powei, 2014-02-24 19:54:15]

On 2014/02/24 19:45:11, miu wrote:
> It's not clear why code should call RWHVAndroid::GetScaledContentMap() instead
> of directly calling RWHVAnrdoid::CopyFromCompositingSurface().  Is the extra
> method needed?  Also, where are the comments explaining its purpose and usage
in
> the .h file?

We are moving towards removing GetScaledContentMap.  It used to be on a
different, synchronized readback path and then switched to use
CopyFromCompositingSurface.  Copying content will likely be exposed through the
single path RenderWidgetHost::CopyFromBackingStore for Android.  And hopefully
that will happen in a few incremental patches.

[miu, 2014-02-24 19:56:36]

On 2014/02/24 19:54:15, powei wrote:
> On 2014/02/24 19:45:11, miu wrote:
> > It's not clear why code should call RWHVAndroid::GetScaledContentMap()
instead
> > of directly calling RWHVAnrdoid::CopyFromCompositingSurface().  Is the extra
> > method needed?  Also, where are the comments explaining its purpose and
usage
> in
> > the .h file?
> 
> We are moving towards removing GetScaledContentMap.  It used to be on a
> different, synchronized readback path and then switched to use
> CopyFromCompositingSurface.  Copying content will likely be exposed through
the
> single path RenderWidgetHost::CopyFromBackingStore for Android.  And hopefully
> that will happen in a few incremental patches.

Ah.  SGTM, then.  :)

[clholgat, 2014-02-25 19:35:56]

On 2014/02/24 19:56:36, miu wrote:
> On 2014/02/24 19:54:15, powei wrote:
> > On 2014/02/24 19:45:11, miu wrote:
> > > It's not clear why code should call RWHVAndroid::GetScaledContentMap()
> instead
> > > of directly calling RWHVAnrdoid::CopyFromCompositingSurface().  Is the
extra
> > > method needed?  Also, where are the comments explaining its purpose and
> usage
> > in
> > > the .h file?
> > 
> > We are moving towards removing GetScaledContentMap.  It used to be on a
> > different, synchronized readback path and then switched to use
> > CopyFromCompositingSurface.  Copying content will likely be exposed through
> the
> > single path RenderWidgetHost::CopyFromBackingStore for Android.  And
hopefully
> > that will happen in a few incremental patches.
> 
> Ah.  SGTM, then.  :)

PTAL, stripping out the PopulateBitmapWithContents resulted in a bit of Android
refactoring. I want to make sure I didn't make any stupid mistakes.

[powei, 2014-02-25 19:38:20]

+tedchoc@ for review of taking out PopulateBitmapWithConents

[clholgat, 2014-02-26 21:20:02]

On 2014/02/25 19:38:20, powei wrote:
> +tedchoc@ for review of taking out PopulateBitmapWithConents

Friendly ping.

[Ted C, 2014-02-26 21:27:15]

lgtm

https://codereview.chromium.org/173023005/diff/180001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/NativePage.java
(right):

https://codereview.chromium.org/173023005/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/NativePage.java:38: boolean
isReadyForSnapshot();
Can you verify if these are all needed for NativePage[s]?  Would be nice to only
keep the ones we need.

[clholgat, 2014-02-26 21:38:34]

https://codereview.chromium.org/173023005/diff/180001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/NativePage.java
(right):

https://codereview.chromium.org/173023005/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/NativePage.java:38: boolean
isReadyForSnapshot();
On 2014/02/26 21:27:15, Ted C wrote:
> Can you verify if these are all needed for NativePage[s]?  Would be nice to
only
> keep the ones we need.

All calls return true, so I'm removing it.

[clholgat, 2014-02-26 22:03:52]

The CQ bit was checked by clholgat@chromium.org

[commit-bot: I haz the power, 2014-02-26 22:08:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clholgat@chromium.org/173023005/200001

[commit-bot: I haz the power, 2014-02-26 22:08:20]

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

[commit-bot: I haz the power, 2014-02-26 22:08:21]

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 #1 FAILED at 698.
  Hunk #2 FAILED at 705.
  Hunk #3 succeeded at 1460 (offset 1 line).
  2 out of 3 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
fefc89104c42d7c69e081309dab3400f1d204a01..ba96126b388c19698ba00cefca73e4cb3deb48b1
100644
--- a/content/browser/android/content_view_core_impl.cc
+++ b/content/browser/android/content_view_core_impl.cc
@@ -698,6 +698,7 @@ void ContentViewCoreImpl::ShowPastePopup(int x_dip, int
y_dip) {
 
 void ContentViewCoreImpl::GetScaledContentBitmap(
     float scale,
+    gfx::Rect src_subrect,
     const base::Callback<void(bool, const SkBitmap&)>& result_callback) {
   RenderWidgetHostViewAndroid* view = GetRenderWidgetHostViewAndroid();
   if (!view) {
@@ -705,7 +706,7 @@ void ContentViewCoreImpl::GetScaledContentBitmap(
     return;
   }
 
-  view->GetScaledContentBitmap(scale, result_callback);
+  view->GetScaledContentBitmap(scale, src_subrect, result_callback);
 }
 
 void ContentViewCoreImpl::StartContentIntent(const GURL& content_url) {
@@ -1459,16 +1460,6 @@ jboolean ContentViewCoreImpl::OnAnimate(JNIEnv* env,
jobject /* obj */,
   return view->Animate(base::TimeTicks::FromInternalValue(frame_time_micros));
 }
 
-jboolean ContentViewCoreImpl::PopulateBitmapFromCompositor(JNIEnv* env,
-                                                           jobject obj,
-                                                           jobject jbitmap) {
-  RenderWidgetHostViewAndroid* view = GetRenderWidgetHostViewAndroid();
-  if (!view)
-    return false;
-
-  return view->PopulateBitmapWithContents(jbitmap);
-}
-
 void ContentViewCoreImpl::WasResized(JNIEnv* env, jobject obj) {
   RenderWidgetHostViewAndroid* view = GetRenderWidgetHostViewAndroid();
   if (view)

[clholgat, 2014-02-26 22:38:32]

The CQ bit was checked by clholgat@chromium.org

[commit-bot: I haz the power, 2014-02-26 22:42:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clholgat@chromium.org/173023005/220001

[commit-bot: I haz the power, 2014-02-27 06:16:01]

Change committed as 253736

[clholgat, 2014-02-27 16:25:15]

On 2014/02/27 06:16:01, I haz the power (commit-bot) wrote:
> Change committed as 253736

Was reverted because the Android side patch had merge conflicts and couldn't be
pushed.

Reopening and attempting to reland.

[clholgat, 2014-02-27 16:25:20]

The CQ bit was checked by clholgat@chromium.org

[commit-bot: I haz the power, 2014-02-27 16:25:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clholgat@chromium.org/173023005/240001

[commit-bot: I haz the power, 2014-02-27 19:38:51]

Change committed as 253898