Implement deletion of storage per site.

Implement deletion of storage per site.

BUG=543635
Committed: https://crrev.com/90db9ac787c8b9b834d91402da2614c99fd52b91
Cr-Commit-Position: refs/heads/master@{#373790}

[Finnur, 2016-02-02 20:27:39]

Description was changed from

==========
Implement deletion of storage per site.

BUG=TBD
==========

to

==========
Implement deletion of storage per site.

BUG=543635
==========

[Finnur, 2016-02-02 21:06:15]

Patchset #1 (id:1) has been deleted

[Finnur, 2016-02-02 21:08:08]

finnur@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Finnur, 2016-02-02 21:08:09]

PTAL.

I noticed when I updated to latest that the layout for the site details page is
all wonky, but I believe that is unrelated to this CL. I'll look into that
tomorrow.

[Finnur, 2016-02-03 12:11:11]

Patchset #2 (id:40001) has been deleted

[Finnur, 2016-02-03 12:16:29]

Ping. PTAL.

(layout issue being wonky was separate issue, see:
https://codereview.chromium.org/1660593003/)

[michaelpg, 2016-02-03 18:39:13]

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:90:
this.storedData_ = '';
Typically we would wait for a callback from C++ to update this, to ensure the UI
reflects what actually happens on the backend (e.g. if the clear fails, the data
should continue being shown).

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:113:
this.currentRoute = {
Is this the same as this.fire('subpage-back')? That may save you from having to
create this.categorySelected.

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:127:
Array.prototype.forEach.call(
see Array.prototype.some

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:129:
function(element) { height += element.offsetHeight; });
with some:
    function(element) { return element.offsetHeight > 0; }
or
    element => (element.offsetHeight > 0)

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/storage_info_fetcher.cc (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
chrome/browser/storage/storage_info_fetcher.cc:38:
base::Bind(&StorageInfoFetcher::OnUsageCleared,
unindent

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/storage_info_fetcher.h (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
chrome/browser/storage/storage_info_fetcher.h:22: virtual void
OnUsageInfoCleared() = 0;
If this isn't used yet, add it in a later patch when you need it.

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:86:
storage_info_fetcher_ = new StorageInfoFetcher(quota_manager);
Why is it necessary to re-initialize this if it already exists? I'm wondering if
unsetting the scoped_refptr here or above may cause the observer from a previous
call to these functions to not trigger.

[Finnur, 2016-02-04 14:56:58]

All addressed, I believe. PTAL.

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:90:
this.storedData_ = '';
On 2016/02/03 18:39:13, michaelpg wrote:
> Typically we would wait for a callback from C++ to update this, to ensure the
UI
> reflects what actually happens on the backend (e.g. if the clear fails, the
data
> should continue being shown).

Done.

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:113:
this.currentRoute = {
That was actually the first thing I tried, but realized it doesn't work due to
the way routes are constructed. However, I realized how to overcome that (by
adding the routes) and managed a nice cleanup in the process (privacy_page.js no
longer handling navigation for one of its subpage and some plumbing to go with
it removed also).

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:127:
Array.prototype.forEach.call(
Hey, neat!

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:129:
function(element) { height += element.offsetHeight; });
Hmmm... is there a benefit to the latter syntax? I find it much less readable
than the first...

BTW - what is this operator called? Searching for 'javascript =>' is not very
fruitful. :)

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/storage_info_fetcher.cc (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
chrome/browser/storage/storage_info_fetcher.cc:38:
base::Bind(&StorageInfoFetcher::OnUsageCleared,
On 2016/02/03 18:39:13, michaelpg wrote:
> unindent

Done.

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/storage_info_fetcher.h (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/storage/...
chrome/browser/storage/storage_info_fetcher.h:22: virtual void
OnUsageInfoCleared() = 0;
It is used in the latest version of this patch.

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:86:
storage_info_fetcher_ = new StorageInfoFetcher(quota_manager);
I suppose it isn't necessary. I do have an upcoming CL where I'm aiming to
simplify the usage of the fetcher class somewhat; I think it will improve this a
bit, but I don't think we need to reset the fetcher between requests.

[michaelpg, 2016-02-04 19:18:01]

lgtm

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:113:
this.currentRoute = {
On 2016/02/04 14:56:58, Finnur wrote:
> That was actually the first thing I tried, but realized it doesn't work due to
> the way routes are constructed. However, I realized how to overcome that (by
> adding the routes) and managed a nice cleanup in the process (privacy_page.js
no
> longer handling navigation for one of its subpage and some plumbing to go with
> it removed also).

Awesome!!

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:129:
function(element) { height += element.offsetHeight; });
On 2016/02/04 14:56:58, Finnur wrote:
> Hmmm... is there a benefit to the latter syntax? I find it much less readable
> than the first...
> 
> BTW - what is this operator called? Searching for 'javascript =>' is not very
> fruitful. :)

It's called an arrow function. Relatively new, no obligation to use it.
Sometimes it makes things easier to read because it's shorter and can be used
without a return statement kind of like a "lambda".

https://codereview.chromium.org/1661533002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/website_usage_private_api.js
(right):

https://codereview.chromium.org/1661533002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/website_usage_private_api.js:38:
/** @param {string} origin */
add type param

https://codereview.chromium.org/1661533002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/website_usage_private_api.js:43:
notifyUsageDeleted: function(origin) {
nit: @param

[michaelpg, 2016-02-05 01:32:22]

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/1661533002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:129:
function(element) { height += element.offsetHeight; });
On 2016/02/04 19:18:01, michaelpg wrote:
> On 2016/02/04 14:56:58, Finnur wrote:
> > Hmmm... is there a benefit to the latter syntax? I find it much less
readable
> > than the first...
> > 
> > BTW - what is this operator called? Searching for 'javascript =>' is not
very
> > fruitful. :)
> 
> It's called an arrow function. Relatively new, no obligation to use it.
> Sometimes it makes things easier to read because it's shorter and can be used
> without a return statement kind of like a "lambda".

btw, arrow functions will break the presubmit linter and it would be difficult
to fix this. so for now, probably best to avoid them.

[Finnur, 2016-02-05 10:21:59]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-02-05 10:22:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/1661533002/#ps120001
(title: "Address nits")

[commit-bot: I haz the power, 2016-02-05 10:22:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1661533002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1661533002/120001

[Finnur, 2016-02-05 10:22:42]

I've made a quick fix for the test and am checking in. Basically, I just created
a dummy currentRoute object. Happy to follow-up if there's a better way.

[commit-bot: I haz the power, 2016-02-05 12:01:13]

Description was changed from

==========
Implement deletion of storage per site.

BUG=543635
==========

to

==========
Implement deletion of storage per site.

BUG=543635
==========

[commit-bot: I haz the power, 2016-02-05 12:01:15]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-02-05 12:01:56]

Description was changed from

==========
Implement deletion of storage per site.

BUG=543635
==========

to

==========
Implement deletion of storage per site.

BUG=543635

Committed: https://crrev.com/90db9ac787c8b9b834d91402da2614c99fd52b91
Cr-Commit-Position: refs/heads/master@{#373790}
==========

[commit-bot: I haz the power, 2016-02-05 12:01:57]

Patchset 5 (id:??) landed as
https://crrev.com/90db9ac787c8b9b834d91402da2614c99fd52b91
Cr-Commit-Position: refs/heads/master@{#373790}