Switch about box to web ui on Windows.

chrome/browser/ui/webui/help/version_updater_win.cc

+// Use of this source code is governed by a BSD-style license that can be
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/webui/help/version_updater_win.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/version.h"
+#include "base/win/windows_version.h"
+#include "base/win/win_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/common/chrome_version_info.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/install_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/user_metrics.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/views/widget/widget.h"
+
+using content::BrowserThread;
+using content::UserMetricsAction;
+
+namespace {
+
+views::Widget* GetWidget() {
+  // Find an active browser so we can return it's native window as a widget.
+  Browser* browser = NULL;
+  for (BrowserList::const_iterator browser_iterator = BrowserList::begin();
+       browser_iterator != BrowserList::end(); browser_iterator++) {
+    if ((*browser_iterator)->window()) {
+      browser = *browser_iterator;
+      break;
+    }
+  }
+  if (!browser)
+    return NULL;
+  return views::Widget::GetWidgetForNativeWindow(
+      browser->window()->GetNativeWindow());
+}

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

This will find any browser object, not necessarily the one that initiated the
about box.  That's OK?

[MAD, 2012/07/05 20:50:41]

Yes, it's OK, all we need is A browser window... This will be needed by the
elevation prompt which will take over the whole desktop anyway...

+
+// We use this class to read the version of the FILE thread and then call back

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

of --> on ?

[MAD, 2012/07/05 20:50:41]

Done.

+// the version updater in the UI thread. We use a class so that we can control
+// the lifespan of the Version independently of the lifespan of the version
+// updater, which may die while asynchonicity is happening, thus the usage of
+// the WeakPtr, which can only use from the thread that created it.

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

use --> be used ?

[MAD, 2012/07/05 20:50:41]

Done.

+class VersionReader
+    : public base::RefCountedThreadSafe<VersionReader> {
+ public:
+  explicit VersionReader(base::WeakPtr<VersionUpdaterWin> version_updater)

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

const ref?

[MAD, 2012/07/05 20:50:41]

Done.

+      : version_updater_(version_updater) {
+  }

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

add DCHECK to make sure we are on UI thread?

[MAD, 2012/07/05 20:50:41]

Actually, we don't really care where this one comes from... The important thing
is that we send to FILE and come back to UI...

+
+  void GetVersionFromFileThread() {
+    BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+    installed_version_.reset(InstallUtil::GetChromeVersion(dist, false));
+    if (!installed_version_.get()) {
+      // User-level Chrome is not installed, check system-level.
+      installed_version_.reset(InstallUtil::GetChromeVersion(dist, true));
+    }
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
+          &VersionReader::SetVersionInUIThread, this));

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

if installed_version_ is still null at line 64, is it still worth posting the
task?  I say this because GotInstalledVersion() will ignore it in that case.

[MAD, 2012/07/05 20:50:41]

It doesn't actually ignore, it still makes a call to callback_, but use a
different status.

+  }
+
+  void SetVersionInUIThread() {
+    if (version_updater_.get() != NULL)
+      version_updater_->GotInstalledVersion(installed_version_.get());
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<VersionReader>;
+
+  base::WeakPtr<VersionUpdaterWin> version_updater_;
+  scoped_ptr<Version> installed_version_;
+};
+
+}  // namespace
+
+VersionUpdater* VersionUpdater::Create() {
+  return new VersionUpdaterWin;
+}
+
+VersionUpdaterWin::VersionUpdaterWin()
+    : google_updater_(new GoogleUpdate()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+  google_updater_->set_status_listener(this);
+}
+
+VersionUpdaterWin::~VersionUpdaterWin() {
+  // The Google Updater will hold a pointer to us until it reports status, so we
+  // need to let it know that we will no longer be listening.
+  if (google_updater_)
+    google_updater_->set_status_listener(NULL);
+}
+
+void VersionUpdaterWin::CheckForUpdate(const StatusCallback& callback) {
+  callback_ = callback;
+
+  // On-demand updates for Chrome don't work in Vista RTM when UAC is turned
+  // off. So, in this case we just want the About box to not mention
+  // on-demand updates. Silent updates (in the background) should still
+  // work as before - enabling UAC or installing the latest service pack
+  // for Vista is another option.
+  if (!(base::win::GetVersion() == base::win::VERSION_VISTA &&
+        (base::win::OSInfo::GetInstance()->service_pack().major == 0) &&
+        !base::win::UserAccountControlIsEnabled())) {

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

indent line 109 4 more spaces?

do you need to check for vista and later OS?

[MAD, 2012/07/05 20:50:41]

Why? It's at the same logic level as 108, isn't it?
Based on the comment above, we are only looking for a specific VISTA version
that had that specific problem.

+    // This could happen if we already got results, but the page got refreshed.
+    if (!google_updater_) {
+      google_updater_ = new GoogleUpdate();
+      google_updater_->set_status_listener(this);
+    }
+    UpdateStatus(UPGRADE_CHECK_STARTED, GOOGLE_UPDATE_NO_ERROR, string16());
+    google_updater_->CheckForUpdate(false, GetWidget());  // Don't upgrade yet.
+  }
+}
+
+void VersionUpdaterWin::RelaunchBrowser() const {
+  browser::AttemptRestart();
+}
+
+void VersionUpdaterWin::OnReportResults(
+    GoogleUpdateUpgradeResult result, GoogleUpdateErrorCode error_code,
+    const string16& error_message, const string16& version) {
+  // Drop the last reference to the object so that it gets cleaned up here.
+  google_updater_ = NULL;
+  UpdateStatus(result, error_code, error_message);
+}
+
+void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
+                                     GoogleUpdateErrorCode error_code,
+                                     const string16& error_message) {
+#if !defined(GOOGLE_CHROME_BUILD)
+  // For Chromium builds it would show an error message.
+  // But it looks weird because in fact there is no error,
+  // just the update server is not available for non-official builds.
+  return;
+#endif

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

this is strange.  why not reverse condition and put the entire method content
inside the #ifdef?

[MAD, 2012/07/05 20:50:41]

Simply copied from:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/ab...

+
+  Status status = UPDATED;
+  string16 message;
+
+  switch (result) {
+    case UPGRADE_CHECK_STARTED: {
+      content::RecordAction(UserMetricsAction("UpgradeCheck_Started"));
+      status = CHECKING;
+      break;
+    }
+    case UPGRADE_STARTED: {
+      content::RecordAction(UserMetricsAction("Upgrade_Started"));
+      status = UPDATING;
+      break;
+    }
+    case UPGRADE_IS_AVAILABLE: {
+      content::RecordAction(
+          UserMetricsAction("UpgradeCheck_UpgradeIsAvailable"));
+      DCHECK(!google_updater_);  // Should have been nulled out already.
+      google_updater_ = new GoogleUpdate();
+      google_updater_->set_status_listener(this);

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

lines 160 and 161 are repeated three times (if you include the ctor).  Would be
better to write a small helper function to do this.

[MAD, 2012/07/05 20:50:41]

Done.

+      UpdateStatus(UPGRADE_STARTED, GOOGLE_UPDATE_NO_ERROR, string16());
+      google_updater_->CheckForUpdate(true, GetWidget());  // Upgrade now.

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

is the comment valid for this line ?

[MAD, 2012/07/05 20:50:41]

Yes, this explains the passing of true...
Made it more explicit now, OK?

+      return;
+    }
+    case UPGRADE_ALREADY_UP_TO_DATE: {
+      // Google Update reported that Chrome is up-to-date.
+      // We need to confirm we are running the updated version.
+      // But we must defer version reading to the file thread...
+      // We'll handle the rest of this case within GotInstalledVersion below.
+      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
+            &VersionReader::GetVersionFromFileThread,
+            new VersionReader(weak_factory_.GetWeakPtr())));
+      return;
+    }
+    case UPGRADE_SUCCESSFUL: {
+      content::RecordAction(UserMetricsAction("UpgradeCheck_Upgraded"));
+      status = NEARLY_UPDATED;
+      break;
+    }
+    case UPGRADE_ERROR: {
+      content::RecordAction(UserMetricsAction("UpgradeCheck_Error"));
+      status = FAILED;
+      if (!error_message.empty()) {
+        message =
+            l10n_util::GetStringFUTF16(IDS_ABOUT_BOX_ERROR_DURING_UPDATE_CHECK,
+                                       error_message);
+      }
+      if (error_code != GOOGLE_UPDATE_DISABLED_BY_POLICY) {
+        message =
+            l10n_util::GetStringFUTF16Int(IDS_UPGRADE_ERROR, error_code);
+      } else {
+        message =
+            l10n_util::GetStringUTF16(IDS_UPGRADE_DISABLED_BY_POLICY);
+      }

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

I'm not sure I understand the logic of setting message.  Won't the value
assigned at line 105 always be overridden by the value assigned at either line
190 or 193?  Or is line 189 supposed to be an "else if"?

[MAD, 2012/07/05 20:50:41]

Good point, I don't know... :-)

This was stolen from:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/ab...

By someone else, and I guess he tried to merge both messages into a single
one... I'll see how I can fix that...

+      break;
+    }
+  }
+
+  // TODO(mad): Get proper progress value instead of passing 0.

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

open a bug to track this?

[MAD, 2012/07/05 20:50:41]

Done.

+  callback_.Run(status, 0, message);
+}
+
+void VersionUpdaterWin::GotInstalledVersion(Version* version) {

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

use const Version*

[MAD, 2012/07/05 20:50:41]

Done.

+  // Make sure that we are running the latest version and if not,
+  // notify the user by setting the status to NEARLY_UPDATED.
+  //
+  // The extra version check is necessary on Windows because the application
+  // may be already up to date on disk though the running app is still
+  // out of date.
+  chrome::VersionInfo version_info;
+  scoped_ptr<Version> running_version(
+      Version::GetVersionFromString(version_info.Version()));
+  if (!version || (version->CompareTo(*running_version) <= 0)) {
+    content::RecordAction(
+        UserMetricsAction("UpgradeCheck_AlreadyUpToDate"));
+    callback_.Run(UPDATED, 0, string16());
+  } else {
+    content::RecordAction(UserMetricsAction("UpgradeCheck_AlreadyUpgraded"));
+    callback_.Run(NEARLY_UPDATED, 0, string16());

[Roger Tawa OOO till Jul 10th, 2012/07/05 16:13:22]

The callback will be called in the file thread, whereas when it is called at
line 201 above it is called in the ui thread.  Is that ok?  Does the caller make
any assumption about which thread it is called in?  Would be worth adding to
documentation of base class VersionUpdater.

[MAD, 2012/07/05 20:50:41]

Here, we should be in the UI thread... The file thread posted back to the UI
thread after reading the version data from withih the FILE thread.

+  }
+}