Add a button to clear local storage for a given origin to Website Settings dialog.

Add a button to clear local storage for a given origin to Website Settings dialog.

Add functionality for button to stop origins on the Website Settings dialog.

BUG=372607
Committed: https://crrev.com/0d93885678215de0d9104ed1fc7ef9b5a743c563
Cr-Commit-Position: refs/heads/master@{#291578}

[Daniel Nishi, 2014-08-22 20:25:28]

bauerb@: PTAL.

The KillOrigins functionality will be surfaced with the battery auditing surface
that will be up once my last battery auditing backend patch gets committed.

[Daniel Nishi, 2014-08-22 20:25:55]

scheib@: FYI

[Bernhard Bauer, 2014-08-22 20:42:39]

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings_edit_page.js (right):

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings_edit_page.js:68:
populateOrigin_: function(local_storage, permissions, show_page) {
This should be named showPage (same goes for local_storage, sorry I missed
that).

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings_edit_site.html (right):

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings_edit_site.html:9: <button
id="websiteSettingsStorageDeleteButton"
id uses separated-by-dashes-style.

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:473: profile ==
Profile::FromBrowserContext(it->GetBrowserContext())) {
How does this interact with incognito?

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:474: it->Close();
This closes the tabs completely, right? Is that the desired behavior? Could we
discard the tab instead (see TabStripModel::DiscardWebContentsAt)?

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:487: local_storage_
= NULL;
Is this assignment really necessary?

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:94: // usage for a
given |site_url|. If |show_page|, it raises a new single site
"If |show_page| is true, [...]"

[Daniel Nishi, 2014-08-22 21:45:25]

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings_edit_page.js (right):

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings_edit_page.js:68:
populateOrigin_: function(local_storage, permissions, show_page) {
On 2014/08/22 20:42:38, Bernhard Bauer wrote:
> This should be named showPage (same goes for local_storage, sorry I missed
> that).

Done.

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings_edit_site.html (right):

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/website_settings_edit_site.html:9: <button
id="websiteSettingsStorageDeleteButton"
On 2014/08/22 20:42:38, Bernhard Bauer wrote:
> id uses separated-by-dashes-style.

Done.

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:473: profile ==
Profile::FromBrowserContext(it->GetBrowserContext())) {
On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> How does this interact with incognito?

Incognito isn't affected. I'm pretty sure it is because there isn't ptr
equality, since the on-the-record Profile owns the off-the-record one.

Also, you can't get the chrome://settings page in incognito to my knowledge, so
you can't use it to stop incognito pages.

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:474: it->Close();
On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> This closes the tabs completely, right? Is that the desired behavior? Could
we
> discard the tab instead (see TabStripModel::DiscardWebContentsAt)?

Ooh. I didn't know about the Discard functionality.

Now using it.

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:487: local_storage_
= NULL;
On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> Is this assignment really necessary?

Hmm. I think I misunderstood a comment in the scoped_refptr documentation.

I've taken it out.

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.h:94: // usage for a
given |site_url|. If |show_page|, it raises a new single site
On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> "If |show_page| is true, [...]"

Done.

[Bernhard Bauer, 2014-08-22 21:54:36]

LGTM w/ a nit. If we decide to do something about incognito, we can do that in a
followup.

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:473: profile ==
Profile::FromBrowserContext(it->GetBrowserContext())) {
On 2014/08/22 21:45:25, Daniel Nishi wrote:
> On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> > How does this interact with incognito?
> 
> Incognito isn't affected. I'm pretty sure it is because there isn't ptr
> equality, since the on-the-record Profile owns the off-the-record one.
> 
> Also, you can't get the chrome://settings page in incognito to my knowledge,
so
> you can't use it to stop incognito pages.

Yeah, I was more wondering whether that's something we would want to do... We do
show incognito content settings in chrome://settings, for example.

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

https://codereview.chromium.org/493383004/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:483: continue;
I would probably wrap this in braces, what with the four-line if-statement.

Alternatively, if you do early-continue anyway, split this up into several
checks?

[Daniel Nishi, 2014-08-22 22:12:47]

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:473: profile ==
Profile::FromBrowserContext(it->GetBrowserContext())) {
On 2014/08/22 21:54:36, Bernhard Bauer wrote:
> On 2014/08/22 21:45:25, Daniel Nishi wrote:
> > On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> > > How does this interact with incognito?
> > 
> > Incognito isn't affected. I'm pretty sure it is because there isn't ptr
> > equality, since the on-the-record Profile owns the off-the-record one.
> > 
> > Also, you can't get the chrome://settings page in incognito to my knowledge,
> so
> > you can't use it to stop incognito pages.
> 
> Yeah, I was more wondering whether that's something we would want to do... We
do
> show incognito content settings in chrome://settings, for example. 

I did a little experiment -- you can see OTR content settings for everything
except location, notifications, and mediastream, making this incognito behavior
actually match the exceptions area.

It sounds like that might be a bug, though. Is the proper behavior to display
the OTR content settings both here and in the content settings exceptions pages?

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

https://codereview.chromium.org/493383004/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:483: continue;
On 2014/08/22 21:54:36, Bernhard Bauer wrote:
> I would probably wrap this in braces, what with the four-line if-statement.
> 
> Alternatively, if you do early-continue anyway, split this up into several
> checks?

Split into two checks.

[Bernhard Bauer, 2014-08-22 22:41:27]

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

https://codereview.chromium.org/493383004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:473: profile ==
Profile::FromBrowserContext(it->GetBrowserContext())) {
On 2014/08/22 22:12:47, Daniel Nishi wrote:
> On 2014/08/22 21:54:36, Bernhard Bauer wrote:
> > On 2014/08/22 21:45:25, Daniel Nishi wrote:
> > > On 2014/08/22 20:42:39, Bernhard Bauer wrote:
> > > > How does this interact with incognito?
> > > 
> > > Incognito isn't affected. I'm pretty sure it is because there isn't ptr
> > > equality, since the on-the-record Profile owns the off-the-record one.
> > > 
> > > Also, you can't get the chrome://settings page in incognito to my
knowledge,
> > so
> > > you can't use it to stop incognito pages.
> > 
> > Yeah, I was more wondering whether that's something we would want to do...
We
> do
> > show incognito content settings in chrome://settings, for example. 
> 
> I did a little experiment -- you can see OTR content settings for everything
> except location, notifications, and mediastream, making this incognito
behavior
> actually match the exceptions area.
> 
> It sounds like that might be a bug, though. Is the proper behavior to display
> the OTR content settings both here and in the content settings exceptions
pages?

Yeah, I think so. We could probably mark them up differently so it's clear
they're only temporary.

[Daniel Nishi, 2014-08-22 22:43:43]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-08-22 22:44:15]

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

[commit-bot: I haz the power, 2014-08-23 21:19:09]

Committed patchset #4 (80001) as 635ab05eb239b3c2bf2a79eb489cfc60acfb59fb

[commit-bot: I haz the power, 2014-09-10 02:30:50]

Patchset 4 (id:??) landed as
https://crrev.com/0d93885678215de0d9104ed1fc7ef9b5a743c563
Cr-Commit-Position: refs/heads/master@{#291578}