Reland "Reland "Fix node screenshot. Part 1. Add `captureBeyondViewport` param...

Reland "Reland "Fix node screenshot. Part 1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`.""

This reverts commit f01c0ad1.

Reason for revert:
1. Fixed `CaptureScreenshotBeyondViewport_OutOfView` by proper rounding.
2. Fixed `CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown` on some platform by relaxing color tolerance.

Original change's description:
> Revert "Reland "Fix node screenshot. Part 1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`.""
>
> This reverts commit b8a64dcd.
>
> Reason for revert: CaptureScreenshotTest.CaptureScreenshotBeyondViewport_OutOfView is failing consistently on "Android FYI SkiaRenderer Vulkan (Pixel 2)"
>
> https://ci.chromium.org/p/chromium/builders/ci/Android%20FYI%20SkiaRenderer%20Vulkan%20%28Pixel%202%29/b8863213321775995088
>
> Original change's description:
> > Reland "Fix node screenshot. Part 1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`."
> >
> > This reverts commit 85d844e3.
> >
> > Reason for revert: reland initial commit with disabled flaky test.
> >
> > Original change's description:
> > > Revert "Fix node screenshot. Part 1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`."
> > >
> > > This reverts commit 114bc6ab.
> > >
> > > Reason for revert: CaptureScreenshotTest.CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown is failing consistently on linux-chromeos-rel:
> > >
> > > https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/42849
> > > https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/42850
> > >
> > > Original change's description:
> > > > Fix node screenshot. Part 1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`.
> > > >
> > > > 1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`.
> > > > 2. Use `WebSettingsImpl::SetMainFrameClipsContent(false)` to force the whole page to be rendered.
> > > > 3. Set `hide_scrollbars` to avoid artificial scrollbars.
> > > > 4. Set `record_whole_document` to force the whole page to be rendered.
> > > > 5. Removed DCHECK from `third_party/blink/renderer/core/frame/visual_viewport.cc`, as soon as it didn't expect to have preferences to be changed "on the flight".
> > > >
> > > > Details: https://bugs.chromium.org/p/chromium/issues/detail?id=1003629#c37
> > > >
> > > > Screenshots:
> > > >
> > > > - Before: https://i.imgur.com/yt6WZRx.png
> > > > - Patchset #1: https://i.imgur.com/VsocJ3L.png - artificial scrollbars.
> > > > - Patchset #3:
> > > >   * https://imgur.com/UVqpzUQ - line breaks are not exactly the same as in original view.
> > > >   * https://imgur.com/MXpgHOl
> > > >   * https://imgur.com/haxemcr - not aligned with the node.
> > > >   * https://imgur.com/RbdlYVT
> > > > - Patchset #18:
> > > >   * https://imgur.com/EWUmn0O
> > > >   * https://imgur.com/DUrQ1yF
> > > >   * https://imgur.com/cT5oBSi - perfectly aligned.
> > > >   * https://imgur.com/Jy4UWtf - no artificial scrollbars (because of the view extended).
> > > >
> > > > Bug: 1003629
> > > > Change-Id: I6bbc85cd0995626a8b1fb748ec9048c9d586200e
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470876
> > > > Commit-Queue: Maksim Sadym <sadym@chromium.org>
> > > > Auto-Submit: Maksim Sadym <sadym@chromium.org>
> > > > Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
> > > > Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> > > > Cr-Commit-Position: refs/heads/master@{#827440}
> > >
> > > TBR=dgozman@chromium.org,wangxianzhu@chromium.org,pdr@chromium.org,caseq@chromium.org,sigurds@chromium.org,sadym@chromium.org
> > >
> > > Change-Id: Iaa9b6ee9f5584adbb4a521808e2d201834a6a6dc
> > > No-Presubmit: true
> > > No-Tree-Checks: true
> > > No-Try: true
> > > Bug: 1003629
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2538543
> > > Reviewed-by: Scott Little <sclittle@chromium.org>
> > > Commit-Queue: Scott Little <sclittle@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#827505}
> >
> > TBR=dgozman@chromium.org,wangxianzhu@chromium.org,pdr@chromium.org,caseq@chromium.org,sclittle@chromium.org,sigurds@chromium.org,sadym@chromium.org
> >
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> >
> > Bug: 1003629
> > Change-Id: I2d394d77d0235762c61449ff8e90573de4c0c5d5
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545524
> > Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> > Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
> > Commit-Queue: Maksim Sadym <sadym@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#829147}
>
> TBR=wangxianzhu@chromium.org,caseq@chromium.org,sadym@chromium.org
>
> Change-Id: I2422b2560d10910cd90290c5647eb068743ac682
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1003629
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2550242
> Reviewed-by: Mingjing Zhang <mjzhang@chromium.org>
> Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#829230}

TBR=wangxianzhu@chromium.org,caseq@chromium.org,mjzhang@chromium.org,sadym@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 1003629
Change-Id: Ib6e44df890e6707d811fccf2c6a8817983123c9d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2551114
Commit-Queue: Maksim Sadym <sadym@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829636}

content/browser/devtools/protocol/devtools_protocol_browsertest.cc

@@ -326,7 +326,7 @@ std::unique_ptr<SkBitmap> DecodeJPEG(std::string base64_data) {
       jpeg_data.size());
 }
 
-bool ColorsMatchWithinLimit(SkColor color1, SkColor color2, int error_limit) {
+int ColorsSquareDiff(SkColor color1, SkColor color2) {
   auto a_diff = static_cast<int>(SkColorGetA(color1)) -
                 static_cast<int>(SkColorGetA(color2));
   auto r_diff = static_cast<int>(SkColorGetR(color1)) -
@@ -336,8 +336,11 @@ bool ColorsMatchWithinLimit(SkColor color1, SkColor color2, int error_limit) {
   auto b_diff = static_cast<int>(SkColorGetB(color1)) -
                 static_cast<int>(SkColorGetB(color2));
   return a_diff * a_diff + r_diff * r_diff + g_diff * g_diff +
-             b_diff * b_diff <=
-         error_limit * error_limit;
+             b_diff * b_diff;
+}
+
+bool ColorsMatchWithinLimit(SkColor color1, SkColor color2, int max_collor_diff) {
+  return ColorsSquareDiff(color1, color2) <= max_collor_diff * max_collor_diff;
 }
 
 // Adapted from cc::ExactPixelComparator.
@@ -345,7 +348,7 @@ bool MatchesBitmap(const SkBitmap& expected_bmp,
                    const SkBitmap& actual_bmp,
                    const gfx::Rect& matching_mask,
                    float device_scale_factor,
-                   int error_limit) {
+                   int max_collor_diff) {
   // Number of pixels with an error
   int error_pixels_count = 0;
 
@@ -355,10 +358,12 @@ bool MatchesBitmap(const SkBitmap& expected_bmp,
   device_scale_factor = device_scale_factor ? device_scale_factor : 1;
 
   // Check that bitmaps have identical dimensions.
-  EXPECT_EQ(expected_bmp.width() * device_scale_factor, actual_bmp.width());
-  EXPECT_EQ(expected_bmp.height() * device_scale_factor, actual_bmp.height());
-  if (expected_bmp.width() * device_scale_factor != actual_bmp.width() ||
-      expected_bmp.height() * device_scale_factor != actual_bmp.height()) {
+  int expected_width = round(expected_bmp.width() * device_scale_factor);
+  int expected_height = round(expected_bmp.height() * device_scale_factor);
+  EXPECT_EQ(expected_width, actual_bmp.width());
+  EXPECT_EQ(expected_height, actual_bmp.height());
+  if (expected_width != actual_bmp.width() ||
+      expected_height != actual_bmp.height()) {
     return false;
   }
 
@@ -369,10 +374,12 @@ bool MatchesBitmap(const SkBitmap& expected_bmp,
       SkColor actual_color =
           actual_bmp.getColor(x * device_scale_factor, y * device_scale_factor);
       SkColor expected_color = expected_bmp.getColor(x, y);
-      if (!ColorsMatchWithinLimit(actual_color, expected_color, error_limit)) {
+      if (!ColorsMatchWithinLimit(actual_color, expected_color, max_collor_diff)) {
         if (error_pixels_count < 10) {
-          LOG(ERROR) << "Pixel (" << x << "," << y << "): expected " << std::hex
-                     << expected_color << " actual " << actual_color;
+          LOG(ERROR) << "Pixel (" << x << "," << y
+                     << "). Expected: " << std::hex << expected_color
+                     << ", actual: " << actual_color << std::dec
+                     << ", square diff: " << ColorsSquareDiff(expected_color, actual_color);
         }
         error_pixels_count++;
         error_bounding_rect.Union(gfx::Rect(x, y, 1, 1));
@@ -398,11 +405,15 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
       ScreenshotEncoding encoding,
       bool from_surface,
       const gfx::RectF& clip = gfx::RectF(),
-      float clip_scale = 0) {
+      float clip_scale = 0,
+      bool capture_beyond_viewport = false) {
     std::unique_ptr<base::DictionaryValue> params(new base::DictionaryValue());
     params->SetString("format", encoding == ENCODING_PNG ? "png" : "jpeg");
     params->SetInteger("quality", 100);
     params->SetBoolean("fromSurface", from_surface);
+    if (capture_beyond_viewport) {
+      params->SetBoolean("captureBeyondViewport", true);
+    }
     if (clip_scale) {
       std::unique_ptr<base::DictionaryValue> clip_value(
           new base::DictionaryValue());
@@ -433,9 +444,10 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
                                      bool from_surface,
                                      float device_scale_factor = 0,
                                      const gfx::RectF& clip = gfx::RectF(),
-                                     float clip_scale = 0) {
-    std::unique_ptr<SkBitmap> result_bitmap =
-        CaptureScreenshot(encoding, from_surface, clip, clip_scale);
+                                     float clip_scale = 0,
+                                     bool capture_beyond_viewport = false) {
+    std::unique_ptr<SkBitmap> result_bitmap = CaptureScreenshot(
+        encoding, from_surface, clip, clip_scale, capture_beyond_viewport);
 
     gfx::Rect matching_mask(gfx::SkIRectToRect(expected_bitmap.bounds()));
 #if defined(OS_MAC)
@@ -449,10 +461,10 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
     // Allow some error between actual and expected pixel values.
     // That assumes there is no shift in pixel positions, so it only works
     // reliably if all pixels have equal values.
-    int error_limit = 16;
+    int max_collor_diff = 20;
 
     EXPECT_TRUE(MatchesBitmap(expected_bitmap, *result_bitmap, matching_mask,
-                              device_scale_factor, error_limit));
+                              device_scale_factor, max_collor_diff));
   }
 
   // Takes a screenshot of a colored box that is positioned inside the frame.
@@ -531,6 +543,96 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
 #endif
 };
 
+IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest,
+                       CaptureScreenshotBeyondViewport_OutOfView) {
+  // TODO(crbug.com/653637) This test fails consistently on low-end Android
+  // devices.
+  if (base::SysInfo::IsLowEndDevice())
+    return;
+
+  // Load dummy page before getting the window size.
+  shell()->LoadURL(GURL("data:text/html,"));
+  gfx::Size window_size =
+      static_cast<RenderWidgetHostViewBase*>(
+          shell()->web_contents()->GetRenderWidgetHostView())
+          ->GetCompositorViewportPixelSize();
+
+  // Make a page a bit bigger than the view to force scrollbars to be shown.
+  float content_height = window_size.height() + 10;
+  float content_width = window_size.width() + 10;
+
+  shell()->LoadURL(
+      GURL("data:text/html,<body "
+           "style='background:%23123456;height:" +
+           base::NumberToString(content_height) +
+           "px;width:" + base::NumberToString(content_width) + "px'></body>"));
+
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  Attach();
+
+  // Generate expected screenshot without any scrollbars.
+  SkBitmap expected_bitmap;
+  expected_bitmap.allocN32Pixels(content_width, content_height);
+  expected_bitmap.eraseColor(SkColorSetRGB(0x12, 0x34, 0x56));
+
+  float device_scale_factor =
+      display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();
+
+  // Verify there are no scrollbars on the screenshot.
+  CaptureScreenshotAndCompareTo(
+      expected_bitmap, ENCODING_PNG, true, device_scale_factor,
+      gfx::RectF(0, 0, content_width, content_height), 1, true);
+}
+
+// ChromeOS and Android has fading out scrollbars, which makes the test flacky.
+// TODO(crbug.com/1150059) Android has a problem with changing scale.
+// TODO(crbug.com/1147911) Android Lollipop has a problem with capturing
+// screenshot.
+#if defined(OS_ANDROID) || defined(OS_CHROMEOS)
+#define MAYBE_CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown \
+  DISABLED_CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown
+#else
+#define MAYBE_CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown \
+  CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown
+#endif
+IN_PROC_BROWSER_TEST_F(
+    CaptureScreenshotTest,
+    MAYBE_CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown) {
+  // TODO(crbug.com/653637) This test fails consistently on low-end Android
+  // devices.
+  if (base::SysInfo::IsLowEndDevice())
+    return;
+
+  shell()->LoadURL(GURL(
+      "data:text/html,<body><div style='width: 50px; height: 50px; overflow: "
+      "scroll;'><h3>test</h3><h3>test</h3><h3>test</h3></div></body>"));
+
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  Attach();
+
+  // We compare against the actual physical backing size rather than the
+  // view size, because the view size is stored adjusted for DPI and only in
+  // integer precision.
+  gfx::Size view_size = static_cast<RenderWidgetHostViewBase*>(
+                            shell()->web_contents()->GetRenderWidgetHostView())
+                            ->GetCompositorViewportPixelSize();
+
+  // Capture a screenshot not "form surface", meaning without emulation and
+  // without changing preferences, as-is.
+  std::unique_ptr<SkBitmap> expected_bitmap =
+      CaptureScreenshot(ENCODING_PNG, false);
+
+  float device_scale_factor =
+      display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();
+
+  // Compare the captured screenshot with one made "from_surface", where actual
+  // scrollbar magic happened, and verify it looks the same, meaning the
+  // internal scrollbars are rendered.
+  CaptureScreenshotAndCompareTo(
+      *expected_bitmap, ENCODING_PNG, true, device_scale_factor,
+      gfx::RectF(0, 0, view_size.width(), view_size.height()), 1, true);
+}
+
 IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest, CaptureScreenshot) {
   // This test fails consistently on low-end Android devices.
   // See crbug.com/653637.

content/browser/devtools/protocol/page_handler.cc

@@ -15,6 +15,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/optional.h"
 #include "base/process/process_handle.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string16.h"
@@ -681,6 +682,7 @@ void PageHandler::CaptureScreenshot(
     Maybe<int> quality,
     Maybe<Page::Viewport> clip,
     Maybe<bool> from_surface,
+    Maybe<bool> capture_beyond_viewport,
     std::unique_ptr<CaptureScreenshotCallback> callback) {
   if (!host_ || !host_->GetRenderWidgetHost() ||
       !host_->GetRenderWidgetHost()->GetView()) {
@@ -712,7 +714,8 @@ void PageHandler::CaptureScreenshot(
         base::BindOnce(&PageHandler::ScreenshotCaptured,
                        weak_factory_.GetWeakPtr(), std::move(callback),
                        screenshot_format, screenshot_quality, gfx::Size(),
-                       gfx::Size(), blink::DeviceEmulationParams()),
+                       gfx::Size(), blink::DeviceEmulationParams(),
+                       base::nullopt),
         false);
     return;
   }
@@ -775,6 +778,30 @@ void PageHandler::CaptureScreenshot(
     }
   }
 
+  base::Optional<blink::web_pref::WebPreferences> maybe_original_web_prefs;
+  if (capture_beyond_viewport.fromMaybe(false)) {
+    blink::web_pref::WebPreferences original_web_prefs =
+        host_->GetRenderViewHost()->GetDelegate()->GetOrCreateWebPreferences();
+    maybe_original_web_prefs = original_web_prefs;
+
+    blink::web_pref::WebPreferences modified_web_prefs = original_web_prefs;
+    // Hiding scrollbar is needed to avoid scrollbar artefacts on the
+    // screenshot. Details: https://crbug.com/1003629.
+    modified_web_prefs.hide_scrollbars = true;
+    modified_web_prefs.record_whole_document = true;
+    host_->GetRenderViewHost()->GetDelegate()->SetWebPreferences(
+        modified_web_prefs);
+
+    {
+      // TODO(crbug.com/1141835): Remove the bug is fixed.
+      // Walkaround for the bug. Emulated `view_size` has to be set twice,
+      // otherwise the scrollbar will be on the screenshot present.
+      blink::DeviceEmulationParams tmp_params = modified_params;
+      tmp_params.view_size = gfx::Size(1, 1);
+      emulation_handler_->SetDeviceEmulationParams(tmp_params);
+    }
+  }
+
   // We use DeviceEmulationParams to either emulate, set viewport or both.
   emulation_handler_->SetDeviceEmulationParams(modified_params);
 
@@ -807,7 +834,8 @@ void PageHandler::CaptureScreenshot(
       base::BindOnce(&PageHandler::ScreenshotCaptured,
                      weak_factory_.GetWeakPtr(), std::move(callback),
                      screenshot_format, screenshot_quality, original_view_size,
-                     requested_image_size, original_params),
+                     requested_image_size, original_params,
+                     maybe_original_web_prefs),
       true);
 }
 
@@ -1110,6 +1138,8 @@ void PageHandler::ScreenshotCaptured(
     const gfx::Size& original_view_size,
     const gfx::Size& requested_image_size,
     const blink::DeviceEmulationParams& original_emulation_params,
+    const base::Optional<blink::web_pref::WebPreferences>&
+        maybe_original_web_prefs,
     const gfx::Image& image) {
   if (original_view_size.width()) {
     RenderWidgetHostImpl* widget_host = host_->GetRenderWidgetHost();
@@ -1117,6 +1147,11 @@ void PageHandler::ScreenshotCaptured(
     emulation_handler_->SetDeviceEmulationParams(original_emulation_params);
   }
 
+  if (maybe_original_web_prefs) {
+    host_->GetRenderViewHost()->GetDelegate()->SetWebPreferences(
+        maybe_original_web_prefs.value());
+  }
+
   if (image.IsEmpty()) {
     callback->sendFailure(
         Response::ServerError("Unable to capture screenshot"));

content/browser/devtools/protocol/page_handler.h

@@ -126,6 +126,7 @@ class PageHandler : public DevToolsDomainHandler,
       Maybe<int> quality,
       Maybe<Page::Viewport> clip,
       Maybe<bool> from_surface,
+      Maybe<bool> capture_beyond_viewport,
       std::unique_ptr<CaptureScreenshotCallback> callback) override;
   void CaptureSnapshot(
       Maybe<std::string> format,
@@ -187,13 +188,15 @@ class PageHandler : public DevToolsDomainHandler,
       std::unique_ptr<Page::ScreencastFrameMetadata> metadata,
       const protocol::Binary& data);
 
-  void ScreenshotCaptured(std::unique_ptr<CaptureScreenshotCallback> callback,
-                          const std::string& format,
-                          int quality,
-                          const gfx::Size& original_view_size,
-                          const gfx::Size& requested_image_size,
-                          const blink::DeviceEmulationParams& original_params,
-                          const gfx::Image& image);
+  void ScreenshotCaptured(
+      std::unique_ptr<CaptureScreenshotCallback> callback,
+      const std::string& format,
+      int quality,
+      const gfx::Size& original_view_size,
+      const gfx::Size& requested_image_size,
+      const blink::DeviceEmulationParams& original_params,
+      const base::Optional<blink::web_pref::WebPreferences>& original_web_prefs,
+      const gfx::Image& image);
 
   void GotManifest(std::unique_ptr<GetAppManifestCallback> callback,
                    const GURL& manifest_url,

third_party/blink/public/devtools_protocol/browser_protocol.pdl

@@ -6342,6 +6342,8 @@ domain Page
       optional Viewport clip
       # Capture the screenshot from the surface, rather than the view. Defaults to true.
       experimental optional boolean fromSurface
+      # Capture the screenshot beyond the viewport. Defaults to false.
+      experimental optional boolean captureBeyondViewport
     returns
       # Base64-encoded image data.
       binary data

third_party/blink/renderer/core/frame/visual_viewport.cc

@@ -599,15 +599,12 @@ void VisualViewport::InitializeScrollbars() {
 
   needs_paint_property_update_ = true;
 
+  scrollbar_layer_horizontal_ = nullptr;
+  scrollbar_layer_vertical_ = nullptr;
   if (VisualViewportSuppliesScrollbars() &&
       !GetPage().GetSettings().GetHideScrollbars()) {
-    DCHECK(!scrollbar_layer_horizontal_);
-    DCHECK(!scrollbar_layer_vertical_);
     UpdateScrollbarLayer(kHorizontalScrollbar);
     UpdateScrollbarLayer(kVerticalScrollbar);
-  } else {
-    scrollbar_layer_horizontal_ = nullptr;
-    scrollbar_layer_vertical_ = nullptr;
   }
 
   // Ensure existing LocalFrameView scrollbars are removed if the visual