Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 2 months ago by Deepak Modified:
5 years, 2 months ago Reviewers:
Bernhard Bauer, Ken Rockot(use gerrit already), AKV CC:
chromium-reviews, markusheintz_, raymes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking 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}
Patch Set 1 #Patch Set 2 : updating variable name. #
Total comments: 10
Patch Set 3 : Changes as per review comments. #Patch Set 4 : Adding Test Case. #
Total comments: 2
Patch Set 5 : Changes as per review comments. #
Total comments: 4
Patch Set 6 : Changes as per review comments. #
Total comments: 4
Patch Set 7 : Changes as per review comments. #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : Changes as per review comments. #Patch Set 12 : Changes as per review comments. #Patch Set 13 : #
Total comments: 13
Patch Set 14 : Changes as per review comments. #Patch Set 15 : #
Total comments: 14
Patch Set 16 : Changes as per review comments. #
Total comments: 12
Patch Set 17 : Changes as per review comments. #Patch Set 18 : #Patch Set 19 : #
Total comments: 4
Patch Set 20 : #
Total comments: 2
Patch Set 21 : Removing chnages from pref_service_bridge.cc file. #
Total comments: 4
Patch Set 22 : Addressing nits. #
Created: 5 years, 2 months ago
Messages
Total messages: 36 (4 generated)
deepak.m1@samsung.com changed reviewers: + bauerb@chromium.org
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 { 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.
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.
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.
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.
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).
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.
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.
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.
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.m1@samsung.com changed reviewers: + ajith.v@chromium.org
++ 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.
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 ☺
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
@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.
Thanks. peer review looks good to me!
On 2015/10/07 06:19:17, AKV wrote: > Thanks. > peer review looks good to me! @AKV Thanks. @Bernhard PTAL
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.
@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.
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.
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.
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().
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.
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.
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.
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?
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.
LGTM, thanks!
deepak.m1@samsung.com changed reviewers: + rockot@chromium.org
@Bernhard Thanks. @Ken Rockot Please give approval for chrome/browser/extensions/api/content_settings/* Thanks
extensions lgtm
The CQ bit was checked by deepak.m1@samsung.com
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
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/c1d73e5f7f0b38057d41bf78d282da4521f1fdc8 Cr-Commit-Position: refs/heads/master@{#353226} |
