Creat BrowsingDataCookieHelper and CannedBrowsingDataCookieHelper for logging cookies at UI thread.

Creat BrowsingDataCookieHelper and CannedBrowsingDataCookieHelper for logging cookies at UI thread.

Moving from http://codereview.chromium.org/7355025/

BUG=XXXX
TEST=BrowsingDataCookieHelperTest

TBR=rdsmith

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

[ycxiao, 2011-08-09 13:55:54]

Hello,

PTAL.

This CL move from http://codereview.chromium.org/7355025/
The previous commit reverted because of the failure of windows reliability test.
I also notice there there is a memory leak problem related to the previous
commit. 

For the windows reliability problem, it is because the
BrowseringDataCookieHelper loges invalided cookies. The problem fixed by
creating a CanonicalCookie::Create method at CookieMonster which can get rid of
those invalided cookies. And using this CanonicalCookie::Create at
BrowseringDataCookieHelper.

For the memory leak problem, the heap check fails because of leak at
GetURLRequestContext in brosering_data_cookie_helper_unittest.cc.
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. To fixed it, at
BrowsingDataCookieHelperTest::TearDown ResetRequestContext before reset IO
thread.

Sorry for any inconvenience. Thanks a lot!

Yancheng

[jochen (gone - plz use gerrit), 2011-08-09 14:08:15]

browsing data helper LGTM

http://codereview.chromium.org/7601018/diff/1/chrome/browser/browsing_data_co...
File chrome/browser/browsing_data_cookie_helper.cc (right):

http://codereview.chromium.org/7601018/diff/1/chrome/browser/browsing_data_co...
chrome/browser/browsing_data_cookie_helper.cc:139: // cookie line and the url
don't have the same domain+registry, or url host
can you move the comment before the cc.reset(...) line? And maybe start it with
"This fails to create a canonical cookie, if..." so it becomes clearer?

[ycxiao1, 2011-08-09 14:41:04]

Sure. Thanks!

On Tue, Aug 9, 2011 at 10:08 AM, <jochen@chromium.org> wrote:

> browsing data helper LGTM
>
>
> http://codereview.chromium.**org/7601018/diff/1/chrome/**
>
browser/browsing_data_cookie_**helper.cc<http://codereview.chromium.org/7601018/diff/1/chrome/browser/browsing_data_cookie_helper.cc>
> File chrome/browser/browsing_data_**cookie_helper.cc (right):
>
> http://codereview.chromium.**org/7601018/diff/1/chrome/**
>
browser/browsing_data_cookie_**helper.cc#newcode139<http://codereview.chromium.org/7601018/diff/1/chrome/browser/browsing_data_cookie_helper.cc#newcode139>
> chrome/browser/browsing_data_**cookie_helper.cc:139: // cookie line and
> the url don't have the same domain+registry, or url host
> can you move the comment before the cc.reset(...) line? And maybe start
> it with "This fails to create a canonical cookie, if..." so it becomes
> clearer?
>
>
>
http://codereview.chromium.**org/7601018/<http://codereview.chromium.org/7601...
>

[Paweł Hajdan Jr., 2011-08-09 16:43:00]

Drive-by with a testing comment (chrome/test/OWNERS). No need to wait for me.

http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_unittest.cc:17: class
BrowsingDataCookieHelperTest : public testing::Test {
Could you please use TestingBrowserProcessTest? Otherwise this test would be
relying on a compat code that will disappear soon.

[ycxiao1, 2011-08-09 16:58:14]

On Tue, Aug 9, 2011 at 12:43 PM, <phajdan.jr@chromium.org> wrote:

> Drive-by with a testing comment (chrome/test/OWNERS). No need to wait for
> me.
>
>
> http://codereview.chromium.**org/7601018/diff/6007/chrome/**
>
browser/browsing_data_cookie_**helper_unittest.cc<http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data_cookie_helper_unittest.cc>
> File chrome/browser/browsing_data_**cookie_helper_unittest.cc (right):
>
> http://codereview.chromium.**org/7601018/diff/6007/chrome/**
>
browser/browsing_data_cookie_**helper_unittest.cc#newcode17<http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data_cookie_helper_unittest.cc#newcode17>
> chrome/browser/browsing_data_**cookie_helper_unittest.cc:17: class
> BrowsingDataCookieHelperTest : public testing::Test {
> Could you please use TestingBrowserProcessTest? Otherwise this test
> would be relying on a compat code that will disappear soon.


Sorry I am not familiar with test class.
Why this test is relying on a compat code? And How to
TestingBrowserProcessTest to prevent this issue?

>
>
>
http://codereview.chromium.**org/7601018/<http://codereview.chromium.org/7601...
>

[ycxiao, 2011-08-09 17:01:15]

On Tue, Aug 9, 2011 at 12:43 PM, <phajdan.jr@chromium.org> wrote:

> Drive-by with a testing comment (chrome/test/OWNERS). No need to wait for
> me.
>
>
> http://codereview.chromium.**org/7601018/diff/6007/chrome/**
>
browser/browsing_data_cookie_**helper_unittest.cc<http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data_cookie_helper_unittest.cc>
> File chrome/browser/browsing_data_**cookie_helper_unittest.cc (right):
>
> http://codereview.chromium.**org/7601018/diff/6007/chrome/**
>
browser/browsing_data_cookie_**helper_unittest.cc#newcode17<http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data_cookie_helper_unittest.cc#newcode17>
> chrome/browser/browsing_data_**cookie_helper_unittest.cc:17: class
> BrowsingDataCookieHelperTest : public testing::Test {
> Could you please use TestingBrowserProcessTest? Otherwise this test
> would be relying on a compat code that will disappear soon.


Sorry I am not familiar with test class.
Why this test is relying on a compat code? And How to
TestingBrowserProcessTest to prevent this issue?

>
>
>
http://codereview.chromium.**org/7601018/<http://codereview.chromium.org/7601...
>

[erikwright (departed), 2011-08-10 15:42:45]

Yancheng, TestingBrowserProcessTest seems to be a subclass of testing::Test that
ensures that certain browser process services are available. You should be able
to subclass it instead of subclassing testing::Test directly.

Pawel, it would be helpful if you could briefly point out which of the
dependencies of this test is ultimately depending on this
TestingBrowserProcessTest. Firstly so that we can be sure that it is correct to
add it, but secondly so that we can be sure to use it correctly in future CLs.

Thanks,

Erik

On 2011/08/09 17:01:15, ycxiao wrote:
> On Tue, Aug 9, 2011 at 12:43 PM, <mailto:phajdan.jr@chromium.org> wrote:
> 
> > Drive-by with a testing comment (chrome/test/OWNERS). No need to wait for
> > me.
> >
> >
> > http://codereview.chromium.**org/7601018/diff/6007/chrome/**
> >
>
browser/browsing_data_cookie_**helper_unittest.cc<http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data_cookie_helper_unittest.cc>
> > File chrome/browser/browsing_data_**cookie_helper_unittest.cc (right):
> >
> > http://codereview.chromium.**org/7601018/diff/6007/chrome/**
> >
>
browser/browsing_data_cookie_**helper_unittest.cc#newcode17<http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data_cookie_helper_unittest.cc#newcode17>
> > chrome/browser/browsing_data_**cookie_helper_unittest.cc:17: class
> > BrowsingDataCookieHelperTest : public testing::Test {
> > Could you please use TestingBrowserProcessTest? Otherwise this test
> > would be relying on a compat code that will disappear soon.
> 
> 
> Sorry I am not familiar with test class.
> Why this test is relying on a compat code? And How to
> TestingBrowserProcessTest to prevent this issue?
> 
> >
> >
> >
>
http://codereview.chromium.**org/7601018/%3Chttp://codereview.chromium.org/76...>
> >

[Paweł Hajdan Jr., 2011-08-10 17:28:37]

You're using TestingProfile, which depends on g_browser_process. Currently
g_browser_process is initialized globally by ChromeTestingSuite, but that's the
compat code I mentioned, and it's going to disappear _soon_ (like this week).

After that change trying to write code like this _without_
TestingBrowserProcessTest will just crash. Of course I'm fixing all the existing
cases, but for future CLs this could be surprising.

[ycxiao, 2011-08-10 18:55:54]

On 2011/08/10 17:28:37, Paweł Hajdan Jr. wrote:
> You're using TestingProfile, which depends on g_browser_process. Currently
> g_browser_process is initialized globally by ChromeTestingSuite, but that's
the
> compat code I mentioned, and it's going to disappear _soon_ (like this week).
> 
> After that change trying to write code like this _without_
> TestingBrowserProcessTest will just crash. Of course I'm fixing all the
existing
> cases, but for future CLs this could be surprising.

Clear! Thanks, Pawel.

Yancheng

[ycxiao, 2011-08-11 19:07:58]

Hi Erik,

The try job pass for mac, and failed at linux and win for
ExtensionWebRequestApiTest.WebRequestBlocking which is reverted now.

I will try to commit it if you agree.

Thanks,
Yancheng

http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

http://codereview.chromium.org/7601018/diff/6007/chrome/browser/browsing_data...
chrome/browser/browsing_data_cookie_helper_unittest.cc:17: class
BrowsingDataCookieHelperTest : public testing::Test {
On 2011/08/09 16:43:00, Paweł Hajdan Jr. wrote:
> Could you please use TestingBrowserProcessTest? Otherwise this test would be
> relying on a compat code that will disappear soon.

Done.

[commit-bot: I haz the power, 2011-08-12 01:37:27]

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

[commit-bot: I haz the power, 2011-08-12 17:09:20]

Change committed as 96569