New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 679736 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Feature

Blocking:
issue 579697
issue 690969



Sign in to add a comment

Split CookieStoreIOS into 2 parts

Project Member Reported by eugene...@chromium.org, Jan 10 2017

Issue description

CookieStoreIOS can be created either with CookieMonster (NOT_SYNCHRONIZED) or with NSHTTPCookieStore (SYNCHRONIZED). And depending on SynchronizationState (which persist for lifetime) this class have different logic. Making 2 separate classes would simplify the code which is peppered with switch statements.
 
Status: Started (was: Available)
I'll take care about this issue.
Cc: bzanotti@chromium.org maksim.s...@intel.com
Thanks Maxim! Please note that Chrome for iOS has non-open source code that calls net::CookieStoreIOS::CreateCookieStore method. I think extracting NOT_SYNCHRONIZED code into a separate class and keeping CookieStoreIOS as SYNCHRONIZED class could be a good direction.
Owner: maksim.s...@intel.com
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdd9f091afe9235efdfc1c1fce584d560ab9523f

commit fdd9f091afe9235efdfc1c1fce584d560ab9523f
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Mon Jan 30 10:41:42 2017

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}

[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/components/cronet/ios/cronet_environment.mm
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/chrome/browser/net/cookie_util.mm
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/crnet/crnet_environment.mm
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/net/BUILD.gn
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/net/cookies/cookie_store_ios.mm
[add] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/net/cookies/cookie_store_ios_persistent.h
[add] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/net/cookies/cookie_store_ios_persistent.mm
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/net/cookies/cookie_store_ios_unittest.mm
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/web/shell/shell_url_request_context_getter.mm
[modify] https://crrev.com/fdd9f091afe9235efdfc1c1fce584d560ab9523f/ios/web_view/internal/criwv_url_request_context_getter.mm

Comment 5 by mmenke@chromium.org, Jan 31 2017

Status: Fixed (was: Started)
Owner: ----
Status: Available (was: Fixed)
The tests still live in the same file, so marking as Available for now.
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e209771cd1fcaeef978eabe82eaa03297c33b50e

commit e209771cd1fcaeef978eabe82eaa03297c33b50e
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Thu Feb 02 17:10:13 2017

Divide Cookie Store IOS tests into different files

This is a follow-up CL, which splits cookie_store_ios_unittest into two
different files with different backends. In order to avoid code
replication, helper methods and classes are put to
cookie_store_ios_unittest_helper.*

Main CL - https://codereview.chromium.org/2649083002/

BUG= 679736 

Review-Url: https://codereview.chromium.org/2667753007
Cr-Commit-Position: refs/heads/master@{#447783}

[modify] https://crrev.com/e209771cd1fcaeef978eabe82eaa03297c33b50e/ios/net/BUILD.gn
[add] https://crrev.com/e209771cd1fcaeef978eabe82eaa03297c33b50e/ios/net/cookies/cookie_store_ios_persistent_unittest.mm
[add] https://crrev.com/e209771cd1fcaeef978eabe82eaa03297c33b50e/ios/net/cookies/cookie_store_ios_test_util.h
[add] https://crrev.com/e209771cd1fcaeef978eabe82eaa03297c33b50e/ios/net/cookies/cookie_store_ios_test_util.mm
[modify] https://crrev.com/e209771cd1fcaeef978eabe82eaa03297c33b50e/ios/net/cookies/cookie_store_ios_unittest.mm

Status: Fixed (was: Available)

Comment 9 by mef@chromium.org, Feb 9 2017

Components: Internals>Network>Library
Owner: mef@chromium.org
Status: Assigned (was: Fixed)
The change has broken Cronet compilation and tests, for example https://build.chromium.org/p/chromium.fyi/builders/ios-simulator-cronet/builds/884

We've missed the breakage when it happened because our fyi bot does not send notification emails. 

I'll work on the fix.

Comment 10 by mef@chromium.org, Feb 10 2017

Blocking: 690969
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/674565161b44e1505e220f1864803647698d9a8e

commit 674565161b44e1505e220f1864803647698d9a8e
Author: mef <mef@chromium.org>
Date: Fri Feb 10 18:44:24 2017

[Cronet] Fix compilation and test errors for CookieStoreIOS.

BUG= 679736 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2684933009
Cr-Commit-Position: refs/heads/master@{#449669}

[modify] https://crrev.com/674565161b44e1505e220f1864803647698d9a8e/components/cronet/ios/cronet_environment.mm
[modify] https://crrev.com/674565161b44e1505e220f1864803647698d9a8e/ios/crnet/crnet_environment.mm
[modify] https://crrev.com/674565161b44e1505e220f1864803647698d9a8e/ios/net/cookies/cookie_store_ios.mm

Comment 12 by mef@chromium.org, Feb 14 2017

Status: Fixed (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/785bf143e2dd152e2a7deaa86f7ca39a46eb624a

commit 785bf143e2dd152e2a7deaa86f7ca39a46eb624a
Author: mef <mef@chromium.org>
Date: Thu Mar 02 22:17:59 2017

CookieStoreIOS should consider SystemCookiesAllowed unless system cookies policy is set to never.

BUG= 679736 

Review-Url: https://codereview.chromium.org/2732513002
Cr-Commit-Position: refs/heads/master@{#454402}

[modify] https://crrev.com/785bf143e2dd152e2a7deaa86f7ca39a46eb624a/ios/net/cookies/cookie_store_ios.mm

Cc: -maksim.s...@intel.com maksim.sisov@chromium.org

Sign in to add a comment