Refactor ProtectedMediaIdentifierPermissionContext to derive from PermissionContextBase.

Refactor ProtectedMediaIdentifierPermissionContext to derive from PermissionContextBase.

Refactoring of ProtectedMediaIdentifierPermissionContext to derive from
PermissionContextBase class to conform with other APIs like geolocation
and midi. This has the advantage of less code and a consistent path for
all permissions. Also it makes it easier to implement PermissionService
because some functionality like HasPermission() can be implemented at
the level of PermissionContextBase.

Also add the corresponding UMA bits in histograms.xml and
permission_context_uma_util.cc.

BUG=

Committed: https://crrev.com/1baa38863fad853087e411af1fff950ba16cf5de
Cr-Commit-Position: refs/heads/master@{#308100}

[timvolodine, 2014-12-02 18:30:47]

timvolodine@chromium.org changed reviewers:
+ kkimlabs@chromium.org, miguelg@chromium.org

[Kibeom Kim (inactive), 2014-12-02 21:19:14]

LGTM

thanks for cleaning up this!

[mlamouri (slow - plz ping), 2014-12-02 22:43:29]

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

[mlamouri (slow - plz ping), 2014-12-02 22:43:30]

That looks great! I have a couple of comments below. Mostly nits except for the
UI checks clean-up.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1956:
->RequestPermission(web_contents,
nit: indentation

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:113: Profile*
GetProfile() const;
It's a getter, per coding style it should be |profile()| I believe.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:40:
DCHECK(!GetQueueController());
I  think you can remove that. This dtor must be run from the UI thread.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.h
(right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.h:42: void
ShutdownOnUIThread();
Do you still need that? PermissionContextBase requires the dtor to be called
from the UI thread so you probably no longer need all the jazz related to
shutting down that object from the UI thread.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.h:53: bool
shutting_down_;
I guess that could go away too.

[timvolodine, 2014-12-03 16:44:25]

looks even slimmer now :)

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1956:
->RequestPermission(web_contents,
On 2014/12/02 22:43:30, Mounir Lamouri wrote:
> nit: indentation

Done.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:113: Profile*
GetProfile() const;
On 2014/12/02 22:43:30, Mounir Lamouri wrote:
> It's a getter, per coding style it should be |profile()| I believe.

Done.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:40:
DCHECK(!GetQueueController());
On 2014/12/02 22:43:30, Mounir Lamouri wrote:
> I  think you can remove that. This dtor must be run from the UI thread.

Done.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.h
(right):

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.h:42: void
ShutdownOnUIThread();
On 2014/12/02 22:43:30, Mounir Lamouri wrote:
> Do you still need that? PermissionContextBase requires the dtor to be called
> from the UI thread so you probably no longer need all the jazz related to
> shutting down that object from the UI thread.

Done.

https://codereview.chromium.org/769103002/diff/20001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.h:53: bool
shutting_down_;
On 2014/12/02 22:43:30, Mounir Lamouri wrote:
> I guess that could go away too.

Done.

[mlamouri (slow - plz ping), 2014-12-03 16:52:55]

lgtm, thanks! :)

[timvolodine, 2014-12-03 16:59:44]

timvolodine@chromium.org changed reviewers:
+ thakis@chromium.org

[timvolodine, 2014-12-03 16:59:45]

+ thakis@ as chrome/ owner

[Nico, 2014-12-03 18:42:28]

lgtm

[Miguel Garcia, 2014-12-03 18:57:37]

Looks really good, some questions and nits below.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_uma_util.cc:44: void
RecordPermissionAction(
Don't you need to add media here as well?

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_uma_util.cc:108: #if
defined(OS_ANDROID)
You need a change in histograms.xml to add the new type don't you?

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(left):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:62: #if
defined(ENABLE_EXTENSIONS)
Is this really needed? I thought protected media only worked on Android where
there are no extensions.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:62: if
(!requesting_frame_origin.is_valid() || !embedder.is_valid()) {
It seems to me that this could go in the base class if it is really needed.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:75:
prefs::kProtectedMediaIdentifierEnabled)) {
so while you are at it you can probably remove this switch (after checking with
the original author) since we have already launched this right?

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.h
(right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.h:22: public
base::RefCountedThreadSafe<
Do you still need this RefCounted inheritance? PermisssionContext is a key
exported service

[timvolodine, 2014-12-04 17:22:26]

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_uma_util.cc:44: void
RecordPermissionAction(
On 2014/12/03 18:57:36, Miguel Garcia wrote:
> Don't you need to add media here as well?

Done.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_uma_util.cc:108: #if
defined(OS_ANDROID)
On 2014/12/03 18:57:36, Miguel Garcia wrote:
> You need a change in histograms.xml to add the new type don't you?

Yes, thanks for catching this. Done.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(left):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:62: #if
defined(ENABLE_EXTENSIONS)
On 2014/12/03 18:57:36, Miguel Garcia wrote:
> Is this really needed? I thought protected media only worked on Android where
> there are no extensions.

don't know.
kkimlabs@ : any ideas on this?

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:62: if
(!requesting_frame_origin.is_valid() || !embedder.is_valid()) {
On 2014/12/03 18:57:37, Miguel Garcia wrote:
> It seems to me that this could go in the base class if it is really needed.

it probably can, we have something similar in geolocation as well. I'll try to
do this in a separate patch though as this goes a bit beyond the purpose of this
CL.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:75:
prefs::kProtectedMediaIdentifierEnabled)) {
On 2014/12/03 18:57:36, Miguel Garcia wrote:
> so while you are at it you can probably remove this switch (after checking
with
> the original author) since we have already launched this right?

ok I'll check with the author (think it's kkimlabs@?). looks like it is still
used in e.g. pref_service_bridge.cc when the default setting == BLOCK. Anyways I
think the removal of the switch should go into a separate patch, because it
would be easier to revert in case of trouble.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.h
(right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.h:22: public
base::RefCountedThreadSafe<
On 2014/12/03 18:57:37, Miguel Garcia wrote:
> Do you still need this RefCounted inheritance? PermisssionContext is a key
> exported service

Done.

[timvolodine, 2014-12-04 17:24:39]

more updates from talking to qinmin@:

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(left):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:62: #if
defined(ENABLE_EXTENSIONS)
On 2014/12/03 18:57:36, Miguel Garcia wrote:
> Is this really needed? I thought protected media only worked on Android where
> there are no extensions.

Protected media is not android-only, from what I can see it is also used on
ChromeOS.

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/769103002/diff/40001/chrome/browser/media/pro...
chrome/browser/media/protected_media_identifier_permission_context.cc:75:
prefs::kProtectedMediaIdentifierEnabled)) {
On 2014/12/03 18:57:36, Miguel Garcia wrote:
> so while you are at it you can probably remove this switch (after checking
with
> the original author) since we have already launched this right?

We cannot drop this switch, it is used to allow users to disable sending the
hardware identifier to the server.

[timvolodine, 2014-12-04 17:40:07]

timvolodine@chromium.org changed reviewers:
+ isherman@chromium.org

[timvolodine, 2014-12-04 17:40:07]

+isherman@ : for histograms.xml

[Kibeom Kim (inactive), 2014-12-04 18:35:35]

Yes right, ChromeOS also has it, so I think we want to remove #if
defined(OS_ANDROID) in permission_context_uma_util.cc . Though, last time I
heard, ChromeOS’ protected media permission UI is different (modal bubble) from
Clank(info bar), and I haven't checked if it takes the same code path or not.

+mukai@ for clarification

[Kibeom Kim (inactive), 2014-12-04 18:35:48]

kkimlabs@chromium.org changed reviewers:
+ mukai@chromium.org

[Jun Mukai, 2014-12-04 18:42:59]

mukai@chromium.org changed reviewers:
+ dkrahn@chromium.org

[Jun Mukai, 2014-12-04 18:52:40]

On 2014/12/04 18:35:35, Kibeom Kim wrote:
> Yes right, ChromeOS also has it, so I think we want to remove #if
> defined(OS_ANDROID) in permission_context_uma_util.cc . Though, last time I
> heard, ChromeOS’ protected media permission UI is different (modal bubble)
from
> Clank(info bar), and I haven't checked if it takes the same code path or not.
> 
> +mukai@ for clarification

+dkrahn who owns the actual code.

That is true (I don't know why it has to be, and I'm fine to use the same
permission code though).
It seems that the ChromeOS code does not invoke permission_context_uma_util.
That means allowing PROTECTED_MEDIA to OS_CHROMEOS is harmless (also it's better
to do that
in ChromeOS as well, but that should be done in another CL in ChromeOS side I
guess).

By the way PROTECTED_MEDIA content type is only defined in OS_ANDROID or
OS_CHROMEOS,
so you cannot remove the #if-clause itself, you can just say OS_ANDROID ||
OS_CHROMEOS.

[Jun Mukai, 2014-12-04 18:55:32]

On 2014/12/04 18:52:40, Jun Mukai wrote:
> On 2014/12/04 18:35:35, Kibeom Kim wrote:
> > Yes right, ChromeOS also has it, so I think we want to remove #if
> > defined(OS_ANDROID) in permission_context_uma_util.cc . Though, last time I
> > heard, ChromeOS’ protected media permission UI is different (modal bubble)
> from
> > Clank(info bar), and I haven't checked if it takes the same code path or
not.
> > 
> > +mukai@ for clarification
> 
> +dkrahn who owns the actual code.
> 
> That is true (I don't know why it has to be, and I'm fine to use the same
> permission code though).

To follow up: in ChromeOS the dialog is used in v2 apps too, and probably that's
why.
I have no idea what will happen if a v2 app tries to show an infobar.

> It seems that the ChromeOS code does not invoke permission_context_uma_util.
> That means allowing PROTECTED_MEDIA to OS_CHROMEOS is harmless (also it's
better
> to do that
> in ChromeOS as well, but that should be done in another CL in ChromeOS side I
> guess).
> 
> By the way PROTECTED_MEDIA content type is only defined in OS_ANDROID or
> OS_CHROMEOS,
> so you cannot remove the #if-clause itself, you can just say OS_ANDROID ||
> OS_CHROMEOS.

[Darren Krahn, 2014-12-04 19:02:34]

On 2014/12/04 18:55:32, Jun Mukai wrote:
> On 2014/12/04 18:52:40, Jun Mukai wrote:
> > On 2014/12/04 18:35:35, Kibeom Kim wrote:
> > > Yes right, ChromeOS also has it, so I think we want to remove #if
> > > defined(OS_ANDROID) in permission_context_uma_util.cc . Though, last time
I
> > > heard, ChromeOS’ protected media permission UI is different (modal bubble)
> > from
> > > Clank(info bar), and I haven't checked if it takes the same code path or
> not.
> > > 
> > > +mukai@ for clarification
> > 
> > +dkrahn who owns the actual code.
> > 
> > That is true (I don't know why it has to be, and I'm fine to use the same
> > permission code though).
> 
> To follow up: in ChromeOS the dialog is used in v2 apps too, and probably
that's
> why.
> I have no idea what will happen if a v2 app tries to show an infobar.
> 
> > It seems that the ChromeOS code does not invoke permission_context_uma_util.
> > That means allowing PROTECTED_MEDIA to OS_CHROMEOS is harmless (also it's
> better
> > to do that
> > in ChromeOS as well, but that should be done in another CL in ChromeOS side
I
> > guess).
> > 
> > By the way PROTECTED_MEDIA content type is only defined in OS_ANDROID or
> > OS_CHROMEOS,
> > so you cannot remove the #if-clause itself, you can just say OS_ANDROID ||
> > OS_CHROMEOS.

ChromeOS is actively using the PROTECTED_MEDIA content type; it would be best to
have it work the same way on both platforms.

[Ilya Sherman, 2014-12-04 19:26:04]

histograms.xml lgtm

[timvolodine, 2014-12-12 15:32:07]

thanks for the comments everybody, I've added #if defined(OS_CHROMEOS) to be
consistent with the definition in content_settings_types.h.

[timvolodine, 2014-12-12 15:32:12]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-12-12 15:33:07]

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

[commit-bot: I haz the power, 2014-12-12 16:33:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-12 16:34:15]

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