Clears out external protocol data when cookies and site data is cleared.

Clears out external protocol data when cookies and site data is cleared.

Clears out the external protocol data stored on Profile, when user
clears browsing history, and checks 'cookies and other site and plugin
data'.

BUG=457254
Review-Url: https://codereview.chromium.org/2664253006
Cr-Commit-Position: refs/heads/master@{#451517}
Committed: https://chromium.googlesource.com/chromium/src/+/561a9cde06e63fc405ccedf47842ebab307d906d

[ramyasharma, 2017-02-09 03:01:06]

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

[commit-bot: I haz the power, 2017-02-09 03:01:38]

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, 2017-02-09 05:11:50]

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

[commit-bot: I haz the power, 2017-02-09 05:11:51]

Dry run: This issue passed the CQ dry run.

[ramyasharma, 2017-02-09 06:31:47]

Patchset #1 (id:1) has been deleted

[ramyasharma, 2017-02-09 06:32:06]

Description was changed from

==========
Clears out external protocol data when cookies and site data is cleared.

Clears out the external protocol data stored on Profile, when user
clears browsing history, and checks 'cookies and other site and plugin
data'.

BUG=457254
==========

to

==========
Clears out external protocol data when cookies and site data is cleared.

Clears out the external protocol data stored on Profile, when user
clears browsing history, and checks 'cookies and other site and plugin
data'.

BUG=457254
==========

[ramyasharma, 2017-02-09 06:32:07]

ramyasharma@chromium.org changed reviewers:
+ dominickn@chromium.org

[dominickn, 2017-02-10 03:10:29]

Ready for review?

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:931: if
(remove_mask & BrowsingDataRemover::REMOVE_COOKIES)
REMOVE_SITE_DATA instead?

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:353:
prefs->ClearPref(prefs::kExcludedSchemes);
I don't think you need the if check here - do the tests pass without it? prefs
shouldn't be null when this is called.

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler_unittest.cc
(right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler_unittest.cc:263: 
Nit: remove the extra newline

[msramek, 2017-02-10 17:20:11]

msramek@chromium.org changed reviewers:
+ msramek@chromium.org

[msramek, 2017-02-10 17:20:12]

Drive-by review! (And thanks for doing this!)

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:931: if
(remove_mask & BrowsingDataRemover::REMOVE_COOKIES)
On 2017/02/10 03:10:29, dominickn wrote:
> REMOVE_SITE_DATA instead?

Note that REMOVE_SITE_DATA is a union of several types. This would work
correctly when used from the UI, but would otherwise be quite non-intuitive.
Please add a new datatype to the mask and then also add it as a disjunct of
REMOVE_SITE_DATA.

Please also add a testcase to chrome_browsing_data_remover_delegate_unittest.cc.

[ramyasharma, 2017-02-13 06:03:02]

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

[commit-bot: I haz the power, 2017-02-13 06:03:14]

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, 2017-02-13 06:34:13]

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

[commit-bot: I haz the power, 2017-02-13 06:34:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ramyasharma, 2017-02-14 00:11:40]

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

[commit-bot: I haz the power, 2017-02-14 00:12:52]

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

[ramyasharma, 2017-02-14 00:17:17]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-14 00:19:18]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ramyasharma, 2017-02-14 00:20:49]

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

[commit-bot: I haz the power, 2017-02-14 00:21:21]

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

[ramyasharma, 2017-02-14 00:26:19]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-14 01:13:49]

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

[commit-bot: I haz the power, 2017-02-14 01:13:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ramyasharma, 2017-02-14 04:04:25]

Patchset #2 (id:80001) has been deleted

[ramyasharma, 2017-02-14 04:04:48]

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

[commit-bot: I haz the power, 2017-02-14 04:05:25]

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, 2017-02-14 04:50:37]

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

[commit-bot: I haz the power, 2017-02-14 04:50:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ramyasharma, 2017-02-14 06:45:36]

Patchset #2 (id:100001) has been deleted

[ramyasharma, 2017-02-15 02:21:18]

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

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

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

[ramyasharma, 2017-02-15 02:29:46]

Patchset #2 (id:120001) has been deleted

[ramyasharma, 2017-02-15 02:54:18]

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

[commit-bot: I haz the power, 2017-02-15 02:55:45]

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

[ramyasharma, 2017-02-15 03:22:21]

Patchset #2 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-02-15 05:01:35]

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

[commit-bot: I haz the power, 2017-02-15 05:01:36]

Dry run: This issue passed the CQ dry run.

[ramyasharma, 2017-02-15 05:03:36]

Thanks Dom and msramek. PTAL?

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:931: if
(remove_mask & BrowsingDataRemover::REMOVE_COOKIES)
On 2017/02/10 17:20:12, msramek wrote:
> On 2017/02/10 03:10:29, dominickn wrote:
> > REMOVE_SITE_DATA instead?
> 
> Note that REMOVE_SITE_DATA is a union of several types. This would work
> correctly when used from the UI, but would otherwise be quite non-intuitive.
> Please add a new datatype to the mask and then also add it as a disjunct of
> REMOVE_SITE_DATA.
> 
> Please also add a testcase to
chrome_browsing_data_remover_delegate_unittest.cc.

Thanks msramek, done.

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:931: if
(remove_mask & BrowsingDataRemover::REMOVE_COOKIES)
On 2017/02/10 03:10:29, dominickn wrote:
> REMOVE_SITE_DATA instead?

Acknowledged.

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:353:
prefs->ClearPref(prefs::kExcludedSchemes);
On 2017/02/10 03:10:29, dominickn wrote:
> I don't think you need the if check here - do the tests pass without it? prefs
> shouldn't be null when this is called.

Thanks I had not tried it without the check

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler_unittest.cc
(right):

https://codereview.chromium.org/2664253006/diff/20001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler_unittest.cc:263: 
On 2017/02/10 03:10:29, dominickn wrote:
> Nit: remove the extra newline

Done.

[ramyasharma, 2017-02-15 05:14:38]

ramyasharma@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[ramyasharma, 2017-02-15 05:16:46]

ramyasharma@chromium.org changed reviewers:
+ meacer@chromium.org

[ramyasharma, 2017-02-15 05:16:46]

meacer@chromium.org: Please review changes in 
chrome/browser/external_protocol/*

Martin: I needed to add some extra changes, in browser extensions api, after
introducing a new mask REMOVE_EXTERNAL_PROTOCOL_DATA. PTAL if this change looks
ok?

[Devlin, 2017-02-15 15:56:46]

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.h (right):

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.h:313: class
BrowsingDataRemoveExternalProtocolDataFunction
I don't see anywhere in the bug or CL description that indicates this should be
adding a new extension API function, nor anywhere that defines this function so
it could be exposed to an extension.  Is this intentional/necessary?

[msramek, 2017-02-15 20:36:53]

c/b/browsing_data/ LGTM

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:54: const char
kExternalProtocolDataKey[] = "externalProtocolData";
Note that it is not a requirement from privacy side to wire every new datatype
to the extension `browsingData` API.

If you only did so because this test suite (browsing_data_test.cc) was failing,
you can use the workaround from this CL instead:
https://codereview.chromium.org/2354843006/

[ramyasharma, 2017-02-16 01:09:55]

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

[commit-bot: I haz the power, 2017-02-16 01:11:22]

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

[ramyasharma, 2017-02-16 01:14:08]

Patchset #3 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-02-16 01:16:53]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ramyasharma, 2017-02-16 01:20:13]

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

[commit-bot: I haz the power, 2017-02-16 01:20:47]

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

[ramyasharma, 2017-02-16 01:23:40]

Patchset #3 (id:200001) has been deleted

[ramyasharma, 2017-02-16 01:24:01]

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

[commit-bot: I haz the power, 2017-02-16 01:24:54]

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, 2017-02-16 03:28:52]

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

[commit-bot: I haz the power, 2017-02-16 03:28:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[ramyasharma, 2017-02-16 03:29:48]

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:54: const char
kExternalProtocolDataKey[] = "externalProtocolData";
On 2017/02/15 20:36:52, msramek wrote:
> Note that it is not a requirement from privacy side to wire every new datatype
> to the extension `browsingData` API.
> 
> If you only did so because this test suite (browsing_data_test.cc) was
failing,
> you can use the workaround from this CL instead:
> https://codereview.chromium.org/2354843006/

Thank you. This worked, and I reverted the changes to the extension api.

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.h (right):

https://codereview.chromium.org/2664253006/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.h:313: class
BrowsingDataRemoveExternalProtocolDataFunction
On 2017/02/15 15:56:46, Devlin wrote:
> I don't see anywhere in the bug or CL description that indicates this should
be
> adding a new extension API function, nor anywhere that defines this function
so
> it could be exposed to an extension.  Is this intentional/necessary?

This API was changed because test suite browsing_data_test.cc was failing. And I
was not sure if there is a way to work around these failures. But as msramek@
confirmed it is not necessary to add to the extension API, and there is another
way to make the tests pass, I am reverting these changes, and not implementing
this API change. Sorry for the confusion, I was waiting to hear from msramek@
before updating the cl.

[Devlin, 2017-02-17 23:36:17]

extensions lgtm

[meacer, 2017-02-18 00:19:52]

chrome/browser/external_protocol/* lgtm

[ramyasharma, 2017-02-19 07:08:17]

The CQ bit was checked by ramyasharma@chromium.org

[ramyasharma, 2017-02-19 07:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2664253006/#ps220001
(title: "a")

[commit-bot: I haz the power, 2017-02-19 07:08:29]

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-19 07:49:29]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487488097697930,
"parent_rev": "22f522e10dd7a816ec45547fd35a6fa0356b3d21", "commit_rev":
"561a9cde06e63fc405ccedf47842ebab307d906d"}

[commit-bot: I haz the power, 2017-02-19 07:50:22]

Description was changed from

==========
Clears out external protocol data when cookies and site data is cleared.

Clears out the external protocol data stored on Profile, when user
clears browsing history, and checks 'cookies and other site and plugin
data'.

BUG=457254
==========

to

==========
Clears out external protocol data when cookies and site data is cleared.

Clears out the external protocol data stored on Profile, when user
clears browsing history, and checks 'cookies and other site and plugin
data'.

BUG=457254

Review-Url: https://codereview.chromium.org/2664253006
Cr-Commit-Position: refs/heads/master@{#451517}
Committed:
https://chromium.googlesource.com/chromium/src/+/561a9cde06e63fc405ccedf47842...
==========

[commit-bot: I haz the power, 2017-02-19 07:50:23]

Committed patchset #3 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/561a9cde06e63fc405ccedf47842...