Reset profile: reset cookies and site data.

Reset profile: reset cookies and site data.
Fixed Profile Reset unit tests.

BUG=235037
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207833

[vasilii, 2013-06-19 15:23:06]

Hi!

Please take a look.

[battre, 2013-06-20 06:15:57]

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_browsertest.cc (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:21: class
RemoveCookieTester {
highlevel class comment?

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:62:
base::Unretained(profile_->GetRequestContext())));
with this suggestions below, this function would become something like

...
       base::Bind(&RemoveCookieTester::GetCookieOnIOThread,
                  base::Unretained(this),
                  base::Unretained(profile_->GetRequestContext())
                  cookie_hostname)));
  BlockUntilNotified();
  return last_cookies_;

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:78:
net::URLRequestContextGetter* rq_context) {
nit: We try not to use abbreviations in variable names. I would call it
context_getter.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:83:
GURL("http://host1/"), net::CookieOptions(),
nit: -2 indentation

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:83:
GURL("http://host1/"), net::CookieOptions(),
What do you think of passing the host name as a parameter?

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:83:
GURL("http://host1/"), net::CookieOptions(),
can you move this string literal and also the A=1 out of the functions? -->
first they become global literals
const char* kCookieHostname = "http://host1";
const char* kCookieDefinition = "A=1";
and second they become parameters to that are passed from the test into this
helper class.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:94:
GURL("http://host1/"), "A=1", net::CookieOptions(),
what do you think of passing the hostname and cookie value as parameters?

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:108:
get_cookie_success_ = true;
opt: What do you think of just storing cookies into an attribute |last_cookies_|
and then replacing return get_cookie_success_; in
RemoveCookieTester::ContainsCookie() with "return last_cookies_ ==
kCookieDefinition;"?

reasoning: I don't like the get_cookie_success_ variable name so much and that
the "A=1" string is in this function.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:155:
tester.AddCookie();
how about making this call
tester.AddCookie(kCookieHostname, kCookieDefinition);

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:163:
EXPECT_FALSE(tester.ContainsCookie());
and making this here:
EXPECT_EQ(kCookieDefinition, tester.GetCookies(kCookieHostname));

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_test_base.cc (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.cc:5: #include
"chrome/browser/profile_resetter/profile_resetter.h"
needed?

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.cc:6: #include
"chrome/browser/profile_resetter/profile_resetter_test_base.h"
this should be the first include line

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_test_base.h (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.h:13: class
ProfileResetterMockObject {
highlevel class comment?

once you can describe the purpose of the object in a short statement, we may
come up with a better name

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.h:24:
DISALLOW_COPY_AND_ASSIGN(ProfileResetterMockObject);
nit: this should be the last line in the class.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.h:29: class
ProfileResetterTestBase {
Highlevel class comment?

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:216:
scoped_ptr<content::WebContents> contents1(CreateWebContents());
did this leak in the past?

[vasilii, 2013-06-20 13:08:22]

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_browsertest.cc (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:21: class
RemoveCookieTester {
On 2013/06/20 06:15:57, battre wrote:
> highlevel class comment?

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:62:
base::Unretained(profile_->GetRequestContext())));
On 2013/06/20 06:15:57, battre wrote:
> with this suggestions below, this function would become something like
> 
> ...
>        base::Bind(&RemoveCookieTester::GetCookieOnIOThread,
>                   base::Unretained(this),
>                   base::Unretained(profile_->GetRequestContext())
>                   cookie_hostname)));
>   BlockUntilNotified();
>   return last_cookies_;

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:78:
net::URLRequestContextGetter* rq_context) {
On 2013/06/20 06:15:57, battre wrote:
> nit: We try not to use abbreviations in variable names. I would call it
> context_getter.

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:83:
GURL("http://host1/"), net::CookieOptions(),
On 2013/06/20 06:15:57, battre wrote:
> nit: -2 indentation

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:83:
GURL("http://host1/"), net::CookieOptions(),
On 2013/06/20 06:15:57, battre wrote:
> What do you think of passing the host name as a parameter?

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:83:
GURL("http://host1/"), net::CookieOptions(),
On 2013/06/20 06:15:57, battre wrote:
> can you move this string literal and also the A=1 out of the functions? -->
> first they become global literals
> const char* kCookieHostname = "http://host1";
> const char* kCookieDefinition = "A=1";
> and second they become parameters to that are passed from the test into this
> helper class.

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:94:
GURL("http://host1/"), "A=1", net::CookieOptions(),
On 2013/06/20 06:15:57, battre wrote:
> what do you think of passing the hostname and cookie value as parameters?

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:108:
get_cookie_success_ = true;
On 2013/06/20 06:15:57, battre wrote:
> opt: What do you think of just storing cookies into an attribute
|last_cookies_|
> and then replacing return get_cookie_success_; in
> RemoveCookieTester::ContainsCookie() with "return last_cookies_ ==
> kCookieDefinition;"?
> 
> reasoning: I don't like the get_cookie_success_ variable name so much and that
> the "A=1" string is in this function.

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:155:
tester.AddCookie();
On 2013/06/20 06:15:57, battre wrote:
> how about making this call
> tester.AddCookie(kCookieHostname, kCookieDefinition);

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:163:
EXPECT_FALSE(tester.ContainsCookie());
On 2013/06/20 06:15:57, battre wrote:
> and making this here:
> EXPECT_EQ(kCookieDefinition, tester.GetCookies(kCookieHostname));

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_test_base.cc (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.cc:5: #include
"chrome/browser/profile_resetter/profile_resetter.h"
On 2013/06/20 06:15:57, battre wrote:
> needed?

Yes, unfortunately. There is a ProfileResetter::~ProfileResetter() call here.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.cc:6: #include
"chrome/browser/profile_resetter/profile_resetter_test_base.h"
On 2013/06/20 06:15:57, battre wrote:
> this should be the first include line

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_test_base.h (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.h:13: class
ProfileResetterMockObject {
On 2013/06/20 06:15:57, battre wrote:
> highlevel class comment?
> 
> once you can describe the purpose of the object in a short statement, we may
> come up with a better name

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.h:24:
DISALLOW_COPY_AND_ASSIGN(ProfileResetterMockObject);
On 2013/06/20 06:15:57, battre wrote:
> nit: this should be the last line in the class.

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_test_base.h:29: class
ProfileResetterTestBase {
On 2013/06/20 06:15:57, battre wrote:
> Highlevel class comment?

Done.

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/17274006/diff/1/chrome/browser/profile_resett...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:216:
scoped_ptr<content::WebContents> contents1(CreateWebContents());
On 2013/06/20 06:15:57, battre wrote:
> did this leak in the past?

No, BrowserWithTestWindowTest cleans up all the tabs. I copied this pattern from
the highlevel comment on WebContents.

[battre, 2013-06-21 04:54:56]

lgtm

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_browsertest.cc (right):

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:63:
last_cookies_.clear();
DCHECK(!waiting_callback_); ?

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_test_base.h (right):

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_test_base.h:44:
testing::StrictMock<ProfileResetterMockObject> mock_object_;
nit: -1 space

[vasilii, 2013-06-21 07:44:17]

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_browsertest.cc (right):

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_browsertest.cc:63:
last_cookies_.clear();
On 2013/06/21 04:54:56, battre wrote:
> DCHECK(!waiting_callback_); ?

Done.

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_test_base.h (right):

https://codereview.chromium.org/17274006/diff/11001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_test_base.h:44:
testing::StrictMock<ProfileResetterMockObject> mock_object_;
On 2013/06/21 04:54:56, battre wrote:
> nit: -1 space

Done.

[commit-bot: I haz the power, 2013-06-21 07:44:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/17274006/22001

[commit-bot: I haz the power, 2013-06-21 15:35:28]

Change committed as 207833