[Background Fetch] Get the right icon size based on the display.

Currently, I'm
hardcoding values for Android and returning 0 for all other platforms.
In a subsequent CL I'll pick the right icon size from the list provided
by the developer, based on this size.

TBR=peter@chromium.org

Bug: 813564
Change-Id: If1d302a7439f7533d1c5649cce7eb2d0bf5583bc
Reviewed-on: https://chromium-review.googlesource.com/968865
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Martin Barbella <mbarbella@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546115}

chrome/browser/background_fetch/background_fetch_browsertest.cc

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
+#include "build/build_config.h"
 #include "chrome/browser/background_fetch/background_fetch_delegate_impl.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
@@ -357,9 +358,13 @@ IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
   // Get visuals associated with the newly added offline item.
   std::unique_ptr<OfflineItemVisuals> out_visuals;
   GetVisualsForOfflineItemSync(offline_item.id, &out_visuals);
+#if defined(OS_ANDROID)
   EXPECT_FALSE(out_visuals->icon.IsEmpty());
   EXPECT_EQ(out_visuals->icon.Size().width(), 100);
   EXPECT_EQ(out_visuals->icon.Size().height(), 100);
+#else
+  EXPECT_TRUE(out_visuals->icon.IsEmpty());
+#endif
 }
 
 }  // namespace

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -10,6 +10,7 @@
 #include "base/guid.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "build/build_config.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -20,6 +21,7 @@
 #include "content/public/browser/background_fetch_response.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia.h"
 
 BackgroundFetchDelegateImpl::BackgroundFetchDelegateImpl(Profile* profile)
@@ -90,6 +92,22 @@ void BackgroundFetchDelegateImpl::JobDetails::UpdateOfflineItem() {
     offline_item.state = OfflineItemState::IN_PROGRESS;
 }
 
+void BackgroundFetchDelegateImpl::GetIconDisplaySize(
+    BackgroundFetchDelegate::GetIconDisplaySizeCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // If Android, return 192x192, else return 0x0. 0x0 means not loading an
+  // icon at all, which is returned for all non-Android platforms as the
+  // icons can't be displayed on the UI yet.
+  // TODO(nator): Move this logic to OfflineItemsCollection, and return icon
+  // size based on display.
+  gfx::Size display_size;
+#if defined(OS_ANDROID)
+  display_size = gfx::Size(192, 192);
+#endif
+  std::move(callback).Run(display_size);
+}
+
 void BackgroundFetchDelegateImpl::CreateDownloadJob(
     const std::string& job_unique_id,
     const std::string& title,

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -48,6 +48,7 @@ class BackgroundFetchDelegateImpl
   void Shutdown() override;
 
   // BackgroundFetchDelegate implementation:
+  void GetIconDisplaySize(GetIconDisplaySizeCallback callback) override;
   void CreateDownloadJob(
       const std::string& job_unique_id,
       const std::string& title,

content/browser/background_fetch/background_fetch_context.cc

@@ -114,6 +114,13 @@ void BackgroundFetchContext::StartFetch(
                      std::move(callback)));
 }
 
+void BackgroundFetchContext::GetIconDisplaySize(
+    blink::mojom::BackgroundFetchService::GetIconDisplaySizeCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  delegate_proxy_.GetIconDisplaySize(std::move(callback));
+}
+
 void BackgroundFetchContext::DidCreateRegistration(
     const BackgroundFetchRegistrationId& registration_id,
     const BackgroundFetchOptions& options,

content/browser/background_fetch/background_fetch_context.h

@@ -74,6 +74,11 @@ class CONTENT_EXPORT BackgroundFetchContext
                   const SkBitmap& icon,
                   blink::mojom::BackgroundFetchService::FetchCallback callback);
 
+  // Gets display size for the icon for Background Fetch UI.
+  void GetIconDisplaySize(
+      blink::mojom::BackgroundFetchService::GetIconDisplaySizeCallback
+          callback);
+
   // Aborts the Background Fetch for the |registration_id|. The callback will be
   // invoked with INVALID_ID if the registration has already completed or
   // aborted, STORAGE_ERROR if an I/O error occurs, or NONE for success.

content/browser/background_fetch/background_fetch_delegate_proxy.cc

@@ -10,7 +10,6 @@
 #include "components/download/public/common/download_item.h"
 #include "components/download/public/common/download_url_parameters.h"
 #include "content/browser/background_fetch/background_fetch_job_controller.h"
-#include "content/public/browser/background_fetch_delegate.h"
 #include "content/public/browser/background_fetch_response.h"
 #include "content/public/browser/download_manager.h"
 
@@ -41,6 +40,29 @@ class BackgroundFetchDelegateProxy::Core
     return weak_ptr_factory_.GetWeakPtr();
   }
 
+  void ForwardGetIconDisplaySizeCallbackToIO(
+      BackgroundFetchDelegate::GetIconDisplaySizeCallback callback,
+      const gfx::Size& display_size) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::BindOnce(std::move(callback), display_size));
+  }
+
+  void GetIconDisplaySize(
+      BackgroundFetchDelegate::GetIconDisplaySizeCallback callback) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    if (delegate_) {
+      delegate_->GetIconDisplaySize(
+          base::BindOnce(&Core::ForwardGetIconDisplaySizeCallbackToIO,
+                         weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::IO, FROM_HERE,
+          base::BindOnce(std::move(callback), gfx::Size(0, 0)));
+    }
+  }
+
   void CreateDownloadJob(const std::string& job_unique_id,
                          const std::string& title,
                          const url::Origin& origin,
@@ -230,6 +252,14 @@ BackgroundFetchDelegateProxy::~BackgroundFetchDelegateProxy() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 }
 
+void BackgroundFetchDelegateProxy::GetIconDisplaySize(
+    BackgroundFetchDelegate::GetIconDisplaySizeCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::BindOnce(&Core::GetIconDisplaySize,
+                                         ui_core_ptr_, std::move(callback)));
+}
+
 void BackgroundFetchDelegateProxy::CreateDownloadJob(
     const std::string& job_unique_id,
     const std::string& title,

content/browser/background_fetch/background_fetch_delegate_proxy.h

@@ -15,8 +15,10 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "content/browser/background_fetch/background_fetch_request_info.h"
+#include "content/public/browser/background_fetch_delegate.h"
 #include "content/public/browser/background_fetch_response.h"
 #include "content/public/browser/browser_thread.h"
+#include "third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom.h"
 
 class SkBitmap;
 
@@ -56,6 +58,10 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
 
   ~BackgroundFetchDelegateProxy();
 
+  // Gets size of the icon to display with the Background Fetch UI.
+  void GetIconDisplaySize(
+      BackgroundFetchDelegate::GetIconDisplaySizeCallback callback);
+
   // Creates a new download grouping identified by |job_unique_id|. Further
   // downloads started by StartRequest will also use this |job_unique_id| so
   // that a notification can be updated with the current status. If the download

content/browser/background_fetch/background_fetch_delegate_proxy_unittest.cc

@@ -27,6 +27,8 @@ class FakeBackgroundFetchDelegate : public BackgroundFetchDelegate {
   FakeBackgroundFetchDelegate() {}
 
   // BackgroundFetchDelegate implementation:
+  void GetIconDisplaySize(
+      BackgroundFetchDelegate::GetIconDisplaySizeCallback callback) override {}
   void CreateDownloadJob(
       const std::string& job_unique_id,
       const std::string& title,

content/browser/background_fetch/background_fetch_service_impl.cc

@@ -100,6 +100,12 @@ void BackgroundFetchServiceImpl::Fetch(
                                         icon, std::move(callback));
 }
 
+void BackgroundFetchServiceImpl::GetIconDisplaySize(
+    blink::mojom::BackgroundFetchService::GetIconDisplaySizeCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  background_fetch_context_->GetIconDisplaySize(std::move(callback));
+}
+
 void BackgroundFetchServiceImpl::UpdateUI(const std::string& unique_id,
                                           const std::string& title,
                                           UpdateUICallback callback) {

content/browser/background_fetch/background_fetch_service_impl.h

@@ -42,6 +42,7 @@ class CONTENT_EXPORT BackgroundFetchServiceImpl
              const BackgroundFetchOptions& options,
              const SkBitmap& icon,
              FetchCallback callback) override;
+  void GetIconDisplaySize(GetIconDisplaySizeCallback callback) override;
   void UpdateUI(const std::string& unique_id,
                 const std::string& title,
                 UpdateUICallback callback) override;

content/browser/background_fetch/mock_background_fetch_delegate.cc

@@ -55,6 +55,8 @@ MockBackgroundFetchDelegate::MockBackgroundFetchDelegate() {}
 
 MockBackgroundFetchDelegate::~MockBackgroundFetchDelegate() {}
 
+void MockBackgroundFetchDelegate::GetIconDisplaySize(
+    BackgroundFetchDelegate::GetIconDisplaySizeCallback callback) {}
 void MockBackgroundFetchDelegate::CreateDownloadJob(
     const std::string& job_unique_id,
     const std::string& title,

content/browser/background_fetch/mock_background_fetch_delegate.h

@@ -63,6 +63,8 @@ class MockBackgroundFetchDelegate : public BackgroundFetchDelegate {
   ~MockBackgroundFetchDelegate() override;
 
   // BackgroundFetchDelegate implementation:
+  void GetIconDisplaySize(
+      BackgroundFetchDelegate::GetIconDisplaySizeCallback callback) override;
   void CreateDownloadJob(
       const std::string& job_unique_id,
       const std::string& title,

content/public/browser/background_fetch_delegate.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -17,6 +18,10 @@
 class GURL;
 class SkBitmap;
 
+namespace gfx {
+class Size;
+}
+
 namespace net {
 class HttpRequestHeaders;
 struct NetworkTrafficAnnotationTag;
@@ -36,6 +41,8 @@ struct BackgroundFetchResult;
 // BackgroundFetchDelegateProxy.
 class CONTENT_EXPORT BackgroundFetchDelegate {
  public:
+  using GetIconDisplaySizeCallback = base::OnceCallback<void(const gfx::Size&)>;
+
   // Client interface that a BackgroundFetchDelegate would use to signal the
   // progress of a background fetch.
   class Client {
@@ -75,6 +82,9 @@ class CONTENT_EXPORT BackgroundFetchDelegate {
 
   virtual ~BackgroundFetchDelegate();
 
+  // Gets size of the icon to display with the Background Fetch UI.
+  virtual void GetIconDisplaySize(GetIconDisplaySizeCallback callback) = 0;
+
   // Creates a new download grouping identified by |job_unique_id|. Further
   // downloads started by DownloadUrl will also use this |job_unique_id| so that
   // a notification can be updated with the current status. If the download was

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.cpp

@@ -41,6 +41,11 @@ BackgroundFetchBridge::BackgroundFetchBridge(
 
 BackgroundFetchBridge::~BackgroundFetchBridge() = default;
 
+void BackgroundFetchBridge::GetIconDisplaySize(
+    GetIconDisplaySizeCallback callback) {
+  GetService()->GetIconDisplaySize(std::move(callback));
+}
+
 void BackgroundFetchBridge::Fetch(
     const String& developer_id,
     Vector<WebServiceWorkerRequest> requests,

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchBridge.h

@@ -39,6 +39,7 @@ class BackgroundFetchBridge final
   using RegistrationCallback =
       base::OnceCallback<void(mojom::blink::BackgroundFetchError,
                               BackgroundFetchRegistration*)>;
+  using GetIconDisplaySizeCallback = base::OnceCallback<void(const WebSize&)>;
   using UpdateUICallback =
       base::OnceCallback<void(mojom::blink::BackgroundFetchError)>;
 
@@ -55,6 +56,9 @@ class BackgroundFetchBridge final
              const SkBitmap& icon,
              RegistrationCallback);
 
+  // Gets the size of the icon to be displayed in Background Fetch UI.
+  void GetIconDisplaySize(GetIconDisplaySizeCallback);
+
   // Updates the user interface for the Background Fetch identified by
   // |unique_id| with the updated |title|. Will invoke the |callback| when the
   // interface has been requested to update.

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchIconLoader.cpp

@@ -6,14 +6,17 @@
 
 #include "core/dom/ExecutionContext.h"
 #include "core/loader/ThreadableLoader.h"
+#include "modules/background_fetch/BackgroundFetchBridge.h"
 #include "modules/background_fetch/IconDefinition.h"
 #include "platform/graphics/ColorBehavior.h"
+#include "platform/heap/HeapAllocator.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "platform/image-decoders/ImageFrame.h"
 #include "platform/loader/fetch/ResourceLoaderOptions.h"
 #include "platform/loader/fetch/ResourceRequest.h"
 #include "platform/weborigin/KURL.h"
 #include "platform/wtf/Threading.h"
+#include "public/platform/WebSize.h"
 #include "public/platform/WebURLRequest.h"
 #include "skia/ext/image_operations.h"
 
@@ -32,14 +35,28 @@ BackgroundFetchIconLoader::~BackgroundFetchIconLoader() {
 }
 
 // TODO(nator): Add functionality to select which icon to load.
-void BackgroundFetchIconLoader::Start(ExecutionContext* execution_context,
+void BackgroundFetchIconLoader::Start(BackgroundFetchBridge* bridge,
+                                      ExecutionContext* execution_context,
                                       HeapVector<IconDefinition> icons,
                                       IconCallback icon_callback) {
   DCHECK(!stopped_);
   DCHECK_GE(icons.size(), 1u);
+  DCHECK(bridge);
+
   icons_ = std::move(icons);
+  bridge->GetIconDisplaySize(
+      WTF::Bind(&BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon,
+                WrapWeakPersistent(this), WrapWeakPersistent(execution_context),
+                std::move(icon_callback)));
+}
 
-  if (!icons_[0].hasSrc()) {
+void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
+    ExecutionContext* execution_context,
+    IconCallback icon_callback,
+    const WebSize& icon_display_size_pixels) {
+  // TODO(nator): Pick the appropriate icon based on display size instead,
+  // and resize it, if needed.
+  if (icon_display_size_pixels.IsEmpty() || !icons_[0].hasSrc()) {
     std::move(icon_callback).Run(SkBitmap());
     return;
   }

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchIconLoader.h

@@ -16,7 +16,9 @@
 
 namespace blink {
 
+class BackgroundFetchBridge;
 class IconDefinition;
+struct WebSize;
 
 class MODULES_EXPORT BackgroundFetchIconLoader final
     : public GarbageCollectedFinalized<BackgroundFetchIconLoader>,
@@ -35,7 +37,10 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
 
   // Asynchronously download an icon from the given url, decodes the loaded
   // data, and passes the bitmap to the given callback.
-  void Start(ExecutionContext*, HeapVector<IconDefinition>, IconCallback);
+  void Start(BackgroundFetchBridge*,
+             ExecutionContext*,
+             HeapVector<IconDefinition>,
+             IconCallback);
 
   // Cancels the pending load, if there is one. The |icon_callback_| will not
   // be run.
@@ -54,8 +59,15 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
   }
 
  private:
+  friend class BackgroundFetchIconLoaderTest;
   void RunCallbackWithEmptyBitmap();
 
+  // Callback for BackgroundFetchBridge::GetIconDisplaySize()
+  void DidGetIconDisplaySizeIfSoLoadIcon(
+      ExecutionContext*,
+      IconCallback,
+      const WebSize& icon_display_size_pixels);
+
   bool stopped_ = false;
   scoped_refptr<SharedBuffer> data_;
   IconCallback icon_callback_;

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchIconLoaderTest.cpp

@@ -11,6 +11,7 @@
 #include "platform/testing/URLTestHelpers.h"
 #include "platform/testing/UnitTestHelpers.h"
 #include "platform/weborigin/KURL.h"
+#include "public/platform/WebSize.h"
 #include "public/platform/WebURL.h"
 #include "public/platform/WebURLLoaderMockFactory.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -28,6 +29,8 @@ constexpr char kBackgroundFetchImageLoaderBaseUrl[] = "http://test.com/";
 constexpr char kBackgroundFetchImageLoaderBaseDir[] = "notifications/";
 constexpr char kBackgroundFetchImageLoaderIcon500x500[] = "500x500.png";
 
+}  // namespace
+
 class BackgroundFetchIconLoaderTest : public PageTestBase {
  public:
   BackgroundFetchIconLoaderTest() : loader_(new BackgroundFetchIconLoader()) {}
@@ -63,9 +66,11 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
     icon.setType("image/png");
     icon.setSizes("500x500");
     HeapVector<IconDefinition> icons(1, icon);
-    loader_->Start(GetContext(), icons,
-                   Bind(&BackgroundFetchIconLoaderTest::IconLoaded,
-                        WTF::Unretained(this)));
+    loader_->icons_ = std::move(icons);
+    loader_->DidGetIconDisplaySizeIfSoLoadIcon(
+        GetContext(),
+        Bind(&BackgroundFetchIconLoaderTest::IconLoaded, WTF::Unretained(this)),
+        WebSize(192, 192));
   }
 
   ExecutionContext* GetContext() const { return &GetDocument(); }
@@ -85,5 +90,4 @@ TEST_F(BackgroundFetchIconLoaderTest, SuccessTest) {
   EXPECT_EQ(BackgroundFetchLoadState::kLoadSuccessful, loaded_);
 }
 
-}  // namespace
 }  // namespace blink

third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp

@@ -261,7 +261,7 @@ ScriptPromise BackgroundFetchManager::fetch(
       mojom::blink::BackgroundFetchOptions::From(options);
   if (options.icons().size()) {
     loader_->Start(
-        execution_context, options.icons(),
+        bridge_.Get(), execution_context, options.icons(),
         WTF::Bind(&BackgroundFetchManager::DidLoadIcons, WrapPersistent(this),
                   id, WTF::Passed(std::move(web_requests)),
                   std::move(options_ptr), WrapPersistent(resolver)));

third_party/WebKit/public/platform/modules/background_fetch/background_fetch.mojom

@@ -6,6 +6,7 @@ module blink.mojom;
 
 import "skia/public/interfaces/bitmap.mojom";
 import "third_party/WebKit/public/platform/modules/fetch/fetch_api_request.mojom";
+import "ui/gfx/geometry/mojo/geometry.mojom";
 
 enum BackgroundFetchError {
   NONE,
@@ -99,6 +100,10 @@ interface BackgroundFetchService {
       => (BackgroundFetchError error,
           array<string> developer_ids);
 
+  // Gets size of the icon to display with the Background Fetch UI.
+  GetIconDisplaySize()
+    => (gfx.mojom.Size icon_size_pixels);
+
   // Registers the |observer| to receive events for the given registration
   // that is identified by the |unique_id|.
   AddRegistrationObserver(string unique_id,