Introduce ContentSettingsClient interface

Introduce ContentSettingsClient interface

This CL:
* Introduces ContentSettingsClient interface.
* Includes some functions to be transferred to interface from TabSpecificContentSettings.

To be done in follow-up CLs:
* Introduce a Chrome implementation of this interface.
* Add code to create those objects.
* Move the methods from TabSpecificContentSettings included in the interface to the implementation.

BUG=387075, 384873, 393248
Committed: https://crrev.com/478dd52b98307a5b27eca64a8a8af14e0977cc67
Cr-Commit-Position: refs/heads/master@{#299090}

[vabr (Chromium), 2014-10-08 14:46:19]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2014-10-08 14:48:00]

vabr@chromium.org changed reviewers:
+ blundell@chromium.org

[vabr (Chromium), 2014-10-08 14:48:01]

Hi Colin,

Mind taking a look?

Normally, I would ask content_settings OWNERS for review, but adding the client
interface has much more to do with componentisation than with content_settings.
Obviously, feel free to object to my reviewer choice. :)

Cheers,
Vaclav

[vabr (Chromium), 2014-10-08 16:12:14]

Ah, sorry, I obviously cannot code (forgot to compile locally).
Fixing all the stupid mistakes, will ping once ready for review.

[vabr (Chromium), 2014-10-08 16:34:00]

All right, this ready for review again.

Thanks, Colin!
Vaclav

[blundell, 2014-10-09 05:50:51]

LGTM with nits

Exciting, thanks! It will be really great to have content_settings
componentized, thanks again for taking on this work.

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
File components/content_settings.gypi (right):

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings.gypi:75:
'content_settings/core/browser/stub_content_settings_client.cc',
I think this could live in content_settings/core/test and be called
content_settings_core_test_utils.

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings.gypi:75:
'content_settings/core/browser/stub_content_settings_client.cc',
nit: these objects would conventionally be called test_foo (e.g.,
test_content_settings_client)

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
File components/content_settings/core/browser/content_settings_client.cc
(right):

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings/core/browser/content_settings_client.cc:9:
ContentSettingsClient::ContentSettingsClient() {
Do you anticipate adding optional methods? If not, I would fold these into the
.h file.

[vabr (Chromium), 2014-10-09 13:47:54]

Hi Colin,

Thanks for your review and support!

I addressed all your comments but the one about renaming Stup* to Test* --
please see below, and let me know what you think.

Cheers,
Vaclav

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
File components/content_settings.gypi (right):

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings.gypi:75:
'content_settings/core/browser/stub_content_settings_client.cc',
On 2014/10/09 05:50:50, blundell wrote:
> I think this could live in content_settings/core/test and be called
> content_settings_core_test_utils.

Done.

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings.gypi:75:
'content_settings/core/browser/stub_content_settings_client.cc',
On 2014/10/09 05:50:50, blundell wrote:
> nit: these objects would conventionally be called test_foo (e.g.,
> test_content_settings_client)

I tried to copy the structure we ended up with in password manager:
PasswordManagerClient is the interface
StubPasswordManagerClient stubs the pure virtual methods for convenience
TestPasswordManagerClient/MockPasswordManagerClient are typically local to a
test, inherit from Stub* and modify methods relevant to the test.

Therefore I would like to stick with Stub* here as well. What do you think?

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
File components/content_settings/core/browser/content_settings_client.cc
(right):

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings/core/browser/content_settings_client.cc:9:
ContentSettingsClient::ContentSettingsClient() {
On 2014/10/09 05:50:50, blundell wrote:
> Do you anticipate adding optional methods? If not, I would fold these into the
> .h file.

Done.

[blundell, 2014-10-09 14:00:40]

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
File components/content_settings.gypi (right):

https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
components/content_settings.gypi:75:
'content_settings/core/browser/stub_content_settings_client.cc',
On 2014/10/09 13:47:54, vabr (Chromium) wrote:
> On 2014/10/09 05:50:50, blundell wrote:
> > nit: these objects would conventionally be called test_foo (e.g.,
> > test_content_settings_client)
> 
> I tried to copy the structure we ended up with in password manager:
> PasswordManagerClient is the interface
> StubPasswordManagerClient stubs the pure virtual methods for convenience
> TestPasswordManagerClient/MockPasswordManagerClient are typically local to a
> test, inherit from Stub* and modify methods relevant to the test.
> 
> Therefore I would like to stick with Stub* here as well. What do you think?

Heh, now that you mention it, I've always had a problem with the way that
Autofill/PasswordManager work in this context. What is the motivation to keep an
empty StubContentSettingsClient? Why not fold all the logic that tests use into
that class, making it configurable as necessary so that different tests can do
the different kinds of mocking they want to do? I don't see any benefit to
having to have slightly different test versions of FooClient across N different
tests vs. one TestFooClient that is configurable to work in all the N tests plus
be used out-of-the-box for new tests that get developed.

[vabr (Chromium), 2014-10-09 14:15:23]

On 2014/10/09 14:00:40, blundell wrote:
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> File components/content_settings.gypi (right):
> 
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> components/content_settings.gypi:75:
> 'content_settings/core/browser/stub_content_settings_client.cc',
> On 2014/10/09 13:47:54, vabr (Chromium) wrote:
> > On 2014/10/09 05:50:50, blundell wrote:
> > > nit: these objects would conventionally be called test_foo (e.g.,
> > > test_content_settings_client)
> > 
> > I tried to copy the structure we ended up with in password manager:
> > PasswordManagerClient is the interface
> > StubPasswordManagerClient stubs the pure virtual methods for convenience
> > TestPasswordManagerClient/MockPasswordManagerClient are typically local to a
> > test, inherit from Stub* and modify methods relevant to the test.
> > 
> > Therefore I would like to stick with Stub* here as well. What do you think?
> 
> Heh, now that you mention it, I've always had a problem with the way that
> Autofill/PasswordManager work in this context. What is the motivation to keep
an
> empty StubContentSettingsClient? Why not fold all the logic that tests use
into
> that class, making it configurable as necessary so that different tests can do
> the different kinds of mocking they want to do? I don't see any benefit to
> having to have slightly different test versions of FooClient across N
different
> tests vs. one TestFooClient that is configurable to work in all the N tests
plus
> be used out-of-the-box for new tests that get developed.

To see the benefit for Mock* subclasses, consider the alternatives:
(1) Having one global Mock* class, which mocks all the virtual methods. -> Then
you have to explicitly tell GMock to ignore calls on those you are not
interested in, or use a non-strict setting in GMock which might cover some bugs.
(2) Having no global class, and mock locally the base interface. -> Then you
have to stub all methods you don't want to mock.

For testing classes, the devil is in the details: if you have a global test
class, how do you make it configurable without too much overhead? If the
interface has something like GetObject, and some tests want to return a test
version of that object, while other tests want their specific mocked version of
the object, you probably need to pass it as a constructor argument. That means
all the tests have to pass all such returned data members as arguments, even if
they don't care. Compare that to having the stub global class, and inheriting a
test class from it locally, only overriding what you care about -- such code
makes it also clearer, what is important for the given test.

A general argument for having the Stub* mid-step is to separate functions: the
interface defines the API, the stub is a no-op implementation of that API,
test/mock provide specific functions for specific tests.

[blundell, 2014-10-09 14:19:04]

On 2014/10/09 14:15:23, vabr (Chromium) wrote:
> On 2014/10/09 14:00:40, blundell wrote:
> >
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> > File components/content_settings.gypi (right):
> > 
> >
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> > components/content_settings.gypi:75:
> > 'content_settings/core/browser/stub_content_settings_client.cc',
> > On 2014/10/09 13:47:54, vabr (Chromium) wrote:
> > > On 2014/10/09 05:50:50, blundell wrote:
> > > > nit: these objects would conventionally be called test_foo (e.g.,
> > > > test_content_settings_client)
> > > 
> > > I tried to copy the structure we ended up with in password manager:
> > > PasswordManagerClient is the interface
> > > StubPasswordManagerClient stubs the pure virtual methods for convenience
> > > TestPasswordManagerClient/MockPasswordManagerClient are typically local to
a
> > > test, inherit from Stub* and modify methods relevant to the test.
> > > 
> > > Therefore I would like to stick with Stub* here as well. What do you
think?
> > 
> > Heh, now that you mention it, I've always had a problem with the way that
> > Autofill/PasswordManager work in this context. What is the motivation to
keep
> an
> > empty StubContentSettingsClient? Why not fold all the logic that tests use
> into
> > that class, making it configurable as necessary so that different tests can
do
> > the different kinds of mocking they want to do? I don't see any benefit to
> > having to have slightly different test versions of FooClient across N
> different
> > tests vs. one TestFooClient that is configurable to work in all the N tests
> plus
> > be used out-of-the-box for new tests that get developed.
> 
> To see the benefit for Mock* subclasses, consider the alternatives:
> (1) Having one global Mock* class, which mocks all the virtual methods. ->
Then
> you have to explicitly tell GMock to ignore calls on those you are not
> interested in, or use a non-strict setting in GMock which might cover some
bugs.
> (2) Having no global class, and mock locally the base interface. -> Then you
> have to stub all methods you don't want to mock.
> 

Agreed with this. I'm thinking about test functionality more than mocking here.

> For testing classes, the devil is in the details: if you have a global test
> class, how do you make it configurable without too much overhead? If the
> interface has something like GetObject, and some tests want to return a test
> version of that object, while other tests want their specific mocked version
of
> the object, you probably need to pass it as a constructor argument. That means
> all the tests have to pass all such returned data members as arguments, even
if
> they don't care. Compare that to having the stub global class, and inheriting
a
> test class from it locally, only overriding what you care about -- such code
> makes it also clearer, what is important for the given test.

You could have the default behavior of returning NULL just like the stub class,
and then have setters.

> 
> A general argument for having the Stub* mid-step is to separate functions: the
> interface defines the API, the stub is a no-op implementation of that API,
> test/mock provide specific functions for specific tests.

I think it leads to duplicate code and increasing the barrier to writing new
tests :).

[blundell, 2014-10-09 14:20:21]

Note: I think it's up to the individual module owners to make the call here, so
my objections aren't blocking (this is also why I've never taken up this issue
in the autofill/password manager context).

[vabr (Chromium), 2014-10-09 15:47:00]

On 2014/10/09 14:19:04, blundell wrote:
> On 2014/10/09 14:15:23, vabr (Chromium) wrote:
> > On 2014/10/09 14:00:40, blundell wrote:
> > >
> >
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> > > File components/content_settings.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> > > components/content_settings.gypi:75:
> > > 'content_settings/core/browser/stub_content_settings_client.cc',
> > > On 2014/10/09 13:47:54, vabr (Chromium) wrote:
> > > > On 2014/10/09 05:50:50, blundell wrote:
> > > > > nit: these objects would conventionally be called test_foo (e.g.,
> > > > > test_content_settings_client)
> > > > 
> > > > I tried to copy the structure we ended up with in password manager:
> > > > PasswordManagerClient is the interface
> > > > StubPasswordManagerClient stubs the pure virtual methods for convenience
> > > > TestPasswordManagerClient/MockPasswordManagerClient are typically local
to
> a
> > > > test, inherit from Stub* and modify methods relevant to the test.
> > > > 
> > > > Therefore I would like to stick with Stub* here as well. What do you
> think?
> > > 
> > > Heh, now that you mention it, I've always had a problem with the way that
> > > Autofill/PasswordManager work in this context. What is the motivation to
> keep
> > an
> > > empty StubContentSettingsClient? Why not fold all the logic that tests use
> > into
> > > that class, making it configurable as necessary so that different tests
can
> do
> > > the different kinds of mocking they want to do? I don't see any benefit to
> > > having to have slightly different test versions of FooClient across N
> > different
> > > tests vs. one TestFooClient that is configurable to work in all the N
tests
> > plus
> > > be used out-of-the-box for new tests that get developed.
> > 
> > To see the benefit for Mock* subclasses, consider the alternatives:
> > (1) Having one global Mock* class, which mocks all the virtual methods. ->
> Then
> > you have to explicitly tell GMock to ignore calls on those you are not
> > interested in, or use a non-strict setting in GMock which might cover some
> bugs.
> > (2) Having no global class, and mock locally the base interface. -> Then you
> > have to stub all methods you don't want to mock.
> > 
> 
> Agreed with this. I'm thinking about test functionality more than mocking
here.
> 
> > For testing classes, the devil is in the details: if you have a global test
> > class, how do you make it configurable without too much overhead? If the
> > interface has something like GetObject, and some tests want to return a test
> > version of that object, while other tests want their specific mocked version
> of
> > the object, you probably need to pass it as a constructor argument. That
means
> > all the tests have to pass all such returned data members as arguments, even
> if
> > they don't care. Compare that to having the stub global class, and
inheriting
> a
> > test class from it locally, only overriding what you care about -- such code
> > makes it also clearer, what is important for the given test.
> 
> You could have the default behavior of returning NULL just like the stub
class,
> and then have setters.
That might or might not result in a simpler case. What if the test
implementation swapped between two objects it returns or did something which
would be tedious to do with setters, and easy with an overridden method?

> 
> > 
> > A general argument for having the Stub* mid-step is to separate functions:
the
> > interface defines the API, the stub is a no-op implementation of that API,
> > test/mock provide specific functions for specific tests.
> 
> I think it leads to duplicate code and increasing the barrier to writing new
> tests :).

I don't fully understand -- avoiding code duplication and lowering the barrier
for writing new tests was exactly why I created the Stub* classes for password
manager.
With the stubs, each method has at most one trivial implementation (and as many
different non-trivial ones, as needed). This is not getting better without
stubs, or do I miss something?
And to write a new test, you take the prepared stub, and only add what you need
to add for the test. As a side effect, the tests are self-contained, i.e., all
the special functions are listed inside the test's .cc file.
In the world before stubs, when I wrote a new test, I looked through the
codebase, found two different test implementations of a single interface, which
together did what I needed in my new test. The I either merged them (often more
changed codelines than the original test), or I either created a 3rd one (smells
of code duplication). With stubs, no such dilemma: I take a stub and add what I
need.

> Note: I think it's up to the individual module owners to make the call here,
so
> my objections aren't blocking (this is also why I've never taken up this issue
> in the autofill/password manager context).

I can try to bring this up with Markus (who is welcome to interfere, being on Cc
of this :)).
But maybe the easiest to do, since I don't add any tests right now, is to remove
the stub stuff from this CL, and revisit it once I need it in a test.
I'll do that tomorrow.

Cheers,
Vaclav

[blundell, 2014-10-10 09:19:02]

On 2014/10/09 15:47:00, vabr (Chromium) wrote:
> On 2014/10/09 14:19:04, blundell wrote:
> > On 2014/10/09 14:15:23, vabr (Chromium) wrote:
> > > On 2014/10/09 14:00:40, blundell wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> > > > File components/content_settings.gypi (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/640753002/diff/60001/components/content_setti...
> > > > components/content_settings.gypi:75:
> > > > 'content_settings/core/browser/stub_content_settings_client.cc',
> > > > On 2014/10/09 13:47:54, vabr (Chromium) wrote:
> > > > > On 2014/10/09 05:50:50, blundell wrote:
> > > > > > nit: these objects would conventionally be called test_foo (e.g.,
> > > > > > test_content_settings_client)
> > > > > 
> > > > > I tried to copy the structure we ended up with in password manager:
> > > > > PasswordManagerClient is the interface
> > > > > StubPasswordManagerClient stubs the pure virtual methods for
convenience
> > > > > TestPasswordManagerClient/MockPasswordManagerClient are typically
local
> to
> > a
> > > > > test, inherit from Stub* and modify methods relevant to the test.
> > > > > 
> > > > > Therefore I would like to stick with Stub* here as well. What do you
> > think?
> > > > 
> > > > Heh, now that you mention it, I've always had a problem with the way
that
> > > > Autofill/PasswordManager work in this context. What is the motivation to
> > keep
> > > an
> > > > empty StubContentSettingsClient? Why not fold all the logic that tests
use
> > > into
> > > > that class, making it configurable as necessary so that different tests
> can
> > do
> > > > the different kinds of mocking they want to do? I don't see any benefit
to
> > > > having to have slightly different test versions of FooClient across N
> > > different
> > > > tests vs. one TestFooClient that is configurable to work in all the N
> tests
> > > plus
> > > > be used out-of-the-box for new tests that get developed.
> > > 
> > > To see the benefit for Mock* subclasses, consider the alternatives:
> > > (1) Having one global Mock* class, which mocks all the virtual methods. ->
> > Then
> > > you have to explicitly tell GMock to ignore calls on those you are not
> > > interested in, or use a non-strict setting in GMock which might cover some
> > bugs.
> > > (2) Having no global class, and mock locally the base interface. -> Then
you
> > > have to stub all methods you don't want to mock.
> > > 
> > 
> > Agreed with this. I'm thinking about test functionality more than mocking
> here.
> > 
> > > For testing classes, the devil is in the details: if you have a global
test
> > > class, how do you make it configurable without too much overhead? If the
> > > interface has something like GetObject, and some tests want to return a
test
> > > version of that object, while other tests want their specific mocked
version
> > of
> > > the object, you probably need to pass it as a constructor argument. That
> means
> > > all the tests have to pass all such returned data members as arguments,
even
> > if
> > > they don't care. Compare that to having the stub global class, and
> inheriting
> > a
> > > test class from it locally, only overriding what you care about -- such
code
> > > makes it also clearer, what is important for the given test.
> > 
> > You could have the default behavior of returning NULL just like the stub
> class,
> > and then have setters.
> That might or might not result in a simpler case. What if the test
> implementation swapped between two objects it returns or did something which
> would be tedious to do with setters, and easy with an overridden method?
> 

Nothing would prevent a specific test suite from creating their own custom test
client if they needed to be something really complex/one-off.

> > 
> > > 
> > > A general argument for having the Stub* mid-step is to separate functions:
> the
> > > interface defines the API, the stub is a no-op implementation of that API,
> > > test/mock provide specific functions for specific tests.
> > 
> > I think it leads to duplicate code and increasing the barrier to writing new
> > tests :).
> 
> I don't fully understand -- avoiding code duplication and lowering the barrier
> for writing new tests was exactly why I created the Stub* classes for password
> manager.
> With the stubs, each method has at most one trivial implementation (and as
many
> different non-trivial ones, as needed). This is not getting better without
> stubs, or do I miss something?

I meant "keeping the stub empty instead of populating it with code that tests
need" leads to duplication over "populating the stub/test impl with code that
tests need."

I agree that having no stub at all is the worst of all worlds.

> And to write a new test, you take the prepared stub, and only add what you
need
> to add for the test. As a side effect, the tests are self-contained, i.e., all
> the special functions are listed inside the test's .cc file.

I don't think that this matters :). After all, you have a test_utils file here
for example.

> In the world before stubs, when I wrote a new test, I looked through the
> codebase, found two different test implementations of a single interface,
which
> together did what I needed in my new test. The I either merged them (often
more
> changed codelines than the original test), or I either created a 3rd one
(smells
> of code duplication). With stubs, no such dilemma: I take a stub and add what
I
> need.
> 

But you still end up with code duplication in precisely the case that you're
mentioning: more than 1 test needs the same functionality from the test client
impl.
Again, I'm not arguing for eliminating the stub, but rather building it out as
needed into a functional test impl.

> > Note: I think it's up to the individual module owners to make the call here,
> so
> > my objections aren't blocking (this is also why I've never taken up this
issue
> > in the autofill/password manager context).
> 
> I can try to bring this up with Markus (who is welcome to interfere, being on
Cc
> of this :)).
> But maybe the easiest to do, since I don't add any tests right now, is to
remove
> the stub stuff from this CL, and revisit it once I need it in a test.
> I'll do that tomorrow.
> 

SGTM

> Cheers,
> Vaclav

[vabr (Chromium), 2014-10-10 09:40:26]

Thanks, Colin,

For putting effort into explaining the issues you see in using creating the
stub. I agree in particular with your summary:
> Again, I'm not arguing for eliminating the stub, but rather building it out as
needed into a functional test impl.

In order to do that, as promised, I removed the stub from this CL, and am happy
to discuss the best stubbing strategies once tests start needing them.

Since I removed the stub, and you approved of the rest of the CL, I'm going to
assume landing this is OK with you, and land it in a couple of hours, unless I
hear back objections from you.

Cheers!
Vaclav

[blundell, 2014-10-10 10:46:35]

On 2014/10/10 09:40:26, vabr (Chromium) wrote:
> Thanks, Colin,
> 
> For putting effort into explaining the issues you see in using creating the
> stub. I agree in particular with your summary:
> > Again, I'm not arguing for eliminating the stub, but rather building it out
as
> needed into a functional test impl.
> 
> In order to do that, as promised, I removed the stub from this CL, and am
happy
> to discuss the best stubbing strategies once tests start needing them.
> 
> Since I removed the stub, and you approved of the rest of the CL, I'm going to
> assume landing this is OK with you, and land it in a couple of hours, unless I
> hear back objections from you.
> 
> Cheers!
> Vaclav

SGTM, thanks!

[vabr (Chromium), 2014-10-10 10:48:03]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-10-10 10:49:04]

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

[commit-bot: I haz the power, 2014-10-10 11:13:39]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2014-10-10 11:14:23]

Patchset 5 (id:??) landed as
https://crrev.com/478dd52b98307a5b27eca64a8a8af14e0977cc67
Cr-Commit-Position: refs/heads/master@{#299090}