Fix Android tab navigation "stretch" regression.

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 5af7d031e7f5cf1d6e7f42a2fd9e8156f2c03a4d..416a575f57b08aa385945ef665d6c98586d8f780 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -539,24 +539,28 @@ void RenderWidgetHostViewAndroid::SetBounds(const gfx::Rect& rect) {
 void RenderWidgetHostViewAndroid::GetScaledContentBitmap(
     float scale,
     SkColorType preferred_color_type,
-    gfx::Rect src_subrect,
+    gfx::Rect src_subrect_in_pixel,

[aelias_OOO_until_Jul13, 2017/03/07 21:08:01]

It looks like this is always called with empty rect (callsites start in
ChromeActivity.java and CustomTabObserver.java), so please delete this parameter
throughout the entire call chain and also your else {} block.

[miu, 2017/03/08 01:42:04]

Yeah, I noticed this. Wasn't going to expand the scope of this change, but I'm
always a fan of reducing entropy in the codebase. ;-)

Done. In fact, I was able to delete this method altogether and have
WebContentsAndroid call CopyFromSurface() directly. I removed the bitmap format
and source rect from all the Java→JNI→C++ method signatures, and instead of a
scale factor, the caller must provide the exact width/height of the resulting
bitmap.

     const ReadbackRequestCallback& result_callback) {
   if (!host_ || host_->is_hidden() || !IsSurfaceAvailableForCopy()) {
     result_callback.Run(SkBitmap(), READBACK_SURFACE_UNAVAILABLE);
     return;
   }
-  gfx::Size bounds = current_surface_size_;
-  if (src_subrect.IsEmpty())
-    src_subrect = gfx::Rect(bounds);
-  DCHECK_LE(src_subrect.width() + src_subrect.x(), bounds.width());
-  DCHECK_LE(src_subrect.height() + src_subrect.y(), bounds.height());
-  const display::Display& display =
-      display::Screen::GetScreen()->GetDisplayNearestWindow(GetNativeView());
-  float device_scale_factor = display.device_scale_factor();
-  DCHECK_GT(device_scale_factor, 0);
-  gfx::Size dst_size(
-      gfx::ScaleToCeiledSize(src_subrect.size(), scale / device_scale_factor));
-  src_subrect = gfx::ConvertRectToDIP(device_scale_factor, src_subrect);
+
+  gfx::Rect src_subrect;
+  gfx::Size dst_size;
+  if (src_subrect_in_pixel.IsEmpty()) {
+    src_subrect = gfx::Rect(GetVisibleViewportSize());
+    dst_size = gfx::ScaleToCeiledSize(src_subrect.size(), scale);
+  } else {
+    const display::Display& display =
+        display::Screen::GetScreen()->GetDisplayNearestWindow(GetNativeView());
+    float device_scale_factor = display.device_scale_factor();
+    DCHECK_GT(device_scale_factor, 0);
+    src_subrect =
+        gfx::ConvertRectToDIP(device_scale_factor, src_subrect_in_pixel);
+    dst_size = gfx::ScaleToCeiledSize(src_subrect_in_pixel.size(),
+                                      scale / device_scale_factor);
+  }
 
   CopyFromSurface(src_subrect, dst_size, result_callback, preferred_color_type);
 }
@@ -942,8 +946,14 @@ void RenderWidgetHostViewAndroid::CopyFromSurface(
   float device_scale_factor = display.device_scale_factor();
   gfx::Size dst_size_in_pixel =
       gfx::ConvertRectToPixel(device_scale_factor, gfx::Rect(dst_size)).size();
-  gfx::Rect src_subrect_in_pixel =
-      gfx::ConvertRectToPixel(device_scale_factor, src_subrect);
+  // Note: When |src_subrect| is empty, a conversion from the view size must be
+  // made instead of using |current_frame_size_|. The latter sometimes also
+  // includes extra height for the toolbar UI, which is not intended for
+  // capture.
+  gfx::Rect src_subrect_in_pixel = gfx::ConvertRectToPixel(

[aelias_OOO_until_Jul13, 2017/03/07 21:08:01]

I don't think making a change here is necessary at all given that you've got the
empty-rect case covered in the higher-level spot GetScaledContentBitmap(),
right?  Could you just not touch this?  I consider doing something totally
different on empty rect to be an antipattern and I'd rather not introduce it
here.

[miu, 2017/03/08 01:42:04]

Actually, this is necessary. This method is the Android impl for the public
RenderWidgetHostView::CopyFromSurface() API. It is very common to want to
request "the whole content region" and many call points throughout Chromium
leverage that aspect of the public API.

Ref:
https://cs.chromium.org/chromium/src/content/public/browser/render_widget_hos...

+      device_scale_factor, src_subrect.IsEmpty()
+                               ? gfx::Rect(GetVisibleViewportSize())
+                               : src_subrect);
 
   if (!using_browser_compositor_) {
     SynchronousCopyContents(src_subrect_in_pixel, dst_size_in_pixel, callback,
@@ -1192,11 +1202,13 @@ void RenderWidgetHostViewAndroid::SynchronousCopyContents(
     const gfx::Size& dst_size_in_pixel,
     const ReadbackRequestCallback& callback,
     const SkColorType color_type) {
-  gfx::Size input_size_in_pixel;
-  if (src_subrect_in_pixel.IsEmpty())
-    input_size_in_pixel = current_surface_size_;
-  else
-    input_size_in_pixel = src_subrect_in_pixel.size();
+  // Current implementation does not support read-back of regions that do not
+  // originate at (0,0). http://crbug.com/698974
+  if (src_subrect_in_pixel.origin() != gfx::Point(0, 0)) {
+    callback.Run(SkBitmap(), READBACK_FAILED);

[aelias_OOO_until_Jul13, 2017/03/07 21:08:01]

Hmm, this is just replacing one bug by another from my POV.  I don't think it
would be that hard to apply the appropriate translation on the skia canvas to
make this work, but failing that I'd prefer for you to just add a comment
instead of changing anything (this bug has been present since this function was
written it looks like, no reason to rock the boat).

[miu, 2017/03/08 01:42:04]

Done.

+  }
+  const gfx::Size& input_size_in_pixel = src_subrect_in_pixel.size();
+  DCHECK(!input_size_in_pixel.IsEmpty());
 
   gfx::Size output_size_in_pixel;
   if (dst_size_in_pixel.IsEmpty())