Only Register() desktop-only content settings types on desktop platforms

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

[raymes, 2016-04-19 07:36:23]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-19 07:36:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902703002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902703002/1

[commit-bot: I haz the power, 2016-04-19 11:28:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 11:28:05]

Dry run: 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...)

[msramek, 2016-05-17 18:58:26]

msramek@chromium.org changed reviewers:
+ msramek@chromium.org

[msramek, 2016-05-17 18:58:27]

Thanks, Raymes. I left some comments.

In the meantime, let me also double-check that this is the correct set of
settings.

https://codereview.chromium.org/1902703002/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1902703002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:185:
RegisterDesktop(CONTENT_SETTINGS_TYPE_PPAPI_BROKER, "ppapi-broker",
nit: Move this down as well?

https://codereview.chromium.org/1902703002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:328: void
ContentSettingsRegistry::RegisterDesktop(
Since we do basically everything else via enum, maybe this should follow the
pattern? ALL_PLATFORMS, DESKTOP_ONLY, MOBILE_ONLY (this probably doesn't exist
yet) or something like that...

That will also force people to think about what they need instead of just using
Register() and not noticing RegisterDesktop(). And it will make it easier to
define more types if necessary (e.g. Blink-related settings would be defined on
Desktop and Android but not iOS).

https://codereview.chromium.org/1902703002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:337: #if
!defined(OS_ANDROID)
Hmm, but what about iOS? :)

[msramek, 2016-05-17 19:12:11]

https://codereview.chromium.org/1902703002/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1902703002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:337: #if
!defined(OS_ANDROID)
On 2016/05/17 18:58:27, msramek wrote:
> Hmm, but what about iOS? :)

OK, after a quick search it appears that we have supported cookies and popups
default settings at some point in time, I'm not sure which we do right now (I
don't have an iPhone to test).

It seems that exceptions are not used though (which also means there is no UI)
and default settings are hidden in helper methods like this one:

https://code.google.com/p/chromium/codesearch#chromium/src/components/content...

[msramek, 2016-06-22 11:11:20]

Can we close this, since it was superseded by lshang@'s work?

[raymes, 2016-06-23 00:16:09]

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() 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
==========

[raymes, 2016-06-23 00:16:27]

Yes!

On Wed, 22 Jun 2016 at 21:11 <msramek@chromium.org> wrote:

> Can we close this, since it was superseded by lshang@'s work?
>
> https://codereview.chromium.org/1902703002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.