Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644313002:
SafeAudioVideoChecker: use UtilityProcessMojoClient (Closed)
|
Created:
3 years, 11 months ago by Noel Gordon Modified:
3 years, 9 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, qsr+mojo_chromium.org, tommycli, posciak+watch_chromium.org, tzik, viettrungluu+watch_chromium.org, nhiroki, abarth-chromium, Lei Zhang, yzshen+watch_chromium.org, chromium-apps-reviews_chromium.org, darin (slow to review), kinuko+fileapi, Aaron Boodman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafeAudioVideoChecker: use UtilityProcessMojoClient
Remove SafeAudioVideoChecker's public derivation from the
UtilityProcessHost class, and the interfaces it required,
OnMessageReceived, OnProcessCrashed and the state_ member
used to prevent multiple callbacks from them.
Replace all the above with a UtilityProcessMojoClient, to
make the code all mojo, per TODO. Covered by browsertests
MediaFileValidatorTest.{ValidVideo,ValidAudio}.
BUG=680928
Review-Url: https://codereview.chromium.org/2644313002
Cr-Commit-Position: refs/heads/master@{#445557}
Committed: https://chromium.googlesource.com/chromium/src/+/2ce603cd53ff8a412483d343937b8e65bcfc27dc
Patch Set 1 : Depends on another patch, but let's move on. #Patch Set 2 : Dependent patch landed, sync up to ToT. #Patch Set 3 : Comments and other minor clean-up. #
Total comments: 1
Messages
Total messages: 45 (34 generated)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wip] SafeAudioVideoChecker: use UtilityProcessMojoClient BUG=680928 patch from issue 2639503002 at patchset 80001 (http://crrev.com/2639503002#ps80001) ========== to ========== SafeAudioVideoChecker: use UtilityProcessMojoClient BUG=680928 patch from issue 2639503002 at patchset 80001 (http://crrev.com/2639503002#ps80001) ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SafeAudioVideoChecker: use UtilityProcessMojoClient BUG=680928 patch from issue 2639503002 at patchset 80001 (http://crrev.com/2639503002#ps80001) ========== to ========== SafeAudioVideoChecker: use UtilityProcessMojoClient BUG=680928 ==========
Description was changed from
==========
SafeAudioVideoChecker: use UtilityProcessMojoClient
BUG=680928
==========
to
==========
SafeAudioVideoChecker: use UtilityProcessMojoClient
Covered by browser tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.
BUG=680928
==========
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
SafeAudioVideoChecker: use UtilityProcessMojoClient
Covered by browser tests MediaFileValidatorTest.{ValidVideo, ValidAudio}.
BUG=680928
==========
to
==========
SafeAudioVideoChecker: use UtilityProcessMojoClient
Remove SafeAudioVideoChecker's public derivation from the
UtilityProcessHost class, and the interfaces it required,
OnMessageReceived, OnProcessCrashed and the state_ member
used to prevent multiple callbacks from them.
Replace all the above with a UtilityProcessMojoClient, to
make the code all mojo, per TODO. Covered by browsertests
MediaFileValidatorTest.{ValidVideo,ValidAudio}.
BUG=680928
==========
noel@chromium.org changed reviewers: + sammc@chromium.org, tibell@chromium.org, tommycli@chromium.org
lgtm
lgtm
lgtm from media galleries perspective
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by noel@chromium.org
noel@chromium.org changed reviewers: + jochen@chromium.org
+jochen for OWNERS
On 2017/01/23 22:38:04, noel gordon wrote: > +jochen for OWNERS Ignore me: if chrome butler is correct, this can land.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485216905487980,
"parent_rev": "58b74d7b4bbc0f48f47e2fc4f408e79571a650c5", "commit_rev":
"2ce603cd53ff8a412483d343937b8e65bcfc27dc"}
Message was sent while issue was closed.
Description was changed from
==========
SafeAudioVideoChecker: use UtilityProcessMojoClient
Remove SafeAudioVideoChecker's public derivation from the
UtilityProcessHost class, and the interfaces it required,
OnMessageReceived, OnProcessCrashed and the state_ member
used to prevent multiple callbacks from them.
Replace all the above with a UtilityProcessMojoClient, to
make the code all mojo, per TODO. Covered by browsertests
MediaFileValidatorTest.{ValidVideo,ValidAudio}.
BUG=680928
==========
to
==========
SafeAudioVideoChecker: use UtilityProcessMojoClient
Remove SafeAudioVideoChecker's public derivation from the
UtilityProcessHost class, and the interfaces it required,
OnMessageReceived, OnProcessCrashed and the state_ member
used to prevent multiple callbacks from them.
Replace all the above with a UtilityProcessMojoClient, to
make the code all mojo, per TODO. Covered by browsertests
MediaFileValidatorTest.{ValidVideo,ValidAudio}.
BUG=680928
Review-Url: https://codereview.chromium.org/2644313002
Cr-Commit-Position: refs/heads/master@{#445557}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ce603cd53ff8a412483d343937b...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2ce603cd53ff8a412483d343937b...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Thank you for doing this Mojo conversion and simplifying the code. https://codereview.chromium.org/2644313002/diff/80001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right): https://codereview.chromium.org/2644313002/diff/80001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:36: utility_process_mojo_client_.reset( nit for future CLs in general: foo_unique_ptr.reset(new Foo(bar)) -> foo_unique_ptr = base::MakeUnique<Foo>(bar);
Message was sent while issue was closed.
On 2017/03/11 00:44:19, Lei Zhang (super slow) wrote: > Thank you for doing this Mojo conversion and simplifying the code. Thanks for mentioning it. > https://codereview.chromium.org/2644313002/diff/80001/chrome/browser/media_ga... > File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right): > > https://codereview.chromium.org/2644313002/diff/80001/chrome/browser/media_ga... > chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:36: > utility_process_mojo_client_.reset( > nit for future CLs in general: foo_unique_ptr.reset(new Foo(bar)) -> > foo_unique_ptr = base::MakeUnique<Foo>(bar); Yeah, this CL was early-on in our work with the utility_process_mojo_client. As work progressed mojo'n the utility process clients, we changed the client-side pattern we use to [1] 81 utility_process_mojo_client_ = base::MakeUnique< 82 content::UtilityProcessMojoClient<chrome::mojom::SafeArchiveAnalyzer>>( 83 l10n_util::GetStringUTF16( 84 IDS_UTILITY_PROCESS_SAFE_BROWSING_ZIP_FILE_ANALYZER_NAME)); 85 utility_process_mojo_client_->set_error_callback( 86 base::Bind(&SandboxedZipAnalyzer::AnalyzeFileDone, this, Results())); 87 88 utility_process_mojo_client_->Start(); 89 ... viz., make use of base::MakeUnique<Foo> as you noted. [1] https://codereview.chromium.org/2737763002/diff/240001/chrome/browser/safe_br... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
