[Background Fetch] Part 2: Add UKM metrics.

This adds the following metric:

BackgroundFetch.RatioOfIdealToChosenIconSize

Bug: 877512
Change-Id: Id4289b13178ca9e3bd76e1ea9371f778788b5699
Reviewed-on: https://chromium-review.googlesource.com/1199591
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589496}

chrome/browser/background_fetch/background_fetch_browsertest.cc

@@ -47,6 +47,9 @@ using offline_items_collection::OfflineItemVisuals;
 
 namespace {
 
+// Scripts run by this test are defined in
+// chrome/test/data/background_fetch/background_fetch.js.
+
 // URL of the test helper page that helps drive these tests.
 const char kHelperPage[] = "/background_fetch/background_fetch.html";
 
@@ -63,19 +66,19 @@ const char kSingleFileDownloadTitle[] = "Single-file Background Fetch";
 const int kDownloadedResourceSizeInBytes = 82;
 
 // Incorrect downloadTotal, when it's set too high in the JavaScript file
-// loaded by this test (chrome/test/data/background_fetch/background_fetch.js)
+// loaded by this test.
 const int kDownloadTotalBytesTooHigh = 1000;
 
 // Incorrect downloadTotal, when it's set too low in the JavaScript file
-// loaded by this test (chrome/test/data/background_fetch/background_fetch.js)
+// loaded by this test.
 const int kDownloadTotalBytesTooLow = 80;
 
 // Number of requests in the fetch() call from the JavaScript file loaded by
-// this test (chrome/test/data/background_fetch/background_fetch.js)
+// this test.
 const int kNumRequestsInFetch = 1;
 
 // Number of icons in the fetch() call from the JavaScript file loaded by this
-// test (chrome/test/data/background_fetch/background_fetch.js)
+// test.
 const int kNumIconsInFetch = 1;
 
 // Exponential bucket spacing for UKM event data.
@@ -303,6 +306,28 @@ class BackgroundFetchBrowserTest : public InProcessBrowserTest {
     ASSERT_EQ("ok", result);
   }
 
+  // Gets the ideal display size.
+  gfx::Size GetIconDisplaySize() {
+    gfx::Size out_display_size;
+    base::RunLoop run_loop;
+    browser()->profile()->GetBackgroundFetchDelegate()->GetIconDisplaySize(
+        base::BindOnce(&BackgroundFetchBrowserTest::DidGetIconDisplaySize,
+                       base::Unretained(this), run_loop.QuitClosure(),
+                       &out_display_size));
+    run_loop.Run();
+    return out_display_size;
+  }
+
+  // Called when we've received the ideal icon display size from
+  // BackgroundFetchDelegate.
+  void DidGetIconDisplaySize(base::OnceClosure quit_closure,
+                             gfx::Size* out_display_size,
+                             const gfx::Size& display_size) {
+    DCHECK(out_display_size);
+    *out_display_size = display_size;
+    std::move(quit_closure).Run();
+  }
+
   // Callback for WaitableDownloadLoggerObserver::DownloadAcceptedCallback().
   void DidAcceptDownloadCallback(base::OnceClosure quit_closure,
                                  std::string* out_guid,
@@ -424,6 +449,13 @@ IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
                                    kUkmEventDataBucketSpacing));
   test_ukm_recorder_->ExpectEntryMetric(
       entry, ukm::builders::BackgroundFetch::kDeniedDueToPermissionsName, 0);
+  // There is currently no desktop UI for BackgroundFetch, hence the icon
+  // display size is set to 0,0. Once that's no longer the case, this ASSERT
+  // will start failing and the unit test will have to be updated.
+  ASSERT_TRUE(GetIconDisplaySize().IsEmpty());
+  test_ukm_recorder_->ExpectEntryMetric(
+      entry, ukm::builders::BackgroundFetch::kRatioOfIdealToChosenIconSizeName,
+      -1);
 }
 
 IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,

content/browser/background_fetch/background_fetch_context.cc

@@ -135,6 +135,7 @@ void BackgroundFetchContext::StartFetch(
     const std::vector<ServiceWorkerFetchRequest>& requests,
     const BackgroundFetchOptions& options,
     const SkBitmap& icon,
+    blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
     RenderFrameHost* render_frame_host,
     blink::mojom::BackgroundFetchService::FetchCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -152,7 +153,7 @@ void BackgroundFetchContext::StartFetch(
       registration_id.origin(), render_frame_host,
       base::BindOnce(&BackgroundFetchContext::DidGetPermission,
                      weak_factory_.GetWeakPtr(), registration_id, requests,
-                     options, icon, frame_tree_node_id));
+                     options, icon, std::move(ukm_data), frame_tree_node_id));
 }
 
 void BackgroundFetchContext::GetPermissionForOrigin(
@@ -178,6 +179,7 @@ void BackgroundFetchContext::DidGetPermission(
     const std::vector<ServiceWorkerFetchRequest>& requests,
     const BackgroundFetchOptions& options,
     const SkBitmap& icon,
+    blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
     int frame_tree_node_id,
     bool has_permission) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -186,7 +188,7 @@ void BackgroundFetchContext::DidGetPermission(
       BrowserThread::UI, FROM_HERE,
       base::BindOnce(&background_fetch::RecordBackgroundFetchUkmEvent,
                      registration_id.origin(), requests, options, icon,
-                     frame_tree_node_id, has_permission));
+                     std::move(ukm_data), frame_tree_node_id, has_permission));
 
   if (has_permission) {
     data_manager_->BackgroundFetchDataManager::CreateRegistration(
@@ -207,7 +209,6 @@ void BackgroundFetchContext::DidGetPermission(
 void BackgroundFetchContext::GetIconDisplaySize(
     blink::mojom::BackgroundFetchService::GetIconDisplaySizeCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   delegate_proxy_.GetIconDisplaySize(std::move(callback));
 }
 

content/browser/background_fetch/background_fetch_context.h

@@ -91,6 +91,7 @@ class CONTENT_EXPORT BackgroundFetchContext
                   const std::vector<ServiceWorkerFetchRequest>& requests,
                   const BackgroundFetchOptions& options,
                   const SkBitmap& icon,
+                  blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
                   RenderFrameHost* render_frame_host,
                   blink::mojom::BackgroundFetchService::FetchCallback callback);
 
@@ -275,6 +276,7 @@ class CONTENT_EXPORT BackgroundFetchContext
                         const std::vector<ServiceWorkerFetchRequest>& requests,
                         const BackgroundFetchOptions& options,
                         const SkBitmap& icon,
+                        blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
                         int frame_tree_node_id,
                         bool has_permission);
 

content/browser/background_fetch/background_fetch_metrics.cc

@@ -36,6 +36,7 @@ void RecordBackgroundFetchUkmEvent(
     const std::vector<ServiceWorkerFetchRequest>& requests,
     const BackgroundFetchOptions& options,
     const SkBitmap& icon,
+    blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
     int frame_tree_node_id,
     bool has_permission) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -56,6 +57,7 @@ void RecordBackgroundFetchUkmEvent(
   ukm::builders::BackgroundFetch(source_id)
       .SetHasTitle(!options.title.empty())
       .SetNumIcons(options.icons.size())
+      .SetRatioOfIdealToChosenIconSize(ukm_data->ideal_to_chosen_icon_size)
       .SetDownloadTotal(ukm::GetExponentialBucketMin(
           options.download_total, kUkmEventDataBucketSpacing))
       .SetNumRequestsInFetch(ukm::GetExponentialBucketMin(

content/browser/background_fetch/background_fetch_metrics.h

@@ -32,6 +32,7 @@ void RecordBackgroundFetchUkmEvent(
     const std::vector<ServiceWorkerFetchRequest>& requests,
     const BackgroundFetchOptions& options,
     const SkBitmap& icon,
+    blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
     int frame_tree_node_id,
     bool has_permission);
 

content/browser/background_fetch/background_fetch_service_impl.cc

@@ -105,6 +105,7 @@ void BackgroundFetchServiceImpl::Fetch(
     const std::vector<ServiceWorkerFetchRequest>& requests,
     const BackgroundFetchOptions& options,
     const SkBitmap& icon,
+    blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
     FetchCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!ValidateDeveloperId(developer_id) || !ValidateRequests(requests)) {
@@ -122,9 +123,9 @@ void BackgroundFetchServiceImpl::Fetch(
                                                 origin_, developer_id,
                                                 base::GenerateGUID());
 
-  background_fetch_context_->StartFetch(registration_id, requests, options,
-                                        icon, render_frame_host_,
-                                        std::move(callback));
+  background_fetch_context_->StartFetch(
+      registration_id, requests, options, icon, std::move(ukm_data),
+      render_frame_host_, std::move(callback));
 }
 
 void BackgroundFetchServiceImpl::GetIconDisplaySize(

content/browser/background_fetch/background_fetch_service_impl.h

@@ -50,6 +50,7 @@ class CONTENT_EXPORT BackgroundFetchServiceImpl
              const std::vector<ServiceWorkerFetchRequest>& requests,
              const BackgroundFetchOptions& options,
              const SkBitmap& icon,
+             blink::mojom::BackgroundFetchUkmDataPtr ukm_data,
              FetchCallback callback) override;
   void GetIconDisplaySize(GetIconDisplaySizeCallback callback) override;
   void MatchRequests(

content/browser/background_fetch/background_fetch_service_unittest.cc

@@ -124,6 +124,7 @@ class BackgroundFetchServiceTest : public BackgroundFetchTestBase {
     base::RunLoop run_loop;
     service_->Fetch(
         service_worker_registration_id, developer_id, requests, options, icon,
+        blink::mojom::BackgroundFetchUkmData::New(),
         base::BindOnce(&BackgroundFetchServiceTest::DidGetRegistration,
                        base::Unretained(this), run_loop.QuitClosure(),
                        out_error, out_registration));
@@ -179,6 +180,7 @@ class BackgroundFetchServiceTest : public BackgroundFetchTestBase {
     base::RunLoop run_loop;
     service_->Fetch(
         service_worker_registration_id, developer_id, requests, options, icon,
+        blink::mojom::BackgroundFetchUkmData::New(),
         base::BindOnce(&BackgroundFetchServiceTest::DidGetRegistration,
                        base::Unretained(this), run_loop.QuitClosure(),
                        out_error, out_registration));

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

@@ -87,6 +87,15 @@ struct BackgroundFetchRegistration {
   BackgroundFetchFailureReason failure_reason;
 };
 
+// This contains the data we need to record UKM metrics, that isn't needed for
+// the background fetch itself.
+struct BackgroundFetchUkmData {
+  // Ratio of the icon size we display and that of the most suitable icon
+  // provided by the developer, times 100. -1 means that either the display
+  // size is empty, or no suitable icons have been provided.
+  int64 ideal_to_chosen_icon_size = -1;
+};
+
 interface BackgroundFetchRegistrationObserver {
   // Notifies the BackgroundFetchRegistration about progress in the fetches that
   // are part of it. The JavaScript `progress` event will be fired.
@@ -106,7 +115,8 @@ interface BackgroundFetchService {
         string developer_id,
         array<FetchAPIRequest> requests,
         BackgroundFetchOptions options,
-        skia.mojom.Bitmap? icon)
+        skia.mojom.Bitmap? icon,
+        BackgroundFetchUkmData ukm_data)
       => (BackgroundFetchError error,
           BackgroundFetchRegistration? registration);
 

third_party/blink/renderer/modules/background_fetch/background_fetch_bridge.cc

@@ -64,10 +64,11 @@ void BackgroundFetchBridge::Fetch(
     Vector<WebServiceWorkerRequest> requests,
     mojom::blink::BackgroundFetchOptionsPtr options,
     const SkBitmap& icon,
+    mojom::blink::BackgroundFetchUkmDataPtr ukm_data,
     RegistrationCallback callback) {
   GetService()->Fetch(
       GetSupplementable()->WebRegistration()->RegistrationId(), developer_id,
-      std::move(requests), std::move(options), icon,
+      std::move(requests), std::move(options), icon, std::move(ukm_data),
       WTF::Bind(&BackgroundFetchBridge::DidGetRegistration,
                 WrapPersistent(this), WTF::Passed(std::move(callback))));
 }

third_party/blink/renderer/modules/background_fetch/background_fetch_bridge.h

@@ -54,6 +54,7 @@ class BackgroundFetchBridge final
              Vector<WebServiceWorkerRequest> requests,
              mojom::blink::BackgroundFetchOptionsPtr options,
              const SkBitmap& icon,
+             mojom::blink::BackgroundFetchUkmDataPtr ukm_data,
              RegistrationCallback callback);
 
   // Gets the size of the icon to be displayed in Background Fetch UI.
@@ -62,7 +63,7 @@ class BackgroundFetchBridge final
   // Matches completed requests for the fetch associated with the |developer_id|
   // and |unique_id| and returns the {request, response} pairs based on the rest
   // of the arguments. If |filter_by_request| is true, only response(s) for
-  // |request_to_match| are returned. |cache_query_params| are options for the
+  // |request_to_match| are returned. |cache_query_params|s are options for the
   // query to the cache storage. |match_all|, when true, returns all responses
   // from the result set, and when false, returns only the first one.
   void MatchRequests(

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.cc

@@ -71,14 +71,16 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
   // If |icon_display_size_pixels_| is empty then no image will be displayed by
   // the UI powering Background Fetch. Bail out immediately.
   if (icon_display_size_pixels_.IsEmpty()) {
-    std::move(icon_callback).Run(SkBitmap());
+    std::move(icon_callback)
+        .Run(SkBitmap(), -1 /* ideal_to_chosen_icon_size_times_hundred */);
     return;
   }
 
   KURL best_icon_url = PickBestIconForDisplay(execution_context);
   if (best_icon_url.IsEmpty()) {
     // None of the icons provided was suitable.
-    std::move(icon_callback).Run(SkBitmap());
+    std::move(icon_callback)
+        .Run(SkBitmap(), -1 /* ideal_to_chosen_icon_size_times_hundred */);
     return;
   }
 
@@ -169,6 +171,7 @@ void BackgroundFetchIconLoader::DecodeAndResizeImageOnBackgroundThread(
       ColorBehavior::TransformToSRGB(),
       {icon_display_size_pixels_.width, icon_display_size_pixels_.height});
 
+  int64_t ideal_to_chosen_icon_size_times_hundred = -1;
   if (decoder) {
     ImageFrame* image_frame = decoder->DecodeFrameBufferAtIndex(0);
     if (image_frame) {
@@ -184,6 +187,8 @@ void BackgroundFetchIconLoader::DecodeAndResizeImageOnBackgroundThread(
           static_cast<double>(icon_display_size_pixels_.width) / width,
           static_cast<double>(icon_display_size_pixels_.height) / height);
 
+      ideal_to_chosen_icon_size_times_hundred = std::round(scale * 100.0);
+
       if (scale < 1) {
         width = ClampToRange(scale * width, 1, icon_display_size_pixels_.width);
         height =
@@ -200,7 +205,8 @@ void BackgroundFetchIconLoader::DecodeAndResizeImageOnBackgroundThread(
 
   PostCrossThreadTask(*task_runner, FROM_HERE,
                       CrossThreadBind(&BackgroundFetchIconLoader::RunCallback,
-                                      WrapCrossThreadPersistent(this)));
+                                      WrapCrossThreadPersistent(this),
+                                      ideal_to_chosen_icon_size_times_hundred));
 }
 
 void BackgroundFetchIconLoader::DidFail(const ResourceError& error) {
@@ -211,18 +217,20 @@ void BackgroundFetchIconLoader::DidFailRedirectCheck() {
   RunCallbackWithEmptyBitmap();
 }
 
-void BackgroundFetchIconLoader::RunCallback() {
+void BackgroundFetchIconLoader::RunCallback(
+    int64_t ideal_to_chosen_icon_size_times_hundred) {
   // If this has been stopped it is not desirable to trigger further work,
   // there is a shutdown of some sort in progress.
   if (stopped_)
     return;
 
-  std::move(icon_callback_).Run(decoded_icon_);
+  std::move(icon_callback_)
+      .Run(decoded_icon_, ideal_to_chosen_icon_size_times_hundred);
 }
 
 void BackgroundFetchIconLoader::RunCallbackWithEmptyBitmap() {
   DCHECK(decoded_icon_.isNull());
-  RunCallback();
+  RunCallback(-1 /* ideal_to_chosen_icon_size_times_hundred */);
 }
 
 }  // namespace blink

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.h

@@ -24,9 +24,11 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
     : public GarbageCollectedFinalized<BackgroundFetchIconLoader>,
       public ThreadableLoaderClient {
  public:
-  // The bitmap may be empty if the request failed or the image data
-  // could not be decoded.
-  using IconCallback = base::OnceCallback<void(const SkBitmap&)>;
+  // The bitmap may be empty if the request failed or the image data could not
+  // be decoded. The int64_t returned is the scale of the ideal to chosen icon,
+  // before resizing. This is -1 if the ideal icon size is empty, or if no icon
+  // provided was suitable.
+  using IconCallback = base::OnceCallback<void(const SkBitmap&, int64_t)>;
 
   BackgroundFetchIconLoader();
   ~BackgroundFetchIconLoader() override;
@@ -56,7 +58,7 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
  private:
   friend class BackgroundFetchIconLoaderTest;
 
-  void RunCallback();
+  void RunCallback(int64_t ideal_to_chosen_icon_size_times_hundred);
   void RunCallbackWithEmptyBitmap();
 
   // Callback for BackgroundFetchBridge::GetIconDisplaySize()

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader_test.cc

@@ -64,7 +64,9 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
 
   // Callback for BackgroundFetchIconLoader. This will set up the state of the
   // load as either success or failed based on whether the bitmap is empty.
-  void IconLoaded(base::OnceClosure quit_closure, const SkBitmap& bitmap) {
+  void IconLoaded(base::OnceClosure quit_closure,
+                  const SkBitmap& bitmap,
+                  int64_t ideal_to_chosen_icon_size) {
     bitmap_ = bitmap;
 
     if (!bitmap_.isNull())

third_party/blink/renderer/modules/background_fetch/background_fetch_manager.cc

@@ -276,8 +276,8 @@ ScriptPromise BackgroundFetchManager::fetch(
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
   ScriptPromise promise = resolver->Promise();
 
-  // Load Icons. Right now, we just load the first icon. Lack of icons or
-  // inability to load them should not be fatal to the fetch.
+  // Pick the best icon, and load it.
+  // Inability to load them should not be fatal to the fetch.
   mojom::blink::BackgroundFetchOptionsPtr options_ptr =
       mojom::blink::BackgroundFetchOptions::From(options);
   if (options.icons().size()) {
@@ -290,7 +290,7 @@ ScriptPromise BackgroundFetchManager::fetch(
   }
 
   DidLoadIcons(id, std::move(web_requests), std::move(options_ptr), resolver,
-               SkBitmap());
+               SkBitmap(), -1 /* ideal_to_chosen_icon_size */);
   return promise;
 }
 
@@ -299,9 +299,13 @@ void BackgroundFetchManager::DidLoadIcons(
     Vector<WebServiceWorkerRequest> web_requests,
     mojom::blink::BackgroundFetchOptionsPtr options,
     ScriptPromiseResolver* resolver,
-    const SkBitmap& icon) {
+    const SkBitmap& icon,
+    int64_t ideal_to_chosen_icon_size) {
+  auto ukm_data = mojom::blink::BackgroundFetchUkmData::New();
+  ukm_data->ideal_to_chosen_icon_size = ideal_to_chosen_icon_size;
   bridge_->Fetch(
       id, std::move(web_requests), std::move(options), icon,
+      std::move(ukm_data),
       WTF::Bind(&BackgroundFetchManager::DidFetch, WrapPersistent(this),
                 WrapPersistent(resolver), base::Time::Now()));
 }

third_party/blink/renderer/modules/background_fetch/background_fetch_manager.h

@@ -76,7 +76,8 @@ class MODULES_EXPORT BackgroundFetchManager final
                     Vector<WebServiceWorkerRequest> web_requests,
                     mojom::blink::BackgroundFetchOptionsPtr options,
                     ScriptPromiseResolver* resolver,
-                    const SkBitmap& icon);
+                    const SkBitmap& icon,
+                    int64_t ideal_to_chosen_icon_size);
   void DidFetch(ScriptPromiseResolver* resolver,
                 base::Time time_started,
                 mojom::blink::BackgroundFetchError error,

third_party/blink/renderer/modules/background_fetch/background_fetch_update_ui_event.cc

@@ -72,7 +72,8 @@ ScriptPromise BackgroundFetchUpdateUIEvent::updateUI(
   ScriptPromise promise = resolver->Promise();
 
   if (ui_options.icons().IsEmpty()) {
-    DidGetIcon(resolver, ui_options.title(), SkBitmap());
+    DidGetIcon(resolver, ui_options.title(), SkBitmap(),
+               -1 /* ideal_to_chosen_icon_size */);
   } else {
     DCHECK(!loader_);
     loader_ = new BackgroundFetchIconLoader();
@@ -87,9 +88,11 @@ ScriptPromise BackgroundFetchUpdateUIEvent::updateUI(
   return promise;
 }
 
-void BackgroundFetchUpdateUIEvent::DidGetIcon(ScriptPromiseResolver* resolver,
-                                              const String& title,
-                                              const SkBitmap& icon) {
+void BackgroundFetchUpdateUIEvent::DidGetIcon(
+    ScriptPromiseResolver* resolver,
+    const String& title,
+    const SkBitmap& icon,
+    int64_t ideal_to_chosen_icon_size) {
   BackgroundFetchBridge::From(service_worker_registration_)
       ->UpdateUI(registration_->id(), registration_->unique_id(), title, icon,
                  WTF::Bind(&BackgroundFetchUpdateUIEvent::DidUpdateUI,

third_party/blink/renderer/modules/background_fetch/background_fetch_update_ui_event.h

@@ -61,7 +61,8 @@ class MODULES_EXPORT BackgroundFetchUpdateUIEvent final
 
   void DidGetIcon(ScriptPromiseResolver* resolver,
                   const String& title,
-                  const SkBitmap& icon);
+                  const SkBitmap& icon,
+                  int64_t ideal_to_chosen_icon_size);
 
   void DidUpdateUI(ScriptPromiseResolver* resolver,
                    mojom::blink::BackgroundFetchError error);

tools/metrics/ukm/ukm.xml

@@ -2571,6 +2571,13 @@ be describing additional metrics about the same event.
       method with a value of 2.0 for spacing.
     </summary>
   </metric>
+  <metric name="RatioOfIdealToChosenIconSize">
+    <summary>
+      Ratio of the ideal icon to the chosen icon size, times hundred. This will
+      be set to -1 if either ideal icon size is 0, or if none of the provided
+      icons are suitable.
+    </summary>
+  </metric>
 </event>
 
 <event name="OfflinePages.SavePageRequested">