Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2649083002:
Divide CookieStoreIOS into two different classes with different backends (Closed)
|
Created:
3 years, 11 months ago by maksims (do not use this acc) Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org, lilyhoughton Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDivide CookieStoreIOS into two different classes with different backends
CookieStoreIOS is used as a synchronized object with NSHTTPCookieStorge,
and CookieStoreIOSPersistent is a class that uses CookieMonster as it's
backend.
BUG=679736
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2649083002
Cr-Commit-Position: refs/heads/master@{#446966}
Committed: https://chromium.googlesource.com/chromium/src/+/fdd9f091afe9235efdfc1c1fce584d560ab9523f
Patch Set 1 #
Total comments: 20
Patch Set 2 : Eugene's comments #
Total comments: 14
Patch Set 3 : nits #Patch Set 4 : fix typo #Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : rebased and added todo #Patch Set 7 : fix compilation #
Total comments: 4
Messages
Total messages: 68 (43 generated)
The CQ bit was checked by maksim.sisov@intel.com 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.
maksim.sisov@intel.com changed reviewers: + eugenebut@chromium.org, mmenke@chromium.org
please take a look
On 2017/01/23 12:43:41, maksims wrote: > please take a look I'm happy to review this, but I'll wait for Eugene to do the first pass, to make sure this is along the lines he was thinking.
Thank you for doing this! General direction lg. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios.h (left): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.h:71: static std::unique_ptr<CookieStoreIOS> CreateCookieStore( This will break compilation in close source Chrome for iOS repo: https://bugs.chromium.org/p/chromium/issues/detail?id=679736#c2 Could you please do these things: 1.) Leave this method for now. Its implementation should call CookieStoreIOS(NSHTTPCookieStorage*) constructor to be compatible with existing code. 2.) Add TODO(crbug.com/683964): Remove this method. Then I will remove CreateCookieStore call from that close-source repo and will clean up this method. Thanks! https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios.h (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.h:54: // Create an instance of CookieStoreIOS that is generated from the cookies nit: s/Create/Creates I realize that this is just moving the code, but still would be great to fix. From Style Guide: These comments should be descriptive ("Opens the file") rather than imperative ("Open the file"); https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.h:180: SynchronizationState synchronization_state_; I think we can remove this member as well (see comments in the implementation). If you want to do this in a separate CL, then please add TODOs to the code. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.mm:605: // Copy the cookies from the global cookie store to |cookie_monster_|. Should this code go to subclass? https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.mm:624: DCHECK_EQ(SYNCHRONIZED, synchronization_state_); ditto https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.mm:651: if (synchronization_state_ != SYNCHRONIZED || ditto https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios_persistent.h (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_persistent.h:11: #include "ios/net/cookies/cookie_store_ios.h" s/include/import This is because cookie_store_ios.h is an Objective-C header. https://google.github.io/styleguide/objcguide.xml#_import_and__include https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios_persistent.mm (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_persistent.mm:5: #include "ios/net/cookies/cookie_store_ios_persistent.h" s/include/import https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_unittest.mm:17: #include "ios/net/cookies/cookie_store_ios_persistent.h" s/include/import https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_unittest.mm:234: store_(new net::CookieStoreIOSPersistent(backend_.get())) { Could you please use MakeUnique here and in other 2 places where the code was changed.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com 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/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios.h (left): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.h:71: static std::unique_ptr<CookieStoreIOS> CreateCookieStore( On 2017/01/23 17:37:47, Eugene But wrote: > This will break compilation in close source Chrome for iOS repo: > https://bugs.chromium.org/p/chromium/issues/detail?id=679736#c2 > > Could you please do these things: > 1.) Leave this method for now. Its implementation should call > CookieStoreIOS(NSHTTPCookieStorage*) constructor to be compatible with existing > code. > 2.) Add TODO(crbug.com/683964): Remove this method. > > Then I will remove CreateCookieStore call from that close-source repo and will > clean up this method. > > Thanks! Oh, that what you meant. I though If it could compile with the try bots, then it would be safe to remove the function. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios.h (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.h:54: // Create an instance of CookieStoreIOS that is generated from the cookies On 2017/01/23 17:37:47, Eugene But wrote: > nit: s/Create/Creates > > I realize that this is just moving the code, but still would be great to fix. > From Style Guide: > These comments should be descriptive ("Opens the file") rather than imperative > ("Open the file"); > https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.h:180: SynchronizationState synchronization_state_; On 2017/01/23 17:37:47, Eugene But wrote: > I think we can remove this member as well (see comments in the implementation). > If you want to do this in a separate CL, then please add TODOs to the code. Done. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.mm:605: // Copy the cookies from the global cookie store to |cookie_monster_|. On 2017/01/23 17:37:47, Eugene But wrote: > Should this code go to subclass? No, this code is for NSHTTPCookieStorage backed up CookieStoreIOS. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.mm:624: DCHECK_EQ(SYNCHRONIZED, synchronization_state_); On 2017/01/23 17:37:48, Eugene But wrote: > ditto Removed this line. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios.mm:651: if (synchronization_state_ != SYNCHRONIZED || On 2017/01/23 17:37:48, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios_persistent.h (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_persistent.h:11: #include "ios/net/cookies/cookie_store_ios.h" On 2017/01/23 17:37:48, Eugene But wrote: > s/include/import > > This is because cookie_store_ios.h is an Objective-C header. > https://google.github.io/styleguide/objcguide.xml#_import_and__include Done. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios_persistent.mm (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_persistent.mm:5: #include "ios/net/cookies/cookie_store_ios_persistent.h" On 2017/01/23 17:37:48, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... File ios/net/cookies/cookie_store_ios_unittest.mm (right): https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_unittest.mm:17: #include "ios/net/cookies/cookie_store_ios_persistent.h" On 2017/01/23 17:37:48, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2649083002/diff/1/ios/net/cookies/cookie_stor... ios/net/cookies/cookie_store_ios_unittest.mm:234: store_(new net::CookieStoreIOSPersistent(backend_.get())) { On 2017/01/23 17:37:48, Eugene But wrote: > Could you please use MakeUnique here and in other 2 places where the code was > changed. Done.
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nit in CL description: s/Devide/Divide/ Thanks again! https://codereview.chromium.org/2649083002/diff/60001/ios/chrome/browser/net/... File ios/chrome/browser/net/cookie_util.mm (right): https://codereview.chromium.org/2649083002/diff/60001/ios/chrome/browser/net/... ios/chrome/browser/net/cookie_util.mm:16: #include "ios/net/cookies/cookie_store_ios.h" nit: do we still need this include? https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... File ios/net/cookies/cookie_store_ios.h (right): https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.h:131: enum SynchronizationState { Do we still need this enum? https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.h:156: friend struct CookieStoreIOSTestTraits; Do we still need this friend? Looks like you removed access to private variables from CookieStoreIOSTestTraits struct. https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.mm:615: if (!cookie_monster_->IsEphemeral()) Conceptually it seems strange to have no-op behavior for Ephemeral cookie monster. Also I think this changes the logic for case when CookieStoreIOSPersistent object is created with null persistent_store. Maybe CookieStoreIOSPersistent::WriteToCookieMonster should be explicitly no-op instead by implementing that method with the empty body? https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.mm:663: if (!cookie_monster_->IsEphemeral() || Same question here? Should we make CookieStoreIOSPersistent::WriteToCookieMonster empty instead? https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... File ios/net/cookies/cookie_store_ios_persistent.h (right): https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios_persistent.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2649083002/diff/60001/ios/web/shell/shell_url... File ios/web/shell/shell_url_request_context_getter.mm (right): https://codereview.chromium.org/2649083002/diff/60001/ios/web/shell/shell_url... ios/web/shell/shell_url_request_context_getter.mm:16: #import "ios/net/cookies/cookie_store_ios.h" nit: do we still need this include?
The CQ bit was checked by maksim.sisov@intel.com 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com 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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by maksim.sisov@intel.com 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 ========== Devide CookieStoreIOS into two different classes with different backends CookieStoreIOS is used as a synchronized object with NSHTTPCookieStorge, and CookieStoreIOSPersistent is a class that uses CookieMonster as it's backend. BUG=679736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Divide CookieStoreIOS into two different classes with different backends CookieStoreIOS is used as a synchronized object with NSHTTPCookieStorge, and CookieStoreIOSPersistent is a class that uses CookieMonster as it's backend. BUG=679736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
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 maksim.sisov@intel.com 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.
https://codereview.chromium.org/2649083002/diff/60001/ios/chrome/browser/net/... File ios/chrome/browser/net/cookie_util.mm (right): https://codereview.chromium.org/2649083002/diff/60001/ios/chrome/browser/net/... ios/chrome/browser/net/cookie_util.mm:16: #include "ios/net/cookies/cookie_store_ios.h" On 2017/01/24 22:31:30, Eugene But wrote: > nit: do we still need this include? Done. https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... File ios/net/cookies/cookie_store_ios.h (right): https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.h:131: enum SynchronizationState { On 2017/01/24 22:31:30, Eugene But wrote: > Do we still need this enum? Done. https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.h:156: friend struct CookieStoreIOSTestTraits; On 2017/01/24 22:31:30, Eugene But wrote: > Do we still need this friend? Looks like you removed access to private variables > from CookieStoreIOSTestTraits struct. Done. https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.mm:615: if (!cookie_monster_->IsEphemeral()) On 2017/01/24 22:31:30, Eugene But wrote: > Conceptually it seems strange to have no-op behavior for Ephemeral cookie > monster. Also I think this changes the logic for case when > CookieStoreIOSPersistent object is created with null persistent_store. Maybe > CookieStoreIOSPersistent::WriteToCookieMonster should be explicitly no-op > instead by implementing that method with the empty body? Done. https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios.mm:663: if (!cookie_monster_->IsEphemeral() || On 2017/01/24 22:31:30, Eugene But wrote: > Same question here? Should we make > CookieStoreIOSPersistent::WriteToCookieMonster empty instead? Done. https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... File ios/net/cookies/cookie_store_ios_persistent.h (right): https://codereview.chromium.org/2649083002/diff/60001/ios/net/cookies/cookie_... ios/net/cookies/cookie_store_ios_persistent.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2017/01/24 22:31:30, Eugene But wrote: > nit: 2017 Are you sure? I was told last year that it was not allowed to changed existing files. The year corresponds to the creation date. https://codereview.chromium.org/2649083002/diff/60001/ios/web/shell/shell_url... File ios/web/shell/shell_url_request_context_getter.mm (right): https://codereview.chromium.org/2649083002/diff/60001/ios/web/shell/shell_url... ios/web/shell/shell_url_request_context_getter.mm:16: #import "ios/net/cookies/cookie_store_ios.h" On 2017/01/24 22:31:30, Eugene But wrote: > nit: do we still need this include? Done.
Think I'm just going to completely defer to Eugene on this one. I'll sign off on cronet once you get his signoff.
lgtm and thank you fox doing this. I run internal tests with your changes and they all passed. https://codereview.chromium.org/2649083002/diff/140001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios_persistent.h (right): https://codereview.chromium.org/2649083002/diff/140001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios_persistent.h:26: class CookieStoreIOSPersistent : public CookieStoreIOS { I suspect that it is possible to avoid this inheritance, however this would be too much for this CL. Could you please add "// TODO(crbug.com/686147): evaluate if inheritance is needed here.
LGTM
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2649083002/#ps180001 (title: "rebased and added todo")
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
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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by maksim.sisov@intel.com 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.
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2649083002/#ps200001 (title: "fix compilation")
https://codereview.chromium.org/2649083002/diff/140001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios_persistent.h (right): https://codereview.chromium.org/2649083002/diff/140001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios_persistent.h:26: class CookieStoreIOSPersistent : public CookieStoreIOS { On 2017/01/27 19:30:06, Eugene But wrote: > I suspect that it is possible to avoid this inheritance, however this would be > too much for this CL. > Could you please add "// TODO(crbug.com/686147): evaluate if inheritance is > needed here. Done.
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": 200001, "attempt_start_ts": 1485772622534660,
"parent_rev": "754c3ba80aa6fccd46a02bcdd6689a7f524abc9a", "commit_rev":
"fdd9f091afe9235efdfc1c1fce584d560ab9523f"}
Message was sent while issue was closed.
Description was changed from ========== Divide CookieStoreIOS into two different classes with different backends CookieStoreIOS is used as a synchronized object with NSHTTPCookieStorge, and CookieStoreIOSPersistent is a class that uses CookieMonster as it's backend. BUG=679736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Divide CookieStoreIOS into two different classes with different backends CookieStoreIOS is used as a synchronized object with NSHTTPCookieStorge, and CookieStoreIOSPersistent is a class that uses CookieMonster as it's backend. BUG=679736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2649083002 Cr-Commit-Position: refs/heads/master@{#446966} Committed: https://chromium.googlesource.com/chromium/src/+/fdd9f091afe9235efdfc1c1fce58... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/fdd9f091afe9235efdfc1c1fce58...
Message was sent while issue was closed.
mef@chromium.org changed reviewers: + mef@chromium.org
Message was sent while issue was closed.
FYI, this broke Cronet. https://codereview.chromium.org/2649083002/diff/200001/components/cronet/ios/... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2649083002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_environment.mm:315: base::MakeUnique<CookieStoreIOS>( This should've been net::CookieStoreIOS. https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios.mm:328: DCHECK(SystemCookiesAllowed()); This check fails on Cronet tester: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2Fios-simul... but passes locally. Any ideas why and what is a proper way to fix this?
Message was sent while issue was closed.
https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios.mm:328: DCHECK(SystemCookiesAllowed()); On 2017/02/09 18:37:16, mef wrote: > This check fails on Cronet tester: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2Fios-simul... > but passes locally. > > Any ideas why and what is a proper way to fix this? Sorry, I missed this during review. CookieStoreIOS::CreateCookieStore used to call |[cookie_storage setCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways];|, but constructor does not call it anymore. I think the right fix would be to call |[[NSHTTPCookieStorage sharedHTTPCookieStorage] etCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways] somewhere here: https://cs.chromium.org/chromium/src/ios/crnet/crnet_environment.mm?q=crnet_e... WDYT?
Message was sent while issue was closed.
On 2017/02/09 18:37:16, mef wrote: > FYI, this broke Cronet. > > https://codereview.chromium.org/2649083002/diff/200001/components/cronet/ios/... > File components/cronet/ios/cronet_environment.mm (right): > > https://codereview.chromium.org/2649083002/diff/200001/components/cronet/ios/... > components/cronet/ios/cronet_environment.mm:315: > base::MakeUnique<CookieStoreIOS>( > This should've been net::CookieStoreIOS. > > https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... > File ios/net/cookies/cookie_store_ios.mm (right): > > https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... > ios/net/cookies/cookie_store_ios.mm:328: DCHECK(SystemCookiesAllowed()); > This check fails on Cronet tester: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2Fios-simul... > but passes locally. > > Any ideas why and what is a proper way to fix this? So, are you going to fix it?
Message was sent while issue was closed.
Maksim, yes, I've got https://codereview.chromium.org/2684933009/ to fix this. https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... File ios/net/cookies/cookie_store_ios.mm (right): https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... ios/net/cookies/cookie_store_ios.mm:328: DCHECK(SystemCookiesAllowed()); On 2017/02/10 00:10:56, Eugene But wrote: > On 2017/02/09 18:37:16, mef wrote: > > This check fails on Cronet tester: > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2Fios-simul... > > but passes locally. > > > > Any ideas why and what is a proper way to fix this? > Sorry, I missed this during review. CookieStoreIOS::CreateCookieStore used to > call |[cookie_storage setCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways];|, > but constructor does not call it anymore. > > I think the right fix would be to call > |[[NSHTTPCookieStorage sharedHTTPCookieStorage] > etCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways] > > somewhere here: > https://cs.chromium.org/chromium/src/ios/crnet/crnet_environment.mm?q=crnet_e... > > WDYT? > sgtm. Looking at CookieStoreIOS::CreateCookieStore above I see that it also used to do cookie_store->synchronization_state_ = SYNCHRONIZED; cookie_store->FlushStore(base::Closure()) Do we need to do it explicitly in Cronet as well?
Message was sent while issue was closed.
On 2017/02/10 16:33:57, mef wrote: > Maksim, yes, I've got https://codereview.chromium.org/2684933009/ to fix this. > > https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... > File ios/net/cookies/cookie_store_ios.mm (right): > > https://codereview.chromium.org/2649083002/diff/200001/ios/net/cookies/cookie... > ios/net/cookies/cookie_store_ios.mm:328: DCHECK(SystemCookiesAllowed()); > On 2017/02/10 00:10:56, Eugene But wrote: > > On 2017/02/09 18:37:16, mef wrote: > > > This check fails on Cronet tester: > > > > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2Fios-simul... > > > but passes locally. > > > > > > Any ideas why and what is a proper way to fix this? > > Sorry, I missed this during review. CookieStoreIOS::CreateCookieStore used to > > call |[cookie_storage setCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways];|, > > but constructor does not call it anymore. > > > > I think the right fix would be to call > > |[[NSHTTPCookieStorage sharedHTTPCookieStorage] > > etCookieAcceptPolicy:NSHTTPCookieAcceptPolicyAlways] > > > > somewhere here: > > > https://cs.chromium.org/chromium/src/ios/crnet/crnet_environment.mm?q=crnet_e... > > > > WDYT? > > > > sgtm. Looking at CookieStoreIOS::CreateCookieStore above I see that it also used > to do > > cookie_store->synchronization_state_ = SYNCHRONIZED; > cookie_store->FlushStore(base::Closure()) > > Do we need to do it explicitly in Cronet as well? Doing |cookie_store->FlushStore(base::Closure())| will preserve the old behavior, but unfortunately I'm not familiar with CookieStore API and can't definitely say if we need this call :(
Message was sent while issue was closed.
sdefresne@chromium.org changed reviewers: + droger@chromium.org, sdefresne@chromium.org
Message was sent while issue was closed.
+droger who may still remember how cookie store works on iOS.
Message was sent while issue was closed.
On 2017/02/13 13:46:06, sdefresne wrote: > +droger who may still remember how cookie store works on iOS. The code seems to have changed quite a bit since I worked on this, but from my understanding CookieStoreIOS is only used by CroNet, and CroNet basically only needs the basic CookieStore functionality. We could probably get rid of cookie_monster_, FlushStore(), etc,.. as I expect they are never actually called, except when the system cookies are disabled (they allow to still access a copy of the cookies even though the cookies are no longer accessible in the system store), and I don't think this functionality is needed by CroNet.
Message was sent while issue was closed.
On 2017/02/14 10:09:12, droger wrote: > On 2017/02/13 13:46:06, sdefresne wrote: > > +droger who may still remember how cookie store works on iOS. > > The code seems to have changed quite a bit since I worked on this, but from my > understanding CookieStoreIOS is only used by CroNet, and CroNet basically only > needs the basic CookieStore functionality. > > We could probably get rid of cookie_monster_, FlushStore(), etc,.. as I expect > they are never actually called, except when the system cookies are disabled > (they allow to still access a copy of the cookies even though the cookies are no > longer accessible in the system store), and I don't think this functionality is > needed by CroNet. CookieStoreIOS is also used for SignIn, but that code maintains the original behavior and flushes cookies.
Message was sent while issue was closed.
On 2017/02/14 16:09:51, Eugene But wrote: > On 2017/02/14 10:09:12, droger wrote: > > On 2017/02/13 13:46:06, sdefresne wrote: > > > +droger who may still remember how cookie store works on iOS. > > > > The code seems to have changed quite a bit since I worked on this, but from my > > understanding CookieStoreIOS is only used by CroNet, and CroNet basically only > > needs the basic CookieStore functionality. > > > > We could probably get rid of cookie_monster_, FlushStore(), etc,.. as I expect > > they are never actually called, except when the system cookies are disabled > > (they allow to still access a copy of the cookies even though the cookies are > no > > longer accessible in the system store), and I don't think this functionality > is > > needed by CroNet. > CookieStoreIOS is also used for SignIn, but that code maintains the original > behavior and flushes cookies. So, if an app uses Cronet for SignIn, should Cronet also flush cookies?
Message was sent while issue was closed.
On 2017/02/14 16:15:31, mef wrote: > On 2017/02/14 16:09:51, Eugene But wrote: > > On 2017/02/14 10:09:12, droger wrote: > > > On 2017/02/13 13:46:06, sdefresne wrote: > > > > +droger who may still remember how cookie store works on iOS. > > > > > > The code seems to have changed quite a bit since I worked on this, but from > my > > > understanding CookieStoreIOS is only used by CroNet, and CroNet basically > only > > > needs the basic CookieStore functionality. > > > > > > We could probably get rid of cookie_monster_, FlushStore(), etc,.. as I > expect > > > they are never actually called, except when the system cookies are disabled > > > (they allow to still access a copy of the cookies even though the cookies > are > > no > > > longer accessible in the system store), and I don't think this functionality > > is > > > needed by CroNet. > > CookieStoreIOS is also used for SignIn, but that code maintains the original > > behavior and flushes cookies. > > So, if an app uses Cronet for SignIn, should Cronet also flush cookies? Sorry, I don't know :(
Message was sent while issue was closed.
On 2017/02/14 16:22:30, Eugene But wrote: > On 2017/02/14 16:15:31, mef wrote: > > On 2017/02/14 16:09:51, Eugene But wrote: > > > On 2017/02/14 10:09:12, droger wrote: > > > > On 2017/02/13 13:46:06, sdefresne wrote: > > > > > +droger who may still remember how cookie store works on iOS. > > > > > > > > The code seems to have changed quite a bit since I worked on this, but > from > > my > > > > understanding CookieStoreIOS is only used by CroNet, and CroNet basically > > only > > > > needs the basic CookieStore functionality. > > > > > > > > We could probably get rid of cookie_monster_, FlushStore(), etc,.. as I > > expect > > > > they are never actually called, except when the system cookies are > disabled > > > > (they allow to still access a copy of the cookies even though the cookies > > are > > > no > > > > longer accessible in the system store), and I don't think this > functionality > > > is > > > > needed by CroNet. > > > CookieStoreIOS is also used for SignIn, but that code maintains the original > > > behavior and flushes cookies. > > > > So, if an app uses Cronet for SignIn, should Cronet also flush cookies? > Sorry, I don't know :( SignIn is a ios/chrome level feature, so AFAICT, it cannot be used by Cronet. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
