|
|
Descriptionnet: make CanonicalCookie's constructor private
Since almost everything was converted to Create method, now
it can be moved to private section.
While doing this, CanonicalCookieTest::Constructor had to be
converted to Create method as well.
BUG=57061
TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor
TEST=ios_net_unittests
R=mmenke@chromium.org
Committed: https://crrev.com/9c770cf31089d791900b601bbc94ce49b4673b4f
Cr-Commit-Position: refs/heads/master@{#407312}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix rebase #Patch Set 4 : fix ios #Patch Set 5 : fix cookie_store_ios_unittest.mm #Patch Set 6 : trying to fix ios_net_unittests #Patch Set 7 : android fix? #Patch Set 8 : rebase #
Total comments: 4
Patch Set 9 : matt fixes #Patch Set 10 : fix CookieUtil test? #Patch Set 11 : fix CookieStore test? #
Total comments: 7
Patch Set 12 : matt's review #
Total comments: 4
Patch Set 13 : fix CookieStoreIOS tests? #Patch Set 14 : leading slash #
Total comments: 2
Patch Set 15 : domain #
Messages
Total messages: 80 (64 generated)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by tfarina@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, when you have a spare time, could you take a look at the astronomical failures in ios_net_unittests? As I should have antecipated, there were more uses than codesearch could find (this is a thing that Grok should learn one day, how to index on other platforms like Mac and Windows).
Not a full review, just trying to figure out what's going wrong on iOS. https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/cookie_... File ios/net/cookies/cookie_cache_unittest.cc (right): https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/cookie_... ios/net/cookies/cookie_cache_unittest.cc:20: return *CanonicalCookie::Create(url, name, value, url.host(), url.path(), url.host() should be std::string(), to maintain behavior. https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/system_... File ios/net/cookies/system_cookie_util.mm (right): https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/system_... ios/net/cookies/system_cookie_util.mm:70: GURL("http://example.com"), base::SysNSStringToUTF8([cookie name]), I think this one is the issue. Switch it to using the constructor that doesn't take a GURL, and I think iOS will magically work. (i.e, just delete the first argument)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by tfarina@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...
Still in fire. I have sent an email to Sylvain asking for help on how to run the test under iossim to see if I can reproduce locally, so I can better look at it. https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/cookie_... File ios/net/cookies/cookie_cache_unittest.cc (right): https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/cookie_... ios/net/cookies/cookie_cache_unittest.cc:20: return *CanonicalCookie::Create(url, name, value, url.host(), url.path(), On 2016/07/20 20:50:11, mmenke wrote: > url.host() should be std::string(), to maintain behavior. Done. https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/system_... File ios/net/cookies/system_cookie_util.mm (right): https://codereview.chromium.org/2159373002/diff/70005/ios/net/cookies/system_... ios/net/cookies/system_cookie_util.mm:70: GURL("http://example.com"), base::SysNSStringToUTF8([cookie name]), On 2016/07/20 20:50:11, mmenke wrote: > I think this one is the issue. Switch it to using the constructor that doesn't > take a GURL, and I think iOS will magically work. (i.e, just delete the first > argument) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by tfarina@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! Good work on these. https://codereview.chromium.org/2159373002/diff/180001/chrome/browser/android... File chrome/browser/android/cookies/cookies_fetcher.cc (right): https://codereview.chromium.org/2159373002/diff/180001/chrome/browser/android... chrome/browser/android/cookies/cookies_fetcher.cc:161: GURL("http://example.com"), base::android::ConvertJavaStringToUTF8(env, name), Should just remove this GURL. Looks like no tests are running this code, or they'd fail. :( https://codereview.chromium.org/2159373002/diff/180001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://codereview.chromium.org/2159373002/diff/180001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios_unittest.mm:263: net::CanonicalCookie::Create( To keep old behavior, add "GURL("http://domain/")" as the first parmeter. https://codereview.chromium.org/2159373002/diff/180001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2159373002/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:132: CanonicalCookie::CanonicalCookie(const GURL& url, nit: This should be moved down below FullCompare, to match declaration order.
The CQ bit was checked by tfarina@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...
tfarina@chromium.org changed reviewers: + dfalcantara@chromium.org, droger@chromium.org
Adding onwers for android and ios changes. c/b/android/cookies -> dfalcantara ios/net/cookies -> droger https://codereview.chromium.org/2159373002/diff/180001/chrome/browser/android... File chrome/browser/android/cookies/cookies_fetcher.cc (right): https://codereview.chromium.org/2159373002/diff/180001/chrome/browser/android... chrome/browser/android/cookies/cookies_fetcher.cc:161: GURL("http://example.com"), base::android::ConvertJavaStringToUTF8(env, name), On 2016/07/21 15:34:57, mmenke wrote: > Should just remove this GURL. Looks like no tests are running this code, or > they'd fail. :( Done. https://codereview.chromium.org/2159373002/diff/180001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://codereview.chromium.org/2159373002/diff/180001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios_unittest.mm:263: net::CanonicalCookie::Create( On 2016/07/21 15:34:57, mmenke wrote: > To keep old behavior, add "GURL("http://domain/")" as the first parmeter. Done. https://codereview.chromium.org/2159373002/diff/180001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/2159373002/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:132: CanonicalCookie::CanonicalCookie(const GURL& url, On 2016/07/21 15:34:57, mmenke wrote: > nit: This should be moved down below FullCompare, to match declaration order. Looks like CanonPath is not at the right place either, but I'm not going to move it, just the code I'm touching. https://codereview.chromium.org/2159373002/diff/180001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:132: CanonicalCookie::CanonicalCookie(const GURL& url, On 2016/07/21 15:34:57, mmenke wrote: > nit: This should be moved down below FullCompare, to match declaration order. Done.
Android lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... ios/net/cookies/cookie_store_ios_unittest.mm:263: net::CanonicalCookie::Create(GURL("http://domain"), "name", Is this now returning NULL? Could it be because the domain is the empty string?
The CQ bit was checked by tfarina@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...
Description was changed from ========== net: make CanonicalCookie's constructor private Since almost everything was converted to Create method, now it can be moved to private section. While doing this, CanonicalCookieTest::Constructor had to be converted to Create method as well. BUG=57061 TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor R=mmenke@chromium.org ========== to ========== net: make CanonicalCookie's constructor private Since almost everything was converted to Create method, now it can be moved to private section. While doing this, CanonicalCookieTest::Constructor had to be converted to Create method as well. BUG=57061 TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor TEST=ios_net_unittests R=mmenke@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... ios/net/cookies/cookie_store_ios_unittest.mm:263: net::CanonicalCookie::Create(GURL("http://domain"), "name", On 2016/07/22 08:58:10, droger wrote: > Is this now returning NULL? > > Could it be because the domain is the empty string? I added it back but I think it still returns NULL. Matt, could you advise here what is the right fix for creating a bad canonical cookie?
https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... ios/net/cookies/cookie_store_ios_unittest.mm:263: net::CanonicalCookie::Create(GURL("http://domain"), "name", On 2016/07/22 12:37:15, tfarina wrote: > On 2016/07/22 08:58:10, droger wrote: > > Is this now returning NULL? > > > > Could it be because the domain is the empty string? > I added it back but I think it still returns NULL. Matt, could you advise here > what is the right fix for creating a bad canonical cookie? I think the path is invalid - it needs a leading slash (i.e. "/path/"). Code so often creating invalid cookies is just the reason for switching to the validating constructor.
The CQ bit was checked by tfarina@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...
droger, could you take another look? https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://chromiumcodereview.appspot.com/2159373002/diff/200001/ios/net/cookies... ios/net/cookies/cookie_store_ios_unittest.mm:263: net::CanonicalCookie::Create(GURL("http://domain"), "name", On 2016/07/22 14:52:44, mmenke wrote: > On 2016/07/22 12:37:15, tfarina wrote: > > On 2016/07/22 08:58:10, droger wrote: > > > Is this now returning NULL? > > > > > > Could it be because the domain is the empty string? > > I added it back but I think it still returns NULL. Matt, could you advise here > > what is the right fix for creating a bad canonical cookie? > > I think the path is invalid - it needs a leading slash (i.e. "/path/"). Code so > often creating invalid cookies is just the reason for switching to the > validating constructor. Thanks Matt! ios_net_unittests is green again with this.
https://codereview.chromium.org/2159373002/diff/240001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://codereview.chromium.org/2159373002/diff/240001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios_unittest.mm:264: "\x81r\xe4\xbd\xa0\xe5\xa5\xbd", "domain", Could you either replace "domain" with std::string() or make it ".domain", so it corresponds to a valid cookie line (Currently "domain" is mapped to ".domain" due to buggy servers - I'd suggest just using std::string() to match old behavior, but doesn't really matter).
lgtm
The CQ bit was checked by tfarina@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...
https://codereview.chromium.org/2159373002/diff/240001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://codereview.chromium.org/2159373002/diff/240001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios_unittest.mm:264: "\x81r\xe4\xbd\xa0\xe5\xa5\xbd", "domain", On 2016/07/22 20:10:29, mmenke wrote: > Could you either replace "domain" with std::string() or make it ".domain", so it > corresponds to a valid cookie line (Currently "domain" is mapped to ".domain" > due to buggy servers - I'd suggest just using std::string() to match old > behavior, but doesn't really matter). Done.
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 tfarina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, dfalcantara@chromium.org, droger@chromium.org Link to the patchset: https://codereview.chromium.org/2159373002/#ps260001 (title: "domain")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== net: make CanonicalCookie's constructor private Since almost everything was converted to Create method, now it can be moved to private section. While doing this, CanonicalCookieTest::Constructor had to be converted to Create method as well. BUG=57061 TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor TEST=ios_net_unittests R=mmenke@chromium.org ========== to ========== net: make CanonicalCookie's constructor private Since almost everything was converted to Create method, now it can be moved to private section. While doing this, CanonicalCookieTest::Constructor had to be converted to Create method as well. BUG=57061 TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor TEST=ios_net_unittests R=mmenke@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #15 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== net: make CanonicalCookie's constructor private Since almost everything was converted to Create method, now it can be moved to private section. While doing this, CanonicalCookieTest::Constructor had to be converted to Create method as well. BUG=57061 TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor TEST=ios_net_unittests R=mmenke@chromium.org ========== to ========== net: make CanonicalCookie's constructor private Since almost everything was converted to Create method, now it can be moved to private section. While doing this, CanonicalCookieTest::Constructor had to be converted to Create method as well. BUG=57061 TEST=net_unittests --gtest_filter=CanonicalCookieTest.Constructor TEST=ios_net_unittests R=mmenke@chromium.org Committed: https://crrev.com/9c770cf31089d791900b601bbc94ce49b4673b4f Cr-Commit-Position: refs/heads/master@{#407312} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9c770cf31089d791900b601bbc94ce49b4673b4f Cr-Commit-Position: refs/heads/master@{#407312} |