Make ZIP archiver a component extension.

Make ZIP archiver a component extension.

This CL adds ZIP archiver to the list of component extensions. Currently,
ZIP archiver is in ui/third_party/zip_archiver. This extension was
originally maintained in https://chromium.googlesource.com/apps/unpacker.

By making it a component extension, we can get the following benefits.
(1) Tighter integration into Files app on Chrome OS.
(2) Availability in guest mode.
(3) Automated tests.
(4) Easier development cycles.

ZIP archiver uses Native Client to wrap MiniZip in C++. Since .pexe file,
which is a binary file that contains modules written in C++, is built in
compile time, we need to manually specify the path of the file in
chrome_component_extension_resource_manager.cc.
Note that we use .pexe.js instead of .pexe because .grd file ignores .pexe files.

BUG=607078
TEST=manually tested

Review-Url: https://codereview.chromium.org/2805183002
Cr-Commit-Position: refs/heads/master@{#465947}
Committed: https://chromium.googlesource.com/chromium/src/+/2cab8929074f911590ef5a1da345b5519dfc1c81

[takise, 2017-04-07 02:29:35]

Description was changed from

==========
Make ZIP archiver a component extension.

This CL adds ZIP archiver to the list of component extensions. Currently,
ZIP archiver is in ui/third_party/zip_archiver. This extension was
originally maintained in https://chromium.googlesource.com/apps/unpacker.

By making it a component extension, we can get the following benefits.
(1) Tighter integration into Files app on Chrome OS.
(2) Availability in guest mode.
(3) Automated tests.
(4) Easier development cycles.

ZIP archiver uses Native Client to wrap MiniZip in C++. Since .pexe file,
which is a binary file that contains modules written in C++, is built in
compile time, It cannot be loaded through .grd file. We add this file in
chrome_component_extension_resource_manager.cc.

BUG=607078
TEST=manually tested
==========

to

==========
Make ZIP archiver a component extension.

This CL adds ZIP archiver to the list of component extensions. Currently,
ZIP archiver is in ui/third_party/zip_archiver. This extension was
originally maintained in https://chromium.googlesource.com/apps/unpacker.

By making it a component extension, we can get the following benefits.
(1) Tighter integration into Files app on Chrome OS.
(2) Availability in guest mode.
(3) Automated tests.
(4) Easier development cycles.

ZIP archiver uses Native Client to wrap MiniZip in C++. Since .pexe file,
which is a binary file that contains modules written in C++, is built in
compile time, we need to manually specify the path of the file in
chrome_component_extension_resource_manager.cc.
Note that we use .pexe.js instead of .pexe because .grd file ignores .pexe
files.

BUG=607078
TEST=manually tested
==========

[takise, 2017-04-07 02:32:18]

takise@google.com changed reviewers:
+ benwells@chromium.org

[takise, 2017-04-07 02:32:44]

PTAL. Thank you!

[benwells, 2017-04-07 03:29:36]

On 2017/04/07 02:32:44, takise wrote:
> PTAL. Thank you!

What builds the pexe? Like, is it built by the build system?

Also what automated tests are there for this? You list it as an advantage but
there don't seem to be any.

[takise, 2017-04-07 04:33:07]

On 2017/04/07 03:29:36, benwells wrote:
> On 2017/04/07 02:32:44, takise wrote:
> > PTAL. Thank you!
> 
> What builds the pexe? Like, is it built by the build system?

Actually, we are currently working on the task to build .pexe file in
ui/file_manager/zip_archiver.
The BUILD.gn file to build .pexe has not been landed yet. Now I'm creating the
CL for that.
We are not going to land this patch until the task to build .pexe file is
completed.
For now, we just want you to review the code in chrome/browser/extensions.
Once another task is done, we can tell you the CL and ask again if it's still
okay.

> Also what automated tests are there for this? You list it as an advantage but
> there don't seem to be any.

We also need to rewrite the code in ui/file_manager/zip_archiver/unpacker_test
to run the tests automatically.
We are planning to start working on this once the task to build .pexe file is
completed.

[benwells, 2017-04-07 04:55:43]

On 2017/04/07 04:33:07, takise wrote:
> On 2017/04/07 03:29:36, benwells wrote:
> > On 2017/04/07 02:32:44, takise wrote:
> > > PTAL. Thank you!
> > 
> > What builds the pexe? Like, is it built by the build system?
> 
> Actually, we are currently working on the task to build .pexe file in
> ui/file_manager/zip_archiver.
> The BUILD.gn file to build .pexe has not been landed yet. Now I'm creating the
> CL for that.
> We are not going to land this patch until the task to build .pexe file is
> completed.
> For now, we just want you to review the code in chrome/browser/extensions.
> Once another task is done, we can tell you the CL and ask again if it's still
> okay.

OK - it looks fine but before approving I'd like to see a clean run through the
try system.

> 
> > Also what automated tests are there for this? You list it as an advantage
but
> > there don't seem to be any.
> 
> We also need to rewrite the code in ui/file_manager/zip_archiver/unpacker_test
> to run the tests automatically.
> We are planning to start working on this once the task to build .pexe file is
> completed.

OK, makes sense.

[benwells, 2017-04-07 04:55:45]

On 2017/04/07 04:33:07, takise wrote:
> On 2017/04/07 03:29:36, benwells wrote:
> > On 2017/04/07 02:32:44, takise wrote:
> > > PTAL. Thank you!
> > 
> > What builds the pexe? Like, is it built by the build system?
> 
> Actually, we are currently working on the task to build .pexe file in
> ui/file_manager/zip_archiver.
> The BUILD.gn file to build .pexe has not been landed yet. Now I'm creating the
> CL for that.
> We are not going to land this patch until the task to build .pexe file is
> completed.
> For now, we just want you to review the code in chrome/browser/extensions.
> Once another task is done, we can tell you the CL and ask again if it's still
> okay.

OK - it looks fine but before approving I'd like to see a clean run through the
try system.

> 
> > Also what automated tests are there for this? You list it as an advantage
but
> > there don't seem to be any.
> 
> We also need to rewrite the code in ui/file_manager/zip_archiver/unpacker_test
> to run the tests automatically.
> We are planning to start working on this once the task to build .pexe file is
> completed.

OK, makes sense.

[takise, 2017-04-14 00:34:16]

The CQ bit was checked by takise@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 00:34:35]

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-04-14 00:45:28]

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

[commit-bot: I haz the power, 2017-04-14 00:45:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[takise, 2017-04-14 00:51:12]

The CQ bit was checked by takise@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 00:51: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-04-14 02:10:28]

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

[commit-bot: I haz the power, 2017-04-14 02:10:29]

Dry run: This issue passed the CQ dry run.

[takise, 2017-04-14 02:19:12]

benwells@ Please take a look at the code again because I change the code a bit.

Also, I have confirmed that the code passed try run.

You can see the build files from the following CL.
https://codereview.chromium.org/2808993003/

Test code has not been prepared yet, although I mentioned it as one of the
benefits of this CL. Please ignore it for now.
Thank you! (I would really appreciate it if you could response soon, because
today is my last day at Google:) Sorry to rush you! )

[benwells, 2017-04-14 05:12:32]

https://codereview.chromium.org/2805183002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2805183002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.cc:565: #if defined(OS_CHROMEOS)
why do you need the flag here and in the body of AddZipArchiverExtension?

The pattern to these commands is to check conditional things in the body of the
function, so you should just call AddZipArchiverExtension here, and check the
build and command line in the body.

[takise, 2017-04-14 05:30:03]

> why do you need the flag here and in the body of AddZipArchiverExtension?

Thank you for the comments.
The reason is that we can't compile the following code if OS_CHROMEOS is not
defined.

chromeos::switches::kEnableZipArchiverOnFileManager

This is because we include chromeos/chromeos_switches.h in #define OS_CHROMEOS. 
Should we move #include "chromeos/chromeos_switches.h" outside #define
OS_CHROMEOS? or Is there any better way?

[benwells, 2017-04-14 05:45:16]

lgtm

[takise, 2017-04-14 05:46:03]

The CQ bit was checked by takise@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 05:46: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-04-14 06:32:47]

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

[commit-bot: I haz the power, 2017-04-14 06:32:48]

Dry run: This issue passed the CQ dry run.

[takise, 2017-04-14 06:33:42]

The CQ bit was checked by takise@google.com

[commit-bot: I haz the power, 2017-04-14 06:33:53]

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-04-14 06:43:25]

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

[commit-bot: I haz the power, 2017-04-14 06:43:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yawano, 2017-04-20 01:21:18]

yawano@chromium.org changed reviewers:
+ brettw@chromium.org, yawano@chromium.org

[yawano, 2017-04-20 01:21:18]

brettw@: PTAL whitelist.cc. Thank you!

[yawano, 2017-04-20 01:43:16]

Please wait to review this CL. Thanks.

https://codereview.chromium.org/2805183002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/component_extensions_whitelist/whitelist.cc
(right):

https://codereview.chromium.org/2805183002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/component_extensions_whitelist/whitelist.cc:44:
extension_misc::kZIPUnpackerExtensionId,
IIUC, we need to add ZipArchiverExtensionId here.
ComponentLoader::Add(manifest_resource_id, ...) falls into
ComponentLoader::Add(parsed_manifest, ...) and there is a check by extension id
in the function. As I cannot upload a patch to this CL, I'll download this CL
and re-upload to another CL.

[brettw, 2017-04-20 03:23:17]

https://codereview.chromium.org/2805183002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/component_extensions_whitelist/whitelist.cc
(right):

https://codereview.chromium.org/2805183002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/component_extensions_whitelist/whitelist.cc:44:
extension_misc::kZIPUnpackerExtensionId,
Don't we need this to be done before submitting?

[yawano, 2017-04-20 03:52:24]

This CL was ready for review.
Sorry for going back and forth.

brettw@: PTAL whitelist.cc. Thank you!

https://codereview.chromium.org/2805183002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/component_extensions_whitelist/whitelist.cc
(right):

https://codereview.chromium.org/2805183002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/component_extensions_whitelist/whitelist.cc:44:
extension_misc::kZIPUnpackerExtensionId,
On 2017/04/20 03:23:17, brettw (plz ping after 24h) wrote:
> Don't we need this to be done before submitting?

No, we don't need to do it. ComponentLoader::Add(parsed_manifest, ...) is called
with skip_whitelist=true. In that case, whitelist check for extension id is
passed.

Also if I check other component extensions (e.g. file manager, gallery), they
only have their entries only in resource id list.

[brettw, 2017-04-20 04:39:59]

Oh, I get it, the extension would either use the extension ID or the manifest,
and you moved it from one group to the other.

LGTM

[yawano, 2017-04-20 05:38:20]

On 2017/04/20 04:39:59, brettw (plz ping after 24h) wrote:
> Oh, I get it, the extension would either use the extension ID or the manifest,
> and you moved it from one group to the other.
> 
> LGTM

Thank you!

BTW, what do you mean 'you moved it from one group to the other'. I haven't
moved it.
Zip archiver was in the manifest resource id list from the first patch set.

[yawano, 2017-04-20 07:30:28]

On 2017/04/20 05:38:20, yawano wrote:
> On 2017/04/20 04:39:59, brettw (plz ping after 24h) wrote:
> > Oh, I get it, the extension would either use the extension ID or the
manifest,
> > and you moved it from one group to the other.
> > 
> > LGTM
> 
> Thank you!
> 
> BTW, what do you mean 'you moved it from one group to the other'. I haven't
> moved it.
> Zip archiver was in the manifest resource id list from the first patch set.

On the second thought, I think you meant we moved it from one group to the other
as part of replacing zip unpacker (old one) with zip archiver (new one).

In this patch, the old one isn't yet removed because the new one has to be
tested under a flag. Once the new one is fully ready, we'll remove the old one
from this file in a separate patch.

Let me submit this CL as is for now. Thanks!

[yawano, 2017-04-20 07:30:39]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2017-04-20 07:31:02]

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-04-20 08:25:42]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492673439708750,
"parent_rev": "81695d0da81e63990e210da79b7a65c7dd386140", "commit_rev":
"2cab8929074f911590ef5a1da345b5519dfc1c81"}

[commit-bot: I haz the power, 2017-04-20 08:25:48]

Description was changed from

==========
Make ZIP archiver a component extension.

This CL adds ZIP archiver to the list of component extensions. Currently,
ZIP archiver is in ui/third_party/zip_archiver. This extension was
originally maintained in https://chromium.googlesource.com/apps/unpacker.

By making it a component extension, we can get the following benefits.
(1) Tighter integration into Files app on Chrome OS.
(2) Availability in guest mode.
(3) Automated tests.
(4) Easier development cycles.

ZIP archiver uses Native Client to wrap MiniZip in C++. Since .pexe file,
which is a binary file that contains modules written in C++, is built in
compile time, we need to manually specify the path of the file in
chrome_component_extension_resource_manager.cc.
Note that we use .pexe.js instead of .pexe because .grd file ignores .pexe
files.

BUG=607078
TEST=manually tested
==========

to

==========
Make ZIP archiver a component extension.

This CL adds ZIP archiver to the list of component extensions. Currently,
ZIP archiver is in ui/third_party/zip_archiver. This extension was
originally maintained in https://chromium.googlesource.com/apps/unpacker.

By making it a component extension, we can get the following benefits.
(1) Tighter integration into Files app on Chrome OS.
(2) Availability in guest mode.
(3) Automated tests.
(4) Easier development cycles.

ZIP archiver uses Native Client to wrap MiniZip in C++. Since .pexe file,
which is a binary file that contains modules written in C++, is built in
compile time, we need to manually specify the path of the file in
chrome_component_extension_resource_manager.cc.
Note that we use .pexe.js instead of .pexe because .grd file ignores .pexe
files.

BUG=607078
TEST=manually tested

Review-Url: https://codereview.chromium.org/2805183002
Cr-Commit-Position: refs/heads/master@{#465947}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cab8929074f911590ef5a1da345...
==========

[commit-bot: I haz the power, 2017-04-20 08:25:49]

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