Convert utility process zip creator IPC to mojo

Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreatorImpl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
Review-Url: https://codereview.chromium.org/2705613003
Cr-Commit-Position: refs/heads/master@{#453825}
Committed: https://chromium.googlesource.com/chromium/src/+/b8721b0aa68a020f092aede9b18b33389ed24e8f

[Noel Gordon, 2017-02-17 16:53:40]

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

[commit-bot: I haz the power, 2017-02-17 16:54:06]

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-02-17 17:07:48]

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

[commit-bot: I haz the power, 2017-02-17 17:07:49]

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-...)

[Noel Gordon, 2017-02-17 17:38:58]

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

[commit-bot: I haz the power, 2017-02-17 17:39:43]

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-02-17 17:51:35]

Patchset #2 (id:20001) has been deleted

[Noel Gordon, 2017-02-17 17:53:18]

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

[commit-bot: I haz the power, 2017-02-17 17:53:44]

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-02-17 20:23:58]

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

[commit-bot: I haz the power, 2017-02-17 20:23:59]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-18 06:13:46]

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

[commit-bot: I haz the power, 2017-02-18 06:14: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-02-18 07:56:26]

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

[commit-bot: I haz the power, 2017-02-18 07:56:26]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-20 00:43:09]

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

[commit-bot: I haz the power, 2017-02-20 00:43:26]

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-02-20 00:45:41]

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

[commit-bot: I haz the power, 2017-02-20 00:45:42]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Noel Gordon, 2017-02-20 01:18:16]

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

[commit-bot: I haz the power, 2017-02-20 01:18:26]

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-02-20 02:29:12]

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

[commit-bot: I haz the power, 2017-02-20 02:29:13]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-22 07:37:59]

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

[commit-bot: I haz the power, 2017-02-22 07:39:25]

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-02-22 09:39:05]

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

[commit-bot: I haz the power, 2017-02-22 09:39:06]

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

[Noel Gordon, 2017-02-22 11:09:51]

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

[commit-bot: I haz the power, 2017-02-22 11:10:11]

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-02-22 11:10:36]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
that IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

In the chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helper routines. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[Noel Gordon, 2017-02-22 11:10:36]

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

[Noel Gordon, 2017-02-22 11:12:35]

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

[Noel Gordon, 2017-02-22 11:12:36]

PTAL

+ Sam, tibell for mojo
+ Daniel for IPC sec
+ jochen for OWNERS

[Noel Gordon, 2017-02-22 11:13:03]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
that IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

In the chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helper routines. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
that IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

In the chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helper routines. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[commit-bot: I haz the power, 2017-02-22 12:22:44]

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

[commit-bot: I haz the power, 2017-02-22 12:22:47]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-22 16:28:22]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
that IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

In the chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helper routines. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
that IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[Noel Gordon, 2017-02-23 01:58:20]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
that IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[Noel Gordon, 2017-02-23 02:07:04]

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

[commit-bot: I haz the power, 2017-02-23 02:07:47]

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-02-23 02:11:53]

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

[commit-bot: I haz the power, 2017-02-23 02:12:19]

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-02-23 02:13:33]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreator impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[tibell, 2017-02-23 02:15:50]

lgtm

https://codereview.chromium.org/2705613003/diff/120001/chrome/common/zip_file...
File chrome/common/zip_file_creator.mojom (right):

https://codereview.chromium.org/2705613003/diff/120001/chrome/common/zip_file...
chrome/common/zip_file_creator.mojom:15:
CreateZipFile(mojo.common.mojom.FilePath source,
Nit: |source_dir| might be clearer than |source|. It also matches the naming in
the underlying ZipFiles API better.

[Noel Gordon, 2017-02-23 02:39:41]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreator impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreator impl class. Add
missing DISALLOW_COPY_AND_ASSIGN.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[Noel Gordon, 2017-02-23 02:40:13]

https://codereview.chromium.org/2705613003/diff/120001/chrome/common/zip_file...
File chrome/common/zip_file_creator.mojom (right):

https://codereview.chromium.org/2705613003/diff/120001/chrome/common/zip_file...
chrome/common/zip_file_creator.mojom:15:
CreateZipFile(mojo.common.mojom.FilePath source,
On 2017/02/23 02:15:50, tibell wrote:
> Nit: |source_dir| might be clearer than |source|. It also matches the naming
in
> the underlying ZipFiles API better.

Done.

[Sam McNally, 2017-02-23 02:43:47]

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:11: #include
"base/message_loop/message_loop.h"
Remove.

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:79:
utility_process_mojo_client_.reset();
Make this
auto callback = base::ResetAndReturn(&callback_);
utility_process_mojo_client_.reset();
callback.Run(success);

Resetting |utility_process_mojo_client_| could release the last reference to
|this|.

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:81: if (callback_)
|callback|_ should never be null in ReportDone().

[Noel Gordon, 2017-02-23 02:59:48]

New patch uploaded ...

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:11: #include
"base/message_loop/message_loop.h"
On 2017/02/23 02:43:47, Sam McNally wrote:
> Remove.

Nicely spotted.  Done.

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:79:
utility_process_mojo_client_.reset();
On 2017/02/23 02:43:47, Sam McNally wrote:
> Make this
> auto callback = base::ResetAndReturn(&callback_);
> utility_process_mojo_client_.reset();
> callback.Run(success);
> 
> Resetting |utility_process_mojo_client_| could release the last reference to
> |this|.

Nice, done.

https://codereview.chromium.org/2705613003/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:81: if (callback_)
On 2017/02/23 02:43:47, Sam McNally wrote:
> |callback|_ should never be null in ReportDone().

Acknowledged.

[Noel Gordon, 2017-02-23 02:59:51]

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

[commit-bot: I haz the power, 2017-02-23 03:00:29]

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

[Sam McNally, 2017-02-23 03:05:36]

lgtm

[commit-bot: I haz the power, 2017-02-23 04:05:21]

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

[commit-bot: I haz the power, 2017-02-23 04:05:22]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-23 05:41:53]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreator impl class. Add
missing DISALLOW_COPY_AND_ASSIGN.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreator impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[Noel Gordon, 2017-02-23 10:25:30]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreator impl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreatorImpl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

[jochen (gone - plz use gerrit), 2017-02-24 15:57:28]

lgtm

[dcheng, 2017-02-24 19:16:36]

https://codereview.chromium.org/2705613003/diff/200001/chrome/browser/chrome_...
File chrome/browser/chrome_content_utility_manifest_overlay.json (right):

https://codereview.chromium.org/2705613003/diff/200001/chrome/browser/chrome_...
chrome/browser/chrome_content_utility_manifest_overlay.json:11:
"chrome::mojom::ZipFileCreator",
I've been thinking... shouldn't this be in a chromeos specific overlay?

https://codereview.chromium.org/2705613003/diff/200001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2705613003/diff/200001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:693: "zip_file_creator.mojom",
Can we include this in a if (is_chromeos) block?

[Noel Gordon, 2017-02-27 02:00:00]

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

[commit-bot: I haz the power, 2017-02-27 02:00:15]

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-02-27 02:58:08]

https://codereview.chromium.org/2705613003/diff/200001/chrome/browser/chrome_...
File chrome/browser/chrome_content_utility_manifest_overlay.json (right):

https://codereview.chromium.org/2705613003/diff/200001/chrome/browser/chrome_...
chrome/browser/chrome_content_utility_manifest_overlay.json:11:
"chrome::mojom::ZipFileCreator",
On 2017/02/24 19:16:35, dcheng wrote:
> I've been thinking... shouldn't this be in a chromeos specific overlay?

Nope.  Mojo does not have per-os overlays.

And with my mojo developers hat on, I don't think we should add them (nor
per-build-flag overlays either).  Overlays are a developer paint pain, so the
fewer of them, the better imho.

There might be some benefit to your suggestion, which I might be overlooking,
but this is not the right forum to address it ... perhaps send mail to the mojo
list instead.

https://codereview.chromium.org/2705613003/diff/200001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2705613003/diff/200001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:693: "zip_file_creator.mojom",
On 2017/02/24 19:16:35, dcheng wrote:
> Can we include this in a if (is_chromeos) block?

Done.

[commit-bot: I haz the power, 2017-02-27 03:05:38]

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

[commit-bot: I haz the power, 2017-02-27 03:05:39]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2017-02-27 11:27:49]

Ahem: Overlays are a developer _pain point_, so the fewer ...

[dcheng, 2017-02-28 06:22:18]

lgtm

I guess I'll ask about the overlay thing on chromium-mojo. Can you elaborate why
you find it a pain point though? This would probably be useful to know for any
followup discussion.

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:78: auto callback =
base::ResetAndReturn(&callback_);
Nit: save a line by just calling base::ResetAndReturn(&callback_).Run() on line
80

[Noel Gordon, 2017-02-28 10:30:59]

On 2017/02/28 06:22:18, dcheng wrote:
> lgtm
> 
> I guess I'll ask about the overlay thing on chromium-mojo. Can you elaborate
why
> you find it a pain point though? This would probably be useful to know for any
> followup discussion.

You get your mojo defined and compiling "but it is just not working" is the
common exclaim I hear to often from newish-to-mojo developers.

Standard response: "Did you forget to add your mojo to the overlay file?".

Developer: "Overlay file?" and the story of overlay files, and then which one to
use, is retold, as it has been retold many times before.

That's the pain point I see, it matches my experience.  I worry that having even
more overlay files to deal with might make things more difficult for developers
trying to mojo their parts of the system, and especially if there is no mojo
expert nearby to consult with.

[Noel Gordon, 2017-02-28 10:40:12]

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:78: auto callback =
base::ResetAndReturn(&callback_);
On 2017/02/28 06:22:17, dcheng wrote:
> Nit: save a line by just calling base::ResetAndReturn(&callback_).Run() on
line
> 80

Think you might have missed Sam's comment #47, in summary:

"Resetting |utility_process_mojo_client_| could release the last reference to
|this|."

so I rewrote the code, as Sam suggested, to avoid that.

[Noel Gordon, 2017-02-28 10:40:36]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-02-28 10:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tibell@chromium.org,
sammc@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2705613003/#ps220001
(title: "Add if ( is_chromeos ) block to BUILD.gn.")

[commit-bot: I haz the power, 2017-02-28 10:40:49]

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

[dcheng, 2017-02-28 10:56:44]

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:78: auto callback =
base::ResetAndReturn(&callback_);
On 2017/02/28 10:40:12, noel gordon wrote:
> On 2017/02/28 06:22:17, dcheng wrote:
> > Nit: save a line by just calling base::ResetAndReturn(&callback_).Run() on
> line
> > 80
> 
> Think you might have missed Sam's comment #47, in summary:
> 
> "Resetting |utility_process_mojo_client_| could release the last reference to
> |this|."
> 
> so I rewrote the code, as Sam suggested, to avoid that.

Please add a comment that documents this.

[Noel Gordon, 2017-02-28 11:35:46]

The CQ bit was unchecked by noel@chromium.org

[Noel Gordon, 2017-02-28 13:10:53]

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:78: auto callback =
base::ResetAndReturn(&callback_);
On 2017/02/28 10:56:44, dcheng wrote:
> On 2017/02/28 10:40:12, noel gordon wrote:
> > On 2017/02/28 06:22:17, dcheng wrote:
> > > Nit: save a line by just calling base::ResetAndReturn(&callback_).Run() on
> > line
> > > 80
> > 
> > Think you might have missed Sam's comment #47, in summary:
> > 
> > "Resetting |utility_process_mojo_client_| could release the last reference
to
> > |this|."
> > 
> > so I rewrote the code, as Sam suggested, to avoid that.
> 
> Please add a comment that documents this.

Done.

[Noel Gordon, 2017-02-28 13:16:31]

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

[commit-bot: I haz the power, 2017-02-28 13:16:49]

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-02-28 14:41:00]

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

[commit-bot: I haz the power, 2017-02-28 14:41:01]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-01 01:32:39]

still lgtm

[Noel Gordon, 2017-03-01 03:22:47]

https://codereview.chromium.org/2705613003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):

https://codereview.chromium.org/2705613003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/zip_file_creator.cc:80: // and delete
|this|. So save |callback_| before resetting the client.
sammc, tibell: I figure that mojo users might need to add a similar comment in
their code, because of the way ref counted types and base::Bind work behind the
scenes, which (to me) sometimes makes it hard to reason about ref counts.

Was thinking that if mojo always used the pattern here (save the callback first,
before invoking it), then an extra ref on the callback's |this| client (for
refcounted |this|) would be assured.   So any clients that delete / reset their
mojo stuff during the mojo callback, as in this code, would not need the comment
... since |this| would be guaranteed to have at least one ref for the duration
of the mojo callback.

Something to think about, anyho ...

[Noel Gordon, 2017-03-01 03:23:38]

The CQ bit was checked by noel@chromium.org

[Noel Gordon, 2017-03-01 03:23:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
jochen@chromium.org, tibell@chromium.org
Link to the patchset: https://codereview.chromium.org/2705613003/#ps240001
(title: "Add a comment about release references.")

[commit-bot: I haz the power, 2017-03-01 03:24:13]

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-01 03:28:48]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1488338618945840,
"parent_rev": "7f16f2053695f68f84310aaab0c6f2c60b36052c", "commit_rev":
"b8721b0aa68a020f092aede9b18b33389ed24e8f"}

[commit-bot: I haz the power, 2017-03-01 03:29:30]

Description was changed from

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreatorImpl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124
==========

to

==========
Convert utility process zip creator IPC to mojo

Convert the OS_CHROMEOS-only zip creator IPC to use mojo rather
than IPC. Expose the mojo to the browser process using the mojo
utility process policy file. Update the client to call the mojo
service using a mojo utility client, remove all dependencies on
UtilityProcessHostClient. Add a new mojo utility client api:

  utility_process_mojo_client_->set_exposed_directory(dir)

to allow mojo utility clients to expose the target directory in
the utility process sandbox just like UtilityProcessHostClient.
Minor name changes, no abbrvs.

Now in chrome/utility/chrome_content_utility_client.cc continue
the saga of deleting the Send and ReleaseProcessIfNeeded helper
routines: make remaining users call these services directly and
remove the helpers. Add the mojo ZipFileCreatorImpl class.

Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per
https://codereview.chromium.org/2703463005

BUG=597124

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

[commit-bot: I haz the power, 2017-03-01 03:29:33]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/b8721b0aa68a020f092aede9b18b...

[Noel Gordon, 2017-03-02 02:38:31]

On 2017/03/01 03:22:47, noel gordon wrote:
>
https://codereview.chromium.org/2705613003/diff/240001/chrome/browser/chromeo...
> File chrome/browser/chromeos/file_manager/zip_file_creator.cc (right):
> 
>
https://codereview.chromium.org/2705613003/diff/240001/chrome/browser/chromeo...
> chrome/browser/chromeos/file_manager/zip_file_creator.cc:80: // and delete
> |this|. So save |callback_| before resetting the client.
> sammc, tibell: I figure that mojo users might need to add a similar comment in
> their code, because of the way ref counted types and base::Bind work behind
the
> scenes, which (to me) sometimes makes it hard to reason about ref counts.
> 
> Was thinking that if mojo always used the pattern here (save the callback
first,
> before invoking it), then an extra ref on the callback's |this| client (for
> refcounted |this|) would be assured.   So any clients that delete / reset
their
> mojo stuff during the mojo callback, as in this code, would not need the
comment
> ... since |this| would be guaranteed to have at least one ref for the duration
> of the mojo callback.
>
> Something to think about, anyho ...

Did more than just think about it: Sam and I got out the debugger yesterday
afternoon, and it turns out the comment is wrong.   Mojo works fine.  After the
reset(), there is always a reference to |this|, the comment is bunk.    I will
upload a patch to remove it, and write the callback Run() as Daniel suggested.

[Noel Gordon, 2017-03-02 03:53:59]

On 2017/03/02 02:38:31, noel gordon wrote:
> the comment is bunk.    I will
> upload a patch to remove it, and write the callback Run() as Daniel suggested.

https://codereview.chromium.org/2730583002

[Noel Gordon, 2017-03-03 02:00:54]

On 2017/02/27 02:58:08, noel gordon wrote:
>
https://codereview.chromium.org/2705613003/diff/200001/chrome/browser/chrome_...
> File chrome/browser/chrome_content_utility_manifest_overlay.json (right):
> 
>
https://codereview.chromium.org/2705613003/diff/200001/chrome/browser/chrome_...
> chrome/browser/chrome_content_utility_manifest_overlay.json:11:
> "chrome::mojom::ZipFileCreator",
> On 2017/02/24 19:16:35, dcheng wrote:

>
https://codereview.chromium.org/2705613003/diff/200001/chrome/common/BUILD.gn...
> chrome/common/BUILD.gn:693: "zip_file_creator.mojom",
> On 2017/02/24 19:16:35, dcheng wrote:
> > Can we include this in a if (is_chromeos) block?
> 
> Done.

Downside here: I can't see the generated C++ bindings code in codesearch because
of the if (is_chomeos) block.  PITA.