Add an outdated upgrade bubble view.

chrome/browser/upgrade_detector_impl.cc

Index: chrome/browser/upgrade_detector_impl.cc
diff --git a/chrome/browser/upgrade_detector_impl.cc b/chrome/browser/upgrade_detector_impl.cc
index 570275e391ba59c8452c3de6d6861f7f4ead8e04..0ed122738348879be30a6b5516bc81932fc8ab31 100644
--- a/chrome/browser/upgrade_detector_impl.cc
+++ b/chrome/browser/upgrade_detector_impl.cc
@@ -7,28 +7,37 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/build_time.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
+#include "base/metrics/field_trial.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
-#include "base/time.h"
 #include "base/utf_string_conversions.h"
+#include "base/version.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
-#include "chrome/installer/util/browser_distribution.h"
 #include "content/public/browser/browser_thread.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_status.h"
 #include "ui/base/resource/resource_bundle.h"
 
 #if defined(OS_WIN)
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/install_util.h"
 #elif defined(OS_MACOSX)
 #include "chrome/browser/mac/keystone_glue.h"
 #elif defined(OS_POSIX)
 #include "base/process_util.h"
-#include "base/version.h"
 #endif
 
 using content::BrowserThread;
@@ -46,6 +55,13 @@ const int kNotifyCycleTimeMs = 20 * 60 * 1000;  // 20 minutes.
 // Same as kNotifyCycleTimeMs but only used during testing.
 const int kNotifyCycleTimeForTestingMs = 500;  // Half a second.
 
+// Default server of Variations seed info.

[Finnur, 2013/01/30 15:50:48]

Here is the 'Variations' comment also. Is that intentional? 

[MAD, 2013/01/31 21:31:42]

Damn... Really sorry about that, I should have been more careful with this copy
paste thingy... :-(

+const char kTimeServerURL[] = "https://www.google.com";
+const int kMaxRetryTimeFetch = 5;
+
+const char kOutadedInstallCheckTrialName[] = "OutadedInstallCheck";
+const char kOutadedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls";

[Finnur, 2013/01/30 15:50:48]

There are three cases of Outdaded misspellings here on these two lines. :)

[MAD, 2013/01/31 21:31:42]

Copy/Paste from hell!!! :-)

+
 std::string CmdLineInterval() {
   const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();
   return cmd_line.GetSwitchValueASCII(switches::kCheckForUpdateIntervalSec);
@@ -54,7 +70,9 @@ std::string CmdLineInterval() {
 bool IsTesting() {
   const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();
   return cmd_line.HasSwitch(switches::kSimulateUpgrade) ||
-      cmd_line.HasSwitch(switches::kCheckForUpdateIntervalSec);
+      cmd_line.HasSwitch(switches::kCheckForUpdateIntervalSec) ||
+      cmd_line.HasSwitch(switches::kSimulateCriticalUpdate) ||
+      cmd_line.HasSwitch(switches::kSimulateOutdated);
 }
 
 // How often to check for an upgrade.
@@ -68,6 +86,18 @@ int GetCheckForUpgradeEveryMs() {
   return kCheckForUpgradeMs;
 }
 
+#if defined(OS_WIN)
+bool IsSystemInstall() {
+  FilePath exe_path;
+  if (!PathService::Get(base::DIR_EXE, &exe_path)) {
+    NOTREACHED() << "Failed to find executable path";
+    return false;
+  }
+
+  return !InstallUtil::IsPerUserInstall(exe_path.value().c_str());
+}
+#endif
+
 // This task checks the currently running version of Chrome against the
 // installed version. If the installed version is newer, it runs the passed
 // callback task. Otherwise it just deletes the task.
@@ -83,14 +113,7 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
   // Get the version of the currently *installed* instance of Chrome,
   // which might be newer than the *running* instance if we have been
   // upgraded in the background.
-  FilePath exe_path;
-  if (!PathService::Get(base::DIR_EXE, &exe_path)) {
-    NOTREACHED() << "Failed to find executable path";
-    return;
-  }
-
-  bool system_install =
-      !InstallUtil::IsPerUserInstall(exe_path.value().c_str());
+  bool system_install = IsSystemInstall();
 
   // TODO(tommi): Check if using the default distribution is always the right
   // thing to do.
@@ -150,11 +173,26 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
   }
 }
 
+#if defined(OS_WIN)
+// This task checks the update policy and calls back the task only if automatic
+// updates are allowed.
+void DetectUpdatability(bool* is_auto_update_allowed) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  string16 app_guid = installer::GetAppGuidForUpdates(IsSystemInstall());
+  DCHECK(!app_guid.empty());
+  *is_auto_update_allowed = GoogleUpdateSettings::AUTOMATIC_UPDATES ==
+      GoogleUpdateSettings::GetAppUpdatePolicy(app_guid, NULL);
+}
+#endif  // defined(OS_WIN)
+
 }  // namespace
 
 UpgradeDetectorImpl::UpgradeDetectorImpl()
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      is_unstable_channel_(false) {
+      is_unstable_channel_(false),
+      is_outdated_reinstall_allowed_(false),
+      build_date_(base::GetBuildTime()) {
   CommandLine command_line(*CommandLine::ForCurrentProcess());
   if (command_line.HasSwitch(switches::kDisableBackgroundNetworking))
     return;
@@ -162,12 +200,40 @@ UpgradeDetectorImpl::UpgradeDetectorImpl()
     UpgradeDetected();
     return;
   }
+  if (command_line.HasSwitch(switches::kSimulateCriticalUpdate)) {
+    is_critical_upgrade_ = true;
+    UpgradeDetected();
+    return;
+  }
+  if (command_line.HasSwitch(switches::kSimulateOutdated)) {
+    // The outdated simulation can work without a value, which means outdated
+    // now, or with a value that must be a well formed date/time string that
+    // overrides the build date.
+    is_outdated_reinstall_allowed_ = true;
+    std::string build_date = command_line.GetSwitchValueASCII(
+        switches::kSimulateOutdated);
+    base::Time maybe_build_time;
+    bool result = base::Time::FromString(build_date.c_str(), &maybe_build_time);
+    if (result && !maybe_build_time.is_null()) {
+      // We got a valid build date simulation so use it and check for upgrades.
+      build_date_ = maybe_build_time;
+      detect_upgrade_timer_.Start(FROM_HERE,
+          base::TimeDelta::FromMilliseconds(GetCheckForUpgradeEveryMs()),
+          this, &UpgradeDetectorImpl::CheckForUpgrade);
+    } else {
+      // Without a valid date, we simulate that we are already outdated...
+      is_outdated_install_ = true;
+      UpgradeDetected();
+    }
+    return;
+  }
   // Windows: only enable upgrade notifications for official builds.
   // Mac: only enable them if the updater (Keystone) is present.
   // Linux (and other POSIX): always enable regardless of branding.
 #if (defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)) || defined(OS_POSIX)
 #if defined(OS_MACOSX)
-  if (keystone_glue::KeystoneEnabled())
+  is_outdated_reinstall_allowed_ = keystone_glue::KeystoneEnabled();
+  if (is_outdated_reinstall_allowed_)
 #endif
   {
     detect_upgrade_timer_.Start(FROM_HERE,
@@ -175,12 +241,40 @@ UpgradeDetectorImpl::UpgradeDetectorImpl()
         this, &UpgradeDetectorImpl::CheckForUpgrade);
   }
 #endif
+
+  // Only enable the outdated install check if we are running the trial for it.
+  if (base::FieldTrialList::FindFullName(kOutadedInstallCheckTrialName) !=
+      kOutadedInstallCheck12WeeksGroupName) {
+    is_outdated_reinstall_allowed_ = false;
+    return;
+  }
+
+// On Windows, there might be a policy preventing updates,
+// so validate updatability to set |is_outdated_reinstall_allowed_|.

[Finnur, 2013/01/30 15:50:48]

I don't understand the second line of this comment.
Specifically, 'validate to set' is hard to grok.
Is this a case of s/to/and/ ?

Also, there was more room on line 252 -- you didn't need to move that many words
to the next line. :)

[MAD, 2013/01/31 21:31:42]

Done.

+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&DetectUpdatability,
+                                     &is_outdated_reinstall_allowed_));
+#elif !(defined(OS_MACOSX) || defined(OS_CHROME))
+  // All other devices are always allowed, except OS_MACOSX which sets it above,
+  // and Chrome OS which doesn't support the reinstall bubble.
+  is_outdated_reinstall_allowed_ = true;

[Finnur, 2013/01/30 15:50:48]

If we ever remove the field trial guard, then this will start returning true for
Chromium...

[MAD, 2013/01/31 21:31:42]

D'Ho!

Added  && defined(OS_POSIX) which is || to WIN && OFFICIAL above.

+#endif
 }
 
 UpgradeDetectorImpl::~UpgradeDetectorImpl() {
 }
 
 void UpgradeDetectorImpl::CheckForUpgrade() {
+  if (is_outdated_reinstall_allowed_ && !is_outdated_install_)
+    CompareBuildTimeToSaneTime();
+
+  // If we are simulating an outdated install, no need to detect an upgrade.
+  // Since upgrade has precedence over outdated installs, it would prevent
+  // testing.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSimulateOutdated))
+    return;

[Finnur, 2013/01/30 15:50:48]

Can we DCHECK or something if tricky combinations are given (e.g.
kSimulateOutdated and kSimulate[Critical]Update)?

[MAD, 2013/01/31 21:31:42]

Done.

+
   weak_factory_.InvalidateWeakPtrs();
   base::Closure callback_task =
       base::Bind(&UpgradeDetectorImpl::UpgradeDetected,
@@ -196,6 +290,35 @@ void UpgradeDetectorImpl::CheckForUpgrade() {
                                      &is_critical_upgrade_));
 }
 
+void UpgradeDetectorImpl::CompareBuildTimeToSaneTime() {

[Finnur, 2013/01/30 15:50:48]

Check the design doc for a proposal that does not involve a network request in
the vast majority of the cases.

[MAD, 2013/01/31 21:31:42]

Done.

+  DCHECK(is_outdated_reinstall_allowed_);
+  // Check again, in case we got posted more than once by CheckForUpgrade.
+  if (is_outdated_install_)
+    return;
+  // Only fetch time from server when it has not been done already.
+  if (server_time_.is_null()) {
+    if (!pending_time_request_.get()) {
+      pending_time_request_.reset(net::URLFetcher::Create(
+          GURL(kTimeServerURL), net::URLFetcher::GET, this));
+      pending_time_request_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                                          net::LOAD_DO_NOT_SAVE_COOKIES);
+      pending_time_request_->SetRequestContext(
+          g_browser_process->system_request_context());
+      pending_time_request_->SetMaxRetriesOn5xx(kMaxRetryTimeFetch);

[Finnur, 2013/01/30 15:50:48]

I don't think we should be retry-ing on 500 errors. The value you are after
isn't super critical. If it fails, so be it. We'll probably catch it on the next
startup.

[MAD, 2013/01/31 21:31:42]

Done.

+      pending_time_request_->Start();
+    }
+  } else {
+    // The current sane time is the server time offset by elasped ticks.
+    base::Time sane_time = server_time_ + (base::TimeTicks::Now() -
+                                           server_time_ticks_);
+    // TODO(mad): Add server side control of the time delta.

[Finnur, 2013/01/30 15:50:48]

What does this mean?

[MAD, 2013/01/31 21:31:42]

It was about a discussion we had about changing the 12 weeks delta from a value
returned from a server (e.g., a Finch experiment) but we decided to only use
Finch as an on/off experiment instead.

+    if (sane_time - build_date_ > base::TimeDelta::FromDays(12 * 7)) {

[Finnur, 2013/01/30 15:50:48]

nit: Add constant kOutdatedInDays, or something, at top?

[MAD, 2013/01/31 21:31:42]

Done.

+      is_outdated_install_ = true;
+      UpgradeDetected();
+    }
+  }
+}
+
 void UpgradeDetectorImpl::UpgradeDetected() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -222,13 +345,14 @@ void UpgradeDetectorImpl::NotifyOnUpgrade() {
   bool is_testing = IsTesting();
   int64 time_passed = is_testing ? delta.InSeconds() : delta.InHours();
 
+  bool is_critical_or_oudated = is_critical_upgrade_ || is_outdated_install_;
   if (is_unstable_channel_) {
     // There's only one threat level for unstable channels like dev and
     // canary, and it hits after one hour. During testing, it hits after one
     // minute.
     const int kUnstableThreshold = 1;
 
-    if (is_critical_upgrade_)
+    if (is_critical_or_oudated)
       set_upgrade_notification_stage(UPGRADE_ANNOYANCE_CRITICAL);
     else if (time_passed >= kUnstableThreshold) {
       set_upgrade_notification_stage(UPGRADE_ANNOYANCE_LOW);
@@ -247,10 +371,10 @@ void UpgradeDetectorImpl::NotifyOnUpgrade() {
     const int kLowThreshold = 2 * kMultiplier;
 
     // These if statements must be sorted (highest interval first).
-    if (time_passed >= kSevereThreshold || is_critical_upgrade_) {
+    if (time_passed >= kSevereThreshold || is_critical_or_oudated) {
       set_upgrade_notification_stage(
-          is_critical_upgrade_ ? UPGRADE_ANNOYANCE_CRITICAL :
-                                 UPGRADE_ANNOYANCE_SEVERE);
+          is_critical_or_oudated ? UPGRADE_ANNOYANCE_CRITICAL :
+                                   UPGRADE_ANNOYANCE_SEVERE);
 
       // We can't get any higher, baby.
       upgrade_notification_timer_.Stop();
@@ -268,6 +392,22 @@ void UpgradeDetectorImpl::NotifyOnUpgrade() {
   NotifyUpgradeRecommended();
 }
 
+void UpgradeDetectorImpl::OnURLFetchComplete(const net::URLFetcher* source) {
+  // The fetcher will be deleted when the request is handled.
+  scoped_ptr<const net::URLFetcher> request(pending_time_request_.release());
+  if (request->GetStatus().status() == net::URLRequestStatus::SUCCESS) {
+    base::Time response_date;
+    if (request->GetResponseHeaders()->GetDateValue(&response_date)) {
+      DCHECK(!response_date.is_null() && response_date >= build_date_);
+      server_time_ = response_date;
+      server_time_ticks_ = base::TimeTicks::Now();
+    }
+  }
+  // If |server_time_| was successfully set, this will compare it to
+  // |build_time_|, otherwise, it will try again.
+  CompareBuildTimeToSaneTime();

[Finnur, 2013/01/30 15:50:48]

Wow! Holy Duncan Ferguson! Stop the press! :)

This will be an infinite loop on errors. If people are unhappy with pinging,
they'll be super unhappy with infinite pings on failure. :)

I'd say, if server time is not set, abort and just try again next startup. We
need to let this fail.

See also my suggestion on the design doc to drastically reduce the number of
times we need to do a ping.

[MAD, 2013/01/31 21:31:42]

Triple D'Ho!... I often feel stupid when I write bad code... This is one of
these cases... :-(

[Finnur, 2013/02/01 17:09:15]

No worries. We've all been there. :)

On 2013/01/31 21:31:42, MAD wrote:

+}
+
 // static
 UpgradeDetectorImpl* UpgradeDetectorImpl::GetInstance() {
   return Singleton<UpgradeDetectorImpl>::get();