Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289853002:
Extend the origin-based deletion in TemplateURLService to an URL filter (Closed)
DescriptionExtend the origin-based deletion in TemplateURLService to a URL filter
The current implementation allows filtering for one origin.
The GURL->bool filter that is nowadays used by BrowsingDataRemover
allows specifying whitelists or blacklists of multiple URLs
(typically entire origins).
TBR=droger@chromium.org
BUG=589586
Committed: https://crrev.com/db42f899495a51c7645dfa0ceb97f589fa346e5c
Cr-Commit-Position: refs/heads/master@{#415614}
Patch Set 1 #Patch Set 2 : Updated test, iOS #
Total comments: 9
Patch Set 3 : Addressed comments. #
Messages
Total messages: 34 (21 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...
msramek@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, Please have a look! 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...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Oops... please hold on until I make the bots green :) Codesearch links are apparently still broken, as I didn't see the other callsites...
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.
Alright, the tests are green now :) I realized that TemplateURLService actually works with URLs, not origins, so the limitation on the deletion would be artificial. I renamed |origin_filter| to |url_filter|. In practice, BrowsingDataRemover will always supply a filter that has equivalence classes over URLs on the same origin, but there is no need for TemplateURLService to know that. I fixed the test and the iOS callsite. Please take a look! :)
Please update the CL title and description (origin -> URL?) LGTM https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:554: // object to avoid having to copy them to callback methods. ...as long as that isn't going to introduce a behavior race if we execute a new task before OnKeywordsLoaded() is called. https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:385: void OnKeywordsLoaded(base::Callback<bool(const GURL&)> origin_filter); Nit: Should presumably also be named |url_filter|? https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:206: // auto-generated keywords in the range. What does this last sentence mean? I don't know what it means for a callback to be null. https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:208: const base::Callback<bool(const GURL&)>& origin_filter, Nit: Looks like you didn't actually rename this |url_filter|.
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/2289853002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:554: // object to avoid having to copy them to callback methods. On 2016/08/31 04:30:29, Peter Kasting wrote: > ...as long as that isn't going to introduce a behavior race if we execute a new > task before OnKeywordsLoaded() is called. This is taken care of by the |waiting_for_clear_keyword_data_| below - it (and many similar member variables) must return to false before the next task is executed. https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.h (right): https://codereview.chromium.org/2289853002/diff/20001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.h:385: void OnKeywordsLoaded(base::Callback<bool(const GURL&)> origin_filter); On 2016/08/31 04:30:29, Peter Kasting wrote: > Nit: Should presumably also be named |url_filter|? Done. Yep, missed that. https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:206: // auto-generated keywords in the range. On 2016/08/31 04:30:29, Peter Kasting wrote: > What does this last sentence mean? I don't know what it means for a callback to > be null. I meant base::Callback::is_null(), i.e. an instance of Callback with no function bound. Updated the comment to say "is_null()" so that it's clearer. https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:208: const base::Callback<bool(const GURL&)>& origin_filter, On 2016/08/31 04:30:29, Peter Kasting wrote: > Nit: Looks like you didn't actually rename this |url_filter|. Done. Missed that as well. Sorry for the slopiness :/
Description was changed from ========== Extend the origin-based deletion in TemplateURLService to an origin filter The current implementation allows filtering for one origin. The GURL->bool origin filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple origins. BUG=589586 ========== to ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). BUG=589586 ==========
On 2016/08/31 04:30:29, Peter Kasting wrote: > Please update the CL title and description (origin -> URL?) Done. > > LGTM Thank you!
Description was changed from ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). BUG=589586 ========== to ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 ==========
msramek@chromium.org changed reviewers: + droger@chromium.org
+David as TBR for the updated iOS callsite. This callsite used RemoveAutoGeneratedForOriginBetween() and should be updated to match the method's new signature, but since in reality it did not specify any URLs to filter, it can be replaced by the more specific RemoveAutoGeneratedBetween().
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 pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2289853002/#ps40001 (title: "Addressed comments.")
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...
ios lgtm
Thanks!
Message was sent while issue was closed.
Description was changed from ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 ========== to ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 ========== to ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 Committed: https://crrev.com/db42f899495a51c7645dfa0ceb97f589fa346e5c Cr-Commit-Position: refs/heads/master@{#415614} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/db42f899495a51c7645dfa0ceb97f589fa346e5c Cr-Commit-Position: refs/heads/master@{#415614}
Message was sent while issue was closed.
Description was changed from ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 Committed: https://crrev.com/db42f899495a51c7645dfa0ceb97f589fa346e5c Cr-Commit-Position: refs/heads/master@{#415614} ========== to ========== Extend the origin-based deletion in TemplateURLService to a URL filter The current implementation allows filtering for one origin. The GURL->bool filter that is nowadays used by BrowsingDataRemover allows specifying whitelists or blacklists of multiple URLs (typically entire origins). TBR=droger@chromium.org BUG=589586 Committed: https://crrev.com/db42f899495a51c7645dfa0ceb97f589fa346e5c Cr-Commit-Position: refs/heads/master@{#415614} ==========
Message was sent while issue was closed.
https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2289853002/diff/20001/components/search_engin... components/search_engines/template_url_service.h:206: // auto-generated keywords in the range. On 2016/08/31 11:08:34, msramek wrote: > On 2016/08/31 04:30:29, Peter Kasting wrote: > > What does this last sentence mean? I don't know what it means for a callback > to > > be null. > > I meant base::Callback::is_null(), i.e. an instance of Callback with no function > bound. Updated the comment to say "is_null()" so that it's clearer. Argh, I went looking yesterday through all the callback headers and couldn't find this function, so I thought it had been removed from the codebase. Of course today I find it in three seconds. I actually think the old "is null" read better than "is_null()" and my question was more how someone actually creates a callback in this state, but bleh, don't worry about it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
