Download autocheckout whitelist and enable autocheckout for whitelisted sites only.

chrome/browser/autofill/autocheckout/whitelist_manager.cc

+// Copyright (c) 2013 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/autofill/autocheckout/whitelist_manager.h"
+
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/string_split.h"
+#include "base/supports_user_data.h"

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Remove this; it's already present in the header.

[benquan, 2013/01/25 00:55:31]

removed it from the header and kept this line.

+#include "chrome/browser/autofill/autocheckout/whitelist_url.h"
+#include "chrome/common/chrome_switches.h"
+#include "content/public/browser/browser_context.h"
+#include "googleurl/src/gurl.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+
+namespace {
+
+// Back off in seconds after each whitelist download is attempted.
+const int kDownloadIntervalSeconds = 86400;  // 1 day
+
+// The delay in seconds after startup before download whitelist. This helps
+// to reduce contention at startup time.
+const int kInitialDownloadDelaySeconds = 3;

[Ilya Sherman, 2013/01/24 22:01:55]

nit: I think you want a slightly larger delay for this.

[benquan, 2013/01/25 00:55:31]

Download will be triggered only when we create  WhitelistManager instance in
GetForBrowserContext() which is called from AutofillManager, I think we already
have some delay there already. 

[Ilya Sherman, 2013/01/25 01:22:40]

Ok.

+
+const char kWhiteListKeyName[] = "autocheckout_whitelist_manager";
+
+} //  namespace
+
+
+namespace autocheckout {
+
+// static
+WhitelistManager* WhitelistManager::GetForBrowserContext(
+    content::BrowserContext* context) {
+  DCHECK(context);

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Omit this.

[benquan, 2013/01/25 00:55:31]

Done.

+  WhitelistManager* wm = static_cast<WhitelistManager*>(

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Please avoid abbreviations in variable names.  Instead, use a name like
manager_ or whitelist_manager_

[benquan, 2013/01/25 00:55:31]

Done.

+      context->GetUserData(kWhiteListKeyName));
+  if (!wm) {
+    wm = new WhitelistManager(context->GetRequestContext());
+    wm->ScheduleDownload(kInitialDownloadDelaySeconds);
+    context->SetUserData(kWhiteListKeyName, wm);
+  }
+  return wm;
+}
+
+WhitelistManager::WhitelistManager(
+    net::URLRequestContextGetter* context_getter)
+    : context_getter_(context_getter),
+      callback_pending_(false),
+      experimental_form_filling_enabled_(
+          CommandLine::ForCurrentProcess()->HasSwitch(
+              switches::kEnableExperimentalFormFilling)) {
+  DCHECK(context_getter);
+}
+
+bool WhitelistManager::ScheduleDownload(int interval_seconds) {

[Ilya Sherman, 2013/01/24 22:01:55]

nit: The return value from this method doesn't seem to ever be used, so I'd
recommend changing this to a void function.

[benquan, 2013/01/25 00:55:31]

The unittest uses it to check if a new download was scheduled by this function.

[Ilya Sherman, 2013/01/25 01:22:40]

That means that the unittest is not testing the production code.  If you were to
reverse what the code actually does, but keep the return values the same, the
unit test should fail; but if it's relying on the returned values to be
meaningful, it might pass instead.

[benquan, 2013/01/26 02:05:53]

Done.

+  if (!experimental_form_filling_enabled_) {
+    // The feature is not enabled: do not do the request.
+    return false;
+  }
+  if (download_timer_.IsRunning() || callback_pending_) {
+    // A download activity is already scheduled or happening.
+    DVLOG(1) << "Autocheckout DownloadWhitelist scheduler is already running.";

[Ilya Sherman, 2013/01/24 22:01:55]

Will this DVLOG and others in this file be useful after this CL lands?  (It's
fine if they are; they just strike me as being more like debugging code than
long-term logging code.)

[benquan, 2013/01/25 00:55:31]

removed

+    return false;
+  }
+
+  download_timer_.Start(FROM_HERE,
+                        base::TimeDelta::FromSeconds(interval_seconds),
+                        this,
+                        &WhitelistManager::TriggerDownload);
+  DVLOG(1) << "Autocheckout DownloadWhitelist was scheduled for "
+           << interval_seconds << " seconds.";
+  return true;
+}
+
+void WhitelistManager::TriggerDownload() {
+  callback_pending_ = true;
+  DVLOG(1) << "Autocheckout DownloadWhitelist...";
+
+  request_.reset(net::URLFetcher::Create(
+      0, GetAutocheckoutWhitelistUrl(), net::URLFetcher::GET, this));
+  request_->SetRequestContext(context_getter_);
+  request_->Start();
+  return;

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Omit this.

[benquan, 2013/01/25 00:55:31]

Done.

+}
+
+void WhitelistManager::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(callback_pending_);
+  callback_pending_ = false;
+  scoped_ptr<net::URLFetcher> old_request = request_.Pass();
+  DCHECK_EQ(source, old_request.get());
+
+  DVLOG(1) << "Autocheckout got response from " << source->GetOriginalURL()
+           << ". Response code: " << source->GetResponseCode();
+
+  if (source->GetResponseCode() != net::HTTP_OK)
+    return;

[Ilya Sherman, 2013/01/24 22:01:55]

So, if the download ever fails, you'll never schedule a new one?  Are you sure
that's the behavior you want?

[benquan, 2013/01/25 00:55:31]

fixed
On 2013/01/24 22:01:55, Ilya Sherman wrote:

+
+  std::string data;
+  source->GetResponseAsString(&data);
+  DVLOG(1) << "Autocheckout whitelist response data: " << data;
+  BuildWhitelist(data);
+
+  ScheduleDownload(kDownloadIntervalSeconds);
+}
+
+bool WhitelistManager::IsAutocheckoutEnabled(const GURL& url) {
+  if (!experimental_form_filling_enabled_) {
+    // The feature is not enabled, return false.

[Ilya Sherman, 2013/01/24 22:01:55]

nit: This comment is redundant with the code; please remove it.

[benquan, 2013/01/25 00:55:31]

Done.

+    return false;
+  }
+
+  if (url.is_empty())
+    return false;
+
+  for (std::vector<std::string>::iterator it = url_prefixes_.begin();
+      it != url_prefixes_.end(); ++it) {
+    // This is only for ~20 sites initially, liner search is sufficient.
+    // TODO(benquan): Look for optimization options when we support
+    // more sites.
+    if (url.spec().compare(0, it->size(), *it) == 0)

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Use StartsWith() from base/string_util.h for this check.

[benquan, 2013/01/25 00:55:31]

Done.

+      return true;
+  }
+  return false;
+}
+
+void WhitelistManager::BuildWhitelist(const std::string& data) {
+  // TODO(benquan): find a better way to parse csv data.
+  std::vector<std::string> new_url_prefixes;
+
+  std::stringstream dataStream(data);

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Use base::SplitString rather than using a stringstream.

[benquan, 2013/01/25 00:55:31]

Done.

+  std::string line;
+  while (std::getline(dataStream, line)) {
+    if (!line.empty()) {
+      std::vector<std::string> fields;
+      base::SplitString(line, ',', &fields);
+      // The whilist file is a simple CSV file, and the first column is the url
+      // prefix.

[Ilya Sherman, 2013/01/24 22:01:55]

What are the remaining columns?

[benquan, 2013/01/25 00:55:31]

We only have one column right now. It's for backward compatibility, if we decide
to add additional metadata as more columns, the old version of chrome may not be
impacted.

[Ilya Sherman, 2013/01/25 01:22:40]

Please mention that in the comment.

[benquan, 2013/01/26 02:05:53]

Done.

+      if (!fields[0].empty())
+        new_url_prefixes.push_back(fields[0]);
+    }
+  }
+  url_prefixes_ = new_url_prefixes;
+}
+
+}  // namespace autocheckout
+