Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234133003:
[HBD] Call PurgePluginListCache() whenever plugin content settings change (Closed)
Description[HBD] Call PurgePluginListCache() whenever plugin content settings change
Call PluginService::PurgePluginListCache() whenever plugin content settings change. This is made to ensure that Page's PluginData instance is updated for the next navigation.
BUG=626728
Committed: https://crrev.com/3e5d33572d42429457897450228f773d30e7af17
Cr-Commit-Position: refs/heads/master@{#413856}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use HCSMObserver to observe for content settings changes #
Total comments: 22
Patch Set 3 : Fix style nits #
Total comments: 4
Patch Set 4 : Changes per bauerb@'s comments #
Total comments: 4
Patch Set 5 : Rebase #Patch Set 6 : Changes per Lei Zhang's comments #Patch Set 7 : Changes per tommycli@'s comments #Patch Set 8 : Add endline after namespace #Patch Set 9 : Rebase #Patch Set 10 : Rebase and tracked patchset change #
Depends on Patchset: Messages
Total messages: 35 (18 generated)
Description was changed from ========== [HBD] Call PurgePluginListCache() whenever plugin content settings changes Call PluginService::PurgePluginListCache() whenever plugin content settings changes. This is made to ensure that Page's PluginData instance is updated for the next navigation. BUG=626728 ========== to ========== [HBD] Call PurgePluginListCache() whenever plugin content settings change Call PluginService::PurgePluginListCache() whenever plugin content settings change. This is made to ensure that Page's PluginData instance is updated for the next navigation. BUG=626728 ==========
trizzofo@google.com changed reviewers: + bauerb@chromium.org, tommycli@chromium.org
bauerb, ptal. We are now trying to purge the plugin list whenever the settings change. As you can see, in this CL I added ChromePluginServiceFilter as an observer of HostContentSettingsMap to trigger OnContentSettingChanged(). However, I couldn't find a good way of retrieving the profile associated with the settings change, and passing a nullptr to PurgePluginListCache purges all plugin list caches for all profiles. Do you have any suggestions on how to do that? I would really appreciate it. Thanks!
On 2016/08/11 01:20:02, trizzofo wrote: > bauerb, ptal. > We are now trying to purge the plugin list whenever the settings change. As you > can see, in this CL I added ChromePluginServiceFilter as an observer of > HostContentSettingsMap to trigger OnContentSettingChanged(). However, I couldn't > find a good way of retrieving the profile associated with the settings change, > and passing a nullptr to PurgePluginListCache purges all plugin list caches for > all profiles. Do you have any suggestions on how to do that? I would really > appreciate it. > Thanks! Hm, when you register as an observer, you do know which profile the HCSM belongs to, so you could keep a collection of separate observers that each keep a profile pointer? (Or a collection of callbacks that have the profile bound, which really is equivalent.) Actually, we already have two maps that both use the ResourceContext as a key -- we could merge them into a single map that contains a struct as value with all the per-profile information (HCSM, PluginPrefs, and then the HCSMObserver as well). https://codereview.chromium.org/2234133003/diff/1/chrome/browser/plugins/chro... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2234133003/diff/1/chrome/browser/plugins/chro... chrome/browser/plugins/chrome_plugin_service_filter.cc:174: std::string resource_identifier) { You could return if the |content_type| is not PLUGINS.
It basically looks good to me. I have a variety of coding style nits. See below. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:8: #include <string> Since you included string in .h file you don't need it here. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:73: context_info.hcsm_observer.SetProfile(profile); I think the observer should only be constructed and added if the kPreferHtmlOverPlugins feature is on. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:256: std::string resource_identifier) { DCHECK that the feature is on here too. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:21: #include "components/content_settings/core/browser/host_content_settings_map.h" These can still be forward declared right? https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:83: class HCSMObserver : public content_settings::Observer { Can you make forward declare it here, then put the whole implementation within the anonymous namespace of the cc file? https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:83: class HCSMObserver : public content_settings::Observer { Also we don't really use abbreviations in Chromium code. Maybe a ProfileContentSettingObserver? https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:85: HCSMObserver(); If you can forward declare it, i would just set the profile within the constructor. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:102: ContextInfo(const ContextInfo& other); Hmm... I think you never want to copy this struct. Can you have no constructors or destructors? (Assuming you change HCSMObserver to a unique_ptr) https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:107: HCSMObserver hcsm_observer; If you forward declare it you may need to make this a unique_ptr, but that's OK with me.
https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:81: resource_context_map_[context].host_content_settings_map->RemoveObserver( You could put this into the destructor of the observer itself. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:107: HCSMObserver hcsm_observer; On 2016/08/11 23:31:36, tommycli wrote: > If you forward declare it you may need to make this a unique_ptr, but that's OK > with me. Actually, you might be able to forward-declare *this* struct as well, and then have both definitions in the .cc file, so you could directly have the observer as a member again :)
https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:107: HCSMObserver hcsm_observer; On 2016/08/12 09:35:55, Bernhard Bauer wrote: > On 2016/08/11 23:31:36, tommycli wrote: > > If you forward declare it you may need to make this a unique_ptr, but that's > OK > > with me. > > Actually, you might be able to forward-declare *this* struct as well, and then > have both definitions in the .cc file, so you could directly have the observer > as a member again :) Or you might be able to make *this* struct the same as the observer. Given all this stuff we store per-profile (or IO context), it feels like this class should be per-profile. I'm not suggesting we change it in this CL, but just a potential future refactor...
https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:8: #include <string> On 2016/08/11 23:31:36, tommycli wrote: > Since you included string in .h file you don't need it here. Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:73: context_info.hcsm_observer.SetProfile(profile); On 2016/08/11 23:31:36, tommycli wrote: > I think the observer should only be constructed and added if the > kPreferHtmlOverPlugins feature is on. In the next patchset, when kPreferHtmlOverPlugins feature is on, it's still being constructed but it's not being added as an observer. Should I avoid constructing it when it's off? https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:81: resource_context_map_[context].host_content_settings_map->RemoveObserver( On 2016/08/12 09:35:55, Bernhard Bauer wrote: > You could put this into the destructor of the observer itself. Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:256: std::string resource_identifier) { On 2016/08/11 23:31:36, tommycli wrote: > DCHECK that the feature is on here too. Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:21: #include "components/content_settings/core/browser/host_content_settings_map.h" On 2016/08/11 23:31:36, tommycli wrote: > These can still be forward declared right? Yes, you're right. Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:83: class HCSMObserver : public content_settings::Observer { On 2016/08/11 23:31:36, tommycli wrote: > Also we don't really use abbreviations in Chromium code. Maybe a > ProfileContentSettingObserver? Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:83: class HCSMObserver : public content_settings::Observer { On 2016/08/11 23:31:36, tommycli wrote: > Can you make forward declare it here, then put the whole implementation within > the anonymous namespace of the cc file? I forward declared it and put the implementation in the global namespace of the cc file. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:85: HCSMObserver(); On 2016/08/11 23:31:36, tommycli wrote: > If you can forward declare it, i would just set the profile within the > constructor. Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:102: ContextInfo(const ContextInfo& other); On 2016/08/11 23:31:36, tommycli wrote: > Hmm... I think you never want to copy this struct. Can you have no constructors > or destructors? (Assuming you change HCSMObserver to a unique_ptr) Done. https://codereview.chromium.org/2234133003/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:107: HCSMObserver hcsm_observer; On 2016/08/12 09:35:55, Bernhard Bauer wrote: > On 2016/08/11 23:31:36, tommycli wrote: > > If you forward declare it you may need to make this a unique_ptr, but that's > OK > > with me. > > Actually, you might be able to forward-declare *this* struct as well, and then > have both definitions in the .cc file, so you could directly have the observer > as a member again :) I forward declared it but ended up using unique_ptr anyway to avoid ContextInfo copying.
lgtm https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:130: resource_context_map_[context] = std::unique_ptr<ContextInfo>( You can use base::MakeUnique<>() for this :) https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:26: class Profile; Nit: Keep the forward declarations sorted please :)
trizzofo@google.com changed reviewers: + thestig@chromium.org
Lei Zhang, ptal at profile files. https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.cc (right): https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.cc:130: resource_context_map_[context] = std::unique_ptr<ContextInfo>( On 2016/08/13 11:57:24, Bernhard Bauer wrote: > You can use base::MakeUnique<>() for this :) Done. https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/40001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:26: class Profile; On 2016/08/13 11:57:24, Bernhard Bauer wrote: > Nit: Keep the forward declarations sorted please :) Done.
https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:627: ChromePluginServiceFilter::GetInstance()->RegisterResourceContext( Can we just pass in |this|, and have RegisterResourceContext() figure out the PluginPrefs and HostContentSettingsMap from the Profile* ?
The CQ bit was checked by trizzofo@google.com 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 #5 (id:70001) has been deleted
Patchset #5 (id:90001) has been deleted
Patchset #5 (id:110001) has been deleted
one more suggestion Tomas: and yeah I like thestig's suggestion. https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:83: class ProfileContentSettingObserver; I think this can just be a class within the anonymous namespace within the cc file. (Not an inner class)
https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_plugin_service_filter.h (right): https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/plugins/... chrome/browser/plugins/chrome_plugin_service_filter.h:83: class ProfileContentSettingObserver; On 2016/08/15 19:25:11, tommycli wrote: > I think this can just be a class within the anonymous namespace within the cc > file. (Not an inner class) Done. https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/2234133003/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:627: ChromePluginServiceFilter::GetInstance()->RegisterResourceContext( On 2016/08/15 18:37:56, Lei Zhang wrote: > Can we just pass in |this|, and have RegisterResourceContext() figure out the > PluginPrefs and HostContentSettingsMap from the Profile* ? Yes. Done!
The CQ bit was checked by trizzofo@google.com 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...
lgtm
thank you tomas, lgtm
The CQ bit was checked by trizzofo@google.com 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 trizzofo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, tommycli@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2234133003/#ps230001 (title: "Rebase and tracked patchset change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [HBD] Call PurgePluginListCache() whenever plugin content settings change Call PluginService::PurgePluginListCache() whenever plugin content settings change. This is made to ensure that Page's PluginData instance is updated for the next navigation. BUG=626728 ========== to ========== [HBD] Call PurgePluginListCache() whenever plugin content settings change Call PluginService::PurgePluginListCache() whenever plugin content settings change. This is made to ensure that Page's PluginData instance is updated for the next navigation. BUG=626728 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== [HBD] Call PurgePluginListCache() whenever plugin content settings change Call PluginService::PurgePluginListCache() whenever plugin content settings change. This is made to ensure that Page's PluginData instance is updated for the next navigation. BUG=626728 ========== to ========== [HBD] Call PurgePluginListCache() whenever plugin content settings change Call PluginService::PurgePluginListCache() whenever plugin content settings change. This is made to ensure that Page's PluginData instance is updated for the next navigation. BUG=626728 Committed: https://crrev.com/3e5d33572d42429457897450228f773d30e7af17 Cr-Commit-Position: refs/heads/master@{#413856} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3e5d33572d42429457897450228f773d30e7af17 Cr-Commit-Position: refs/heads/master@{#413856} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
