Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892953002:
Remove the ClearCookies and ClearCache methods from ContentBrowserClient (Closed)
DescriptionRemove the ClearCookies and ClearCache methods from ContentBrowserClient
These methods were used by content/ to request the deletion of cookies
and cache from embedder. This should no longer be necessary, since
BrowsingDataRemover has been moved to content/ and can be used directly.
Embedders that need to execute additional deletions can still subclass
BrowsingDataRemoverDelegate and implement them there.
There were only two ContentBrowserClient subclasses that used
the methods:
1. ChromeContentBrowserClient::ClearCache() was used by dev_tools/.
This was replaced by an equivalent call to BrowsingDataRemover.
2. In android_webview, ClearCookies() and ClearCache() were defined,
but not called. ClearCache() used android_webview::RemoveHttpDiskCache
for cache deletion, which is in fact a subset of functionality that
already exists in BrowsingDataRemover, and can be replaced by it.
BUG=668114
Review-Url: https://codereview.chromium.org/2892953002
Cr-Commit-Position: refs/heads/master@{#473434}
Committed: https://chromium.googlesource.com/chromium/src/+/507bbc4d6eaf6ebd7b5ed1e63b611169e0d13308
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 4
Patch Set 3 : Use BrowsingDataRemover to delete cache. #Patch Set 4 : Remove unused includes #Patch Set 5 : Merge conflict in BUILD.gn -> Rebase #Messages
Total messages: 53 (31 generated)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msramek@chromium.org changed reviewers: + boliu@chromium.org, dgozman@chromium.org
Hi Dmitry and Bo, Please have a look! Dmitry: devtools/ and content/public/ Bo: android_webview/ - I left one TODO there; if you agree with it, I can resolve it immediately in this CL as well. Thanks, Martin
this CL lgtm https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... File android_webview/browser/aw_contents.cc (right): https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... android_webview/browser/aw_contents.cc:716: // BrowsingDataRemover would do for REMOVE_CACHE mask. Consider calling that so what does does REMOVE_CACHE remove?
Thanks! https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... File android_webview/browser/aw_contents.cc (right): https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... android_webview/browser/aw_contents.cc:716: // BrowsingDataRemover would do for REMOVE_CACHE mask. Consider calling that On 2017/05/19 17:31:31, boliu wrote: > so what does does REMOVE_CACHE remove? It calls to StoragePartitionHttpCacheDataRemover, which clears disk_cache::Backend for HTTP and media request contexts (same as android_webview already does), and then also QUIC and SDCH configuration.
https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... File android_webview/browser/aw_contents.cc (right): https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... android_webview/browser/aw_contents.cc:716: // BrowsingDataRemover would do for REMOVE_CACHE mask. Consider calling that On 2017/05/19 17:35:29, msramek wrote: > On 2017/05/19 17:31:31, boliu wrote: > > so what does does REMOVE_CACHE remove? > > It calls to StoragePartitionHttpCacheDataRemover, which clears > disk_cache::Backend for HTTP and media request contexts (same as android_webview > already does), and then also QUIC and SDCH configuration. Ahh, different protocols. Yes, this should clear those as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... File android_webview/browser/aw_contents.cc (right): https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... android_webview/browser/aw_contents.cc:716: // BrowsingDataRemover would do for REMOVE_CACHE mask. Consider calling that On 2017/05/19 17:38:36, boliu wrote: > On 2017/05/19 17:35:29, msramek wrote: > > On 2017/05/19 17:31:31, boliu wrote: > > > so what does does REMOVE_CACHE remove? > > > > It calls to StoragePartitionHttpCacheDataRemover, which clears > > disk_cache::Backend for HTTP and media request contexts (same as > android_webview > > already does), and then also QUIC and SDCH configuration. > > Ahh, different protocols. Yes, this should clear those as well. Actually, I have one clarifying question :) BrowsingDataRemover clears cache for the default storage partition, because in Chrome, only packaged apps have separate storage partitions and we don't need to handle their data deletion. Does android_webview use more than one storage partition? If it's possible that web_contents_->GetRenderProcessHost() on line 719 uses a nondefault storage partition, then we can't straightforwardly replace this with BrowsingDataRemover. (But we would still be able to use StoragePartitionHttpCacheDataRemover to delete QUIC and SDCH).
On 2017/05/19 18:14:59, msramek wrote: > https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... > File android_webview/browser/aw_contents.cc (right): > > https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... > android_webview/browser/aw_contents.cc:716: // BrowsingDataRemover would do for > REMOVE_CACHE mask. Consider calling that > On 2017/05/19 17:38:36, boliu wrote: > > On 2017/05/19 17:35:29, msramek wrote: > > > On 2017/05/19 17:31:31, boliu wrote: > > > > so what does does REMOVE_CACHE remove? > > > > > > It calls to StoragePartitionHttpCacheDataRemover, which clears > > > disk_cache::Backend for HTTP and media request contexts (same as > > android_webview > > > already does), and then also QUIC and SDCH configuration. > > > > Ahh, different protocols. Yes, this should clear those as well. > > Actually, I have one clarifying question :) BrowsingDataRemover clears cache for > the default storage partition, because in Chrome, only packaged apps have > separate storage partitions and we don't need to handle their data deletion. > > Does android_webview use more than one storage partition? Nope > If it's possible that > web_contents_->GetRenderProcessHost() on line 719 uses a nondefault storage > partition, then we can't straightforwardly replace this with > BrowsingDataRemover. (But we would still be able to use > StoragePartitionHttpCacheDataRemover to delete QUIC and SDCH).
Description was changed from ========== Remove the ClearCookies and ClearCache methods from ContentBrowserClient These methods were used by content/ to request the deletion of cookies and cache from embedder. This should no longer be necessary, since BrowsingDataRemover has been moved to content/ and can be used directly. Embedders that need to execute additional deletions can still subclass BrowsingDataRemoverDelegate and implement them there. There were only two ContentBrowserClient subclasses that used the methods: 1. ChromeContentBrowserClient::ClearCache() was used by dev_tools/. This was replaced by an equivalent call to BrowsingDataRemover. 2. In android_webview, ClearCookies() and ClearCache() were defined, but not called. ClearCache() used android_webview::RemoveHttpDiskCache for cache deletion, which is in fact a subset of functionality that already exists in BrowsingDataRemover, and could be replaced by it to avoid code duplication. A TODO was added for this. BUG=668114 ========== to ========== Remove the ClearCookies and ClearCache methods from ContentBrowserClient These methods were used by content/ to request the deletion of cookies and cache from embedder. This should no longer be necessary, since BrowsingDataRemover has been moved to content/ and can be used directly. Embedders that need to execute additional deletions can still subclass BrowsingDataRemoverDelegate and implement them there. There were only two ContentBrowserClient subclasses that used the methods: 1. ChromeContentBrowserClient::ClearCache() was used by dev_tools/. This was replaced by an equivalent call to BrowsingDataRemover. 2. In android_webview, ClearCookies() and ClearCache() were defined, but not called. ClearCache() used android_webview::RemoveHttpDiskCache for cache deletion, which is in fact a subset of functionality that already exists in BrowsingDataRemover, and can be replaced by it. BUG=668114 ==========
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
On 2017/05/19 18:18:35, boliu wrote: > On 2017/05/19 18:14:59, msramek wrote: > > > https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... > > File android_webview/browser/aw_contents.cc (right): > > > > > https://codereview.chromium.org/2892953002/diff/20001/android_webview/browser... > > android_webview/browser/aw_contents.cc:716: // BrowsingDataRemover would do > for > > REMOVE_CACHE mask. Consider calling that > > On 2017/05/19 17:38:36, boliu wrote: > > > On 2017/05/19 17:35:29, msramek wrote: > > > > On 2017/05/19 17:31:31, boliu wrote: > > > > > so what does does REMOVE_CACHE remove? > > > > > > > > It calls to StoragePartitionHttpCacheDataRemover, which clears > > > > disk_cache::Backend for HTTP and media request contexts (same as > > > android_webview > > > > already does), and then also QUIC and SDCH configuration. > > > > > > Ahh, different protocols. Yes, this should clear those as well. > > > > Actually, I have one clarifying question :) BrowsingDataRemover clears cache > for > > the default storage partition, because in Chrome, only packaged apps have > > separate storage partitions and we don't need to handle their data deletion. > > > > Does android_webview use more than one storage partition? > > Nope Alright, PTAL :) > > > If it's possible that > > web_contents_->GetRenderProcessHost() on line 719 uses a nondefault storage > > partition, then we can't straightforwardly replace this with > > BrowsingDataRemover. (But we would still be able to use > > StoragePartitionHttpCacheDataRemover to delete QUIC and SDCH).
Can remove net_disk_cache_remover.cc/h as well I think?
On 2017/05/19 18:49:18, boliu wrote: > Can remove net_disk_cache_remover.cc/h as well I think? oh wait, can't read, lgtm
Thanks! But you did make me double-check and notice that I didn't remove their #include-s :)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Thanks! Landing this now.
The CQ bit was unchecked by msramek@chromium.org
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2892953002/#ps80001 (title: "Remove unused includes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by msramek@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2892953002/#ps100001 (title: "Merge conflict in BUILD.gn -> Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by msramek@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by msramek@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495280928857820,
"parent_rev": "1b298605df3e00d8bb378fb2c17a8014d0d1c578", "commit_rev":
"507bbc4d6eaf6ebd7b5ed1e63b611169e0d13308"}
Message was sent while issue was closed.
Description was changed from ========== Remove the ClearCookies and ClearCache methods from ContentBrowserClient These methods were used by content/ to request the deletion of cookies and cache from embedder. This should no longer be necessary, since BrowsingDataRemover has been moved to content/ and can be used directly. Embedders that need to execute additional deletions can still subclass BrowsingDataRemoverDelegate and implement them there. There were only two ContentBrowserClient subclasses that used the methods: 1. ChromeContentBrowserClient::ClearCache() was used by dev_tools/. This was replaced by an equivalent call to BrowsingDataRemover. 2. In android_webview, ClearCookies() and ClearCache() were defined, but not called. ClearCache() used android_webview::RemoveHttpDiskCache for cache deletion, which is in fact a subset of functionality that already exists in BrowsingDataRemover, and can be replaced by it. BUG=668114 ========== to ========== Remove the ClearCookies and ClearCache methods from ContentBrowserClient These methods were used by content/ to request the deletion of cookies and cache from embedder. This should no longer be necessary, since BrowsingDataRemover has been moved to content/ and can be used directly. Embedders that need to execute additional deletions can still subclass BrowsingDataRemoverDelegate and implement them there. There were only two ContentBrowserClient subclasses that used the methods: 1. ChromeContentBrowserClient::ClearCache() was used by dev_tools/. This was replaced by an equivalent call to BrowsingDataRemover. 2. In android_webview, ClearCookies() and ClearCache() were defined, but not called. ClearCache() used android_webview::RemoveHttpDiskCache for cache deletion, which is in fact a subset of functionality that already exists in BrowsingDataRemover, and can be replaced by it. BUG=668114 Review-Url: https://codereview.chromium.org/2892953002 Cr-Commit-Position: refs/heads/master@{#473434} Committed: https://chromium.googlesource.com/chromium/src/+/507bbc4d6eaf6ebd7b5ed1e63b61... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/507bbc4d6eaf6ebd7b5ed1e63b61... |
