Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2030233002:
net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method - WIP (Closed)
Descriptionnet: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method
This patch converts the usage of CanonicalCookie's constructor in
CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method.
We are doing this because Create() is more robust, as it does
validation of the parameters and return NULL as an error signal
if they fail to comply with the checks.
BUG=57061
TEST=net_unittests
R=mmenke@chromium.org
Committed: https://crrev.com/aa0b9f052e9e4a0ac0ec38a41ba44c8be1f1bb16
Cr-Commit-Position: refs/heads/master@{#397771}
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix CookieMonsterStrict test #
Total comments: 2
Patch Set 3 : fix GarbageCollectionTriggers test #Messages
Total messages: 15 (6 generated)
Matt, could you take a look at this when you have time? I'm really hacking in poor conditions, with an iPad and a ssh connection. https://codereview.chromium.org/2030233002/diff/1/net/cookies/cookie_monster_... File net/cookies/cookie_monster_store_test.cc (right): https://codereview.chromium.org/2030233002/diff/1/net/cookies/cookie_monster_... net/cookies/cookie_monster_store_test.cc:255: store->AddCookie(*cc.release()); I'm really missing something here. The test fails with release(). I have also tried with just *cc, but this also fails. Do you know the way to get this to work?
https://codereview.chromium.org/2030233002/diff/1/net/cookies/cookie_monster_... File net/cookies/cookie_monster_store_test.cc (right): https://codereview.chromium.org/2030233002/diff/1/net/cookies/cookie_monster_... net/cookies/cookie_monster_store_test.cc:255: store->AddCookie(*cc.release()); On 2016/06/02 21:38:48, tfarina wrote: > I'm really missing something here. The test fails with release(). I have also > tried with just *cc, but this also fails. > > Do you know the way to get this to work? You need to make GURL be a valid URL(Something like GURL(base::StringPrintf("http://h%05d.izzle/", i))), and not set domain (Since it's not a domain with a leading "."). And you shouldn't be using release. CanonicalCookie::Create and "new CanonicalCookie" treat GURL and domain fairly differently, and that's the main reason to get rid of them - they're very confusing (Though I didn't realize quite how confusing until you took this on).
Description was changed from ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method - WIP BUG= TEST=net_unittests R=mmenke@chromium.org ========== to ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method - WIP net: migrate cookie_monster_unittest.cc to CanonicalCookie::Create() method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ==========
Description was changed from ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method - WIP net: migrate cookie_monster_unittest.cc to CanonicalCookie::Create() method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ========== to ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method - WIP This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ==========
https://codereview.chromium.org/2030233002/diff/1/net/cookies/cookie_monster_... File net/cookies/cookie_monster_store_test.cc (right): https://codereview.chromium.org/2030233002/diff/1/net/cookies/cookie_monster_... net/cookies/cookie_monster_store_test.cc:255: store->AddCookie(*cc.release()); On 2016/06/02 21:56:50, mmenke wrote: > On 2016/06/02 21:38:48, tfarina wrote: > > I'm really missing something here. The test fails with release(). I have also > > tried with just *cc, but this also fails. > > > > Do you know the way to get this to work? > > You need to make GURL be a valid URL(Something like > GURL(base::StringPrintf("http://h%05d.izzle/", i))), and not set domain (Since > it's not a domain with a leading "."). > > And you shouldn't be using release. > > CanonicalCookie::Create and "new CanonicalCookie" treat GURL and domain fairly > differently, and that's the main reason to get rid of them - they're very > confusing (Though I didn't realize quite how confusing until you took this on). Done. Thank you.
Sorry, didn't realize this last pass. https://codereview.chromium.org/2030233002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster_store_test.cc (right): https://codereview.chromium.org/2030233002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_store_test.cc:254: CookieSameSite::DEFAULT_MODE, false, COOKIE_PRIORITY_DEFAULT)); Think you still need to set the last access date with cc->SetLastAccessDate.
https://codereview.chromium.org/2030233002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster_store_test.cc (right): https://codereview.chromium.org/2030233002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_store_test.cc:254: CookieSameSite::DEFAULT_MODE, false, COOKIE_PRIORITY_DEFAULT)); On 2016/06/03 12:00:06, mmenke wrote: > Think you still need to set the last access date with cc->SetLastAccessDate. Awesome. Thanks Matt. This fixed it. Done.
LGTM, assuming all tests pass.
Description was changed from ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method - WIP This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ========== to ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ==========
The CQ bit was checked by tfarina@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030233002/40001
Message was sent while issue was closed.
Description was changed from ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ========== to ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org ========== to ========== net: migrate CreateMonsterFromStoreForGC() to CanonicalCookies::Create method This patch converts the usage of CanonicalCookie's constructor in CreateMonsterFromStoreForCG() to CanonicalCookie's Create() method. We are doing this because Create() is more robust, as it does validation of the parameters and return NULL as an error signal if they fail to comply with the checks. BUG=57061 TEST=net_unittests R=mmenke@chromium.org Committed: https://crrev.com/aa0b9f052e9e4a0ac0ec38a41ba44c8be1f1bb16 Cr-Commit-Position: refs/heads/master@{#397771} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aa0b9f052e9e4a0ac0ec38a41ba44c8be1f1bb16 Cr-Commit-Position: refs/heads/master@{#397771} |
