media: Simplify CdmHostFile(s)

media: Simplify CdmHostFile(s)

Today if any files are missing we close files immediately. This resulted in many complicated logic and requires a special command-line switch for testing. 

This CL simplifies the implementation by not treating missing files specially. When a file or a sig file is missing, an invalid file descriptor (e.g. -1 on posix) will be passed to the CDM. The CDM already has logic to deal with this case.

This will also be useful in the future when we explore other ways to verify CDM host files.

BUG=694707
TEST=Updated browser test.

Review-Url: https://codereview.chromium.org/2773283002
Cr-Commit-Position: refs/heads/master@{#461193}
Committed: https://chromium.googlesource.com/chromium/src/+/e23c1b6375d994f06ed745805ba3847e8218cdba

[xhwang, 2017-03-25 07:23:23]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 07:23:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 07:35:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 07:35:01]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[xhwang, 2017-03-25 07:35:32]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 07:35:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 07:48:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 07:48:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[xhwang, 2017-03-26 04:56:17]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 04:56:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 08:29:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 08:29:00]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-03-27 17:47:18]

Description was changed from

==========
media: Simplify CdmHostFile(s)

BUG=
TEST=
==========

to

==========
media: Simplify CdmHostFile(s)

Today if any files are missing we close files immediately. This resulted in many
complicated logic and requires a special command-line switch for testing. This
CL simplifies the implementation by not treating missing files specially. This
will also be useful in the future when we explore other ways to verify CDM host
files.

BUG=694707
TEST=Updated browser test.
==========

[xhwang, 2017-03-27 17:47:19]

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

[xhwang, 2017-03-27 17:47:58]

hmchen: PTAL

[xhwang, 2017-03-30 23:58:56]

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

[xhwang, 2017-03-30 23:58:57]

hmchen: kindly ping!
kerrnel: Please also take a look!

[Greg K, 2017-03-31 14:49:55]

On 2017/03/30 23:58:57, xhwang_slow wrote:
> hmchen: kindly ping!
> kerrnel: Please also take a look!

I like the simplifications, but you can update the commit message to explain
what the new logic/policy is for dealing with missing files?

[xhwang, 2017-03-31 16:26:32]

Description was changed from

==========
media: Simplify CdmHostFile(s)

Today if any files are missing we close files immediately. This resulted in many
complicated logic and requires a special command-line switch for testing. This
CL simplifies the implementation by not treating missing files specially. This
will also be useful in the future when we explore other ways to verify CDM host
files.

BUG=694707
TEST=Updated browser test.
==========

to

==========
media: Simplify CdmHostFile(s)

Today if any files are missing we close files immediately. This resulted in many
complicated logic and requires a special command-line switch for testing. 

This CL simplifies the implementation by not treating missing files specially.
When a file or a sig file is missing, an invalid file descriptor (e.g. -1 on
posix) will be passed to the CDM. The CDM already has logic to deal with this
case.

This will also be useful in the future when we explore other ways to verify CDM
host files.

BUG=694707
TEST=Updated browser test.
==========

[xhwang, 2017-03-31 16:26:51]

On 2017/03/31 14:49:55, Greg K wrote:
> On 2017/03/30 23:58:57, xhwang_slow wrote:
> > hmchen: kindly ping!
> > kerrnel: Please also take a look!
> 
> I like the simplifications, but you can update the commit message to explain
> what the new logic/policy is for dealing with missing files?

kerrnel: Done. PTAL again!

[Haoming Chen, 2017-03-31 16:46:54]

LGTM % nit

https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:280: if (num_files < 3) {
nit: kNumRequiredHostFile or kMinimumNumHostFile?

https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:309: if (num_opened_files <
2) {
nit: kNumCdmFiles?

[Greg K, 2017-03-31 17:22:31]

On 2017/03/31 16:46:54, hmchen wrote:
> LGTM % nit
> 
>
https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
> File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):
> 
>
https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
> media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:280: if (num_files < 3) {
> nit: kNumRequiredHostFile or kMinimumNumHostFile?
> 
>
https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
> media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:309: if (num_opened_files
<
> 2) {
> nit: kNumCdmFiles?

LGTM

[xhwang, 2017-03-31 18:05:19]

comments addressed

[xhwang, 2017-03-31 18:05:37]

https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:280: if (num_files < 3) {
On 2017/03/31 16:46:53, hmchen wrote:
> nit: kNumRequiredHostFile or kMinimumNumHostFile?

Done.

https://codereview.chromium.org/2773283002/diff/40001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:309: if (num_opened_files <
2) {
On 2017/03/31 16:46:53, hmchen wrote:
> nit: kNumCdmFiles?

Done.

[xhwang, 2017-03-31 18:07:07]

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

[xhwang, 2017-03-31 18:07:07]

alexmos@chromium.org: Please OWNERS review trivial changes in 

content/browser/ppapi_plugin_process_host.cc
content/browser/zygote_host/zygote_communication_linux.cc

[xhwang, 2017-03-31 18:07:15]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 18:07:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-03-31 18:13:30]

LGTM

[xhwang, 2017-03-31 18:51:49]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2017-03-31 18:51:50]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-03-31 18:51:51]

The patchset sent to the CQ was uploaded after l-g-t-m from hmchen@chromium.org,
kerrnel@chromium.org
Link to the patchset: https://codereview.chromium.org/2773283002/#ps60001
(title: "comments addressed")

[commit-bot: I haz the power, 2017-03-31 18:52:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 19:59:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490986310995780,
"parent_rev": "863e4742a37cc39631f68b80408d1f32b408ef54", "commit_rev":
"e23c1b6375d994f06ed745805ba3847e8218cdba"}

[commit-bot: I haz the power, 2017-03-31 20:00:03]

Description was changed from

==========
media: Simplify CdmHostFile(s)

Today if any files are missing we close files immediately. This resulted in many
complicated logic and requires a special command-line switch for testing. 

This CL simplifies the implementation by not treating missing files specially.
When a file or a sig file is missing, an invalid file descriptor (e.g. -1 on
posix) will be passed to the CDM. The CDM already has logic to deal with this
case.

This will also be useful in the future when we explore other ways to verify CDM
host files.

BUG=694707
TEST=Updated browser test.
==========

to

==========
media: Simplify CdmHostFile(s)

Today if any files are missing we close files immediately. This resulted in many
complicated logic and requires a special command-line switch for testing. 

This CL simplifies the implementation by not treating missing files specially.
When a file or a sig file is missing, an invalid file descriptor (e.g. -1 on
posix) will be passed to the CDM. The CDM already has logic to deal with this
case.

This will also be useful in the future when we explore other ways to verify CDM
host files.

BUG=694707
TEST=Updated browser test.

Review-Url: https://codereview.chromium.org/2773283002
Cr-Commit-Position: refs/heads/master@{#461193}
Committed:
https://chromium.googlesource.com/chromium/src/+/e23c1b6375d994f06ed745805ba3...
==========

[commit-bot: I haz the power, 2017-03-31 20:00:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e23c1b6375d994f06ed745805ba3...

[xhwang, 2017-04-05 22:24:42]

A revert of this CL (patchset #4 id:60001) has been created in
https://chromiumcodereview.appspot.com/2802853002/ by xhwang@chromium.org.

The reason for reverting is: This issue is in the blame list of issue 708410.
Though I don't really see how this could cause that failure, we decided to
revert this for now to rule out possibilities. If we confirmed this is not the
cause I'll reland it.

BUG=708410.