Adding Priority field to cookies.

Adding Priority field to cookies.

This CL focuses on cookie parser, CanonicalCookie, and making callers pass PRIORITY_DEFAULT where applicable.

What's NOT included in this CL:
- TODO(rogerm): Persistence in SQL database.
- TODO(huangs): Using cookie priority to affect cookie eviction.
- TODO(huangs): Make priorities available for extension API (right now default value is used to set).
- TODO(lower priority): Cookie viewer update.
- TODO(lower priority): WebCookie update in webkit/glue.

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

[huangs, 2013-04-15 17:59:09]

PTAL.

[erikwright (departed), 2013-04-15 19:00:37]

https://codereview.chromium.org/14113014/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/cookies.json (right):

https://codereview.chromium.org/14113014/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/cookies.json:21: "priority": {"type": "string",
"optional": true, "description": "The priority of the cookie for eviction
avoidance."},
"The retention priority of the cookie." (here and below)

https://codereview.chromium.org/14113014/diff/1/net/cookies/canonical_cookie_...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/canonical_cookie_...
net/cookies/canonical_cookie_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Add tests that the cookie stores and returns the provided priority. Also add
tests that verify that the priority does not affect equivalence.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants.cc
File net/cookies/cookie_constants.cc (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:13: std::string kPriorityLow("Low");
I believe these need to be const char[].

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:13: std::string kPriorityLow("Low");
I don't see any reason to output these with capitalization. In any case, we will
never output them over the wire, so simplify the production code below by using
lower-case.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:21: ret = kPriorityHigh;
just return from here.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:40: ret = PRIORITY_HIGH;
just return the value directly here.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants.h
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:21: // Returns a constant string for a given
|priority|.

"Returns the Set-Cookie header priority token corresponding to |priority|."

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:22: NET_EXPORT const std::string
PriorityToString(CookiePriority priority);
CookiePriorityToString

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:25: // PRIORITY_DEFAULT for empty or unrecognized
strings.
Converts the Set-Cookie header priority token |priority| to a CookiePriority.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:26: NET_EXPORT CookiePriority
StringToPriority(const std::string& priority_str);
remove "_str". Optionally, "priority_token"

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:26: NET_EXPORT CookiePriority
StringToPriority(const std::string& priority_str);
StringToCookiePriority

[huangs, 2013-04-15 21:29:40]

Note that 

chrome/browser/extensions/api/cookies/cookies_helpers.cc
chrome/browser/extensions/api/cookies/cookies_unittest.cc

were added.  PTAL.

https://codereview.chromium.org/14113014/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/cookies.json (right):

https://codereview.chromium.org/14113014/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/cookies.json:21: "priority": {"type": "string",
"optional": true, "description": "The priority of the cookie for eviction
avoidance."},
On 2013/04/15 19:00:37, erikwright wrote:
> "The retention priority of the cookie." (here and below)

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/canonical_cookie_...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/canonical_cookie_...
net/cookies/canonical_cookie_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/04/15 19:00:37, erikwright wrote:
> Add tests that the cookie stores and returns the provided priority. Also add
> tests that verify that the priority does not affect equivalence.

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants.cc
File net/cookies/cookie_constants.cc (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:13: std::string kPriorityLow("Low");
On 2013/04/15 19:00:37, erikwright wrote:
> I believe these need to be const char[].

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:13: std::string kPriorityLow("Low");
Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:21: ret = kPriorityHigh;
On 2013/04/15 19:00:37, erikwright wrote:
> just return from here.

Done (although this leads to longer code).

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:40: ret = PRIORITY_HIGH;
On 2013/04/15 19:00:37, erikwright wrote:
> just return the value directly here.

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants.h
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:21: // Returns a constant string for a given
|priority|.
On 2013/04/15 19:00:37, erikwright wrote:
> 
> "Returns the Set-Cookie header priority token corresponding to |priority|."

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:22: NET_EXPORT const std::string
PriorityToString(CookiePriority priority);
On 2013/04/15 19:00:37, erikwright wrote:
> CookiePriorityToString

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:25: // PRIORITY_DEFAULT for empty or unrecognized
strings.
On 2013/04/15 19:00:37, erikwright wrote:
> Converts the Set-Cookie header priority token |priority| to a CookiePriority.

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:26: NET_EXPORT CookiePriority
StringToPriority(const std::string& priority_str);
On 2013/04/15 19:00:37, erikwright wrote:
> StringToCookiePriority

Done.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.h:26: NET_EXPORT CookiePriority
StringToPriority(const std::string& priority_str);
On 2013/04/15 19:00:37, erikwright wrote:
> remove "_str". Optionally, "priority_token"

Done (using just |priority|).

[erikwright (departed), 2013-04-16 14:16:15]

LGTM.

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants.cc
File net/cookies/cookie_constants.cc (right):

https://codereview.chromium.org/14113014/diff/1/net/cookies/cookie_constants....
net/cookies/cookie_constants.cc:21: ret = kPriorityHigh;
On 2013/04/15 21:29:40, huangs1 wrote:
> On 2013/04/15 19:00:37, erikwright wrote:
> > just return from here.
> 
> Done (although this leads to longer code).

Well, you did also add 3 lines of whitespace ;)

The WS is not required. Up to you. Same in StringToCookiePriority

Consider whether you still find a case statement with returns instead of breaks
distasteful. It might make the code easier to read at this point.

https://codereview.chromium.org/14113014/diff/7001/net/cookies/cookie_constan...
File net/cookies/cookie_constants.cc (right):

https://codereview.chromium.org/14113014/diff/7001/net/cookies/cookie_constan...
net/cookies/cookie_constants.cc:14: // These are lower-case to facilitate
compare.
I don't think this comment is necessary. The file is very small and I think you
have a unit-test that would fail if someone capitalized them. If you don't, add
one.

[huangs, 2013-04-16 16:04:42]

Converted to use switch(), and removed redundant NL.  Also isolated
cookie_constants_unittest.cc.  PTAL.

https://codereview.chromium.org/14113014/diff/7001/net/cookies/cookie_constan...
File net/cookies/cookie_constants.cc (right):

https://codereview.chromium.org/14113014/diff/7001/net/cookies/cookie_constan...
net/cookies/cookie_constants.cc:14: // These are lower-case to facilitate
compare.
On 2013/04/16 14:16:15, erikwright wrote:
> I don't think this comment is necessary. The file is very small and I think
you
> have a unit-test that would fail if someone capitalized them. If you don't,
add
> one.

Done. Added cookie_constants_unittest.cc.

[huangs, 2013-04-16 17:36:41]

OWNER reviews to:

mmenke@ for:
  net/net.gyp:
    Adding 3 files: net/cookies/cookie_constants*

phajdan.jr@ for:
  chrome/browser/automation/automation_util.cc:
    Adding "priority" field.
    
mkwst@ for:
  chrome/browser/extensions/api/cookies/cookies_*: (3 files)
  chrome/common/extensions/api/cookies.json
    Adding "priority" field.

Thanks!

[mmenke, 2013-04-16 17:49:44]

On 2013/04/16 17:36:41, huangs1 wrote:
> OWNER reviews to:
> 
> mmenke@ for:
>   net/net.gyp:
>     Adding 3 files: net/cookies/cookie_constants*
> 
> phajdan.jr@ for:
>   chrome/browser/automation/automation_util.cc:
>     Adding "priority" field.
>     
> mkwst@ for:
>   chrome/browser/extensions/api/cookies/cookies_*: (3 files)
>   chrome/common/extensions/api/cookies.json
>     Adding "priority" field.
> 
> Thanks!

net/net.gyp LGTM.

[Paweł Hajdan Jr., 2013-04-16 17:52:49]

automation LGTM

[Use mkwst_at_chromium.org plz., 2013-04-16 18:18:06]

On 2013/04/16 17:36:41, huangs1 wrote:
> OWNER reviews to:
> ...
> mkwst@ for:
>   chrome/browser/extensions/api/cookies/cookies_*: (3 files)
>   chrome/common/extensions/api/cookies.json
>     Adding "priority" field.

Sorry, you'll need someone from the extensions team to review API changes. Docs
look fine to me, though. I'm CCing mpcomplete@ in the hopes that he can take a
look (hi Matt!).

[Matt Perry, 2013-04-16 18:24:08]

https://codereview.chromium.org/14113014/diff/16003/chrome/common/extensions/...
File chrome/common/extensions/api/cookies.json (right):

https://codereview.chromium.org/14113014/diff/16003/chrome/common/extensions/...
chrome/common/extensions/api/cookies.json:21: "priority": {"type": "string",
"description": "The retention priority of the cookie."},
This should be an enum type. Here's an example of the syntax:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[huangs, 2013-04-16 19:43:14]

@Matt Perry: Erik and I decided that we'll defer the changes to the extensions
API, since the focus is to show that this works right now.  Now the changes only
involves passing PRIORITY_DEFAULT to get things to compile.  PTAL.  Thanks!

https://codereview.chromium.org/14113014/diff/16003/chrome/common/extensions/...
File chrome/common/extensions/api/cookies.json (right):

https://codereview.chromium.org/14113014/diff/16003/chrome/common/extensions/...
chrome/common/extensions/api/cookies.json:21: "priority": {"type": "string",
"description": "The retention priority of the cookie."},
Thanks, we'll use this when we implement for extensions API again.

[Matt Perry, 2013-04-16 19:44:22]

lgtm

[erikwright (departed), 2013-04-16 20:25:55]

LGTM

https://codereview.chromium.org/14113014/diff/26001/net/cookies/cookie_consta...
File net/cookies/cookie_constants_unittest.cc (right):

https://codereview.chromium.org/14113014/diff/26001/net/cookies/cookie_consta...
net/cookies/cookie_constants_unittest.cc:13: class CookieConstantsTest : public
testing::Test { };
This declaration has no effect and can be removed. It would only be used if the
TEST below were TEST_F.

[huangs, 2013-04-16 20:33:41]

Done.

https://codereview.chromium.org/14113014/diff/26001/net/cookies/cookie_consta...
File net/cookies/cookie_constants_unittest.cc (right):

https://codereview.chromium.org/14113014/diff/26001/net/cookies/cookie_consta...
net/cookies/cookie_constants_unittest.cc:13: class CookieConstantsTest : public
testing::Test { };
Done.  Also removed for ParsedCookieTest, which had this construct but does not
use TEST_F.

[huangs, 2013-04-16 21:06:31]

Adding
content\browser\net\sqlite_persistent_cookie_store_unittest.cc
content\browser\net\sqlite_persistent_cookie_store_perftest.cc

[erikwright (departed), 2013-04-17 15:05:46]

still LGTM.

[erikwright (departed), 2013-04-18 19:33:19]

https://codereview.chromium.org/14113014/diff/40001/net/cookies/cookie_consta...
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/14113014/diff/40001/net/cookies/cookie_consta...
net/cookies/cookie_constants.h:15: PRIORITY_LOW     = 0,
Sorry to bring this up late in the game, but after reviewing Roger's CL I
realize these should be COOKIE_PRIORITY_LOW, etc.

Otherwise they are quite ambiguous (net::PRIORITY_LOW could refer to a number of
things).

[huangs, 2013-04-18 23:16:26]

Updated.

[erikwright (departed), 2013-04-19 18:41:15]

Committed patchset #8 manually as r195245 (presubmit successful).