Creat BrowsingDataCookieHelper and CannedBrowsingDataCookieHelper for logging cookies at UI thread.

chrome/browser/browsing_data_cookie_helper.h

+// Copyright (c) 2011 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.
+
+#ifndef CHROME_BROWSER_BROWSING_DATA_COOKIE_HELPER_H_
+#define CHROME_BROWSER_BROWSING_DATA_COOKIE_HELPER_H_
+#pragma once
+
+#include <string>
+
+#include "net/base/cookie_monster.h"
+#include "net/url_request/url_request_context_getter.h"
+

[erikwright (departed), 2011/07/18 02:08:30]

Include what you use (scoped_refptr, DISALLOW_COPY_AND_ASSIGN,
refcountedthreadsafe, GURL are all transient dependencies).

You should be able to forward declare urlrequestcontextgetter instead of
including it.

[ycxiao1, 2011/07/19 22:12:48]

Done.

+class Profile;
+
+// This class fetches cookie information on behalf of a caller
+// on the UI thread.
+class BrowsingDataCookieHelper
+    : public base::RefCountedThreadSafe<BrowsingDataCookieHelper> {
+ public:
+  explicit BrowsingDataCookieHelper(Profile* profile);

[erikwright (departed), 2011/07/18 02:08:30]

Add documentation for each of the methods.

[ycxiao1, 2011/07/19 22:12:48]

Done.

+
+  virtual void StartFetching(
+      const net::CookieMonster::GetCookieListCallback& callback);

[erikwright (departed), 2011/07/18 02:08:30]

Don't re-use the callback type from CookieMonster. Just declare it here, as the
other helpers do.

It's an implementation detail that this happens to be the same signature used by
the underlying cookie monster.

[ycxiao1, 2011/07/19 22:12:48]

Done.

+  virtual void CancelNotification();
+  virtual void DeleteCookie(const net::CookieMonster::CanonicalCookie& cookie);
+
+  const net::CookieList& cookie_list();

[erikwright (departed), 2011/07/18 02:08:30]

This accessor seems inconsistent with helpers for other stores. Must it exist
and be public?

If you remove it I don't think you need a member cookie_list_ anymore in the
BrowsingDataCookieHelper, in which case you can move it to
CannedBrowsingDataCookieHelper.

[ycxiao1, 2011/07/19 22:12:48]

Done. Moving cookie_list_ to CannedBrowsingDataCookieHelper and
MockBrowsingDataCookieHelper.

+
+ protected:
+  friend class base::RefCountedThreadSafe<BrowsingDataCookieHelper>;
+  virtual ~BrowsingDataCookieHelper();
+
+  net::CookieList cookie_list_;
+  bool is_fetching_;

[erikwright (departed), 2011/07/18 02:08:30]

is_fetching_ may be private.

[ycxiao1, 2011/07/19 22:12:48]

Done.

+
+ private:
+  void OnFetchComplete(const net::CookieList& cookies);
+
+  Profile* profile_;
+  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+  net::CookieMonster::GetCookieListCallback completion_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowsingDataCookieHelper);
+};
+
+// This class is a thin wrapper around BrowsingDataCookieHelper that does not
+// fetch its information from the persistent cookie store, but gets them passed
+// as a parameter during construction.
+class CannedBrowsingDataCookieHelper : public BrowsingDataCookieHelper {
+ public:
+  explicit CannedBrowsingDataCookieHelper(Profile* profile);
+
+  // Return a copy of the cookie helper. Only one consumer can use the
+  // StartFetching method at a time, so we need to create a copy of the helper
+  // everytime we instantiate a cookies tree model for it.
+  CannedBrowsingDataCookieHelper* Clone();
+
+  // Add those cookies which are not in the current CookieList.
+  void AddReadCookie(const GURL& url,
+                     const net::CookieList& cookie_list);
+
+  // Add the cookies which can successfully parsed and no in the current

[erikwright (departed), 2011/07/18 02:08:30]

This comment is confusing. I guess it means, "Parses the cookie line, adds all
successfully parsed cookies that are not already in the cookie helper.

Can you document what it means to "already be in the cookie helper?" Same name
and domain? Same name, value, and domain? Etc... Have you verified that it is
consistent with the previous behaviour?

[ycxiao1, 2011/07/19 22:12:48]

Need more work for the consistent behaviour. One idea is to checking the unit
tests for TabSpecificContentSetting.

+  // CookieList.
+  void AddChangeCookie(const GURL& url,
+                       const std::string& cookie_line,
+                       const net::CookieOptions& options);
+
+  // Clears the list of canned cookies.
+  void Reset();
+
+  // True if no cookie are currently stored.
+  bool empty() const;
+
+  // BrowsingDataCookieHelper methods.
+  virtual void StartFetching(
+      const net::CookieMonster::GetCookieListCallback& callback);
+  virtual void CancelNotification() {}

[erikwright (departed), 2011/07/18 02:08:30]

Don't inline virtual methods.

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

[ycxiao1, 2011/07/19 22:12:48]

Done.

+
+ private:
+  bool HasCookie(const net::CookieMonster::CanonicalCookie& add_cookie);
+  virtual ~CannedBrowsingDataCookieHelper();
+
+  Profile* profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(CannedBrowsingDataCookieHelper);
+};
+
+#endif  // CHROME_BROWSER_BROWSING_DATA_COOKIE_HELPER_H_