Allow the app banner installability check to run on page load.

chrome/browser/installable/installable_manager.cc

 // Copyright 2016 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/installable/installable_manager.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/manifest/manifest_icon_downloader.h"
@@ skipping 43 matching lines @@
       if (size.width() >= kIconMinimumSizeInPx &&
           size.height() >= kIconMinimumSizeInPx) {
         return true;
       }
     }
   }
 
   return false;
 }
 
+// Returns true if |params| specifies a full PWA check.
+bool IsParamsForPwaCheck(const InstallableParams& params) {
+  return params.check_installable && params.fetch_valid_primary_icon;
+}
+
+bool IsInstallabilityCheckCompleted(
+    InstallableMetrics::InstallabilityCheckStatus status) {
+  return status == InstallableMetrics::COMPLETE_PWA ||
+         status == InstallableMetrics::COMPLETE_NON_PWA;
+}
+
 }  // namespace
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(InstallableManager);
 
 struct InstallableManager::ManifestProperty {
   InstallableStatusCode error = NO_ERROR_DETECTED;
   GURL url;
   content::Manifest manifest;
   bool fetched = false;
 };
@@ skipping 13 matching lines @@
   InstallableStatusCode error = NO_ERROR_DETECTED;
   GURL url;
   std::unique_ptr<SkBitmap> icon;
   bool fetched;
 
  private:
   // This class contains a std::unique_ptr and therefore must be move-only.
   DISALLOW_COPY_AND_ASSIGN(IconProperty);
 };
 
-
 InstallableManager::InstallableManager(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       manifest_(new ManifestProperty()),
       installable_(new InstallableProperty()),
+      status_(InstallableMetrics::NOT_STARTED),
+      menu_opened_count_(0),
+      menu_item_add_to_homescreen_count_(0),
       is_active_(false),
-      weak_factory_(this) { }
+      weak_factory_(this) {}
 
 InstallableManager::~InstallableManager() = default;
 
 // static
 bool InstallableManager::IsContentSecure(content::WebContents* web_contents) {
   if (!web_contents)
     return false;
 
   // Whitelist localhost. Check the VisibleURL to match what the
   // SecurityStateTabHelper looks at.
@@ skipping 19 matching lines @@
   // Return immediately if we're already working on a task. The new task will be
   // looked at once the current task is finished.
   tasks_.push_back({params, callback});
   if (is_active_)
     return;
 
   is_active_ = true;
   StartNextTask();
 }
 
+void InstallableManager::RecordMenuOpenHistogram() {
+  if (IsInstallabilityCheckCompleted(status_)) {
+    InstallableMetrics::RecordMenuOpenHistogram(status_);
+  } else {
+    ++menu_opened_count_;
+  }
+}
+
+void InstallableManager::RecordMenuItemAddToHomescreenHistogram() {
+  if (IsInstallabilityCheckCompleted(status_)) {
+    InstallableMetrics::RecordMenuItemAddToHomescreenHistogram(status_);
+  } else {
+    ++menu_item_add_to_homescreen_count_;
+  }
+}
+
 InstallableManager::IconParams InstallableManager::ParamsForPrimaryIcon(
     const InstallableParams& params) const {
   return std::make_tuple(params.ideal_primary_icon_size_in_px,
                          params.minimum_primary_icon_size_in_px,
                          IconPurpose::ANY);
 }
 
 InstallableManager::IconParams InstallableManager::ParamsForBadgeIcon(
     const InstallableParams& params) const {
   return std::make_tuple(params.ideal_badge_icon_size_in_px,
@@ skipping 72 matching lines @@
          (!params.fetch_valid_badge_icon ||
              IsIconFetched(ParamsForBadgeIcon(params)));
 }
 
 void InstallableManager::Reset() {
   // Prevent any outstanding callbacks to or from this object from being called.
   weak_factory_.InvalidateWeakPtrs();
   tasks_.clear();
   icons_.clear();
 
+  status_ = InstallableMetrics::NOT_STARTED;
+  menu_opened_count_ = 0;
+  menu_item_add_to_homescreen_count_ = 0;
+
   manifest_.reset(new ManifestProperty());
   installable_.reset(new InstallableProperty());
 
   is_active_ = false;
 }
 
 void InstallableManager::SetManifestDependentTasksComplete() {
   DCHECK(!tasks_.empty());
   const InstallableParams& params = tasks_[0].first;
 
@@ skipping 39 matching lines @@
 
   DCHECK(is_active_);
   WorkOnTask();
 }
 
 void InstallableManager::WorkOnTask() {
   DCHECK(!tasks_.empty());
   const Task& task = tasks_[0];
   const InstallableParams& params = task.first;
 
+  if (status_ == InstallableMetrics::NOT_STARTED &&
+      IsParamsForPwaCheck(params)) {
+    // Use the NOT_COMPLETE_PWA state to represent the general case of the check

[benwells, 2017/03/30 08:22:37]

You mean IN_PROGRESS_PWA?

[dominickn, 2017/03/30 23:42:52]

Done.

+    // being in progress.
+    status_ = InstallableMetrics::IN_PROGRESS_PWA;
+  }
+
   InstallableStatusCode code = GetErrorCode(params);
-  if (code != NO_ERROR_DETECTED || IsComplete(params)) {
+  bool check_passed = (code == NO_ERROR_DETECTED);
+  if (!check_passed || IsComplete(params)) {
+    if (status_ == InstallableMetrics::IN_PROGRESS_PWA &&
+        IsParamsForPwaCheck(params)) {
+      status_ = check_passed ? InstallableMetrics::COMPLETE_PWA
+                             : InstallableMetrics::COMPLETE_NON_PWA;
+
+      // Compute what the status would have been and record metrics.

[benwells, 2017/03/30 08:22:37]

Nit: 'would have been' -> 'would have been for any RecordMenu* calls received
while the check was running'. Or something. It took me a while to realise that.

[dominickn, 2017/03/30 23:42:53]

Done.

+      InstallableMetrics::InstallabilityCheckStatus prev_status =
+          check_passed ? InstallableMetrics::IN_PROGRESS_PWA
+                       : InstallableMetrics::IN_PROGRESS_NON_PWA;

[pkotwicz, 2017/03/30 20:48:25]

What is the value of having both IN_PROGRESS_PWA and IN_PROGRESS_NON_PWA?

If InstallableManager::RecordMenuOpenHistogram() is called while the check is in
progress, InstallableMetrics::IN_PROGRESS_PWA will always be recorded

[dominickn, 2017/03/30 23:42:52]

This is not correct. RecordMenuOpenHistogram() only records something if the
state is COMPLETE_PWA or COMPLETE_NON_PWA. If the check is in progress (i.e. the
state is IN_PROGRESS_PWA), then nothing is recorded until the check is done. See
the definition of that method.

We need to distinguish these cases because the case we most care about is when a
site IS a PWA and the check HAS / HASN'T completed. We don't want that to be
obscured in what is likely to be the much more frequent cases of NOT PWA / (NOT)
COMPLETED.

It will not be useful if all we get from this metric is that 90% of the time we
have completed the PWA check when the menu is opened. We'll have no idea whether
PWAs are in or out of the 90%, and that's what we care about (i.e. when the menu
is opened, is the check complete AND the site is a PWA). So we have to have the
two separate cases of COMPLETE/NOT COMPLETE for PWAs and NON PWAs.

[pkotwicz, 2017/03/31 04:12:37]

I understand.

In this case the code on lines 327 - 331 is useless

|status_| can be replaced by:
bool pwa_check_completed_;
InstallabilityCheckStatus pwa_check_result_;

[dominickn, 2017/03/31 04:59:17]

Done, thanks!

+      while (menu_opened_count_-- > 0)
+        InstallableMetrics::RecordMenuOpenHistogram(prev_status);
+
+      while (menu_item_add_to_homescreen_count_-- > 0)
+        InstallableMetrics::RecordMenuItemAddToHomescreenHistogram(prev_status);
+
+      menu_opened_count_ = 0;

[benwells, 2017/03/30 08:22:37]

Do you need to reset these two variables? They should just just been reset to
zero by the while loops.

[dominickn, 2017/03/30 23:42:52]

The while loops may have set them to -1 if they started at 0. So yes, they need
to be set explicitly to 0 unless I want to add extra conditionals to not loop if
they're at 0 already. This seemed slightly clearer.

[benwells, 2017/03/31 04:00:35]

Ah ... I see. You could use a for loop instead:

for (; menu_ > 0; menu_--) {
}

Or just:
while (menu_ > 0) {
  \\blah
  menu_--;
}

Up to you...

[dominickn, 2017/03/31 04:59:17]

Changed to the for loop, that's a bit nicer.

+      menu_item_add_to_homescreen_count_ = 0;
+    }
+
     RunCallback(task, code);

[pkotwicz, 2017/03/30 20:48:25]

I think that it would be clearer if the metrics recording code was in GetData()
and RunCallback()

Having the logic in this function is confusing to me. I keep on wondering: "Do
we record metrics when WorkOnTask() is called but the callback is not called"

[dominickn, 2017/03/30 23:42:53]

RunCallback() already has to do a bunch of fetching, and we would need to pass
the params to it. I added a new method instead. This is the appropriate time to
call it because WorkOnTask() is what computes whether or not a task is completed
since it's the main run loop in InstallableManager.

[pkotwicz, 2017/03/31 04:12:37]

I still think that RunCallback() is the correct location for this code.
WorkOnTask() also does a whole bunch of work

RunCallback() is passed a InstallableManager::Task which is:
std::pair<InstallableParams, InstallableCallback>

[dominickn, 2017/03/31 04:59:17]

Acknowledged. I prefer it here as WorkOnTask is the main eval-dispatch loop.

     tasks_.erase(tasks_.begin());
     StartNextTask();
     return;
   }
 
   if (!manifest_->fetched) {
     FetchManifest();
   } else if (params.check_installable && !installable_->fetched) {
     CheckInstallable();
   } else if (params.fetch_valid_primary_icon &&
@@ skipping 192 matching lines @@
   return manifest_->url;
 }
 
 const content::Manifest& InstallableManager::manifest() const {
   return manifest_->manifest;
 }
 
 bool InstallableManager::is_installable() const {
   return installable_->installable;
 }