Support the removal of only session cookies or persistent cookies

Support the removal of only session cookies or persistent cookies
through the <webview> cleardata API.

BUG=687082
Review-Url: https://codereview.chromium.org/2700473003
Cr-Commit-Position: refs/heads/master@{#451119}
Committed: https://chromium.googlesource.com/chromium/src/+/66cdb5cf76f67ac76c426998dece842e7b739209

[lfg, 2017-02-15 18:33:20]

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

[commit-bot: I haz the power, 2017-02-15 18:34:11]

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

[lfg, 2017-02-15 19:39:20]

lfg@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lfg, 2017-02-15 19:39:20]

Hey Istiaque, can you take a look?

[commit-bot: I haz the power, 2017-02-15 20:18:02]

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

[commit-bot: I haz the power, 2017-02-15 20:18:03]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-02-15 21:45:27]

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/bootstrap.js
(right):

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/bootstrap.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
here and other places: 2017

Also my comments in either of the two test files actually apply to both. I
didn't want to explode the CL with comments.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/bootstrap.js:41:
{ 'since': 1}, { 'persistentCookies': true },
rm space before 'since'

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html
(right):

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html:20:
var notifyEmbedder = function(msg_array) {
msgArray

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html:39:
if (part.lastIndexOf(nameEq, 0) == 0) {
part.startsWith(nameEq) will also work.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html:51:
setCookie("foo", "fooValue");
nit: here and elsewhere: all strings single quoted

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js
(right):

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:12:
// step1. Ask guest to load some cookie.
load session (bar) and persistent (foo) cookies.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:14:
// embedder clears cookie data of the guest via clearData API.
clears session cookie data...

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:16:
// step4. Guest responds with cookie values, embedder verifies they are unset.
verifies session cookie is unset but persistent cookie is still set.

Do the same for other test to update the step descriptions.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:47:
chrome.test.assertEq(['fooValue', null], cookies);
Here you want to note what this means. e.g. barValue is session so it's gone,
etc.

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:96: mask |=
StoragePartition::REMOVE_DATA_MASK_COOKIES;
nit: {}

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:436: if ((removal_mask
& webview::WEB_VIEW_REMOVE_DATA_MASK_COOKIES) ||
It's a bit more code, but for readability, consider:
bool remove_session_cookies = !!(mask&SESSION_COOKIES);
bool remove_persistent_cookies !!(mask&..PERSISTENT_COOKIES);
bool remove_all_cookies = !!(mask&MASK_COOKIES) || (remove_session_cookies &&
remove_persistent_cookies);


if (remove_all_cookies) ...
else if (remove_persistent) ..
else if (remove_session) ...
else NOTREACHED

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:441: [](const
net::CanonicalCookie& cookie) -> bool { return true; });
I also wonder if we should continue to use the non CookieMatcherFunction variant
of ClearData below when we know we want to remove all. As removing all should be
the most common case and we shouldn't require calling callback for each cookie.
WDYT?

https://codereview.chromium.org/2700473003/diff/1/extensions/common/api/web_v...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/2700473003/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:26: "description": "The browser's
cookies."
Add a note saying this will remove all cookies including session cookies and
persistent ones.

https://codereview.chromium.org/2700473003/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:31: "description": "The browser's
session cookies."
Here and also for other places: replace browser's with WebSites', since this
applies to <webview> but not entire browser.

[lfg, 2017-02-15 22:42:01]

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

[commit-bot: I haz the power, 2017-02-15 22:43:07]

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

[lfg, 2017-02-15 22:44:01]

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/bootstrap.js
(right):

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/bootstrap.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2017/02/15 21:45:26, lazyboy wrote:
> here and other places: 2017
> 
> Also my comments in either of the two test files actually apply to both. I
> didn't want to explode the CL with comments.

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/bootstrap.js:41:
{ 'since': 1}, { 'persistentCookies': true },
On 2017/02/15 21:45:26, lazyboy wrote:
> rm space before 'since'

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html
(right):

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html:20:
var notifyEmbedder = function(msg_array) {
On 2017/02/15 21:45:26, lazyboy wrote:
> msgArray

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html:39:
if (part.lastIndexOf(nameEq, 0) == 0) {
On 2017/02/15 21:45:26, lazyboy wrote:
> part.startsWith(nameEq) will also work.

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_persistent/guest.html:51:
setCookie("foo", "fooValue");
On 2017/02/15 21:45:26, lazyboy wrote:
> nit: here and elsewhere: all strings single quoted

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js
(right):

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:12:
// step1. Ask guest to load some cookie.
On 2017/02/15 21:45:27, lazyboy wrote:
> load session (bar) and persistent (foo) cookies.

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:14:
// embedder clears cookie data of the guest via clearData API.
On 2017/02/15 21:45:26, lazyboy wrote:
> clears session cookie data...

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:16:
// step4. Guest responds with cookie values, embedder verifies they are unset.
On 2017/02/15 21:45:26, lazyboy wrote:
> verifies session cookie is unset but persistent cookie is still set.
> 
> Do the same for other test to update the step descriptions.

Done.

https://codereview.chromium.org/2700473003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/common/cleardata_session/bootstrap.js:47:
chrome.test.assertEq(['fooValue', null], cookies);
On 2017/02/15 21:45:26, lazyboy wrote:
> Here you want to note what this means. e.g. barValue is session so it's gone,
> etc.

Done.

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:96: mask |=
StoragePartition::REMOVE_DATA_MASK_COOKIES;
On 2017/02/15 21:45:27, lazyboy wrote:
> nit: {}

Done.

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:436: if ((removal_mask
& webview::WEB_VIEW_REMOVE_DATA_MASK_COOKIES) ||
On 2017/02/15 21:45:27, lazyboy wrote:
> It's a bit more code, but for readability, consider:
> bool remove_session_cookies = !!(mask&SESSION_COOKIES);
> bool remove_persistent_cookies !!(mask&..PERSISTENT_COOKIES);
> bool remove_all_cookies = !!(mask&MASK_COOKIES) || (remove_session_cookies &&
> remove_persistent_cookies);
> 
> 
> if (remove_all_cookies) ...
> else if (remove_persistent) ..
> else if (remove_session) ...
> else NOTREACHED

Done, but the NOTREACHED part isn't correct, since the API may be used to clear
data that doesn't involve cookies.

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:441: [](const
net::CanonicalCookie& cookie) -> bool { return true; });
On 2017/02/15 21:45:27, lazyboy wrote:
> I also wonder if we should continue to use the non CookieMatcherFunction
variant
> of ClearData below when we know we want to remove all. As removing all should
be
> the most common case and we shouldn't require calling callback for each
cookie.
> WDYT?

Good point. I switched so that the CookieMatcherFunction is left unset when
removing all cookies. ClearData will do the right thing and skip it, see
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c...
.

https://codereview.chromium.org/2700473003/diff/1/extensions/common/api/web_v...
File extensions/common/api/web_view_internal.json (right):

https://codereview.chromium.org/2700473003/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:26: "description": "The browser's
cookies."
On 2017/02/15 21:45:27, lazyboy wrote:
> Add a note saying this will remove all cookies including session cookies and
> persistent ones.

Done.

https://codereview.chromium.org/2700473003/diff/1/extensions/common/api/web_v...
extensions/common/api/web_view_internal.json:31: "description": "The browser's
session cookies."
On 2017/02/15 21:45:27, lazyboy wrote:
> Here and also for other places: replace browser's with WebSites', since this
> applies to <webview> but not entire browser.

Done.

[commit-bot: I haz the power, 2017-02-16 00:32:51]

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

[commit-bot: I haz the power, 2017-02-16 00:32:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[lazyboy, 2017-02-16 02:15:34]

lgtm

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:436: if ((removal_mask
& webview::WEB_VIEW_REMOVE_DATA_MASK_COOKIES) ||
On 2017/02/15 22:44:00, lfg wrote:
> On 2017/02/15 21:45:27, lazyboy wrote:
> > It's a bit more code, but for readability, consider:
> > bool remove_session_cookies = !!(mask&SESSION_COOKIES);
> > bool remove_persistent_cookies !!(mask&..PERSISTENT_COOKIES);
> > bool remove_all_cookies = !!(mask&MASK_COOKIES) || (remove_session_cookies
&&
> > remove_persistent_cookies);
> > 
> > 
> > if (remove_all_cookies) ...
> > else if (remove_persistent) ..
> > else if (remove_session) ...
> > else NOTREACHED
> 
> Done, but the NOTREACHED part isn't correct, since the API may be used to
clear
> data that doesn't involve cookies.

Acknowledged.

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:441: [](const
net::CanonicalCookie& cookie) -> bool { return true; });
On 2017/02/15 22:44:00, lfg wrote:
> On 2017/02/15 21:45:27, lazyboy wrote:
> > I also wonder if we should continue to use the non CookieMatcherFunction
> variant
> > of ClearData below when we know we want to remove all. As removing all
should
> be
> > the most common case and we shouldn't require calling callback for each
> cookie.
> > WDYT?
> 
> Good point. I switched so that the CookieMatcherFunction is left unset when
> removing all cookies. ClearData will do the right thing and skip it, see
>
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c...
> .

Great, I'd also comment this before line 434 saying empty cookie_matcher will
cause all cookies to be purged.

[lfg, 2017-02-16 17:44:25]

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/2700473003/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:441: [](const
net::CanonicalCookie& cookie) -> bool { return true; });
On 2017/02/16 02:15:34, lazyboy wrote:
> On 2017/02/15 22:44:00, lfg wrote:
> > On 2017/02/15 21:45:27, lazyboy wrote:
> > > I also wonder if we should continue to use the non CookieMatcherFunction
> > variant
> > > of ClearData below when we know we want to remove all. As removing all
> should
> > be
> > > the most common case and we shouldn't require calling callback for each
> > cookie.
> > > WDYT?
> > 
> > Good point. I switched so that the CookieMatcherFunction is left unset when
> > removing all cookies. ClearData will do the right thing and skip it, see
> >
>
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c...
> > .
> 
> Great, I'd also comment this before line 434 saying empty cookie_matcher will
> cause all cookies to be purged.

Done.

[lfg, 2017-02-16 17:44:48]

The CQ bit was checked by lfg@chromium.org

[lfg, 2017-02-16 17:44:49]

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

[commit-bot: I haz the power, 2017-02-16 17:45:43]

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, 2017-02-16 17:55:25]

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

[commit-bot: I haz the power, 2017-02-16 17:55:27]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2017-02-16 18:38:00]

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

[commit-bot: I haz the power, 2017-02-16 18:38:38]

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

[lfg, 2017-02-16 19:02:01]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2017-02-16 19:02:02]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2017-02-16 19:03:33]

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, 2017-02-16 22:25:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487271722412200,
"parent_rev": "b756297e51407c3b602c6d65746a8f18d835d71b", "commit_rev":
"66cdb5cf76f67ac76c426998dece842e7b739209"}

[commit-bot: I haz the power, 2017-02-16 22:26:57]

Description was changed from

==========
Support the removal of only session cookies or persistent cookies
through the <webview> cleardata API.

BUG=687082
==========

to

==========
Support the removal of only session cookies or persistent cookies
through the <webview> cleardata API.

BUG=687082

Review-Url: https://codereview.chromium.org/2700473003
Cr-Commit-Position: refs/heads/master@{#451119}
Committed:
https://chromium.googlesource.com/chromium/src/+/66cdb5cf76f67ac76c426998dece...
==========

[commit-bot: I haz the power, 2017-02-16 22:26:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/66cdb5cf76f67ac76c426998dece...