Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2617853002:
[Cronet] Use CookieStoreIOS to sync cookies to system store. (Closed)
|
Created:
3 years, 11 months ago by mef Modified:
3 years, 11 months ago Reviewers:
kapishnikov CC:
chromium-reviews, cbentzel+watch_chromium.org, lilyhoughton Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Use CookieStoreIOS to sync cookies to system store.
BUG=666792
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2617853002
Cr-Commit-Position: refs/heads/master@{#441986}
Committed: https://chromium.googlesource.com/chromium/src/+/2989c3716678c88ff9415e0a5d04333b3afbc434
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add SetSystemCookie test, address comments. #
Total comments: 4
Patch Set 3 : Address Andrei's comments. #
Messages
Total messages: 26 (17 generated)
Description was changed from ========== [Cronet] Use CookieStoreIOS to sync cookies to system store. BUG=666792 ========== to ========== [Cronet] Use CookieStoreIOS to sync cookies to system store. BUG=666792 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by mef@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...
mef@chromium.org changed reviewers: + kapishnikov@chromium.org
PTAL. I think it would be nice to add another test to verify that cookies set in system store get sent by Cronet. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/05 23:20:31, mef wrote: > PTAL. > > I think it would be nice to add another test to verify that cookies set in > system store get sent by Cronet. > > WDYT? I agree. It will test the cookie sync with the system cookie store in both directions.
https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:45: #include "net/ssl/channel_id_service.h" Is this include used? https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_http_test.mm:268: EXPECT_FALSE([[cookie name] containsString:@"aaaa"]); Since 'isEqual' (or better 'isEqualToString') is used to search for the cookie below, I would suggest using the same method here for symmetry. https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_http_test.mm:292: if ([cookie.name isEqual:@"aaaa"]) { isEqualToString should be a little faster. https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_http_test.mm:297: EXPECT_TRUE([[systemCookie value] containsString:@"bbb"]); Why don't use isEqualToString instead of containsString?
Thanks, PTAL. https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:45: #include "net/ssl/channel_id_service.h" On 2017/01/06 15:24:45, kapishnikov wrote: > Is this include used? Yes, SetCookieAndChannelIdStores takes std::unique_ptr<ChannelIDService> which needs class definition for destructor. https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_http_test.mm:268: EXPECT_FALSE([[cookie name] containsString:@"aaaa"]); On 2017/01/06 15:24:45, kapishnikov wrote: > Since 'isEqual' (or better 'isEqualToString') is used to search for the cookie > below, I would suggest using the same method here for symmetry. Done. https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_http_test.mm:292: if ([cookie.name isEqual:@"aaaa"]) { On 2017/01/06 15:24:45, kapishnikov wrote: > isEqualToString should be a little faster. Done. https://codereview.chromium.org/2617853002/diff/1/components/cronet/ios/test/... components/cronet/ios/test/cronet_http_test.mm:297: EXPECT_TRUE([[systemCookie value] containsString:@"bbb"]); On 2017/01/06 15:24:45, kapishnikov wrote: > Why don't use isEqualToString instead of containsString? Done.
The CQ bit was checked by mef@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/2617853002/diff/20001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2617853002/diff/20001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:315: EXPECT_TRUE([[delegate_ responseBody] isEqualToString:@"cccc=dddd"]); I would use containsString here since the cookie header value may contain some extra characters. https://codereview.chromium.org/2617853002/diff/20001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:316: [systemCookieStorage deleteCookie:systemCookie]; If the above 'EXPECTs' fail, the cookie is not going to be deleted. As the result, the test rerun may not test what we want, namely adding the cookie after the chromium cookie store has been initialized. We should move 'deleteCookie' up. I am also thinking, maybe we should generate the cookie name and value randomly with some predefined prefix. Thus, we can guarantee that the test will not be impacted by some residual values.
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 mef@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...
Thanks, PTAL. https://codereview.chromium.org/2617853002/diff/20001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2617853002/diff/20001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:315: EXPECT_TRUE([[delegate_ responseBody] isEqualToString:@"cccc=dddd"]); On 2017/01/06 16:23:17, kapishnikov wrote: > I would use containsString here since the cookie header value may contain some > extra characters. Done. https://codereview.chromium.org/2617853002/diff/20001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:316: [systemCookieStorage deleteCookie:systemCookie]; On 2017/01/06 16:23:17, kapishnikov wrote: > If the above 'EXPECTs' fail, the cookie is not going to be deleted. As the > result, the test rerun may not test what we want, namely adding the cookie after > the chromium cookie store has been initialized. We should move 'deleteCookie' > up. > > I am also thinking, maybe we should generate the cookie name and value randomly > with some predefined prefix. Thus, we can guarantee that the test will not be > impacted by some residual values. Done.
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 mef@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": 40001, "attempt_start_ts": 1483727446656300,
"parent_rev": "7c03b2072b7497184e8a1127c8ab443aeae42a96", "commit_rev":
"2989c3716678c88ff9415e0a5d04333b3afbc434"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Use CookieStoreIOS to sync cookies to system store. BUG=666792 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Use CookieStoreIOS to sync cookies to system store. BUG=666792 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2617853002 Cr-Commit-Position: refs/heads/master@{#441986} Committed: https://chromium.googlesource.com/chromium/src/+/2989c3716678c88ff9415e0a5d04... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2989c3716678c88ff9415e0a5d04... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
