Extract requestMediaKeySystemAccess() algorithm.

Extract requestMediaKeySystemAccess() algorithm.

This separates the Chromium side of requestMediaKeySystemAccess() into the Blink interface implementation (WebEncryptedMediaClientImpl), which also handles the UMAs, and the core algorithm (KeySystemConfigSelector). This should make it easy to test the core algorithm.

BUG=468912
Committed: https://crrev.com/ac2d3244accf7e64f6507ca9ae972adc63976a5f
Cr-Commit-Position: refs/heads/master@{#323983}

[sandersd (OOO until July 31), 2015-04-02 20:28:33]

sandersd@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[sandersd (OOO until July 31), 2015-04-02 20:28:33]

We'll want to be careful to merge in any changes that are made in
WebEncryptedMediaClient while this CL is pending. I've merged John's changes
from yesterday, and I believe Xiaohan's CdmFactory CL won't conflict.

[sandersd (OOO until July 31), 2015-04-02 20:37:50]

https://codereview.chromium.org/1052273004/diff/1/media/blink/webencryptedmed...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/1052273004/diff/1/media/blink/webencryptedmed...
media/blink/webencryptedmediaclient_impl.cc:84:
key_system_config_selector_(&KeySystems::GetInstance(), media_permission),
This makes me think that GetInstance() should return a KeySystems* instead of a
KeySystems&, but it's equally reasonable to store a ref in
KeySystemConfigSelector. WDYT?

[ddorwin, 2015-04-03 22:09:18]

First pass (did not do a detailed review in all files).

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:58: ConfigState(const
SelectionRequest* request)
It seems wrong to pass the entire request object here, especially since (I
think) ConfigState is used for a single config and the request contains
multiple. Passing individual bools was better.

If left like this, it would need explicit.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:193: scoped_ptr<SelectionRequest>
request(new SelectionRequest());
nit: It seems wrong to create an object to check the string. Normally we would
create when we no we'll succeed. The compiler can probably avoid the copy.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:206: OnSelectConfig(request.Pass());
nit: "On" doesn't seem appropriate, even if this was posted.

The same old naming debate: Do..., ...Internal, etc.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:232: ConfigState
config_state(request.get());
Why is ConfigState passed in? It's not used outside GetSupportedConfiguration().

For testing?

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:305: //    distinctiveIdentifier
attribute from the following list:
regression in use of "member"? Below as well.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:540: bool
KeySystemConfigSelector::GetSupportedCapabilities(
It might have been better to leave the order to simplify reviewing. Then move it
separately if you want to.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webcontentd...
File media/blink/webcontentdecryptionmoduleaccess_impl.cc (right):

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webcontentd...
media/blink/webcontentdecryptionmoduleaccess_impl.cc:12: #include
"third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h"
Shouldn't this be in the header? It's used there and is not a reference or
pointer.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webencrypte...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.cc:134: // TODO(sandersd): Avoid doing
ASCII conversion more than once.
+1

Is there a reason the Selector does not accept std::string?

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.cc:139: // Return a singleton so that
UMA reports will be once-per-frame.
per frame or per process?

[ddorwin, 2015-04-03 22:36:46]

https://codereview.chromium.org/1052273004/diff/1/media/blink/webencryptedmed...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/1052273004/diff/1/media/blink/webencryptedmed...
media/blink/webencryptedmediaclient_impl.cc:84:
key_system_config_selector_(&KeySystems::GetInstance(), media_permission),
On 2015/04/02 20:37:50, sandersd wrote:
> This makes me think that GetInstance() should return a KeySystems* instead of
a
> KeySystems&, but it's equally reasonable to store a ref in
> KeySystemConfigSelector. WDYT?

We don't usually store refs to things as members, so this looks correct.

However, I don't know why or whether GetInstance() returns a ref (perhaps
related to or an artifact of the lazy instance pattern or mime_util code
KeySystems was originally adapted from). I'd do whatever seems to be common for
GetInstance(). A quick search seems to indicate returning a pointer is more
common.

[sandersd (OOO until July 31), 2015-04-03 22:44:58]

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:58: ConfigState(const
SelectionRequest* request)
On 2015/04/03 22:09:17, ddorwin wrote:
> It seems wrong to pass the entire request object here, especially since (I
> think) ConfigState is used for a single config and the request contains
> multiple. Passing individual bools was better.
> 
> If left like this, it would need explicit.

Done.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:193: scoped_ptr<SelectionRequest>
request(new SelectionRequest());
On 2015/04/03 22:09:17, ddorwin wrote:
> nit: It seems wrong to create an object to check the string. Normally we would
> create when we no we'll succeed. The compiler can probably avoid the copy.

Done.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:206: OnSelectConfig(request.Pass());
On 2015/04/03 22:09:17, ddorwin wrote:
> nit: "On" doesn't seem appropriate, even if this was posted.
> 
> The same old naming debate: Do..., ...Internal, etc.

Done.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:232: ConfigState
config_state(request.get());
On 2015/04/03 22:09:17, ddorwin wrote:
> Why is ConfigState passed in? It's not used outside
GetSupportedConfiguration().
> 
> For testing?

Two reasons:
  1) It's clearer than passing two bools.
  2) In the future we will want to pull some bits out of the config state (eg.
for Android SetSecurityLevel()).

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:305: //    distinctiveIdentifier
attribute from the following list:
On 2015/04/03 22:09:18, ddorwin wrote:
> regression in use of "member"? Below as well.

Done.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:540: bool
KeySystemConfigSelector::GetSupportedCapabilities(
On 2015/04/03 22:09:17, ddorwin wrote:
> It might have been better to leave the order to simplify reviewing. Then move
it
> separately if you want to.

Done.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webcontentd...
File media/blink/webcontentdecryptionmoduleaccess_impl.cc (right):

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webcontentd...
media/blink/webcontentdecryptionmoduleaccess_impl.cc:12: #include
"third_party/WebKit/public/platform/WebContentDecryptionModuleResult.h"
On 2015/04/03 22:09:18, ddorwin wrote:
> Shouldn't this be in the header? It's used there and is not a reference or
> pointer.

Done.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webencrypte...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.cc:134: // TODO(sandersd): Avoid doing
ASCII conversion more than once.
On 2015/04/03 22:09:18, ddorwin wrote:
> +1
> 
> Is there a reason the Selector does not accept std::string?

Only because there was nowhere to store the converted string, so doing it this
way removed duplicate code.

https://codereview.chromium.org/1052273004/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.cc:139: // Return a singleton so that
UMA reports will be once-per-frame.
On 2015/04/03 22:09:18, ddorwin wrote:
> per frame or per process?

Done.

[sandersd (OOO until July 31), 2015-04-03 22:46:34]

Patchset #3 (id:40001) has been deleted

[jrummell, 2015-04-06 21:26:42]

lgtm

[sandersd (OOO until July 31), 2015-04-06 22:30:01]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2015-04-06 22:30:42]

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

[commit-bot: I haz the power, 2015-04-07 00:11:37]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-07 00:12:39]

Patchset 6 (id:??) landed as
https://crrev.com/ac2d3244accf7e64f6507ca9ae972adc63976a5f
Cr-Commit-Position: refs/heads/master@{#323983}