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

Issue 2861063003: Remove dangerous CanonicalCookie::Create method. (Closed)

Created:
3 years, 7 months ago by Randy Smith (Not in Mondays)
Modified:
3 years, 7 months ago
CC:
cbentzel+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, net-reviews_chromium.org, msarda, droger
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove dangerous CanonicalCookie::Create method. Remove the CanonicalCookie::Create method that takes a URL and a domain, creating host cookies if the domain is null and domain cookies if it isn't. Usage was almost completely confined to tests. BUG=None R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2861063003 Cr-Commit-Position: refs/heads/master@{#471378} Committed: https://chromium.googlesource.com/chromium/src/+/b4ecb4c3b8454ef2f6535d820131dfa57e3e6118

Patch Set 1 #

Patch Set 2 : Content unittest changes. #

Total comments: 2

Patch Set 3 : Fixed try jobs and braino in browsing data remover test. #

Patch Set 4 : Checkpoint to run iOS tryjobs. #

Patch Set 5 : No known bugs. #

Patch Set 6 : Rework exist pathway from iOS implementation. #

Patch Set 7 : Fix CC constructor iOS use. #

Patch Set 8 : Fixes from self-review. #

Patch Set 9 : Fixed bugs from DCHECKing name & path. #

Total comments: 14

Patch Set 10 : Incorporated all comments. #

Total comments: 2

Patch Set 11 : Moved test to shared file. #

Total comments: 2

Patch Set 12 : Use creation_time for last_access_time as per Elly's suggestion. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+359 lines, -374 lines) Patch
M chrome/browser/extensions/api/cookies/cookies_unittest.cc View 1 2 3 4 5 6 7 8 6 chunks +19 lines, -23 lines 0 comments Download
M components/signin/core/browser/gaia_cookie_manager_service.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/browsing_data/browsing_data_remover_impl_unittest.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/net/quota_policy_cookie_store_unittest.cc View 1 7 chunks +14 lines, -15 lines 0 comments Download
M headless/public/util/generic_url_request_job_test.cc View 1 2 3 4 5 6 7 8 9 1 chunk +14 lines, -14 lines 0 comments Download
M ios/net/cookies/cookie_cache_unittest.cc View 1 2 3 1 chunk +4 lines, -4 lines 0 comments Download
M ios/net/cookies/cookie_store_ios.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +34 lines, -3 lines 0 comments Download
M ios/net/cookies/cookie_store_ios_test_util.mm View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M net/cookies/canonical_cookie.h View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -17 lines 0 comments Download
M net/cookies/canonical_cookie.cc View 1 2 3 4 5 6 7 5 chunks +35 lines, -93 lines 0 comments Download
M net/cookies/canonical_cookie_unittest.cc View 10 chunks +49 lines, -58 lines 0 comments Download
M net/cookies/cookie_monster.cc View 1 2 3 4 5 6 7 8 9 1 chunk +32 lines, -5 lines 0 comments Download
M net/cookies/cookie_monster_store_test.cc View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -5 lines 0 comments Download
M net/cookies/cookie_monster_unittest.cc View 10 4 chunks +46 lines, -46 lines 0 comments Download
M net/cookies/cookie_store_unittest.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +29 lines, -0 lines 0 comments Download
M net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -3 lines 0 comments Download
M net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc View 20 chunks +65 lines, -81 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 75 (52 generated)
mmenke
Know this isn't ready for review yet, but your question made me curious https://codereview.chromium.org/2861063003/diff/20001/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc File ...
3 years, 7 months ago (2017-05-05 17:51:57 UTC) #9
Randy Smith (Not in Mondays)
Thanks for the pointer! Fixed that and all the failing try jobs. https://codereview.chromium.org/2861063003/diff/20001/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc File content/browser/browsing_data/browsing_data_remover_impl_unittest.cc ...
3 years, 7 months ago (2017-05-05 21:12:58 UTC) #11
Randy Smith (Not in Mondays)
Sorry, to be clear: Not yet asking for review until the try jobs actually show ...
3 years, 7 months ago (2017-05-05 21:13:41 UTC) #13
mmenke
On 2017/05/05 21:13:41, Randy Smith (Not in Mondays) wrote: > Sorry, to be clear: Not ...
3 years, 7 months ago (2017-05-05 23:12:58 UTC) #16
Randy Smith (Not in Mondays)
On 2017/05/05 23:12:58, mmenke wrote: > On 2017/05/05 21:13:41, Randy Smith (Not in Mondays) wrote: ...
3 years, 7 months ago (2017-05-06 00:04:29 UTC) #17
Randy Smith (Not in Mondays)
I'm going to jump the gun (all bots green but one, which is still yellow) ...
3 years, 7 months ago (2017-05-09 03:25:19 UTC) #38
mmenke
https://codereview.chromium.org/2861063003/diff/160001/headless/public/util/generic_url_request_job_test.cc File headless/public/util/generic_url_request_job_test.cc (right): https://codereview.chromium.org/2861063003/diff/160001/headless/public/util/generic_url_request_job_test.cc#newcode418 headless/public/util/generic_url_request_job_test.cc:418: "bad_path_cookie", "7", "example.com", "/gadgets", base::Time(), .example.com https://codereview.chromium.org/2861063003/diff/160001/net/cookies/canonical_cookie.h File net/cookies/canonical_cookie.h ...
3 years, 7 months ago (2017-05-09 16:51:09 UTC) #41
Randy Smith (Not in Mondays)
Next round; PTAL? https://codereview.chromium.org/2861063003/diff/160001/headless/public/util/generic_url_request_job_test.cc File headless/public/util/generic_url_request_job_test.cc (right): https://codereview.chromium.org/2861063003/diff/160001/headless/public/util/generic_url_request_job_test.cc#newcode418 headless/public/util/generic_url_request_job_test.cc:418: "bad_path_cookie", "7", "example.com", "/gadgets", base::Time(), On ...
3 years, 7 months ago (2017-05-09 23:52:36 UTC) #43
mmenke
Just one comment, otherwise, looks good. https://codereview.chromium.org/2861063003/diff/180001/net/cookies/cookie_monster_unittest.cc File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/2861063003/diff/180001/net/cookies/cookie_monster_unittest.cc#newcode3224 net/cookies/cookie_monster_unittest.cc:3224: TEST_F(CookieMonsterTest, SetCookieWithDetailsSecureEnforcement) { ...
3 years, 7 months ago (2017-05-10 15:51:55 UTC) #47
Randy Smith (Not in Mondays)
PTA(nother)L? https://codereview.chromium.org/2861063003/diff/180001/net/cookies/cookie_monster_unittest.cc File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/2861063003/diff/180001/net/cookies/cookie_monster_unittest.cc#newcode3224 net/cookies/cookie_monster_unittest.cc:3224: TEST_F(CookieMonsterTest, SetCookieWithDetailsSecureEnforcement) { On 2017/05/10 15:51:54, mmenke wrote: ...
3 years, 7 months ago (2017-05-10 20:31:52 UTC) #50
mmenke
LGTM
3 years, 7 months ago (2017-05-10 20:33:19 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2861063003/200001
3 years, 7 months ago (2017-05-10 21:57:05 UTC) #57
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/433174)
3 years, 7 months ago (2017-05-10 22:17:53 UTC) #59
Randy Smith (Not in Mondays)
+droger: ios/net +mkwst: chrome/browser/extensions/api/cookies, content/browser/browsing_data +jam: content/browser/net (Not sure why this isn't under net/OWNERS, but ...
3 years, 7 months ago (2017-05-10 23:39:09 UTC) #61
alex clarke (OOO till 29th)
headless/ LGTM
3 years, 7 months ago (2017-05-11 07:12:35 UTC) #62
Mike West
//chrome/browser/extensions/api/cookies and //content/browser/browsing_data LGTM.
3 years, 7 months ago (2017-05-11 07:42:23 UTC) #63
jam
lgtm
3 years, 7 months ago (2017-05-11 15:00:11 UTC) #64
Randy Smith (Not in Mondays)
Elly, would you be willing to look at ios/net? Roger, would you be willing to ...
3 years, 7 months ago (2017-05-11 16:26:37 UTC) #66
Elly Fong-Jones
ios/net lgtm https://codereview.chromium.org/2861063003/diff/200001/ios/net/cookies/cookie_store_ios.mm File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2861063003/diff/200001/ios/net/cookies/cookie_store_ios.mm#newcode412 ios/net/cookies/cookie_store_ios.mm:412: // TODO(rdsmith): Check with ios reviewer about ...
3 years, 7 months ago (2017-05-11 17:24:35 UTC) #67
Roger Tawa OOO till Jul 10th
components/signin lgtm
3 years, 7 months ago (2017-05-12 16:47:37 UTC) #68
Randy Smith (Not in Mondays)
Thanks, all! https://codereview.chromium.org/2861063003/diff/200001/ios/net/cookies/cookie_store_ios.mm File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2861063003/diff/200001/ios/net/cookies/cookie_store_ios.mm#newcode412 ios/net/cookies/cookie_store_ios.mm:412: // TODO(rdsmith): Check with ios reviewer about ...
3 years, 7 months ago (2017-05-12 17:02:02 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2861063003/220001
3 years, 7 months ago (2017-05-12 17:03:02 UTC) #72
commit-bot: I haz the power
3 years, 7 months ago (2017-05-12 18:39:21 UTC) #75
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/b4ecb4c3b8454ef2f6535d820131...

Powered by Google App Engine
This is Rietveld 408576698