Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(786)

Issue 2938163002: Store base::Value in ContentSettingPatternSource instead of an enum (Closed)

Created:
3 years, 6 months ago by tbansal1
Modified:
3 years, 5 months ago
CC:
chromium-reviews, extensions-reviews_chromium.org, dominickn+watch_chromium.org, qsr+mojo_chromium.org, droger+watchlist_chromium.org, viettrungluu+watch_chromium.org, tfarina, sdefresne+watchlist_chromium.org, msramek+watch_chromium.org, raymes+watch_chromium.org, abarth-chromium, Aaron Boodman, yzshen+watch_chromium.org, blundell+watchlist_chromium.org, chromium-apps-reviews_chromium.org, markusheintz_, darin (slow to review)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Store base::Value in ContentSettingPatternSource instead of an enum. This makes it possible for the consumers of the content settings API to store different types of information. Additional mechanical change: Move some of the methods from content_settings/core/browser/content_settings_utils.h to content_settings/core/common/content_settings_utils.h. This makes it possible to access the moved methods from chrome/renderer. BUG=735518 TBR=sdefresne@chromium.org Review-Url: https://codereview.chromium.org/2938163002 Cr-Commit-Position: refs/heads/master@{#483829} Committed: https://chromium.googlesource.com/chromium/src/+/08a0e3ee7ad01c643155daf1c95a70954fe05c00

Patch Set 1 : ps #

Total comments: 11

Patch Set 2 : Rebased #

Patch Set 3 : moar rebase, raymes comments #

Total comments: 10

Patch Set 4 : raymes comments #

Total comments: 4

Patch Set 5 : Rebased #

Patch Set 6 : nasko comments #

Total comments: 2

Patch Set 7 : nasko comments #

Total comments: 6

Patch Set 8 : lazyboy comments #

Patch Set 9 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+260 lines, -189 lines) Patch
M chrome/browser/android/preferences/pref_service_bridge.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/android/preferences/website_preference_bridge.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc View 1 2 5 chunks +8 lines, -8 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 2 chunks +14 lines, -19 lines 0 comments Download
M chrome/browser/content_settings/content_settings_origin_identifier_value_map_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/content_settings/content_settings_policy_provider_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/content_settings/content_settings_supervised_provider_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/engagement/important_sites_util.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/engagement/site_engagement_service.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/content_settings/content_settings_store.cc View 1 2 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/extension_special_storage_policy.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/notifications/desktop_notification_profile_util_unittest.cc View 1 2 2 chunks +8 lines, -11 lines 0 comments Download
M chrome/browser/notifications/notification_channels_provider_android.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/notification_channels_provider_android_unittest.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/notification_interactive_uitest_support.cc View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/plugins/plugin_utils.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/content_settings/content_setting_bubble_model.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/page_info/page_info.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/webui/options/content_settings_handler.cc View 1 2 3 4 2 chunks +4 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/settings/site_settings_handler.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/webui/site_settings_helper.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M chrome/renderer/content_settings_observer.cc View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/renderer/content_settings_observer_browsertest.cc View 1 2 14 chunks +46 lines, -60 lines 0 comments Download
M components/content_settings/core/browser/content_settings_default_provider.cc View 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_pref.cc View 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_utils.h View 1 2 3 4 5 6 7 3 chunks +0 lines, -18 lines 0 comments Download
M components/content_settings/core/browser/content_settings_utils.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -36 lines 0 comments Download
M components/content_settings/core/browser/cookie_settings.cc View 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.cc View 1 2 5 chunks +8 lines, -5 lines 0 comments Download
M components/content_settings/core/common/BUILD.gn View 3 chunks +8 lines, -0 lines 0 comments Download
M components/content_settings/core/common/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/common/OWNERS View 1 chunk +3 lines, -0 lines 0 comments Download
M components/content_settings/core/common/content_settings.h View 1 2 2 chunks +10 lines, -2 lines 0 comments Download
M components/content_settings/core/common/content_settings.cc View 1 2 3 2 chunks +24 lines, -6 lines 0 comments Download
M components/content_settings/core/common/content_settings.mojom View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M components/content_settings/core/common/content_settings_struct_traits.h View 1 2 3 4 5 6 2 chunks +6 lines, -2 lines 0 comments Download
M components/content_settings/core/common/content_settings_struct_traits.cc View 1 chunk +2 lines, -1 line 0 comments Download
A components/content_settings/core/common/content_settings_utils.h View 1 2 1 chunk +27 lines, -0 lines 0 comments Download
A components/content_settings/core/common/content_settings_utils.cc View 1 2 3 1 chunk +47 lines, -0 lines 0 comments Download
M components/content_settings/core/test/content_settings_test_utils.cc View 1 chunk +1 line, -0 lines 0 comments Download
M ios/chrome/browser/ui/settings/block_popups_collection_view_controller.mm View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 205 (180 generated)
tbansal1
raymes: PTAL. The core changes are in content_settings/core/browser/content_settings_utils.*, content_settings/core/common/content_settings_utils.*, components/content_settings/core/common/content_settings.*. Rest are mechanical changes. I ...
3 years, 6 months ago (2017-06-21 03:46:02 UTC) #112
raymes
Overall the approach looks good. Some minor comments. Thanks :) https://codereview.chromium.org/2938163002/diff/510001/components/content_settings/core/browser/host_content_settings_map.cc File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2938163002/diff/510001/components/content_settings/core/browser/host_content_settings_map.cc#newcode764 ...
3 years, 6 months ago (2017-06-22 03:41:26 UTC) #113
tbansal1
raymes: ptal. Thanks. https://codereview.chromium.org/2938163002/diff/510001/components/content_settings/core/common/content_settings.cc File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2938163002/diff/510001/components/content_settings/core/common/content_settings.cc#newcode12 components/content_settings/core/common/content_settings.cc:12: //#include "components/content_settings/core/browser/content_settings_utils.h" On 2017/06/22 03:41:26, raymes ...
3 years, 6 months ago (2017-06-22 21:46:16 UTC) #136
raymes
On 2017/06/22 21:46:16, tbansal1 wrote: > raymes: ptal. Thanks. > > https://codereview.chromium.org/2938163002/diff/510001/components/content_settings/core/common/content_settings.cc > File components/content_settings/core/common/content_settings.cc ...
3 years, 5 months ago (2017-06-26 05:53:48 UTC) #139
raymes
lg just 2 comments https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h#newcode61 components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; I was just ...
3 years, 5 months ago (2017-06-27 03:49:33 UTC) #140
tbansal1
raymes: qq below. https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h#newcode61 components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; On 2017/06/27 03:49:33, raymes ...
3 years, 5 months ago (2017-06-27 07:51:38 UTC) #141
raymes
https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h#newcode61 components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; My understanding is that with the current ...
3 years, 5 months ago (2017-06-27 08:59:55 UTC) #142
tbansal1
https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h#newcode61 components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; On 2017/06/27 08:59:54, raymes wrote: > My ...
3 years, 5 months ago (2017-06-27 09:56:34 UTC) #143
tbansal1
https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h#newcode61 components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; On 2017/06/27 09:56:34, tbansal1 wrote: > On ...
3 years, 5 months ago (2017-06-27 10:29:53 UTC) #144
raymes
On 2017/06/27 10:29:53, tbansal1 wrote: > https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h > File components/content_settings/core/common/content_settings.h (right): > > https://codereview.chromium.org/2938163002/diff/600001/components/content_settings/core/common/content_settings.h#newcode61 > ...
3 years, 5 months ago (2017-06-28 01:15:44 UTC) #145
tbansal1
nasko: ptal at .mojom, traits, .typemap, OWNERS in components/content_settings/core/common/. For reference, the struct that got ...
3 years, 5 months ago (2017-06-28 14:16:41 UTC) #156
nasko
Looks mostly good, couple of small questions. https://codereview.chromium.org/2938163002/diff/620001/components/content_settings/core/common/content_settings.mojom File components/content_settings/core/common/content_settings.mojom (right): https://codereview.chromium.org/2938163002/diff/620001/components/content_settings/core/common/content_settings.mojom#newcode64 components/content_settings/core/common/content_settings.mojom:64: mojo.common.mojom.Value? setting_value; ...
3 years, 5 months ago (2017-06-28 20:59:37 UTC) #157
tbansal1
nasko: ptal. Thanks. https://codereview.chromium.org/2938163002/diff/620001/components/content_settings/core/common/content_settings.mojom File components/content_settings/core/common/content_settings.mojom (right): https://codereview.chromium.org/2938163002/diff/620001/components/content_settings/core/common/content_settings.mojom#newcode64 components/content_settings/core/common/content_settings.mojom:64: mojo.common.mojom.Value? setting_value; On 2017/06/28 20:59:37, nasko ...
3 years, 5 months ago (2017-06-29 15:57:53 UTC) #169
nasko
IPC LGTM with a nit. https://codereview.chromium.org/2938163002/diff/680001/components/content_settings/core/common/content_settings_struct_traits.h File components/content_settings/core/common/content_settings_struct_traits.h (right): https://codereview.chromium.org/2938163002/diff/680001/components/content_settings/core/common/content_settings_struct_traits.h#newcode14 components/content_settings/core/common/content_settings_struct_traits.h:14: #include "mojo/common/common_custom_types_struct_traits.h" If this ...
3 years, 5 months ago (2017-06-29 18:43:21 UTC) #170
tbansal1
maria: ptal at chrome/browser/android/ lazyboy: chrome/browser/extensions/ dimich: chrome/browser/notifications/ csharrison: subresource_filter Thanks!! https://codereview.chromium.org/2938163002/diff/680001/components/content_settings/core/common/content_settings_struct_traits.h File components/content_settings/core/common/content_settings_struct_traits.h (right): ...
3 years, 5 months ago (2017-06-29 22:57:46 UTC) #180
tbansal1
sdefresne: ptal at ios/
3 years, 5 months ago (2017-06-29 22:59:10 UTC) #182
Maria
lgtm for chrome/browser/android
3 years, 5 months ago (2017-06-30 00:08:56 UTC) #183
Charlie Harrison
LGTM
3 years, 5 months ago (2017-06-30 00:21:58 UTC) #184
tbansal1
thestig: ptal at thestig: chrome/browser/plugins/plugin_utils.cc, chrome/browser/ui/content_settings/content_setting_bubble_model.cc, chrome/browser/ui/webui/site_settings_helper.cc, chrome/renderer/content_settings_observer.cc, chrome/renderer/content_settings_observer_browsertest.cc Thanks.
3 years, 5 months ago (2017-06-30 17:45:41 UTC) #186
Lei Zhang
On 2017/06/30 17:45:41, tbansal1 wrote: > thestig: ptal at thestig: chrome/browser/plugins/plugin_utils.cc, > chrome/browser/ui/content_settings/content_setting_bubble_model.cc, > chrome/browser/ui/webui/site_settings_helper.cc, ...
3 years, 5 months ago (2017-06-30 17:55:24 UTC) #187
lazyboy
lgtm w/ couple of drive by-s and one possible extension specific issue to look after. ...
3 years, 5 months ago (2017-06-30 18:11:40 UTC) #188
tbansal1
thestig: glad you are good :) sorry for the copypasta typo. https://codereview.chromium.org/2938163002/diff/700001/chrome/browser/extensions/api/content_settings/content_settings_store.cc File chrome/browser/extensions/api/content_settings/content_settings_store.cc (right): ...
3 years, 5 months ago (2017-06-30 18:32:31 UTC) #189
Dmitry Titov
notifications lgtm
3 years, 5 months ago (2017-06-30 20:01:57 UTC) #196
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2938163002/740001
3 years, 5 months ago (2017-06-30 21:23:30 UTC) #202
commit-bot: I haz the power
3 years, 5 months ago (2017-06-30 21:30:32 UTC) #205
Message was sent while issue was closed.
Committed patchset #9 (id:740001) as
https://chromium.googlesource.com/chromium/src/+/08a0e3ee7ad01c643155daf1c95a...

Powered by Google App Engine
This is Rietveld 408576698