PaymentHandler: Refactor PaymentInstrumentIconFetcher class.

The current PaymentInstrumentIconFetcher class is too complicated and
can not ensure thread-safety due to misuse some APIs. So, this patch
refactors the class to use ManifestIconDownloader internally. After
this patch, we can remove much duplicated codes and resolve some
crashes in debug mode as well.

Bug: 752836
Change-Id: I04c01d433d093ae138148e6aa2849bc1713c1c85
Reviewed-on: https://chromium-review.googlesource.com/658201
Commit-Queue: Jinho Bang <jinho.bang@samsung.com>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501589}

content/browser/payments/payment_app_database.cc

@@ -17,6 +17,7 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/common/manifest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/image/image.h"
 #include "url/gurl.h"
@@ -194,15 +195,26 @@ void PaymentAppDatabase::WritePaymentInstrument(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (instrument->icons.size() > 0) {
-    std::unique_ptr<PaymentInstrumentIconFetcher> icon_fetcher =
-        base::MakeUnique<PaymentInstrumentIconFetcher>();
-    icon_fetcher->Start(
-        instrument->icons, service_worker_context_,
+    std::vector<Manifest::Icon> icons;
+    for (const auto& image_object : instrument->icons) {
+      Manifest::Icon icon;
+      icon.src = image_object->src;
+      // TODO(zino): We should pass an actual image size comes from the
+      // renderer, but we just use gfx::Size(0, 0) for now until the feature
+      // is implemented. It means "any" size.
+      // Please see https://crbug.com/763886
+      icon.sizes.emplace_back(gfx::Size());
+      icon.purpose.emplace_back(Manifest::Icon::IconPurpose::ANY);
+      icons.emplace_back(icon);
+    }
+
+    PaymentInstrumentIconFetcher::Start(
+        scope, service_worker_context_->GetProviderHostIds(scope.GetOrigin()),
+        std::move(icons),
         base::BindOnce(&PaymentAppDatabase::DidFetchedPaymentInstrumentIcon,
                        weak_ptr_factory_.GetWeakPtr(), scope, instrument_key,
                        base::Passed(std::move(instrument)),
-                       base::Passed(std::move(callback)),
-                       base::Passed(std::move(icon_fetcher))));
+                       base::Passed(std::move(callback))));
   } else {
     service_worker_context_->FindReadyRegistrationForPattern(
         scope,
@@ -219,11 +231,9 @@ void PaymentAppDatabase::DidFetchedPaymentInstrumentIcon(
     const std::string& instrument_key,
     payments::mojom::PaymentInstrumentPtr instrument,
     WritePaymentInstrumentCallback callback,
-    std::unique_ptr<PaymentInstrumentIconFetcher> icon_fetcher,
     const std::string& icon) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  icon_fetcher.reset();
   if (icon.empty()) {
     std::move(callback).Run(PaymentHandlerStatus::FETCH_INSTRUMENT_ICON_FAILED);
     return;

content/browser/payments/payment_app_database.h

@@ -165,7 +165,6 @@ class CONTENT_EXPORT PaymentAppDatabase {
       const std::string& instrument_key,
       payments::mojom::PaymentInstrumentPtr instrument,
       WritePaymentInstrumentCallback callback,
-      std::unique_ptr<PaymentInstrumentIconFetcher> fetcher,
       const std::string& icon);
 
   // ClearPaymentInstruments callbacks

content/browser/payments/payment_instrument_icon_fetcher.cc

@@ -6,160 +6,145 @@
 
 #include "base/base64.h"
 #include "base/bind_helpers.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/storage_partition_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/common/service_manager_connection.h"
-#include "net/base/load_flags.h"
-#include "net/http/http_status_code.h"
-#include "net/traffic_annotation/network_traffic_annotation.h"
-#include "services/data_decoder/public/cpp/decode_image.h"
-#include "services/service_manager/public/cpp/connector.h"
+#include "content/public/browser/manifest_icon_downloader.h"
+#include "content/public/browser/manifest_icon_selector.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image.h"
 
 namespace content {
-
 namespace {
 
-net::NetworkTrafficAnnotationTag g_traffic_annotation =
-    net::DefineNetworkTrafficAnnotation("payment_instrument_icon_fetcher", R"(
-        semantics {
-          sender: "Web Payments"
-          description:
-            "Chromium downloads payment instrument icons when registering "
-            "payment instruments."
-          trigger:
-            "When user navigates to a website to register web payment apps."
-          data:
-            "URL of the required icon to fetch. No user information is sent."
-          destination: WEBSITE
-        }
-        policy {
-          cookies_allowed: NO
-          setting:
-            "This feature cannot be disabled in settings. Users can refuse to "
-            "install web payment apps."
-          policy_exception_justification: "Not implemented."
-        })");
-
-}  // namespace
-
-PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcher()
-    : checking_image_object_index_(0), weak_ptr_factory_(this) {}
-PaymentInstrumentIconFetcher::~PaymentInstrumentIconFetcher() {}
-
-void PaymentInstrumentIconFetcher::Start(
-    const std::vector<payments::mojom::ImageObjectPtr>& image_objects,
-    scoped_refptr<ServiceWorkerContextWrapper> service_worker_context,
-    PaymentInstrumentIconFetcherCallback callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  for (const auto& obj : image_objects) {
-    image_objects_.emplace_back(payments::mojom::ImageObject::New(obj->src));
-  }
-  DCHECK_GT(image_objects_.size(), 0U);
-
-  callback_ = std::move(callback);
-
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&PaymentInstrumentIconFetcher::StartFromUIThread,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     std::move(service_worker_context)));
-}
-
-void PaymentInstrumentIconFetcher::StartFromUIThread(
-    scoped_refptr<ServiceWorkerContextWrapper> service_worker_context) {
+// TODO(zino): Choose appropriate icon size dynamically on different platforms.
+// Here we choose a large ideal icon size to be big enough for all platforms.
+// Note that we only scale down for this icon size but not scale up.
+// Please see: https://crbug.com/763886
+const int kPaymentAppIdealIconSize = 0xFFFF;
+const int kPaymentAppMinimumIconSize = 0;
+
+void DownloadBestMatchingIcon(
+    WebContents* web_contents,
+    const std::vector<Manifest::Icon>& icons,
+    PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcherCallback
+        callback);
+
+void OnIconFetched(
+    WebContents* web_contents,
+    const std::vector<Manifest::Icon>& icons,
+    PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcherCallback callback,
+    const SkBitmap& bitmap) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  url_request_context_getter_ =
-      service_worker_context->storage_partition()->GetURLRequestContext();
-  if (!url_request_context_getter_) {
-    PostCallbackToIOThread(std::string());
+  if (bitmap.drawsNothing()) {
+    if (icons.empty()) {
+      BrowserThread::PostTask(
+          BrowserThread::IO, FROM_HERE,
+          base::BindOnce(std::move(callback), std::string()));
+    } else {
+      // If could not download or decode the chosen image(e.g. not supported,
+      // invalid), try it again with remaining icons.
+      DownloadBestMatchingIcon(web_contents, icons, std::move(callback));
+    }
     return;
   }
 
-  FetchIcon();
+  gfx::Image decoded_image = gfx::Image::CreateFrom1xBitmap(bitmap);
+  scoped_refptr<base::RefCountedMemory> raw_data = decoded_image.As1xPNGBytes();
+  std::string encoded_data;
+  base::Base64Encode(
+      base::StringPiece(raw_data->front_as<char>(), raw_data->size()),
+      &encoded_data);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::BindOnce(std::move(callback), encoded_data));
 }
 
-void PaymentInstrumentIconFetcher::FetchIcon() {
+void DownloadBestMatchingIcon(
+    WebContents* web_contents,
+    const std::vector<Manifest::Icon>& icons,
+    PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcherCallback
+        callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (checking_image_object_index_ >= image_objects_.size()) {
-    PostCallbackToIOThread(std::string());
-    return;
-  }
+  GURL icon_url = ManifestIconSelector::FindBestMatchingIcon(
+      icons, kPaymentAppIdealIconSize, kPaymentAppMinimumIconSize,
+      Manifest::Icon::IconPurpose::ANY);
 
-  GURL* icon_src_url = &(image_objects_[checking_image_object_index_]->src);
-  if (!icon_src_url->is_valid()) {
-    checking_image_object_index_++;
-    FetchIcon();
-    return;
+  std::vector<Manifest::Icon> copy_icons;
+  for (const auto& icon : icons) {
+    if (icon.src != icon_url) {
+      copy_icons.emplace_back(icon);
+    }
   }
 
-  fetcher_ = net::URLFetcher::Create(*icon_src_url, net::URLFetcher::GET, this,
-                                     g_traffic_annotation);
-  fetcher_->SetRequestContext(url_request_context_getter_.get());
-  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                         net::LOAD_DO_NOT_SAVE_COOKIES);
-  fetcher_->SetStopOnRedirect(true);
-  fetcher_->Start();
+  bool can_download_icon = ManifestIconDownloader::Download(
+      web_contents, icon_url, kPaymentAppIdealIconSize,
+      kPaymentAppMinimumIconSize,
+      base::Bind(&OnIconFetched, web_contents, copy_icons,
+                 base::Passed(std::move(callback))));
+  // If the icon url is invalid, it's better to give the information to
+  // developers in advance unlike when fetching or decoding fails.
+  // We already checked whether they are valid in renderer side. So, if the
+  // icon url is invalid, it's something wrong.
+  if (!can_download_icon) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::BindOnce(std::move(callback), std::string()));
+  }
 }
 
-void PaymentInstrumentIconFetcher::OnURLFetchComplete(
-    const net::URLFetcher* source) {
+WebContents* GetWebContentsFromProviderHostIds(
+    const GURL& scope,
+    std::unique_ptr<std::vector<std::pair<int, int>>> provider_hosts) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  DCHECK_EQ(fetcher_.get(), source);
-  std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_);
-
-  std::string data;
-  if (!(source->GetStatus().is_success() &&
-        source->GetResponseCode() == net::HTTP_OK &&
-        source->GetResponseAsString(&data))) {
-    checking_image_object_index_++;
-    FetchIcon();
-    return;
+  for (const auto& host : *provider_hosts) {
+    RenderFrameHostImpl* render_frame_host =
+        RenderFrameHostImpl::FromID(host.first, host.second);
+    if (!render_frame_host)
+      continue;
+
+    WebContentsImpl* web_contents = static_cast<WebContentsImpl*>(
+        WebContents::FromRenderFrameHost(render_frame_host));
+    if (!web_contents || web_contents->IsHidden() ||
+        scope.GetOrigin().spec().compare(
+            web_contents->GetLastCommittedURL().GetOrigin().spec()) != 0) {
+      continue;
+    }
+    return web_contents;
   }
-
-  service_manager::mojom::ConnectorRequest connector_request;
-  std::unique_ptr<service_manager::Connector> connector =
-      service_manager::Connector::Create(&connector_request);
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->BindConnectorRequest(std::move(connector_request));
-
-  std::vector<uint8_t> image_data(data.begin(), data.end());
-  data_decoder::DecodeImage(
-      connector.get(), image_data, data_decoder::mojom::ImageCodec::DEFAULT,
-      false, data_decoder::kDefaultMaxSizeInBytes, gfx::Size(),
-      base::BindOnce(&PaymentInstrumentIconFetcher::DecodeImageCallback,
-                     weak_ptr_factory_.GetWeakPtr()));
+  return nullptr;
 }
 
-void PaymentInstrumentIconFetcher::DecodeImageCallback(const SkBitmap& bitmap) {
+void StartOnUI(
+    const GURL& scope,
+    std::unique_ptr<std::vector<std::pair<int, int>>> provider_hosts,
+    const std::vector<Manifest::Icon>& icons,
+    PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcherCallback
+        callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (bitmap.drawsNothing()) {
-    checking_image_object_index_++;
-    FetchIcon();
-    return;
-  }
+  WebContents* web_contents =
+      GetWebContentsFromProviderHostIds(scope, std::move(provider_hosts));
+  DownloadBestMatchingIcon(web_contents, icons, std::move(callback));
+}
 
-  gfx::Image decoded_image = gfx::Image::CreateFrom1xBitmap(bitmap);
-  scoped_refptr<base::RefCountedMemory> raw_data = decoded_image.As1xPNGBytes();
-  std::string base_64;
-  base::Base64Encode(
-      base::StringPiece(raw_data->front_as<char>(), raw_data->size()),
-      &base_64);
+}  // namespace
 
-  PostCallbackToIOThread(base_64);
-}
+// static
+void PaymentInstrumentIconFetcher::Start(
+    const GURL& scope,
+    std::unique_ptr<std::vector<std::pair<int, int>>> provider_hosts,
+    const std::vector<Manifest::Icon>& icons,
+    PaymentInstrumentIconFetcherCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-void PaymentInstrumentIconFetcher::PostCallbackToIOThread(
-    const std::string& decoded_data) {
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::BindOnce(std::move(callback_), decoded_data));
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&StartOnUI, scope, std::move(provider_hosts), icons,
+                     std::move(callback)));
 }
 
 }  // namespace content

content/browser/payments/payment_instrument_icon_fetcher.h

@@ -8,65 +8,28 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "content/browser/service_worker/service_worker_context_wrapper.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_fetcher_delegate.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "content/public/common/manifest.h"
 #include "third_party/WebKit/public/platform/modules/payments/payment_app.mojom.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace content {
 
-class PaymentInstrumentIconFetcher final : private net::URLFetcherDelegate {
+class PaymentInstrumentIconFetcher {
  public:
   using PaymentInstrumentIconFetcherCallback =
       base::OnceCallback<void(const std::string&)>;
 
-  PaymentInstrumentIconFetcher();
-  ~PaymentInstrumentIconFetcher() override;
-
-  // Starts fetching and decoding payment instrument icon from online. The
-  // result will be send back through |callback|.
-  // TODO(gogerald): Right now, we return the first fetchable and decodable icon
-  // with smallest available size. We may add more logic to choose appropriate
-  // icon according to icon size and our usage.
-  void Start(const std::vector<payments::mojom::ImageObjectPtr>& image_objects,
-             scoped_refptr<ServiceWorkerContextWrapper> service_worker_context,
-             PaymentInstrumentIconFetcherCallback callback);
+  // Should be called on IO thread.
+  static void Start(
+      const GURL& scope,
+      std::unique_ptr<std::vector<std::pair<int, int>>> provider_hosts,
+      const std::vector<Manifest::Icon>& icons,
+      PaymentInstrumentIconFetcherCallback callback);
 
  private:
-  void StartFromUIThread(
-      scoped_refptr<ServiceWorkerContextWrapper> service_worker_context);
-  void PostCallbackToIOThread(const std::string& decoded_data);
-
-  // data_decoder::mojom::ImageDecoder::DecodeImageCallback.
-  void DecodeImageCallback(const SkBitmap& bitmap);
-
-  // Override net::URLFetcherDelegate.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
-
-  void FetchIcon();
-
-  // Declared set of image objects of the payment instrument.
-  std::vector<payments::mojom::ImageObjectPtr> image_objects_;
-
-  // The index of the currently checking image object in image_objects_.
-  size_t checking_image_object_index_;
-
-  // The callback to notify after complete.
-  PaymentInstrumentIconFetcherCallback callback_;
-
-  // The url request context getter for fetching icon from online.
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
-
-  // The url fetcher to fetch raw icon from online.
-  std::unique_ptr<net::URLFetcher> fetcher_;
-
-  base::WeakPtrFactory<PaymentInstrumentIconFetcher> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(PaymentInstrumentIconFetcher);
+  DISALLOW_IMPLICIT_CONSTRUCTORS(PaymentInstrumentIconFetcher);
 };
 
 }  // namespace content

content/test/data/payments/payment_app_invocation.html

@@ -17,14 +17,54 @@ function registerPaymentApp() {
         'basic-card-payment-app-id',
         {
           name: 'Visa ****',
-          enabledMethods: ['basic-card']
+          enabledMethods: ['basic-card'],
+          icons: [
+            {
+              'src': 'icon-1x.png',
+              'sizes': '48x48',
+              'type': 'image/png'
+            },
+            {
+              'src': 'icon-1-5x.png',
+              'sizes': '72x72',
+              'type': 'image/png'
+            },
+            {
+              'src': 'icon-2x.png',
+              'sizes': '96x96',
+              'type': 'image/png',
+              'purpose': 'any badge'
+            }
+          ]
         }),
 
       registration.paymentManager.instruments.set(
         'bobpay-payment-app-id',
         {
-          name: "Bob Pay",
-          enabledMethods: ['https://bobpay.com']
+          name: 'Bob Pay',
+          enabledMethods: ['https://bobpay.com'],
+          icons: [
+            {
+              'src': 'icon-1x.png',
+              'sizes': '72x72',
+              'type': 'image/png'
+            },
+            {
+              'src': 'icon-3x.png',
+              'sizes': '144x144',
+              'type': 'image/png'
+            },
+            {
+              'src': 'icon-4x.png',
+              'sizes': '192x192',
+              'type': 'image/png'
+            },
+            {
+              'src': 'invalid_icon.png',
+              'sizes': '192x192',
+              'type': 'image/png'
+            }
+          ]
         }),
       ];
 
@@ -34,7 +74,7 @@ function registerPaymentApp() {
         sendResultToTest('registered');
       })
     .catch(result => {
-        sendResultToTest('error');
+        sendResultToTest(result);
       });
 }