Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly considered doing a subclass of CookieStore that
has an NSHTTPCookieStorage as the backend. But I ended up
re-implementing a lot of the logic from CookieStoreIOS
(such as the conversion between a canonical cookie and an
NSHTTPCookie)
Injecting this dependency will avoid unnecessary code duplication.

BUG=542866
Committed: https://crrev.com/1ae3c3a3b26dc42489c0209bbf8ab12738094857
Cr-Commit-Position: refs/heads/master@{#357603}

[shreyasv1, 2015-10-29 17:55:52]

shreyasv@chromium.org changed reviewers:
+ droger@chromium.org, stuartmorgan@chromium.org

[shreyasv1, 2015-10-29 17:55:53]

+Stuart in case he can turn it around faster.

[shreyasv1, 2015-10-29 17:56:07]

Description was changed from

==========
Injecting a NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly consider doing a subclass of CookieStore that
has a NSHTTPCookieStorage as the backend. But I ended up copy
pasting a lot of the logic from CookieStoreIOS. Injecting this
dependency will avoid unnecessary code duplication.

BUG=542866
==========

to

==========
Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly consider doing a subclass of CookieStore that
has a NSHTTPCookieStorage as the backend. But I ended up copy
pasting a lot of the logic from CookieStoreIOS. Injecting this
dependency will avoid unnecessary code duplication.

BUG=542866
==========

[shreyasv1, 2015-10-29 18:02:36]

Description was changed from

==========
Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly consider doing a subclass of CookieStore that
has a NSHTTPCookieStorage as the backend. But I ended up copy
pasting a lot of the logic from CookieStoreIOS. Injecting this
dependency will avoid unnecessary code duplication.

BUG=542866
==========

to

==========
Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly considered doing a subclass of CookieStore that
has a NSHTTPCookieStorage as the backend. But I ended up copy
pasting a lot of the logic from CookieStoreIOS. Injecting this
dependency will avoid unnecessary code duplication.

BUG=542866
==========

[shreyasv1, 2015-10-29 18:03:46]

Description was changed from

==========
Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly considered doing a subclass of CookieStore that
has a NSHTTPCookieStorage as the backend. But I ended up copy
pasting a lot of the logic from CookieStoreIOS. Injecting this
dependency will avoid unnecessary code duplication.

BUG=542866
==========

to

==========
Injecting an NSHTTPCookieStorage dependency into CookieStoreIOS

In Chrome we plan to use a subclass of NSHTTPCookieStorage as we
plan to implement an in-memory only cookie store (as opposed to the
default implementation in NSHTTPCookieStorage which passes it
to CFNetwork).

I briefly considered doing a subclass of CookieStore that
has an NSHTTPCookieStorage as the backend. But I ended up 
re-implementing a lot of the logic from CookieStoreIOS
(such as the conversion between a canonical cookie and an 
NSHTTPCookie)
Injecting this dependency will avoid unnecessary code duplication.

BUG=542866
==========

[droger, 2015-10-30 17:02:51]

The code LGTM (with the comment below).

I don't know how this is going to be used though, and so far I'm not completely
convinced that this is useful.

In particular, I don't understand how you will achieve consistency of the
cookies as seen by javascript.

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
File ios/net/cookies/cookie_store_ios.mm (right):

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
ios/net/cookies/cookie_store_ios.mm:303: NSHTTPCookieStorage* store =
[NSHTTPCookieStorage sharedHTTPCookieStorage];
Also, not for this CL, but you should make sure that whatever custom cookie
store you inject honors the cookie policy set by the user.

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
ios/net/cookies/cookie_store_ios.mm:303: NSHTTPCookieStorage* store =
[NSHTTPCookieStorage sharedHTTPCookieStorage];
Using sharedHTTPCookieStorage here is a bit scary.

I would suggest either
- passing the cookie storage as a parameter, or
- passing the shared NSHTTPCookieStorage to the NotifyCookiePolicyChanged()
function, and in  OnSystemCookiePolicyChanged(), do a early return if the cookie
storages don't match.

In the latter case, please also add a comment in the .h saying that the cookie
policy only affects the CookieStoreIOS instances that are backed by the
[NSHTTPCookieStorage sharedHTTPCookieStorage].

[shreyasv1, 2015-11-02 22:21:44]

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
File ios/net/cookies/cookie_store_ios.mm (right):

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
ios/net/cookies/cookie_store_ios.mm:303: NSHTTPCookieStorage* store =
[NSHTTPCookieStorage sharedHTTPCookieStorage];
On 2015/10/30 17:02:51, droger wrote:
> Also, not for this CL, but you should make sure that whatever custom cookie
> store you inject honors the cookie policy set by the user.

Acknowledged.

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
ios/net/cookies/cookie_store_ios.mm:303: NSHTTPCookieStorage* store =
[NSHTTPCookieStorage sharedHTTPCookieStorage];
On 2015/10/30 17:02:51, droger wrote:
> Using sharedHTTPCookieStorage here is a bit scary.
> 
> I would suggest either
> - passing the cookie storage as a parameter, or
> - passing the shared NSHTTPCookieStorage to the NotifyCookiePolicyChanged()
> function, and in  OnSystemCookiePolicyChanged(), do a early return if the
cookie
> storages don't match.
> 
> In the latter case, please also add a comment in the .h saying that the cookie
> policy only affects the CookieStoreIOS instances that are backed by the
> [NSHTTPCookieStorage sharedHTTPCookieStorage].

I don't really understand the need to pass a NSHTTPCookieStorage* param into
NotifyCookiePolicyChanged because this is an internal method.

What I think is a tad cleaner is that I have added a comment on the
CookieNotificationObserver saying that these
methods are called only for NSHTTPCookieStorge sharedHTTPCookieStorage. My
concern with adding a param on  OnSystemCookiePolicyChanged this leads to
incongruity. e.g. OnSystemCookiePolicyChanged takes an NSHTTPCookieStorage param
but OnSystemCookiesChanged does not.

I have added comments and early returns  in the relevant places.

[droger, 2015-11-03 09:49:00]

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
File ios/net/cookies/cookie_store_ios.mm (right):

https://codereview.chromium.org/1428673003/diff/1/ios/net/cookies/cookie_stor...
ios/net/cookies/cookie_store_ios.mm:303: NSHTTPCookieStorage* store =
[NSHTTPCookieStorage sharedHTTPCookieStorage];
On 2015/11/02 22:21:44, shreyasv1 wrote:
> On 2015/10/30 17:02:51, droger wrote:
> > Using sharedHTTPCookieStorage here is a bit scary.
> > 
> > I would suggest either
> > - passing the cookie storage as a parameter, or
> > - passing the shared NSHTTPCookieStorage to the NotifyCookiePolicyChanged()
> > function, and in  OnSystemCookiePolicyChanged(), do a early return if the
> cookie
> > storages don't match.
> > 
> > In the latter case, please also add a comment in the .h saying that the
cookie
> > policy only affects the CookieStoreIOS instances that are backed by the
> > [NSHTTPCookieStorage sharedHTTPCookieStorage].
> 
> I don't really understand the need to pass a NSHTTPCookieStorage* param into
> NotifyCookiePolicyChanged because this is an internal method.
> 
> What I think is a tad cleaner is that I have added a comment on the
> CookieNotificationObserver saying that these
> methods are called only for NSHTTPCookieStorge sharedHTTPCookieStorage. My
> concern with adding a param on  OnSystemCookiePolicyChanged this leads to
> incongruity. e.g. OnSystemCookiePolicyChanged takes an NSHTTPCookieStorage
param
> but OnSystemCookiesChanged does not.
> 
> I have added comments and early returns  in the relevant places.

Ok, what you did works too. The important bit was to do early returns if the
cookie stores don't match.

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
File ios/net/cookies/cookie_store_ios.mm (right):

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
ios/net/cookies/cookie_store_ios.mm:643: if (synchronization_state_ !=
SYNCHRONIZED ||
== NOT_SYNCHRONIZED
is not the same as
!= SYNCHRONIZED

Please don't change the behavior here, unless there is a good reason to change
it.

[droger, 2015-11-03 10:24:08]

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
File ios/net/cookies/cookie_store_ios.h (right):

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
ios/net/cookies/cookie_store_ios.h:89: // as its default backend.
Nit: While you are touching this, can you add something like:
"and is initially synchronized with it"

[shreyasv1, 2015-11-03 18:15:46]

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
File ios/net/cookies/cookie_store_ios.h (right):

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
ios/net/cookies/cookie_store_ios.h:89: // as its default backend.
On 2015/11/03 10:24:07, droger wrote:
> Nit: While you are touching this, can you add something like:
> "and is initially synchronized with it"

Done.

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
File ios/net/cookies/cookie_store_ios.mm (right):

https://codereview.chromium.org/1428673003/diff/20001/ios/net/cookies/cookie_...
ios/net/cookies/cookie_store_ios.mm:643: if (synchronization_state_ !=
SYNCHRONIZED ||
On 2015/11/03 09:49:00, droger wrote:
> == NOT_SYNCHRONIZED
> is not the same as
> != SYNCHRONIZED
> 
> Please don't change the behavior here, unless there is a good reason to change
> it.

Done.

[shreyasv1, 2015-11-03 19:30:14]

The CQ bit was checked by shreyasv@chromium.org

[shreyasv1, 2015-11-03 19:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org
Link to the patchset: https://codereview.chromium.org/1428673003/#ps40001
(title: "y")

[commit-bot: I haz the power, 2015-11-03 19:36:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428673003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428673003/40001

[commit-bot: I haz the power, 2015-11-03 20:13:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-03 20:14:39]

Patchset 3 (id:??) landed as
https://crrev.com/1ae3c3a3b26dc42489c0209bbf8ab12738094857
Cr-Commit-Position: refs/heads/master@{#357603}