|
|
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. |
DescriptionStore 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 #Messages
Total messages: 205 (180 generated)
Description was changed from ========== w iw w w w w w w w w w w w w w w Not working not working w w w w w w w w BUG= ========== to ========== Content settings refactor BUG= ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:220001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:240001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:260001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:280001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:410001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #3 (id:470001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Content settings refactor BUG= ========== to ========== Content settings refactor 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG= ==========
Description was changed from ========== Content settings refactor 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG= ========== to ========== Content settings refactor 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG= ==========
Description was changed from ========== Content settings refactor 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG= ========== to ========== Store base::Value in ContentSettingPatternSource. 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #3 (id:490001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:430001) has been deleted
Patchset #1 (id:450001) has been deleted
tbansal@chromium.org changed reviewers: + raymes@chromium.org
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 am going to file the relevant bugs in the morning PST.
Overall the approach looks good. Some minor comments. Thanks :) https://codereview.chromium.org/2938163002/diff/510001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:764: kProviderNamesSourceMap[provider_type].provider_name, incognito)); I'm not sure I understand the changes here. Could we just preserve the existing logic? i.e. base::Value setting_value; if ... { ... setting_value = base::Value(CONTENT_SETTING_ALLOW); } else { setting_value = base::Value(*rule.value); } ... https://codereview.chromium.org/2938163002/diff/510001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:12: //#include "components/content_settings/core/browser/content_settings_utils.h" Is this needed? https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:81: const base::Value& setting_value, To avoid additional copies, should we pass in a unique_ptr on construction? The dictionaries could potentially be large. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:98: setting_value = other.setting_value->CreateDeepCopy(); I think CreateDeepCopy is deprecated? I think the base::Value copy constructor is used instead. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:110: return *this; nit: please keep the order of assignments the same in these and match the order of the member variables. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:113: ContentSettingPatternSource::~ContentSettingPatternSource() {} Did you consider exposing a GetContentSetting() function on this class rather than calling ValueToContentSetting everywhere?
Description was changed from ========== Store base::Value in ContentSettingPatternSource. 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG= ========== to ========== Store base::Value in ContentSettingPatternSource. 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG=735518 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:550001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:570001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:580043) has been deleted
raymes: ptal. Thanks. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2938163002/diff/510001/components/content_set... 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 wrote: > Is this needed? Obviously not :P https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:81: const base::Value& setting_value, On 2017/06/22 03:41:26, raymes wrote: > To avoid additional copies, should we pass in a unique_ptr on construction? The > dictionaries could potentially be large. Done. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:98: setting_value = other.setting_value->CreateDeepCopy(); On 2017/06/22 03:41:26, raymes wrote: > I think CreateDeepCopy is deprecated? I think the base::Value copy constructor > is used instead. Done. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:110: return *this; On 2017/06/22 03:41:26, raymes wrote: > nit: please keep the order of assignments the same in these and match the order > of the member variables. Done. https://codereview.chromium.org/2938163002/diff/510001/components/content_set... components/content_settings/core/common/content_settings.cc:113: ContentSettingPatternSource::~ContentSettingPatternSource() {} On 2017/06/22 03:41:26, raymes wrote: > Did you consider exposing a GetContentSetting() function on this class rather > than calling ValueToContentSetting everywhere? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/22 21:46:16, tbansal1 wrote: > raymes: ptal. Thanks. > > https://codereview.chromium.org/2938163002/diff/510001/components/content_set... > File components/content_settings/core/common/content_settings.cc (right): > > https://codereview.chromium.org/2938163002/diff/510001/components/content_set... > 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 wrote: > > Is this needed? > > Obviously not :P > > https://codereview.chromium.org/2938163002/diff/510001/components/content_set... > components/content_settings/core/common/content_settings.cc:81: const > base::Value& setting_value, > On 2017/06/22 03:41:26, raymes wrote: > > To avoid additional copies, should we pass in a unique_ptr on construction? > The > > dictionaries could potentially be large. > > Done. > > https://codereview.chromium.org/2938163002/diff/510001/components/content_set... > components/content_settings/core/common/content_settings.cc:98: setting_value = > other.setting_value->CreateDeepCopy(); > On 2017/06/22 03:41:26, raymes wrote: > > I think CreateDeepCopy is deprecated? I think the base::Value copy constructor > > is used instead. > > Done. > > https://codereview.chromium.org/2938163002/diff/510001/components/content_set... > components/content_settings/core/common/content_settings.cc:110: return *this; > On 2017/06/22 03:41:26, raymes wrote: > > nit: please keep the order of assignments the same in these and match the > order > > of the member variables. > > Done. > > https://codereview.chromium.org/2938163002/diff/510001/components/content_set... > components/content_settings/core/common/content_settings.cc:113: > ContentSettingPatternSource::~ContentSettingPatternSource() {} > On 2017/06/22 03:41:26, raymes wrote: > > Did you consider exposing a GetContentSetting() function on this class rather > > than calling ValueToContentSetting everywhere? > > Done. Sorry for the delay on this! I will take a look tomorrow.
lg just 2 comments https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; I was just chatting with mgiuca@ about this. It looks like it's now safe to store base::Value's directly as members. This would allow us to avoid defining the copy constructor and assignment operator. That means that we can change the constructor back to pass a base::Value by reference (sorry for the churn) and just set up the member in the constructor. I realized we're not getting any benefit from it right now anyway because we're only ever passing in int's as the base::Value (and we're doing a deep copy in the constructor anyway). https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings_utils.cc (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings_utils.cc:9: #include "components/content_settings/core/common/content_settings_types.h" nit: is this needed?
raymes: qq below. https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; On 2017/06/27 03:49:33, raymes wrote: > I was just chatting with mgiuca@ about this. It looks like it's now safe to > store base::Value's directly as members. This would allow us to avoid defining > the copy constructor and assignment operator. That means that we can change the > constructor back to pass a base::Value by reference (sorry for the churn) and > just set up the member in the constructor. I realized we're not getting any > benefit from it right now anyway because we're only ever passing in int's as the > base::Value (and we're doing a deep copy in the constructor anyway). qq: With unique_ptr, it seems that the ctor requires only one deep copy (because the created ContentSettingPatternSource object simply takes the ownership of the already constructed base::Value unique_ptr). When passing the ref, and having the base::Value as the member, it seems two deep copies would be required. Is my understanding correct? Is the extra overhead associated with not using unique_ptr a concern?
https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings.h:61: std::unique_ptr<base::Value> setting_value; My understanding is that with the current patchset, a copy will be created in the MakeUnique call inside the constructor. That's the only copy at present. We could actually eliminate that copy since the ContentSettingsPatternSource takes ownership of the pointer. If we passed by ref, the only copy would be in the initialization of the member variable (which invokes the copy constructor). This is one more copy than is strictly necessary but I think worth the tradeoff in simplicity. The other alternative would be to keep the unique_ptr but not define a copy constructor/assignment operator and only allow the struct to be moved. This would remove all copies. But I suspect this may require additional changes in the code and I'm not sure how hard it would be depending on how the struct is used.
https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... 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 understanding is that with the current patchset, a copy will be created in > the MakeUnique call inside the constructor. That's the only copy at present. We > could actually eliminate that copy since the ContentSettingsPatternSource takes > ownership of the pointer. > > If we passed by ref, the only copy would be in the initialization of the member > variable (which invokes the copy constructor). This is one more copy than is > strictly necessary but I think worth the tradeoff in simplicity. > > The other alternative would be to keep the unique_ptr but not define a copy > constructor/assignment operator and only allow the struct to be moved. This > would remove all copies. But I suspect this may require additional changes in > the code and I'm not sure how hard it would be depending on how the struct is > used. > I see. Storing the base::value directly as a member (instead of unique_ptr) may make the mojo interface less unreadable. mojo/common/values.typemap maps mojo.common.mojom.Value to a std::unique_ptr<base::Value>. That makes it possible for components/content_settings/core/common/content_settings.typemap to directly map the mojo CSPS (ContentSettingPatternSOurce) to native CSPS. IIUC, changing the native CSPS to store base::value directly would require defining a new way to serialize/unserialize base::value. I would look at the possibility of avoiding the extra copy in the copy/assignment ctors which seems like the best approach anyways.
https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... 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 2017/06/27 08:59:54, raymes wrote: > > My understanding is that with the current patchset, a copy will be created in > > the MakeUnique call inside the constructor. That's the only copy at present. > We > > could actually eliminate that copy since the ContentSettingsPatternSource > takes > > ownership of the pointer. > > > > If we passed by ref, the only copy would be in the initialization of the > member > > variable (which invokes the copy constructor). This is one more copy than is > > strictly necessary but I think worth the tradeoff in simplicity. > > > > The other alternative would be to keep the unique_ptr but not define a copy > > constructor/assignment operator and only allow the struct to be moved. This > > would remove all copies. But I suspect this may require additional changes in > > the code and I'm not sure how hard it would be depending on how the struct is > > used. > > > > I see. Storing the base::value directly as a member (instead of unique_ptr) may > make the mojo interface less unreadable. mojo/common/values.typemap maps > mojo.common.mojom.Value to a std::unique_ptr<base::Value>. That makes it > possible for components/content_settings/core/common/content_settings.typemap to > directly map the mojo CSPS (ContentSettingPatternSOurce) to native CSPS. > > IIUC, changing the native CSPS to store base::value directly would require > defining a new way to serialize/unserialize base::value. > > I would look at the possibility of avoiding the extra copy in the > copy/assignment ctors which seems like the best approach anyways. Another straight-forward option is to keep the current patch, but add a DCHECK somewhere that base::Value is of type int. For the new content setting, I only need to store a single 64 bit int (a timestamp to be precise). With int's, the overhead of copies is not going to be prohibitive. WDYT?
On 2017/06/27 10:29:53, tbansal1 wrote: > https://codereview.chromium.org/2938163002/diff/600001/components/content_set... > File components/content_settings/core/common/content_settings.h (right): > > https://codereview.chromium.org/2938163002/diff/600001/components/content_set... > 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 2017/06/27 08:59:54, raymes wrote: > > > My understanding is that with the current patchset, a copy will be created > in > > > the MakeUnique call inside the constructor. That's the only copy at present. > > We > > > could actually eliminate that copy since the ContentSettingsPatternSource > > takes > > > ownership of the pointer. > > > > > > If we passed by ref, the only copy would be in the initialization of the > > member > > > variable (which invokes the copy constructor). This is one more copy than is > > > strictly necessary but I think worth the tradeoff in simplicity. > > > > > > The other alternative would be to keep the unique_ptr but not define a copy > > > constructor/assignment operator and only allow the struct to be moved. This > > > would remove all copies. But I suspect this may require additional changes > in > > > the code and I'm not sure how hard it would be depending on how the struct > is > > > used. > > > > > > > I see. Storing the base::value directly as a member (instead of unique_ptr) > may > > make the mojo interface less unreadable. mojo/common/values.typemap maps > > mojo.common.mojom.Value to a std::unique_ptr<base::Value>. That makes it > > possible for components/content_settings/core/common/content_settings.typemap > to > > directly map the mojo CSPS (ContentSettingPatternSOurce) to native CSPS. > > > > IIUC, changing the native CSPS to store base::value directly would require > > defining a new way to serialize/unserialize base::value. Ah thanks for explaining this. I tried to make the type movable instead of copyable but it doesn't work because the mojo bindings currently require it to be copied. Given all of the above, I think it's ok to keep it as is. Please just address the other nit above and I've added one additional nit here. Thanks for your patience :) lgtm https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings.cc:100: incognito = other.incognito; nit: I think we can reuse code here by just calling *this = other; https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings_utils.cc (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings_utils.cc:9: #include "components/content_settings/core/common/content_settings_types.h" On 2017/06/27 03:49:33, raymes wrote: > nit: is this needed? Please also remove this if it's not needed.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Store base::Value in ContentSettingPatternSource. 1. 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. 2. Store base::Value in ContentSettingPatternSource struct instead of ContentSetting enum. BUG=735518 ========== to ========== 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 ==========
tbansal@chromium.org changed reviewers: + nasko@chromium.org
nasko: ptal at .mojom, traits, .typemap, OWNERS in components/content_settings/core/common/. For reference, the struct that got changed (and triggered changes in all other files in this CL) is in components/content_settings/core/common/content_settings.h. Thanks. https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings.cc:100: incognito = other.incognito; On 2017/06/28 01:15:43, raymes wrote: > nit: I think we can reuse code here by just calling > *this = other; Done. https://codereview.chromium.org/2938163002/diff/600001/components/content_set... File components/content_settings/core/common/content_settings_utils.cc (right): https://codereview.chromium.org/2938163002/diff/600001/components/content_set... components/content_settings/core/common/content_settings_utils.cc:9: #include "components/content_settings/core/common/content_settings_types.h" On 2017/06/28 01:15:43, raymes wrote: > On 2017/06/27 03:49:33, raymes wrote: > > nit: is this needed? > > Please also remove this if it's not needed. Done.
Looks mostly good, couple of small questions. https://codereview.chromium.org/2938163002/diff/620001/components/content_set... File components/content_settings/core/common/content_settings.mojom (right): https://codereview.chromium.org/2938163002/diff/620001/components/content_set... components/content_settings/core/common/content_settings.mojom:64: mojo.common.mojom.Value? setting_value; Is the setting value really optional? https://codereview.chromium.org/2938163002/diff/620001/components/content_set... File components/content_settings/core/common/content_settings.typemap (right): https://codereview.chromium.org/2938163002/diff/620001/components/content_set... components/content_settings/core/common/content_settings.typemap:24: "//mojo/common/common_custom_types_struct_traits.h", Which part requires this include and the corresponding deps above?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:660001) has been deleted
nasko: ptal. Thanks. https://codereview.chromium.org/2938163002/diff/620001/components/content_set... File components/content_settings/core/common/content_settings.mojom (right): https://codereview.chromium.org/2938163002/diff/620001/components/content_set... components/content_settings/core/common/content_settings.mojom:64: mojo.common.mojom.Value? setting_value; On 2017/06/28 20:59:37, nasko (slow) wrote: > Is the setting value really optional? Done. https://codereview.chromium.org/2938163002/diff/620001/components/content_set... File components/content_settings/core/common/content_settings.typemap (right): https://codereview.chromium.org/2938163002/diff/620001/components/content_set... components/content_settings/core/common/content_settings.typemap:24: "//mojo/common/common_custom_types_struct_traits.h", On 2017/06/28 20:59:37, nasko (slow) wrote: > Which part requires this include and the corresponding deps above? Done.
IPC LGTM with a nit. https://codereview.chromium.org/2938163002/diff/680001/components/content_set... File components/content_settings/core/common/content_settings_struct_traits.h (right): https://codereview.chromium.org/2938163002/diff/680001/components/content_set... components/content_settings/core/common/content_settings_struct_traits.h:14: #include "mojo/common/common_custom_types_struct_traits.h" If this wasn't required in the typemap, why is it required here?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tbansal@chromium.org changed reviewers: + csharrison@chromium.org, dimich@chromium.org, lazyboy@chromium.org, mariakhomenko@chromium.org
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_set... File components/content_settings/core/common/content_settings_struct_traits.h (right): https://codereview.chromium.org/2938163002/diff/680001/components/content_set... components/content_settings/core/common/content_settings_struct_traits.h:14: #include "mojo/common/common_custom_types_struct_traits.h" On 2017/06/29 18:43:20, nasko (slow) wrote: > If this wasn't required in the typemap, why is it required here? values_struct_traits.h is needed but common_custom_types_struct_traits.h is not.
tbansal@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne: ptal at ios/
lgtm for chrome/browser/android
LGTM
tbansal@chromium.org changed reviewers: + thestig@chromium.org
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.
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, > chrome/renderer/content_settings_observer.cc, > chrome/renderer/content_settings_observer_browsertest.cc /me looks at myself... lgtm /me looks at the files... lgtm
lgtm w/ couple of drive by-s and one possible extension specific issue to look after. https://codereview.chromium.org/2938163002/diff/700001/chrome/browser/extensi... File chrome/browser/extensions/api/content_settings/content_settings_store.cc (right): https://codereview.chromium.org/2938163002/diff/700001/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_store.cc:249: std::unique_ptr<base::ListValue> ContentSettingsStore::GetSettingsForExtension( Since this touches persistent extension preference, I'd carefully check if extension's content settings preferences stored before the CL can be correctly retrieved by this function after the CL. From the look of it, it (extension preference) doesn't seem to have changed at all though. https://codereview.chromium.org/2938163002/diff/700001/components/content_set... File components/content_settings/core/browser/content_settings_utils.h (right): https://codereview.chromium.org/2938163002/diff/700001/components/content_set... components/content_settings/core/browser/content_settings_utils.h:8: #include <memory> Remove since std::unique_ptr is gone. https://codereview.chromium.org/2938163002/diff/700001/components/content_set... File components/content_settings/core/common/content_settings_utils.h (right): https://codereview.chromium.org/2938163002/diff/700001/components/content_set... components/content_settings/core/common/content_settings_utils.h:22: // a valid content setting. Otherwise, returns a nullptr. drive by: Isn't CONTENT_SETTING_DEFAULT a valid content setting? This function will return nullptr in that case, whereas the comment calls it invalid.
thestig: glad you are good :) sorry for the copypasta typo. https://codereview.chromium.org/2938163002/diff/700001/chrome/browser/extensi... File chrome/browser/extensions/api/content_settings/content_settings_store.cc (right): https://codereview.chromium.org/2938163002/diff/700001/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_store.cc:249: std::unique_ptr<base::ListValue> ContentSettingsStore::GetSettingsForExtension( On 2017/06/30 18:11:40, lazyboy wrote: > Since this touches persistent extension preference, I'd carefully check if > extension's content settings preferences stored before the CL can be correctly > retrieved by this function after the CL. > > From the look of it, it (extension preference) doesn't seem to have changed at > all though. Correct, this CL should not affect any consumers. This CL is not changing what is stored on the disk. The only change is in an intermediate data structure that is populated from the contents stored on the disk. https://codereview.chromium.org/2938163002/diff/700001/components/content_set... File components/content_settings/core/browser/content_settings_utils.h (right): https://codereview.chromium.org/2938163002/diff/700001/components/content_set... components/content_settings/core/browser/content_settings_utils.h:8: #include <memory> On 2017/06/30 18:11:40, lazyboy wrote: > Remove since std::unique_ptr is gone. Done. https://codereview.chromium.org/2938163002/diff/700001/components/content_set... File components/content_settings/core/common/content_settings_utils.h (right): https://codereview.chromium.org/2938163002/diff/700001/components/content_set... components/content_settings/core/common/content_settings_utils.h:22: // a valid content setting. Otherwise, returns a nullptr. On 2017/06/30 18:11:40, lazyboy wrote: > drive by: > Isn't CONTENT_SETTING_DEFAULT a valid content setting? This function will return > nullptr in that case, whereas the comment calls it invalid. DEFAULT is encoded as a null value. In the function that reverse maps in components/content_settings/core/common/content_settings_utils.cc, there is also a comment which says: "Note that |CONTENT_SETTING_DEFAULT| is encoded as a NULL value, so it is not allowed as an integer value."
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
notifications lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, nasko@chromium.org, mariakhomenko@chromium.org, thestig@chromium.org, csharrison@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2938163002/#ps740001 (title: "rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 740001, "attempt_start_ts": 1498857784558460, "parent_rev": "cd18a26eb800d22aa951c00f42f67d98db3c8b15", "commit_rev": "08a0e3ee7ad01c643155daf1c95a70954fe05c00"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/08a0e3ee7ad01c643155daf1c95a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:740001) as https://chromium.googlesource.com/chromium/src/+/08a0e3ee7ad01c643155daf1c95a... |