Adding usage and quota entry to chrome://settings/cookies.

Adding usage entry to chrome://settings/cookies.


BUG=88644, 91816, 91836
TEST='BrowsingDataQuotaHelperTest.*'


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95607
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=95779

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95959

[tzik, 2011-07-25 04:40:58]

Hi.

As we mailed some of you, we are adding Size column to the cookies tree view in
chrome://settings/cookies.
Screenshots are available on http://crbug.com/88644 as attached file.

Could you review it and give some advice to me?

mdm:  chrome/browser/resources/options/*
kinuko: webkit/quota/*, chrome/browser/browsing_data_quota_helper*
estade: others

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/cookies_tree...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/cookies_tree...
chrome/browser/cookies_tree_model.cc:1011:
root->GetOrCreateOriginNode(GURL("http://" + quota_info->host));
CookiesTreeOriginNode looks not fully host-based, but we have only hostname
around quota system.
Can we safely use "http://" + hostname as origin?

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_view.html (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_view.html:9: </div>
The remove button and search box hide cookies-size-column, so I pushed below the
cookies tree.
But there seems to be better way to place them.

[kinuko, 2011-07-25 13:22:41]

A few initial comments... (haven't looked closely)

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.h (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:35: // QuotaInfo contains host-based
quota and usage informations for persistent
nit: should this comment be moved at the definition of QuotaInfo (i.e. between
line 40-41)?

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:39: BrowserThread::DeleteOnIOThread>
{
As we chatted locally, we should either 1) use MessageLoopProxy to delete
instance on the correct thread, or 2) use BrowserThread in the testing code too.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:69: virtual
~BrowsingDataQuotaHelper() { }
no space between { } ?

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:80: bool operator ==(const
BrowsingDataQuotaHelper::QuotaInfo& lhs,
Is this really necessary?  (Just asking)

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.cc (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:130: quota::StorageType type)
{
nit: weird indentation

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:140: int64 usage) {
ditto

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:196: const
BrowsingDataQuotaHelper::QuotaInfo& rhs) {
nit: indent

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_unittest.cc (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:25: void SetUp() {
virtual?

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:34:
base::MessageLoopProxy::CreateForCurrentThread(), /* io_thread */
not quite sure if we should rather create UI/IO/DB BrowserThreads for testing as
well.  (Probably the current code is ok)

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:38: void TearDown() {
virtual?

[tzik, 2011-07-26 10:52:41]

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.h (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:35: // QuotaInfo contains host-based
quota and usage informations for persistent
On 2011/07/25 13:22:41, kinuko wrote:
> nit: should this comment be moved at the definition of QuotaInfo (i.e. between
> line 40-41)?

Done.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:39: BrowserThread::DeleteOnIOThread>
{
On 2011/07/25 13:22:41, kinuko wrote:
> As we chatted locally, we should either 1) use MessageLoopProxy to delete
> instance on the correct thread, or 2) use BrowserThread in the testing code
too.

Done.
I modified them as 1).
We can easily switch single/multi thread style in unittest with 1).

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:69: virtual
~BrowsingDataQuotaHelper() { }
On 2011/07/25 13:22:41, kinuko wrote:
> no space between { } ?

Done.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:80: bool operator ==(const
BrowsingDataQuotaHelper::QuotaInfo& lhs,
On 2011/07/25 13:22:41, kinuko wrote:
> Is this really necessary?  (Just asking)

We need it for
browsing_data_quota_helper_unittest.cc:111: EXPECT_TRUE(expected == actual);

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.cc (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:130: quota::StorageType type)
{
On 2011/07/25 13:22:41, kinuko wrote:
> nit: weird indentation

Done.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:140: int64 usage) {
On 2011/07/25 13:22:41, kinuko wrote:
> ditto

Done.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:196: const
BrowsingDataQuotaHelper::QuotaInfo& rhs) {
On 2011/07/25 13:22:41, kinuko wrote:
> nit: indent

Done.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_unittest.cc (right):

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:25: void SetUp() {
On 2011/07/25 13:22:41, kinuko wrote:
> virtual?

Done.

http://codereview.chromium.org/7387007/diff/18006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:38: void TearDown() {
On 2011/07/25 13:22:41, kinuko wrote:
> virtual?

Done.

[Evan Stade, 2011-07-26 17:03:22]

Mike, will you have time to look at this?

[Mike Mammarella, 2011-07-26 21:46:31]

Sorry for the delay. I looked at the resources/options code as well as glanced
at some of the handler code (though estade should look at that code also). My
main comment is that I don't very much like how it looks, but I also don't have
any suggestion to improve it.

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_list.js (right):

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:239: this.sizeChild.textContent
= info.quota.totalUsage;
It looks odd to only have the size column filled in when there is a size, since
apparently cookies don't have a size. So most of the rows don't have anything
here. Could we get a string for that, something like "N/A" or "negligible" so it
doesn't look broken?

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:435: info.quota = {
For a multiline statement like this, you'd need to add {} to this else, and
therefore also to all the other cases as well. Why not just info.quota =
this.data; though?

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:473: if (!text)
What is this for?

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_view.html (right):

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_view.html:3: <div
id="remove-all-cookies-search-column">
So, I don't have a better suggestion, but I don't particularly like how this
looks. Do we have some UI lead suggestions here, and has some UI lead looked at
this?

[Mike Mammarella, 2011-07-26 22:24:03]

Actually I do have one suggestion. What if instead of actually including the
"Data Size" column header you just left the search box there? The data size
field doesn't show up on most rows anyway, so the net effect would be to add it
only on a few rows that have actual data. The presence of units (MB) should
hopefully make it obvious what it means when it shows up.

[tzik, 2011-07-27 03:51:47]

arv, could you give me some advice about where to place these UI parts (Remove
button, search box and Data Size column header in chrome://settings/cookies), or
could you redirect this to some UI people?

On 2011/07/26 22:24:03, Mike Mammarella wrote:
> Actually I do have one suggestion. What if instead of actually including the
> "Data Size" column header you just left the search box there? The data size
> field doesn't show up on most rows anyway, so the net effect would be to add
it
> only on a few rows that have actual data. The presence of units (MB) should
> hopefully make it obvious what it means when it shows up.

That looks good for current change.
But, after this change be landed, we'll make another charge to add "Sort by
size", and maybe ratio of usage over quota.
So now or later, adding column header seems hard to avoid.

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_list.js (right):

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:239: this.sizeChild.textContent
= info.quota.totalUsage;
On 2011/07/26 21:46:31, Mike Mammarella wrote:
> It looks odd to only have the size column filled in when there is a size,
since
> apparently cookies don't have a size. So most of the rows don't have anything
> here. Could we get a string for that, something like "N/A" or "negligible" so
it
> doesn't look broken?

Done.

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:435: info.quota = {
On 2011/07/26 21:46:31, Mike Mammarella wrote:
> For a multiline statement like this, you'd need to add {} to this else, and
> therefore also to all the other cases as well. Why not just info.quota =
> this.data; though?

Done.

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:473: if (!text)
On 2011/07/26 21:46:31, Mike Mammarella wrote:
> What is this for?

I want not to allocate bubble for quota since quota is not a storage, so I
didn't allocate label for quota and skip unlabeled bubble in this line.

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_view.html (right):

http://codereview.chromium.org/7387007/diff/13006/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_view.html:3: <div
id="remove-all-cookies-search-column">
On 2011/07/26 21:46:31, Mike Mammarella wrote:
> So, I don't have a better suggestion, but I don't particularly like how this
> looks. Do we have some UI lead suggestions here, and has some UI lead looked
at
> this?

No, we don't have yet.

[Evan Stade, 2011-07-27 16:33:15]

arv is on sabbatical. Try ainslie or alcor for the UI advice

[tzik, 2011-07-29 07:16:17]

Thanks for your suggestion, estade.
But, seeing past discussion for this UI and new UI plan around Storage,
I feel it better to leave the UI minimal with no Data Size column header, as mdm
said, in this changelist.

I update the screenshot of this changeset, available on http://crbug.com/88644 .
How about this changeset?

[Mike Mammarella, 2011-07-29 17:59:26]

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_list.js (right):

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:242:
localStrings.getString('usage_unavailable');
Without the header it might be better to leave this off, actually. But thanks
for adding it so we could see how it looks!

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/ui/webui/coo...
File chrome/browser/ui/webui/cookies_tree_model_util.cc (right):

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/ui/webui/coo...
chrome/browser/ui/webui/cookies_tree_model_util.cc:205:
dict->SetString(kKeyTotalUsage,
In the example screenshot you have some data that's only measured in bytes, so
it shows up as B. I wonder if that's small enough that without the column header
it won't be obvious what it means; KB and MB are more recognizable as sizes.
Maybe if it's less than 1 KB we should omit it?

[tzik, 2011-08-01 02:46:26]

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/resources/op...
File chrome/browser/resources/options/cookies_list.js (right):

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/resources/op...
chrome/browser/resources/options/cookies_list.js:242:
localStrings.getString('usage_unavailable');
On 2011/07/29 17:59:26, Mike Mammarella wrote:
> Without the header it might be better to leave this off, actually. But thanks
> for adding it so we could see how it looks!

Done.

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/ui/webui/coo...
File chrome/browser/ui/webui/cookies_tree_model_util.cc (right):

http://codereview.chromium.org/7387007/diff/28009/chrome/browser/ui/webui/coo...
chrome/browser/ui/webui/cookies_tree_model_util.cc:205:
dict->SetString(kKeyTotalUsage,
On 2011/07/29 17:59:26, Mike Mammarella wrote:
> In the example screenshot you have some data that's only measured in bytes, so
> it shows up as B. I wonder if that's small enough that without the column
header
> it won't be obvious what it means; KB and MB are more recognizable as sizes.
> Maybe if it's less than 1 KB we should omit it?

Done.

[Mike Mammarella, 2011-08-01 18:53:47]

chrome/browser/resources/options and general UI appearance LGTM, didn't notice
any issues with other code but other reviewers should look at that as well

[tzik, 2011-08-02 08:20:47]

On 2011/08/01 18:53:47, Mike Mammarella wrote:
> chrome/browser/resources/options and general UI appearance LGTM, didn't notice
> any issues with other code but other reviewers should look at that as well

Thank you for reviewing.

Then, how about you, kinuko and estade?

[kinuko, 2011-08-02 09:09:30]

looks good, mostly nits.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.cc (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.cc:9: persistent_usage(0){ }
nit: space between ){, no space between { } (here and elsewhere)

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.h (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:45: // QuotaInfo contains host-based
quota and usage informations for persistent
nit: s/informations/information/ would be better I think :) (usually it's
uncountable)

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:69: virtual void
DeleteQuotaHost(const std::string& host) { }
nit: no space between { }

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.cc (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:70: const std::set<GURL>&
origins) {
A heads-up; depends on whether this change is submitted or not:
http://codereview.chromium.org/7533013/
you may need to add 'type' parameter to this method.

(...and then you may want to merge or drop these two methods in favor of single
GotOrigins method.)

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.h (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.h:49: // Callback function for
GetHostUsage
style-nit: end comments with period.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_unittest.cc (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:52: bool
fetching_completed() {
nit: could be a const method?

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/ui/webui/coo...
File chrome/browser/ui/webui/cookies_tree_model_util.h (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/ui/webui/coo...
chrome/browser/ui/webui/cookies_tree_model_util.h:24: // Returns false if the
|node| need not to send.
nit: ... if the |node| does not need to be shown. ?

http://codereview.chromium.org/7387007/diff/37002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7387007/diff/37002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:36: void NotifyAllOrigins();
nit: please move this method above the comment '// To add or...' or below the
two methods (AddOrigin.. and ModifyOrigin..).  If you reorder the methods please
reorder the methods body in .cc file too.

naming-nit: since this method simulates 'touch' on all origins how about naming
this TouchAllOriginsAndNotify?

[Evan Stade, 2011-08-02 17:07:44]

mdm's LGTM stands for me (consider this a rubber stamp)

-- Evan Stade


On Tue, Aug 2, 2011 at 2:09 AM, <kinuko@chromium.org> wrote:

> looks good, mostly nits.
>
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper.cc<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper.cc>
> File chrome/browser/browsing_data_**quota_helper.cc (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper.cc#newcode9<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper.cc#newcode9>
> chrome/browser/browsing_data_**quota_helper.cc:9: persistent_usage(0){ }
> nit: space between ){, no space between { } (here and elsewhere)
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper.h<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper.h>
>
> File chrome/browser/browsing_data_**quota_helper.h (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper.h#newcode45<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper.h#newcode45>
> chrome/browser/browsing_data_**quota_helper.h:45: // QuotaInfo contains
>
> host-based quota and usage informations for persistent
> nit: s/informations/information/ would be better I think :) (usually
> it's uncountable)
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper.h#newcode69<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper.h#newcode69>
> chrome/browser/browsing_data_**quota_helper.h:69: virtual void
> DeleteQuotaHost(const std::string& host) { }
> nit: no space between { }
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper_impl.cc<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper_impl.cc>
>
> File chrome/browser/browsing_data_**quota_helper_impl.cc (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper_impl.cc#newcode70<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper_impl.cc#newcode70>
> chrome/browser/browsing_data_**quota_helper_impl.cc:70: const
> std::set<GURL>& origins) {
> A heads-up; depends on whether this change is submitted or not:
>
http://codereview.chromium.**org/7533013/<http://codereview.chromium.org/7533...
> you may need to add 'type' parameter to this method.
>
> (...and then you may want to merge or drop these two methods in favor of
> single GotOrigins method.)
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper_impl.h<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper_impl.h>
> File chrome/browser/browsing_data_**quota_helper_impl.h (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper_impl.h#newcode49<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper_impl.h#newcode49>
> chrome/browser/browsing_data_**quota_helper_impl.h:49: // Callback
> function for GetHostUsage
> style-nit: end comments with period.
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper_unittest.cc<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper_unittest.cc>
>
> File chrome/browser/browsing_data_**quota_helper_unittest.cc (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/browsing_data_quota_**helper_unittest.cc#newcode52<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_data_quota_helper_unittest.cc#newcode52>
> chrome/browser/browsing_data_**quota_helper_unittest.cc:52: bool
> fetching_completed() {
> nit: could be a const method?
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/ui/webui/cookies_tree_**model_util.h<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/ui/webui/cookies_tree_model_util.h>
> File chrome/browser/ui/webui/**cookies_tree_model_util.h (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/chrome/**
>
browser/ui/webui/cookies_tree_**model_util.h#newcode24<http://codereview.chromium.org/7387007/diff/37002/chrome/browser/ui/webui/cookies_tree_model_util.h#newcode24>
> chrome/browser/ui/webui/**cookies_tree_model_util.h:24: // Returns false
> if the |node| need not to send.
> nit: ... if the |node| does not need to be shown. ?
>
> http://codereview.chromium.**org/7387007/diff/37002/webkit/**
>
quota/mock_storage_client.h<http://codereview.chromium.org/7387007/diff/37002/webkit/quota/mock_storage_client.h>
> File webkit/quota/mock_storage_**client.h (right):
>
> http://codereview.chromium.**org/7387007/diff/37002/webkit/**
>
quota/mock_storage_client.h#**newcode36<http://codereview.chromium.org/7387007/diff/37002/webkit/quota/mock_storage_client.h#newcode36>
> webkit/quota/mock_storage_**client.h:36: void NotifyAllOrigins();
> nit: please move this method above the comment '// To add or...' or
> below the two methods (AddOrigin.. and ModifyOrigin..).  If you reorder
> the methods please reorder the methods body in .cc file too.
>
> naming-nit: since this method simulates 'touch' on all origins how about
> naming this TouchAllOriginsAndNotify?
>
>
>
http://codereview.chromium.**org/7387007/<http://codereview.chromium.org/7387...
>

[tzik, 2011-08-03 04:15:00]

Thanks you all for reviewing.

If there looks no problem, I'll check commit checkbox after 7533013 landed.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.cc (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.cc:9: persistent_usage(0){ }
On 2011/08/02 09:09:30, kinuko wrote:
> nit: space between ){, no space between { } (here and elsewhere)

Done.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.h (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:45: // QuotaInfo contains host-based
quota and usage informations for persistent
On 2011/08/02 09:09:30, kinuko wrote:
> nit: s/informations/information/ would be better I think :) (usually it's
> uncountable)

Done.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:69: virtual void
DeleteQuotaHost(const std::string& host) { }
On 2011/08/02 09:09:30, kinuko wrote:
> nit: no space between { }

Done.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.cc (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:70: const std::set<GURL>&
origins) {
On 2011/08/02 09:09:30, kinuko wrote:
> A heads-up; depends on whether this change is submitted or not:
> http://codereview.chromium.org/7533013/
> you may need to add 'type' parameter to this method.
> 
> (...and then you may want to merge or drop these two methods in favor of
single
> GotOrigins method.)

Done.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.h (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.h:49: // Callback function for
GetHostUsage
On 2011/08/02 09:09:30, kinuko wrote:
> style-nit: end comments with period.

Done.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_unittest.cc (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_unittest.cc:52: bool
fetching_completed() {
On 2011/08/02 09:09:30, kinuko wrote:
> nit: could be a const method?

Done.

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/ui/webui/coo...
File chrome/browser/ui/webui/cookies_tree_model_util.h (right):

http://codereview.chromium.org/7387007/diff/37002/chrome/browser/ui/webui/coo...
chrome/browser/ui/webui/cookies_tree_model_util.h:24: // Returns false if the
|node| need not to send.
On 2011/08/02 09:09:30, kinuko wrote:
> nit: ... if the |node| does not need to be shown. ?

Done.

http://codereview.chromium.org/7387007/diff/37002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7387007/diff/37002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:36: void NotifyAllOrigins();
On 2011/08/02 09:09:30, kinuko wrote:
> nit: please move this method above the comment '// To add or...' or below the
> two methods (AddOrigin.. and ModifyOrigin..).  If you reorder the methods
please
> reorder the methods body in .cc file too.
> 
> naming-nit: since this method simulates 'touch' on all origins how about
naming
> this TouchAllOriginsAndNotify?

Done.

[tzik, 2011-08-03 04:17:34]

On 2011/08/03 04:15:00, tzik wrote:
> Thanks you all for reviewing.
> 
> If there looks no problem, I'll check commit checkbox after 7533013 landed.
> 

oops, I forgot.
I have to complete be reviewed by kinuko-san.

[kinuko, 2011-08-03 06:20:35]

lgtm

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.h (right):

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:60: typedef std::vector<QuotaInfo>
QuotaInfoList;
super-nit: List would make people think it's a list... maybe xxxVector or Array?

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.cc (right):

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:85: } else {  // type ==
quota::kStorageTypePersistent
nit: mind moving this comment to the next line?

[tzik, 2011-08-03 06:52:04]

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper.h (right):

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper.h:60: typedef std::vector<QuotaInfo>
QuotaInfoList;
On 2011/08/03 06:20:35, kinuko wrote:
> super-nit: List would make people think it's a list... maybe xxxVector or
Array?

Done.

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_quota_helper_impl.cc (right):

http://codereview.chromium.org/7387007/diff/52001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_quota_helper_impl.cc:85: } else {  // type ==
quota::kStorageTypePersistent
On 2011/08/03 06:20:35, kinuko wrote:
> nit: mind moving this comment to the next line?

Done.

[commit-bot: I haz the power, 2011-08-04 03:56:16]

Try job failure for 7387007-51005 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-08-05 13:01:52]

Change committed as 95607

[tzik, 2011-08-08 06:53:27]

Sorry, I made leaks in the previous patchset.
I had to do MessageLoop::RunAllPending() to delete a instance of a class that
inherits RefCounted<*, DeleteOn*Thread>.

I added RunAllPending() and removed suppression of this leaks.
Could you review this update?

kinuko:
chrome/browser/{cookies_tree_model_unittest.cc,browsing_data_quota_helper_unittest.cc}
timurrrr: tools/{valgrind,heapcheck}/suppressions.txt

[Timur Iskhodzhanov, 2011-08-08 07:05:59]

Maybe you should create a separate codereview issue?

LGTM for the suppressions
and consider testing with Valgrind or Heapchecker locally before commiting next
time.

On 2011/08/08 06:53:27, tzik wrote:
> Sorry, I made leaks in the previous patchset.
> I had to do MessageLoop::RunAllPending() to delete a instance of a class that
> inherits RefCounted<*, DeleteOn*Thread>.
> 
> I added RunAllPending() and removed suppression of this leaks.
> Could you review this update?
> 
> kinuko:
>
chrome/browser/{cookies_tree_model_unittest.cc,browsing_data_quota_helper_unittest.cc}
> timurrrr: tools/{valgrind,heapcheck}/suppressions.txt

[kinuko, 2011-08-08 08:52:30]

On 2011/08/08 06:53:27, tzik wrote:
> Sorry, I made leaks in the previous patchset.
> I had to do MessageLoop::RunAllPending() to delete a instance of a class that
> inherits RefCounted<*, DeleteOn*Thread>.
> 
> I added RunAllPending() and removed suppression of this leaks.
> Could you review this update?
> 
> kinuko:
>
chrome/browser/{cookies_tree_model_unittest.cc,browsing_data_quota_helper_unittest.cc}
> timurrrr: tools/{valgrind,heapcheck}/suppressions.txt

LGTM

[tzik, 2011-08-08 13:32:38]

Thank you for reviewing.

> Maybe you should create a separate codereview issue?
I'll do it next time. (I hope no chance to do it.)

On 2011/08/08 07:05:59, Timur Iskhodzhanov wrote:
> Maybe you should create a separate codereview issue?
> 
> LGTM for the suppressions
> and consider testing with Valgrind or Heapchecker locally before commiting
next
> time.
> 
> On 2011/08/08 06:53:27, tzik wrote:
> > Sorry, I made leaks in the previous patchset.
> > I had to do MessageLoop::RunAllPending() to delete a instance of a class
that
> > inherits RefCounted<*, DeleteOn*Thread>.
> > 
> > I added RunAllPending() and removed suppression of this leaks.
> > Could you review this update?
> > 
> > kinuko:
> >
>
chrome/browser/{cookies_tree_model_unittest.cc,browsing_data_quota_helper_unittest.cc}
> > timurrrr: tools/{valgrind,heapcheck}/suppressions.txt

[commit-bot: I haz the power, 2011-08-08 21:07:38]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac, revision is 95864, job name
was 7387007-17028 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-09 06:48:33]

Change committed as 95959