Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2909533002:
Fix loading bad cookies from disk. (Closed)
|
Created:
3 years, 7 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 6 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix loading bad cookies from disk.
BUG=723734
R=mmenke@chromium.org
Review-Url: https://codereview.chromium.org/2909533002
Cr-Commit-Position: refs/heads/master@{#481043}
Committed: https://chromium.googlesource.com/chromium/src/+/3df4c7446b9f3421636917277428f71a306a857f
Patch Set 1 #Patch Set 2 : Rebased on top of dependent branch. #Patch Set 3 : Rebased on top of dependency CL. #Patch Set 4 : Bring into alignment with new canonical definition. #Patch Set 5 : Rebased on top of latest PS in dependency CL. #
Total comments: 4
Patch Set 6 : Rebased onto dependency CL (@ pp480954). #Patch Set 7 : Use C++ loop syntax #
Depends on Patchset: Messages
Total messages: 25 (15 generated)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Just fixing getting bad cookies in from disk. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/25 19:53:02, Randy Smith (Not in Mondays) wrote: > Just fixing getting bad cookies in from disk. PTAL. Going to put off this one until we've ironed out IsCanonical.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Matt: Given that you've stamped CLs both before and after this one in the dependency chain, I wanted to make sure you remembered this one. It's not one that I'm actually dependent on, so if for some reason you don't like it I'm happy to drop it. I created it when I thought we were seeing instances of bad data being restored from disk, before I realized that null name cookies were supported.
On 2017/06/16 21:59:28, Randy Smith (Not in Mondays) wrote: > Matt: Given that you've stamped CLs both before and after this one in the > dependency chain, I wanted to make sure you remembered this one. > > It's not one that I'm actually dependent on, so if for some reason you don't > like it I'm happy to drop it. I created it when I thought we were seeing > instances of bad data being restored from disk, before I realized that null name > cookies were supported. Sorry, I hadn't realized this one was still alive. Don't hesitate to ping me earlier. Egads, been sitting for 3 weeks! Sorry about that.
On 2017/06/16 22:03:09, mmenke wrote: > On 2017/06/16 21:59:28, Randy Smith (Not in Mondays) wrote: > > Matt: Given that you've stamped CLs both before and after this one in the > > dependency chain, I wanted to make sure you remembered this one. > > > > It's not one that I'm actually dependent on, so if for some reason you don't > > like it I'm happy to drop it. I created it when I thought we were seeing > > instances of bad data being restored from disk, before I realized that null > name > > cookies were supported. > > Sorry, I hadn't realized this one was still alive. Don't hesitate to ping me > earlier. Egads, been sitting for 3 weeks! Sorry about that. Hey, you said you wanted to wait until after IsCanonical was done, and we just finished it! :-}
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:546: i++) { for (const auto& cookie_info : cookies_info) (Even without that C++11 niftiness, there's also arraysize, which is also prettier). https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:562: ASSERT_EQ(1U, cookies.size()); Hrm...Most of this file uses U, but there are a few cases of u. I think u is more common. :(
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Matt: Incorporated one of your comments, but not sure if the other was requesting a change? https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:546: i++) { On 2017/06/19 17:57:13, mmenke wrote: > for (const auto& cookie_info : cookies_info) > > (Even without that C++11 niftiness, there's also arraysize, which is also > prettier). Completely fair. Done. https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:562: ASSERT_EQ(1U, cookies.size()); On 2017/06/19 17:57:13, mmenke wrote: > Hrm...Most of this file uses U, but there are a few cases of u. I think u is > more common. :( I'm not sure if you're requesting a change? 'u' is almost certainly more common across the chromium codebase (haven't checked) but 'U' is more common in this file (20 instances of 'U' versus 4 of 'u'). Given those two facts I'm inclined to use U here (though I understand if that causes twitches). Let me know if you disagree.
On 2017/06/20 21:17:18, Randy Smith (Not in Mondays) wrote: > Matt: Incorporated one of your comments, but not sure if the other was > requesting a change? > > https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... > File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right): > > https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:546: i++) { > On 2017/06/19 17:57:13, mmenke wrote: > > for (const auto& cookie_info : cookies_info) > > > > (Even without that C++11 niftiness, there's also arraysize, which is also > > prettier). > > Completely fair. Done. > > https://codereview.chromium.org/2909533002/diff/80001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:562: ASSERT_EQ(1U, > cookies.size()); > On 2017/06/19 17:57:13, mmenke wrote: > > Hrm...Most of this file uses U, but there are a few cases of u. I think u is > > more common. :( > > I'm not sure if you're requesting a change? 'u' is almost certainly more common > across the chromium codebase (haven't checked) but 'U' is more common in this > file (20 instances of 'U' versus 4 of 'u'). Given those two facts I'm inclined > to use U here (though I understand if that causes twitches). Let me know if you > disagree. That was just in case you felt like doing anything about it. Leaving well enough is fine with me, still LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498004796077350,
"parent_rev": "2f120a7f827522fa4b6e724b87a399615b86e8ae", "commit_rev":
"3df4c7446b9f3421636917277428f71a306a857f"}
Message was sent while issue was closed.
Description was changed from ========== Fix loading bad cookies from disk. BUG=723734 R=mmenke@chromium.org ========== to ========== Fix loading bad cookies from disk. BUG=723734 R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2909533002 Cr-Commit-Position: refs/heads/master@{#481043} Committed: https://chromium.googlesource.com/chromium/src/+/3df4c7446b9f3421636917277428... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3df4c7446b9f3421636917277428... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
