Show local storage usage on the Website Settings options page.

Show local storage usage on the Website Settings options page.

BUG=372607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285045

[Daniel Nishi, 2014-07-18 20:29:30]

bauerb: Would you mind taking a look at this patch, or re-assigning if you would
rather someone else did?

Thanks!

[Bernhard Bauer, 2014-07-21 11:29:30]

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/origin_resources_list.js:17: else
If you have braces for the if clause, also use braces for else.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:101: for (var origin in
originDict) {
Consider using Object.keys(originDict).map(...);

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:103: usage:
originDict[origin]['usage'],
Use .usage?

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:130:
WebsiteSettingsManager.updateLocalStorage = function(originDict) {
Call this method also populateLocalStorage()?

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:69:
content_settings::GetTypeFromName(content_setting_name, &content_type);
Can we add some more checks in here? I would like to avoid that a nonsensical
content type sneaks into |last_setting_| and then causes an error much later on.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:82: if
(last_setting_ == "storage")
Can you extract "storage" to a constant?

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:83:
UpdateLocalStorage(filter);
So, this means that every time the filter string changes (and it changes for
every typed character), we will fetch all local storage items again? That seems
like a recipe for jank. We will probably need to cache the results somewhere
(see also my comment below about a container for browsing data).

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:90: if
(!local_storage_.get()) {
.get() is not necessary; scoped_refptr (and scoped_ptr) has an implicit coercion
to bool that will Do The Right Thing.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:92: local_storage_ =
(new BrowsingDataLocalStorageHelper(profile));
Parentheses are not necessary.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:97:
UpdateLocalStorage(std::string());
What if I had previously typed something into the filter box?

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:110: std::string
origin = (*it).origin_url.spec();
This can be `it->...`.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:119: "usage_string",
new base::StringValue(ui::FormatBytes((*it).size)));
Nit: Javascript properties use lowercasedCamelCase, not unix_hacker_style.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:162:
base::Unretained(this),
Uh-oh... |local_storage_| might call this callback at any point, including when
the WebUI has already been torn down. It's a bit unfortunate that all the
browsing data helpers are refcounted with no way to cancel them, but as long as
that's the case, you'll probably need to use a weak pointer here.

When you're adding more browsing data helpers here, it might make sense to add
some container that will hold their scoped_refptrs and be the callback target;
then you can use weak pointers to that instead of the whole handler, and have
the handler simply own it. Basically, something like LocalDataContainer, except
without a CookiesTreeModel[*]. 

[*] You could probably even use CookiesTreeModel, but it's a bit awkward, as it
dates from the pre-WebUI version of the cookies dialog, which actually was,
y'know, a tree view, and its recursive structure doesn't really fit in well with
an origin list.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/website_settings_handler.h (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.h:44: void
OnLocalStorageFetch(
Nit: OnLocalStoragedFetched?

[Daniel Nishi, 2014-07-21 17:53:37]

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/origin_resources_list.js:17: else
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> If you have braces for the if clause, also use braces for else.

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:101: for (var origin in
originDict) {
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> Consider using Object.keys(originDict).map(...);

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:103: usage:
originDict[origin]['usage'],
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> Use .usage?

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:130:
WebsiteSettingsManager.updateLocalStorage = function(originDict) {
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> Call this method also populateLocalStorage()?

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:69:
content_settings::GetTypeFromName(content_setting_name, &content_type);
On 2014/07/21 11:29:30, Bernhard Bauer wrote:
> Can we add some more checks in here? I would like to avoid that a nonsensical
> content type sneaks into |last_setting_| and then causes an error much later
on.

I added a DCHECK that GetTypeFromName got a valid type back and a DCHECK to
ensure that it was a ContentType we can handle.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:82: if
(last_setting_ == "storage")
On 2014/07/21 11:29:30, Bernhard Bauer wrote:
> Can you extract "storage" to a constant?

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:83:
UpdateLocalStorage(filter);
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> So, this means that every time the filter string changes (and it changes for
> every typed character), we will fetch all local storage items again? That
seems
> like a recipe for jank. We will probably need to cache the results somewhere
> (see also my comment below about a container for browsing data).

I've changed the implementation to cache the local storage data when you select
the category and to use the cached list after that.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:90: if
(!local_storage_.get()) {
On 2014/07/21 11:29:30, Bernhard Bauer wrote:
> .get() is not necessary; scoped_refptr (and scoped_ptr) has an implicit
coercion
> to bool that will Do The Right Thing.

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:92: local_storage_ =
(new BrowsingDataLocalStorageHelper(profile));
On 2014/07/21 11:29:30, Bernhard Bauer wrote:
> Parentheses are not necessary.

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:97:
UpdateLocalStorage(std::string());
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> What if I had previously typed something into the filter box?

Last query now stored and reused.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:110: std::string
origin = (*it).origin_url.spec();
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> This can be `it->...`.

Done and replaced in other places where I used (*it) instead.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:119: "usage_string",
new base::StringValue(ui::FormatBytes((*it).size)));
On 2014/07/21 11:29:30, Bernhard Bauer wrote:
> Nit: Javascript properties use lowercasedCamelCase, not unix_hacker_style. 

Done.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:162:
base::Unretained(this),
On 2014/07/21 11:29:29, Bernhard Bauer wrote:
> Uh-oh... |local_storage_| might call this callback at any point, including
when
> the WebUI has already been torn down. It's a bit unfortunate that all the
> browsing data helpers are refcounted with no way to cancel them, but as long
as
> that's the case, you'll probably need to use a weak pointer here.
> 
> When you're adding more browsing data helpers here, it might make sense to add
> some container that will hold their scoped_refptrs and be the callback target;
> then you can use weak pointers to that instead of the whole handler, and have
> the handler simply own it. Basically, something like LocalDataContainer,
except
> without a CookiesTreeModel[*]. 
> 
> [*] You could probably even use CookiesTreeModel, but it's a bit awkward, as
it
> dates from the pre-WebUI version of the cookies dialog, which actually was,
> y'know, a tree view, and its recursive structure doesn't really fit in well
with
> an origin list.

I have it using a weak_ptr now. If another browsing data helper gets used, I'll
pull out the LocalDataContainer's dependency on CookiesTreeModel and work from
there, I think.

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/website_settings_handler.h (right):

https://codereview.chromium.org/408493003/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.h:44: void
OnLocalStorageFetch(
On 2014/07/21 11:29:30, Bernhard Bauer wrote:
> Nit: OnLocalStoragedFetched?

Done.

[Bernhard Bauer, 2014-07-22 08:51:48]

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:17: else {
Else goes on the same line as the closing brace.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:101:
Object.keys(originDict).map(function(origin) {
map() will return a list containing the return values of the function, so you
can assign that directly to origins. What you wrote here would be forEach().

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:104: usage_string:
originDict[origin].usageString});
Make this usageString as well.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:76:
std::end(kValidTypes),
Can you run some tryjobs for this, in particular on Windows? IIRC, VC++ had some
trouble with using end iterators in DCHECK comparisons, because DCHECK will
stringify the compared values, and that won't work for an iterator that points
nowhere.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:147: void
WebsiteSettingsHandler::UpdateLocalStorage(const std::string& filter) {
You don't need to pass the filter as an argument now, because you have it as
|last_filter_|.

[Daniel Nishi, 2014-07-22 22:30:57]

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/origin_resources_list.js (right):

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/origin_resources_list.js:17: else {
On 2014/07/22 08:51:47, Bernhard Bauer wrote:
> Else goes on the same line as the closing brace.

Done.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:101:
Object.keys(originDict).map(function(origin) {
On 2014/07/22 08:51:47, Bernhard Bauer wrote:
> map() will return a list containing the return values of the function, so you
> can assign that directly to origins. What you wrote here would be forEach().

Whoops.

Now using map() to assign directly.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:104: usage_string:
originDict[origin].usageString});
On 2014/07/22 08:51:47, Bernhard Bauer wrote:
> Make this usageString as well.

Done and fixed other locations as well.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:77:
std::find(std::begin(kValidTypes), std::end(kValidTypes), content_type));
I've removed the end iterator after running the trybots.

https://codereview.chromium.org/408493003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:147: void
WebsiteSettingsHandler::UpdateLocalStorage(const std::string& filter) {
On 2014/07/22 08:51:48, Bernhard Bauer wrote:
> You don't need to pass the filter as an argument now, because you have it as
> |last_filter_|.

Done.

[Bernhard Bauer, 2014-07-23 09:09:44]

LGTM with some nits:

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:101: return {origin:
origin,
Nit: This still looks a bit awkward... Can you write it like this:

return {
  origin: origin,
  ...
};

?

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/website_settings_handler.h (right):

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:54: // and
Un-break this line.

[Daniel Nishi, 2014-07-23 16:01:11]

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:101: return {origin:
origin,
On 2014/07/23 09:09:43, Bernhard Bauer wrote:
> Nit: This still looks a bit awkward... Can you write it like this:
> 
> return {
>   origin: origin,
>   ...
> };
> 
> ?

Done.

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/website_settings_handler.h (right):

https://codereview.chromium.org/408493003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:54: // and
On 2014/07/23 09:09:43, Bernhard Bauer wrote:
> Un-break this line.

Done.

[Daniel Nishi, 2014-07-23 16:01:14]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-07-23 16:02:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dhnishi@chromium.org/408493003/80001

[commit-bot: I haz the power, 2014-07-23 21:25:29]

Change committed as 285045