Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2554413002:
Extract embedder-specific data types from BrowsingDataRemover (Closed)
DescriptionExtract embedder-specific data types from BrowsingDataRemover
Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover
will delegate all embedder-specific deletions. This class is in 1:1
relationship to BrowsingDataRemover and is owned by it.
Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by
extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl().
This is the first step of the implementation plan. See the design doc for more
background and explanation of the next steps:
https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBEaM/edit#heading=h.m5pzu7ah79n9
This change is covered by BrowsingDataRemoverTest and
BrowsingDataRemoverBrowserTest.
NOTES FOR REVIEWERS:
1. This CL tries to keep the diffs at minimum for easier review, even if the
method ordering could be improved. That will be improved in a follow-up CL.
2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and
ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can
convince themself that they are still equivalent by trusting the test coverage
(58 unittests and 6 browsertests).
3. One can convince themself that no datatype was left out by checking the
list of asynchronous tasks, such as "waiting_for_clear_cache_",
"waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone()
method of both classes.
4. The distribution of datatypes might not be final. For example, downloads are
known to content but have a chrome/ dependency. Similarly, the DNS cache lives
in net/ but is retrieved via a class in chrome/. Future CLs might correct their
placement based on how strong the chrome/ dependencies are.
Patchset 3: Simplify the "bool waiting_for_clear_<type>" paradigm by encapsulating it to a SubTask class.
BUG=668114
Committed: https://crrev.com/3248025c236a2f55f79ff58f696b500f8787f28a
Cr-Commit-Position: refs/heads/master@{#438175}
Patch Set 1 #Patch Set 2 : Android fix #
Total comments: 5
Patch Set 3 : SubTask class #Patch Set 4 : Another Android fix. #Patch Set 5 : Rebase, manual merge of https://codereview.chromium.org/2566123002/ into CBDRC.cc #
Total comments: 4
Patch Set 6 : Addressed nit. #Messages
Total messages: 76 (68 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: This issue passed the CQ dry run.
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #2 (id:20001) has been deleted
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Description was changed from ========== Split RemoveImpl() into platform and embedder-specific datatypes. The deletion calls of RemoveImpl() are wrapped in two sub-methods RemoveEmbedderData() and RemovePlatformData(). Some initialization and metrics that are common to both stay in RemoveImpl(). Both sub-methods take the same arguments as RemoveImpl() (which directly passes them), and an addtional |may_delete_history| parameter which is known to the embedder but needs to be taken into account in the platform datatypes. This will be later passed through ContentBrowserClient. Some reordering: - Moved Http auth cache to passwords - Moved everything related to the storage partition together - Moved offline pages into the cache block BUG=668114 ========== to ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 #1 (id:100001) has been deleted
msramek@chromium.org changed reviewers: + bauerb@chromium.org, mkwst@chromium.org
Hi Bernhard and Mike! Here comes the first - and the messiest (!) - CL of moving BrowsingDataRemover to content/. What will follow is mostly moving classes around, but in this one we need to actually saw BrowsingDataRemover in half. Please have a look! Mind the reviewer notes in the CL description, and the linked design doc for my plan for next steps. Thanks, Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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: This issue passed the CQ dry run.
Looks pretty good! Just a small nit, and a suggestion for moar refactorings: https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1132: std::unique_ptr<WebappRegistry>(new TestWebappRegistry())); Small nit while you're here: Use base::WrapUnique? https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h (right): https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:152: bool waiting_for_synchronous_clear_operations_ = false; If we're rewriting this class anyway... I feel a bit like this list and the list of callbacks have grown out of hand. Would it make sense to replace these with a list of tasks, which essentially have a promise-style interface, so each of the callbacks above can be replaced with a callback with the fulfillment method of a task? We could use those tasks in the content/ BrowsingDataRemover as well.
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
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:160001) has been deleted
https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1132: std::unique_ptr<WebappRegistry>(new TestWebappRegistry())); On 2016/12/12 14:52:06, Bernhard Bauer wrote: > Small nit while you're here: Use base::WrapUnique? Done. https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h (right): https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:152: bool waiting_for_synchronous_clear_operations_ = false; On 2016/12/12 14:52:06, Bernhard Bauer wrote: > If we're rewriting this class anyway... > > I feel a bit like this list and the list of callbacks have grown out of hand. > Would it make sense to replace these with a list of tasks, which essentially > have a promise-style interface, so each of the callbacks above can be replaced > with a callback with the fulfillment method of a task? We could use those tasks > in the content/ BrowsingDataRemover as well. Done. I created a "SubTask" class, the "sub" part is supposed to emphasize that this is something smaller than "RemovalTask". I replaced as much as possible in BDR and CBDRD, just left some custom callback methods as they were. This cut a lot of lines of code, but blew up the constructor (unfortunately, a SubTask needs to have some kind of link to its owner), and also the memory footprint (because of weak ptr factories; this is something that might be worth optimizing). I also tried optimizing the callback to NotifyIfDone() that needs to be shared by all instances of SubTask by instantiating it once and passing a reference, as opposed to copying it to each instance.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Patchset #4 (id:200001) has been deleted
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. ========== to ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. Patchset 3: Simplify the "bool waiting_for_clear_<type>" paradigm by encapsulating it to a SubTask class. 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Awesome, thanks! LGTM with one nit, the rest is optimizations we could do in a followup if you prefer that. https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h (right): https://codereview.chromium.org/2554413002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h:152: bool waiting_for_synchronous_clear_operations_ = false; On 2016/12/12 20:48:56, msramek wrote: > On 2016/12/12 14:52:06, Bernhard Bauer wrote: > > If we're rewriting this class anyway... > > > > I feel a bit like this list and the list of callbacks have grown out of hand. > > Would it make sense to replace these with a list of tasks, which essentially > > have a promise-style interface, so each of the callbacks above can be replaced > > with a callback with the fulfillment method of a task? We could use those > tasks > > in the content/ BrowsingDataRemover as well. > > Done. > > I created a "SubTask" class, the "sub" part is supposed to emphasize that this > is something smaller than "RemovalTask". > > I replaced as much as possible in BDR and CBDRD, just left some custom callback > methods as they were. > > This cut a lot of lines of code, but blew up the constructor (unfortunately, a > SubTask needs to have some kind of link to its owner), and also the memory > footprint (because of weak ptr factories; this is something that might be worth > optimizing). OK. We could cut down the constructor (and NotifyIfDone() as well!) by putting all the subtasks into a vector (after adding the required move constructors), and only pushing the subtasks to it that are actually needed. For reducing the memory footprint, we could plumb all the callbacks through the existing weak_ptr_factory_ in the outer class instead, which will cancel all callbacks on destruction. > I also tried optimizing the callback to NotifyIfDone() that needs to be shared > by all instances of SubTask by instantiating it once and passing a reference, as > opposed to copying it to each instance. Eh, callback copies are actually pretty cheap (they have internal refcounted storage). https://codereview.chromium.org/2554413002/diff/240001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2554413002/diff/240001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:129: BrowsingDataRemover::CompletionInhibitor* Ooh... this could be made an additional SubTask as well that can be injected from the outside :) https://codereview.chromium.org/2554413002/diff/240001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:154: DCHECK(!forward_callback_.is_null()); DCHECK this in the constructor?
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...
Thanks Bernhard! I addressed the nit, and the rest of the optimizations I would indeed prefer to do in a followup. Don't want to keep this CL brewing for too long as it's prone to merge collisions (and I already had one!). Mike, since you're marked as "(slow)", I'm leaving this as FYI (or TBR) for you. https://codereview.chromium.org/2554413002/diff/240001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2554413002/diff/240001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:129: BrowsingDataRemover::CompletionInhibitor* On 2016/12/13 13:26:22, Bernhard Bauer wrote: > Ooh... this could be made an additional SubTask as well that can be injected > from the outside :) Acknowledged. There's also a TODO to make it non-static which we'll finally get rid of :) https://codereview.chromium.org/2554413002/diff/240001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_remover.cc:154: DCHECK(!forward_callback_.is_null()); On 2016/12/13 13:26:22, Bernhard Bauer wrote: > DCHECK this in the constructor? Done.
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 bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2554413002/#ps260001 (title: "Addressed nit.")
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": 260001, "attempt_start_ts": 1481638925686880,
"parent_rev": "ee3417c04fb1eaaa23f673fb530cb3cc9d9a44c4", "commit_rev":
"f0ebbecb7e9308bfa34f21cc9b6463748a222bd3"}
Message was sent while issue was closed.
Description was changed from ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. Patchset 3: Simplify the "bool waiting_for_clear_<type>" paradigm by encapsulating it to a SubTask class. BUG=668114 ========== to ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. Patchset 3: Simplify the "bool waiting_for_clear_<type>" paradigm by encapsulating it to a SubTask class. BUG=668114 Review-Url: https://codereview.chromium.org/2554413002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. Patchset 3: Simplify the "bool waiting_for_clear_<type>" paradigm by encapsulating it to a SubTask class. BUG=668114 Review-Url: https://codereview.chromium.org/2554413002 ========== to ========== Extract embedder-specific data types from BrowsingDataRemover Define BrowsingDataRemoverDelegate, a class to which BrowsingDataRemover will delegate all embedder-specific deletions. This class is in 1:1 relationship to BrowsingDataRemover and is owned by it. Create a Chrome-specific subclass ChromeBrowsingDataRemoverDelegate by extracting all Chrome datatypes from BrowsingDataRemover::RemoveImpl(). This is the first step of the implementation plan. See the design doc for more background and explanation of the next steps: https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBE... This change is covered by BrowsingDataRemoverTest and BrowsingDataRemoverBrowserTest. NOTES FOR REVIEWERS: 1. This CL tries to keep the diffs at minimum for easier review, even if the method ordering could be improved. That will be improved in a follow-up CL. 2. However, the diffs widely differ in BrowsingDataRemover::RemoveImpl and ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. Fortunately, one can convince themself that they are still equivalent by trusting the test coverage (58 unittests and 6 browsertests). 3. One can convince themself that no datatype was left out by checking the list of asynchronous tasks, such as "waiting_for_clear_cache_", "waiting_for_clear_cookies_" etc. present in both .h files and in the AllDone() method of both classes. 4. The distribution of datatypes might not be final. For example, downloads are known to content but have a chrome/ dependency. Similarly, the DNS cache lives in net/ but is retrieved via a class in chrome/. Future CLs might correct their placement based on how strong the chrome/ dependencies are. Patchset 3: Simplify the "bool waiting_for_clear_<type>" paradigm by encapsulating it to a SubTask class. BUG=668114 Committed: https://crrev.com/3248025c236a2f55f79ff58f696b500f8787f28a Cr-Commit-Position: refs/heads/master@{#438175} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3248025c236a2f55f79ff58f696b500f8787f28a Cr-Commit-Position: refs/heads/master@{#438175} |
