Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1004733003:
Split the default content settings into syncable and nonsyncable. (Closed)
DescriptionSplit the default content settings into syncable and nonsyncable.
Parallel to the default content setting dictionary pref, I added N individual integer prefs. Syncable preferences are kept in sync with the matching dictionary entries, so that they can be synced to/from old versions of Chrome.
Default provider used the following, somewhat complicated, logic:
- when we read from a dictionary and there is no value for a particular content type, we reset the value to default, except if default=CONTENT_SETTING_DEFAULT, then we reset it to NULL
- when someone calls SetWebsiteSetting() with value NULL, we reset the value to default, except if default=CONTENT_SETTING_DEFAULT, then we keep NULL
I kept this logic when working with the individual preferences, which explains why there are a lot of "if NULL" and "if default==..." checks.
To make sure that we don't forget these checks on any read/write, I added two pairs of methods for reading to/writing from individual prefs/dictionary pref:
(Read|Write)(Individual|Dictionary)Pref\(\)
and a wrapper for writing to the cache
ChangeSetting().
BUG=452388
TESTED=Between trunk and old M40 stable. Syncable settings are synced both ways. Nonsyncable settings are not synced either way.
Committed: https://crrev.com/4e9ea92c1cdcb084be89748a08f5b2e05b37c950
Cr-Commit-Position: refs/heads/master@{#321988}
Patch Set 1 #Patch Set 2 : Renamed pref, changed one check. #Patch Set 3 : Notifying the default as well. #Patch Set 4 : Refactored, tests seem to pass. #Patch Set 5 : NULL in SetWebsiteSetting must be converted to the default. #Patch Set 6 : Migration code. #Patch Set 7 : Changing APP_BANNER to unsyncable (recent decision). #
Total comments: 16
Patch Set 8 : Fixes, rewrote OnPreferenceUpdate. #
Total comments: 2
Patch Set 9 : Reset PMI, reacting to unsyncable pref changes as well. #
Total comments: 14
Messages
Total messages: 36 (10 generated)
msramek@chromium.org changed reviewers: + markusheintz@chromium.org
Hi Markus, could you PTAL? This CL splits the default content settings dictionary to individual prefs, and unsyncs some of them. Thanks, Martin
On 2015/03/17 10:00:42, msramek wrote: > Hi Markus, > > could you PTAL? > > This CL splits the default content settings dictionary to individual prefs, and > unsyncs some of them. > > Thanks, > Martin Does this work with older Chrome versions connected via sync?
On 2015/03/17 10:08:06, markusheintz_ wrote: > On 2015/03/17 10:00:42, msramek wrote: > > Hi Markus, > > > > could you PTAL? > > > > This CL splits the default content settings dictionary to individual prefs, > and > > unsyncs some of them. > > > > Thanks, > > Martin > > Does this work with older Chrome versions connected via sync? Yes. I tested syncing with an old Chrome M40. Though that was two patchsets ago, so let me try again to be sure.
On 2015/03/17 10:10:31, msramek wrote: > On 2015/03/17 10:08:06, markusheintz_ wrote: > > On 2015/03/17 10:00:42, msramek wrote: > > > Hi Markus, > > > > > > could you PTAL? > > > > > > This CL splits the default content settings dictionary to individual prefs, > > and > > > unsyncs some of them. > > > > > > Thanks, > > > Martin > > > > Does this work with older Chrome versions connected via sync? > > Yes. I tested syncing with an old Chrome M40. Though that was two patchsets ago, > so let me try again to be sure. Ok, I confirmed that it works. However, I just realized that even non-syncable settings should be synced *once*, otherwise they will be reset to defaults when you install a new version. Let me fix that.
On 2015/03/17 10:20:05, msramek wrote: > On 2015/03/17 10:10:31, msramek wrote: > > On 2015/03/17 10:08:06, markusheintz_ wrote: > > > On 2015/03/17 10:00:42, msramek wrote: > > > > Hi Markus, > > > > > > > > could you PTAL? > > > > > > > > This CL splits the default content settings dictionary to individual > prefs, > > > and > > > > unsyncs some of them. > > > > > > > > Thanks, > > > > Martin > > > > > > Does this work with older Chrome versions connected via sync? > > > > Yes. I tested syncing with an old Chrome M40. Though that was two patchsets > ago, > > so let me try again to be sure. > > Ok, I confirmed that it works. However, I just realized that even non-syncable > settings should be synced *once*, otherwise they will be reset to defaults when > you install a new version. Let me fix that. I added the migration code as well. PTAL.
+xhwang@: FYI, my CL for un-syncing the default settings (exceptions are separate). The MigrateDefaultSettings() method in DefaultProvider can be changed to reset the media identifier by adding: WriteIndividualPref(PROTECTED_MEDIA_IDENTIFIER, NULL); If it's just this, I can add it here. But if there is more logic that you want to do, I'd leave it for your follow-up CL.
https://codereview.chromium.org/1004733003/diff/120001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:32: const ContentSetting default_value; This does not work if we want to support arbitrary values. But let's do one step after the other. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:39: // default content setting. This array must be kept in sync with enum nit: "... sync with THE enum ..." https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:122: registry->RegisterIntegerPref( Can we register a general pref? https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:249: if (kDefaultSettings[content_type].syncable) Please add a comment here that describes why this is done. This code can be remove when we don't need it any more in a few milestones from today. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:304: == kDefaultSettings[content_type].default_value); Is this the correct indentation? https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:362: for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) Please use { } for the block of the for loop https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:385: if (content_type == CONTENT_SETTINGS_TYPE_DEFAULT) { As discussed offline. Let have distinct if () blocks. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:412: for (size_t i = 0; i < to_notify.size(); ++i) Please use { } for the for block
Patchset #9 (id:160001) has been deleted
https://codereview.chromium.org/1004733003/diff/120001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:32: const ContentSetting default_value; On 2015/03/18 14:39:49, markusheintz_ wrote: > This does not work if we want to support arbitrary values. But let's do one step > after the other. Acknowledged. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:39: // default content setting. This array must be kept in sync with enum On 2015/03/18 14:39:49, markusheintz_ wrote: > nit: "... sync with THE enum ..." Done. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:122: registry->RegisterIntegerPref( On 2015/03/18 14:39:49, markusheintz_ wrote: > Can we register a general pref? Hm, I was wrong. PregRegistrySyncable::RegisterSyncablePreference which takes base::Value is a protected method. So no, the general approach would be a dictionary preference everywhere. Unless we need to support arbitrary default values, that seems like an overkill now. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:249: if (kDefaultSettings[content_type].syncable) On 2015/03/18 14:39:49, markusheintz_ wrote: > Please add a comment here that describes why this is done. This code can be > remove when we don't need it any more in a few milestones from today. Done. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:304: == kDefaultSettings[content_type].default_value); On 2015/03/18 14:39:49, markusheintz_ wrote: > Is this the correct indentation? Code search suggests that some people use it this way, some with the extra 4 spaces indent. Hm, I agree that the other one is clearer. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:362: for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) On 2015/03/18 14:39:49, markusheintz_ wrote: > Please use { } for the block of the for loop Done. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:385: if (content_type == CONTENT_SETTINGS_TYPE_DEFAULT) { On 2015/03/18 14:39:49, markusheintz_ wrote: > As discussed offline. Let have distinct if () blocks. Done. https://codereview.chromium.org/1004733003/diff/120001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:412: for (size_t i = 0; i < to_notify.size(); ++i) On 2015/03/18 14:39:49, markusheintz_ wrote: > Please use { } for the for block Done.
On 2015/03/18 16:39:41, msramek wrote: > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > File > components/content_settings/core/browser/content_settings_default_provider.cc > (right): > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:32: > const ContentSetting default_value; > On 2015/03/18 14:39:49, markusheintz_ wrote: > > This does not work if we want to support arbitrary values. But let's do one > step > > after the other. > > Acknowledged. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:39: > // default content setting. This array must be kept in sync with enum > On 2015/03/18 14:39:49, markusheintz_ wrote: > > nit: "... sync with THE enum ..." > > Done. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:122: > registry->RegisterIntegerPref( > On 2015/03/18 14:39:49, markusheintz_ wrote: > > Can we register a general pref? > > Hm, I was wrong. PregRegistrySyncable::RegisterSyncablePreference which takes > base::Value is a protected method. So no, the general approach would be a > dictionary preference everywhere. > > Unless we need to support arbitrary default values, that seems like an overkill > now. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:249: > if (kDefaultSettings[content_type].syncable) > On 2015/03/18 14:39:49, markusheintz_ wrote: > > Please add a comment here that describes why this is done. This code can be > > remove when we don't need it any more in a few milestones from today. > > Done. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:304: > == kDefaultSettings[content_type].default_value); > On 2015/03/18 14:39:49, markusheintz_ wrote: > > Is this the correct indentation? > > Code search suggests that some people use it this way, some with the extra 4 > spaces indent. Hm, I agree that the other one is clearer. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:362: > for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) > On 2015/03/18 14:39:49, markusheintz_ wrote: > > Please use { } for the block of the for loop > > Done. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:385: > if (content_type == CONTENT_SETTINGS_TYPE_DEFAULT) { > On 2015/03/18 14:39:49, markusheintz_ wrote: > > As discussed offline. Let have distinct if () blocks. > > Done. > > https://codereview.chromium.org/1004733003/diff/120001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:412: > for (size_t i = 0; i < to_notify.size(); ++i) > On 2015/03/18 14:39:49, markusheintz_ wrote: > > Please use { } for the for block > > Done. LGTM
On 2015/03/18 13:43:37, msramek wrote: > +xhwang@: FYI, my CL for un-syncing the default settings (exceptions are > separate). > > The MigrateDefaultSettings() method in DefaultProvider can be changed to reset > the media identifier by adding: > > WriteIndividualPref(PROTECTED_MEDIA_IDENTIFIER, NULL); What will this do exactly? Will it clear the PROTECTED_MEDIA_IDENTIFIER setting in user's profile or change it to the default setting (ASK)? If so, do we also need to remove this line in the next release() (assuming migration is done)? > If it's just this, I can add it here. But if there is more logic that you want > to do, I'd leave it for your follow-up CL.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Please see my comment about questions on the migration plan: https://code.google.com/p/chromium/issues/detail?id=466715#c7
On 2015/03/18 20:32:11, xhwang wrote: > On 2015/03/18 13:43:37, msramek wrote: > > +xhwang@: FYI, my CL for un-syncing the default settings (exceptions are > > separate). > > > > The MigrateDefaultSettings() method in DefaultProvider can be changed to reset > > the media identifier by adding: > > > > WriteIndividualPref(PROTECTED_MEDIA_IDENTIFIER, NULL); > > What will this do exactly? Will it clear the PROTECTED_MEDIA_IDENTIFIER setting > in user's profile or change it to the default setting (ASK)? If so, do we also > need to remove this line in the next release() (assuming migration is done)? > > > If it's just this, I can add it here. But if there is more logic that you want > > to do, I'd leave it for your follow-up CL. Yes, exactly. The preference will be cleared in the profile, and therefore the hardcoded default ASK will be used. After the transition period, we will remove this line; in fact, we will remove about half of the code here, since everything has to be duplicated to work with the old dictionary pref and the new prefs alike.
raymes@: FYI. We discussed the design with markusheintz@ offline, so here's a brief explanation. 1. All settings are moved from dictionary to individual prefs on the first run. 2. If the dictionary is synced from an old version, only the syncable entries are synced to the individual prefs. 3. If an individual pref is changed, then iff it is syncable it will be written back to the old dictionary, so that it propagates to old versions as well. I'm putting this on hold until the discussion about #1 is resolved.
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/1004733003/diff/140001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/140001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:365: // this after two stable releases. Hmm, is there another way that the preference could be changed besides through this class?
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/1004733003/diff/140001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/140001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:365: // this after two stable releases. On 2015/03/19 23:45:21, raymes wrote: > Hmm, is there another way that the preference could be changed besides through > this class? Hm, good point. I wrote this method with only one thing in mind - the dictionary or syncable individual preferences being changed through sync. The unsyncable individual preferences cannot be changed by other means than someone calling SetWebsiteSetting() in HostContentSettingsMap and that propagating to this provider. But just in case it was possible, it's safer to rewrite if (kDefaultSettings[content_type].syncable) on line 403 to only contain WriteDictionaryPref() and all other actions (e.g. ChangeSetting()) can happen on unsyncable prefs as well.
https://codereview.chromium.org/1004733003/diff/200001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:502: WriteIndividualPref(CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER, NULL); Thank you!
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from markusheintz@chromium.org Link to the patchset: https://codereview.chromium.org/1004733003/#ps200001 (title: "Reset PMI, reacting to unsyncable pref changes as well.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1004733003/200001
The CQ bit was unchecked by msramek@chromium.org
The CQ bit was checked by msramek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1004733003/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4e9ea92c1cdcb084be89748a08f5b2e05b37c950 Cr-Commit-Position: refs/heads/master@{#321988}
Message was sent while issue was closed.
https://codereview.chromium.org/1004733003/diff/200001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:502: WriteIndividualPref(CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER, NULL); Just to double check. Will this only reset the "default setting"? We also want the per-origin settings be reset as well.
Message was sent while issue was closed.
On 2015/03/24 22:28:49, xhwang wrote: > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > File > components/content_settings/core/browser/content_settings_default_provider.cc > (right): > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:502: > WriteIndividualPref(CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER, NULL); > Just to double check. Will this only reset the "default setting"? We also want > the per-origin settings be reset as well. Yes, this is just the default. Per-origin settings are processed elsewhere (PrefProvider), so it's a separate change. Don't worry, it will be ready soon.
Message was sent while issue was closed.
On 2015/03/25 09:21:52, msramek wrote: > On 2015/03/24 22:28:49, xhwang wrote: > > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > > File > > components/content_settings/core/browser/content_settings_default_provider.cc > > (right): > > > > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > > > components/content_settings/core/browser/content_settings_default_provider.cc:502: > > WriteIndividualPref(CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER, NULL); > > Just to double check. Will this only reset the "default setting"? We also want > > the per-origin settings be reset as well. > > Yes, this is just the default. Per-origin settings are processed elsewhere > (PrefProvider), so it's a separate change. Don't worry, it will be ready soon. Cool! Thanks for the update. In that case I'll hold off my CL [1] until you land all your changes. [1] https://codereview.chromium.org/1024563005/
Message was sent while issue was closed.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
Message was sent while issue was closed.
Post-commit drive-by review! There are some style nits in here: https://codereview.chromium.org/1004733003/diff/200001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:42: {prefs::kDefaultCookiesSetting, CONTENT_SETTING_ALLOW, true}, Indent these lines by two spaces, and add a space after the opening brace and before the closing brace. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:121: for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) { Use an int unless you really really need size_t. Also, I *think* you might be able to iterate over the array with the C++11 syntax, then you don't need the index variable at all. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:305: return (value == NULL || Use nullptr, or just `!value` (which would also be consistent with the style in ChangeSetting). https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:377: for (ValueMap::iterator it = dictionary->begin(); Use a C++11-style loop? https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:428: return make_scoped_ptr((base::Value*)NULL); Use nullptr; that might even allow you to get rid of the cast. (Also, if you were to keep the cast, it should be static_cast<base::Value*>.) Also also, it might make sense to extract this logic into a function that creates a base::Value from a ContentSetting (we do this in a couple of other places as well). https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:460: ValueMap* value_map = new ValueMap(); Put this into a scoped_ptr, and return it immediately if !dictionary?
Message was sent while issue was closed.
On 2015/03/26 10:12:35, Bernhard Bauer wrote: > Post-commit drive-by review! There are some style nits in here: > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > File > components/content_settings/core/browser/content_settings_default_provider.cc > (right): > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:42: > {prefs::kDefaultCookiesSetting, CONTENT_SETTING_ALLOW, true}, > Indent these lines by two spaces, and add a space after the opening brace and > before the closing brace. > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:121: > for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) { > Use an int unless you really really need size_t. > > Also, I *think* you might be able to iterate over the array with the C++11 > syntax, then you don't need the index variable at all. > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:305: > return (value == NULL || > Use nullptr, or just `!value` (which would also be consistent with the style in > ChangeSetting). > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:377: > for (ValueMap::iterator it = dictionary->begin(); > Use a C++11-style loop? > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:428: > return make_scoped_ptr((base::Value*)NULL); > Use nullptr; that might even allow you to get rid of the cast. (Also, if you > were to keep the cast, it should be static_cast<base::Value*>.) > > Also also, it might make sense to extract this logic into a function that > creates a base::Value from a ContentSetting (we do this in a couple of other > places as well). > > https://codereview.chromium.org/1004733003/diff/200001/components/content_set... > components/content_settings/core/browser/content_settings_default_provider.cc:460: > ValueMap* value_map = new ValueMap(); > Put this into a scoped_ptr, and return it immediately if !dictionary? Thanks, Bernhard! I'll address those ASAP (in a separate CL, obviously :) ). And I'll add you to the follow-up CL about splitting PrefProvider.
Message was sent while issue was closed.
Hi Bernhard, I addressed your comments in a follow-up CL 1038203003. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:42: {prefs::kDefaultCookiesSetting, CONTENT_SETTING_ALLOW, true}, On 2015/03/26 10:12:34, Bernhard (OOO) wrote: > Indent these lines by two spaces, and add a space after the opening brace and > before the closing brace. Done. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:121: for (size_t i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) { On 2015/03/26 10:12:34, Bernhard (OOO) wrote: > Use an int unless you really really need size_t. > > Also, I *think* you might be able to iterate over the array with the C++11 > syntax, then you don't need the index variable at all. Ah. This is a leftover. I originally used size_t, because I had i < arraysize(kDefaultSettings). But there is no need, as we have the assert anyway. All the code doing something "for all content types" I encountered seems to use normal "for(int ...)", so I'll keep that. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:305: return (value == NULL || On 2015/03/26 10:12:34, Bernhard (OOO) wrote: > Use nullptr, or just `!value` (which would also be consistent with the style in > ChangeSetting). Done. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:377: for (ValueMap::iterator it = dictionary->begin(); On 2015/03/26 10:12:34, Bernhard (OOO) wrote: > Use a C++11-style loop? Done. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:428: return make_scoped_ptr((base::Value*)NULL); On 2015/03/26 10:12:35, Bernhard (OOO) wrote: > Use nullptr; that might even allow you to get rid of the cast. (Also, if you > were to keep the cast, it should be static_cast<base::Value*>.) > > Also also, it might make sense to extract this logic into a function that > creates a base::Value from a ContentSetting (we do this in a couple of other > places as well). Done. Cast is necessary here even with nullptr. Anyway, I made a separate function. https://codereview.chromium.org/1004733003/diff/200001/components/content_set... components/content_settings/core/browser/content_settings_default_provider.cc:460: ValueMap* value_map = new ValueMap(); On 2015/03/26 10:12:34, Bernhard (OOO) wrote: > Put this into a scoped_ptr, and return it immediately if !dictionary? Done. I just found it more elegant to create the scoped_ptr on return than to Pass() it on return. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
