Creat BrowsingDataCookieHelper and CannedBrowsingDataCookieHelper for logging cookies at UI thread.

Creat BrowsingDataCookieHelper and CannedBrowsingDataCookieHelper for logging cookies at UI thread.
BUG=XXX
TEST=XXX

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95534

[ycxiao, 2011-07-13 19:58:59]

PTAL.

Thanks,
Yancheng

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

Not a complete review, but hopefully enough to keep you busy for a bit ;)

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:8: #include
"chrome/browser/net/chrome_url_request_context.h"
why do you need chrome_url_request_context and not just the base class
url_request_context?

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:11: 
#include base::Time, DCHECK, scoped_ptr

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:22: if
(BrowserThread::CurrentlyOn(BrowserThread::UI)) {
DCHECK that the member completion_callback_ is null.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:34:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Separate the UI and IO thread portions of this method into separate methods, as
in the Database helper. The reason is that you actually are allowing calls
directly on the IO thread that then skip the DCHECKs and setting of is_fetching,
completion_callback_, etc., which would be disastrous.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:38: if (cookie_monster)
if !cookie_monster, unset is_fetching_ and invoke completion_callback_ with an
empty cookie list.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:57:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Separate into two methods as mentioned above.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:73: void
BrowsingDataCookieHelper::OnFetchComplete(const net::CookieList& cookies) {
Separate into two methods.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:13: 
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.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:21: explicit
BrowsingDataCookieHelper(Profile* profile);
Add documentation for each of the methods.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:24: const
net::CookieMonster::GetCookieListCallback& callback);
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.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:28: const net::CookieList&
cookie_list();
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.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:35: bool is_fetching_;
is_fetching_ may be private.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:63: // Add the cookies which can
successfully parsed and no in the current
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?

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:78: virtual void
CancelNotification() {}
Don't inline virtual methods.

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

[erikwright (departed), 2011-07-19 14:40:33]

Looking good.

Please remember to hit 'done' on all comments after responding to them. It makes
the follow-up review a little bit easier.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:9: #include
"base/memory/scoped_ptr.h"
Include either here or in the .h, not both.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:32: //  cookie_list_.clear();
Remove.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:37: return;
Remove the return.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:52: return;
Remove.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:55:
BrowsingDataCookieHelper::~BrowsingDataCookieHelper() {
Move next to constructor.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:76: return;
Remove.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:97: if (cookie_monster)
Use braces whenever the conditional block is more than one line.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:99: cookie,
If these two arguments fit on one line, you can put them together.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:150: const net::CookieOptions&
options) {
options is not used. Remove it from the signature (and propagate the change to
the callers, if they don't need the options either).

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:13: #include
"base/memory/ref_counted.h"
I think you only use scoped_ptr in the .cc. Can you remove it here?

Otherwise remove from the .cc. "include what you use" does allow you to depend
on includes from your own header file.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:55: //  net::CookieList
cookie_list_;
Remove this line.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:99: // Add those cookies which are
differnet from the current cookies at Name,
"Adds those cookie which differ from the current cookies by Name, Domain, or
Path."

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:104: // Parses the cookie line,
adds all successfully parsed cookies that are
"Parses the cookie line and ... that differ from ... " like above.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:117: const net::CookieList&
cookie_list();
Remove this. See my comment in the test for further information, feel free to
ask for info.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper_browsertest.cc (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:16: void
CreatCookiesForTest() {
Creat -> Create

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:39: void
FetchCallback(const net::CookieList& cookies) {
Is there any reason why these callback methods cannot be members of the Test
fixture? I don't see the value of the second class and it seems annoying to have
to declare instances of it everywhere.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:43:
net::CookieList::const_iterator it = cookies.begin();
Is there any guarantee that the cookies will be delivered in this order? Is it
an expectation that they are the same order inserted, or just a coincidence? In
the latter case, you should modify your expectations to verify that the cookies
are there, but not the order. One option is to sort the cookies after retrieving
them.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:139: cookie =
helper->cookie_list();
If I understand correctly, you are using this accessor as a convenient way to
get a list of Canonical Cookies.

In general needing to add properties or methods to a class only for the purpose
of testing is distasteful, and usually unnecessary if the classes are designed
properly. Please find another way to do this, and ask for help if it's not
clear.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.h:9: #include
<set>
Why do you need to add these here?

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model.h:20: #include
"chrome/browser/browsing_data_cookie_helper.h"
I believe you should be able to forward declare the BrowsingDataCookieHelper
type.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model.h:26: #include
"ui/base/models/tree_node_model.h"
Include what you use: net::CookieMonster::CanonicalCookie requires
cookie_monster, unfortunately.

Feel free to move CanonicalCookie into a top-level class (i.e.,
net::CanonicalCookie) for bonus points, but do it in a separate CL cause it will
touch many files.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
File chrome/browser/cookies_tree_model_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model_unittest.cc:12: #include
"chrome/browser/mock_browsing_data_cookie_helper.h"
Did you forget to add this file?

http://codereview.chromium.org/7355025/diff/9001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7355025/diff/9001/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:2101: GetCookieDomainWithString(url, domain_string,
What's the background on this change?

[ycxiao1, 2011-07-19 22:12:47]

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:8: #include
"chrome/browser/net/chrome_url_request_context.h"
On 2011/07/18 02:08:30, erikwright wrote:
> why do you need chrome_url_request_context and not just the base class
> url_request_context?

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:11: 
On 2011/07/18 02:08:30, erikwright wrote:
> #include base::Time, DCHECK, scoped_ptr

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:22: if
(BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2011/07/18 02:08:30, erikwright wrote:
> DCHECK that the member completion_callback_ is null.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:34:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2011/07/18 02:08:30, erikwright wrote:
> Separate the UI and IO thread portions of this method into separate methods,
as
> in the Database helper. The reason is that you actually are allowing calls
> directly on the IO thread that then skip the DCHECKs and setting of
is_fetching,
> completion_callback_, etc., which would be disastrous.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:38: if (cookie_monster)
On 2011/07/18 02:08:30, erikwright wrote:
> if !cookie_monster, unset is_fetching_ and invoke completion_callback_ with an
> empty cookie list.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:57:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2011/07/18 02:08:30, erikwright wrote:
> Separate into two methods as mentioned above.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:73: void
BrowsingDataCookieHelper::OnFetchComplete(const net::CookieList& cookies) {
On 2011/07/18 02:08:30, erikwright wrote:
> Separate into two methods.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:13: 
On 2011/07/18 02:08:30, erikwright wrote:
> 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.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:21: explicit
BrowsingDataCookieHelper(Profile* profile);
On 2011/07/18 02:08:30, erikwright wrote:
> Add documentation for each of the methods.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:24: const
net::CookieMonster::GetCookieListCallback& callback);
On 2011/07/18 02:08:30, erikwright wrote:
> 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.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:28: const net::CookieList&
cookie_list();
On 2011/07/18 02:08:30, erikwright wrote:
> 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.

Done. Moving cookie_list_ to CannedBrowsingDataCookieHelper and
MockBrowsingDataCookieHelper.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:35: bool is_fetching_;
On 2011/07/18 02:08:30, erikwright wrote:
> is_fetching_ may be private.

Done.

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:63: // Add the cookies which can
successfully parsed and no in the current
On 2011/07/18 02:08:30, erikwright wrote:
> 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?

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

http://codereview.chromium.org/7355025/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:78: virtual void
CancelNotification() {}
On 2011/07/18 02:08:30, erikwright wrote:
> Don't inline virtual methods.
> 
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:9: #include
"base/memory/scoped_ptr.h"
On 2011/07/19 14:40:33, erikwright wrote:
> Include either here or in the .h, not both.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:32: //  cookie_list_.clear();
On 2011/07/19 14:40:33, erikwright wrote:
> Remove.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:37: return;
On 2011/07/19 14:40:33, erikwright wrote:
> Remove the return.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:52: return;
On 2011/07/19 14:40:33, erikwright wrote:
> Remove.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:55:
BrowsingDataCookieHelper::~BrowsingDataCookieHelper() {
On 2011/07/19 14:40:33, erikwright wrote:
> Move next to constructor.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:76: return;
On 2011/07/19 14:40:33, erikwright wrote:
> Remove.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:97: if (cookie_monster)
On 2011/07/19 14:40:33, erikwright wrote:
> Use braces whenever the conditional block is more than one line.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:99: cookie,
On 2011/07/19 14:40:33, erikwright wrote:
> If these two arguments fit on one line, you can put them together.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.cc:150: const net::CookieOptions&
options) {
On 2011/07/19 14:40:33, erikwright wrote:
> options is not used. Remove it from the signature (and propagate the change to
> the callers, if they don't need the options either).

Options is actually needed here.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:13: #include
"base/memory/ref_counted.h"
On 2011/07/19 14:40:33, erikwright wrote:
> I think you only use scoped_ptr in the .cc. Can you remove it here?
> 
> Otherwise remove from the .cc. "include what you use" does allow you to depend
> on includes from your own header file.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:55: //  net::CookieList
cookie_list_;
On 2011/07/19 14:40:33, erikwright wrote:
> Remove this line.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:99: // Add those cookies which are
differnet from the current cookies at Name,
On 2011/07/19 14:40:33, erikwright wrote:
> "Adds those cookie which differ from the current cookies by Name, Domain, or
> Path."

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:104: // Parses the cookie line,
adds all successfully parsed cookies that are
On 2011/07/19 14:40:33, erikwright wrote:
> "Parses the cookie line and ... that differ from ... " like above.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper.h:117: const net::CookieList&
cookie_list();
On 2011/07/19 14:40:33, erikwright wrote:
> Remove this. See my comment in the test for further information, feel free to
> ask for info.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper_browsertest.cc (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:16: void
CreatCookiesForTest() {
On 2011/07/19 14:40:33, erikwright wrote:
> Creat -> Create

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:39: void
FetchCallback(const net::CookieList& cookies) {
On 2011/07/19 14:40:33, erikwright wrote:
> Is there any reason why these callback methods cannot be members of the Test
> fixture? I don't see the value of the second class and it seems annoying to
have
> to declare instances of it everywhere.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:43:
net::CookieList::const_iterator it = cookies.begin();
On 2011/07/19 14:40:33, erikwright wrote:
> Is there any guarantee that the cookies will be delivered in this order? Is it
> an expectation that they are the same order inserted, or just a coincidence?
In
> the latter case, you should modify your expectations to verify that the
cookies
> are there, but not the order. One option is to sort the cookies after
retrieving
> them.
> 

The CookieMonster::GetAllCookies will sort the cookies.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:139: cookie =
helper->cookie_list();
On 2011/07/19 14:40:33, erikwright wrote:
> If I understand correctly, you are using this accessor as a convenient way to
> get a list of Canonical Cookies.
> 
> In general needing to add properties or methods to a class only for the
purpose
> of testing is distasteful, and usually unnecessary if the classes are designed
> properly. Please find another way to do this, and ask for help if it's not
> clear.

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/content_setti...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/content_setti...
chrome/browser/content_settings/tab_specific_content_settings.h:9: #include
<set>
On 2011/07/19 14:40:33, erikwright wrote:
> Why do you need to add these here?

Requested by gcl lint.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model.h:20: #include
"chrome/browser/browsing_data_cookie_helper.h"
On 2011/07/19 14:40:33, erikwright wrote:
> I believe you should be able to forward declare the BrowsingDataCookieHelper
> type.
> 

Done.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model.h:26: #include
"ui/base/models/tree_node_model.h"
On 2011/07/19 14:40:33, erikwright wrote:
> Include what you use: net::CookieMonster::CanonicalCookie requires
> cookie_monster, unfortunately.
> 
> Feel free to move CanonicalCookie into a top-level class (i.e.,
> net::CanonicalCookie) for bonus points, but do it in a separate CL cause it
will
> touch many files.

Done. Postpone the moving of CanonicalCookie.

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
File chrome/browser/cookies_tree_model_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/9001/chrome/browser/cookies_tree_...
chrome/browser/cookies_tree_model_unittest.cc:12: #include
"chrome/browser/mock_browsing_data_cookie_helper.h"
On 2011/07/19 14:40:33, erikwright wrote:
> Did you forget to add this file?

Done.

http://codereview.chromium.org/7355025/diff/9001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7355025/diff/9001/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:2101: GetCookieDomainWithString(url, domain_string,
On 2011/07/19 14:40:33, erikwright wrote:
> What's the background on this change?

This canonicalcookie construct was originally used by cookie_info_view.cc and
gtk_chrome_cookie_view.cc. 

This canonicalcookie construct is wanted by
CannedBrowsingDataCookieHelper::AddChangeCookie which loges setcookie. In this
case, the caller can't make sure that the passing arguments are good. It is
normal that SetCookie may fails because of GetCookieDomainWithString failure.
And AddChangeCookie won't log this setcookie if it will fails at persistent
cookie store. This can be implemented by checking the value of domain. The
cookie_domain will equal to "" if and only if GetCookieDomainWithString fails.

[Paweł Hajdan Jr., 2011-07-21 16:29:41]

Drive-by (chrome/test/OWNERS). Please let me also review the updated version.

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_browsertest.cc (right):

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:14: class
BrowsingDataCookieHelperTest : public InProcessBrowserTest {
Does this need to be a browser test? Have you considered making it a unit_test
instead? InProcessBrowserTest starts up the entire browser, making your test
slower and more flakiness-prone.

[ycxiao, 2011-07-21 16:43:35]

On 2011/07/21 16:29:41, Paweł Hajdan Jr. wrote:
> Drive-by (chrome/test/OWNERS). Please let me also review the updated version.
> 
>
http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
> File chrome/browser/browsing_data_cookie_helper_browsertest.cc (right):
> 
>
http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
> chrome/browser/browsing_data_cookie_helper_browsertest.cc:14: class
> BrowsingDataCookieHelperTest : public InProcessBrowserTest {
> Does this need to be a browser test? Have you considered making it a unit_test
> instead? InProcessBrowserTest starts up the entire browser, making your test
> slower and more flakiness-prone.

Hi Pawel,

Thanks for being a reviewer.
I will notice you after I updated the CL according to Erik's comments.

Yancheng

[erikwright (departed), 2011-07-21 17:25:08]

Agree with Pawel, preferable and seemingly reasonable to convert to a unit test
(other tests use testing_profile outside of the browser, for example).

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:126: void
CannedBrowsingDataCookieHelper::AddChangeCookie(
Please add a comment explaining that the implementation of this function is
designed to mirror the logic of SetCookieWithOptions, only storing those cookies
that would be stored by the CookieMonster.

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:174: bool
CannedBrowsingDataCookieHelper::CheckCookie(
Rename to DeleteMatchingCookies

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/mock_browsin...
File chrome/browser/mock_browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/mock_browsin...
chrome/browser/mock_browsing_data_cookie_helper.h:34: // Returns true if all
cookies since the last Reset() invokation were
invokation -> invocation

[ycxiao, 2011-07-21 19:04:49]

Hi,

Remove the browser test, and add it to unit test. 

PTAL.

Thanks!

Yancheng

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:126: void
CannedBrowsingDataCookieHelper::AddChangeCookie(
On 2011/07/21 17:25:08, erikwright wrote:
> Please add a comment explaining that the implementation of this function is
> designed to mirror the logic of SetCookieWithOptions, only storing those
cookies
> that would be stored by the CookieMonster.

Done.

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:174: bool
CannedBrowsingDataCookieHelper::CheckCookie(
On 2011/07/21 17:25:08, erikwright wrote:
> Rename to DeleteMatchingCookies

Done.

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_browsertest.cc (right):

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_browsertest.cc:14: class
BrowsingDataCookieHelperTest : public InProcessBrowserTest {
On 2011/07/21 16:29:41, Paweł Hajdan Jr. wrote:
> Does this need to be a browser test? Have you considered making it a unit_test
> instead? InProcessBrowserTest starts up the entire browser, making your test
> slower and more flakiness-prone.

Combine the browser test to the unit test.

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/mock_browsin...
File chrome/browser/mock_browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/26017/chrome/browser/mock_browsin...
chrome/browser/mock_browsing_data_cookie_helper.h:34: // Returns true if all
cookies since the last Reset() invokation were
On 2011/07/21 17:25:08, erikwright wrote:
> invokation -> invocation

Done.

[erikwright (departed), 2011-07-21 19:24:36]

Almost there.

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:22:
cookie_monster->SetCookieWithOptions(
Don't use the synchronous methods, since you are going to get rid of them.

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/cookies_view_handler.cc:18: #include
"net/url_request/url_request_context_getter.h"
Seems like this can be removed.

[ycxiao, 2011-07-21 21:17:15]

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:22:
cookie_monster->SetCookieWithOptions(
On 2011/07/21 19:24:36, erikwright wrote:
> Don't use the synchronous methods, since you are going to get rid of them.

Done.

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/7355025/diff/35003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/cookies_view_handler.cc:18: #include
"net/url_request/url_request_context_getter.h"
On 2011/07/21 19:24:36, erikwright wrote:
> Seems like this can be removed.

Done.

[Paweł Hajdan Jr., 2011-07-22 18:07:02]

Test code LGTM with a minor comment. Thank you.

http://codereview.chromium.org/7355025/diff/36007/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/36007/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:85: MessageLoop
message_loop;
nit: Don't duplicate those MessageLoop and BrowserThread vars in each test, just
add them to the test fixture (the base class). The entire test fixture is
created fresh and destroyed for each test.

[erikwright (departed), 2011-07-25 16:50:48]

A few minor comments. Please address them before submitting to an OWNER review
with, of course, appropriate context.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:22:
request_context_getter_(profile->GetRequestContext()) {
DCHECK constructed in UI thread.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:29: const
net::CookieMonster::GetCookieListCallback& callback) {
Change the declaration of this parameter to match the header.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:28: MessageLoop
message_loop;
I think you should probably be keeping the Threads and MessageLoop as members,
initializing them in constructor or setUp and ensuring in your tearDown or
destructor to call Stop on the IO thread.

Reasons:

1) It's a bit unclear what happens when you create them on the stack and
immediately have them go out of scope. It's certainly not clear to me that this
is guaranteed to have the desired behaviour.
2) It's not obvious that this method (CreateCookiesForTest) also initializes the
threads. I could easily see someone writing a test that didn't need
CreateCookiesForTest and not realizing that it also had an important role in
setting up threads.
3) I suspect that you really want to be calling Stop on that IO thread.

[ycxiao, 2011-07-26 15:35:37]

http://codereview.chromium.org/7355025/diff/36007/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/36007/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:85: MessageLoop
message_loop;
On 2011/07/22 18:07:02, Paweł Hajdan Jr. wrote:
> nit: Don't duplicate those MessageLoop and BrowserThread vars in each test,
just
> add them to the test fixture (the base class). The entire test fixture is
> created fresh and destroyed for each test.

Done.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:22:
request_context_getter_(profile->GetRequestContext()) {
On 2011/07/25 16:50:48, erikwright wrote:
> DCHECK constructed in UI thread.

Done.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:29: const
net::CookieMonster::GetCookieListCallback& callback) {
On 2011/07/25 16:50:48, erikwright wrote:
> Change the declaration of this parameter to match the header.

Done.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:28: MessageLoop
message_loop;
On 2011/07/25 16:50:48, erikwright wrote:
> I think you should probably be keeping the Threads and MessageLoop as members,
> initializing them in constructor or setUp and ensuring in your tearDown or
> destructor to call Stop on the IO thread.
> 
> Reasons:
> 
> 1) It's a bit unclear what happens when you create them on the stack and
> immediately have them go out of scope. It's certainly not clear to me that
this
> is guaranteed to have the desired behaviour.
> 2) It's not obvious that this method (CreateCookiesForTest) also initializes
the
> threads. I could easily see someone writing a test that didn't need
> CreateCookiesForTest and not realizing that it also had an important role in
> setting up threads.
> 3) I suspect that you really want to be calling Stop on that IO thread.

The BrowserThread seems to be different with Thread. It don't need call Start()
and Stop(). The example:
https://cs.corp.google.com/#chrome/src/chrome/browser/browsing_data_file_syst...

[ycxiao, 2011-07-26 15:57:18]

Hi Jochen,

PTAL. Thanks!

The purpose for this CL was applying the new asynchronous API to
tab_specific_context_setting and cookies_tree_model.

The browsing_data_cookie_helper.{h/cc} is helper classes for drawing the cookies
tree model. It contains BrowsingDataCookieHelper and
CannedBrowsingDataCookieHelper. The BrowsingDataCookieHelper can fetch cookie
and delete cookie from the persistent cookie store. The Canned one can't, and is
for logging purpose.

The mock_browsing_data_cookie_helper.{h/cc} is a mock of
BrowsingDataCookieHelper for the unit tests of cookie tree model.

For whom not clear about the back ground:

I am designing an asynchronous API for cookie store. The current cookie store
will block the IO thread while loading the cookies during the initialization.
Also the first request of cookies will wait until the entire cookie store was
loaded. When this project was done, the loading process will moving to the
backend instead of blocking the IO thread. Furthermore, the backend thread,
database thread, will first loading the request cookies and return it instead of
waiting until the whole cookie store loaded.

Currently, I am create an asynchronous wrap on the top of the synchronous cookie
store and update the callers to use it.

Yancheg

[ycxiao, 2011-07-26 17:24:14]

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:160: 
Change the place, otherwise the DCHECK of current thread at
BrowsingDataCookieHelper constructor may fails.

[erikwright (departed), 2011-07-26 17:42:00]

LGTM with one comment nit.

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/44001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:28: MessageLoop
message_loop;
On 2011/07/26 15:35:38, ycxiao wrote:
> On 2011/07/25 16:50:48, erikwright wrote:
> > I think you should probably be keeping the Threads and MessageLoop as
members,
> > initializing them in constructor or setUp and ensuring in your tearDown or
> > destructor to call Stop on the IO thread.
> > 
> > Reasons:
> > 
> > 1) It's a bit unclear what happens when you create them on the stack and
> > immediately have them go out of scope. It's certainly not clear to me that
> this
> > is guaranteed to have the desired behaviour.
> > 2) It's not obvious that this method (CreateCookiesForTest) also initializes
> the
> > threads. I could easily see someone writing a test that didn't need
> > CreateCookiesForTest and not realizing that it also had an important role in
> > setting up threads.
> > 3) I suspect that you really want to be calling Stop on that IO thread.
> 
> The BrowserThread seems to be different with Thread. It don't need call
Start()
> and Stop(). The example:
>
https://cs.corp.google.com/#chrome/src/chrome/browser/browsing_data_file_syst...

BrowserThread is a subclass of Thread. You do need to call Start, though not for
the main (UI) thread because that thread is the one already running.

It is an error for multiple instances to exist at the same time with the same
type (IO, UI, etc.). The destructor automatically stops them, so calling 'reset'
is the equivalent of calling Stop.

The code as it exists is fine now.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:21:
testing_profile_.GetCookieMonster();
"to make sure our ... tested." -> "because parts of the BrowsingDataCookieHelper
expect to run on that thread."

[jochen (gone - plz use gerrit), 2011-07-27 07:17:31]

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:134: if
(options.exclude_httponly() && pc.IsHttpOnly()) {
How can this happen? I don't think there's a code path where a cookie is
written, marked as http only, and we exclude http only cookies

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:140: if (cc->Domain() == "") {
when does this happen?

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:145: for (cookie_iterator cookie =
cookie_list_.begin();
deletematchingcookie(cc)?

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.h:98: void AddReadCookie(const GURL&
url,
AddReadCookies (plural)

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.h:103: void AddChangeCookie(const
GURL& url,
AddChange*d*Cookie

[ycxiao, 2011-07-27 14:29:46]

Hi Jochen,

Done with the rename change. The AddChangedCookie is designed to mirror the
logic of SetCookieWithOptions. Please find the details at comments.

Thanks,
Yancheng

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:134: if
(options.exclude_httponly() && pc.IsHttpOnly()) {
On 2011/07/27 07:17:32, jochen wrote:
> How can this happen? I don't think there's a code path where a cookie is
> written, marked as http only, and we exclude http only cookies

The code path from URLRequestHttpJob set include http only. 
https://cs.corp.google.com/#chrome/trunk/src/net/url_request/url_request_http...

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:140: if (cc->Domain() == "") {
On 2011/07/27 07:17:32, jochen wrote:
> when does this happen?

The CookieStore method SetCookieWithOptions may failed because of the failure of
GetCookieDomainWithString. I want to have consistent behaviours as before. If
GetCookieDomainWithString fails, it will return empty string. Cookie will not be
stored in this situation.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:145: for (cookie_iterator cookie =
cookie_list_.begin();
On 2011/07/27 07:17:32, jochen wrote:
> deletematchingcookie(cc)?

It is also for the consistent behaviours as CookieMonster::SetCookieWithOptions.
If equivalent cookie comes, delete the old one. By equivalent means the same
Name, Domain and Path.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.h (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.h:98: void AddReadCookie(const GURL&
url,
On 2011/07/27 07:17:32, jochen wrote:
> AddReadCookies (plural)

Done.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.h:103: void AddChangeCookie(const
GURL& url,
On 2011/07/27 07:17:32, jochen wrote:
> AddChange*d*Cookie

Done.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:21: // verify which
thread is running are actually tested.
On 2011/07/26 17:42:00, erikwright wrote:
> "to make sure our ... tested." -> "because parts of the
BrowsingDataCookieHelper
> expect to run on that thread."

Done.

[erikwright (departed), 2011-07-27 14:47:24]

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:134: if
(options.exclude_httponly() && pc.IsHttpOnly()) {
On 2011/07/27 14:29:46, ycxiao wrote:
> On 2011/07/27 07:17:32, jochen wrote:
> > How can this happen? I don't think there's a code path where a cookie is
> > written, marked as http only, and we exclude http only cookies
> 
> The code path from URLRequestHttpJob set include http only. 
>
https://cs.corp.google.com/#chrome/trunk/src/net/url_request/url_request_http...
> 

Yancheng: note that the code path you referred to is 'include_httponly' whereas
here we are looking at 'exclude_...'.

Jochen + Yancheng: I assumed that this could happen in the case of cookies set
by Javascript. i.e., the path from the renderer would set exclude_httponly, and
since the cookie line is as supplied from the script, it could theoretically be
(illegally) marked HTTP only.

Yancheng, can you confirm that this assumption is correct? If so, perhaps add a
brief comment stating that this could occur if a Javascript cookie illegally
specified the HTTP only flag.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:140: if (cc->Domain() == "") {
On 2011/07/27 14:29:46, ycxiao wrote:
> On 2011/07/27 07:17:32, jochen wrote:
> > when does this happen?
> 
> The CookieStore method SetCookieWithOptions may failed because of the failure
of
> GetCookieDomainWithString. I want to have consistent behaviours as before. If
> GetCookieDomainWithString fails, it will return empty string. Cookie will not
be
> stored in this situation.

Yancheng, can you clarify at which point GetCookieDomainWithString is being
called? Presumably under either the ParsedCookie constructor or the
CanonicalCookie constructor?

Please add a comment in the code stating under which conditions Domain may be
blank.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:145: for (cookie_iterator cookie =
cookie_list_.begin();
On 2011/07/27 14:29:46, ycxiao wrote:
> On 2011/07/27 07:17:32, jochen wrote:
> > deletematchingcookie(cc)?
> 
> It is also for the consistent behaviours as
CookieMonster::SetCookieWithOptions.
> If equivalent cookie comes, delete the old one. By equivalent means the same
> Name, Domain and Path. 
> 

Yes, but since it is identical to DeleteMatchingCookie, can you replace the code
segment here with a call to that method?

[ycxiao, 2011-07-27 16:19:58]

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:134: if
(options.exclude_httponly() && pc.IsHttpOnly()) {
On 2011/07/27 14:47:24, erikwright wrote:
> On 2011/07/27 14:29:46, ycxiao wrote:
> > On 2011/07/27 07:17:32, jochen wrote:
> > > How can this happen? I don't think there's a code path where a cookie is
> > > written, marked as http only, and we exclude http only cookies
> > 
> > The code path from URLRequestHttpJob set include http only. 
> >
>
https://cs.corp.google.com/#chrome/trunk/src/net/url_request/url_request_http...
> > 
> 
> Yancheng: note that the code path you referred to is 'include_httponly'
whereas
> here we are looking at 'exclude_...'.
> 
> Jochen + Yancheng: I assumed that this could happen in the case of cookies set
> by Javascript. i.e., the path from the renderer would set exclude_httponly,
and
> since the cookie line is as supplied from the script, it could theoretically
be
> (illegally) marked HTTP only.
> 
> Yancheng, can you confirm that this assumption is correct? If so, perhaps add
a
> brief comment stating that this could occur if a Javascript cookie illegally
> specified the HTTP only flag.

Yes, the render message filter set exclude_http_only. It will happen, if the
cookie line illegally be specified as HTTP only.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:140: if (cc->Domain() == "") {
On 2011/07/27 14:47:24, erikwright wrote:
> On 2011/07/27 14:29:46, ycxiao wrote:
> > On 2011/07/27 07:17:32, jochen wrote:
> > > when does this happen?
> > 
> > The CookieStore method SetCookieWithOptions may failed because of the
failure
> of
> > GetCookieDomainWithString. I want to have consistent behaviours as before.
If
> > GetCookieDomainWithString fails, it will return empty string. Cookie will
not
> be
> > stored in this situation.
> 
> Yancheng, can you clarify at which point GetCookieDomainWithString is being
> called? Presumably under either the ParsedCookie constructor or the
> CanonicalCookie constructor?
> 
> Please add a comment in the code stating under which conditions Domain may be
> blank.

Sorry. The GetCookieDomainWithString is called by CanonicalCookie constructor.
The domain may be blank, because of failure of getting the normalized domain
specified in cookie line; url and normalized domain don't have the same
domain+registry; or |url_host| isn't |cookie_domain| or one of its subdomains.

http://codereview.chromium.org/7355025/diff/47005/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper.cc:145: for (cookie_iterator cookie =
cookie_list_.begin();
On 2011/07/27 14:47:24, erikwright wrote:
> On 2011/07/27 14:29:46, ycxiao wrote:
> > On 2011/07/27 07:17:32, jochen wrote:
> > > deletematchingcookie(cc)?
> > 
> > It is also for the consistent behaviours as
> CookieMonster::SetCookieWithOptions.
> > If equivalent cookie comes, delete the old one. By equivalent means the same
> > Name, Domain and Path. 
> > 
> 
> Yes, but since it is identical to DeleteMatchingCookie, can you replace the
code
> segment here with a call to that method?

Done.

[ycxiao, 2011-07-28 14:05:44]

Hi Jochen,

I modify the code according to your review.
PTAL.

Thanks,
Yancheng

[jochen (gone - plz use gerrit), 2011-07-28 14:11:12]

LGTM, thx

[commit-bot: I haz the power, 2011-07-29 16:27:45]

Presubmit check for 7355025-28027 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: net/base/cookie_monster.cc

Presubmit checks took 1.4s to calculate.

[ycxiao, 2011-07-29 16:36:50]

Hi Randy,

I need your LGTM for cookiemonster.cc to submit this CL.
It is a small change which removes a DCHECK. Please find details at
http://codereview.chromium.org/7355025/diff/9001/net/base/cookie_monster.cc

Thanks,
Yancheng

[ycxiao, 2011-07-29 19:07:32]

Hi Randy,

I need your LGTM for cookiemonster.cc to submit this CL.
It is a small change which removes a DCHECK. Please find details at
http://codereview.chromium.org/7355025/diff/9001/net/base/cookie_monster.cc

Thanks,
Yancheng

[ycxiao, 2011-07-29 22:12:04]

Hi Randy,

PTAL.
Changed the constructor of CanonicalCookie to Create().
 
Thanks,
Yancheng

[Randy Smith (Not in Mondays), 2011-08-03 18:05:06]

LGTM.  Another possibility (up to you, might be slightly cleaner) would be to
create a version of CanonicalCookie::Create() that took a ParsedCookie.  I think
this would fall within the spirit of the style guide ("Use overloaded functions
(including constructors) only if a reader looking at a call site can get a good
idea of what is happening without having to first figure out exactly which
overload is being called.")

[ycxiao, 2011-08-03 23:49:27]

On 2011/08/03 18:05:06, rdsmith wrote:
> LGTM.  Another possibility (up to you, might be slightly cleaner) would be to
> create a version of CanonicalCookie::Create() that took a ParsedCookie.  I
think
> this would fall within the spirit of the style guide ("Use overloaded
functions
> (including constructors) only if a reader looking at a call site can get a
good
> idea of what is happening without having to first figure out exactly which
> overload is being called.")

The suggested change makes sense to me also. I would like to submit this CL
first. And after I go back to Montreal(I am at mvt for some intern event
currently), I can create another CL for this concept.

Thanks for the suggestion and also the explanation of the background reasons.

[commit-bot: I haz the power, 2011-08-04 00:39:07]

Try job failure for 7355025-57001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-08-04 19:10:46]

Try job failure for 7355025-68001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-08-04 23:28:18]

Change committed as 95534

[ycxiao, 2011-08-08 15:28:15]

http://codereview.chromium.org/7355025/diff/78026/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7355025/diff/78026/chrome/browser/browsing_dat...
chrome/browser/browsing_data_cookie_helper_unittest.cc:30: // Create an new IO
thread to current message loop because
The heap check fails because of leak at GetURLRequestContext.
http://build.chromium.org/p/chromium.memory/builders/Linux%20Heapcheck/builds...
I notice that the URLRequestContextGetter need be delete on the IO thread. So if
we don't have an IO thread it will leak. It also need to be at the same thread
as the created thread. So I delete the real io thread and create a new one at
current messageloop.