Remove last usage functions from HostContentSettingsMap

Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 1 year. There is no concerete plan to use them.
2) Aren't implemented in a very clean way, e.g.:
  -There are 5 functions in HostContentSettingsMap for this functionality,
   making the interface more complex. It's unclear whether this 
   functionality should even be in HostContentSettingsMap.
  -There is special casing in SetContentSetting for each permission which
   shouldn't be there.

I considered refactoring this code to support the scoping cleanup work, but
since it is not used this could just be wasted time. I would rather remove it
and when someone needs it they can add it back.

BUG=551747

[raymes, 2015-11-09 04:18:03]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 1 year. There is no concerete plan to use them.
2) Aren't implemented in a very clean way, e.g.:
  -There are 5 functions in HostContentSettingsMap for this functionality,
   making the interface more complex. It's unclear which function to use. It's
   unclear whether this functionality should even be in HostContentSettingsMap.
  -There is special casing in SetContentSetting for each permission which
   shouldn't be there.

I considered refactoring this code to support the scoping cleanup work, but
since it is not used this could just be wasted time. I would rather remove it
and when someone needs it they can add it back in a cleaner way.

BUG=551747
==========

to

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 1 year. There is no concerete plan to use them.
2) Aren't implemented in a very clean way, e.g.:
  -There are 5 functions in HostContentSettingsMap for this functionality,
   making the interface more complex. It's unclear whether this 
   functionality should even be in HostContentSettingsMap.
  -There is special casing in SetContentSetting for each permission which
   shouldn't be there.

I considered refactoring this code to support the scoping cleanup work, but
since it is not used this could just be wasted time. I would rather remove it
and when someone needs it they can add it back in a cleaner way.

BUG=551747
==========

[raymes, 2015-11-09 04:18:35]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 1 year. There is no concerete plan to use them.
2) Aren't implemented in a very clean way, e.g.:
  -There are 5 functions in HostContentSettingsMap for this functionality,
   making the interface more complex. It's unclear whether this 
   functionality should even be in HostContentSettingsMap.
  -There is special casing in SetContentSetting for each permission which
   shouldn't be there.

I considered refactoring this code to support the scoping cleanup work, but
since it is not used this could just be wasted time. I would rather remove it
and when someone needs it they can add it back in a cleaner way.

BUG=551747
==========

to

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 1 year. There is no concerete plan to use them.
2) Aren't implemented in a very clean way, e.g.:
  -There are 5 functions in HostContentSettingsMap for this functionality,
   making the interface more complex. It's unclear whether this 
   functionality should even be in HostContentSettingsMap.
  -There is special casing in SetContentSetting for each permission which
   shouldn't be there.

I considered refactoring this code to support the scoping cleanup work, but
since it is not used this could just be wasted time. I would rather remove it
and when someone needs it they can add it back.

BUG=551747
==========

[raymes, 2015-11-09 05:41:45]

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

[Michael van Ouwerkerk, 2015-11-09 10:32:50]

On 2015/11/09 05:41:46, raymes wrote:

The CL description does not seem correct: the last usage functions are used, and
this CL deletes that usage. What is really the problem being solved? Is e.g.
this information just not used later on?

[msramek, 2015-11-09 10:57:55]

On 2015/11/09 10:32:50, Michael van Ouwerkerk wrote:
> On 2015/11/09 05:41:46, raymes wrote:
> 
> The CL description does not seem correct: the last usage functions are used,
and
> this CL deletes that usage. What is really the problem being solved? Is e.g.
> this information just not used later on?

Drive by!

This was only used in an experimental view which was deleted here:
https://codereview.chromium.org/1156983002/ The only remaining callsites are in
tests. As there are currently no plans to reuse it, it makes sense to delete it.

[mlamouri (slow - plz ping), 2015-11-09 20:33:12]

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

[mlamouri (slow - plz ping), 2015-11-09 20:33:13]

https://codereview.chromium.org/1411653004/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1411653004/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:342: // TODO(tsergeant): Use
this for measuring permission usage stats.
I'm confused. How is that method not used if it is actually used here?

[Bernhard Bauer, 2015-11-09 20:37:14]

bauerb@chromium.org changed reviewers:
+ jhawkins@google.com

[Bernhard Bauer, 2015-11-09 20:37:14]

On 2015/11/09 10:57:55, msramek wrote:
> On 2015/11/09 10:32:50, Michael van Ouwerkerk wrote:
> > On 2015/11/09 05:41:46, raymes wrote:
> > 
> > The CL description does not seem correct: the last usage functions are used,
> and
> > this CL deletes that usage. What is really the problem being solved? Is e.g.
> > this information just not used later on?
> 
> Drive by!
> 
> This was only used in an experimental view which was deleted here:
> https://codereview.chromium.org/1156983002/ The only remaining callsites are
in
> tests. As there are currently no plans to reuse it, it makes sense to delete
it.

Hm… as linked on that CL, there are mocks that seem to include a view with last
usage (e.g. go/fzjzy), so if there are indeed plans to do that soon, it might be
counterproductive to remove this now. I believe +jhawkins is working on the MD
settings redesign?

[msramek, 2015-11-09 20:42:28]

On 2015/11/09 20:37:14, Bernhard Bauer wrote:
> On 2015/11/09 10:57:55, msramek wrote:
> > On 2015/11/09 10:32:50, Michael van Ouwerkerk wrote:
> > > On 2015/11/09 05:41:46, raymes wrote:
> > > 
> > > The CL description does not seem correct: the last usage functions are
used,
> > and
> > > this CL deletes that usage. What is really the problem being solved? Is
e.g.
> > > this information just not used later on?
> > 
> > Drive by!
> > 
> > This was only used in an experimental view which was deleted here:
> > https://codereview.chromium.org/1156983002/ The only remaining callsites are
> in
> > tests. As there are currently no plans to reuse it, it makes sense to delete
> it.
> 
> Hm… as linked on that CL, there are mocks that seem to include a view with
last
> usage (e.g. go/fzjzy), so if there are indeed plans to do that soon, it might
be
> counterproductive to remove this now. I believe +jhawkins is working on the MD
> settings redesign?

Ah, I take that back. I found the old email thread where this CL was discussed,
and yes, this was one of the things that we wanted to keep, because it's in the
MD mocks. jhawkins@ was on that thread as well.

[Bernhard Bauer, 2015-11-09 20:49:47]

bauerb@chromium.org changed reviewers:
+ jhawkins@chromium.org
- jhawkins@google.com

[Bernhard Bauer, 2015-11-09 20:49:47]

(Oops, let's make that Chromium jhawkins)

[raymes, 2015-11-09 23:07:58]

Thanks all! If this is really going to be used soon then of course I'm happy to
leave it here :). But the flip side is that there is a cost to leaving dead code
in the codebase and there is no incentive to refactor or improve it. It's
already been dead for a long time.

jhawkins: if there is a concrete plan (engineer assigned) to use this code
within the next quarter then I'm happy to work around it. Otherwise I think it's
reasonable to remove it and we can always get it back from version control
later.

https://codereview.chromium.org/1411653004/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/1411653004/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_manager.cc:342: // TODO(tsergeant): Use
this for measuring permission usage stats.
What I'm trying to say is that the usage information is collected here, but is
never used for anything :)

tsergeant wants to add usage metrics very soon (as in within the next 2 weeks)
so I left this hook in for him to use. This CL removes all the code below it
though, which is not useful for him.

[James Hawkins, 2015-11-09 23:09:25]

On 2015/11/09 23:07:58, raymes wrote:
> Thanks all! If this is really going to be used soon then of course I'm happy
to
> leave it here :). But the flip side is that there is a cost to leaving dead
code
> in the codebase and there is no incentive to refactor or improve it. It's
> already been dead for a long time.
> 
> jhawkins: if there is a concrete plan (engineer assigned) to use this code
> within the next quarter then I'm happy to work around it. Otherwise I think
it's
> reasonable to remove it and we can always get it back from version control
> later.
> 
>
https://codereview.chromium.org/1411653004/diff/1/chrome/browser/permissions/...
> File chrome/browser/permissions/permission_manager.cc (right):
> 
>
https://codereview.chromium.org/1411653004/diff/1/chrome/browser/permissions/...
> chrome/browser/permissions/permission_manager.cc:342: // TODO(tsergeant): Use
> this for measuring permission usage stats.
> What I'm trying to say is that the usage information is collected here, but is
> never used for anything :)
> 
> tsergeant wants to add usage metrics very soon (as in within the next 2 weeks)
> so I left this hook in for him to use. This CL removes all the code below it
> though, which is not useful for him.

Hey, I actually just sent out https://codereview.chromium.org/1418973006/, so
you should find another reviewer from that OWNERS file!

[Bernhard Bauer, 2015-11-09 23:22:03]

bauerb@chromium.org changed reviewers:
+ finnur@chromium.org
- jhawkins@chromium.org

[Bernhard Bauer, 2015-11-09 23:22:03]

lgtm

[Bernhard Bauer, 2015-11-09 23:22:27]

not lgtm for now (sorry, i misclicked)

[Bernhard Bauer, 2015-11-09 23:24:07]

What I meant to say was: 😃

Moving James to CC, +Finnur

tbuckley@ just told me that there is interest in principle to implement these
mocks and that Finnur is working on it.

[raymes, 2015-11-10 03:43:37]

On 2015/11/09 23:24:07, Bernhard Bauer wrote:
> What I meant to say was: 😃
> 
> Moving James to CC, +Finnur
> 
> tbuckley@ just told me that there is interest in principle to implement these
> mocks and that Finnur is working on it.

Hey Finnur, please let us know if there is a concrete plan to implement this
soon.

Thanks,
Raymes

[tbuckley, 2015-11-10 05:46:37]

The mocks show how this data is intended to be used:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...

[Finnur, 2015-11-10 12:17:54]

Yes, the mocks for the MD-settings initiative include showing the usage data for
the permissions and this is an area I'm working full time on at the moment. It
is early stages and I'm just getting the building blocks up (showing list of
sites within categories and the basic site details), but this is one of those
things I was planning on including in the new UI.

[Michael van Ouwerkerk, 2016-01-07 10:34:53]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org

[raymes, 2016-06-06 04:50:56]

raymes@chromium.org changed reviewers:
- mlamouri@chromium.org

[raymes, 2016-06-06 04:50:57]

To loop back on this: it's now been 1.5 years that the code has been unused. I
chatted with Finnur and he said he was ok with removing it. UX was ok with not
including it for the time being. The quality of the data for most permissions is
not very good at present. Only geolocation usage data is actually plumbed
through from the renderer. Most other permissions will not have accurate usage
data. 

If we want to add it back we can do so when it's going to be used. Bernhard,
does this seem ok to you?

Thanks!

[Bernhard Bauer, 2016-06-06 08:14:29]

OK, if there are no plans right now to show last usage in content settings UI,
LGTM.