Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221143003:
Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter() (Closed)
DescriptionFormalize the restrictions of BrowsingDataRemover::RemoveWithFilter()
Only some of the datatypes in BrowsingDataRemover currently support
per-URL (/ per-origin / per-domain) filtering. This is currently noted
in a comment next to the RemoveWithFilter() method.
This CL formalizes which datatypes are filterable and DCHECKS if
RemoveWithFilter() is used with an unsupported type.
In addition, this CL adds a guildeline on adding new browsing data types.
The motivation is the upcoming launch of the Clear-Site-Data header which
needs to handle filtering of REMOVE_CACHE and REMOVE_SITE_DATA*. We want
to ensure that developers won't add new data storage backends falling into
these buckets without properly handing them. These guidelines should be
enforced in code reviews.
This CL immediately applies the guidelines for the zero suggest cache.
[*] REMOVE_PLUGIN_DATA filtering is still not implemented as of this CL.
BUG=589586
Committed: https://crrev.com/549dfeeb8e66aa91e05abe927dbe699ef4124b2c
Cr-Commit-Position: refs/heads/master@{#412208}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase (switch to my Android checkout). #Patch Set 3 : Addressed comments, fixed Android. #
Total comments: 2
Patch Set 4 : Explicit task count. #
Total comments: 2
Messages
Total messages: 40 (28 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 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...
msramek@chromium.org changed reviewers: + bauerb@chromium.org, markusheintz@chromium.org, michaeln@chromium.org, mkwst@chromium.org
Hello fellow browsing_data/ owners, What do you think about this? It's becoming very difficult to understand when it's OK to use RemoveWithFilter() and when not. I have a spreadsheet to keep track, but I'd like the code to be a bit more self-documenting. Cheers, Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
lgtm https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:352: DCHECK_EQ(FILTERABLE_DATATYPES, remove_mask | FILTERABLE_DATATYPES); Nit: If you changed this to DCHECK_EQ(0, remove_mask & ~FILTERABLE_DATATYPES), you would immediately see the remaining bits in case of a failure (right?). https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:1036: // another server round-trip; no data are actually lost. Nit: "no data is actually lost" :) https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:171: REMOVE_CACHE | Indent one more space.
Patchset #3 (id:60001) 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 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: + dmurph@chromium.org
Thanks, Bernhard! I addressed your comments, but I realized that the red Android tests came from that DCHECK, so I had to update pref_service_bridge as well. Since Android is your domain, PTAL! :) For some background, there was one callsite which called deletion for non-filterable datatypes with a filter. Fortunately, I know that this wasn't a bug and Dan (+cc) knew what he was doing. But it's time to formalize what is deleted with a filter and what is not. Otherwise, as I continue my work with plumbing filters to other data storage backends, I inadvertently affect the set of data types that important sites protect. Dan - in the last patchset I'm using filter for all datatypes for which it is possible. But that is probably not what you want for important sites, right? You only want cookies, site data and cache? https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:352: DCHECK_EQ(FILTERABLE_DATATYPES, remove_mask | FILTERABLE_DATATYPES); On 2016/08/10 14:22:13, Bernhard Bauer wrote: > Nit: If you changed this to DCHECK_EQ(0, remove_mask & ~FILTERABLE_DATATYPES), > you would immediately see the remaining bits in case of a failure (right?). Done. You will see the remaining bits as a decimal number that you still have to convert and compare with enum, so it's still not super helpful, but it's definitely better. https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:1036: // another server round-trip; no data are actually lost. On 2016/08/10 14:22:14, Bernhard Bauer wrote: > Nit: "no data is actually lost" :) *grumble* *grumble* Done. :) But yeah, I remember reading somewhere that singular is preferred, either it was in our code, or generally in en_US. https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:171: REMOVE_CACHE | On 2016/08/10 14:22:14, Bernhard Bauer wrote: > Indent one more space. Done.
https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2221143003/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:1036: // another server round-trip; no data are actually lost. On 2016/08/11 13:11:01, msramek wrote: > On 2016/08/10 14:22:14, Bernhard Bauer wrote: > > Nit: "no data is actually lost" :) > > *grumble* *grumble* Done. :) But yeah, I remember reading somewhere that > singular is preferred, either it was in our code, or generally in en_US. Yes, I think in this case you are not talking about the plural of "datum" (http://www.merriam-webster.com/dictionary/datum), but rather the uncountable noun "data" :) https://codereview.chromium.org/2221143003/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2221143003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:525: : task_count_(2), I think I would slightly prefer passing the initial callback count in as a parameter to the constructor, so the call site can decide.
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...
Patchset #4 (id:100001) has been deleted
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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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_...)
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...
https://codereview.chromium.org/2221143003/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2221143003/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:525: : task_count_(2), On 2016/08/11 13:25:43, Bernhard Bauer wrote: > I think I would slightly prefer passing the initial callback count in as a > parameter to the constructor, so the call site can decide. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2221143003/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2221143003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:673: env, obj, browsing_data_remover, 2 /* tasks_count */); I was actually thinking to increase the count for each of the masks that is non-zero, but this works too (and it avoids the edge case where both are zero).
https://codereview.chromium.org/2221143003/diff/120001/chrome/browser/android... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/2221143003/diff/120001/chrome/browser/android... chrome/browser/android/preferences/pref_service_bridge.cc:673: env, obj, browsing_data_remover, 2 /* tasks_count */); On 2016/08/11 15:16:16, Bernhard Bauer wrote: > I was actually thinking to increase the count for each of the masks that is > non-zero, but this works too (and it avoids the edge case where both are zero). I see. Yes, I was exactly trying to avoid that edge case so that it's very obvious that |observer| won't leak. The current solution allows you to quickly convince yourself about it - we initialize it to 2, and on the next 20 lines it's clear that all execution paths lead to 2 callbacks.
lgtm
Thanks! I'll land this now (I'm not waiting for other people's LGTM since this CL is quite small and does not really need an extensive review, I just wanted to make y'all aware of it).
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...
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter() Only some of the datatypes in BrowsingDataRemover currently support per-URL (/ per-origin / per-domain) filtering. This is currently noted in a comment next to the RemoveWithFilter() method. This CL formalizes which datatypes are filterable and DCHECKS if RemoveWithFilter() is used with an unsupported type. In addition, this CL adds a guildeline on adding new browsing data types. The motivation is the upcoming launch of the Clear-Site-Data header which needs to handle filtering of REMOVE_CACHE and REMOVE_SITE_DATA*. We want to ensure that developers won't add new data storage backends falling into these buckets without properly handing them. These guidelines should be enforced in code reviews. This CL immediately applies the guidelines for the zero suggest cache. [*] REMOVE_PLUGIN_DATA filtering is still not implemented as of this CL. BUG=589586 ========== to ========== Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter() Only some of the datatypes in BrowsingDataRemover currently support per-URL (/ per-origin / per-domain) filtering. This is currently noted in a comment next to the RemoveWithFilter() method. This CL formalizes which datatypes are filterable and DCHECKS if RemoveWithFilter() is used with an unsupported type. In addition, this CL adds a guildeline on adding new browsing data types. The motivation is the upcoming launch of the Clear-Site-Data header which needs to handle filtering of REMOVE_CACHE and REMOVE_SITE_DATA*. We want to ensure that developers won't add new data storage backends falling into these buckets without properly handing them. These guidelines should be enforced in code reviews. This CL immediately applies the guidelines for the zero suggest cache. [*] REMOVE_PLUGIN_DATA filtering is still not implemented as of this CL. BUG=589586 Committed: https://crrev.com/549dfeeb8e66aa91e05abe927dbe699ef4124b2c Cr-Commit-Position: refs/heads/master@{#412208} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/549dfeeb8e66aa91e05abe927dbe699ef4124b2c Cr-Commit-Position: refs/heads/master@{#412208} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
