Split content settings pref related logic into a separate class

Split content settings pref related logic into a separate class

This CL is a pure refactoring which paves the way to allow each content setting type to be stored in a separate pref. A ContentSettingsPref class is introduced. Currently it contains all the logic for reading/writing all content setting types into a single pref. However in a subsequent change N of these objects will be created, one for each content setting type.

BUG=452388
Committed: https://crrev.com/9c7216414298601630ae779d6963d1da3ccbefc9
Cr-Commit-Position: refs/heads/master@{#321085}

[raymes, 2015-03-09 01:51:31]

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

[msramek, 2015-03-09 10:00:25]

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

[msramek, 2015-03-09 10:00:26]

+Markus, who is an owner.

[msramek, 2015-03-09 19:02:29]

Hi Raymes,

overall it LGTM (not that it means much for you now, since I'm not an owner).

I didn't check the correspondence line by line, but which methods moved to the
Pref and which ones stayed in the Provider makes sense to me.

I also added some thoughts.

Regards,
Martin

https://codereview.chromium.org/976243002/diff/20001/components/content_setti...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/976243002/diff/20001/components/content_setti...
components/content_settings/core/browser/content_settings_pref.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
I wonder if we should name this class ContentSettingsExceptionsPref or so, since
we're also going to have separate prefs for default settings in the
DefaultProvider (although there we probably won't need a separate class).

Then of course, this is consistent with how the Provider is named - it's also
not ExceptionsPrefProvider.

Markus, WDYT?

https://codereview.chromium.org/976243002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/976243002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:67:
prefs::kContentSettingsPatternPairs,
This feels incorrect to me, since in this unittest we're testing the Provider
class, but kContentSettingsPatternPairs is a property of the new Pref class.
Shouldn't we have this callback triggered by Notify() or NotifyObservers()?
Then, we wouldn't have to reach one layer under by invoking 

provider_->content_settings_pref()

above. It's maybe not a big deal now, but it will get weird when we split it to
N preferences and then test all N locks here.

https://codereview.chromium.org/976243002/diff/80001/components/content_setti...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/976243002/diff/80001/components/content_setti...
components/content_settings/core/browser/content_settings_pref_provider.h:73:
ContentSettingsPref* content_settings_pref() {
Do we need this exposed publicly somewhere? I would expect ContentSettingsPref
to be used only inside PrefProvider.

If this is only for the unittest, then see my suggestion in the unittest.

[raymes, 2015-03-09 22:13:52]

Thanks!!

https://codereview.chromium.org/976243002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/976243002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:67:
prefs::kContentSettingsPatternPairs,
Hmm I'm not exactly sure what you mean. Could you sketch the code out for me so
I can see more clearly? 

One thought I had is to not access the lock directly above. Instead we could
just call SetWebsiteSetting which is a part of the public interface and will try
to acquire the lock. If it can't get it, it will deadlock the test and timeout.

Maybe what we need to do is add a separate unittest for ContentSettingsPref
after it is changed to store an individual content setting. Then the deadlock
test would be a part of this, since all the locking happens inside of that
class. WDYT?

https://codereview.chromium.org/976243002/diff/80001/components/content_setti...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/976243002/diff/80001/components/content_setti...
components/content_settings/core/browser/content_settings_pref_provider.h:73:
ContentSettingsPref* content_settings_pref() {
I agree with you - this was just for the test and it would be better not to
expose it. This was the easiest thing I could think of.

[msramek, 2015-03-10 19:40:37]

https://codereview.chromium.org/976243002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/976243002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:67:
prefs::kContentSettingsPatternPairs,
On 2015/03/09 22:13:52, raymes wrote:
> Hmm I'm not exactly sure what you mean. Could you sketch the code out for me
so
> I can see more clearly? 
> 
> One thought I had is to not access the lock directly above. Instead we could
> just call SetWebsiteSetting which is a part of the public interface and will
try
> to acquire the lock. If it can't get it, it will deadlock the test and
timeout.
> 
> Maybe what we need to do is add a separate unittest for ContentSettingsPref
> after it is changed to store an individual content setting. Then the deadlock
> test would be a part of this, since all the locking happens inside of that
> class. WDYT?

What I had in mind was - when kContentSettingsPatternPairs callback fires, the
provider will call NotifyObservers(), so we'd add this class as an observer...
or not, because this is a unittest, not a browsertest. Argh. Sorry for the
confusion.

An easy way out is to wrap

provider_->content_settings_pref()->lock_

into

PrefProvider::GetPrefLock()

which doesn't need to be public; instead, we can add DeadlockCheckerThread as a
friend (it originally was, now it's a friend of the Pref class). But that's
still ugly.

I agree that SetWebsiteSetting() probably should do the trick.

And of course, adding a separate unittest for the Pref class is the cleanest and
correct way to do it. I just wanted to avoid suggesting structural changes, so
that you can land this ASAP. We can polish and refactor later :)

[raymes, 2015-03-11 01:03:24]

Done. I went the SetWebsiteSetting route since it used only the public
interface. 

Markus - are you able to take a look? Is there another OWNER who should look at
this?

Thanks!!

[markusheintz_, 2015-03-11 10:50:35]

On 2015/03/11 01:03:24, raymes wrote:
> Done. I went the SetWebsiteSetting route since it used only the public
> interface. 
> 
> Markus - are you able to take a look? Is there another OWNER who should look
at
> this?
> 
> Thanks!!

What's the strategy here? So far it seems like you are extracting some code.
What will be the next step?

[raymes, 2015-03-12 01:56:52]

Right now there is one instance of the ContentSettingsPref class which
writes all content settings types to a single pref. Following this CL we
will create one instance per content settings type. The interface of
ContentSettingsPref will eventually change so that the functions do not
accept a ContentSettingsType and instead operate on a single
ContentSettingsType. That's the high level - there are some details
regarding migration as you mentioned in the meeting.

I hope this is clear enough, if not let me know and I can provide more
details! Or you may want to chat with Martin (for a faster turnaround time)
since I think we are on the same page with regard to the direction to head
in.

Thanks!!

On Wed, 11 Mar 2015 at 21:50 <markusheintz@chromium.org> wrote:

> On 2015/03/11 01:03:24, raymes wrote:
> > Done. I went the SetWebsiteSetting route since it used only the public
> > interface.
>
> > Markus - are you able to take a look? Is there another OWNER who should
> > look
> at
> > this?
>
> > Thanks!!
>
> What's the strategy here? So far it seems like you are extracting some
> code.
> What will be the next step?
>
> https://codereview.chromium.org/976243002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[markusheintz_, 2015-03-12 23:12:55]

LGTM

It is necessary to think about a migration path too. This migration path has to
work with Sync too and support multiple versions.

On 2015/03/12 01:56:52, raymes wrote:
> Right now there is one instance of the ContentSettingsPref class which
> writes all content settings types to a single pref. Following this CL we
> will create one instance per content settings type. The interface of
> ContentSettingsPref will eventually change so that the functions do not
> accept a ContentSettingsType and instead operate on a single
> ContentSettingsType. That's the high level - there are some details
> regarding migration as you mentioned in the meeting.
> 
> I hope this is clear enough, if not let me know and I can provide more
> details! Or you may want to chat with Martin (for a faster turnaround time)
> since I think we are on the same page with regard to the direction to head
> in.
> 
> Thanks!!
> 
> On Wed, 11 Mar 2015 at 21:50 <mailto:markusheintz@chromium.org> wrote:
> 
> > On 2015/03/11 01:03:24, raymes wrote:
> > > Done. I went the SetWebsiteSetting route since it used only the public
> > > interface.
> >
> > > Markus - are you able to take a look? Is there another OWNER who should
> > > look
> > at
> > > this?
> >
> > > Thanks!!
> >
> > What's the strategy here? So far it seems like you are extracting some
> > code.
> > What will be the next step?
> >
> > https://codereview.chromium.org/976243002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[raymes, 2015-03-12 23:18:27]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-03-12 23:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/976243002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-03-12 23:19:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976243002/120001

[commit-bot: I haz the power, 2015-03-13 03:06:39]

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

[commit-bot: I haz the power, 2015-03-13 03:06:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2015-03-13 04:49:59]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-03-13 04:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/976243002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-03-13 04:50:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976243002/140001

[commit-bot: I haz the power, 2015-03-13 08:37:13]

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

[commit-bot: I haz the power, 2015-03-13 08:37:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[markusheintz_, 2015-03-17 09:38:31]

LGTM

[msramek, 2015-03-17 14:04:13]

If the tests fail only because of replacing the explicit locking with
SetWebsiteSetting(), then I'm ok with reverting patchset #7 and keeping the old
approach. I will have chance to improve it in my follow-up CL.

[raymes, 2015-03-18 02:27:18]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-03-18 02:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/976243002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-18 02:27:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976243002/160001

[raymes, 2015-03-18 02:27:58]

I'm so sorry I thought this landed and never realize it didn't. I reverted to
the old patchset to get it landed asap and feel free to change it in your
followup. Sorry again!

[commit-bot: I haz the power, 2015-03-18 06:34:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-18 06:34:58]

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

[msramek, 2015-03-18 20:21:42]

Thanks! And no worries :) I was working on the DefaultProvider in the meantime,
so we didn't lose any time.

[raymes, 2015-03-18 22:52:56]

Cool, glad to hear :) Please loop me in on any reviews to help me learn the
code! Thanks!

On Thu, 19 Mar 2015 at 07:21 <msramek@chromium.org> wrote:

> Thanks! And no worries :) I was working on the DefaultProvider in the
> meantime,
> so we didn't lose any time.
>
> https://codereview.chromium.org/976243002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.