Delete fullscreen/mouselock pref data.

Delete fullscreen/mouselock pref data.

This deletes the default and per-site exceptions for fullscreen and
mouse lock settings on startup. These have not been used for some time,
but the settings have still been around (and visible in the settings
UI). This deletes the data, as it is no longer being used.

Note: The content settings page still allows these to be viewed and
edited, though they will be cleared on startup each time. A follow-up CL
will delete the UI since it is now useless. The UI could not be deleted
before this because otherwise there would be no way for the user to see
what data we are storing.

BUG=591896
Committed: https://crrev.com/cc83d0f2fbe5aa2e75c3861d626f4b6a0b65cbb6
Cr-Commit-Position: refs/heads/master@{#428936}

[Matt Giuca, 2016-10-21 05:32:28]

Patchset #2 (id:20001) has been deleted

[Matt Giuca, 2016-10-21 05:32:51]

mgiuca@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2016-10-24 02:18:44]

lgtm

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

https://codereview.chromium.org/2428433002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_pref_provider.cc:104:
DiscardObsoletePreferences();
It may be better to put this above pref_change_registrar_.Init, to ensure the
callback isn't called as a result of deleting them.

[Matt Giuca, 2016-10-24 23:20:27]

PTAL; I had to update the tests (I previously tested them *after* deleting the
content settings; now I plan to land this before deleting the enum so they have
to temporarily change with weaker expectations).

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

https://codereview.chromium.org/2428433002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_pref_provider.cc:104:
DiscardObsoletePreferences();
On 2016/10/24 02:18:44, raymes wrote:
> It may be better to put this above pref_change_registrar_.Init, to ensure the
> callback isn't called as a result of deleting them.

Done.

[raymes, 2016-10-25 02:06:15]

lgtm

[Matt Giuca, 2016-10-25 02:28:21]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-10-25 02:28:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 02:39:42]

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

[commit-bot: I haz the power, 2016-10-25 02:39:43]

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...)

[Matt Giuca, 2016-10-26 04:18:24]

mgiuca@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Matt Giuca, 2016-10-26 04:18:24]

https://codereview.chromium.org/2428433002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2428433002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:227: // These content
types are deprecated and should not trigger a UI update.
I wish I didn't have to do this (it isn't strictly necessary), but the problem
is that without this, the test SiteSettingsHandlerTest.Incognito fails:
chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc

Because it uses gmock to expect that the final call is onIncognitoStatusChanged,
and this breaks that assumption.

This is temporary; it will be deleted in a follow-up CL
(https://codereview.chromium.org/2419413002/) when I remove these enum values.

[Matt Giuca, 2016-10-26 04:18:39]

(+stevenjb for just that file site_settings_handler.cc)

[stevenjb, 2016-10-26 19:31:00]

lgtm w/ TODO added

https://codereview.chromium.org/2428433002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2428433002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:227: // These content
types are deprecated and should not trigger a UI update.
On 2016/10/26 04:18:24, Matt Giuca wrote:
> I wish I didn't have to do this (it isn't strictly necessary), but the problem
> is that without this, the test SiteSettingsHandlerTest.Incognito fails:
> chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
> 
> Because it uses gmock to expect that the final call is
onIncognitoStatusChanged,
> and this breaks that assumption.
> 
> This is temporary; it will be deleted in a follow-up CL
> (https://codereview.chromium.org/2419413002/) when I remove these enum values.

Could you add a TODO with a bug link? Generally modifying production code for
tests kind of sucks, so let's make sure this is well documented even if it's
going away almost immediately.

[Matt Giuca, 2016-10-26 23:26:47]

https://codereview.chromium.org/2428433002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2428433002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:227: // These content
types are deprecated and should not trigger a UI update.
On 2016/10/26 19:31:00, stevenjb wrote:
> On 2016/10/26 04:18:24, Matt Giuca wrote:
> > I wish I didn't have to do this (it isn't strictly necessary), but the
problem
> > is that without this, the test SiteSettingsHandlerTest.Incognito fails:
> > chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
> > 
> > Because it uses gmock to expect that the final call is
> onIncognitoStatusChanged,
> > and this breaks that assumption.
> > 
> > This is temporary; it will be deleted in a follow-up CL
> > (https://codereview.chromium.org/2419413002/) when I remove these enum
values.
> 
> Could you add a TODO with a bug link? Generally modifying production code for
> tests kind of sucks, so let's make sure this is well documented even if it's
> going away almost immediately.

Done.

[Matt Giuca, 2016-10-26 23:26:52]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-10-26 23:26:52]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2428433002/#ps100001
(title: "Added TODO.")

[commit-bot: I haz the power, 2016-10-26 23:27:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 23:38:45]

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

[commit-bot: I haz the power, 2016-10-26 23:38:46]

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...)

[Matt Giuca, 2016-10-27 03:39:59]

Patchset #6 (id:120001) has been deleted

[Matt Giuca, 2016-10-27 03:40:26]

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

[commit-bot: I haz the power, 2016-10-27 03:40:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 04:51:19]

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

[commit-bot: I haz the power, 2016-10-27 04:51:20]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Matt Giuca, 2016-10-27 07:29:38]

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

[commit-bot: I haz the power, 2016-10-27 07:30:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 08:38:00]

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

[commit-bot: I haz the power, 2016-10-27 08:38:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Matt Giuca, 2016-10-28 00:31:57]

Patchset #8 (id:180001) has been deleted

[Matt Giuca, 2016-10-28 00:38:29]

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

[commit-bot: I haz the power, 2016-10-28 00:39:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 01:36:49]

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

[commit-bot: I haz the power, 2016-10-28 01:36:50]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2016-11-01 02:01:32]

raymes: I forgot to send this earlier.

Can you PTAL at the delta between PS 5 and 8? That's the new stuff I added to
fix those tests that were failing; unfortunately a spaghetti of #ifdefs. :(

[raymes, 2016-11-01 03:06:14]

lgtm

https://codereview.chromium.org/2428433002/diff/200001/components/content_set...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/2428433002/diff/200001/components/content_set...
components/content_settings/core/browser/content_settings_default_provider.cc:36:
"profile.default_content_setting_values.fullscreen";
Would it help slightly to move these definitions down to where they are used?

[Matt Giuca, 2016-11-01 03:56:36]

https://codereview.chromium.org/2428433002/diff/200001/components/content_set...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/2428433002/diff/200001/components/content_set...
components/content_settings/core/browser/content_settings_default_provider.cc:36:
"profile.default_content_setting_values.fullscreen";
On 2016/11/01 03:06:14, raymes wrote:
> Would it help slightly to move these definitions down to where they are used?

Oh... I realised why not, which is that the next CL (which deletes the enums)
needs to explicitly register these, so they will be referred to in two places.

[Matt Giuca, 2016-11-01 03:56:42]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-11-01 03:56:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2428433002/#ps220001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-01 03:56:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 04:37:35]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-11-01 04:39:18]

Description was changed from

==========
Delete fullscreen/mouselock pref data.

This deletes the default and per-site exceptions for fullscreen and
mouse lock settings on startup. These have not been used for some time,
but the settings have still been around (and visible in the settings
UI). This deletes the data, as it is no longer being used.

Note: The content settings page still allows these to be viewed and
edited, though they will be cleared on startup each time. A follow-up CL
will delete the UI since it is now useless. The UI could not be deleted
before this because otherwise there would be no way for the user to see
what data we are storing.

BUG=591896
==========

to

==========
Delete fullscreen/mouselock pref data.

This deletes the default and per-site exceptions for fullscreen and
mouse lock settings on startup. These have not been used for some time,
but the settings have still been around (and visible in the settings
UI). This deletes the data, as it is no longer being used.

Note: The content settings page still allows these to be viewed and
edited, though they will be cleared on startup each time. A follow-up CL
will delete the UI since it is now useless. The UI could not be deleted
before this because otherwise there would be no way for the user to see
what data we are storing.

BUG=591896

Committed: https://crrev.com/cc83d0f2fbe5aa2e75c3861d626f4b6a0b65cbb6
Cr-Commit-Position: refs/heads/master@{#428936}
==========

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

Patchset 9 (id:??) landed as
https://crrev.com/cc83d0f2fbe5aa2e75c3861d626f4b6a0b65cbb6
Cr-Commit-Position: refs/heads/master@{#428936}