media: Fix permission request/check for EME on Android.

media: Fix permission request/check for EME on Android.

- Request permission in ProxyDecryptor.
- Check (instead of request) permission in BrowserCdmManager.

BUG=446263, 455271
TEST=Manually tested on Android.

Committed: https://crrev.com/81a1bd56b3d3c59ba1deed9ce072fafc8593dc24
Cr-Commit-Position: refs/heads/master@{#320979}

[xhwang, 2015-03-16 23:32:06]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org

[xhwang, 2015-03-16 23:32:07]

PTAL

[ddorwin, 2015-03-16 23:58:29]

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:511:
DCHECK(task_runner_->RunsTasksOnCurrentThread());
This seems inconsistent with the need for line 514.

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:537:
CreateSessionAndGenerateRequest(render_frame_id, cdm_id, init_data_type,
It seems odd to do this inside CheckPermissionStatus(). How about returning a
bool and calling this function HasPermission() or something like that. You can
then call CreateSessionAndGenerateRequest() from the caller.

Nevermind - The PostTask above prevents returning a value. :(
Still, I think passing an on_permission_status_cb callback would be better.
Either that or we should rename this function. WDYT?

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:559: if (permission_status !=
PERMISSION_STATUS_GRANTED) {
Why do we do this here? Probably because GenerateRequestIfPermitted used to be
called as a callback. Instead, do this in the function that handles the result
of CheckPermissionStatus() (see previous comment).

Does that cause problems for the thread? If so, let's post back to the correct
thread in that function and DCHECK that we are in the right thread in this
function.

[xhwang, 2015-03-17 01:25:22]

Thanks for the comments! PTAL again.

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:511:
DCHECK(task_runner_->RunsTasksOnCurrentThread());
On 2015/03/16 23:58:28, ddorwin wrote:
> This seems inconsistent with the need for line 514.

|task_runner_| is not always on the UI thread. Added a comment to make this more
clear.

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:537:
CreateSessionAndGenerateRequest(render_frame_id, cdm_id, init_data_type,
On 2015/03/16 23:58:28, ddorwin wrote:
> It seems odd to do this inside CheckPermissionStatus(). How about returning a
> bool and calling this function HasPermission() or something like that. You can
> then call CreateSessionAndGenerateRequest() from the caller.
> 
> Nevermind - The PostTask above prevents returning a value. :(
> Still, I think passing an on_permission_status_cb callback would be better.
> Either that or we should rename this function. WDYT?

Good point. Updated.

https://codereview.chromium.org/1013913002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:559: if (permission_status !=
PERMISSION_STATUS_GRANTED) {
On 2015/03/16 23:58:28, ddorwin wrote:
> Why do we do this here? Probably because GenerateRequestIfPermitted used to be
> called as a callback. Instead, do this in the function that handles the result
> of CheckPermissionStatus() (see previous comment).
> 
> Does that cause problems for the thread? If so, let's post back to the correct
> thread in that function and DCHECK that we are in the right thread in this
> function.

Done.

[ddorwin, 2015-03-17 20:10:00]

LGTM % nits

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:372:
CreateSessionAndGenerateRequestIfPermitted(
Note: This code goes away with your other CL, so we can ignore it when
considering design options.

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:556: bool permission_allowed) {
nit: was_...

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.h (right):

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.h:167: media::EmeInitDataType
eme_init_data_type,
nit: "eme_" probably isn't necessary. Also, it's not named this in the .cc file.

[xhwang, 2015-03-17 20:15:50]

comments addressed

[xhwang, 2015-03-17 20:16:02]

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:372:
CreateSessionAndGenerateRequestIfPermitted(
On 2015/03/17 20:09:59, ddorwin wrote:
> Note: This code goes away with your other CL, so we can ignore it when
> considering design options.

Acknowledged.

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:556: bool permission_allowed) {
On 2015/03/17 20:09:59, ddorwin wrote:
> nit: was_...

Done.

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.h (right):

https://codereview.chromium.org/1013913002/diff/20001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.h:167: media::EmeInitDataType
eme_init_data_type,
On 2015/03/17 20:09:59, ddorwin wrote:
> nit: "eme_" probably isn't necessary. Also, it's not named this in the .cc
file.

Done.

[xhwang, 2015-03-17 20:16:07]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-03-17 20:16:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1013913002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-03-17 20:16:23]

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

[commit-bot: I haz the power, 2015-03-17 21:31:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 21:32:21]

Patchset 3 (id:??) landed as
https://crrev.com/81a1bd56b3d3c59ba1deed9ce072fafc8593dc24
Cr-Commit-Position: refs/heads/master@{#320979}