Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2594723002:
Count number of origins with data affected by clearing "cookies and site data". (Closed)
|
Created:
4 years ago by dullweber Modified:
3 years, 9 months ago Reviewers:
msramek CC:
chromium-reviews, dbeam+watch-options_chromium.org, msramek+watch_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, raymes+watch_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, markusheintz_ Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCount number of origins with data affected by clearing "cookies and site data".
BUG=674526
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2594723002
Cr-Commit-Position: refs/heads/master@{#454851}
Committed: https://chromium.googlesource.com/chromium/src/+/04af4448e4833d6d1e9e06059ab9947a4d936111
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : add CookieCountingHelper #Patch Set 4 : add browser tests, fix memory leak, fix channel GURLs #Patch Set 5 : Add comments for issues with incomplete data deletion #
Total comments: 37
Patch Set 6 : fix include guards #Patch Set 7 : fix browser_tests compilation #Patch Set 8 : remove comments #
Total comments: 26
Patch Set 9 : fix review issues #
Total comments: 6
Patch Set 10 : convert browsertest to unittest #Patch Set 11 : file filepath strings #
Total comments: 2
Patch Set 12 : try fix android compile #Patch Set 13 : fix android test #Patch Set 14 : fix android test #Patch Set 15 : only use counter for cbd tab ui #Patch Set 16 : dont create cookie counter if not needed #Patch Set 17 : rebase #Patch Set 18 : improve string description #
Total comments: 6
Patch Set 19 : small fixes #Messages
Total messages: 56 (43 generated)
Description was changed from ========== Count number of origins with data affected by clearing "cookies and site data". BUG=674526 ========== to ========== Count number of origins with data affected by clearing "cookies and site data". BUG=674526 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dullweber@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-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dullweber@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_asan_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 dullweber@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_asan_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 dullweber@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.
dullweber@chromium.org changed reviewers: + msramek@chromium.org
Hi Martin, please have a look at this cl. There is still some debug output but I added comments for all cases of incomplete data deletion that I found.
I left the first round of comments. I didn't review the logic of the counter yet, as one of my comments might cause it to restructure. I responded to the four callsites with incomplete deletions and recommended an action; please follow those actions and clean up the comments before sending the CL to other reviewers. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:183: "browsing_data/cookie_counter.cc", Please remove the "chrome/browser_data" file that you accidentally uploaded. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.cc:92: return base::UTF8ToUTF16(result.str()); We should definitely output more than just a number - something like "%d sites" / "%d websites" etc. (check what's in the mocks). Please add the new string to generated_resources.grd. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:152: // TODO? This code skips quota entries without size and hides them this way I see. This is a bug somewhere in StoragePartition / QuotaManager. Could you please file a bug and either remove the TODO from here, or change it to TODO(crbug.com/123456)? https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:157: info.syncable_usage <= 0) style nit: I'd prefer keeping the empty line below for better readabilty. While you're here can you please also add braces? if without braces is only allowed if the condition fits on one line. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counter.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Ditto. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counter.h (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: s/(c) //g (we don't use "(c)" anymore in new files) nit: It's 2017. Happy new year! https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:9: #include "chrome/browser/profiles/profile.h" nit: Forward declaration should be enough. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:12: class CookieCounter : public browsing_data::BrowsingDataCounter { Even though we colloquially refers to the item in the CBD dialog as "cookies", I think it's quite misleading here. I would suggest naming it CookiesAndSiteDataCounter, or simply SiteDataCounter (as cookies are also site data). https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:20: Profile* profile_; style: Methods before attributes (I know, I know :) ) https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counting_helper.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Ditto. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counting_helper.h (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Ditto. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper.h:31: class CookieCountingHelper { Note that we have SiteDataSizeCollector (used in chrome://settings/ -> Device -> Storage management, at least on ChromeOS, not sure about Desktop). Can it be reused to simplify this class? I know that SiteDataSizeCollector misses a few things (e.g. the website settings), but most of the stuff should be there. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counting_helper_browsertest.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper_browsertest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Ditto. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/clear_browser_data_overlay.html (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.html:44: aria-controls="delete-cookies-counter"> I don't think it was the right decision, but anyways - we decided not to add the new counters to options/, only to settings/. https://codereview.chromium.org/2594723002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2594723002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:728: std::vector<GURL> HostContentSettingsMap::GetSitesWithSettingsInPrefProvider( The website settings that are part of REMOVE_SITE_DATA (APP_BANNER, SITE_ENGAGEMENT, DURABLE_STORAGE) can't be managed by policies or extensions, therefore you can assume they all come from PrefProvider. We can remove this and instead use HostContentSettingsMap::HostContentSettingsMap::GetSettingsForOneType (and then do the pattern->url conversion on the caller side). https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_impl.cc (right): https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_impl.cc:261: /*if (session_storage_database_.get()) { Since you already know how to solve it, just need a confirmation from the code owners that it's the right thing, I'd suggest extracting this to a separate CL. https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:173: style: This empty line is superfluous. https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:177: // TODO? This method sometimes leaves traces of localstorage behind that show Let's file this one as a bug as well. Please leave a TODO with a bug number. https://codereview.chromium.org/2594723002/diff/80001/storage/browser/fileapi... File storage/browser/fileapi/file_system_quota_client.cc (right): https://codereview.chromium.org/2594723002/diff/80001/storage/browser/fileapi... storage/browser/fileapi/file_system_quota_client.cc:170: // TODO? This method only deletes filesytem entries associated with a quota Let's ask FS owners about this.
The CQ bit was checked by dullweber@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/2594723002/diff/80001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:183: "browsing_data/cookie_counter.cc", On 2017/01/09 12:54:43, msramek wrote: > Please remove the "chrome/browser_data" file that you accidentally uploaded. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_counter_utils.cc:92: return base::UTF8ToUTF16(result.str()); On 2017/01/09 12:54:43, msramek wrote: > We should definitely output more than just a number - something like "%d sites" > / "%d websites" etc. (check what's in the mocks). Please add the new string to > generated_resources.grd. I added the new string from the discussion document: "42 sites use cookies. Cookies keep you signed in to your favorite sites.". There are also other strings in the slides from Max. "For 42 sites" and a two line string that is probably too long for the current ui. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:152: // TODO? This code skips quota entries without size and hides them this way On 2017/01/09 12:54:43, msramek wrote: > I see. This is a bug somewhere in StoragePartition / QuotaManager. Could you > please file a bug and either remove the TODO from here, or change it to > TODO(crbug.com/123456)? I created https://bugs.chromium.org/p/chromium/issues/detail?id=679330 and removed the todo. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_quota_helper_impl.cc:157: info.syncable_usage <= 0) On 2017/01/09 12:54:43, msramek wrote: > style nit: I'd prefer keeping the empty line below for better readabilty. While > you're here can you please also add braces? if without braces is only allowed if > the condition fits on one line. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counter.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/01/09 12:54:43, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counter.h (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/01/09 12:54:43, msramek wrote: > nit: s/(c) //g (we don't use "(c)" anymore in new files) > > nit: It's 2017. Happy new year! Updated all new files https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:9: #include "chrome/browser/profiles/profile.h" On 2017/01/09 12:54:43, msramek wrote: > nit: Forward declaration should be enough. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:12: class CookieCounter : public browsing_data::BrowsingDataCounter { On 2017/01/09 12:54:43, msramek wrote: > Even though we colloquially refers to the item in the CBD dialog as "cookies", I > think it's quite misleading here. I would suggest naming it > CookiesAndSiteDataCounter, or simply SiteDataCounter (as cookies are also site > data). Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counter.h:20: Profile* profile_; On 2017/01/09 12:54:43, msramek wrote: > style: Methods before attributes (I know, I know :) ) Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counting_helper.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/01/09 12:54:43, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counting_helper.h (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/01/09 12:54:44, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper.h:31: class CookieCountingHelper { On 2017/01/09 12:54:43, msramek wrote: > Note that we have SiteDataSizeCollector (used in chrome://settings/ -> Device -> > Storage management, at least on ChromeOS, not sure about Desktop). > > Can it be reused to simplify this class? I know that SiteDataSizeCollector > misses a few things (e.g. the website settings), but most of the stuff should be > there. The SiteDataSizeCollector uses the same helper classes as the CookieTreeModel. I think the structure of shared helper classes to retrieve the relevant information is very nice but the helpers currently don't support retrieval of data by timestamp. Also the logic of what exactly is site_data differs slightly between the CookieTreeModel and the CBD dialog. I don't think that is a good idea to differ between these dialogs as I thought that settings/cookies is just a detail view for what is considered "cookies and site data" in CBD. Should I see if I can extend the helper classes and maybe later also change the deletion to use these helpers? https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... File chrome/browser/browsing_data/cookie_counting_helper_browsertest.cc (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/browsing... chrome/browser/browsing_data/cookie_counting_helper_browsertest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2017/01/09 12:54:44, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/clear_browser_data_overlay.html (right): https://codereview.chromium.org/2594723002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/clear_browser_data_overlay.html:44: aria-controls="delete-cookies-counter"> On 2017/01/09 12:54:44, msramek wrote: > I don't think it was the right decision, but anyways - we decided not to add the > new counters to options/, only to settings/. Ok, I reverted this https://codereview.chromium.org/2594723002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2594723002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:728: std::vector<GURL> HostContentSettingsMap::GetSitesWithSettingsInPrefProvider( On 2017/01/09 12:54:44, msramek wrote: > The website settings that are part of REMOVE_SITE_DATA (APP_BANNER, > SITE_ENGAGEMENT, DURABLE_STORAGE) can't be managed by policies or extensions, > therefore you can assume they all come from PrefProvider. > > We can remove this and instead use > HostContentSettingsMap::HostContentSettingsMap::GetSettingsForOneType (and then > do the pattern->url conversion on the caller side). Done. https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_impl.cc (right): https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_impl.cc:261: /*if (session_storage_database_.get()) { On 2017/01/09 12:54:44, msramek wrote: > Since you already know how to solve it, just need a confirmation from the code > owners that it's the right thing, I'd suggest extracting this to a separate CL. I created https://codereview.chromium.org/2622603002 https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2594723002/diff/80001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_context_wrapper.cc:177: // TODO? This method sometimes leaves traces of localstorage behind that show On 2017/01/09 12:54:44, msramek wrote: > Let's file this one as a bug as well. Please leave a TODO with a bug number. I filed it as http://crbug.com/679344 https://codereview.chromium.org/2594723002/diff/80001/storage/browser/fileapi... File storage/browser/fileapi/file_system_quota_client.cc (right): https://codereview.chromium.org/2594723002/diff/80001/storage/browser/fileapi... storage/browser/fileapi/file_system_quota_client.cc:170: // TODO? This method only deletes filesytem entries associated with a quota On 2017/01/09 12:54:44, msramek wrote: > Let's ask FS owners about this. I created http://crbug.com/679362 for it.
https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper.cc (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:103: GetOriginsFromHostContentSettignsMap(hcsm, APP_BANNER would also be removed as a part of REMOVE_SITE_USAGE_DATA, which is a part of REMOVE_SITE_DATA. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:123: for (const auto& rule : settings) { Don't use auto if the type is not obvious. Here, it's not obvious that ContentSettingsForOneType is std::vector<ContentSettingPatternSource>. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:151: for (const auto& cookie : cookies) { Ditto. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:220: for (const auto& channel_id : channel_ids) { Ditto. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:222: origins.push_back(GURL("https://" + channel_id.server_identifier())); Hmm, this might lead to a wrong count. If I visited https://example.com:4443 and have one local storage entry and one channel ID from that visit, it will count as two sites, because the first origin will be https://example.com:4443 and the second one https://example.com(:443). The same probably goes for cookies (which are domain-scoped). We could improve this by not adding origin if an origin with the same scheme and host (but possibly different port) already exists. But it's also not a huge problem, so leaving a comment / TODO is fine. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:234: if (BrowsingDataHelper::HasWebScheme(origin)) { If an origin with a non-web scheme, e.g. the file:// scheme has site data, I don't think we should ignore it. Generally, we don't want to show "0" in the CBD dialog if there actually are data to delete. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:35: void TearDownOnMainThread() override {} Do we need this? https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:122: volatile int tasks_; I don't think volatile is necessary here. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:125: Could you please add a test for one other data storage backend, and then also one for the fact that the same origin from two different data storage backends (e.g. cookies and local storage) is not counted twice? https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:144: sub-label="[[counters_.cookies]]" This was "$i18n{clearCookiesCounter}" (i.e. the dummy string for cookies, as we don't have [[counters_.cookies]] in settings/. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/clear_browser_data_handler.cc:65: browsing_data::prefs::kDeleteCookies, This shouldn't be here if we don't have a counter for cookies. https://codereview.chromium.org/2594723002/diff/140001/components/browsing_da... File components/browsing_data_strings.grdp (right): https://codereview.chromium.org/2594723002/diff/140001/components/browsing_da... components/browsing_data_strings.grdp:3: style: Unnecessary empty line. https://codereview.chromium.org/2594723002/diff/140001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2594723002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:723: } Please re-add the empty line here. That's currently the only change in storage_partition_impl.cc, and it's not reasonable for it to be a part of this CL just because of that.
https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper.cc (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:103: GetOriginsFromHostContentSettignsMap(hcsm, On 2017/01/10 10:23:02, msramek wrote: > APP_BANNER would also be removed as a part of REMOVE_SITE_USAGE_DATA, which is a > part of REMOVE_SITE_DATA. Done. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:123: for (const auto& rule : settings) { On 2017/01/10 10:23:02, msramek wrote: > Don't use auto if the type is not obvious. Here, it's not obvious that > ContentSettingsForOneType is std::vector<ContentSettingPatternSource>. Done. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:151: for (const auto& cookie : cookies) { On 2017/01/10 10:23:02, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:220: for (const auto& channel_id : channel_ids) { On 2017/01/10 10:23:02, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:222: origins.push_back(GURL("https://" + channel_id.server_identifier())); On 2017/01/10 10:23:02, msramek wrote: > Hmm, this might lead to a wrong count. > > If I visited https://example.com:4443 and have one local storage entry and one > channel ID from that visit, it will count as two sites, because the first origin > will be https://example.com:4443 and the second one https://example.com(:443). > > The same probably goes for cookies (which are domain-scoped). > > We could improve this by not adding origin if an origin with the same scheme and > host (but possibly different port) already exists. But it's also not a huge > problem, so leaving a comment / TODO is fine. Added a comment to explain the problem. There are dozens of entries after you visited a bigger website anyway, so I don't think it is a big issue. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper.cc:234: if (BrowsingDataHelper::HasWebScheme(origin)) { On 2017/01/10 10:23:02, msramek wrote: > If an origin with a non-web scheme, e.g. the file:// scheme has site data, I > don't think we should ignore it. Generally, we don't want to show "0" in the CBD > dialog if there actually are data to delete. Oh, I took that from the BrowsingData*Helper classes. Removed it to be consistent with StoragePartitionImpl https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:35: void TearDownOnMainThread() override {} On 2017/01/10 10:23:02, msramek wrote: > Do we need this? Done. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:122: volatile int tasks_; On 2017/01/10 10:23:02, msramek wrote: > I don't think volatile is necessary here. Done. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:125: On 2017/01/10 10:23:02, msramek wrote: > Could you please add a test for one other data storage backend, and then also > one for the fact that the same origin from two different data storage backends > (e.g. cookies and local storage) is not counted twice? added a test for localstorage and the combination of localstorage and cookies https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:144: sub-label="[[counters_.cookies]]" On 2017/01/10 10:23:02, msramek wrote: > This was "$i18n{clearCookiesCounter}" (i.e. the dummy string for cookies, as we > don't have [[counters_.cookies]] in settings/. This is the new md-settings page and there is a counters_.cookies now. https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right): https://codereview.chromium.org/2594723002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/clear_browser_data_handler.cc:65: browsing_data::prefs::kDeleteCookies, On 2017/01/10 10:23:02, msramek wrote: > This shouldn't be here if we don't have a counter for cookies. Done. https://codereview.chromium.org/2594723002/diff/140001/components/browsing_da... File components/browsing_data_strings.grdp (right): https://codereview.chromium.org/2594723002/diff/140001/components/browsing_da... components/browsing_data_strings.grdp:3: On 2017/01/10 10:23:02, msramek wrote: > style: Unnecessary empty line. Done. https://codereview.chromium.org/2594723002/diff/140001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/2594723002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:723: } On 2017/01/10 10:23:02, msramek wrote: > Please re-add the empty line here. That's currently the only change in > storage_partition_impl.cc, and it's not reasonable for it to be a part of this > CL just because of that. Done.
Thanks. I think this looks good now, but there are still unnecessary files that should not have been uploaded with this CL, please look at my comments below. However, I'll wait with the formal L-G-T-M right now. The reason is that we don't have the new cookie counter string reviewed by the UI review yet, and since we'll have an important branchpoint (for the MD settings) soon, I think it's not a good time to change it to an ad-hoc one. Alternatively, consider landing this CL without linking it to the UI just yet. https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc (right): https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:24: class SiteDataCountingHelperBrowserTest : public InProcessBrowserTest { Optional: Since StoragePartitionImpl seems to test more or less the same functionality in a unittest, we should probably be able to use a unittest here as well (which is a lot faster). That would mean e.g. using TestBrowserThreadBundle to simulate the IO thread. https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:892: if (base::PathExists(profile_path)) { This is not a part of this CL. https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right): https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/clear_browser_data_handler.cc:63: browsing_data::prefs::kDeleteBrowsingHistory, Since we're not making any relevant change in options/, we should not touch their files just because of formatting.
The CQ bit was checked by dullweber@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_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 dullweber@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...
I changed the browsertest to a unittest and fixed the filepath encoding. I hope the try bots on android and windows are happy now https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc (right): https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_browsertest.cc:24: class SiteDataCountingHelperBrowserTest : public InProcessBrowserTest { On 2017/01/10 13:46:32, msramek wrote: > Optional: Since StoragePartitionImpl seems to test more or less the same > functionality in a unittest, we should probably be able to use a unittest here > as well (which is a lot faster). That would mean e.g. using > TestBrowserThreadBundle to simulate the IO thread. I converted it to a unittest https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:892: if (base::PathExists(profile_path)) { On 2017/01/10 13:46:32, msramek wrote: > This is not a part of this CL. That's right, I have no idea how I managed to get this in here https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right): https://codereview.chromium.org/2594723002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/clear_browser_data_handler.cc:63: browsing_data::prefs::kDeleteBrowsingHistory, On 2017/01/10 13:46:32, msramek wrote: > Since we're not making any relevant change in options/, we should not touch > their files just because of formatting. Done.
LGTM, thanks! But as I said, please do not land before we confirm the string. https://codereview.chromium.org/2594723002/diff/200001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper_unittest.cc (right): https://codereview.chromium.org/2594723002/diff/200001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_unittest.cc:15: #include "chrome/test/base/in_process_browser_test.h" nit: Not needed anymore (but I'm surprised that this works without including "testing/gtest/include/gtest/gtest.h", it's probably bundled somewhere).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dullweber@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.
Message was sent while issue was closed.
Description was changed from ========== Count number of origins with data affected by clearing "cookies and site data". BUG=674526 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Count number of origins with data affected by clearing "cookies and site data". BUG=674526 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dullweber@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 dullweber@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...
I changed this cl, so that the cookie counter will only be created if the new dialog is used. Please take another look at the changed code. https://codereview.chromium.org/2594723002/diff/200001/chrome/browser/browsin... File chrome/browser/browsing_data/site_data_counting_helper_unittest.cc (right): https://codereview.chromium.org/2594723002/diff/200001/chrome/browser/browsin... chrome/browser/browsing_data/site_data_counting_helper_unittest.cc:15: #include "chrome/test/base/in_process_browser_test.h" On 2017/01/10 17:21:33, msramek wrote: > nit: Not needed anymore (but I'm surprised that this works without including > "testing/gtest/include/gtest/gtest.h", it's probably bundled somewhere). Done.
LGTM % comments. We'll definitely need to improve the string in the future, as it should also reference "site data", not just cookies, but that doesn't matter for now. https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:66: return base::MakeUnique<SiteDataCounter>(profile); style: braces if the "if" statement takes more than one line And while you're here, please also fix the previous one, as I didn't notice when I was reviewing that change. https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.cc:101: // Cookie counter. DCHECK(IsCookieCounterEnabled()) https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.h:16: bool IsCookieCounterEnabled(); Should this be rather called "IsSiteDataCounterEnabled()", since that's how the class is called?
The CQ bit was checked by dullweber@chromium.org
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/2594723002/#ps360001 (title: "small fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right): https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_factory.cc:66: return base::MakeUnique<SiteDataCounter>(profile); On 2017/03/06 09:40:57, msramek wrote: > style: braces if the "if" statement takes more than one line > > And while you're here, please also fix the previous one, as I didn't notice when > I was reviewing that change. Done. https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right): https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.cc:101: // Cookie counter. On 2017/03/06 09:40:57, msramek wrote: > DCHECK(IsCookieCounterEnabled()) Done. https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... File chrome/browser/browsing_data/browsing_data_counter_utils.h (right): https://codereview.chromium.org/2594723002/diff/340001/chrome/browser/browsin... chrome/browser/browsing_data/browsing_data_counter_utils.h:16: bool IsCookieCounterEnabled(); On 2017/03/06 09:40:57, msramek wrote: > Should this be rather called "IsSiteDataCounterEnabled()", since that's how the > class is called? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1488797064560920,
"parent_rev": "f193ee6d21e6548ed425746ec80fb71faa7998a5", "commit_rev":
"04af4448e4833d6d1e9e06059ab9947a4d936111"}
Message was sent while issue was closed.
Description was changed from ========== Count number of origins with data affected by clearing "cookies and site data". BUG=674526 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Count number of origins with data affected by clearing "cookies and site data". BUG=674526 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2594723002 Cr-Commit-Position: refs/heads/master@{#454851} Committed: https://chromium.googlesource.com/chromium/src/+/04af4448e4833d6d1e9e06059ab9... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://chromium.googlesource.com/chromium/src/+/04af4448e4833d6d1e9e06059ab9... |
