Making structure for ContentSettings and its corresponding strings.

Making structure entry for ContentSetting and corresponding strings for better understanding and maintainability for conversion from ContentSetting to string and vise a versa.

BUG=538138
Committed: https://crrev.com/c1d73e5f7f0b38057d41bf78d282da4521f1fdc8
Cr-Commit-Position: refs/heads/master@{#353226}

[Deepak, 2015-10-01 13:19:14]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[Deepak, 2015-10-01 13:19:14]

PTAL

[Bernhard Bauer, 2015-10-01 14:36:09]

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:28: struct
ContentSettingsToFromString {
Nit: ContentSettingsStringMapping might be better. Same for the constant.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:49:
DCHECK(setting >= CONTENT_SETTING_DEFAULT &&
Split this up into two checks, and use DCHECK_GE / DCHECK_LT.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:51: return
kContentSettingsToFromString[setting].content_setting_str;
There is an implicit assumption here that
`kContentSettingsToFromString[setting].content_setting == setting`. At the very
least, we should verify that with a DCHECK, and add a unit test that exercises
this for all possible values. Even that is not great, as DCHECKs are not always
enabled in Release builds (and if they are, the stack traces are hard to
decipher), so a unit test on a Release bot will be kind of worthless.

But testing the roundtrip (content setting to string to content setting) should
work in a unit test, and fail if someone updates the ContentSetting enum without
updating this file.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:55: size_t i
= 0;
Inline this into the loop?

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:58: break;
You could directly return here, then you only need to do the NOTREACHED() below.

[Deepak, 2015-10-03 09:02:12]

Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:28: struct
ContentSettingsToFromString {
On 2015/10/01 14:36:09, Bernhard Bauer wrote:
> Nit: ContentSettingsStringMapping might be better. Same for the constant.

Done.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:49:
DCHECK(setting >= CONTENT_SETTING_DEFAULT &&
On 2015/10/01 14:36:09, Bernhard Bauer wrote:
> Split this up into two checks, and use DCHECK_GE / DCHECK_LT.

Done.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:51: return
kContentSettingsToFromString[setting].content_setting_str;
On 2015/10/01 14:36:09, Bernhard Bauer wrote:
> There is an implicit assumption here that
> `kContentSettingsToFromString[setting].content_setting == setting`. At the
very
> least, we should verify that with a DCHECK, and add a unit test that exercises
> this for all possible values. Even that is not great, as DCHECKs are not
always
> enabled in Release builds (and if they are, the stack traces are hard to
> decipher), so a unit test on a Release bot will be kind of worthless.
> 
> But testing the roundtrip (content setting to string to content setting)
should
> work in a unit test, and fail if someone updates the ContentSetting enum
without
> updating this file.

Done.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:55: size_t i
= 0;
On 2015/10/01 14:36:09, Bernhard Bauer wrote:
> Inline this into the loop?

Done.

https://codereview.chromium.org/1372353004/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:58: break;
On 2015/10/01 14:36:09, Bernhard Bauer wrote:
> You could directly return here, then you only need to do the NOTREACHED()
below.

Done.

[Bernhard Bauer, 2015-10-05 08:15:10]

BTW, there is also a ContentSettingToString method in
chrome/browser/extensions/api/content_settings/content_settings_helpers.h; it
might make sense to unify these.

https://codereview.chromium.org/1372353004/diff/60001/components/content_sett...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:56: for
(size_t i = 1; i < arraysize(kContentSettingsStringMapping); ++i) {
Please add a comment explaining why we start with 1 here.

[Deepak, 2015-10-05 09:51:30]

PTAL

https://codereview.chromium.org/1372353004/diff/60001/components/content_sett...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.cc:56: for
(size_t i = 1; i < arraysize(kContentSettingsStringMapping); ++i) {
On 2015/10/05 08:15:09, Bernhard Bauer wrote:
> Please add a comment explaining why we start with 1 here.

Done.

[Bernhard Bauer, 2015-10-05 11:15:41]

Thanks for the yak shaving!

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(left):

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:27:
"session_only",
We are probably going to have to do this the other way around: these strings are
part of the public extension API, so we can't change them. What we can change is
the strings used internally, as those are just identifiers for Web UI. So let's
update the internal strings to match this, and update the corresponding Web UI.

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:123:
bool StringToContentSetting(const std::string& setting_str,
Can we remove this one as well? This version seems to allow
CONTENT_SETTING_DEFAULT, but that is wrong (DEFAULT is not a valid setting).

[Deepak, 2015-10-05 12:14:38]

Please correct me if I misunderstood something.
Rest changes done.
PTAL.

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(left):

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:27:
"session_only",
On 2015/10/05 11:15:41, Bernhard Bauer wrote:
> We are probably going to have to do this the other way around: these strings
are
> part of the public extension API, so we can't change them. What we can change
is
> the strings used internally, as those are just identifiers for Web UI. So
let's
> update the internal strings to match this, and update the corresponding Web
UI.

I have removed this array here, and updated the strings in
content_settings_utils.cc file.

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/1372353004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:123:
bool StringToContentSetting(const std::string& setting_str,
On 2015/10/05 11:15:41, Bernhard Bauer wrote:
> Can we remove this one as well? This version seems to allow
> CONTENT_SETTING_DEFAULT, but that is wrong (DEFAULT is not a valid setting).

I think as the return type of this API is bool, so we can keep this API and
functionality we can update.

[Bernhard Bauer, 2015-10-05 12:34:29]

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:88: return
"session_only";
I think you need to update the corresponding Web UI now. Luckily, "session" is
only used for cookies, and "detect" only for plugins, so you can search the
content settings Web UI for those.

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:117:
if (content_setting != CONTENT_SETTING_DEFAULT) {
Hm... this way around is a bit awkward, with the explicit comparison to DEFAULT.
Actually, ContentSettingFromString will DCHECK when given an invalid string,
while this method is supposed to return false, so this won't actually work. What
we could do is change the other method to this interface, and DCHECK the return
value at the call site.

[Deepak, 2015-10-05 14:25:32]

PTAL

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:88: return
"session_only";
On 2015/10/05 12:34:28, Bernhard Bauer wrote:
> I think you need to update the corresponding Web UI now. Luckily, "session" is
> only used for cookies, and "detect" only for plugins, so you can search the
> content settings Web UI for those.

Done.

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/1372353004/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:117:
if (content_setting != CONTENT_SETTING_DEFAULT) {
On 2015/10/05 12:34:28, Bernhard Bauer wrote:
> Hm... this way around is a bit awkward, with the explicit comparison to
DEFAULT.
> Actually, ContentSettingFromString will DCHECK when given an invalid string,
> while this method is supposed to return false, so this won't actually work.
What
> we could do is change the other method to this interface, and DCHECK the
return
> value at the call site.

Done.

[Bernhard Bauer, 2015-10-05 14:31:48]

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:212:
DCHECK_NE(CONTENT_SETTING_DEFAULT, setting);
Sorry, this is the wrong way around (and a DCHECK is not the same thing as
EXTENSION_FUNCTION_VALIDATE!).

What I meant was: Keep the signature that returns a bool and write to an output
parameter, and change call sites with the other signature to that. That way, we
can have code that asserts the string is valid (by DCHECKing the return value),
or code that handles invalid strings in a different way (like here). You can't
do it if the conversion function already does the DCHECK.

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:89:
optionDetect.value = 'detect_important_content';
I think you also need to update
chrome/browser/resources/options/content_settings.html.

[Deepak, 2015-10-06 15:18:40]

deepak.m1@samsung.com changed reviewers:
+ ajith.v@chromium.org

[Deepak, 2015-10-06 15:18:41]

++ Adding AKV for Peer review.

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:212:
DCHECK_NE(CONTENT_SETTING_DEFAULT, setting);
On 2015/10/05 14:31:48, Bernhard Bauer wrote:
> Sorry, this is the wrong way around (and a DCHECK is not the same thing as
> EXTENSION_FUNCTION_VALIDATE!).
> 
> What I meant was: Keep the signature that returns a bool and write to an
output
> parameter, and change call sites with the other signature to that. That way,
we
> can have code that asserts the string is valid (by DCHECKing the return
value),
> or code that handles invalid strings in a different way (like here). You can't
> do it if the conversion function already does the DCHECK.

Done.

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/1372353004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/content_settings_exceptions_area.js:89:
optionDetect.value = 'detect_important_content';
On 2015/10/05 14:31:48, Bernhard Bauer wrote:
> I think you also need to update
> chrome/browser/resources/options/content_settings.html.

Done.

[Bernhard Bauer, 2015-10-06 15:37:25]

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:113:
*setting = content_settings::ContentSettingFromString(setting_str);
This is still incorrect. ContentSettingFromString will DCHECK if an invalid
string is passed, while the contract of this method requires it to return false
-- which will never happen because the DCHECK will crash beforehand. See also my
other comment for what we should do.

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.h
(right):

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_helpers.h:36:
bool StringToContentSetting(const std::string& setting_str,
Let's remove this method, change the other's signature to this one, then update
callers of the other method to DCHECK the return value, so we get the same
behavior.

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:49:
// Checking content_settings::ContentSettingToString() functionality.
Nit: Remove this comment, the code tell you pretty much the same thing ☺

[AKV, 2015-10-06 16:01:38]

Please check my inline comments

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:50:
EXPECT_EQ("default",
If we define constant variable instead of hard coding it would be good, as we
use same string in 2 places;
const char* kContentSettingAllow = "allow"; for individual string

or

we can define an array like as it was in content_settings_helpers.cc and loop
through and EXPECT_EQ with proper indexes.
This will reduce number of LOCs in test code.

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:60:
EXPECT_EQ("detect_important_content",
Can we add a test case, for failure case by passing either empty string or
invalid string, to double ensure validities ?

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:65:
EXPECT_EQ(CONTENT_SETTING_ALLOW,
ditto

[Deepak, 2015-10-07 04:39:43]

@Bernhard & @AKV
Thanks for review.
All suggestions accommodated.
PTAL

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
(right):

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc:113:
*setting = content_settings::ContentSettingFromString(setting_str);
On 2015/10/06 15:37:25, Bernhard Bauer wrote:
> This is still incorrect. ContentSettingFromString will DCHECK if an invalid
> string is passed, while the contract of this method requires it to return
false
> -- which will never happen because the DCHECK will crash beforehand. See also
my
> other comment for what we should do.

Done.

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_helpers.h
(right):

https://codereview.chromium.org/1372353004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_helpers.h:36:
bool StringToContentSetting(const std::string& setting_str,
On 2015/10/06 15:37:25, Bernhard Bauer wrote:
> Let's remove this method, change the other's signature to this one, then
update
> callers of the other method to DCHECK the return value, so we get the same
> behavior.

Done.

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:49:
// Checking content_settings::ContentSettingToString() functionality.
On 2015/10/06 15:37:25, Bernhard Bauer wrote:
> Nit: Remove this comment, the code tell you pretty much the same thing ☺

Done.

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:50:
EXPECT_EQ("default",
On 2015/10/06 16:01:38, AKV wrote:
> If we define constant variable instead of hard coding it would be good, as we
> use same string in 2 places;
> const char* kContentSettingAllow = "allow"; for individual string
> 
> or
> 
> we can define an array like as it was in content_settings_helpers.cc and loop
> through and EXPECT_EQ with proper indexes.
> This will reduce number of LOCs in test code.

Done.

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:60:
EXPECT_EQ("detect_important_content",
On 2015/10/06 16:01:38, AKV wrote:
> Can we add a test case, for failure case by passing either empty string or
> invalid string, to double ensure validities ?

We can add negative test case for content_settings::ContentSettingFromString(),
but negative test for 
content_settings::ContentSettingToString() failed due to DCHECK() in
ContentSettingToString().

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:65:
EXPECT_EQ(CONTENT_SETTING_ALLOW,
On 2015/10/06 16:01:38, AKV wrote:
> ditto

Done.

[AKV, 2015-10-07 06:19:17]

Thanks.
peer review looks good to me!

[Deepak, 2015-10-07 06:29:40]

On 2015/10/07 06:19:17, AKV wrote:
> Thanks.
> peer review looks good to me!

@AKV
Thanks.

@Bernhard
PTAL

[Bernhard Bauer, 2015-10-07 09:32:04]

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/240001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:60:
EXPECT_EQ("detect_important_content",
On 2015/10/07 04:39:43, Deepak wrote:
> On 2015/10/06 16:01:38, AKV wrote:
> > Can we add a test case, for failure case by passing either empty string or
> > invalid string, to double ensure validities ?
> 
> We can add negative test case for
content_settings::ContentSettingFromString(),
> but negative test for 
> content_settings::ContentSettingToString() failed due to DCHECK() in
> ContentSettingToString().

If we remove that DCHECK (see my other comment), we can add a negative test
case.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:65:
NOTREACHED() << name << " is not a recognized content setting.";
Remove this. Passing an invalid string is now acceptable, and the method is
supposed to return false in that case. This is the reason why we added the
DCHECK at the callsites instead.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:12:
namespace {
Empty lines after opening a namespace and before closing it.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:19:
"detect_important_content"
Nit: Add a comma at the end here; it will reduce the number of modified lines if
we should add another setting.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:21:
static_assert(arraysize(kContentSettingNames) <= CONTENT_SETTING_NUM_SETTINGS,
You want == here.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:62:
ContentSetting setting;
Move this into the loop to reduce scope.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:68:
for (size_t type = 0; type < arraysize(kContentSettingNames); ++type) {
Just combine this with the previous loop.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:70:
if (type == 0)
Braces for multi-line bodies.

[Deepak, 2015-10-07 11:12:25]

@bauerb
Thanks for review, Changes done.
PTAL.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:65:
NOTREACHED() << name << " is not a recognized content setting.";
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> Remove this. Passing an invalid string is now acceptable, and the method is
> supposed to return false in that case. This is the reason why we added the
> DCHECK at the callsites instead.

Done.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:12:
namespace {
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> Empty lines after opening a namespace and before closing it.

Done.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:19:
"detect_important_content"
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> Nit: Add a comma at the end here; it will reduce the number of modified lines
if
> we should add another setting.

Done.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:21:
static_assert(arraysize(kContentSettingNames) <= CONTENT_SETTING_NUM_SETTINGS,
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> You want == here.

Done.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:62:
ContentSetting setting;
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> Move this into the loop to reduce scope.

Done.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:68:
for (size_t type = 0; type < arraysize(kContentSettingNames); ++type) {
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> Just combine this with the previous loop.

Done.

https://codereview.chromium.org/1372353004/diff/280001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:70:
if (type == 0)
On 2015/10/07 09:32:04, Bernhard Bauer wrote:
> Braces for multi-line bodies.

Done.

[Bernhard Bauer, 2015-10-07 11:38:21]

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:78: std::string
GetStringForContentSettingsType(
Thanks for finding these!

I took a look at this method, and it's only used to marshal content settings to
Java, where only ALLOW and BLOCK are used. Really, this is kind of unnecessary
to do via strings (I suspect whoever implemented it was looking at the Web UI
version), because it could just use the numeric values instead. I can do that,
as I've found a bunch of other things to clean up in this file anyway ☺

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:252:
setting_string.c_str(),
Actually, just pass in |setting_str| here -- we already have that!

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:191: bool
return_value =
Call this variable |success| maybe? |return_value| is overly generic.

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:44: }  //
namespace
Nit: empty line before closing a namespace

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:52:
kContentSettingsStringMapping[setting].content_setting == setting) {
This is an invariant (it should always be true), so there is no need to check it
at runtime.

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.h (right):

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
components/content_settings/core/browser/content_settings_utils.h:65: bool
ContentSettingToString(ContentSetting setting,
I think for this method returning the old signature is okay, as there should be
no way to get a ContentSetting with an invalid value, so we should be able to
DCHECK that.

[Deepak, 2015-10-07 13:54:43]

Changes done as suggested.
PTAL

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:78: std::string
GetStringForContentSettingsType(
On 2015/10/07 11:38:21, Bernhard Bauer wrote:
> Thanks for finding these!
> 
> I took a look at this method, and it's only used to marshal content settings
to
> Java, where only ALLOW and BLOCK are used. Really, this is kind of unnecessary
> to do via strings (I suspect whoever implemented it was looking at the Web UI
> version), because it could just use the numeric values instead. I can do that,
> as I've found a bunch of other things to clean up in this file anyway ☺

Acknowledged.

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:252:
setting_string.c_str(),
On 2015/10/07 11:38:21, Bernhard Bauer wrote:
> Actually, just pass in |setting_str| here -- we already have that!

Done.

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1372353004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:191: bool
return_value =
On 2015/10/07 11:38:21, Bernhard Bauer wrote:
> Call this variable |success| maybe? |return_value| is overly generic.

Done.

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:44: }  //
namespace
On 2015/10/07 11:38:21, Bernhard Bauer wrote:
> Nit: empty line before closing a namespace

Done.

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:52:
kContentSettingsStringMapping[setting].content_setting == setting) {
On 2015/10/07 11:38:21, Bernhard Bauer wrote:
> This is an invariant (it should always be true), so there is no need to check
it
> at runtime.

Done.

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.h (right):

https://codereview.chromium.org/1372353004/diff/300001/components/content_set...
components/content_settings/core/browser/content_settings_utils.h:65: bool
ContentSettingToString(ContentSetting setting,
On 2015/10/07 11:38:21, Bernhard Bauer wrote:
> I think for this method returning the old signature is okay, as there should
be
> no way to get a ContentSetting with an invalid value, so we should be able to
> DCHECK that.

Done.

[Bernhard Bauer, 2015-10-07 15:05:44]

https://codereview.chromium.org/1372353004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1372353004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:244:
setting_str = content_settings::ContentSettingToString(setting);
This is unnecessary -- You get |setting| from |setting_str| already, so
assigning it back is a no-op.

https://codereview.chromium.org/1372353004/diff/360001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/360001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:54: return
ResourceIdentifier();
This should have never been ResourceIdentifier, but just std::string().

[Deepak, 2015-10-07 15:17:18]

PTAL

https://codereview.chromium.org/1372353004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1372353004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:244:
setting_str = content_settings::ContentSettingToString(setting);
On 2015/10/07 15:05:44, Bernhard Bauer wrote:
> This is unnecessary -- You get |setting| from |setting_str| already, so
> assigning it back is a no-op.

oops, I missed this.
yes, we can directly use setting_str.

https://codereview.chromium.org/1372353004/diff/360001/components/content_set...
File components/content_settings/core/browser/content_settings_utils.cc (right):

https://codereview.chromium.org/1372353004/diff/360001/components/content_set...
components/content_settings/core/browser/content_settings_utils.cc:54: return
ResourceIdentifier();
On 2015/10/07 15:05:44, Bernhard Bauer wrote:
> This should have never been ResourceIdentifier, but just std::string().

Done.

[Bernhard Bauer, 2015-10-07 15:30:16]

https://codereview.chromium.org/1372353004/diff/380001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1372353004/diff/380001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:88: return
"session_only";
Can you revert the changes in this file? They don't affect anything, and I'm
going to remove this function anyway.

[Deepak, 2015-10-08 06:14:53]

PTAL

https://codereview.chromium.org/1372353004/diff/380001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/1372353004/diff/380001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:88: return
"session_only";
On 2015/10/07 15:30:16, Bernhard Bauer wrote:
> Can you revert the changes in this file? They don't affect anything, and I'm
> going to remove this function anyway.

Done.

[Bernhard Bauer, 2015-10-08 09:32:46]

A last set of nits (sorry I missed those earlier):

https://codereview.chromium.org/1372353004/diff/400001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/400001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:68:
for (size_t type = 0; type < arraysize(kContentSettingNames); ++type) {
Nit: |type| isn't actually the correct name for this, as we are iterating over
settings. We could just call it |i| and leave |setting| for the ContentSetting
variable below.

https://codereview.chromium.org/1372353004/diff/400001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:73:
ContentSetting setting_type;
Similarly, |_type| is incorrect here. |converted_setting| maybe?

[Deepak, 2015-10-08 10:48:15]

Nit Addressed.
PTAL
Thanks

https://codereview.chromium.org/1372353004/diff/400001/components/content_set...
File components/content_settings/core/browser/content_settings_utils_unittest.cc
(right):

https://codereview.chromium.org/1372353004/diff/400001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:68:
for (size_t type = 0; type < arraysize(kContentSettingNames); ++type) {
On 2015/10/08 09:32:46, Bernhard Bauer wrote:
> Nit: |type| isn't actually the correct name for this, as we are iterating over
> settings. We could just call it |i| and leave |setting| for the ContentSetting
> variable below.

Done.

https://codereview.chromium.org/1372353004/diff/400001/components/content_set...
components/content_settings/core/browser/content_settings_utils_unittest.cc:73:
ContentSetting setting_type;
On 2015/10/08 09:32:46, Bernhard Bauer wrote:
> Similarly, |_type| is incorrect here. |converted_setting| maybe?

Done.

[Bernhard Bauer, 2015-10-08 11:05:29]

LGTM, thanks!

[Deepak, 2015-10-08 11:13:21]

deepak.m1@samsung.com changed reviewers:
+ rockot@chromium.org

[Deepak, 2015-10-08 11:13:21]

@Bernhard
Thanks.

@Ken Rockot
Please give approval for
chrome/browser/extensions/api/content_settings/*

Thanks

[Ken Rockot(use gerrit already), 2015-10-08 23:23:20]

extensions lgtm

[Deepak, 2015-10-09 02:46:16]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-10-09 02:47:24]

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

[commit-bot: I haz the power, 2015-10-09 04:04:23]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2015-10-09 04:05:04]

Patchset 22 (id:??) landed as
https://crrev.com/c1d73e5f7f0b38057d41bf78d282da4521f1fdc8
Cr-Commit-Position: refs/heads/master@{#353226}