Fix Android tab navigation "stretch" regression.

content/browser/renderer_host/render_widget_host_view_android.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
 
 #include <android/bitmap.h>
 
 #include <utility>
 
@@ skipping 521 matching lines @@
   default_bounds_ = gfx::Rect(default_bounds_.origin(), size);
 }
 
 void RenderWidgetHostViewAndroid::SetBounds(const gfx::Rect& rect) {
   default_bounds_ = 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);
 }
 
 bool RenderWidgetHostViewAndroid::HasValidFrame() const {
   if (!content_view_core_)
     return false;
 
   if (current_surface_size_.IsEmpty())
     return false;
@@ skipping 365 matching lines @@
     callback.Run(SkBitmap(), READBACK_FAILED);
     return;
   }
 
   base::TimeTicks start_time = base::TimeTicks::Now();
   const display::Display& display =
       display::Screen::GetScreen()->GetDisplayNearestWindow(GetNativeView());
   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,
                             preferred_color_type);
     UMA_HISTOGRAM_TIMES("Compositing.CopyFromSurfaceTimeSynchronous",
                         base::TimeTicks::Now() - start_time);
     return;
   }
 
   ui::WindowAndroidCompositor* compositor =
@@ skipping 228 matching lines @@
           // context) to ensure proper handle theming.
           activityContext.is_null() ? base::android::GetApplicationContext()
                                     : activityContext));
 }
 
 void RenderWidgetHostViewAndroid::SynchronousCopyContents(
     const gfx::Rect& src_subrect_in_pixel,
     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())
     output_size_in_pixel = input_size_in_pixel;
   else
     output_size_in_pixel = dst_size_in_pixel;
   int output_width = output_size_in_pixel.width();
   int output_height = output_size_in_pixel.height();
 
   if (!sync_compositor_) {
@@ skipping 763 matching lines @@
   ui::WindowAndroidCompositor* compositor = window_android->GetCompositor();
   if (!compositor)
     return;
 
   overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>(
       overscroll_refresh_handler, compositor,
       ui::GetScaleFactorForNativeView(GetNativeView()));
 }
 
 }  // namespace content