Reland "Reland "cros: Implement ClipboardImageModelFactory""

This reverts commit 1d850b0b.

Reason for revert: this CL didn't cause the problem. The root cause was a tricky infra problem. sorry for wrong revert.

Original change's description:
> Revert "Reland "cros: Implement ClipboardImageModelFactory""
> 
> This reverts commit 57af2c42.
> 
> Reason for revert: No good reason, but this build causes a compilation error. No clear culprit. https://ci.chromium.org/p/chromium/builders/ci/Mac%20ASan%2064%20Builder/104190
> 
> Original change's description:
> > Reland "cros: Implement ClipboardImageModelFactory"
> > 
> > This reverts commit 5822a03a.
> > 
> > Reason for revert:
> > It turned out that the test is flaky regardless of this change.
> > Probably this just made it easier to reproduce the flakiness.
> > 
> > Original change's description:
> > > Revert "cros: Implement ClipboardImageModelFactory"
> > > 
> > > This reverts commit 056d2885.
> > > 
> > > Reason for revert:
> > > This is a speculative revert for failures on 
> > > https://ci.chromium.org/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/38382
> > > 
> > > This could fail the following test:
> > > ClipboardHistoryWithMultiProfileBrowserTest.ShouldPasteHistoryAsPlainText
> > > 
> > > Original change's description:
> > > > cros: Implement ClipboardImageModelFactory
> > > > 
> > > > Implements ClipboardImageModelFactoryImpl.
> > > > Previous CL landed ClipboardImageModelFactory, and all
> > > > ash-side usages of the factory.
> > > > 
> > > > This CL implements rendering of HTML via
> > > > ClipboardImageModelRequest.
> > > > 
> > > > Bug: 1117230
> > > > Change-Id: I2f7ac67eadac5ef1a3c7cfca9918de438f02ff8e
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2367395
> > > > Commit-Queue: Alex Newcomer <newcomer@chromium.org>
> > > > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> > > > Auto-Submit: Alex Newcomer <newcomer@chromium.org>
> > > > Cr-Commit-Position: refs/heads/master@{#802779}
> > > 
> > > TBR=xiyuan@chromium.org,newcomer@chromium.org
> > > 
> > > # Not skipping CQ checks because original CL landed > 1 day ago.
> > > 
> > > Bug: 1117230
> > > Change-Id: Ibc178e2d0b50bb423bc5a128aead30a5f471bd18
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2383532
> > > Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> > > Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#803003}
> > 
> > TBR=xiyuan@chromium.org,nhiroki@chromium.org,newcomer@chromium.org
> > 
> > # Not skipping CQ checks because this is a reland.
> > 
> > Bug: 1117230
> > Change-Id: I49fe530783a23a65e06456ca3c4ab3703dde35e2
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2384712
> > Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> > Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#803027}
> 
> TBR=xiyuan@chromium.org,nhiroki@chromium.org,newcomer@chromium.org
> 
> Change-Id: Iad7614b249091bfcce8a27f10ae4e67de86a2a29
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1117230
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2382603
> Reviewed-by: Maxim Kolosovskiy  <kolos@chromium.org>
> Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#803043}

TBR=xiyuan@chromium.org,nhiroki@chromium.org,kolos@chromium.org,newcomer@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 1117230
Change-Id: I410473bc38efce068f1c3d2a2158489ff4eb4f17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2382122Reviewed-by: Maxim Kolosovskiy  <kolos@chromium.org>
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803080}

ash/clipboard/clipboard_history_menu_model_adapter.cc

@@ -6,6 +6,7 @@
 
 #include "ash/clipboard/clipboard_history_controller.h"
 #include "ash/clipboard/views/clipboard_history_item_view.h"
+#include "ash/public/cpp/clipboard_image_model_factory.h"
 #include "ash/shell.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/base/ui_base_types.h"
@@ -27,6 +28,9 @@ void ClipboardHistoryMenuModelAdapter::Run(const gfx::Rect& anchor_rect) {
   DCHECK(!root_view_);
   DCHECK(model_);
 
+  // Start async rendering of HTML, if any exists.
+  ClipboardImageModelFactory::Get()->Activate();
+
   root_view_ = CreateMenu();
   menu_runner_ = std::make_unique<views::MenuRunner>(
       root_view_, views::MenuRunner::CONTEXT_MENU |
@@ -60,6 +64,10 @@ gfx::Rect ClipboardHistoryMenuModelAdapter::GetMenuBoundsInScreenForTest()
   return root_view_->GetSubmenu()->GetBoundsInScreen();
 }
 
+void ClipboardHistoryMenuModelAdapter::OnMenuClosed(views::MenuItemView* menu) {
+  ClipboardImageModelFactory::Get()->Deactivate();
+}
+
 views::MenuItemView* ClipboardHistoryMenuModelAdapter::AppendMenuItem(
     views::MenuItemView* menu,
     ui::MenuModel* model,

ash/clipboard/clipboard_history_menu_model_adapter.h

@@ -53,6 +53,9 @@ class ClipboardHistoryMenuModelAdapter : views::MenuModelAdapter {
   // Returns menu bounds in screen coordinates.
   gfx::Rect GetMenuBoundsInScreenForTest() const;
 
+  // views::MenuModelAdapter:
+  void OnMenuClosed(views::MenuItemView* menu) override;
+
  private:
   // views::MenuModelAdapter:
   views::MenuItemView* AppendMenuItem(views::MenuItemView* menu,

ash/clipboard/clipboard_history_resource_manager.cc

@@ -71,8 +71,8 @@ ClipboardHistoryResourceManager::ClipboardHistoryResourceManager(
 
 ClipboardHistoryResourceManager::~ClipboardHistoryResourceManager() {
   clipboard_history_->RemoveObserver(this);
-
-  CancelUnfinishedRequests();
+  if (ClipboardImageModelFactory::Get())
+    ClipboardImageModelFactory::Get()->OnShutdown();
 }
 
 ui::ImageModel ClipboardHistoryResourceManager::GetImageModel(

ash/clipboard/clipboard_history_resource_manager_unittest.cc

@@ -75,6 +75,7 @@ class MockClipboardImageModelFactory : public ClipboardImageModelFactory {
   MOCK_METHOD(void, CancelRequest, (const base::UnguessableToken&), (override));
   MOCK_METHOD(void, Activate, (), (override));
   MOCK_METHOD(void, Deactivate, (), (override));
+  void OnShutdown() override {}
 };
 
 class ClipboardHistoryResourceManagerTest : public AshTestBase {
@@ -290,19 +291,4 @@ TEST_F(ClipboardHistoryResourceManagerTest, IneligibleItem) {
   EXPECT_EQ(2u, clipboard_history()->GetItems().size());
 }
 
-// Tests that incomplete requests are canceled when the item corresponding with
-// the request is forgotten by ClipboardHistory.
-TEST_F(ClipboardHistoryResourceManagerTest, IncompleteRequestCanceled) {
-  EXPECT_CALL(*mock_image_factory(), Render).Times(1);
-  EXPECT_CALL(*mock_image_factory(), CancelRequest).Times(1);
-  // Because we do not provide an ON_CALL for MockClipboardImageModelFactory,
-  // Render will do nothing. This simulates an incomplete request from the
-  // perspective of ClipboardHistoryResourceManager.
-  {
-    ui::ScopedClipboardWriter scw(ui::ClipboardBuffer::kCopyPaste);
-    scw.WriteHTML(base::UTF8ToUTF16("test"), "source_url");
-  }
-  FlushMessageLoop();
-}
-
 }  // namespace ash

ash/public/cpp/clipboard_image_model_factory.h

@@ -47,6 +47,9 @@ class ASH_PUBLIC_EXPORT ClipboardImageModelFactory {
   // Called after Activate() to pause rendering requests.
   virtual void Deactivate() = 0;
 
+  // Called during shutdown to cleanup references to Profile.
+  virtual void OnShutdown() = 0;
+
  protected:
   ClipboardImageModelFactory();
   virtual ~ClipboardImageModelFactory();

chrome/browser/ui/BUILD.gn

@@ -1864,6 +1864,8 @@ static_library("ui") {
       "ash/chrome_shell_delegate.h",
       "ash/clipboard_image_model_factory_impl.cc",
       "ash/clipboard_image_model_factory_impl.h",
+      "ash/clipboard_image_model_request.cc",
+      "ash/clipboard_image_model_request.h",
       "ash/clipboard_util.cc",
       "ash/clipboard_util.h",
       "ash/holding_space/holding_space_client_impl.cc",

chrome/browser/ui/ash/clipboard_image_model_factory_impl.cc

@@ -8,7 +8,11 @@
 
 ClipboardImageModelFactoryImpl::ClipboardImageModelFactoryImpl(
     Profile* primary_profile)
-    : primary_profile_(primary_profile) {
+    : primary_profile_(primary_profile),
+      idle_timer_(FROM_HERE,
+                  base::TimeDelta::FromMinutes(2),
+                  this,
+                  &ClipboardImageModelFactoryImpl::OnRequestIdle) {
   DCHECK(primary_profile_);
 }
 
@@ -17,12 +21,71 @@ ClipboardImageModelFactoryImpl::~ClipboardImageModelFactoryImpl() = default;
 void ClipboardImageModelFactoryImpl::Render(const base::UnguessableToken& id,
                                             const std::string& html_markup,
                                             ImageModelCallback callback) {
-  std::move(callback).Run(ui::ImageModel());
+  DCHECK(!html_markup.empty());
+  pending_list_.emplace_front(id, html_markup, std::move(callback));
+  StartNextRequest();
 }
 
 void ClipboardImageModelFactoryImpl::CancelRequest(
-    const base::UnguessableToken& id) {}
+    const base::UnguessableToken& id) {
+  if (request_ && request_->IsRunningRequest(id)) {
+    request_->Stop();
+    return;
+  }
 
-void ClipboardImageModelFactoryImpl::Activate() {}
+  auto iter =
+      std::find_if(pending_list_.begin(), pending_list_.end(),
+                   [&id](const ClipboardImageModelRequest::Params& params) {
+                     return id == params.id;
+                   });
+  if (iter == pending_list_.end())
+    return;
 
-void ClipboardImageModelFactoryImpl::Deactivate() {}
+  pending_list_.erase(iter);
+}
+
+void ClipboardImageModelFactoryImpl::Activate() {
+  active_ = true;
+  StartNextRequest();
+}
+
+void ClipboardImageModelFactoryImpl::Deactivate() {
+  // Prevent new requests from being ran, but do not stop |request_| if it is
+  // running. |request_| will be cleaned up OnRequestIdle().
+  active_ = false;
+}
+
+void ClipboardImageModelFactoryImpl::OnShutdown() {
+  // Reset |request_| to drop its reference to Profile, specifically the
+  // RenderProcessHost of its WebContents.
+  request_.reset();
+}
+
+void ClipboardImageModelFactoryImpl::StartNextRequest() {
+  if (pending_list_.empty() || !active_ ||
+      (request_ && request_->IsRunningRequest())) {
+    return;
+  }
+
+  if (!request_) {
+    request_ = std::make_unique<ClipboardImageModelRequest>(
+        primary_profile_,
+        base::BindRepeating(&ClipboardImageModelFactoryImpl::StartNextRequest,
+                            weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  // Reset the timer that cleans up |request_|. If StartNextRequest() is not
+  // called again in 2 minutes, |request_| will be reset.
+  idle_timer_.Reset();
+  request_->Start(std::move(pending_list_.front()));
+  pending_list_.pop_front();
+}
+
+void ClipboardImageModelFactoryImpl::OnRequestIdle() {
+  if (!request_)
+    return;
+
+  DCHECK(!request_->IsRunningRequest())
+      << "Running requests should timeout or complete before being cleaned up.";
+  request_.reset();
+}

chrome/browser/ui/ash/clipboard_image_model_factory_impl.h

@@ -8,7 +8,10 @@
 #include <string>
 
 #include "ash/public/cpp/clipboard_image_model_factory.h"
+#include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
 #include "base/unguessable_token.h"
+#include "chrome/browser/ui/ash/clipboard_image_model_request.h"
 
 class Profile;
 
@@ -29,10 +32,33 @@ class ClipboardImageModelFactoryImpl : public ash::ClipboardImageModelFactory {
   void CancelRequest(const base::UnguessableToken& id) override;
   void Activate() override;
   void Deactivate() override;
+  void OnShutdown() override;
+
+  // Starts the first request in |pending_list_|.
+  void StartNextRequest();
+
+  // Called when |request_| has been idle for 2 minutes, to clean up resources.
+  void OnRequestIdle();
 
   // The primary profile, used instead of the active profile to create the
   // WebContents that renders html.
   Profile* const primary_profile_;
+
+  // Whether ClipboardImageModelFactoryImpl is activated. If not, requests are
+  // queued until Activate().
+  bool active_ = false;
+
+  // Requests which are waiting to be run.
+  std::list<ClipboardImageModelRequest::Params> pending_list_;
+
+  // The active request. Expensive to keep in memory and expensive to create,
+  // deleted OnRequestIdle.
+  std::unique_ptr<ClipboardImageModelRequest> request_;
+
+  // Timer used to clean up |request_| if it is not used for 2 minutes.
+  base::DelayTimer idle_timer_;
+
+  base::WeakPtrFactory<ClipboardImageModelFactoryImpl> weak_ptr_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_UI_ASH_CLIPBOARD_IMAGE_MODEL_FACTORY_IMPL_H_

chrome/browser/ui/ash/clipboard_image_model_request.cc

+// Copyright 2020 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 "chrome/browser/ui/ash/clipboard_image_model_request.h"
+
+#include <memory>
+
+#include "base/base64.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_widget_host.h"
+#include "content/public/browser/render_widget_host_view.h"
+#include "content/public/browser/web_contents.h"
+#include "ui/aura/window.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/views/controls/webview/webview.h"
+#include "ui/views/layout/fill_layout.h"
+#include "ui/views/widget/widget.h"
+#include "url/gurl.h"
+
+namespace {
+
+// Size of the bitmap saved.
+constexpr gfx::Size kBitmapFinalSize(48, 48);
+// Size of the WebContents used to render HTML.
+constexpr gfx::Rect kWebContentsBounds(100, 100);
+
+}  // namespace
+
+ClipboardImageModelRequest::Params::Params(const base::UnguessableToken& id,
+                                           const std::string& html_markup,
+                                           ImageModelCallback callback)
+    : id(id), html_markup(html_markup), callback(std::move(callback)) {}
+
+ClipboardImageModelRequest::Params::Params(Params&&) = default;
+
+ClipboardImageModelRequest::Params&
+ClipboardImageModelRequest::Params::operator=(Params&&) = default;
+
+ClipboardImageModelRequest::Params::~Params() = default;
+
+ClipboardImageModelRequest::ClipboardImageModelRequest(
+    Profile* profile,
+    base::RepeatingClosure on_request_finished_callback)
+    : widget_(std::make_unique<views::Widget>()),
+      web_view_(new views::WebView(profile)),
+      on_request_finished_callback_(std::move(on_request_finished_callback)) {
+  views::Widget::InitParams widget_params;
+  widget_params.type = views::Widget::InitParams::TYPE_WINDOW_FRAMELESS;
+  widget_params.ownership =
+      views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  widget_params.name = "ClipboardImageModelRequest";
+  widget_->Init(std::move(widget_params));
+  widget_->SetContentsView(web_view_);
+  content::WebContents* web_contents = web_view_->GetWebContents();
+  Observe(web_contents);
+  // TODO(newcomer): Large items show a scrollbar, and small items do not need
+  // this much room. Size the WebContents based on the required bounds.
+  web_contents->GetNativeView()->SetBounds(kWebContentsBounds);
+}
+
+ClipboardImageModelRequest::~ClipboardImageModelRequest() = default;
+
+void ClipboardImageModelRequest::Start(Params&& params) {
+  DCHECK(!deliver_image_model_callback_);
+  DCHECK(params.callback);
+  DCHECK_EQ(base::UnguessableToken(), request_id_);
+
+  request_id_ = std::move(params.id);
+  deliver_image_model_callback_ = std::move(params.callback);
+
+  timeout_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(5), this,
+                       &ClipboardImageModelRequest::OnTimeout);
+  std::string encoded_html;
+  // Hide overflow to prevent scroll bars from showing up, which occurs when the
+  // rendered HTML takes up more space than 'kWebContentsBounds'.
+  params.html_markup.append("<style>body{overflow:hidden;}</style>");
+  base::Base64Encode(params.html_markup, &encoded_html);
+  constexpr char kDataURIPrefix[] = "data:text/html;base64,";
+  web_view_->GetWebContents()->GetController().LoadURLWithParams(
+      content::NavigationController::LoadURLParams(
+          GURL(kDataURIPrefix + encoded_html)));
+  widget_->ShowInactive();
+}
+
+void ClipboardImageModelRequest::Stop() {
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  timeout_timer_.Stop();
+  widget_->Hide();
+  deliver_image_model_callback_.Reset();
+  request_id_ = base::UnguessableToken();
+  on_request_finished_callback_.Run();
+}
+
+bool ClipboardImageModelRequest::IsRunningRequest(
+    base::Optional<base::UnguessableToken> request_id) const {
+  return request_id.has_value() ? *request_id == request_id_
+                                : !request_id_.is_empty();
+}
+
+void ClipboardImageModelRequest::DidStopLoading() {
+  content::RenderWidgetHostView* source_view =
+      web_view_->GetWebContents()->GetRenderViewHost()->GetWidget()->GetView();
+  gfx::Size source_size = source_view->GetViewBounds().size();
+  if (source_size.IsEmpty()) {
+    Stop();
+    return;
+  }
+
+  // There is no guarantee CopyFromSurface will call OnCopyComplete. If this
+  // takes too long, this will be cleaned up by |timeout_timer_|.
+  source_view->CopyFromSurface(
+      gfx::Rect(source_size), kBitmapFinalSize,
+      base::BindOnce(&ClipboardImageModelRequest::OnCopyComplete,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ClipboardImageModelRequest::OnCopyComplete(const SkBitmap& bitmap) {
+  std::move(deliver_image_model_callback_)
+      .Run(ui::ImageModel::FromImageSkia(
+          gfx::ImageSkia::CreateFrom1xBitmap(bitmap)));
+  Stop();
+}
+
+void ClipboardImageModelRequest::OnTimeout() {
+  DCHECK(deliver_image_model_callback_);
+  Stop();
+}

chrome/browser/ui/ash/clipboard_image_model_request.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_UI_ASH_CLIPBOARD_IMAGE_MODEL_REQUEST_H_
+#define CHROME_BROWSER_UI_ASH_CLIPBOARD_IMAGE_MODEL_REQUEST_H_
+
+#include <memory>
+#include <string>
+
+#include "base/optional.h"
+#include "base/timer/timer.h"
+#include "base/unguessable_token.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "ui/base/models/image_model.h"
+
+namespace views {
+class WebView;
+class Widget;
+}  // namespace views
+
+class Profile;
+
+// Renders html in an off-screen WebView, copies the rendered surface, and
+// passes the copy through |deliver_image_model_callback_|. If the request takes
+// takes more than 5s to load, timeout is declared and the callback is not
+// called. If the request is Stop()-ed, the callback is not called.
+class ClipboardImageModelRequest : public content::WebContentsObserver {
+ public:
+  using ImageModelCallback = base::OnceCallback<void(ui::ImageModel)>;
+
+  struct Params {
+    Params() = delete;
+    Params(const base::UnguessableToken& id,
+           const std::string& html_markup,
+           ImageModelCallback request_finished_callback);
+    Params(Params&&);
+    Params& operator=(Params&&);
+    ~Params();
+
+    // A unique identifier, used to cancel running requests.
+    base::UnguessableToken id;
+    // Markup being rendered.
+    std::string html_markup;
+    // The callback to return the results of the request. Not called if the
+    // request is stopped via Stop(), or if timeout occurs.
+    ImageModelCallback callback;
+  };
+
+  ClipboardImageModelRequest(
+      Profile* profile,
+      base::RepeatingClosure on_request_finished_callback);
+  ClipboardImageModelRequest(const ClipboardImageModelRequest&) = delete;
+  ClipboardImageModelRequest operator=(const ClipboardImageModelRequest&) =
+      delete;
+  ~ClipboardImageModelRequest() override;
+
+  // Renders the HTML in a WebView and attempts to copy the surface. If this
+  // fails to load after 5 seconds, OnTimeout is called.
+  void Start(Params&& params);
+
+  // Stops the request and resets state. |web_view_| is still alive to
+  // enable fast restarting of the request.
+  void Stop();
+
+  // Returns whether a request with |request_id| is running, or if any request
+  // is running if no |request_id| is supplied.
+  bool IsRunningRequest(
+      base::Optional<base::UnguessableToken> request_id = base::nullopt) const;
+
+  // content::WebContentsObserver:
+  void DidStopLoading() override;
+
+ private:
+  // Callback called when the rendered surface is done being copied.
+  void OnCopyComplete(const SkBitmap& bitmap);
+
+  // Called when the running request takes too long to complete.
+  void OnTimeout();
+
+  // A Widget that is not shown, but forces |web_view_| to render.
+  std::unique_ptr<views::Widget> const widget_;
+
+  // Contents view of |widget_|. Owned by |widget_|.
+  views::WebView* const web_view_;
+
+  // Unique identifier for this request run. Empty when there are no running
+  // requests.
+  base::UnguessableToken request_id_;
+
+  // Callback used to deliver the rendered ImageModel.
+  ImageModelCallback deliver_image_model_callback_;
+
+  // Callback called when this request finishes (via timeout or completion).
+  base::RepeatingClosure on_request_finished_callback_;
+
+  // Timer used to abort requests which take longer than 5s to load.
+  base::RepeatingTimer timeout_timer_;
+
+  base::WeakPtrFactory<ClipboardImageModelRequest> weak_ptr_factory_{this};
+};
+
+#endif  // CHROME_BROWSER_UI_ASH_CLIPBOARD_IMAGE_MODEL_REQUEST_H_