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..df25f0ae3f44f70b93e02b7a4c9d419dee90dc4a 100644
--- a/chrome/browser/upgrade_detector_impl.cc
+++ b/chrome/browser/upgrade_detector_impl.cc
@@ -7,28 +7,33 @@
 #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 "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 +51,13 @@ const int kNotifyCycleTimeMs = 20 * 60 * 1000;  // 20 minutes.
 // Same as kNotifyCycleTimeMs but only used during testing.
 const int kNotifyCycleTimeForTestingMs = 500;  // Half a second.
 
+// The number of days after which we identify a build/install as outdated.
+const uint64 kOutdatedBuildAgeInDays = 12 * 7;
+
+// Finch Experiment strings to identify if we should check for outdated install.
+const char kOutdatedInstallCheckTrialName[] = "OutdatedInstallCheck";
+const char kOutdatedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls";
+
 std::string CmdLineInterval() {
   const CommandLine& cmd_line = *CommandLine::ForCurrentProcess();
   return cmd_line.GetSwitchValueASCII(switches::kCheckForUpdateIntervalSec);
@@ -54,7 +66,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,12 +82,104 @@ 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());
+}
+
+// This task checks the update policy and calls back the task only if automatic
+// updates are allowed.
+void DetectUpdatability(const base::Closure& start_timer) {

[Finnur, 2013/02/04 11:12:04]

s/start_timer/callback_task/
(or |task|, or something)

[MAD, 2013/02/04 16:01:29]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  string16 app_guid = installer::GetAppGuidForUpdates(IsSystemInstall());
+  DCHECK(!app_guid.empty());
+  if (GoogleUpdateSettings::AUTOMATIC_UPDATES ==
+      GoogleUpdateSettings::GetAppUpdatePolicy(app_guid, NULL)) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, start_timer);
+  }
+}
+#endif  // defined(OS_WIN)
+
+}  // namespace
+
+UpgradeDetectorImpl::UpgradeDetectorImpl()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
+      is_unstable_channel_(false),
+      build_date_(base::GetBuildTime()) {
+  CommandLine command_line(*CommandLine::ForCurrentProcess());
+  // The different command line switches that affect testing can't be used
+  // simultaneously, if they do, here's the precedence order, based on the order
+  // of the if statements below:
+  // - kDisableBackgroundNetworking prevents any of the other command line
+  //   switch from being taken into account.
+  // - kSimulateUpgrade supersedes critical or outdated upgrade switches.
+  // - kSimulateCriticalUpdate has precedence over kSimulateOutdated.
+  // - kSimulateOutdated can work on its own, or with a specified date.
+  if (command_line.HasSwitch(switches::kDisableBackgroundNetworking))
+    return;
+  if (command_line.HasSwitch(switches::kSimulateUpgrade)) {
+    UpgradeDetected(UPGRADE_AVAILABLE_REGULAR, is_unstable_channel_);
+    return;
+  }
+  if (command_line.HasSwitch(switches::kSimulateCriticalUpdate)) {
+    UpgradeDetected(UPGRADE_AVAILABLE_CRITICAL, is_unstable_channel_);
+    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.
+    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;
+      StartCheckForUpgradeTimer();
+    } else {
+      // Without a valid date, we simulate that we are already outdated...
+      UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL, is_unstable_channel_);
+    }
+    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_POSIX)
+#if defined(OS_MACOSX)
+  if (keystone_glue::KeystoneEnabled())
+#endif
+    StartCheckForUpgradeTimer();
+#elif defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+  // On Windows, there might be a policy preventing updates, so validate
+  // updatability, and then call StartCheckForUpgradeTimer appropriately.
+  base::Closure callback_task =
+      base::Bind(&UpgradeDetectorImpl::StartCheckForUpgradeTimer,
+                 weak_factory_.GetWeakPtr());
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&DetectUpdatability,
+                                     callback_task));
+#endif  // defined(OS_POSIX)
+}

[Finnur, 2013/02/04 11:12:04]

I like where this is heading, but can you replace lines 157-171 with:

#if defined(OS_WINDOW) && defined(GOOGLE_CHROME_BUILD)
  [Do what you do on line 163-170 above (post the task)]
  return;  [THIS IS IMPORTANT!] :)
#elif defined(OS_WINDOW) && !defined(GOOGLE_CHROME_BUILD)
  return;  // Chromium has no upgrade channel.
#elif defined(OS_MACOSX)
  if (!keystone_glue::KeystoneEnabled())
    return;  // Keystone updater not enabled.
#elif !defined(OS_POSIX)
  return;
#endif

  StartCheckForUpgradeTimer();
}

This should be functionally equivalent but is so much easier to read and
understand.

[MAD, 2013/02/04 16:01:29]

Agreed, much better, thanks a lot! :-)
Done!

+
+UpgradeDetectorImpl::~UpgradeDetectorImpl() {
+}
+
+// Static
 // 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.
-void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
-                       bool* is_unstable_channel,
-                       bool* is_critical_upgrade) {
+// installed version. If the installed version is newer, it calls back
+// UpgradeDetectorImpl::UpgradeDetected using |upgrade_detector|.
+void UpgradeDetectorImpl::DetectUpgradeTask(
+    base::WeakPtr<UpgradeDetectorImpl> upgrade_detector) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   Version installed_version;
@@ -83,14 +189,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.
@@ -118,8 +217,8 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
 #endif
 
   chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
-  *is_unstable_channel = channel == chrome::VersionInfo::CHANNEL_DEV ||
-                         channel == chrome::VersionInfo::CHANNEL_CANARY;
+  bool is_unstable_channel = channel == chrome::VersionInfo::CHANNEL_DEV ||
+                             channel == chrome::VersionInfo::CHANNEL_CANARY;
 
   // Get the version of the currently *running* instance of Chrome.
   chrome::VersionInfo version_info;
@@ -140,64 +239,78 @@ void DetectUpgradeTask(const base::Closure& upgrade_detected_task,
       (installed_version.CompareTo(running_version) > 0)) {
     // If a more recent version is available, it might be that we are lacking
     // a critical update, such as a zero-day fix.
-    *is_critical_upgrade =
-        critical_update.IsValid() &&
-        (critical_update.CompareTo(running_version) > 0);
+    UpgradeAvailable upgrade_available = UPGRADE_AVAILABLE_REGULAR;
+    if (critical_update.IsValid() &&
+        critical_update.CompareTo(running_version) > 0) {
+      upgrade_available = UPGRADE_AVAILABLE_CRITICAL;
+    }
 
     // Fire off the upgrade detected task.
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            upgrade_detected_task);
+                            base::Bind(&UpgradeDetectorImpl::UpgradeDetected,
+                                       upgrade_detector,
+                                       upgrade_available,
+                                       is_unstable_channel));
   }
 }
 
-}  // namespace
-
-UpgradeDetectorImpl::UpgradeDetectorImpl()
-    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      is_unstable_channel_(false) {
-  CommandLine command_line(*CommandLine::ForCurrentProcess());
-  if (command_line.HasSwitch(switches::kDisableBackgroundNetworking))
-    return;
-  if (command_line.HasSwitch(switches::kSimulateUpgrade)) {
-    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())
-#endif
-  {
-    detect_upgrade_timer_.Start(FROM_HERE,
-        base::TimeDelta::FromMilliseconds(GetCheckForUpgradeEveryMs()),
-        this, &UpgradeDetectorImpl::CheckForUpgrade);
-  }
-#endif
-}
-
-UpgradeDetectorImpl::~UpgradeDetectorImpl() {
+void UpgradeDetectorImpl::StartCheckForUpgradeTimer() {

[Finnur, 2013/02/04 11:12:04]

Suggest renaming: StartTimerForUpgradeCheck
Not only is it less ambiguous, it means you can search for 'CheckForUpgrade' and
not find this function definition nor its call sites.

[MAD, 2013/02/04 16:01:29]

Done.

+  detect_upgrade_timer_.Start(FROM_HERE,
+      base::TimeDelta::FromMilliseconds(GetCheckForUpgradeEveryMs()),
+      this, &UpgradeDetectorImpl::CheckForUpgrade);
 }
 
 void UpgradeDetectorImpl::CheckForUpgrade() {
+  // No need to look fur upgrades if the install is outdated.

[Finnur, 2013/02/04 11:12:04]

s/für/for/ :)

[MAD, 2013/02/04 16:01:29]

Done.

+  if (CheckForOutdatedInstall())

[Finnur, 2013/02/04 11:12:04]

Suggest renaming this function: IsOutdatedInstall()

However, there is a bit of a problem here as the |is_unstable_channel_| variable
is not valid at this point because it is set in DetectUpgradeTask which doesn't
get called until line 274. 

This will lead to the wrong clause being selected in the if statement on line
339 (when CheckForOutdatedInstall calls UpgradeDetected).

I suspect you need to move the CheckForOutdatedInstall call to the
DetectUpgradeTask. It means you need to jump to the UI thread to call
UpgradeDetected, but that task already does so, so you can maybe leverage that
instead of calling UpgradeDetected on line 301.

[MAD, 2013/02/04 16:01:29]

OK, I think I agree with you... I was trying to do the outdated check in the UI
thread since it doesn't need to be done on the file thread. And I ignored the
issue of the unstable channel because in the case of the outdated install, the
code is the same whether is_unstable_channel is true or false, we end up in the
UPGRADE_ANNOYANCE_CRITICAL state either way... But things might change in the
future and so we might as well make it more clear, at the price at going to the
file thread for the outdated check even if we don't really need. Although it
will add complexity on the sane/network time provider which will need to support
being called from the file thread... :-/


Another way to avoid this would be to set is_unstable_channel earlier, since it
only needs to be done once, we could do it at the same time as the policy check,
but since the policy check is not done on all platform combination, this makes
it a bit more complex... I'll see if I can find something clean in that
direction...

[MAD, 2013/02/04 17:17:02]

So after trying a few different things, I couldn't find a simple way to make it
work with the check for outdated install being done on the file thread. I think
doing it in the UI thread like this is a cleaner solution after all... Let me
know if you disagree...

[Finnur, 2013/02/04 21:30:17]

I'll take a closer look at the code tomorrow morning (just wanted to interject
something)...

I think moving the check for unstable channel further up is fine. Perhaps a
little more complex than what I thought of after reading this. If I would have
seen the comments earlier, I would have suggested changing the param for
UpgradeDetected to fast_track, or something, to decouple it from
unstable_channel. Then you could have hard coded that param to false (or true)
instead of passing in is_unstable_channel_ (which isn't really initialized yet
and looks like it shouldn't be used at that point). The places that have the
right value for unstable_channel would of course be able to keep using that
value.

But this is probably fine too. You still have time if you want to make changes
(or longer if you ask me to hold off on reviewing). Otherwise I'll assume this
is the way to go. I'll take a better look tomorrow with a less fried brain. :)

I'd still like the UpgradeDetected call moved out of the CheckForOutdatedInstall
function though.

[Finnur, 2013/02/05 10:52:19]

Might be a moot point, but if this is a concern then you can always get the sane
time in the ctor on the UI thread, or better yet: do it at
StartCheckForUpgradeTimer, which also runs on the UI thread (and runs once
only).

[MAD, 2013/02/05 16:49:16]

But for people not restarting Chrome very often, we want to keep checking for
the current time. So we don't want to do it just once.

[Finnur, 2013/02/05 22:14:50]

Yes, you are right. You'd have to keep track of the delta, which is overkill.
This is fine.

+    return;
+
   weak_factory_.InvalidateWeakPtrs();
-  base::Closure callback_task =
-      base::Bind(&UpgradeDetectorImpl::UpgradeDetected,
-                 weak_factory_.GetWeakPtr());
   // We use FILE as the thread to run the upgrade detection code on all
   // platforms. For Linux, this is because we don't want to block the UI thread
   // while launching a background process and reading its output; on the Mac and
   // on Windows checking for an upgrade requires reading a file.
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DetectUpgradeTask,
-                                     callback_task,
-                                     &is_unstable_channel_,
-                                     &is_critical_upgrade_));
+                          base::Bind(&UpgradeDetectorImpl::DetectUpgradeTask,
+                                     weak_factory_.GetWeakPtr()));
+}
+
+bool UpgradeDetectorImpl::CheckForOutdatedInstall() {
+  // Only enable the outdated install check if we are running the trial for it,
+  // unless we are simulating an outdated isntall.
+  static bool simulate_outdated = CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kSimulateOutdated);
+  if (base::FieldTrialList::FindFullName(kOutdatedInstallCheckTrialName) !=
+          kOutdatedInstallCheck12WeeksGroupName && !simulate_outdated) {
+    return false;
+  }
+
+  base::Time network_time(base::Time::Now());
+// TODO(mad): Uncomment when variations service NetworkTime CL get committed.
+//  if (!g_browser_process->variations_service()->GetNetworkTime(&network_time))
+//    return;
+
+  if (network_time.is_null() || build_date_.is_null() ||
+      build_date_ > network_time) {
+    NOTREACHED();
+    return false;
+  }
+
+  if (network_time - build_date_ >
+      base::TimeDelta::FromDays(kOutdatedBuildAgeInDays)) {
+    UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL, is_unstable_channel_);

[Finnur, 2013/02/04 11:12:04]

It would probably be cleaner to have this function just check and not notify
(and move this line (line 301) over to line 266 (before the return)). But see my
comment on line 265 first. Maybe you can just get rid of this call (from this
function).

[MAD, 2013/02/04 16:01:29]

This makes it much more complex to support simulation of outdated builds on non
CHROME builds, which need to avoid doing the rest of the work in
CheckForUpgrade, yet still want to check for the date... I'll see if I can find
a way to still support that... somehow... :-/

[MAD, 2013/02/04 17:17:02]

Didn't find anything I was happy with.

+    return true;
+  }
+  // If we simlated an oputdated install with a date, we don't want to keep

[Finnur, 2013/02/04 11:12:04]

s/oputdated/outdated/

[MAD, 2013/02/04 16:01:29]

Done.

+  // checking for version upgrades, which happens on non-official builds.
+  return simulate_outdated;
 }
 
-void UpgradeDetectorImpl::UpgradeDetected() {
+void UpgradeDetectorImpl::UpgradeDetected(UpgradeAvailable upgrade_available,
+                                          bool is_unstable_channel) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  is_unstable_channel_ = is_unstable_channel;
+  upgrade_available_ = upgrade_available;
 
   // Stop the recurring timer (that is checking for changes).
   detect_upgrade_timer_.Stop();
@@ -222,13 +335,14 @@ void UpgradeDetectorImpl::NotifyOnUpgrade() {
   bool is_testing = IsTesting();
   int64 time_passed = is_testing ? delta.InSeconds() : delta.InHours();
 
+  bool is_critical_or_outdated = upgrade_available_ > UPGRADE_AVAILABLE_REGULAR;
   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_outdated)
       set_upgrade_notification_stage(UPGRADE_ANNOYANCE_CRITICAL);
     else if (time_passed >= kUnstableThreshold) {
       set_upgrade_notification_stage(UPGRADE_ANNOYANCE_LOW);
@@ -247,10 +361,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_outdated) {
       set_upgrade_notification_stage(
-          is_critical_upgrade_ ? UPGRADE_ANNOYANCE_CRITICAL :
-                                 UPGRADE_ANNOYANCE_SEVERE);
+          is_critical_or_outdated ? UPGRADE_ANNOYANCE_CRITICAL :
+                                    UPGRADE_ANNOYANCE_SEVERE);
 
       // We can't get any higher, baby.
       upgrade_notification_timer_.Stop();