Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14208017:
Add cookie priority to the cookie database. (Closed)
|
Created:
7 years, 8 months ago by Roger McFarlane (Chromium) Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd cookie priority to the cookie database.
This CL adds cookie priorities to the cookie persistance layer. Cookie priorities allow developers to influence the order in which cookies are evicted in order to meet cookie domain limits.
This CL:
- adds a priority field to the cookies table
- updates the persistence layer to save/load the cookie to the new field
- increments the current database version from 5 to 6
- adds a migration step to upgrade version 5 databases to version 6
R= erikwright@chromium.org, huangs@chromium.org
BUG=232693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195954
Patch Set 1 : Initial CL #
Total comments: 2
Patch Set 2 : Fix a typo. #
Total comments: 6
Patch Set 3 : Address Sam's comments re: patchset 2 #
Total comments: 2
Patch Set 4 : Add unittests and rebase onto Sam's change. #
Total comments: 13
Patch Set 5 : Rename priority enum values and rebase onto master. #
Total comments: 6
Patch Set 6 : Rebase #
Messages
Total messages: 17 (0 generated)
Hi Erik and Sam, Here's a first stab at persisting the cookie priority field. It is intended to be layered on top of https://codereview.chromium.org/14113014/ Let me know if I'm on the right track. In particular, I've left kCompatibleVersionNumber at 5 since older versions will simply continue to ignore cookie priority and treat them all as medium. But maybe we want older versions not to simply write "medium" into the cookie database by default if they don't understand priority?
A separate CL updates the histogram XML file.
https://codereview.chromium.org/14208017/diff/2001/content/browser/net/sqlite... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/2001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:666: "secure, httponly, last_access_utc, has_expires, persistent, prioirity " Typo "prioirity"?
https://codereview.chromium.org/14208017/diff/2001/content/browser/net/sqlite... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/2001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:666: "secure, httponly, last_access_utc, has_expires, persistent, prioirity " On 2013/04/16 17:56:28, huangs1 wrote: > Typo "prioirity"? Yes, thank you!
Please update sqlite_server_bound_cert_store_unittest.cc as well. Thanks! https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:319: // order in which cookies are deleted in order to meet domain cookie limits. s/deleted/evicted/g to conform with terminology. https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:377: "priority INTEGER NOT NULL DEFAULT 1")) 1 == net::PRIORITY_DEFAULT. While it's messy to inject it here, perhaps we can at least add comment, and check this in a unit test? https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:807: "ADD COLUMN priority INTEGER DEFAULT 1")) { Add comment that we set 1 == net::PRIORITY_DEFAULT.
sqlite_server_bound_cert_store_unittest.cc? Mayhaps you meant sqlite_persistent_cookie_store_unittest.cc? If so, then yes, I'll update this file as well once your change lands and I rebase to it. https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:319: // order in which cookies are deleted in order to meet domain cookie limits. On 2013/04/16 18:25:55, huangs1 wrote: > s/deleted/evicted/g to conform with terminology. Done. https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:377: "priority INTEGER NOT NULL DEFAULT 1")) On 2013/04/16 18:25:55, huangs1 wrote: > 1 == net::PRIORITY_DEFAULT. While it's messy to inject it here, perhaps we can > at least add comment, and check this in a unit test? Done. https://codereview.chromium.org/14208017/diff/8001/content/browser/net/sqlite... content/browser/net/sqlite_persistent_cookie_store.cc:807: "ADD COLUMN priority INTEGER DEFAULT 1")) { On 2013/04/16 18:25:55, huangs1 wrote: > Add comment that we set 1 == net::PRIORITY_DEFAULT. Done.
Ah yes, I meant sqlite_persistent_cookie_store_unittest.cc. Sorry about that.
LG. I would rather see the integer stored in the DB declared locally in this file. As it stands, if the enum in cookie_constants were to change it would break the DB loading. So define an enum locally, and in the load/save code, use a switch statement to map between the CookiePriority value and the local value. https://codereview.chromium.org/14208017/diff/12001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/12001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:807: // Alter the table to add the priority collum with a default value of collum -> column
PTAL? I've rebased onto Sam's proposed change, so this should more closely resemble the final product. https://codereview.chromium.org/14208017/diff/12001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/12001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:807: // Alter the table to add the priority collum with a default value of On 2013/04/16 20:41:03, erikwright wrote: > collum -> column Done. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:390: DestroyStore(); This seemed to be missing from the test (on comparison to other tests in this file).
On 2013/04/16 20:41:03, erikwright wrote: > LG. I would rather see the integer stored in the DB declared locally > in this file. As it stands, if the enum in cookie_constants were to > change it would break the DB loading. Done.
LGTM. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:340: // This anonymous enum defines the valid integer CookiePriority values. Just say: // Possible values for the 'priority' column. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:346: kCookiePriorityDefault = kCookiePriorityMedium Not used. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:347: }; Maybe name this enum "DBCookiePriority" and use that name instead of int below? https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:352: int CookiePriorityToInt(net::CookiePriority priority) { CookiePriorityToDBCookiePriority and the reverse below, then just remove the comments. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:422: "priority INTEGER NOT NULL DEFAULT 1)")) { // Medium priority. Would this compile if you replace -- 1)" -- with -- " + base::IntToString(CookiePriorityToDBCookiePriority(net::PRIORITY_DEFAULT)) + ")" -- ? https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:455: // Add a default-priority persistent cookie. I would remove this one - it's not testing anything inside the SQLite store because PRIORITY_MEDIUM is mapped to a real priority before it even gets to the store.
Another look? https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:340: // This anonymous enum defines the valid integer CookiePriority values. On 2013/04/18 19:34:05, erikwright wrote: > Just say: > > // Possible values for the 'priority' column. Done. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:346: kCookiePriorityDefault = kCookiePriorityMedium On 2013/04/18 19:34:05, erikwright wrote: > Not used. Removed. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:347: }; On 2013/04/18 19:34:05, erikwright wrote: > Maybe name this enum "DBCookiePriority" and use that name instead of int below? Done. While I don't feel all that strongly about it, I'm not convinced that this change improves the overall readability as compared to just using int. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:352: int CookiePriorityToInt(net::CookiePriority priority) { On 2013/04/18 19:34:05, erikwright wrote: > CookiePriorityToDBCookiePriority and the reverse below, then just remove the > comments. Done. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:422: "priority INTEGER NOT NULL DEFAULT 1)")) { // Medium priority. On 2013/04/18 19:34:05, erikwright wrote: > Would this compile if you replace > > -- > 1)" > -- > > with > > -- > " + base::IntToString(CookiePriorityToDBCookiePriority(net::PRIORITY_DEFAULT)) + > ")" > -- > ? Done. Switched to using base::StringPrintf to get the statement text, here and in the schema migration code. https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/14208017/diff/23002/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:455: // Add a default-priority persistent cookie. On 2013/04/18 19:34:05, erikwright wrote: > I would remove this one - it's not testing anything inside the SQLite store > because PRIORITY_MEDIUM is mapped to a real priority before it even gets to the > store. Done.
Now rebased onto the TOT as Sam's change has landed.
LGTM with nits. https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:474: ASSERT_TRUE(it != cookie_map.end()); Nit: ASSERT_NE(cookie_map.end(), cookie_map.find(kLowName)); https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:478: ASSERT_TRUE(it != cookie_map.end()); ASSERT_NE(cookie_map.end(), cookie_map.find(kMediumName)); https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:482: ASSERT_TRUE(it != cookie_map.end()); ASSERT_NE(cookie_map.end(), cookie_map.find(HighName));
Thanks. Committing. https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:474: ASSERT_TRUE(it != cookie_map.end()); On 2013/04/19 21:15:28, huangs1 wrote: > Nit: > ASSERT_NE(cookie_map.end(), cookie_map.find(kLowName)); Alas, using the gtest macros for iterator comparisons doesn't compile on all platforms. https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:478: ASSERT_TRUE(it != cookie_map.end()); On 2013/04/19 21:15:28, huangs1 wrote: > ASSERT_NE(cookie_map.end(), cookie_map.find(kMediumName)); Alas, using the gtest macros for iterator comparisons doesn't compile on all platforms. https://codereview.chromium.org/14208017/diff/28001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:482: ASSERT_TRUE(it != cookie_map.end()); On 2013/04/19 21:15:28, huangs1 wrote: > ASSERT_NE(cookie_map.end(), cookie_map.find(HighName)); Alas, using the gtest macros for iterator comparisons doesn't compile on all platforms.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerm@chromium.org/14208017/63001
Message was sent while issue was closed.
Change committed as 195954 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
