New issue
Advanced search Search tips

Issue 676144 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 579697



Sign in to add a comment

Cleanup CookieStoreIOS from unneeded code

Project Member Reported by eugene...@chromium.org, Dec 20 2016

Issue description

CookieStoreIOS was implemented on top of NSHTTPCookieStorage for UIWebView.
After switching to WKWebView most of it's API does not work anymore and should be removed.
 
Labels: -Type-Bug Type-Feature
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 21 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/bd8ba3480f76aedec7296e255055e90d6b6b4c92

commit bd8ba3480f76aedec7296e255055e90d6b6b4c92
Author: eugenebut <eugenebut@google.com>
Date: Wed Dec 21 09:20:22 2016

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 21 2016

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

commit f26cdbcbddb5e78a4791e8637653047caafd3156
Author: eugenebut <eugenebut@chromium.org>
Date: Wed Dec 21 15:26:08 2016

[ios] Removed CookieStoreIOS::SetCookiePolicy.

After switching to WKWebView this API is not needed anymore. New cookie
policy takes effect only after app restart which is not useful.

BUG= 676144 

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

[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/chrome/app/app_delegate.mm
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/chrome/app/steps/launch_to_foreground.h
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/chrome/app/steps/launch_to_foreground.mm
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/net/cookies/cookie_store_ios.mm
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/net/cookies/cookie_store_ios_unittest.mm
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/web/public/web_capabilities.cc
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/web/public/web_capabilities.h
[modify] https://crrev.com/f26cdbcbddb5e78a4791e8637653047caafd3156/ios/web/shell/view_controller.mm

Blocking: 579697
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 23 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/cbd04a7bfa3a8cff6033949fb3299d315168368f

commit cbd04a7bfa3a8cff6033949fb3299d315168368f
Author: eugenebut <eugenebut@google.com>
Date: Fri Dec 23 10:23:45 2016

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 23 2016

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

commit 61488b67c9a2cf7fbe27b49e312531bb80af8f13
Author: eugenebut <eugenebut@chromium.org>
Date: Fri Dec 23 16:32:04 2016

[ios] Removed CookieStoreIOS::UnSynchronize.

This method was called when the last incognito tab was closed to prevent
Synchronizing incognito cookies with shared cookie storage. WKWebView's
incognito is ephemeral and web view cookies are not stored anywhere and
not accessible for Chrome app.

Also removed SwitchSynchronizedStore which was used only in tests and
made SetSynchronizedWithSystemStore public to keep it using in tests for now
(instead of UnSynchronize and SwitchSynchronizedStore). Once most of
CookieStoreIOS methods are removed it should be easier to remove
SetSynchronizedWithSystemStore as well.

BUG= 676144 

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

[modify] https://crrev.com/61488b67c9a2cf7fbe27b49e312531bb80af8f13/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/61488b67c9a2cf7fbe27b49e312531bb80af8f13/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/61488b67c9a2cf7fbe27b49e312531bb80af8f13/ios/net/cookies/cookie_store_ios.mm
[modify] https://crrev.com/61488b67c9a2cf7fbe27b49e312531bb80af8f13/ios/net/cookies/cookie_store_ios_unittest.mm
[modify] https://crrev.com/61488b67c9a2cf7fbe27b49e312531bb80af8f13/ios/web/shell/shell_url_request_context_getter.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 28 2016

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

commit d649da83761a5469b5d8b8a294682128ccfcbdee
Author: eugenebut <eugenebut@chromium.org>
Date: Wed Dec 28 16:39:55 2016

[ios] Cleaned up comments and tests for CookieStoreIOS synchronization.

In UIWebView world CookieStoreIOS could dynamically change its state
and either be synchronized with NSHTTPCookieStorage or unsynchronized.

In WKWebView world CookieStoreIOS never changes its state. It is either
always synchronized (for sign in) or always unsynchronized (for regular
browsing).

Notable changes:
- updated comments to reflect that sync state is not dynamic
- removed RoundTripTest which tested non-existing dynamic synchronization flow
- removed other tests which tests dynamic synchronization

BUG= 676144 

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

[modify] https://crrev.com/d649da83761a5469b5d8b8a294682128ccfcbdee/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/d649da83761a5469b5d8b8a294682128ccfcbdee/ios/net/cookies/cookie_store_ios_unittest.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 30 2016

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

commit 2cbccb7fa8907c3fa53840187e11613a5f307270
Author: eugenebut <eugenebut@chromium.org>
Date: Fri Dec 30 16:27:52 2016

Made CookieStoreIOS(PersistentCookieStore, NSHTTPCookieStorage) private.

This constructor is used only by implementation and should not be public

BUG= 676144 

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

[modify] https://crrev.com/2cbccb7fa8907c3fa53840187e11613a5f307270/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/2cbccb7fa8907c3fa53840187e11613a5f307270/ios/net/cookies/cookie_store_ios.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 30 2016

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

commit 646ec2f38d767ac5ab60d2f443e3558061dbf2a0
Author: eugenebut <eugenebut@chromium.org>
Date: Fri Dec 30 16:40:15 2016

[ios] Do not use SetSynchronizedWithSystemStore in tests.

There are 2 ways how CookieStoreIOS() is created in production code:
 - Synchronized with NSHTTPCookieStorage and without persistent_store
   (via CookieStoreIOS::CreateCookieStore factory method).
 - Not synchronized with NSHTTPCookieStorage and with persistent_store
   (via CookieStoreIOS::CookieStoreIOS constructor).

Updated tests to test these 2 flows using 2 different test fixtures:
SynchronizedCookieStoreIOS and NotSynchronizedCookieStoreIOSWithBackend.

With this new approach there is no need to call
|SetSynchronizedWithSystemStore| method and it can be safely removed in
a separate CL as it is not used in production code.

Also removed obsolete tests which tested synchronized store with backend:
 - CookieStoreIOSWithBackend.Synchronizing
 - CookieStoreIOSWithBackend.FlushOnCookieChanged
 - CookieStoreIOSWithBackend.ManualFlush

BUG= 676144 

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

[modify] https://crrev.com/646ec2f38d767ac5ab60d2f443e3558061dbf2a0/ios/net/cookies/cookie_store_ios_unittest.mm

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 9 2017

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

commit bbd36820b3ac7b360ad6cbf351003f1c53b647a9
Author: eugenebut <eugenebut@chromium.org>
Date: Mon Jan 09 17:05:43 2017

[ios] Removed CookieStoreIOS::SetSynchronizedWithSystemStore.

This method is unused anymore.

Also removed AddCookiesToSystemStore/RunAllPendingTasks methods
and g_current_synchronized_store global variable.

SYNCHRONIZING state and tasks_pending_synchronization_ will be removed
in a separate CL.

BUG= 676144 

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

[modify] https://crrev.com/bbd36820b3ac7b360ad6cbf351003f1c53b647a9/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/bbd36820b3ac7b360ad6cbf351003f1c53b647a9/ios/net/cookies/cookie_store_ios.mm

Project Member

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

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

commit 03256bc6332c6e0e3edf1ae6b3f35e9c193ce6f5
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Jan 10 13:56:38 2017

[ios] Removed CookieStoreIOS::SynchronizationState::SYNCHRONIZING.

CookieStoreIOS can not be in SYNCHRONIZING state anymore.

BUG= 676144 

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

[modify] https://crrev.com/03256bc6332c6e0e3edf1ae6b3f35e9c193ce6f5/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/03256bc6332c6e0e3edf1ae6b3f35e9c193ce6f5/ios/net/cookies/cookie_store_ios.mm

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 10 2017

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

commit 8a37ce2583292c6ffcf8440c72db5747b94513a4
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Jan 10 14:13:13 2017

[ios] Removed CookieStoreIOS::set_flush_delay_for_testing.

This method is unused.

BUG= 676144 

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

[modify] https://crrev.com/8a37ce2583292c6ffcf8440c72db5747b94513a4/ios/net/cookies/cookie_store_ios.h
[modify] https://crrev.com/8a37ce2583292c6ffcf8440c72db5747b94513a4/ios/net/cookies/cookie_store_ios.mm

Status: Fixed (was: Assigned)
All unused code was removed. This class should be split into 2 classes, which is a separate bug now: 679736

Sign in to add a comment