Pass a RemovalCause to CookieChangedCallback

Pass a RemovalCause to CookieChangedCallback

This is to help with debugging the callback used in GaiaCookieManagerService
for the Channel ID mismatch issue.

BUG=548423
Committed: https://crrev.com/352933e4909adc3b36cd609507918a384e38c99f
Cr-Commit-Position: refs/heads/master@{#422156}

[nharper, 2016-09-17 00:04:19]

The CQ bit was checked by nharper@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 00:04:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 00:19:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 00:19:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nharper, 2016-09-19 19:01:00]

nharper@chromium.org changed reviewers:
+ ellyjones@chromium.org, mkwst@chromium.org, rogerta@chromium.org,
sgurun@chromium.org

[sgurun-gerrit only, 2016-09-19 20:16:28]

On 2016/09/19 19:01:01, nharper wrote:

aw lgtm

[Roger Tawa OOO till Jul 10th, 2016-09-19 20:23:02]

Thanks Nick.  A couple of questions below for my own info.

https://codereview.chromium.org/2349823003/diff/20001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/20001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:40: NOT_REMOVED,
If I have a value set for a cookie, and I browse to a website which returns a
Set-Cookie header, will I get only a NOT_REMOVED notif, or an EXPLICIT or
OVERWRITE notif followed by a NOT_REMOVED?

https://codereview.chromium.org/2349823003/diff/20001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:47: };
Wondering why you didn't define as ChangeCause instead, or maybe merge with
CookieMonsterDelegate::ChangeCause.  You could then remove the |bool removed|
from the callback signature.  If you changed all calls to the callback with:

removed=false
cause=UNKNOWN

to be just cause=NOT_REMOVED that would accomplish the same without the bool
arg.  Yes/no?

[nharper, 2016-09-19 21:34:31]

https://codereview.chromium.org/2349823003/diff/20001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/20001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:40: NOT_REMOVED,
On 2016/09/19 20:23:02, Roger Tawa wrote:
> If I have a value set for a cookie, and I browse to a website which returns a
> Set-Cookie header, will I get only a NOT_REMOVED notif, or an EXPLICIT or
> OVERWRITE notif followed by a NOT_REMOVED?

It looks like you'll first get a (removed=true) OVERWRITE followed by a
(removed=false) EXPLICIT (or NOT_REMOVED as currently implemented) - afaict that
behavior is implemented by removing the existing value and then inserting the
new value.

https://codereview.chromium.org/2349823003/diff/20001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:47: };
On 2016/09/19 20:23:02, Roger Tawa wrote:
> Wondering why you didn't define as ChangeCause instead, or maybe merge with
> CookieMonsterDelegate::ChangeCause.  You could then remove the |bool removed|
> from the callback signature.  If you changed all calls to the callback with:
> 
> removed=false
> cause=UNKNOWN
> 
> to be just cause=NOT_REMOVED that would accomplish the same without the bool
> arg.  Yes/no?

I'm thinking combining the bool with the enum would make sense. I'm less sure
about combining this enum with CookieMonsterDelegate::ChangeCause - let me ask
on net-dev what people think.

[nharper, 2016-09-22 02:13:15]

The CQ bit was checked by nharper@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 02:13:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 03:13:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 03:13:30]

Dry run: This issue passed the CQ dry run.

[nharper, 2016-09-22 19:18:06]

I've merged CookieMonsterDelegate::ChangeCause with the new
CookieStore::RemovalCause to become CookieStore::ChangeCause, and this enum
replaces the bool (instead of being in addition to it).

[Roger Tawa OOO till Jul 10th, 2016-09-22 19:24:56]

lgtm

Awesome Nick!  I like the addition of ChangeCauseIsDeletion() too.

[nharper, 2016-09-23 21:02:34]

nharper@chromium.org changed reviewers:
+ mmenke@chromium.org, rdevlin.cronin@chromium.org

[nharper, 2016-09-23 21:02:34]

Adding more reviewers for OWNERS coverage

[mmenke, 2016-09-23 21:43:07]

On 2016/09/23 21:02:34, nharper wrote:
> Adding more reviewers for OWNERS coverage

You should generally tell people explicitly what you want them to review.  I
assume you want me to look at net/cookies, and chrome/browser/net (I could well
have missed the last one, for instance)

[nharper, 2016-09-23 21:47:56]

On 2016/09/23 21:43:07, mmenke wrote:
> On 2016/09/23 21:02:34, nharper wrote:
> > Adding more reviewers for OWNERS coverage
> 
> You should generally tell people explicitly what you want them to review.  I
> assume you want me to look at net/cookies, and chrome/browser/net (I could
well
> have missed the last one, for instance)

Thanks for that tip.

Here's who I'm looking for OWNERS reviews from (and for what):
rdevlin.cronin: chrome/browser/extensions
ellyjones: ios/net
mmenke: chrome/browser/net and net/cookies
mkwst: net/cookies

Already reviewed:
sgurun: android_webview
rogerta: components/signin

[Mike West, 2016-09-26 07:55:37]

The //net/cookies change generally looks fine.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:65: ChangeCause cause)>
Can you add some tests for this change? It looks like the changes to the
existing unit tests fold all deletion types together to replicate the old
interface. It would be great to verify that you're getting the correct values
out based upon what's passed in.

[Devlin, 2016-09-26 16:46:10]

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:147: case
net::CookieStore::CHANGE_COOKIE_UNKNOWN_DELETION:
Is it accurate to lump these two in here?  e.g. unknown deletion sounds like it
wasn't necessarily explicit.  And can insertion only happen from explicit
insertions?  If so, what's the difference between insertion and explicit?

[mmenke, 2016-09-26 18:29:26]

Not a fan of this, but that's mostly because it's the old code I don't like, not
so much the new code.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:275: ChangeCausePair ChangeCauseMapping[] = {
While you're here, mind fixing this?  Should be "const kChangeCauseMapping"

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1689: }
nit:  Remove braces

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:39: CHANGE_COOKIE_INSERTED,
Can we make this an enum class (And maybe get rid of the prefix)?

i.e. CookieStore::ChangeCause::INSERTED

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:222: // implementors need to be improved to not
behave this way.
Maybe add a warning that the callback must not synchronously modify another
cookie, while we're here?

[nharper, 2016-09-26 21:09:10]

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:147: case
net::CookieStore::CHANGE_COOKIE_UNKNOWN_DELETION:
On 2016/09/26 16:46:09, Devlin (catching up) wrote:
> Is it accurate to lump these two in here?  e.g. unknown deletion sounds like
it
> wasn't necessarily explicit.  And can insertion only happen from explicit
> insertions?  If so, what's the difference between insertion and explicit?

I agree that UNKNOWN_DELETION doesn't belong here. I've moved it to be with the
default case (the NOTREACHED()). UNKNOWN_DELETION is used in two places. The
first is in GaiaCookieManagerService when a caller forces the
GaiaCookieManagerService to run its cookie changed processing. The
UNKNOWN_DELETION value never leaves the GaiaCookieManagerService in this case.
The other use of UNKNOWN_DELETION is in CookieStoreIOS, and is only used to pass
it in to a CookieChangedCallback, which will never end up here.

The difference between INSERTED and EXPLICIT is that EXPLICIT can be both an add
or a remove, while INSERTED is only an add. According to the documentation at
https://developer.chrome.com/extensions/cookies#type-OnChangedCause, the only
reason here that's valid if a cookie wasn't deleted is "explicit". 

The cause here comes from a call to a CookieMonsterDelegate::OnCookieChanged (in
this case, the CookieMonsterDelegate is an ExtensionCookieMonsterDelegate).
OnCookieChanged is only called in two places, both in CookieMonster, one for
insertions and the other for deletions. The deletion callsite didn't changed -
it takes the CookieMonster::DeletionCause and maps it to the corresponding
CookieStore::ChangeCause (used to be CookieMonsterDelegate::ChangeCause, but the
mapping is the same). The insertion callsite used to always call it with
EXPLICIT, now it's always INSERTED.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:275: ChangeCausePair ChangeCauseMapping[] = {
On 2016/09/26 18:29:26, mmenke wrote:
> While you're here, mind fixing this?  Should be "const kChangeCauseMapping"

Done.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1689: }
On 2016/09/26 18:29:26, mmenke wrote:
> nit:  Remove braces

Done.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:39: CHANGE_COOKIE_INSERTED,
On 2016/09/26 18:29:26, mmenke wrote:
> Can we make this an enum class (And maybe get rid of the prefix)?
> 
> i.e. CookieStore::ChangeCause::INSERTED

Done.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:65: ChangeCause cause)>
On 2016/09/26 07:55:37, Mike West (OOO until 26th) wrote:
> Can you add some tests for this change? It looks like the changes to the
> existing unit tests fold all deletion types together to replicate the old
> interface. It would be great to verify that you're getting the correct values
> out based upon what's passed in.

I've updated cookie_monster_unittest.cc to check the types. I could also make
the same change to cookie_store_ios_unittest.mm if you'd like.

https://codereview.chromium.org/2349823003/diff/40001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:222: // implementors need to be improved to not
behave this way.
On 2016/09/26 18:29:26, mmenke wrote:
> Maybe add a warning that the callback must not synchronously modify another
> cookie, while we're here?

Done.

[Devlin, 2016-09-27 15:48:05]

extensions lgtm

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:147: case
net::CookieStore::CHANGE_COOKIE_UNKNOWN_DELETION:
On 2016/09/26 21:09:10, nharper wrote:
> On 2016/09/26 16:46:09, Devlin (catching up) wrote:
> > Is it accurate to lump these two in here?  e.g. unknown deletion sounds like
> it
> > wasn't necessarily explicit.  And can insertion only happen from explicit
> > insertions?  If so, what's the difference between insertion and explicit?
> 
> I agree that UNKNOWN_DELETION doesn't belong here. I've moved it to be with
the
> default case (the NOTREACHED()). UNKNOWN_DELETION is used in two places. The
> first is in GaiaCookieManagerService when a caller forces the
> GaiaCookieManagerService to run its cookie changed processing. The
> UNKNOWN_DELETION value never leaves the GaiaCookieManagerService in this case.
> The other use of UNKNOWN_DELETION is in CookieStoreIOS, and is only used to
pass
> it in to a CookieChangedCallback, which will never end up here.
> 
> The difference between INSERTED and EXPLICIT is that EXPLICIT can be both an
add
> or a remove, while INSERTED is only an add. According to the documentation at
> https://developer.chrome.com/extensions/cookies#type-OnChangedCause, the only
> reason here that's valid if a cookie wasn't deleted is "explicit". 
> 
> The cause here comes from a call to a CookieMonsterDelegate::OnCookieChanged
(in
> this case, the CookieMonsterDelegate is an ExtensionCookieMonsterDelegate).
> OnCookieChanged is only called in two places, both in CookieMonster, one for
> insertions and the other for deletions. The deletion callsite didn't changed -
> it takes the CookieMonster::DeletionCause and maps it to the corresponding
> CookieStore::ChangeCause (used to be CookieMonsterDelegate::ChangeCause, but
the
> mapping is the same). The insertion callsite used to always call it with
> EXPLICIT, now it's always INSERTED. 

I think that all makes sense - unknown deletion was the really odd one.  Can we
add a comment summarizing why INSERTED is included here?

https://codereview.chromium.org/2349823003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/2349823003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:168: default:
Can we remove the default: case here?

[nharper, 2016-09-27 17:43:48]

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/2349823003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:147: case
net::CookieStore::CHANGE_COOKIE_UNKNOWN_DELETION:
On 2016/09/27 15:48:05, Devlin (catching up) wrote:
> On 2016/09/26 21:09:10, nharper wrote:
> > On 2016/09/26 16:46:09, Devlin (catching up) wrote:
> > > Is it accurate to lump these two in here?  e.g. unknown deletion sounds
like
> > it
> > > wasn't necessarily explicit.  And can insertion only happen from explicit
> > > insertions?  If so, what's the difference between insertion and explicit?
> > 
> > I agree that UNKNOWN_DELETION doesn't belong here. I've moved it to be with
> the
> > default case (the NOTREACHED()). UNKNOWN_DELETION is used in two places. The
> > first is in GaiaCookieManagerService when a caller forces the
> > GaiaCookieManagerService to run its cookie changed processing. The
> > UNKNOWN_DELETION value never leaves the GaiaCookieManagerService in this
case.
> > The other use of UNKNOWN_DELETION is in CookieStoreIOS, and is only used to
> pass
> > it in to a CookieChangedCallback, which will never end up here.
> > 
> > The difference between INSERTED and EXPLICIT is that EXPLICIT can be both an
> add
> > or a remove, while INSERTED is only an add. According to the documentation
at
> > https://developer.chrome.com/extensions/cookies#type-OnChangedCause, the
only
> > reason here that's valid if a cookie wasn't deleted is "explicit". 
> > 
> > The cause here comes from a call to a CookieMonsterDelegate::OnCookieChanged
> (in
> > this case, the CookieMonsterDelegate is an ExtensionCookieMonsterDelegate).
> > OnCookieChanged is only called in two places, both in CookieMonster, one for
> > insertions and the other for deletions. The deletion callsite didn't changed
-
> > it takes the CookieMonster::DeletionCause and maps it to the corresponding
> > CookieStore::ChangeCause (used to be CookieMonsterDelegate::ChangeCause, but
> the
> > mapping is the same). The insertion callsite used to always call it with
> > EXPLICIT, now it's always INSERTED. 
> 
> I think that all makes sense - unknown deletion was the really odd one.  Can
we
> add a comment summarizing why INSERTED is included here?

Done.

https://codereview.chromium.org/2349823003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/2349823003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:168: default:
On 2016/09/27 15:48:05, Devlin (catching up) wrote:
> Can we remove the default: case here?

Done.

[mmenke, 2016-09-27 18:09:21]

chrome/browser/net/ and net/ LGTM, but do wait for mkwst to sign off.

https://codereview.chromium.org/2349823003/diff/80001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/80001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:55: static bool ChangeCauseIsDeletion(ChangeCause
cause);
Need to document this.

[nharper, 2016-09-27 21:50:53]

https://codereview.chromium.org/2349823003/diff/80001/net/cookies/cookie_store.h
File net/cookies/cookie_store.h (right):

https://codereview.chromium.org/2349823003/diff/80001/net/cookies/cookie_stor...
net/cookies/cookie_store.h:55: static bool ChangeCauseIsDeletion(ChangeCause
cause);
On 2016/09/27 18:09:21, mmenke wrote:
> Need to document this.

Done.

[Mike West, 2016-09-28 11:26:08]

LGTM. Thank you for adding the tests.

[Elly Fong-Jones, 2016-09-30 11:13:35]

ios/net/cookies lgtm :)

[nharper, 2016-09-30 16:02:13]

The CQ bit was checked by nharper@chromium.org

[nharper, 2016-09-30 16:02:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
rogerta@chromium.org, rdevlin.cronin@chromium.org, mmenke@chromium.org,
ellyjones@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2349823003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-30 16:02:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 18:25:24]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-30 18:26:46]

Description was changed from

==========
Pass a RemovalCause to CookieChangedCallback

This is to help with debugging the callback used in GaiaCookieManagerService
for the Channel ID mismatch issue.

BUG=548423
==========

to

==========
Pass a RemovalCause to CookieChangedCallback

This is to help with debugging the callback used in GaiaCookieManagerService
for the Channel ID mismatch issue.

BUG=548423

Committed: https://crrev.com/352933e4909adc3b36cd609507918a384e38c99f
Cr-Commit-Position: refs/heads/master@{#422156}
==========

[commit-bot: I haz the power, 2016-09-30 18:26:48]

Patchset 7 (id:??) landed as
https://crrev.com/352933e4909adc3b36cd609507918a384e38c99f
Cr-Commit-Position: refs/heads/master@{#422156}