Move GeolocationSettingsState to ContentSettingsUsagesState

Move GeolocationSettingsState to ContentSettingsUsagesState

GeolocationSettingsState is an useful class to be shared with
other content settings, e.g. coming one for Web MIDI.

TEST=ContentSettingsUsageStateTests.*
BUG=257618
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213057

[Takashi Toyoshima, 2013-07-16 13:18:11]

As a part of following master CL; https://codereview.chromium.org/18004004/
I want to move GeolocationSettingsState to ContentSettingsUsagesState in order
to share this implementation with Web MIDI content settings.

Markus, can you take a look this CL?
Joth, do you agree to use your geolocation stuff from other content settings,
e.g. Web MIDI.

[joth, 2013-07-16 14:47:48]

I'm not working on this area now (and not sure I've seen the files being moved
anyway!) so over to mvanouwerkerk for a first looks, +bulach to approve as he is
in same tz.

Cheers

[Takashi Toyoshima, 2013-07-16 15:21:18]

joth: ok, thank you for forwarding it to proper persons.
It may be better to reflect it to chrome/browser/geolocation/OWNERS file?

[Takashi Toyoshima, 2013-07-17 17:47:11]

Michael: How about this CL?

[bulach, 2013-07-18 08:56:50]

lgtm, thanks! just some nits and one suggestion below:

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_usages_state.h (right):

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state.h:21: // This
class manages a content setting state per tab, and provides information
nit: perhaps "per tab for a given |ContentSettingsType|".

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_usages_state_unittest.cc
(right):

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state_unittest.cc:8:
#include "chrome/browser/content_settings/content_settings_usages_state.h"
nit: this should actually be up there, before <string>

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state_unittest.cc:34:
ContentSettingsUsagesState state(&profile, CONTENT_SETTINGS_TYPE_GEOLOCATION);
nit: CONTENT_SETTINGS_TYPE_GEOLOCATION is hard-coded everywhere..
so how about making it a named constant in the ContentSettingsUsagesStateTests,
and reuse that? it'd be clearer that the class is generic, and the tests are
exercising one particular settings...

just in case: it'd also be nice to add a test to ensure a
ContentSettingsUsagesState for a given type doesn't mess up with other types,
wdyt?

[Michael van Ouwerkerk, 2013-07-18 10:12:58]

lgtm

LGTM but please address Marcus's and my comments.

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_usages_state.cc (right):

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state.cc:21: :
profile_(profile),
Indent with 4 spaces:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

[joth, 2013-07-18 20:18:48]

On 2013/07/16 15:21:18, Takashi Toyoshima (chromium) wrote:
> joth: ok, thank you for forwarding it to proper persons.
> It may be better to reflect it to chrome/browser/geolocation/OWNERS file?

https://codereview.chromium.org/19494008/

[Takashi Toyoshima, 2013-07-22 12:59:18]

Thank you for reviews. I fixed all commented points and upload it as Patch Set
6.

Markus:
Can you take a look this as a content_settings/OWNERS ?

+jochen:
Just in case for content_settings/OWNERS.

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_usages_state.cc (right):

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state.cc:21: :
profile_(profile),
On 2013/07/18 10:12:59, Michael van Ouwerkerk wrote:
> Indent with 4 spaces:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_usages_state.h (right):

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state.h:21: // This
class manages a content setting state per tab, and provides information
On 2013/07/18 08:56:50, bulach wrote:
> nit: perhaps "per tab for a given |ContentSettingsType|".

Done.

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_usages_state_unittest.cc
(right):

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state_unittest.cc:8:
#include "chrome/browser/content_settings/content_settings_usages_state.h"
On 2013/07/18 08:56:50, bulach wrote:
> nit: this should actually be up there, before <string>

Done.

https://codereview.chromium.org/19375002/diff/9001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_usages_state_unittest.cc:34:
ContentSettingsUsagesState state(&profile, CONTENT_SETTINGS_TYPE_GEOLOCATION);
Using a named constant is done.

On testing with other types, I'll add it when I commit a change for Web MIDI
which uses this class.
I added TODO comment.

[markusheintz_, 2013-07-22 19:41:51]

LGTM

https://codereview.chromium.org/19375002/diff/30002/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_usages_state_unittest.cc
(right):

https://codereview.chromium.org/19375002/diff/30002/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_usages_state_unittest.cc:24: //
TODO(toyoshim): Add at least one test to verify the class works with other
Can we file a bug for this please and add the issue number here?

[Takashi Toyoshima, 2013-07-23 05:17:41]

https://codereview.chromium.org/19375002/diff/30002/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_usages_state_unittest.cc
(right):

https://codereview.chromium.org/19375002/diff/30002/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_usages_state_unittest.cc:24: //
TODO(toyoshim): Add at least one test to verify the class works with other
On 2013/07/22 19:41:51, markusheintz_ wrote:
> Can we file a bug for this please and add the issue number here?

Done.

[commit-bot: I haz the power, 2013-07-23 05:17:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/19375002/41001

[commit-bot: I haz the power, 2013-07-23 06:34:34]

Retried try job too often on mac_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-07-23 06:48:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/19375002/41001

[commit-bot: I haz the power, 2013-07-23 07:36:11]

Retried try job too often on mac_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-07-23 07:49:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/19375002/41001

[commit-bot: I haz the power, 2013-07-23 07:54:49]

Change committed as 213057