Add a single site view page to view and modify content settings.

Add a single site view page to view and modify content settings for the
permission and resource manager.

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

[Daniel Nishi, 2014-08-14 22:31:29]

gbillock@chromium.org: Please review changes in
chrome/browser/ui/website_settings/website_settings.cc

bauerb@chromium.org: Please review changes in rest.

Thanks!

[Daniel Nishi, 2014-08-14 22:31:57]

+scheib FYI

[Bernhard Bauer, 2014-08-15 14:53:05]

https://codereview.chromium.org/475193002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/475193002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7957: +        Knows device location
Description and message don't match ID (Also, the description is missing a
"does").

https://codereview.chromium.org/475193002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7966: +        <ph name="USAGE">$1<ex>2.0
KB</ex></ph> stored data
I assume these string will go through string review? I would rather write "2.0
KB of stored data" or "2.0KB data stored".

https://codereview.chromium.org/475193002/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/475193002/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:306:
DCHECK(existing_info.source == content_settings::SETTING_SOURCE_USER);
DCHECK_EQ?

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings_edit_page.js:58: * @param
{Array} args A list containing the amount of local storage used
From the code, it seems this is a dictionary?

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:60: using
content_settings::ContentSettingToString;
Move this before extensions::

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:258: // Users are
not allowed to be the source of setting to ask. It is an
Nit: 'the source of the "ask" setting' might be a bit more understandable.

https://codereview.chromium.org/475193002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:357: if (mic_setting
!= cam_setting || mic_setting == CONTENT_SETTING_ASK)
The second case here has the same effect as the else branch, so it can be left
out.

[Daniel Nishi, 2014-08-15 16:56:45]

+rolfe@
See comments for generated_resource.grd:7966.

https://codereview.chromium.org/475193002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/475193002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7957: +        Knows device location
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> Description and message don't match ID (Also, the description is missing a
> "does").

Done.

https://codereview.chromium.org/475193002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7966: +        <ph name="USAGE">$1<ex>2.0
KB</ex></ph> stored data
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> I assume these string will go through string review? I would rather write "2.0
> KB of stored data" or "2.0KB data stored".

This matches the current mocks, but I'll check on the string review part.

+rolfe@
Do you know how a string review applies to the mocks?

https://codereview.chromium.org/475193002/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/475193002/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:306:
DCHECK(existing_info.source == content_settings::SETTING_SOURCE_USER);
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> DCHECK_EQ?

Done.

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings_edit_page.js:58: * @param
{Array} args A list containing the amount of local storage used
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> From the code, it seems this is a dictionary?

chrome/browser/ui/webui/options/website_settings_handler.cc:318 declares the
passed value as a ListValue, so I assumed that was an Array for typechecking
purposes.

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:60: using
content_settings::ContentSettingToString;
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> Move this before extensions::

Done.

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:258: // Users are
not allowed to be the source of setting to ask. It is an
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> Nit: 'the source of the "ask" setting' might be a bit more understandable.

Done.

https://codereview.chromium.org/475193002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/website_settings_handler.cc:357: if (mic_setting
!= cam_setting || mic_setting == CONTENT_SETTING_ASK)
On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> The second case here has the same effect as the else branch, so it can be left
> out.

Good catch. Done.

[Bernhard Bauer, 2014-08-15 17:13:04]

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings_edit_page.js:58: * @param
{Array} args A list containing the amount of local storage used
On 2014/08/15 16:56:45, Daniel Nishi wrote:
> On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> > From the code, it seems this is a dictionary?
> 
> chrome/browser/ui/webui/options/website_settings_handler.cc:318 declares the
> passed value as a ListValue, so I assumed that was an Array for typechecking
> purposes.

Oh, I overlooked it's the whole arguments array.

CallJavascriptFunction() actually has a version that takes more than one
argument, so you can directly pass the correct arguments without having to
unpack them. (And then permissions can be declared as a dictionary.)

https://codereview.chromium.org/475193002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/475193002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:306:
DCHECK_EQ(existing_info.source, content_settings::SETTING_SOURCE_USER);
If you swap the arguments, the error message on failure is going to make more
sense.

[Daniel Nishi, 2014-08-15 17:33:46]

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

https://codereview.chromium.org/475193002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings_edit_page.js:58: * @param
{Array} args A list containing the amount of local storage used
On 2014/08/15 17:13:04, Bernhard Bauer wrote:
> On 2014/08/15 16:56:45, Daniel Nishi wrote:
> > On 2014/08/15 14:53:05, Bernhard Bauer wrote:
> > > From the code, it seems this is a dictionary?
> > 
> > chrome/browser/ui/webui/options/website_settings_handler.cc:318 declares the
> > passed value as a ListValue, so I assumed that was an Array for typechecking
> > purposes.
> 
> Oh, I overlooked it's the whole arguments array.
> 
> CallJavascriptFunction() actually has a version that takes more than one
> argument, so you can directly pass the correct arguments without having to
> unpack them. (And then permissions can be declared as a dictionary.)

Done.

https://codereview.chromium.org/475193002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/475193002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:306:
DCHECK_EQ(existing_info.source, content_settings::SETTING_SOURCE_USER);
On 2014/08/15 17:13:04, Bernhard Bauer wrote:
> If you swap the arguments, the error message on failure is going to make more
> sense.

Done.

[Bernhard Bauer, 2014-08-15 20:23:33]

LGTM!

[Daniel Nishi, 2014-08-18 15:38:18]

gbillock: Ping.

[Daniel Nishi, 2014-08-19 16:47:49]

Ping gbillock@

[Greg Billock, 2014-08-20 18:50:18]

On 2014/08/19 16:47:49, Daniel Nishi wrote:
> Ping gbillock@

lgtm

[Daniel Nishi, 2014-08-20 19:08:13]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-08-20 19:09:56]

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

[commit-bot: I haz the power, 2014-08-20 19:21:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44301)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/49463)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 19:25:26]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44309)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Daniel Nishi, 2014-08-20 19:25:48]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-08-20 19:27:14]

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

[commit-bot: I haz the power, 2014-08-20 20:52:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-20 20:55:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 20:55:21]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Daniel Nishi, 2014-08-21 16:56:56]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-08-21 16:59:39]

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

[commit-bot: I haz the power, 2014-08-21 21:10:16]

Committed patchset #4 (80001) as 291183