Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752263003:
Count site data size for important sites (Closed)
|
Created:
3 years, 9 months ago by dullweber Modified:
3 years, 7 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, tfarina, dominickn+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGet the size of local storage and used quota for important sites.
BUG=698692
Review-Url: https://codereview.chromium.org/2752263003
Cr-Commit-Position: refs/heads/master@{#472813}
Committed: https://chromium.googlesource.com/chromium/src/+/4870daad28213d17ea5bcea9de843a13f0006d67
Patch Set 1 #Patch Set 2 : fix test #Patch Set 3 : add localstorage to test case #
Total comments: 7
Patch Set 4 : fix comments #Patch Set 5 : rebase #Patch Set 6 : switch to SmallMap #
Total comments: 31
Patch Set 7 : fix comments #Patch Set 8 : move usage counter to separate file #Patch Set 9 : rebase #
Total comments: 16
Patch Set 10 : fix comments #
Total comments: 1
Patch Set 11 : rebase #Patch Set 12 : <utility> newline #Patch Set 13 : rebase #
Total comments: 32
Patch Set 14 : fix comments #Patch Set 15 : Use weakptr #Patch Set 16 : rebase and remove deb file #
Total comments: 2
Patch Set 17 : change runloop order #Patch Set 18 : rebase #Depends on Patchset: Messages
Total messages: 82 (57 generated)
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.
Description was changed from ========== Count site data size for important sites BUG=698692 ========== to ========== Get the size of local storage and used quota for important sites. BUG=698692 ==========
dullweber@chromium.org changed reviewers: + dmurph@chromium.org, msramek@chromium.org
Hi, I added some code to calculate the size of localstorage and quota usage for important sites. I'm not sure if we also want to add cache size here because cache is also influenced by important sites?
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.
https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:440: std::map<std::string, ImportantDomainInfo*> site_map; unordered_map should be more suitable here -- or even a base::SmallMap< std::unordered_map<string, ImportantDomainInfo*> >. https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.h (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.h:69: static void PopulateUsage(storage::QuotaManager* quota_manager, Can you document threading expectations? I guess this whole class expects UI thread, so maybe putting that at the top would be good. https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util_unittest.cc:194: run_loop_->Run(); What would happen if we just created a runloop and did RunUntilIdle, instead of calling quit above? This would be cleaner, as we don't need the local variable or the special method.
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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...
On 2017/03/24 10:43:05, dullweber wrote: > Hi, > I added some code to calculate the size of localstorage and quota usage for > important sites. I'm not sure if we also want to add cache size here because > cache is also influenced by important sites? Hmm, good point. Cache will be tricky, because we can calculate it quickly on mobile, but not on Desktop. So I would consider doing it on mobile, but not here. We could communicate it explicitly by saying something like "10 MB of site data". Or we can completely drop cache from the "Important sites" concept, at least on Desktop. I actually wonder if we'll see any performance differences with Android. So far, I have only tested origin-based deletion on small caches.
On 2017/03/28 13:57:39, msramek wrote: > On 2017/03/24 10:43:05, dullweber wrote: > > Hi, > > I added some code to calculate the size of localstorage and quota usage for > > important sites. I'm not sure if we also want to add cache size here because > > cache is also influenced by important sites? > > Hmm, good point. Cache will be tricky, because we can calculate it quickly on > mobile, but not on Desktop. So I would consider doing it on mobile, but not > here. > > We could communicate it explicitly by saying something like "10 MB of site > data". > > Or we can completely drop cache from the "Important sites" concept, at least on > Desktop. I actually wonder if we'll see any performance differences with > Android. So far, I have only tested origin-based deletion on small caches. Ah, right. Desktop is too slow with the current cache backend. The Android UI doesn't show the used space at all so it is probably not useful to add the code now.
Thanks, I fixed the issues https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:440: std::map<std::string, ImportantDomainInfo*> site_map; On 2017/03/27 22:22:45, dmurph wrote: > unordered_map should be more suitable here -- or even a base::SmallMap< > std::unordered_map<string, ImportantDomainInfo*> >. > I changed it to unordered_map. This map can have up to kMaxImportantSites = 10 entries but is accessed very often (for every localstorage and quota entry). The default size, up to which SmallMap handles the map differently, is kArraySize = 4. I'm not sure if SmallMap could have an advantage. https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.h (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.h:69: static void PopulateUsage(storage::QuotaManager* quota_manager, On 2017/03/27 22:22:45, dmurph wrote: > Can you document threading expectations? I guess this whole class expects UI > thread, so maybe putting that at the top would be good. I checked the other methods and it looks like all of them should be on the UI thread. I added a comment to the class. https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util_unittest.cc:194: run_loop_->Run(); On 2017/03/27 22:22:45, dmurph wrote: > What would happen if we just created a runloop and did RunUntilIdle, instead of > calling quit above? This would be cleaner, as we don't need the local variable > or the special method. That works :) I thought RunUntilIdle() only waits for tasks on the UI thread and that thread is idle for most of the time until the callback is executed but TestBrowserThreadBundle only has a single thread for all tasks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:440: std::map<std::string, ImportantDomainInfo*> site_map; On 2017/03/28 14:06:45, dullweber wrote: > On 2017/03/27 22:22:45, dmurph wrote: > > unordered_map should be more suitable here -- or even a base::SmallMap< > > std::unordered_map<string, ImportantDomainInfo*> >. > > > > I changed it to unordered_map. > > This map can have up to kMaxImportantSites = 10 entries but is accessed very > often (for every localstorage and quota entry). > The default size, up to which SmallMap handles the map differently, is > kArraySize = 4. > I'm not sure if SmallMap could have an advantage. Then SmallMap will definitely be faster. Iterating a vector with size < 200 is faster than using map, as iterating is super fast. Also we aren't making modifications to the map, so there is no vector resizing.
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...
On 2017/03/28 21:51:13, dmurph wrote: > https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... > File chrome/browser/engagement/important_sites_util.cc (right): > > https://codereview.chromium.org/2752263003/diff/40001/chrome/browser/engageme... > chrome/browser/engagement/important_sites_util.cc:440: std::map<std::string, > ImportantDomainInfo*> site_map; > On 2017/03/28 14:06:45, dullweber wrote: > > On 2017/03/27 22:22:45, dmurph wrote: > > > unordered_map should be more suitable here -- or even a base::SmallMap< > > > std::unordered_map<string, ImportantDomainInfo*> >. > > > > > > > I changed it to unordered_map. > > > > This map can have up to kMaxImportantSites = 10 entries but is accessed very > > often (for every localstorage and quota entry). > > The default size, up to which SmallMap handles the map differently, is > > kArraySize = 4. > > I'm not sure if SmallMap could have an advantage. > > Then SmallMap will definitely be faster. Iterating a vector with size < 200 is > faster than using map, as iterating is super fast. Also we aren't making > modifications to the map, so there is no vector resizing. Ok, I'm using a SmallMap now. Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
dullweber@chromium.org changed reviewers: + dominickn@chromium.org
dominickn@chromium.org: Please review changes in chrome/browser/engagement/important_sites_util* I added a method to calculate the size of localstorage and quota for each important site entry. It will be used in the important sites dialog for desktop.
https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:46: using content::BrowserThread; Nit: the other files in engagement/ explicitly spell out content::BrowserThread rather than call using, so perhaps do that for consistency. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:156: std::string GetRegisterableDomainOrIPFromHost(const std::string& host) { Would it be cheaper / more clear to just call GetRegisterableDomainOrIP(GURL(host)); instead of having this method? It's almost identical to the former, with the need to call the url::HostIsIPAddress method the only difference. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:364: // domain in |ImportantDomainInfo| and populates |ImportantDomainInfo::usage|. This is a pretty big class (100 lines). It might be worth splitting it into a new file, and unit testing it separately. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:366: public: You should probably document that the receiver will take ownership of |sites| and modify it, so the caller can't use it again. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:367: UsageReceiver(ImportantSitesUtil::UsageCallback done, Nit: call this "callback" or "finish_callback". "done_" looks like a boolean flag. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:376: for (auto& site : sites_) { It's not clear what the type is here, so I'd prefer ImportantDomainInfo& domain_info : sites_ https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:398: dom_storage_context_->GetLocalStorageUsage(base::Bind( I can't find any other place in the codebase which calls this method on the IO thread. I'm fairly sure that this runs in a non-blocking background task, but it would be good to confirm that. :) https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:405: for (const auto& info : usage_infos) { Nit: no braces for 1 line conditional. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:414: for (const auto& info : storage_infos) { Nit: no braces for 1 line conditional. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:422: void CountUsage(const std::string& domain, int64_t size) { Nit: This is more accurately named "IncrementUsage" https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:423: auto it = site_map_.find(domain); It looks to me like the only reason for site_map_ is to provide a map interface to sites_. But you've declared site_map_ as a SmallMap, which is backed by an array anyway. And there will only be max 10 entries too right? Why not just do this using std::find_if and a lambda directly on sites_, and remove site_map_ all together? Saves the extra overhead of the additional map and the weird ownership semantics where the map is holding pointers to things that are in a vector in this object anyway. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:424: if (it != site_map_.end()) { Nit: no braces for a 1 line conditional. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:431: DCHECK(tasks_ > 0); DCHECK_GE(tasks_, 0); https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util_unittest.cc:574: EXPECT_EQ(important_sites.size(), domain_info().size()); Can you add an explicit comment explaining why you expect the usage to be 22 (and what units that is, etc.) https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:329: std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()); auto result = base::MakeUnique<base::DictionaryValue>(); is shorter and clearer (and will get rewritten to C++14 std::make_unique when all our compilers support it).
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
Thanks, I fixed the issues you mentioned and moved the class to a separate file. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:46: using content::BrowserThread; On 2017/04/10 05:06:12, dominickn wrote: > Nit: the other files in engagement/ explicitly spell out content::BrowserThread > rather than call using, so perhaps do that for consistency. Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:156: std::string GetRegisterableDomainOrIPFromHost(const std::string& host) { On 2017/04/10 05:06:12, dominickn wrote: > Would it be cheaper / more clear to just call > GetRegisterableDomainOrIP(GURL(host)); instead of having this method? It's > almost identical to the former, with the need to call the url::HostIsIPAddress > method the only difference. The issue is that I can't create a GURL from hosts because hosts are missing the scheme. I will go the other way round and let the GetRegisterableDomainOrIP function call GetRegisterableDomainOrIPFromHost with gurl.host_piece(). This removes the duplicated code. (And that's what GetDomainAndRegistry() and url.HostIsIPAdress() do anyway.) https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:364: // domain in |ImportantDomainInfo| and populates |ImportantDomainInfo::usage|. On 2017/04/10 05:06:12, dominickn wrote: > This is a pretty big class (100 lines). It might be worth splitting it into a > new file, and unit testing it separately. I moved it a separate file. That cleaned up this file and the unittest but I had to make the GetRegisterableDomainOrIPFrom* public static methods to share them between ImportantSitesUtil and the new class. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:366: public: On 2017/04/10 05:06:12, dominickn wrote: > You should probably document that the receiver will take ownership of |sites| > and modify it, so the caller can't use it again. The |sites| parameter is a std::vector<ImportantDomainInfo> and not a pointer or reference. I use it by moving the vector into this class, which automatically prevents me from using it again. If the caller wouldn't move the vector, then it also shouldn't see the modifications? I think this is a good way to have cheap transfer of the vector but making it clear which place currently owns it. But maybe I'm wrong with my interpretation of how std::move will behave or is what I am doing an uncommon usage of it? https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:367: UsageReceiver(ImportantSitesUtil::UsageCallback done, On 2017/04/10 05:06:12, dominickn wrote: > Nit: call this "callback" or "finish_callback". "done_" looks like a boolean > flag. Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:376: for (auto& site : sites_) { On 2017/04/10 05:06:12, dominickn wrote: > It's not clear what the type is here, so I'd prefer ImportantDomainInfo& > domain_info : sites_ Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:398: dom_storage_context_->GetLocalStorageUsage(base::Bind( On 2017/04/10 05:06:12, dominickn wrote: > I can't find any other place in the codebase which calls this method on the IO > thread. I'm fairly sure that this runs in a non-blocking background task, but it > would be good to confirm that. :) Yes it looks like localstorage should be queried from the UI thread. Thanks! https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:405: for (const auto& info : usage_infos) { On 2017/04/10 05:06:12, dominickn wrote: > Nit: no braces for 1 line conditional. Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:414: for (const auto& info : storage_infos) { On 2017/04/10 05:06:12, dominickn wrote: > Nit: no braces for 1 line conditional. Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:422: void CountUsage(const std::string& domain, int64_t size) { On 2017/04/10 05:06:12, dominickn wrote: > Nit: This is more accurately named "IncrementUsage" Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:423: auto it = site_map_.find(domain); On 2017/04/10 05:06:11, dominickn wrote: > It looks to me like the only reason for site_map_ is to provide a map interface > to sites_. But you've declared site_map_ as a SmallMap, which is backed by an > array anyway. And there will only be max 10 entries too right? > > Why not just do this using std::find_if and a lambda directly on sites_, and > remove site_map_ all together? Saves the extra overhead of the additional map > and the weird ownership semantics where the map is holding pointers to things > that are in a vector in this object anyway. That sounds like a good idea, thanks! https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:424: if (it != site_map_.end()) { On 2017/04/10 05:06:12, dominickn wrote: > Nit: no braces for a 1 line conditional. Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:431: DCHECK(tasks_ > 0); On 2017/04/10 05:06:12, dominickn wrote: > DCHECK_GE(tasks_, 0); Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util_unittest.cc:574: EXPECT_EQ(important_sites.size(), domain_info().size()); On 2017/04/10 05:06:12, dominickn wrote: > Can you add an explicit comment explaining why you expect the usage to be 22 > (and what units that is, etc.) Done. https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:329: std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()); On 2017/04/10 05:06:12, dominickn wrote: > auto result = base::MakeUnique<base::DictionaryValue>(); is shorter and clearer > (and will get rewritten to C++14 std::make_unique when all our compilers support > it). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, this is way nicer! Mostly nits now, though there's also a threading question and a suggestion for reorganisation (and I apologies for missing the last suggestion in the prior review). https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:366: public: On 2017/04/10 13:29:39, dullweber wrote: > On 2017/04/10 05:06:12, dominickn wrote: > > You should probably document that the receiver will take ownership of |sites| > > and modify it, so the caller can't use it again. > > The |sites| parameter is a std::vector<ImportantDomainInfo> and not a pointer or > reference. I use it by moving the vector into this class, which automatically > prevents me from using it again. If the caller wouldn't move the vector, then it > also shouldn't see the modifications? I think this is a good way to have cheap > transfer of the vector but making it clear which place currently owns it. > > But maybe I'm wrong with my interpretation of how std::move will behave or is > what I am doing an uncommon usage of it? D'oh, I misread and thought that param was an rvalue reference. You're right on this one. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:6: Nit: #include <utility> for std::move https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:24: for (ImportantDomainInfo& site : sites_) { Nit: no braces for 1 line conditional https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:56: content::BrowserThread::PostTask( I'm a bit concerned about the thread lifetime here. |usage_infos| is a member on the GetUsageInfoTask within QuotaManager, which lives on the IO thread. I think it's definitely possible for that GetUsageInfoTask to be deleted (since it's on the IO thread) before ReceiveQuotaUsage() is posted and executed on the UI thread. That'll be a use-after-free. For example, the only other use of GetUsageInfo() is chrome/browser/storage/storage_info_fetcher.cc, which copies the usage_infos into a separate vector and posts to the UI thread using the separate vector. That avoids thread lifetime issues. Can you do the same here? I think it's preferable to copy than to block the IO thread with your own processing here. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:65: for (const storage::UsageInfo& info : usage_infos) Nit: this loop needs braces since the body is more than one line. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:75: for (const content::LocalStorageUsageInfo& info : storage_infos) Nit: this one needs braces because the body is 2 lines. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.h (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:34: // of localstorage and quota bytes used. The |sites| vector with populated Nit: Replace last sentence with: |callback| is synchronously invoked on the UI thread with the |sites| vector (populated with usage) as an argument. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:347: (new ImportantSitesUsageCounter( I just saw how this is used. It might be cleaner to: 1. Make the constructor private 2. Add a static method ImportantSitesUsageCounter::GetUsage(<args for constructor>) 3. Make that static method call new ImportantSitesUsageCounter(<args>), then RunAndDestroySelf() 4. Replace the (new ...) call here with ImportantSitesUsageCounter::GetUsage(<args>) here and in the unit test This pattern means that you can't instantiate one of these objects; instead, it's completely self-managed. You can also add a comment that the static method must be invoked on the UI thread. Sorry that I missed this in the earlier review!
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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Thanks for another round of review. I think passing the vector to a callback is safe because the callback takes a copy. I linked the documents that explains this. What do you think? https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:6: On 2017/04/11 00:18:20, dominickn wrote: > Nit: #include <utility> for std::move Done. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:24: for (ImportantDomainInfo& site : sites_) { On 2017/04/11 00:18:20, dominickn wrote: > Nit: no braces for 1 line conditional Done. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:56: content::BrowserThread::PostTask( On 2017/04/11 00:18:20, dominickn wrote: > I'm a bit concerned about the thread lifetime here. |usage_infos| is a member on > the GetUsageInfoTask within QuotaManager, which lives on the IO thread. I think > it's definitely possible for that GetUsageInfoTask to be deleted (since it's on > the IO thread) before ReceiveQuotaUsage() is posted and executed on the UI > thread. That'll be a use-after-free. > > For example, the only other use of GetUsageInfo() is > chrome/browser/storage/storage_info_fetcher.cc, which copies the usage_infos > into a separate vector and posts to the UI thread using the separate vector. > That avoids thread lifetime issues. > > Can you do the same here? I think it's preferable to copy than to block the IO > thread with your own processing here. Thanks for pointing that out. I just read through the callback documentation to better understand how the different kinds of arguments work and I think passing a const vector<>& should be safe because the callback will store a copy of it? https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass... and https://www.chromium.org/developers/design-documents/threading#How_arguments_... https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:65: for (const storage::UsageInfo& info : usage_infos) On 2017/04/11 00:18:20, dominickn wrote: > Nit: this loop needs braces since the body is more than one line. Done. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:75: for (const content::LocalStorageUsageInfo& info : storage_infos) On 2017/04/11 00:18:20, dominickn wrote: > Nit: this one needs braces because the body is 2 lines. Done. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.h (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:34: // of localstorage and quota bytes used. The |sites| vector with populated On 2017/04/11 00:18:20, dominickn wrote: > Nit: Replace last sentence with: > > |callback| is synchronously invoked on the UI thread with the |sites| vector > (populated with usage) as an argument. Done. https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:347: (new ImportantSitesUsageCounter( On 2017/04/11 00:18:20, dominickn wrote: > I just saw how this is used. It might be cleaner to: > > 1. Make the constructor private > > 2. Add a static method ImportantSitesUsageCounter::GetUsage(<args for > constructor>) > > 3. Make that static method call new ImportantSitesUsageCounter(<args>), then > RunAndDestroySelf() > > 4. Replace the (new ...) call here with > ImportantSitesUsageCounter::GetUsage(<args>) here and in the unit test > > This pattern means that you can't instantiate one of these objects; instead, > it's completely self-managed. You can also add a comment that the static method > must be invoked on the UI thread. > > Sorry that I missed this in the earlier review! Yes that sounds better. The class can't be misused this way. Previously I had a static function that did exactly this because the class was in an anonymous namespace.
https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:56: content::BrowserThread::PostTask( On 2017/04/11 09:01:34, dullweber wrote: > On 2017/04/11 00:18:20, dominickn wrote: > > I'm a bit concerned about the thread lifetime here. |usage_infos| is a member > on > > the GetUsageInfoTask within QuotaManager, which lives on the IO thread. I > think > > it's definitely possible for that GetUsageInfoTask to be deleted (since it's > on > > the IO thread) before ReceiveQuotaUsage() is posted and executed on the UI > > thread. That'll be a use-after-free. > > > > For example, the only other use of GetUsageInfo() is > > chrome/browser/storage/storage_info_fetcher.cc, which copies the usage_infos > > into a separate vector and posts to the UI thread using the separate vector. > > That avoids thread lifetime issues. > > > > Can you do the same here? I think it's preferable to copy than to block the IO > > thread with your own processing here. > > Thanks for pointing that out. I just read through the callback documentation to > better understand how the different kinds of arguments work and I think passing > a const vector<>& should be safe because the callback will store a copy of it? > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass... > and > https://www.chromium.org/developers/design-documents/threading#How_arguments_... I don't think that doc applies here because the usage_info argument isn't bound in the callback (i.e. you don't have closure = base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsageOnIOThread, base::Unretained(this), usage_info) followed by closure.Run() somewhere. That would be okay. Instead, it's passed in directly at https://cs.chromium.org/chromium/src/storage/browser/quota/quota_manager.cc?s... (|callback_| is your callback object in there). So this method is directly invoked with a reference to the underlying GetUsageInfoTask's vector, so I think you definitely have a threading lifetime issue here. https://codereview.chromium.org/2752263003/diff/170001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/170001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:7: #include <utility> Nit: style is to have a blank line after the system headers.
https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/140002/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:56: content::BrowserThread::PostTask( On 2017/04/11 10:03:52, dominickn wrote: > On 2017/04/11 09:01:34, dullweber wrote: > > On 2017/04/11 00:18:20, dominickn wrote: > > > I'm a bit concerned about the thread lifetime here. |usage_infos| is a > member > > on > > > the GetUsageInfoTask within QuotaManager, which lives on the IO thread. I > > think > > > it's definitely possible for that GetUsageInfoTask to be deleted (since it's > > on > > > the IO thread) before ReceiveQuotaUsage() is posted and executed on the UI > > > thread. That'll be a use-after-free. > > > > > > For example, the only other use of GetUsageInfo() is > > > chrome/browser/storage/storage_info_fetcher.cc, which copies the usage_infos > > > into a separate vector and posts to the UI thread using the separate vector. > > > That avoids thread lifetime issues. > > > > > > Can you do the same here? I think it's preferable to copy than to block the > IO > > > thread with your own processing here. > > > > Thanks for pointing that out. I just read through the callback documentation > to > > better understand how the different kinds of arguments work and I think > passing > > a const vector<>& should be safe because the callback will store a copy of it? > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass... > > and > > > https://www.chromium.org/developers/design-documents/threading#How_arguments_... > > I don't think that doc applies here because the usage_info argument isn't bound > in the callback (i.e. you don't have closure = > base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsageOnIOThread, > base::Unretained(this), usage_info) followed by closure.Run() somewhere. That > would be okay. > > Instead, it's passed in directly at > https://cs.chromium.org/chromium/src/storage/browser/quota/quota_manager.cc?s... > (|callback_| is your callback object in there). So this method is directly > invoked with a reference to the underlying GetUsageInfoTask's vector, so I think > you definitely have a threading lifetime issue here. I agree that the usage_infos parameter in ReceiveQuotaUsageOnIOThread is a reference to the |entries_| member of GetUsageInfoTask but I think that the vector is copied by using base::Bind() and ReceiveQuotaUsage will get a reference to the copy: base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsage, base::Unretained(this), usage_infos) The GetUsageInfoTask can't disappear while ReceiveQuotaUsageOnIOThread is called because it is a synchronous call and ReceiveQuotaUsageOnIOThread will make the copy using base::Bind().
Oh, I see, you dispatch using base::Bind there. That makes sense, thanks for bearing with me. lgtm % <utility> newline nit (and a rebase).
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: + dschuyler@chromium.org
dschuyler@chromium.org: This is a follow up cl for important sites, adding code to count the amount of storage used by each site. Please review chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.*
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.* lgtm
dullweber@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in chrome/browser/android/browsing_data/browsing_data_bridge.cc. I moved kMaxImportantSites to ImportantSitesUtil because I is use it from other places outside the browsing_data_bridge.
https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:42: void ImportantSitesUsageCounter::RunAndDestroySelf() { This method doesn't immediately destroy the object, so the name is a bit misleading. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:48: base::Bind(&ImportantSitesUsageCounter::GetQuotaUsageOnIOThread, Nit: base::BindOnce() where possible. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:54: Done(); You can set |tasks_| to 0 and leave out this call. You *know* that there is going to be at least one outstanding task at this point, so you don't really need this check. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:112: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); I would just directly delete |this| here. Given that the object is not used anywhere else, you don't need to wait to destroy it, and it makes analyzing the control flow e.g. from a stack trace much easier if you have a direct call. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.h (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:35: // of localstorage and quota bytes used. |callback| is synchronously invoked The callback is called asynchronously, no? https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:42: UsageCallback callback); Nit: Callbacks are usually passed by const reference. Alternatively, you could make the Callback a OnceCallback, which makes it move-only. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:76: int tasks_; DISALLOW_IMPLICIT_CONSTRUCTORS() https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:28: void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } Did clang-format do this? https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:107: auto* dom_storage_context = Can you use an actual type here? https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:115: base::RunLoop().RunUntilIdle(); I would use an explicit quit closure to quit the runloop rather than letting it run idle. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:120: // https://maps.example.com. On top of that it uses 16B localstorage. Super-nit: the name of the feature is "local storage". https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:354: registerable_domain = std::string(host); I would just directly return std::string(host). https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.h (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.h:50: static std::string GetRegisterableDomainOrIP(const GURL& url); Nit: "registrable" is far more common than "registerable". https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:351: base::Unretained(this), callback_id)); What if this object is destroyed before the callback runs? https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:375: auto result = base::MakeUnique<base::DictionaryValue>(); You could even stack-allocate the DictionaryValue.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, I fixed these issues https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:42: void ImportantSitesUsageCounter::RunAndDestroySelf() { On 2017/05/04 14:16:44, Bernhard Bauer wrote: > This method doesn't immediately destroy the object, so the name is a bit > misleading. I changed it to RunAndDestroySelfWhenFinished() https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:48: base::Bind(&ImportantSitesUsageCounter::GetQuotaUsageOnIOThread, On 2017/05/04 14:16:44, Bernhard Bauer wrote: > Nit: base::BindOnce() where possible. I changed to BindOnce where it was accepted https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:54: Done(); On 2017/05/04 14:16:44, Bernhard Bauer wrote: > You can set |tasks_| to 0 and leave out this call. You *know* that there is > going to be at least one outstanding task at this point, so you don't really > need this check. Ok, I removed it. Just to confirm: I can only do this because the first tasks calls the IO thread, so it is impossible to get a synchronous response? https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:112: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); On 2017/05/04 14:16:44, Bernhard Bauer wrote: > I would just directly delete |this| here. Given that the object is not used > anywhere else, you don't need to wait to destroy it, and it makes analyzing the > control flow e.g. from a stack trace much easier if you have a direct call. Done. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.h (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:35: // of localstorage and quota bytes used. |callback| is synchronously invoked On 2017/05/04 14:16:44, Bernhard Bauer wrote: > The callback is called asynchronously, no? Oh, yes. Thanks! https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:42: UsageCallback callback); On 2017/05/04 14:16:44, Bernhard Bauer wrote: > Nit: Callbacks are usually passed by const reference. Alternatively, you could > make the Callback a OnceCallback, which makes it move-only. changed to OnceCallback. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.h:76: int tasks_; On 2017/05/04 14:16:44, Bernhard Bauer wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS() Done. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:28: void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); } On 2017/05/04 14:16:44, Bernhard Bauer wrote: > Did clang-format do this? yes, clang-format seems to like single-line methods. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:107: auto* dom_storage_context = On 2017/05/04 14:16:45, Bernhard Bauer wrote: > Can you use an actual type here? Done. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:115: base::RunLoop().RunUntilIdle(); On 2017/05/04 14:16:45, Bernhard Bauer wrote: > I would use an explicit quit closure to quit the runloop rather than letting it > run idle. changed https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:120: // https://maps.example.com. On top of that it uses 16B localstorage. On 2017/05/04 14:16:44, Bernhard Bauer wrote: > Super-nit: the name of the feature is "local storage". Done. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.cc:354: registerable_domain = std::string(host); On 2017/05/04 14:16:45, Bernhard Bauer wrote: > I would just directly return std::string(host). Done. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_util.h (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_util.h:50: static std::string GetRegisterableDomainOrIP(const GURL& url); On 2017/05/04 14:16:45, Bernhard Bauer wrote: > Nit: "registrable" is far more common than "registerable". "registerable" was already used everywhere in this file, I just moved this function from the private namespace to the class. Is it ok to leave it this way? https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:351: base::Unretained(this), callback_id)); On 2017/05/04 14:16:45, Bernhard Bauer wrote: > What if this object is destroyed before the callback runs? I assumed that it worked because the BrowsingDataRemover and BrowsingDataCounters are also passing a Unretained pointer but Martin just explained that they work differently. I will pass a weak_ptr instead. https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:375: auto result = base::MakeUnique<base::DictionaryValue>(); On 2017/05/04 14:16:45, Bernhard Bauer wrote: > You could even stack-allocate the DictionaryValue. Done.
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...
lgtm https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter.cc (right): https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:42: void ImportantSitesUsageCounter::RunAndDestroySelf() { On 2017/05/05 08:35:27, dullweber wrote: > On 2017/05/04 14:16:44, Bernhard Bauer wrote: > > This method doesn't immediately destroy the object, so the name is a bit > > misleading. > > I changed it to RunAndDestroySelfWhenFinished() FWIW, I would be fine with just Run() too (and a comment that it will destroy itself) :) https://codereview.chromium.org/2752263003/diff/230001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter.cc:54: Done(); On 2017/05/05 08:35:27, dullweber wrote: > On 2017/05/04 14:16:44, Bernhard Bauer wrote: > > You can set |tasks_| to 0 and leave out this call. You *know* that there is > > going to be at least one outstanding task at this point, so you don't really > > need this check. > > Ok, I removed it. Just to confirm: I can only do this because the first tasks > calls the IO thread, so it is impossible to get a synchronous response? Yes, otherwise it would depend on whether or not GetLocalStorageUsage() might run its callback synchronously. If it's run asynchronously, it would be fine again; if it's synchronously, it would probably still work, but it would be reentrant, which tends to be tricky. https://codereview.chromium.org/2752263003/diff/290001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/290001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:71: if (run_loop_) Do you expect not to have a run loop? If this ends up being called before (the first call to) WaitForResult(), the run loop would never be quit. And if it's called before a later call to WaitForResult(), it would quit the previous run loop. What I would do instead is immediately allocate a run loop, and reset it *after* the call to Run(). RunLoop can handle being quit before it's run (the Run() call will immediately return).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752263003/diff/290001/chrome/browser/engagem... File chrome/browser/engagement/important_sites_usage_counter_unittest.cc (right): https://codereview.chromium.org/2752263003/diff/290001/chrome/browser/engagem... chrome/browser/engagement/important_sites_usage_counter_unittest.cc:71: if (run_loop_) On 2017/05/05 09:36:05, Bernhard Bauer wrote: > Do you expect not to have a run loop? If this ends up being called before (the > first call to) WaitForResult(), the run loop would never be quit. And if it's > called before a later call to WaitForResult(), it would quit the previous run > loop. > > What I would do instead is immediately allocate a run loop, and reset it *after* > the call to Run(). RunLoop can handle being quit before it's run (the Run() call > will immediately return). Thanks! That sounds like a much better approach for runloops.
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.
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org, dominickn@chromium.org, dschuyler@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2752263003/#ps330001 (title: "rebase")
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": 330001, "attempt_start_ts": 1495119511549080,
"parent_rev": "c9d6c1bb4234256176f66cc8c61c9878c931950a", "commit_rev":
"4870daad28213d17ea5bcea9de843a13f0006d67"}
Message was sent while issue was closed.
Description was changed from ========== Get the size of local storage and used quota for important sites. BUG=698692 ========== to ========== Get the size of local storage and used quota for important sites. BUG=698692 Review-Url: https://codereview.chromium.org/2752263003 Cr-Commit-Position: refs/heads/master@{#472813} Committed: https://chromium.googlesource.com/chromium/src/+/4870daad28213d17ea5bcea9de84... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:330001) as https://chromium.googlesource.com/chromium/src/+/4870daad28213d17ea5bcea9de84... |
