Show online policy on CrOS EULA page

chrome/browser/ui/webui/about_ui.cc

Index: chrome/browser/ui/webui/about_ui.cc
diff --git a/chrome/browser/ui/webui/about_ui.cc b/chrome/browser/ui/webui/about_ui.cc
index b2dc2ae6a30a2ba23c4f74eac0c61843e6cecc07..4adddd5eae091cfe5df74e7c78438ff611f9059f 100644
--- a/chrome/browser/ui/webui/about_ui.cc
+++ b/chrome/browser/ui/webui/about_ui.cc
@@ -55,6 +55,9 @@
 #include "grit/locale_settings.h"
 #include "net/base/escape.h"
 #include "net/base/net_util.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/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/webui/jstemplate_builder.h"
@@ -96,6 +99,7 @@ const char kMemoryJsPath[] = "memory.js";
 const char kMemoryCssPath[] = "about_memory.css";
 const char kStatsJsPath[] = "stats.js";
 const char kStringsJsPath[] = "strings.js";
+const int kOnlineTermsTimeout = 5; // seconds

[Evan Stade, 2013/04/15 19:35:08]

two spaces before //. Also please document what this is rather than just the
unit.

[Nikita (slow), 2013/04/16 04:36:35]

nit: You could just rename it to kOnlineTermsTimeoutSec and change comment as
Evan suggested.

[vasilii, 2013/04/16 15:40:18]

Done.

 
 // When you type about:memory, it actually loads this intermediate URL that
 // redirects you to the final page. This avoids the problem where typing
@@ -137,6 +141,69 @@ class AboutMemoryHandler : public MemoryDetails {
 };
 
 #if defined(OS_CHROMEOS)
+// Helper class that fetches the online Chrome OS terms.
+class ChromeOSOnlineTermsHandler : public net::URLFetcherDelegate {
+ public:
+  typedef base::Callback<void (ChromeOSOnlineTermsHandler*)> FetchCallback;
+
+  explicit ChromeOSOnlineTermsHandler(const FetchCallback& callback):
+    fetch_callback_(callback) {

[battre, 2013/04/16 08:34:22]

nit:
: in next line, +2 indentation

[vasilii, 2013/04/16 15:40:18]

Done.

+    // TODO(vasilii): request a real EULA specific to locale.

[Nikita (slow), 2013/04/16 04:36:35]

Is this part of the upcoming CL? I think this should be trivial with the only
exception that some of the Chrome locale constants will not match those that are
used on server side.

[vasilii, 2013/04/16 15:40:18]

Added localization support. But URL is still fake (relevant to Desktop Chrome).
The real one is unavailable.

+    eula_fetcher_.reset(net::URLFetcher::Create(
+        GURL("https://www.google.com/intl/en/chrome/browser/"
+             "privacy/eula_text.html"), net::URLFetcher::GET, this));

[battre, 2013/04/16 08:34:22]

This should be an i18n-ized URL

[vasilii, 2013/04/16 15:40:18]

Done.

+    eula_fetcher_->SetRequestContext(
+        g_browser_process->system_request_context());
+    eula_fetcher_->AddExtraRequestHeader("Accept: text/html");
+    eula_fetcher_->Start();
+    // Abort the download attempt if it takes longer than one minute.

[Nikita (slow), 2013/04/16 04:36:35]

What would be UX during this time? Is there some feedback on UI? In case of
public accounts TOS there's "Loading" string.

Also 5 seconds seems a bit short timeframe for this.
10-15 seconds is preferable.

[vasilii, 2013/04/16 15:40:18]

Changed timeout to 10 sec. Added "Loading" message as TOS screen shows.

+    download_timer_.Start(FROM_HERE,
+                          base::TimeDelta::FromSeconds(kOnlineTermsTimeout),
+                          this,

[battre, 2013/04/16 08:34:22]

what's the lifetime of |this|? Is it guaranteed to survive until the callback is
triggered?

[vasilii, 2013/04/16 15:40:18]

Object destroys itself in either OnURLFetchComplete() or OnDownloadTimeout().
Both OneShotTimer and URLFetcher are destroyed and the second callback is
cancelled automatically.

+                          &ChromeOSOnlineTermsHandler::OnDownloadTimeout);
+  }
+
+  void GetResponseResult(std::string* response_string) {
+    std::string mime_type;
+    if (!eula_fetcher_ ||
+        !eula_fetcher_->GetStatus().is_success() ||
+        !eula_fetcher_->GetResponseHeaders()->GetMimeType(&mime_type) ||
+        mime_type != "text/html" ||
+        !eula_fetcher_->GetResponseAsString(response_string)) {
+      response_string->clear();
+    }
+  }
+
+ private:
+  // This object is to be allocated on the heap only.

[battre, 2013/04/16 08:34:22]

I don't understand this comment. I think what you want to say is that |this|
takes care of destruction.

[vasilii, 2013/04/16 15:40:18]

Done.

+  virtual ~ChromeOSOnlineTermsHandler() {}
+
+  // net::URLFetcherDelegate:
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE {
+    if (source != eula_fetcher_.get()) {
+      NOTREACHED() << "Callback from foreign URL fetcher";
+      return;
+    }
+    fetch_callback_.Run(this);
+    delete this;
+  }
+
+  void OnDownloadTimeout() {
+    eula_fetcher_.reset();
+    fetch_callback_.Run(this);
+    delete this;
+  }

[battre, 2013/04/16 08:34:22]

nit newline.

[vasilii, 2013/04/16 15:40:18]

Done.

+  // Timer that enforces a timeout on the attempt to download the
+  // ChromeOS Terms.
+  base::OneShotTimer<ChromeOSOnlineTermsHandler> download_timer_;

[battre, 2013/04/16 08:34:22]

nit: newline.

[vasilii, 2013/04/16 15:40:18]

Done.

+  // |fetch_callback_| called when fetching succeeded or failed.
+  FetchCallback fetch_callback_;

[battre, 2013/04/16 08:34:22]

nit: newline.

[vasilii, 2013/04/16 15:40:18]

Done.

+  // Helper to fetch online eula.
+  scoped_ptr<net::URLFetcher> eula_fetcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeOSOnlineTermsHandler);
+};
+
 class ChromeOSTermsHandler
     : public base::RefCountedThreadSafe<ChromeOSTermsHandler> {
  public:
@@ -158,39 +225,45 @@ class ChromeOSTermsHandler
       locale_(g_browser_process->GetApplicationLocale()) {
   }
 
-  ~ChromeOSTermsHandler() {}
+  virtual ~ChromeOSTermsHandler() {}
 
   void StartOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ChromeOSTermsHandler::LoadFileOnFileThread, this));
+    if (path_ == chrome::kOemEulaURLPath) {
+      // Load local OEM EULA from the disk.
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&ChromeOSTermsHandler::LoadOemEulaFileOnFileThread, this));
+    } else {
+      // Try to load online version of ChromeOS terms first.
+      // ChromeOSOnlineTermsHandler object destroys itself.
+      new ChromeOSOnlineTermsHandler(
+          base::Bind(&ChromeOSTermsHandler::OnOnlineEULAFetched, this));

[battre, 2013/04/16 08:34:22]

Is it guaranteed that |this| survives the callback?

[vasilii, 2013/04/16 15:40:18]

ChromeOSTermsHandler is ref-counted class. base::Bind returns something that
holds a scoped_refptr to it. Therefore when OnOnlineEULAFetched is called, there
is still alive scoped_refptr<ChromeOSTermsHandler>.

+    }
   }
 
-  void LoadFileOnFileThread() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-    if (path_ == chrome::kOemEulaURLPath) {
-      const chromeos::StartupCustomizationDocument* customization =
-          chromeos::StartupCustomizationDocument::GetInstance();
-      if (customization->IsReady()) {
-        base::FilePath oem_eula_file_path;
-        if (net::FileURLToFilePath(GURL(customization->GetEULAPage(locale_)),
-                                   &oem_eula_file_path)) {
-          if (!file_util::ReadFileToString(oem_eula_file_path, &contents_)) {
-            contents_.clear();
-          }
-        }
-      }
+  void OnOnlineEULAFetched(ChromeOSOnlineTermsHandler* loader) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    loader->GetResponseResult(&contents_);
+    if (contents_.empty()) {
+      // Load local ChromeOS terms from the file.
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&ChromeOSTermsHandler::LoadEulaFileOnFileThread, this));
     } else {
-      std::string file_path =
-          base::StringPrintf(chrome::kEULAPathFormat, locale_.c_str());
-      if (!file_util::ReadFileToString(base::FilePath(file_path), &contents_)) {
-        // No EULA for given language - try en-US as default.
-        file_path = base::StringPrintf(chrome::kEULAPathFormat, "en-US");
-        if (!file_util::ReadFileToString(base::FilePath(file_path),
-                                         &contents_)) {
-          // File with EULA not found, ResponseOnUIThread will load EULA from
-          // resources if contents_ is empty.
+      ResponseOnUIThread();
+    }
+  }
+
+  void LoadOemEulaFileOnFileThread() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    const chromeos::StartupCustomizationDocument* customization =
+        chromeos::StartupCustomizationDocument::GetInstance();
+    if (customization->IsReady()) {
+      base::FilePath oem_eula_file_path;
+      if (net::FileURLToFilePath(GURL(customization->GetEULAPage(locale_)),
+                                 &oem_eula_file_path)) {
+        if (!file_util::ReadFileToString(oem_eula_file_path, &contents_)) {
           contents_.clear();
         }
       }
@@ -200,6 +273,24 @@ class ChromeOSTermsHandler
         base::Bind(&ChromeOSTermsHandler::ResponseOnUIThread, this));
   }
 
+  void LoadEulaFileOnFileThread() {
+    std::string file_path =
+        base::StringPrintf(chrome::kEULAPathFormat, locale_.c_str());
+    if (!file_util::ReadFileToString(base::FilePath(file_path), &contents_)) {
+      // No EULA for given language - try en-US as default.
+      file_path = base::StringPrintf(chrome::kEULAPathFormat, "en-US");
+      if (!file_util::ReadFileToString(base::FilePath(file_path),
+                                       &contents_)) {
+        // File with EULA not found, ResponseOnUIThread will load EULA from
+        // resources if contents_ is empty.
+        contents_.clear();
+      }
+    }
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&ChromeOSTermsHandler::ResponseOnUIThread, this));
+  }
+
   void ResponseOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     // If we fail to load Chrome OS EULA from disk, load it from resources.
@@ -210,13 +301,13 @@ class ChromeOSTermsHandler
   }
 
   // Path in the URL.
-  std::string path_;
+  const std::string path_;
 
   // Callback to run with the response.
   content::URLDataSource::GotDataCallback callback_;
 
   // Locale of the EULA.
-  std::string locale_;
+  const std::string locale_;
 
   // EULA contents that was loaded from file.
   std::string contents_;