Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1991623005:
Only Register() platform specific content settings types on different platforms (Closed)
|
Created:
4 years, 7 months ago by lshang Modified:
4 years, 6 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly Register() platform specific content settings types on different platforms
Content settings are not used on all platforms. Some content settings are only
used on desktop platforms, or desktop and android. We shouldn't register these
types on other platforms that aren't using them, so that we don't create unused
prefs for them and more importantly so that we don't sync settings to these
platforms which would go unused but be a potential privacy concern.
In this CL, an enum WebsiteSettingsInfo::Platform is added and used when
registering content settings to indicate which platform set it is applied to.
BUG=604632
Committed: https://crrev.com/afd97ff8f20acb90a6b495776e385411371651f0
Cr-Commit-Position: refs/heads/master@{#398174}
Patch Set 1 : #Patch Set 2 : fix test failure in ios trying to find plugins #Patch Set 3 : format #
Total comments: 10
Patch Set 4 : test #
Total comments: 13
Patch Set 5 : unsyncable on ios #Patch Set 6 : #
Total comments: 25
Patch Set 7 : address review comments and add tests #
Total comments: 2
Patch Set 8 : fix unit tests #Patch Set 9 : remove debugging logs #Patch Set 10 : reset to approval state #Patch Set 11 : split the changes and register images, plugins, mouselock in this CL #Dependent Patchsets: Messages
Total messages: 77 (58 generated)
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:240001) has been deleted
Description was changed from ========== Only Register() desktop-only content settings types on desktop platforms Some content settings are only used on desktop platforms. We shouldn't register these types on android, so that we don't create unused prefs for them and more importantly so that we don't sync settings to android which would go unused but be a potential privacy concern. An alternative would be to if-def out these settings on android, but the types are prevalent enough in shared code that this creates a bit of a mess at present. After more refactoring we may be able to if-def these out without that concern. BUG=604632 ========== to ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum AppliedPlatform is added and used when registering content settings to indicate which platform set it is applied to. An alternative would be to if-def out these settings on other platforms, but the types are prevalent enough in shared code that this creates a bit of a mess at present. After more refactoring we may be able to if-def these out without that concern. BUG=604632 ==========
lshang@chromium.org changed reviewers: + msramek@chromium.org, raymes@chromium.org
raymes@ msramek@, this is a follow up CL of https://codereview.chromium.org/1902703002/. I changed the implementation according to Martin's comments and added an enum indicating applied platform set when registering content settings. I'm not sure if we should just register cookies and popups (these are the only two types I can see on ios UI) in ios, or make it the same set with android. If ios only registers these two, then we need to be careful not touch other types in the shared code. What do you think?:-)
Thanks Liu! https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:333: WebsiteSettingsInfo::AppliedPlatform applied_platform, nit: perhaps just name this WebsiteSettingsInfo::Platform? https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:335: switch (applied_platform) { I think we should move this into WebsiteSettingsRegistry::Register and then when the content setting registers itself below, we can check the return value and exit if it's a nullptr. That way we can apply these flags to WebsiteSettings too. I think some of those don't need to be registered on some platforms (e.g. app banner is android only I think). https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/content_settings_registry_unittest.cc (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/content_settings_registry_unittest.cc:75: #if !defined(OS_ANDROID) && !defined(OS_IOS) instead of ifdefing this out, perhaps we could just adjust the ifdef at the bottom so that it says #if defined(OS_ANDROID) || defined(OS_IOS) EXPECT_FALSE(plugins_found); #else EXPECT_TRUE(plugins_found); #endif https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:176: MigrateOldSettings(); Maybe we can just rename this to MigrateKeygenSettings and remove the array/loop in the function since we only have one type :) https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/website_settings_info.h:53: enum AppliedPlatform { nit: Maybe just Platform? I wonder if we should move this into WebsiteSettingsRegistry since it's not actually used in this file. It feels like we should have a bitmask here with DESKTOP defined in terms of windows, linux, chromeos and mac. It would make the code which checks the platform simpler and it would avoid having to add more values to this enum in the future. I don't think it would be much more complicated. See e.g. https://code.google.com/p/chromium/codesearch#chromium/src/components/pref_re... Also as I thought about it more I'm not sure this is a good long-term solution. I think long term each content setting should be registered from within the component/code where it is used. Then each component could ifdef its own content settings depending on the platforms on which the component is used. That is a more modular approach. However I think this is good for now. Let's add a TODO, something like: // TODO(...): Remove this enum when content settings can be registered from within the component in which they are used. When this is possible then ifdefs can be contained within each component.
Patchset #5 (id:300001) has been deleted
Patchset #4 (id:280001) has been deleted
Patchset #9 (id:420001) has been deleted
Patchset #6 (id:360001) has been deleted
Thanks Liu, this is pretty much how I envisioned it (even if Raymes doesn't like it long-term :p). I added a few comments and I have a suspicion about the red tests. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:351: std::cout << "-------" << type << "-----" << platform << "------" << (website_settings_info == nullptr) << std::endl; Pro tip: Use LOG(ERROR) << ... for debugging outputs to avoid having to include <iostream> :) https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:175: #if !defined(OS_IOS) To make sure this stays in sync, could we just check if ->Get() == nullptr inside the method itself? https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_info.h:66: ALL_PLATFORMS = PLATFORM_WINDOWS | PLATFORM_LINUX | PLATFORM_CHROMEOS | nit: Switching order and defining ALL_PLATFORMS = DESKTOP | PLATFORM_ANDROID | PLATFORM_IOS would be perhaps more readable. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_info.h:76: WebsiteSettingsInfo(ContentSettingsType type, There is one more irregularity that we should take care of - on iOS, we only have default settings, but not exceptions. WebsiteSettingsInfo registers both preferences with PrefService, so it should be also aware of the platform and *not* register exceptions on iOS. And to avoid things randomly blowing up, perhaps we should instead just register exceptions as unsyncable. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:85: #elif defined(OS_IOS) I read the failing test outputs, and I currently see just one explanation - one of the #elif-s above short circuits the evaluation and jumps out, so the code for OS_IOS never gets executed. I *know* that this should not happen, but I would try it. Could you add a debug output to the #elif defined(OS_IOS) section, or replace #elif-s with #endif #if ?
Patchset #9 (id:460001) has been deleted
Patchset #8 (id:440001) has been deleted
Patchset #11 (id:540001) has been deleted
Patchset #9 (id:500001) has been deleted
Patchset #10 (id:560001) has been deleted
Patchset #10 (id:580001) has been deleted
Patchset #6 (id:380001) has been deleted
Patchset #5 (id:340001) has been deleted
Patchset #5 (id:400001) has been deleted
Patchset #10 (id:660001) has been deleted
Patchset #10 (id:680001) has been deleted
Patchset #10 (id:700001) has been deleted
Patchset #10 (id:720001) has been deleted
Patchset #10 (id:740001) has been deleted
Patchset #11 (id:780001) has been deleted
Patchset #10 (id:760001) has been deleted
Patchset #9 (id:640001) has been deleted
Patchset #9 (id:790001) has been deleted
Patchset #6 (id:520001) has been deleted
Patchset #6 (id:600001) has been deleted
Patchset #4 (id:320001) has been deleted
Patchset #6 (id:810001) has been deleted
msramek@ raymes@, can you take a look again? Thanks! https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:333: WebsiteSettingsInfo::AppliedPlatform applied_platform, On 2016/05/20 19:10:16, raymes wrote: > nit: perhaps just name this WebsiteSettingsInfo::Platform? Done. https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:335: switch (applied_platform) { On 2016/05/20 19:10:16, raymes wrote: > I think we should move this into WebsiteSettingsRegistry::Register and then when > the content setting registers itself below, we can check the return value and > exit if it's a nullptr. That way we can apply these flags to WebsiteSettings > too. I think some of those don't need to be registered on some platforms (e.g. > app banner is android only I think). Done. I checked and confirmed with Dom about app-banner and site-engagement, they are used on desktop and android, not ios. The same with ssl-cert-decision and usb-chooser-data. https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/content_settings_registry_unittest.cc (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/content_settings_registry_unittest.cc:75: #if !defined(OS_ANDROID) && !defined(OS_IOS) On 2016/05/20 19:10:16, raymes wrote: > instead of ifdefing this out, perhaps we could just adjust the ifdef at the > bottom so that it says > #if defined(OS_ANDROID) || defined(OS_IOS) > EXPECT_FALSE(plugins_found); > #else > EXPECT_TRUE(plugins_found); > #endif Done. https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:176: MigrateOldSettings(); On 2016/05/20 19:10:16, raymes wrote: > Maybe we can just rename this to MigrateKeygenSettings and remove the array/loop > in the function since we only have one type :) Good suggestion! I made a separate CL(https://codereview.chromium.org/2006553002/) to do this, for clarity of code change history, because this is not much related to this CL:P https://codereview.chromium.org/1991623005/diff/260001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/260001/components/content_set... components/content_settings/core/browser/website_settings_info.h:53: enum AppliedPlatform { On 2016/05/20 19:10:16, raymes wrote: > nit: Maybe just Platform? > > I wonder if we should move this into WebsiteSettingsRegistry since it's not > actually used in this file. > > It feels like we should have a bitmask here with DESKTOP defined in terms of > windows, linux, chromeos and mac. It would make the code which checks the > platform simpler and it would avoid having to add more values to this enum in > the future. I don't think it would be much more complicated. See e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/components/pref_re... > > Also as I thought about it more I'm not sure this is a good long-term solution. > I think long term each content setting should be registered from within the > component/code where it is used. Then each component could ifdef its own content > settings depending on the platforms on which the component is used. That is a > more modular approach. However I think this is good for now. Let's add a TODO, > something like: > // TODO(...): Remove this enum when content settings can be registered from > within the component in which they are used. When this is possible then ifdefs > can be contained within each component. Done. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:351: std::cout << "-------" << type << "-----" << platform << "------" << (website_settings_info == nullptr) << std::endl; On 2016/05/23 14:49:49, msramek wrote: > Pro tip: Use LOG(ERROR) << ... for debugging outputs to avoid having to include > <iostream> :) Thanks for your tip! That's very useful! And sorry for let you see these logs, I was quite busy debugging this test failure and didn't finish it yesterday. Also, since I don't have an ios test device I have to upload it and try it in trybots. I will remove all these debugging logs when it is ready to go :P https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:175: #if !defined(OS_IOS) On 2016/05/23 14:49:49, msramek wrote: > To make sure this stays in sync, could we just check if ->Get() == nullptr > inside the method itself? Done. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_info.h:66: ALL_PLATFORMS = PLATFORM_WINDOWS | PLATFORM_LINUX | PLATFORM_CHROMEOS | On 2016/05/23 14:49:49, msramek wrote: > nit: Switching order and defining ALL_PLATFORMS = DESKTOP | PLATFORM_ANDROID | > PLATFORM_IOS would be perhaps more readable. Done. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_info.h:76: WebsiteSettingsInfo(ContentSettingsType type, On 2016/05/23 14:49:49, msramek wrote: > There is one more irregularity that we should take care of - on iOS, we only > have default settings, but not exceptions. > > WebsiteSettingsInfo registers both preferences with PrefService, so it should be > also aware of the platform and *not* register exceptions on iOS. > > And to avoid things randomly blowing up, perhaps we should instead just register > exceptions as unsyncable. I made it unsyncable on ios, am I addressing it right? https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:85: #elif defined(OS_IOS) On 2016/05/23 14:49:49, msramek wrote: > I read the failing test outputs, and I currently see just one explanation - one > of the #elif-s above short circuits the evaluation and jumps out, so the code > for OS_IOS never gets executed. > > I *know* that this should not happen, but I would try it. Could you add a debug > output to the #elif defined(OS_IOS) section, or replace #elif-s with #endif #if > ? I added some LOGs and found out that on ios-simulator-gn, both OS_IOS and OS_MACOS is defined, so it jumped out from the code for MACOS and OS_IOS never get executed. My guess is that the simulator is running on Mac desktop so that OS_MACOS is also defined. Also, chromeos trybots will have OS_LINUX defined as well.
Description was changed from ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum AppliedPlatform is added and used when registering content settings to indicate which platform set it is applied to. An alternative would be to if-def out these settings on other platforms, but the types are prevalent enough in shared code that this creates a bit of a mess at present. After more refactoring we may be able to if-def these out without that concern. BUG=604632 ========== to ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. An alternative would be to if-def out these settings on other platforms, but the types are prevalent enough in shared code that this creates a bit of a mess at present. After more refactoring we may be able to if-def these out without that concern. BUG=604632 ==========
LG, but please improve the tests. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:351: std::cout << "-------" << type << "-----" << platform << "------" << (website_settings_info == nullptr) << std::endl; On 2016/05/24 11:55:45, Liu Shang wrote: > On 2016/05/23 14:49:49, msramek wrote: > > Pro tip: Use LOG(ERROR) << ... for debugging outputs to avoid having to > include > > <iostream> :) > > Thanks for your tip! That's very useful! And sorry for let you see these logs, I > was quite busy debugging this test failure and didn't finish it yesterday. Also, > since I don't have an ios test device I have to upload it and try it in trybots. > I will remove all these debugging logs when it is ready to go :P Yeah, no worries about that. This is normal for iOS (or Mac Cocoa) development :) https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_info.h:76: WebsiteSettingsInfo(ContentSettingsType type, On 2016/05/24 11:55:46, Liu Shang wrote: > On 2016/05/23 14:49:49, msramek wrote: > > There is one more irregularity that we should take care of - on iOS, we only > > have default settings, but not exceptions. > > > > WebsiteSettingsInfo registers both preferences with PrefService, so it should > be > > also aware of the platform and *not* register exceptions on iOS. > > > > And to avoid things randomly blowing up, perhaps we should instead just > register > > exceptions as unsyncable. > > I made it unsyncable on ios, am I addressing it right? What I meant is that iOS *does* use the default settings for cookies and popups, just not exceptions. And those two are stored in different prefs. So we could sync the default setting pref to iOS (the one handled by DefaultProvider), but unsync the exception pref (defined in PrefProvider). We could, for example, #ifdef out PrefProvider in HostContentSettingsMap, or #ifdef the RegisterProfilePrefs call in PrefProvider to always use UNSYNCABLE on iOS. But now that I'm thinking about it, the added complexity might not be worth syncing the two default values. We can address that once (if?) we have proper content settings on iOS. Let's go with your solution for now. https://codereview.chromium.org/1991623005/diff/480001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/480001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:85: #elif defined(OS_IOS) On 2016/05/24 11:55:46, Liu Shang wrote: > On 2016/05/23 14:49:49, msramek wrote: > > I read the failing test outputs, and I currently see just one explanation - > one > > of the #elif-s above short circuits the evaluation and jumps out, so the code > > for OS_IOS never gets executed. > > > > I *know* that this should not happen, but I would try it. Could you add a > debug > > output to the #elif defined(OS_IOS) section, or replace #elif-s with #endif > #if > > ? > > I added some LOGs and found out that on ios-simulator-gn, both OS_IOS and > OS_MACOS is defined, so it jumped out from the code for MACOS and OS_IOS never > get executed. My guess is that the simulator is running on Mac desktop so that > OS_MACOS is also defined. > > Also, chromeos trybots will have OS_LINUX defined as well. > Good to know! Maybe you want to share this on the email thread about syncing prefs that went out recently. Other Chromites could run into the same problems. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry_unittest.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:28: TEST_F(WebsiteSettingsRegistryTest, Get) { I think the spirit of this test is not to test specifically whether APP_BANNER is registered, but whether Get() works properly. Why not change APP_BANNER to COOKIES to run the test everywhere? Then we could add one more test to test specifically the platform-dependent behavior, e.g. GetPlatformDependent that would test something that is not on iOS, and something that is not on mobile. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:43: #if !defined(OS_IOS) Ditto here. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:68: #if !defined(OS_IOS) Ditto.
Thanks! https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:334: uint32_t platform, nit: platforms https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:351: if (website_settings_info == nullptr) nit: if (!website_settings_info) Also please add a comment about why we return in this case https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/content_settings_registry.h (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/content_settings_registry.h:66: uint32_t platform, nit: platforms? Also I think we should add a comment to functions which take this so that the type is defined. Alternatively we can typedef a type. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_info.h:56: enum Platform : uint32_t { We could also have typedef uint32_t Platforms; to make the function signatures more descriptive when we pass this around https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_info.h:71: }; Hmm should we just have this is WebsiteSettingsRegistry since it's not used here? https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:82: // Make content settings unsyncable on ios. nit: maybe expand on why we do this, because exceptions aren't used on ios so we don't want other platforms exceptions to sync there. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:84: #endif Should we have: #else #error "Unsupported platform" #endif Otherwise we should have a default case but this might be better to make sure there aren't cases we haven't thought about. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry.h (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.h:43: // is owned by the registry. I think we should add a comment which says that a nullptr will be returned if registration fails (for example if |platforms| doesn't match the current platform). https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.h:51: uint32_t platform, nit: platforms https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry_unittest.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:28: TEST_F(WebsiteSettingsRegistryTest, Get) { On 2016/05/24 14:29:24, msramek wrote: > I think the spirit of this test is not to test specifically whether APP_BANNER > is registered, but whether Get() works properly. Why not change APP_BANNER to > COOKIES to run the test everywhere? > > Then we could add one more test to test specifically the platform-dependent > behavior, e.g. GetPlatformDependent that would test something that is not on > iOS, and something that is not on mobile. +1
Patchset #7 (id:850001) has been deleted
https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:334: uint32_t platform, On 2016/05/25 03:36:07, raymes wrote: > nit: platforms Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:351: if (website_settings_info == nullptr) On 2016/05/25 03:36:07, raymes wrote: > nit: if (!website_settings_info) > > Also please add a comment about why we return in this case Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/content_settings_registry.h (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/content_settings_registry.h:66: uint32_t platform, On 2016/05/25 03:36:07, raymes wrote: > nit: platforms? > > Also I think we should add a comment to functions which take this so that the > type is defined. Alternatively we can typedef a type. Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_info.h:56: enum Platform : uint32_t { On 2016/05/25 03:36:07, raymes wrote: > We could also have > typedef uint32_t Platforms; > to make the function signatures more descriptive when we pass this around Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_info.h:71: }; On 2016/05/25 03:36:07, raymes wrote: > Hmm should we just have this is WebsiteSettingsRegistry since it's not used > here? Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:82: // Make content settings unsyncable on ios. On 2016/05/25 03:36:07, raymes wrote: > nit: maybe expand on why we do this, because exceptions aren't used on ios so we > don't want other platforms exceptions to sync there. Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:84: #endif On 2016/05/25 03:36:07, raymes wrote: > Should we have: > #else > #error "Unsupported platform" > #endif > > Otherwise we should have a default case but this might be better to make sure > there aren't cases we haven't thought about. Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry.h (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.h:43: // is owned by the registry. On 2016/05/25 03:36:07, raymes wrote: > I think we should add a comment which says that a nullptr will be returned if > registration fails (for example if |platforms| doesn't match the current > platform). Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry.h:51: uint32_t platform, On 2016/05/25 03:36:07, raymes wrote: > nit: platforms Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... File components/content_settings/core/browser/website_settings_registry_unittest.cc (right): https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:28: TEST_F(WebsiteSettingsRegistryTest, Get) { On 2016/05/24 14:29:24, msramek wrote: > I think the spirit of this test is not to test specifically whether APP_BANNER > is registered, but whether Get() works properly. Why not change APP_BANNER to > COOKIES to run the test everywhere? > > Then we could add one more test to test specifically the platform-dependent > behavior, e.g. GetPlatformDependent that would test something that is not on > iOS, and something that is not on mobile. Done. Cookies is ContentSettingsInfo, it don't get registered in WebsiteSettingsRegistry. But yeah I agree with your idea and thanks for the reminding! I added a GetPlatformDependent test in ContentSettingsRegistryTest to test platform-dependent behavior. For the 5 types in WebsiteSettingsInfo, auto-select-certificate is unsure whether or not it is used on iOS, so I make it apply to all platforms until we get some evidence. The other 4 types(ssl-cert-decision, app-banner, site-engagement and usb-chooser-data) are confirmed to be not used. I checked in .gypi file, they are under non-ios sources. So I changed app-banner to auto-select-certificate in these tests. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:43: #if !defined(OS_IOS) On 2016/05/24 14:29:24, msramek wrote: > Ditto here. Done. https://codereview.chromium.org/1991623005/diff/830001/components/content_set... components/content_settings/core/browser/website_settings_registry_unittest.cc:68: #if !defined(OS_IOS) On 2016/05/24 14:29:24, msramek wrote: > Ditto. Done.
msramek@ raymes@, PTALA thanks!:-)
LGTM. Thanks a lot for taking care of this!
lgtm https://codereview.chromium.org/1991623005/diff/870001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/870001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:366: // is not used on current platform and don't need to be registered. nit: is not used on the current platform and doesn't need to be registered.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/1991623005/#ps910001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991623005/910001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991623005/910001
Thanks for correcting my grammar!:-)@raymes https://codereview.chromium.org/1991623005/diff/870001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1991623005/diff/870001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:366: // is not used on current platform and don't need to be registered. On 2016/05/30 00:35:58, raymes wrote: > nit: is not used on the current platform and doesn't need to be registered. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #13 (id:990001) has been deleted
Patchset #14 (id:1030001) has been deleted
Patchset #10 (id:930001) has been deleted
Patchset #8 (id:890001) has been deleted
Patchset #9 (id:950001) has been deleted
Patchset #9 (id:970001) has been deleted
Patchset #9 (id:1010001) has been deleted
Patchset #8 (id:910001) has been deleted
Description was changed from ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. An alternative would be to if-def out these settings on other platforms, but the types are prevalent enough in shared code that this creates a bit of a mess at present. After more refactoring we may be able to if-def these out without that concern. BUG=604632 ========== to ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. Several places in shared code are if-defed out when they try to get access to unregistered types. Some tests are also changed to use COOKIES instead of IMAGES to make the tests more platform generic, if they are not testing images specifically. BUG=604632 ==========
Hi Martin, Raymes, unregistering images and plugins caused a lot of crashes on Android. Some more changes are made to handle these cases. Could you please take another look and confirm the changes? Thanks!:-)
Hey Liu - perhaps we could break this off into a separate CL since the previous patchset already had approval? What I mean is, could we keep the previous patchset but allow plugins/images on android and then in a separate patchset, disable them and include the required ifdefs? Does that seem possible? Thanks!
On 2016/06/06 04:56:48, raymes wrote: > Hey Liu - perhaps we could break this off into a separate CL since the previous > patchset already had approval? > > What I mean is, could we keep the previous patchset but allow plugins/images on > android and then in a separate patchset, disable them and include the required > ifdefs? > > Does that seem possible? Thanks! Yep, I'll move the new changes to a separate CL:-)
Description was changed from ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. Several places in shared code are if-defed out when they try to get access to unregistered types. Some tests are also changed to use COOKIES instead of IMAGES to make the tests more platform generic, if they are not testing images specifically. BUG=604632 ========== to ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. BUG=604632 ==========
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/1991623005/#ps1110001 (title: "split the changes and register images, plugins, mouselock in this CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991623005/1110001
Message was sent while issue was closed.
Description was changed from ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. BUG=604632 ========== to ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. BUG=604632 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:1110001)
Message was sent while issue was closed.
Description was changed from ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. BUG=604632 ========== to ========== Only Register() platform specific content settings types on different platforms Content settings are not used on all platforms. Some content settings are only used on desktop platforms, or desktop and android. We shouldn't register these types on other platforms that aren't using them, so that we don't create unused prefs for them and more importantly so that we don't sync settings to these platforms which would go unused but be a potential privacy concern. In this CL, an enum WebsiteSettingsInfo::Platform is added and used when registering content settings to indicate which platform set it is applied to. BUG=604632 Committed: https://crrev.com/afd97ff8f20acb90a6b495776e385411371651f0 Cr-Commit-Position: refs/heads/master@{#398174} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/afd97ff8f20acb90a6b495776e385411371651f0 Cr-Commit-Position: refs/heads/master@{#398174} |
