Split the NOT_STARTED and STOPPED_BEFORE_COMPLETION installability metrics.

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 127 matching lines @@
                                  const InstallableCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // 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;
+  if (page_status_ == InstallabilityCheckStatus::NOT_STARTED)
+    page_status_ = InstallabilityCheckStatus::NOT_COMPLETED;
   StartNextTask();
 }
 
 void InstallableManager::RecordMenuOpenHistogram() {
   if (is_pwa_check_complete_)
     InstallableMetrics::RecordMenuOpenHistogram(page_status_);
   else
     ++menu_open_count_;
 }
 
 void InstallableManager::RecordMenuItemAddToHomescreenHistogram() {
   if (is_pwa_check_complete_)
     InstallableMetrics::RecordMenuItemAddToHomescreenHistogram(page_status_);
   else
     ++menu_item_add_to_homescreen_count_;
 }
 
 void InstallableManager::RecordQueuedMetricsOnTaskCompletion(
     const InstallableParams& params,
     bool check_passed) {
-  // Don't do anything if we've already finished the PWA check or if this check
-  // was not for the full PWA params. Once a full check is completed, metrics
-  // will be directly recorded in Record*Histogram since
-  // |is_pwa_check_complete_| will be true.
-  if (is_pwa_check_complete_ || !IsParamsForPwaCheck(params))
+  // Don't do anything if we've:
+  //  - already finished the PWA check, or
+  //  - we passed the check AND it was not for the full PWA params.
+  // In the latter case, we know if we passed the check and we weren't checking
+  // the full params, we don't yet know if the site is installable. However, any

[benwells, 2017/05/04 05:05:13]

This comment is confusing - it's basically saying in some cases we know that we
don't yet know. Could you rephrase it to be less confusing, e.g. "In the latter
case, (i.e. the check passed but we weren't checking everything), we don't yet
know if the site is installable. However if the check didn't pass we know for
sure the site isn't installable, regardless of how much we checked"

[dominickn, 2017/05/04 05:18:56]

Done.

+  // failure means that we will never pass the check.
+  //
+  // Once a full check is completed, metrics will be directly recorded in
+  // Record*Histogram since |is_pwa_check_complete_| will be true.
+  if (is_pwa_check_complete_ || (check_passed && !IsParamsForPwaCheck(params)))
     return;
 
   is_pwa_check_complete_ = true;
   page_status_ =
       check_passed
           ? InstallabilityCheckStatus::COMPLETE_PROGRESSIVE_WEB_APP
           : InstallabilityCheckStatus::COMPLETE_NON_PROGRESSIVE_WEB_APP;
 
   // Compute what the status would have been for any queued calls to
   // Record*Histogram, and record appropriately.
@@ skipping 105 matching lines @@
              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();
 
   // We may have reset prior to completion, in which case |menu_open_count_| or
-  // |menu_item_add_to_homescreen_count_| will be nonzero. If we completed, then
-  // these values cannot be anything except 0.
-  page_status_ = InstallabilityCheckStatus::NOT_STARTED;
+  // |menu_item_add_to_homescreen_count_| might be nonzero and |page_status_| is
+  // one of NOT_STARTED or NOT_COMPLETED. If we completed, then these values
+  // cannot be anything except 0.
   is_pwa_check_complete_ = false;
 
-  for (; menu_open_count_ > 0; --menu_open_count_) {
-    InstallableMetrics::RecordMenuOpenHistogram(
-        InstallabilityCheckStatus::STOPPED_BEFORE_COMPLETION);
-  }
+  for (; menu_open_count_ > 0; --menu_open_count_)
+    InstallableMetrics::RecordMenuOpenHistogram(page_status_);
 
   for (; menu_item_add_to_homescreen_count_ > 0;
        --menu_item_add_to_homescreen_count_) {
-    InstallableMetrics::RecordMenuItemAddToHomescreenHistogram(
-        InstallabilityCheckStatus::STOPPED_BEFORE_COMPLETION);
+    InstallableMetrics::RecordMenuItemAddToHomescreenHistogram(page_status_);
   }
 
+  page_status_ = InstallabilityCheckStatus::NOT_STARTED;
   manifest_.reset(new ManifestProperty());
   installable_.reset(new InstallableProperty());
 
   is_active_ = false;
 }
 
 void InstallableManager::SetManifestDependentTasksComplete() {
   DCHECK(!tasks_.empty());
   const InstallableParams& params = tasks_[0].first;
 
@@ skipping 256 matching lines @@
   return manifest_->url;
 }
 
 const content::Manifest& InstallableManager::manifest() const {
   return manifest_->manifest;
 }
 
 bool InstallableManager::is_installable() const {
   return installable_->installable;
 }