Move CanonicalCookie into separate files

Move CanonicalCookie into separate files


BUG=137014, 112155
TEST=no
TBR=sky@chromium.org, erg@chromium.org, tony@chromium.org, eroman@chromium.org

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

[battre, 2012-07-16 16:38:45]

Hi Erik,

could you please review this CL?

Patch Set #1 contains a literal copy of cookie_monster{.cc, .h, _unittest.cc} to
canonical_cookie{.cc, .h, unittest.cc} so that the review should be easier if
you compare the last patch set to version #1.

Thanks,
Dominic

[erikwright (departed), 2012-07-16 21:13:20]

http://codereview.chromium.org/10785017/diff/2001/net/cookies/canonical_cooki...
File net/cookies/canonical_cookie_unittest.cc (right):

http://codereview.chromium.org/10785017/diff/2001/net/cookies/canonical_cooki...
net/cookies/canonical_cookie_unittest.cc:32: namespace {
remove the anonymous namespace.

http://codereview.chromium.org/10785017/diff/2001/net/cookies/parsed_cookie.cc
File net/cookies/parsed_cookie.cc (right):

http://codereview.chromium.org/10785017/diff/2001/net/cookies/parsed_cookie.c...
net/cookies/parsed_cookie.cc:284: // Parse a cookie expiration time.  We try to
be lenient, but we need to
I think cookie_util.{h,cc} would be a better place for this.

http://codereview.chromium.org/10785017/diff/2001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10785017/diff/2001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:42: #include
"net/cookies/canonical_cookie.h"
Why is this needed?

http://codereview.chromium.org/10785017/diff/12001/chrome/browser/ui/gtk/coll...
File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/10785017/diff/12001/chrome/browser/ui/gtk/coll...
chrome/browser/ui/gtk/collected_cookies_gtk.cc:35: #include
"net/cookies/canonical_cookie.h"
Why is this needed at all?

[battre, 2012-07-17 15:01:53]

http://codereview.chromium.org/10785017/diff/2001/net/cookies/canonical_cooki...
File net/cookies/canonical_cookie_unittest.cc (right):

http://codereview.chromium.org/10785017/diff/2001/net/cookies/canonical_cooki...
net/cookies/canonical_cookie_unittest.cc:32: namespace {
On 2012/07/16 21:13:20, erikwright wrote:
> remove the anonymous namespace.

Done.

http://codereview.chromium.org/10785017/diff/2001/net/cookies/parsed_cookie.cc
File net/cookies/parsed_cookie.cc (right):

http://codereview.chromium.org/10785017/diff/2001/net/cookies/parsed_cookie.c...
net/cookies/parsed_cookie.cc:284: // Parse a cookie expiration time.  We try to
be lenient, but we need to
On 2012/07/16 21:13:20, erikwright wrote:
> I think cookie_util.{h,cc} would be a better place for this.

Done.

http://codereview.chromium.org/10785017/diff/2001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10785017/diff/2001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:42: #include
"net/cookies/canonical_cookie.h"
On 2012/07/16 21:13:20, erikwright wrote:
> Why is this needed?

I think the compiler complained but it does not happen anymore. I have removed
the line.

Done.

http://codereview.chromium.org/10785017/diff/12001/chrome/browser/ui/gtk/coll...
File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/10785017/diff/12001/chrome/browser/ui/gtk/coll...
chrome/browser/ui/gtk/collected_cookies_gtk.cc:35: #include
"net/cookies/canonical_cookie.h"
On 2012/07/16 21:13:20, erikwright wrote:
> Why is this needed at all?

detailed_info.cookie in line 402 is a CanonicalCookie.

[erikwright (departed), 2012-07-17 15:59:20]

LGTM minus the IWYU/forward-decl comments.

Also, even small forward-decl namespace blocks need to have a "}  // namespace
X" on the end. Please update the various forward declarations that you have
introduced accordingly.

See this query for precedent (declarations of "namespace net" outside of
src/net):
http://code.google.com/searchframe#search/&q=%22namespace%20net%22%20-file:sr...

http://codereview.chromium.org/10785017/diff/6005/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

http://codereview.chromium.org/10785017/diff/6005/net/cookies/cookie_util.cc#...
net/cookies/cookie_util.cc:137: #ifdef COMPILER_MSVC
#include "build/build_config.h"

http://codereview.chromium.org/10785017/diff/6005/net/cookies/cookie_util.cc#...
net/cookies/cookie_util.cc:140: sscanf(
#include <cstdio>

http://codereview.chromium.org/10785017/diff/6005/net/cookies/cookie_util.cc#...
net/cookies/cookie_util.cc:158: exploded.year = atoi(token.c_str());
<cstdlib>

http://codereview.chromium.org/10785017/diff/6005/net/cookies/cookie_util.h
File net/cookies/cookie_util.h (right):

http://codereview.chromium.org/10785017/diff/6005/net/cookies/cookie_util.h#n...
net/cookies/cookie_util.h:38: NET_EXPORT base::Time ParseCookieTime(const
std::string& time_string);
I believe you can forward-declare base::Time.

http://codereview.chromium.org/10785017/diff/6005/net/cookies/parsed_cookie.h
File net/cookies/parsed_cookie.h (right):

http://codereview.chromium.org/10785017/diff/6005/net/cookies/parsed_cookie.h...
net/cookies/parsed_cookie.h:12: #include "base/time.h"
No longer needed.

http://codereview.chromium.org/10785017/diff/6005/net/cookies/parsed_cookie_u...
File net/cookies/parsed_cookie_unittest.cc (right):

http://codereview.chromium.org/10785017/diff/6005/net/cookies/parsed_cookie_u...
net/cookies/parsed_cookie_unittest.cc:6: #include <time.h>
no longer needed.

[battre, 2012-07-17 16:52:54]

On 2012/07/17 15:59:20, erikwright wrote:
> LGTM minus the IWYU/forward-decl comments.
> 
> Also, even small forward-decl namespace blocks need to have a "}  // namespace
> X" on the end. Please update the various forward declarations that you have
> introduced accordingly.

I am happy to do that (the effort is minimal), but I am not sure whether it is
desired. If you consider for example very central classes like
chrome/browser/profiles/profile.h or
content/public/browser/resource_dispatcher_host.h (or
chrome/browser/browsing_data_cookie_helper.h for that matter), you see that none
of them have closing "}  // namspace foo" statements.

If you search in chrome/ about a quarter of forward declarations have the  //
namespace net, in content/ it is less than a quarter:

battre@battre:/media/sdb1/battre/chrome/src/chrome$ git gs "namespace net {" |
wc -l
122
battre@battre:/media/sdb1/battre/chrome/src/chrome$ git gs "}  // namespace net"
| wc -l
33

[erikwright (departed), 2012-07-17 19:46:41]

On 2012/07/17 16:52:54, battre wrote:
> On 2012/07/17 15:59:20, erikwright wrote:
> > LGTM minus the IWYU/forward-decl comments.
> > 
> > Also, even small forward-decl namespace blocks need to have a "}  //
namespace
> > X" on the end. Please update the various forward declarations that you have
> > introduced accordingly.
> 
> I am happy to do that (the effort is minimal), but I am not sure whether it is
> desired. If you consider for example very central classes like
> chrome/browser/profiles/profile.h or
> content/public/browser/resource_dispatcher_host.h (or
> chrome/browser/browsing_data_cookie_helper.h for that matter), you see that
none
> of them have closing "}  // namspace foo" statements.
> 
> If you search in chrome/ about a quarter of forward declarations have the  //
> namespace net, in content/ it is less than a quarter:
> 
> battre@battre:/media/sdb1/battre/chrome/src/chrome$ git gs "namespace net {" |
> wc -l
> 122
> battre@battre:/media/sdb1/battre/chrome/src/chrome$ git gs "}  // namespace
net"
> | wc -l
> 33

SGTM. Disregard that nit.

[battre, 2012-07-18 08:48:43]

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/cookie_...
File net/cookies/cookie_util.cc (right):

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/cookie_...
net/cookies/cookie_util.cc:137: #ifdef COMPILER_MSVC
On 2012/07/17 15:59:20, erikwright wrote:
> #include "build/build_config.h"

Done.

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/cookie_...
net/cookies/cookie_util.cc:140: sscanf(
On 2012/07/17 15:59:20, erikwright wrote:
> #include <cstdio>

Done.

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/cookie_...
net/cookies/cookie_util.cc:158: exploded.year = atoi(token.c_str());
On 2012/07/17 15:59:20, erikwright wrote:
> <cstdlib> 

Done.

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/cookie_...
File net/cookies/cookie_util.h (right):

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/cookie_...
net/cookies/cookie_util.h:38: NET_EXPORT base::Time ParseCookieTime(const
std::string& time_string);
On 2012/07/17 15:59:20, erikwright wrote:
> I believe you can forward-declare base::Time.

No, that works only for pointers or references. (I verified this)

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/parsed_...
File net/cookies/parsed_cookie.h (right):

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/parsed_...
net/cookies/parsed_cookie.h:12: #include "base/time.h"
On 2012/07/17 15:59:20, erikwright wrote:
> No longer needed.

Done.

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/parsed_...
File net/cookies/parsed_cookie_unittest.cc (right):

https://chromiumcodereview.appspot.com/10785017/diff/6005/net/cookies/parsed_...
net/cookies/parsed_cookie_unittest.cc:6: #include <time.h>
On 2012/07/17 15:59:20, erikwright wrote:
> no longer needed.

Done.

[battre, 2012-07-18 11:43:49]

Hi.

I will TBR a couple of you because Erik has done the main review and the rest is
just renaming a class or adding includes because the definition is now found in
a different place. Really boring stuff...

sky for chrome/browser/ui/, content/browser/
erg for chrome/browser/profiles
tony for webkit/
eroman for net/net.gpy ;-)

Best regards,
Dominic

[commit-bot: I haz the power, 2012-07-18 11:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10785017/23001

[commit-bot: I haz the power, 2012-07-18 11:44:44]

Presubmit check for 10785017-23001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/cocoa/content_settings
    chrome/browser/ui/gtk
    content/browser/renderer_host
    chrome/browser/ui/webui
    chrome/browser/profiles
    webkit
    chrome/browser/ui
    net

Presubmit checks took 3.8s to calculate.

[commit-bot: I haz the power, 2012-07-18 12:05:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10785017/23001

[commit-bot: I haz the power, 2012-07-18 13:13:37]

Change committed as 147222

[eroman, 2012-07-18 20:58:35]

LGTM for net/*

https://chromiumcodereview.appspot.com/10785017/diff/23001/net/cookies/canoni...
File net/cookies/canonical_cookie.cc (right):

https://chromiumcodereview.appspot.com/10785017/diff/23001/net/cookies/canoni...
net/cookies/canonical_cookie.cc:380: }  // namespace
Isn't this namespace net?

https://chromiumcodereview.appspot.com/10785017/diff/23001/net/cookies/cookie...
File net/cookies/cookie_monster.cc (right):

https://chromiumcodereview.appspot.com/10785017/diff/23001/net/cookies/cookie...
net/cookies/cookie_monster.cc:2158: }  // namespace
Looks like this comment is wrong, can you fix it?