Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(234)

Issue 10407124: Don't force non-session only cookies to be session only cookies, instead delete on shutdown (Closed)

Created:
8 years, 7 months ago by jochen (gone - plz use gerrit)
Modified:
8 years, 6 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, wtc, rkn, markusheintz_
Visibility:
Public.

Description

Don't force non-session only cookies to be session only cookies, instead delete on shutdown this is more consistent with how we treat other web site data types All storage backends except for cookies use the SpecialStoragePolicy to decide what data to delete and what to keep. Also, since they commonly don't have a session only notion, they will delete the data on shutdown. This CL unifies the way content settings are applied to storage systems by passing a SpecialStoragePolicy to the cookie store backend, and delete cookies on shutdown. BUG=129349 TEST=SQLitePersistentCookieStoreTest.TestSpecialStoragePolicy Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139544

Patch Set 1 #

Total comments: 8

Patch Set 2 : updates #

Total comments: 7

Patch Set 3 : updates #

Patch Set 4 : fix tests #

Patch Set 5 : updates #

Patch Set 6 : include new files #

Patch Set 7 : fix tests #

Total comments: 10

Patch Set 8 : updates #

Unified diffs Side-by-side diffs Delta from patch set Stats (+565 lines, -58 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/content_settings/host_content_settings_map.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/content_settings/host_content_settings_map.cc View 1 2 3 4 1 chunk +24 lines, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.cc View 1 chunk +0 lines, -3 lines 0 comments Download
A chrome/browser/net/clear_on_exit_policy.h View 1 2 3 4 5 1 chunk +47 lines, -0 lines 0 comments Download
A chrome/browser/net/clear_on_exit_policy.cc View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
A chrome/browser/net/clear_on_exit_policy_unittest.cc View 1 2 3 4 5 1 chunk +46 lines, -0 lines 0 comments Download
M chrome/browser/net/sqlite_persistent_cookie_store.h View 1 2 3 4 1 chunk +9 lines, -2 lines 0 comments Download
M chrome/browser/net/sqlite_persistent_cookie_store.cc View 1 2 3 4 5 6 7 10 chunks +74 lines, -6 lines 0 comments Download
M chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc View 4 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc View 1 2 3 4 20 chunks +100 lines, -20 lines 0 comments Download
M chrome/browser/net/sqlite_server_bound_cert_store.h View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
M chrome/browser/net/sqlite_server_bound_cert_store.cc View 1 2 3 4 5 chunks +65 lines, -4 lines 0 comments Download
M chrome/browser/net/sqlite_server_bound_cert_store_unittest.cc View 1 2 3 4 5 6 7 8 chunks +113 lines, -6 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/profiles/profile_impl_io_data.h View 3 chunks +7 lines, -1 line 0 comments Download
M chrome/browser/profiles/profile_impl_io_data.cc View 1 2 3 4 8 chunks +12 lines, -5 lines 0 comments Download
M chrome/browser/profiles/profile_manager.cc View 1 2 3 4 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_service.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/test/functional/cookies.py View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 21 (0 generated)
jochen (gone - plz use gerrit)
plz review I'll add a patch tomorrow
8 years, 7 months ago (2012-05-23 15:47:37 UTC) #1
willchan no longer on Chromium
profiles/ LGTM jochen tells me Erik will handle c/b/n/ which SGTM Asked jochen to update ...
8 years, 7 months ago (2012-05-23 15:59:45 UTC) #2
erikwright (departed)
From a reliability point of view, deleting on startup (in addition to or instead of ...
8 years, 7 months ago (2012-05-23 17:00:06 UTC) #3
jochen (gone - plz use gerrit)
On 2012/05/23 17:00:06, erikwright wrote: > From a reliability point of view, deleting on startup ...
8 years, 7 months ago (2012-05-23 17:49:27 UTC) #4
jochen (gone - plz use gerrit)
PTAL I updated the descriptino, and added tests. I also left some comments explaining why ...
8 years, 7 months ago (2012-05-24 11:04:03 UTC) #5
erikwright (departed)
Here are some draft comments from the previous patchset. I'll look at the new one ...
8 years, 7 months ago (2012-05-24 11:07:15 UTC) #6
jochen (gone - plz use gerrit)
https://chromiumcodereview.appspot.com/10407124/diff/1/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): https://chromiumcodereview.appspot.com/10407124/diff/1/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode843 chrome/browser/net/sqlite_persistent_cookie_store.cc:843: std::map<std::string, int> cookies_per_domain; On 2012/05/24 11:07:19, erikwright wrote: > ...
8 years, 7 months ago (2012-05-24 11:34:54 UTC) #7
erikwright (departed)
http://codereview.chromium.org/10407124/diff/8001/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/10407124/diff/8001/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode973 chrome/browser/net/sqlite_persistent_cookie_store.cc:973: GURL url( On 2012/05/24 11:04:03, jochen wrote: > compare ...
8 years, 7 months ago (2012-05-24 17:36:58 UTC) #8
jochen (gone - plz use gerrit)
On 2012/05/24 17:36:58, erikwright wrote: > http://codereview.chromium.org/10407124/diff/8001/chrome/browser/net/sqlite_persistent_cookie_store.cc > File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): > > http://codereview.chromium.org/10407124/diff/8001/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode973 > ...
8 years, 7 months ago (2012-05-24 17:53:43 UTC) #9
erikwright (departed)
That sounds right to me too. Thanks! On Thu, May 24, 2012 at 1:53 PM, ...
8 years, 7 months ago (2012-05-24 17:59:23 UTC) #10
jochen (gone - plz use gerrit)
I added ClearOnExitPolicy that wraps the SpecialStoragePolicy and does the host ->url magic. I also ...
8 years, 7 months ago (2012-05-25 11:39:47 UTC) #11
jochen (gone - plz use gerrit)
Friendly ping
8 years, 6 months ago (2012-05-29 12:23:13 UTC) #12
erikwright (departed)
mmenke: this change introduces a "SELECT origin FROM origin_bound_certs" in the shutdown of the OB ...
8 years, 6 months ago (2012-05-29 13:08:57 UTC) #13
mmenke
On 2012/05/29 13:08:57, erikwright wrote: > mmenke: this change introduces a "SELECT origin FROM origin_bound_certs" ...
8 years, 6 months ago (2012-05-29 13:29:43 UTC) #14
erikwright (departed)
[+mattm, mmenke to bcc] Sure do! On May 29, 2012 9:29 AM, <mmenke@chromium.org> wrote: > ...
8 years, 6 months ago (2012-05-29 13:33:42 UTC) #15
mattm
The ob cert store is expected to be fairly small (it selects the whole thing ...
8 years, 6 months ago (2012-05-29 19:56:58 UTC) #16
erikwright (departed)
On 2012/05/29 19:56:58, mattm wrote: > The ob cert store is expected to be fairly ...
8 years, 6 months ago (2012-05-29 20:05:40 UTC) #17
jochen (gone - plz use gerrit)
http://codereview.chromium.org/10407124/diff/13044/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (left): http://codereview.chromium.org/10407124/diff/13044/chrome/browser/net/chrome_network_delegate.cc#oldcode307 chrome/browser/net/chrome_network_delegate.cc:307: options->set_force_session(); On 2012/05/29 13:09:03, erikwright wrote: > It looks ...
8 years, 6 months ago (2012-05-30 11:28:42 UTC) #18
marja
lgtm
8 years, 6 months ago (2012-05-30 11:44:06 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jochen@chromium.org/10407124/25001
8 years, 6 months ago (2012-05-30 13:46:06 UTC) #20
commit-bot: I haz the power
8 years, 6 months ago (2012-05-30 15:48:52 UTC) #21
Change committed as 139544

Powered by Google App Engine
This is Rietveld 408576698