Convert utility process CheckMediaFile IPC to mojo

Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it in WIN || OSX #ifdef
and directly call the real IPC::Send instead. Add TODOs.

BUG=680928
Review-Url: https://codereview.chromium.org/2639503002
Cr-Commit-Position: refs/heads/master@{#445054}
Committed: https://chromium.googlesource.com/chromium/src/+/643d85c8fd7fb89a8aea1af3f9ac0dc2eb7ba89e

[Noel Gordon, 2017-01-17 04:11:03]

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

[commit-bot: I haz the power, 2017-01-17 04:11:09]

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

[Noel Gordon, 2017-01-17 04:14:45]

noel@chromium.org changed reviewers:
+ tibell@chromium.org

[Noel Gordon, 2017-01-17 04:14:46]

Based on https://codereview.chromium.org/2611283002/ and now we have landed
MediaParser .mojom, this becomes a lot simpler.  PTAL.

[commit-bot: I haz the power, 2017-01-17 04:22:11]

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

[commit-bot: I haz the power, 2017-01-17 04:22:12]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Noel Gordon, 2017-01-17 04:25:10]

noel@chromium.org changed reviewers:
+ sammc@google.com

[Noel Gordon, 2017-01-17 04:25:10]

add the usual suspects ;)

[Noel Gordon, 2017-01-17 04:25:56]

Description was changed from

==========
[wip] CheckMediaFile IPC to mojo

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

BUG=680928
==========

to

==========
[wip] CheckMediaFile IPC to mojo

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

BUG=680928
==========

[Noel Gordon, 2017-01-17 04:25:56]

noel@chromium.org changed reviewers:
+ sammc@chromium.org
- sammc@google.com

[Noel Gordon, 2017-01-17 04:29:16]

Oh and is that _IPC::Send unused error_ getting a build bots down again?  Seems
so.

[Noel Gordon, 2017-01-17 04:37:49]

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

[commit-bot: I haz the power, 2017-01-17 04:38:04]

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-01-17 05:16:51]

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

[commit-bot: I haz the power, 2017-01-17 05:16:52]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-01-18 07:09:10]

https://codereview.chromium.org/2639503002/diff/20001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/2639503002/diff/20001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:58: const
int64_t kFileDecodeTimeInMS = 250;
constexpr base::TimeDelta kFileDecodeTime =
base::TimeDelta::FromFromMilliseconds(250);

https://codereview.chromium.org/2639503002/diff/20001/chrome/common/extension...
File chrome/common/extensions/media_parser.mojom (right):

https://codereview.chromium.org/2639503002/diff/20001/chrome/common/extension...
chrome/common/extensions/media_parser.mojom:31: CheckMediaFile(int64
decode_time_ms, mojo.common.mojom.File file)
Use a mojo.common.mojom.TimeDelta for |decode_time_ms| (and rename to
decode_time).

[tibell, 2017-01-18 23:07:06]

lgtm

Same comments as sammc, otherwise lgtm.

[Noel Gordon, 2017-01-19 06:25:23]

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

[commit-bot: I haz the power, 2017-01-19 06:25:51]

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

[Noel Gordon, 2017-01-19 06:28:59]

Description was changed from

==========
[wip] CheckMediaFile IPC to mojo

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

BUG=680928
==========

to

==========
Convert utility process CheckMediaFile IPC to mojo

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

BUG=680928
==========

[Noel Gordon, 2017-01-19 06:33:11]

The IPC::Send() helper routine caused build problems as mentioned above due to
platform #ifdefi-ness.  PITA so I also decided to remove it in this patch, and
call the real Send() directly.

https://codereview.chromium.org/2639503002/diff/20001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/2639503002/diff/20001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:58: const
int64_t kFileDecodeTimeInMS = 250;
On 2017/01/18 07:09:09, Sam McNally wrote:
> constexpr base::TimeDelta kFileDecodeTime =
> base::TimeDelta::FromFromMilliseconds(250);

That worked well. Done.

https://codereview.chromium.org/2639503002/diff/20001/chrome/common/extension...
File chrome/common/extensions/media_parser.mojom (right):

https://codereview.chromium.org/2639503002/diff/20001/chrome/common/extension...
chrome/common/extensions/media_parser.mojom:31: CheckMediaFile(int64
decode_time_ms, mojo.common.mojom.File file)
On 2017/01/18 07:09:09, Sam McNally wrote:
> Use a mojo.common.mojom.TimeDelta for |decode_time_ms| (and rename to
> decode_time).

Done (and done).

[Noel Gordon, 2017-01-19 06:38:34]

On 2017/01/18 23:07:06, tibell wrote:
> lgtm
> 
> Same comments as sammc, otherwise lgtm.

nod, sam's comments done.

[Sam McNally, 2017-01-19 06:44:25]

lgtm

[commit-bot: I haz the power, 2017-01-19 07:14:55]

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

[commit-bot: I haz the power, 2017-01-19 07:14:56]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-01-19 10:14:35]

Description was changed from

==========
Convert utility process CheckMediaFile IPC to mojo

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

BUG=680928
==========

to

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery call site to use mojo for this service.

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

Remove the IPC::Send helper to avoid wrapping it a WIN || OSX #ifdef,
call the real IPC::Send directly instead.

BUG=680928
==========

[Noel Gordon, 2017-01-19 10:20:01]

Description was changed from

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery call site to use mojo for this service.

Covered by browser tests: MediaFileValidatorTest.ValidVideo and
MediaFileValidatorTest.ValidAudio.

Remove the IPC::Send helper to avoid wrapping it a WIN || OSX #ifdef,
call the real IPC::Send directly instead.

BUG=680928
==========

to

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it a WIN || OSX #ifdef,
directly call the real IPC::Send instead.

BUG=680928
==========

[Noel Gordon, 2017-01-19 10:21:08]

noel@chromium.org changed reviewers:
+ dcheng@chromium.org, tommycli@chromium.org

[Noel Gordon, 2017-01-19 10:21:08]

+tommycli for media galleries.
+dcheng for IPC security.

[Noel Gordon, 2017-01-19 10:22:11]

Description was changed from

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it a WIN || OSX #ifdef,
directly call the real IPC::Send instead.

BUG=680928
==========

to

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it in WIN || OSX #ifdef
and directly call the real IPC::Send instead.

BUG=680928
==========

[tommycli, 2017-01-19 16:52:03]

On 2017/01/19 10:21:08, noel gordon wrote:
> +tommycli for media galleries.
> +dcheng for IPC security.

lgtm from media galleries perspective

[Noel Gordon, 2017-01-20 00:50:24]

https://codereview.chromium.org/2639503002/diff/40001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/2639503002/diff/40001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:25: #include
"ipc/ipc_platform_file.h"
We should probably clean up here: let me removed the unused #includes [1].

[1] With IWYU, seems we also need a remove what you don't use RWUDU tool.

[Noel Gordon, 2017-01-20 00:52:24]

Patchset #4 (id:60001) has been deleted

[Noel Gordon, 2017-01-20 00:54:05]

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

[commit-bot: I haz the power, 2017-01-20 00:55:33]

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

[Noel Gordon, 2017-01-20 01:06:16]

On 2017/01/20 00:50:24, noel gordon wrote:
> We should probably clean up here: let me remove the unused #includes [1].
> 
> [1] With IWYU, seems we also need a remove what you don't use RWUDU tool.

Right done that, throwing the result at the bots now.

> tommycli wrote:
> On 2017/01/19 10:21:08, noel gordon wrote:
> > +tommycli for media galleries.
> > +dcheng for IPC security.
> >
> lgtm from media galleries perspective

Thanks.  I added some followup TODO.

[commit-bot: I haz the power, 2017-01-20 01:57:27]

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

[commit-bot: I haz the power, 2017-01-20 01:57:28]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-20 08:34:45]

mojo lgtm

[Noel Gordon, 2017-01-20 10:02:31]

noel@chromium.org changed reviewers:
+ jochen@chromium.org

[Noel Gordon, 2017-01-20 10:02:31]

+jochen for OWNERS.

[Noel Gordon, 2017-01-20 10:06:13]

Description was changed from

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it in WIN || OSX #ifdef
and directly call the real IPC::Send instead.

BUG=680928
==========

to

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it in WIN || OSX #ifdef
and directly call the real IPC::Send instead. Add TODOs.

BUG=680928
==========

[jochen (gone - plz use gerrit), 2017-01-20 13:08:05]

lgtm

[Noel Gordon, 2017-01-20 14:54:58]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-01-20 14:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tibell@chromium.org,
sammc@chromium.org, tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2639503002/#ps80001
(title: "Removed unused #include, add TODO.")

[commit-bot: I haz the power, 2017-01-20 14:55:14]

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-01-20 14:58:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484924098602770,
"parent_rev": "d3bdf95134d7104f119a3db170346c527714ebd2", "commit_rev":
"643d85c8fd7fb89a8aea1af3f9ac0dc2eb7ba89e"}

[commit-bot: I haz the power, 2017-01-20 14:58:54]

Description was changed from

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it in WIN || OSX #ifdef
and directly call the real IPC::Send instead. Add TODOs.

BUG=680928
==========

to

==========
Convert utility process CheckMediaFile IPC to mojo

Add CheckMediaFile() method to the extensions::mojo::MediaParser mojo
interface exposed by the utility process to the browser, update media
gallery caller to use mojo calls for this service. Covered by browser
tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.

Remove the IPC::Send helper to avoid wrapping it in WIN || OSX #ifdef
and directly call the real IPC::Send instead. Add TODOs.

BUG=680928

Review-Url: https://codereview.chromium.org/2639503002
Cr-Commit-Position: refs/heads/master@{#445054}
Committed:
https://chromium.googlesource.com/chromium/src/+/643d85c8fd7fb89a8aea1af3f9ac...
==========

[commit-bot: I haz the power, 2017-01-20 14:58:55]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/643d85c8fd7fb89a8aea1af3f9ac...