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

chrome/browser/installable/installable_manager.cc

Index: chrome/browser/installable/installable_manager.cc
diff --git a/chrome/browser/installable/installable_manager.cc b/chrome/browser/installable/installable_manager.cc
index b0bf4496513b77bdd42977cfe2e53435a498d2dc..9199d8eb3903e6ce4e9eea01ad92fed9cbfc5c8f 100644
--- a/chrome/browser/installable/installable_manager.cc
+++ b/chrome/browser/installable/installable_manager.cc
@@ -61,6 +61,17 @@ bool DoesManifestContainRequiredIcon(const content::Manifest& manifest) {
   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);
@@ -94,13 +105,15 @@ struct InstallableManager::IconProperty {
   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;
 
@@ -140,6 +153,22 @@ void InstallableManager::GetData(const InstallableParams& params,
   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,
@@ -232,6 +261,10 @@ void InstallableManager::Reset() {
   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());
 
@@ -291,8 +324,35 @@ void InstallableManager::WorkOnTask() {
   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();