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/+/2382603Reviewed-by: Maxim Kolosovskiy  <kolos@chromium.org>
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803043}

ash/clipboard/clipboard_history_menu_model_adapter.cc

@@ -6,7 +6,6 @@
 
 #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"
@@ -28,9 +27,6 @@ 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 |
@@ -64,10 +60,6 @@ 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,9 +53,6 @@ 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);
-  if (ClipboardImageModelFactory::Get())
-    ClipboardImageModelFactory::Get()->OnShutdown();
+
+  CancelUnfinishedRequests();
 }
 
 ui::ImageModel ClipboardHistoryResourceManager::GetImageModel(

ash/clipboard/clipboard_history_resource_manager_unittest.cc

@@ -75,7 +75,6 @@ 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 {
@@ -291,4 +290,19 @@ 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,9 +47,6 @@ 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,8 +1864,6 @@ 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,11 +8,7 @@
 
 ClipboardImageModelFactoryImpl::ClipboardImageModelFactoryImpl(
     Profile* primary_profile)
-    : primary_profile_(primary_profile),
-      idle_timer_(FROM_HERE,
-                  base::TimeDelta::FromMinutes(2),
-                  this,
-                  &ClipboardImageModelFactoryImpl::OnRequestIdle) {
+    : primary_profile_(primary_profile) {
   DCHECK(primary_profile_);
 }
 
@@ -21,71 +17,12 @@ ClipboardImageModelFactoryImpl::~ClipboardImageModelFactoryImpl() = default;
 void ClipboardImageModelFactoryImpl::Render(const base::UnguessableToken& id,
                                             const std::string& html_markup,
                                             ImageModelCallback callback) {
-  DCHECK(!html_markup.empty());
-  pending_list_.emplace_front(id, html_markup, std::move(callback));
-  StartNextRequest();
+  std::move(callback).Run(ui::ImageModel());
 }
 
 void ClipboardImageModelFactoryImpl::CancelRequest(
-    const base::UnguessableToken& id) {
-  if (request_ && request_->IsRunningRequest(id)) {
-    request_->Stop();
-    return;
-  }
+    const base::UnguessableToken& id) {}
 
-  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::Activate() {}
 
-  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();
-}
+void ClipboardImageModelFactoryImpl::Deactivate() {}

chrome/browser/ui/ash/clipboard_image_model_factory_impl.h

@@ -8,10 +8,7 @@
 #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;
 
@@ -32,33 +29,10 @@ 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_